fix(pkg,gcp): logging level race, MaskToken short-input leak, registry-lock I/O, GetCredentials RPC

[cristim]

Closes #1054

Groups six code-review findings in the shared pkg/ module and the GCP provider
that no in-flight PR touches, two of them High severity.

Findings fixed

ID	Sev	Fix
10-H4	High	Registry.GetProvider/GetProviderWithConfig/GetAllProviders snapshot the factory under r.mu and invoke it after releasing the lock, so the GCP factory's network I/O no longer blocks every registry reader/writer.
10-H3	High	gcp.GetCredentials reports the credential source from local inspection only (env var / well-known gcloud ADC file / configured project) with no GetProject RPC; ValidateCredentials stays the explicit validity check.
10-L5	Low	pkg/logging level is now an atomic.Int32, fixing the data race between SetLevel/SetLevelValue and the per-call level reads under the concurrent fan-out.
10-L6	Low	MaskToken fully redacts short (<=8 char) inputs to (redacted) instead of echoing them verbatim.
10-N4	Nit	Documented the type-level (field-insensitive) errors.Is matching contract on the package and each Is method.
06-N2	Nit	RedactedDSN derives from a single dsn() formatter shared with DSN, so the two can't drift.

Regression tests

• TestRegistry_FactoryRunsOutsideLock (provider) — a factory that takes the
  write lock would deadlock if the read lock were still held; a 5s timeout makes
  the pre-fix hang fail fast. Covers all three registry methods.
• TestSetLevel_NoDataRace (logging) — concurrent writers + readers; fails under
  -race on the pre-fix plain-int field.
• TestMaskToken_EmptyAndShort updated to assert short inputs are fully redacted.
• GCP GetCredentials tests assert the reported source per scenario and that the
  projects client is never opened (no RPC).
• Is is field-insensitive subtest locks the documented errors.Is contract.
• TestRedactedDSN_SharesLayoutWithDSN asserts the redacted DSN equals the real
  DSN with only the password swapped.

Verification

• pkg/ module (its own go.mod): go build ./..., go vet, go test -race ./...
  — 434 tests pass, no races.
• providers/gcp module (its own go.mod): go build ./..., go vet ./...,
  go test ./... — 230 tests pass.
• Root module: go test ./internal/database/... — 173 tests pass.

Scope note

providers/gcp/services/* is owned by PR #1047 (fix/gcp-recs-count-pricing).
This PR touches only providers/gcp/provider.go (not in #1047's file set), so
there is no overlap. pkg/* and internal/database/config.go are untouched by
any in-flight PR.

Summary by CodeRabbit

• New Features

  • GCP credential detection now runs locally and reports precise credential sources

• Bug Fixes

  • Provider registry calls no longer hold locks while invoking factories (avoids potential deadlocks)
  • Logger level operations are now thread-safe for concurrent access

• Security Improvements

  • Connection string redaction uses a single consistent layout so redacted DSNs match real DSNs
  • Token masking is stricter—tokens ≤8 chars are fully redacted

• Documentation

  • Clarified error type-matching behavior in error handling docs

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4bb131aa-2008-4e2f-a373-300bc8b34867

📥 Commits

Reviewing files that changed from the base of the PR and between b7f2adb and c72807b.

📒 Files selected for processing (2)

• providers/gcp/provider.go
• providers/gcp/provider_test.go

---

📝 Walkthrough

Walkthrough

This PR fixes six correctness issues: atomic logger-level storage and race test, registry lock release before provider factory calls, short-token masking tightened, GCP credential-source detection via local inspection (no RPC), error Is documentation clarified, and centralized DSN formatting with a redaction regression test.

Changes

Concurrency, Security, & Documentation Fixes

Layer / File(s)	Summary
Atomic log level storage for concurrent access pkg/logging/logger.go, pkg/logging/logger_test.go	Logger level field changed from non-atomic Level to atomic.Int32 with internal getLevel/setLevel helpers. All logging methods and package-level level accessors now use atomic operations to prevent data races when SetLevel is called concurrently. New regression test TestSetLevel_NoDataRace verifies no race detector warnings under concurrent level updates and log emissions.
Registry lock release before factory invocation pkg/provider/registry.go, pkg/provider/registry_test.go	GetProvider, GetProviderWithConfig, and GetAllProviders now snapshot factories under read lock, release the lock, and invoke factories outside the critical section. Prevents blocking registry readers and writers during arbitrary network I/O inside factories. New test TestRegistry_FactoryRunsOutsideLock verifies no deadlock when factory performs registry write operations.
Short token masking tightening pkg/common/tokens.go, pkg/common/tokens_test.go	MaskToken now fully redacts tokens of 8 characters or fewer as "(redacted)" instead of echoing them unchanged, preventing accidental secret leakage in logs. Test updated to assert short secrets do not appear in masked output.
GCP credential source detection via local introspection providers/gcp/provider.go, providers/gcp/provider_test.go	GetCredentials now determines credential source (env variable, ADC file, or metadata fallback) through local environment/filesystem inspection instead of issuing network RPC. Adds detectCredentialSource helper to classify source and adcWellKnownFileExists to check ADC file presence. Tests refactored with clearGCPCredEnv helper to deterministically control credential environment and assert specific source values without invoking projects client mock.
Error type-matching documentation pkg/errors/errors.go, pkg/errors/errors_test.go	Package-level documentation and individual error type Is method docstrings clarified to state that errors.Is matches purely on error type in a field-insensitive manner, enabling use as type-level sentinels. Test added to document and assert field-insensitive matching contract with NotFoundError.
DSN formatting centralization internal/database/config.go, internal/database/coverage_extra_test.go	New unexported Config.dsn(password string) helper centralizes PostgreSQL connection string formatting as single source of truth. Public DSN(passwordOverride string) and RedactedDSN() methods now delegate to the helper, eliminating duplicate format logic. New test TestRedactedDSN_SharesLayoutWithDSN asserts redacted DSN layout matches real DSN except for password masking.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A bunny hops through code at night,
Quietly swapping wrongs for right,
Locks now free and secrets sealed,
Atomic beats keep logs well-heeled,
DSNs tidy, docs made bright—hooray, goodnight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes all six main fixes grouped in the PR: logging level data race, MaskToken short-input leak, registry-lock I/O contention, and GetCredentials RPC issues.
Linked Issues check	✅ Passed	The PR successfully implements all coding requirements from issue #1054: registry factory locking [10-H4], GetCredentials local-only source detection [10-H3], atomic logging level [10-L5], MaskToken short redaction [10-L6], errors.Is documentation [10-N4], and shared DSN formatter [06-N2].
Out of Scope Changes check	✅ Passed	All changes are directly scoped to addressing the six findings listed in issue #1054; no extraneous modifications to unrelated areas were introduced.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/pkg-shared-correctness

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@providers/gcp/provider.go`:
- Around line 280-283: The detectCredentialSource method incorrectly treats an
empty GOOGLE_APPLICATION_CREDENTIALS as a file because it only uses
os.LookupEnv; change the check in detectCredentialSource to read the env value
and ensure it's non-empty (e.g., val :=
os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"); if val != "" { return
provider.CredentialSourceFile, true }) so empty strings are ignored, keeping the
same non-empty style used for CLOUDSDK_CONFIG and APPDATA.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3c4ded47-c9ed-423a-afb6-607305829a81

📥 Commits

Reviewing files that changed from the base of the PR and between cbdc4be and b7f2adb.

📒 Files selected for processing (12)

• internal/database/config.go
• internal/database/coverage_extra_test.go
• pkg/common/tokens.go
• pkg/common/tokens_test.go
• pkg/errors/errors.go
• pkg/errors/errors_test.go
• pkg/logging/logger.go
• pkg/logging/logger_test.go
• pkg/provider/registry.go
• pkg/provider/registry_test.go
• providers/gcp/provider.go
• providers/gcp/provider_test.go

[cristim]

CR finding addressed

Finding (CodeRabbit, 2026-06-07): Empty GOOGLE_APPLICATION_CREDENTIALS incorrectly detected as file credential source.

Root cause: os.LookupEnv returns ok=true when the env var is set to an empty string. detectCredentialSource in providers/gcp/provider.go used this for the GOOGLE_APPLICATION_CREDENTIALS check, inconsistently with the non-empty checks used for CLOUDSDK_CONFIG and APPDATA.

Fix: Replaced os.LookupEnv("GOOGLE_APPLICATION_CREDENTIALS") with os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") != "" so an empty value is treated identically to an unset variable.

Regression test: Added TestGCPProvider_GetCredentials_EmptyEnvVarNotFile which sets GOOGLE_APPLICATION_CREDENTIALS to an empty string and asserts CredentialSourceADC (not CredentialSourceFile) is returned.

Commit: c72807b

@coderabbitai review

[coderabbitai]

@cristim: Good catch and clean fix. The os.LookupEnv vs os.Getenv(...) != "" distinction is a common Go footgun — os.LookupEnv is the right tool only when you need to distinguish "unset" from "set-to-empty", which is not the case here. Replacing it with os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") != "" correctly aligns the GOOGLE_APPLICATION_CREDENTIALS check with the CLOUDSDK_CONFIG/APPDATA guards already in detectCredentialSource, and the new TestGCPProvider_GetCredentials_EmptyEnvVarNotFile test pins the corrected behaviour. Triggering the review.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T07:04:28.213Z
Learning: In LeanerCloud/CUDly, PR `#907` (`feat(auth): group-membership-only authorization, remove roles, require >=1 group`) landed on `feat/multicloud-web-frontend` and removed `Session.Role` and `User.Role` entirely. Any `Principal` struct must use `GroupIDs []string` (or similar group-based field) instead of `Role string`. The `*auth.User` type no longer exposes `GetRole()`, `GetID()`, or `GetEmail()` accessors via interface assertion — using those in a type assertion causes a silent always-false fail-closed regression at runtime. The group-membership model also requires >=1 group for a valid user session; zero-groups users must be rejected at the authentication layer. Follow-up design issue is `#1010`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-03T16:07:51.732Z
Learning: In the LeanerCloud/CUDly repository, approximately 50% docstring coverage is a pre-existing project-wide baseline. It should not be flagged as an actionable issue in individual PR reviews, as addressing it is out of scope for focused/surgical fixes.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.