[IAmJSD]

Root cause

UpdateGlobalConfigAtomic runs its apply closure inside a transaction that holds one pooled connection for the whole closure, plus the global_config advisory lock. Two handlers ran a permission lookup from inside that closure when a save escalated:

• PUT /api/config (updateConfig): requireRIExchangeAutoModeGrant (execute:ri-exchange) and requireSwitchGrants → requireAutoPurchaseGrant (execute:purchases, for both the auto-purchase and ladder-execution switches).
• PUT /api/ri-exchange/config (updateRIExchangeConfig): requireRIExchangeAutoModeGrant.

Each of those called requirePermission, which reads the auth store through the same pool and so needs a second connection. With N concurrent arming saves on a pool of N, the lock holder is waiting for a connection and the other N-1 hold connections while they wait on the advisory lock in Postgres. Every save hangs until its deadline. Fargate's default pool is 3 and Cloud Run's is 6. The issue names the global-config PUT. The dedicated RI-exchange PUT uses the same closure and gate, so it gets the same fix here.

Fix

This follows #1509's approach: fetch before the transaction and check inside it against the snapshot.

• internal/api/auto_purchase_grant.go: new armingGrants / resolveArmingGrants / resolveGrant. They resolve the requirePermission(execute, purchases) and requirePermission(execute, ri-exchange) outcomes (nil means granted, otherwise the exact error requirePermission returned) before the transaction opens. requireAutoPurchaseGrant is split so its credential half (refuseAutoPurchaseCredential: a nil session or the stateless admin API key is refused) can be reused in the closure through prefetchedAutoPurchaseGrant. Plan and ladder-config callers of requireAutoPurchaseGrant don't run inside a transaction and are unchanged.
• requireAutoPurchaseSwitchGrant, requireLadderExecutionSwitchGrant, requireSwitchGrants and requireRIExchangeAutoModeGrant are now plain functions that take the pre-resolved grant instead of (h, req). They have no way to reach the pool.
• updateConfig resolves both grants after requireGlobalConfigScope, so a caller refused by scope still costs no extra lookups. updateRIExchangeConfig resolves only execute:ri-exchange.

The grant checks are exactly as strict as before.

• Escalation is still decided inside the closure against the advisory-locked row: off→on for both switches, and arming or raising a cap for RI exchange. Only the permission lookup moved out.
• The lookup is the same requirePermission call on the same request, so a user API key is still held to the key's effective permissions and the admin API key is still refused for the purchase switches.
• A lookup error is stored, not raised. It is returned only when the write escalates, which is exactly when the old code would have made the lookup and returned that same error. An ordinary save is therefore unaffected by a failing execute-verb lookup, and a failing lookup never becomes a grant.

Why the snapshot is safe (TOCTOU). The permission is now read a little earlier: before the transaction opens, rather than after the advisory lock is acquired. A revocation that commits in that gap is honored on the caller's next request, not this one. The old code gave no stronger guarantee:

• Its lookup also ran on a different connection, outside this transaction and without locking any auth row. A revocation committed after its lookup but before this transaction's commit was not seen either.
• The update:config check and the requireGlobalConfigScope check that gate this same write have always run before the transaction.
  So the window grows only by the time spent waiting for the global_config advisory lock. That lock is held by short read-modify-write transactions, and no security property depended on that wait. A tx-local re-check isn't possible without a second pool connection, because the auth service has no transaction-scoped API, and that would recreate the bug. So I didn't add one.

Cost: every PUT /api/config now does two extra HasPermissionAPI lookups up front, even when it arms nothing. Previously they ran only when the save escalated. This is the price of not touching the pool inside the closure. The alternative was an abort-and-retry closure that resolves the grant lazily, which was more complex for a low-frequency settings endpoint.

Closes #1517

Verification

All commands were run on 922b8e22 with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true:

• go build ./..., go vet ./..., go vet -tags integration ./...: clean
• go test -count=1 ./internal/api/: ok. go test -count=1 ./internal/mocks/: ok
• golangci-lint run --allow-parallel-runners ./internal/api/, with and without --build-tags integration: 0 issues
• gocyclo -over 10 -ignore "_test\.go" on the five changed non-test files: no output

Real-Postgres regression test (testcontainers), fail-before / pass-after. The test is TestGlobalConfigArmingSavesDoNotExhaustPool in internal/api/handler_config_arming_pool_integration_test.go. The config store, the real auth.Service and its auth.PostgresStore all share one pool of 3 connections (the Fargate default), wired the way internal/server/app.go wires them. The session user holds update:config, execute:purchases and execute:ri-exchange. Each subtest fires 12 concurrent saves (4× the pool) with a 10 s deadline. Arming and disarming bodies alternate so that many lock holders need the grant, not only the first.

go test -tags integration -count=1 -timeout 20m -run 'TestGlobalConfigArmingSave' ./internal/api/

Subtest	origin/main ec78d85c (before)	this branch (after)
config PUT arms auto-purchase	FAIL: batch took 10.01 s, saves fail with context deadline exceeded (pool Acquire timeouts while other saves sit in pg_advisory_xact_lock)	PASS, 168 ms
config PUT arms ladder execution	FAIL: 10.01 s, deadline exceeded	PASS, 131 ms
config PUT arms RI exchange auto mode	FAIL: 10.01 s, deadline exceeded	PASS, 115 ms
RI exchange config PUT arms auto mode	FAIL: 10.00 s, deadline exceeded	PASS, 104 ms

The after run was also repeated with -count=3: all pass.

TestGlobalConfigArmingSaveGrantStaysStrict runs on the same real auth store. A session with only view/update:config is refused (403) on all four arming saves, through both routes. An ordinary save goes through, and nothing armed lands. It passes both before and after, which shows strictness didn't change. The existing TestConfigInfrastructureKeyHTTP (admin API key writes through both routes) also passes.

Unit tests (mock evidence) are in internal/api/global_config_arming_grants_test.go:

• TestGlobalConfigArmingGates_NoAuthCallInsideTransaction uses a store double whose UpdateGlobalConfigAtomic counts auth-service calls made while apply runs. Before the fix, all 8 arming cases (granted and refused, for auto-purchase, ladder execution, RI exchange via /api/config, and RI exchange via the dedicated route) fail with non-zero in-transaction auth calls. The ordinary-save case passes. After the fix, all pass with 0 calls and the same 200/403 outcomes.
• TestGlobalConfigArmingGates_LookupErrorOnlyMattersOnEscalation: an execute-verb lookup that errors doesn't fail an ordinary save. It does fail (fail closed, no write) every escalating save on both routes.
• The existing gate suites (auto_purchase_grant_test.go, ri_exchange_automode_gate_test.go, handler_config_test.go, etc.) pass unchanged, covering the grant matrix through the router with session, user API key and admin API key credentials.

Combined-tree verification at local queue commit ccab8379 = origin/main ec78d85c + fix/issue-1488 (#1509), fix/issue-316-next (#1516), fix/issue-1517 and fix/issue-1518 merged in that order. Passed: go build ./...; go vet with and without -tags integration; go test -race -short -count=1 ./...; golangci-lint v2.10.1 with and without the integration tag; gocyclo -over 10; go test -race -tags integration on internal/api, internal/commitmentopts, internal/config and internal/database/postgres/migrations; the git-secrets scan and scripts/test-git-secrets-allowlist.sh.

Notes / out of scope

• No migration and no store/interface change.
• This merges cleanly with fix(config): make the service-config PUT read-merge-write atomic #1509 (git merge-tree against its head). Both touch handler_config.go, but in different functions.
• No frontend change is needed.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p3, type/bug (the issue has no triaged label).

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 922b8e2215d3a303a812ff65de9cebd68d92f72c

Blocking:

• none

Non-blocking:

• internal/api/handler_config.go:89: every PUT /api/config now runs two execute-verb lookups up front, including ordinary saves that arm nothing (the PR body says so). They're cheap: a sha256 key lookup or HasPermissionAPI, with no writes and no audit or log side effects in requirePermission/authorizeAPIKey. Fine for a settings endpoint. Just noting that a denied execute verb now costs a lookup on every save.
• TestGlobalConfigArmingSaveGrantStaysStrict covers only a session credential on the real auth store. User-API-key and admin-key strictness rest on the existing mock-based router suites, which pass unchanged. A real-store user-API-key case (key holds update:config but not execute:purchases, and must get 403 on arming) would pin the "evaluated against the credential presented" claim end to end. Optional.

Attacked:

• Strictness: the in-closure gates now take pre-resolved errors from the same requirePermission(ctx, req, execute, purchases|ri-exchange) call on the same ctx and request, and escalation is still decided against the advisory-locked row. Admin-key refusal (refuseAutoPurchaseCredential) still runs in the closure for both switches. A lookup error is returned only on escalation (fail closed, no write) and never becomes a grant. Plan and ladder-config callers of requireAutoPurchaseGrant are unchanged. The only UpdateGlobalConfigAtomic callers in internal/api are the two routes, and both are fixed. No other pool access remains in either closure. TOCTOU reasoning holds: the old lookup also ran on a separate connection without locking auth rows.
• Fail-before: I reverted the five non-test files to the merge-base ec78d85c and kept the new tests. TestGlobalConfigArmingGates_NoAuthCallInsideTransaction failed all 8 arming cases. TestGlobalConfigArmingSavesDoNotExhaustPool failed all 4 subtests at 10.01 s (deadline). After restoring, each pool subtest passes in about 150 ms, and ...GrantStaysStrict passes both before and after.
• Ran on the head with origin/main (2faf67fc) merged locally, which merged cleanly: go build ./...; go vet with and without -tags integration on ./internal/api/; golangci-lint v2.10.1 with and without --build-tags integration (0 issues); gocyclo -over 10 on the 5 changed files (silent); go test -race -short -count=1 ./internal/api/ ./internal/mocks/ ok; and go test -race -tags integration -count=1 ./internal/api/ (the whole package) ok.

Body-stale:

• none

_{Reviewer: rF9. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 922b8e2215d3a303a812ff65de9cebd68d92f72c

Blocking:

• none

Non-blocking:

• internal/api/handler_config.go:89: every PUT /api/config now pays two extra permission lookups (and, on the bearer path, two extra ValidateSession reads) even when it arms nothing. The PR body states this. It's acceptable for a settings endpoint. A lazy resolve (only when json.Unmarshal of the body could touch one of the arming fields) would avoid the cost, but it isn't needed.
• internal/api/handler_config_arming_pool_integration_test.go:180-197: the test file adds methods on *Handler (updateConfigErr, updateRIExchangeConfigErr) and a package-level bearerRequest helper under the integration tag. They're fine today (vet -tags integration is clean), but they're generic names that a later integration test in package api could easily collide with. Unexported free functions with an issue-specific prefix would be safer.
• (Raised in the earlier rF9 review; I agree.) TestGlobalConfigArmingSaveGrantStaysStrict covers only a session on the real auth store. A real-store user-API-key case, where the key holds update:config but not execute:purchases, would pin the "credential presented" claim end to end. The mock router suites cover it today. Optional.
• Pre-existing and unrelated: go test -tags integration -run 'Config|RIExchange|AutoPurchase|Ladder' ./internal/api/ trips the package's outbound-network guard (36 IMDS calls) on the merge-base as well. Some test in that subset builds a default AWS config. It isn't caused by this PR.

Attacked:

• Checked that the closures in updateConfig and updateRIExchangeConfig no longer reach h.auth. The gates are now free functions with no handler receiver.
• requirePermission has no side effects (no audit write, no rate limit), so resolving it up front for every save only adds reads.
• Strictness is unchanged. Escalation is still decided against the locked row. The admin key is still refused by refuseAutoPurchaseCredential for both purchase switches and still allowed for RI exchange, as on main. A user API key still goes through authorizeAPIKey. A lookup error is still returned only on escalation, the same error type as before.
• The remaining requireAutoPurchaseGrant callers (plan and ladder-config arm grants) are not inside a transaction.
• Ran at 922b8e2: go build ./..., go vet with and without -tags integration (clean); go test -race -short -count=1 ./internal/api/ (ok); go test -race -tags integration -run 'TestGlobalConfigArmingSave|TestConfigInfrastructureKeyHTTP' ./internal/api/ (pass, 12 saves on a pool of 3 in ~200-260 ms).
• Fail-before: restored the five non-test files from the merge-base. TestGlobalConfigArmingGates_NoAuthCallInsideTransaction then fails all 8 arming cases. The pool integration test hits context deadline exceeded (pgx Acquire timeouts while holders sit in pg_advisory_xact_lock), reproducing fix(config): global-config PUT can exhaust the DB pool when concurrent saves arm automation (permission check inside the transaction) #1517. Restored the files afterwards.
• Combined tree origin/main 2faf67f + this PR + fix(revoke): make the Azure in-flight flip an exclusive claim so only one revoke posts a Return #1520's own commit: build, vet (including e2e), and the unit and targeted integration tests all pass.

Body-stale:

• "Combined-tree verification at ... origin/main ec78d85c": main has since moved to 2faf67f (fix(auth): expose system_managed on API groups and guard UpdateGroup #1513-fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback #1516 merged). This PR still merges cleanly and passes there (see above).

_{Reviewer: rF10. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 922b8e2215d3a303a812ff65de9cebd68d92f72c merged onto main (2faf67fc), together with #1519 in that order (combined tree e5f07e4f, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 922b8e22 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 922b8e2215d3a303a812ff65de9cebd68d92f72c.