perf(startup): fetch distinct credential sources concurrently

[andybons]

gatekeeper fetched each distinct credential source one after another before it opened the listener. The boxes companion config holds 14 network sources (9 gcp-secretmanager, 3 gcp-service-account, 2 github-app); on production a container logged its listener 1.5 s after it started, so a per-box companion was Ready 3.5 to 4 s after its Deployment was created, and boxes waits for that Ready before it personalizes a golden-adopted box. loadCredentials now builds sources in config order, fetches the distinct ones concurrently (at most 16 in flight, 10 s each as before), and applies them in config order. Every credential is still loaded before the listener accepts a connection; dedup of identical sources, one refresh goroutine per shared source, auth scheme handling, and the no-credential-in-errors rule are unchanged; the first failing credential in config order is the reported error. Tests: TestLoadCredentials_FetchesDistinctSourcesConcurrently (12 sources that each wait for all 12 to be in flight) fails on the serial code with 'only 1 fetches in flight, want 12'; TestLoadCredentials_FetchFailureNamesTheFirstFailingCredentialInConfigOrder pins the error order; go test -race ./... passes. CHANGELOG has a v0.25.2 entry.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 18s —— View job

---

Review of #78: concurrent credential fetch

I found no blocking issues.

• Concurrency: Each goroutine writes only its own pendingSource. wg.Wait() orders those writes before the apply loop reads them, so there is no data race. The slots semaphore caps in-flight fetches at 16. Acquiring a slot before go is correct, and the deferred release can't deadlock. The per-fetch 10s timeout is unchanged.
• Ordering and errors: The apply loop runs in config order and returns the first failing entry's error. The error wraps only Fetch's error, not the value, so the no-credential-in-errors rule still holds. Dedup keys on the same SourceConfig, with first marking the single closer registration. refreshOrder makes pendingRefreshes deterministic, where it used to follow map iteration order.
• Behavior change, benign: Previously a fetch failure stopped later sources from being built. Now every source is built and fetched before any error is reported, so a bad credential late in the file costs a few extra network calls on a startup that was already failing.
• Resource handling, benign: Closers are now registered only in the apply phase. If a later config entry fails to build, or any fetch fails, built sources that implement io.Closer are not registered. New returns nil on this error, so Stop never runs. The old code leaked the same way for any source after the failure, and the process exits on this path. I wouldn't fold a fix into this PR.
• Tests: The tests look right. The 12-way barrier test pins concurrency, and the second test pins error order.

I read the diff only and did not run go test -race.

[Copilot]

🟡 Changes recommended

Concurrent loading can misorder errors and leak constructed credential-source resources on failure.

3 open findings

• Preserve config-order error selection across deferred fetches · New
• Close constructed sources when concurrent startup fails · New
• Test and enforce the 16-source concurrent fetch limit · New

What changed in this PR

Improves startup by concurrently fetching distinct credential sources while preserving ordered application.

Changes:

• Adds bounded concurrency for credential fetching.
• Adds concurrency and error-order tests.
• Documents v0.25.2 behavior.

File	Description
gatekeeper.go	Implements concurrent credential fetching.
gatekeeper_test.go	Tests concurrency and fetch-error ordering.
CHANGELOG.md	Adds the v0.25.2 release entry.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 15s —— View job

---

Review of #78: concurrent credential fetch

I read the diff by eye and didn't run the tests.

Copilot's findings against the current head (43abedd)

• Closing built sources on failure: the defer in loadCredentials closes every source in unique when err != nil. This is already handled in 43abedd, and New returns no Server on failure, so there's no double close through s.closers.
• 16-source bound: the slots channel enforces it. 43abedd says it adds a test for the bound. I didn't read or run that test.
• Error ordering across build and fetch: this one is real but minor.
  • Build and validation errors for any credential now come before fetch errors for earlier ones. If credential 1 would fail to fetch and credential 2 has an invalid source, the old serial code reported credential 1. The new code reports credential 2.
  • The code comment documents this ("Build errors come before fetch errors"). It only changes which error you see when two things are wrong at once, and startup fails either way.
  • The PR body says "the first failing credential in config order is the reported error". That is only true among fetch errors, so I'd narrow the wording.

Other checks (no issues found)

• The goroutines write ps.val and ps.err on distinct structs, and wg.Wait() orders those writes before the reads. There is no data race.
• Refresh registration order is deterministic now (refreshOrder). Before, it was map iteration order.
• The error message carries only the host and the fetch error. No credential value is logged or returned.
• A failure in one fetch doesn't cancel the others, so a bad config can still wait up to 10 s. That matches the old worst case per source and is acceptable.

No blocking issues. I'd suggest narrowing the PR description wording on error order.

[Copilot]

🔵 Needs a closer look

Failed startup still leaks sources and resolvers created for earlier Postgres credentials.

0 open findings

3 resolved since last review

• Close constructed sources when concurrent startup fails
• Preserve config-order error selection across deferred fetches
• Test and enforce the 16-source concurrent fetch limit

Previously missed (1)

In code that hasn't changed since last review

Close Postgres and Neon resources when New initialization fails

gatekeeper.go:539

This cleanup omits closers created by loadPostgresCredential. For example, a static Postgres source is appended to s.closers before Fetch; if that fetch fails—or if a later HTTP fetch fails—the deferred cleanup only walks unique, so New returns no server while the Postgres source (and a Neon resolver, when applicable) remains open. Track all closers acquired during this call and close them on error, while avoiding double-closing HTTP sources whose ownership was already transferred to s.closers.

🧠 Review effort: Balanced

[claude]

Claude finished @andybons's task in 18s —— View job

---

Review of #78: concurrent credential fetch (latest push)

I found no blocking issues. I read the diff and did not run the tests.

• Concurrency: Each goroutine writes only its own pendingSource, and wg.Wait() orders those writes before the apply loop reads them. go.mod is go 1.25.5, so the ps capture in the goroutine is per-iteration and safe. The slots semaphore caps fetches at 16.
• Cleanup on failure (Copilot's remaining finding): The deferred cleanup in loadCredentials appears to cover it.
  • It closes every unique source, and also s.closers[closersBase:], which holds the Postgres sources and Neon resolvers appended by loadPostgresCredential.
  • Header sources join s.closers only after the apply loop succeeds, so nothing is closed twice.
  • The truncation of s.closers also keeps a later Stop from touching closed resources.
  • This is a read of the diff. Nothing in the s.closers path could be double-closed that I could see.
• Error order: Build errors come first, then the first fetch failure in config order. The comment and CHANGELOG state this. The error wraps only Fetch's error, so the no-credential-in-errors rule still holds.
• Determinism: refreshOrder makes pendingRefreshes follow config order instead of map iteration order.
• Minor, not blocking: a panic inside a Fetch goroutine would crash the process, the same as before, since the old serial fetch panicked on the main goroutine. No change is needed.

[Copilot]

🟢 Approval recommended

The implementation is well-bounded and comprehensively tested; only a minor test-comment correction remains.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Correct comment describing failAfterSource failure modes

gatekeeper_test.go:3614

This comment refers to failNow, but no such helper exists; the immediate-failure behavior is the nil-release case of failAfterSource. Reword it so the test helper's two modes are accurately documented.

🧠 Review effort: Balanced