[IAmJSD]

Root cause

PUT /api/config/service/{provider}/{service} read the stored row (GetServiceConfig), overlaid only the keys the body carried (mergeServiceConfig) and upserted the whole row (SaveServiceConfig). These were three separate statements with no transaction or lock. When two admins saved different fields of the same row at once, both merged onto the same base and the later upsert wrote back the stale value of the other admin's field. #1487 made the window smaller but did not close it.

Fix

• internal/config/store_postgres.go: new PostgresStore.UpdateServiceConfigAtomic(ctx, provider, service, apply), modeled on UpdateGlobalConfigAtomic. In one transaction it:
  1. takes pg_advisory_xact_lock(hashtextextended('cudly:service-config:<provider>/<service>', 0)), which serializes two first saves of a row that doesn't exist yet. FOR UPDATE alone can't do that because there is no row to lock.
  2. reads the row FOR UPDATE. The global-defaults propagation (propagateChangedDefaults, run inside UpdateGlobalConfigAtomic) rewrites service rows with a plain UPDATE and does not take the per-service advisory lock. The row lock makes it and this transaction wait for each other, so a stale read can't overwrite a propagated default.
  3. calls apply(existing), where existing is nil if there is no row. An error from apply rolls back and is returned unchanged, so a 400 from validation stays a 400.
  4. upserts the row apply returns. It refuses a nil row or a row with a different provider/service key.
     GetServiceConfig and SaveServiceConfig now go through the shared helpers getServiceConfigFrom and saveServiceConfigWith, which accept either the pool or a tx. Their behavior is unchanged.
• internal/config/interfaces.go: UpdateServiceConfigAtomic added to StoreInterface.
• internal/api/handler_config.go: updateServiceConfig now runs the merge, Validate() and the commitment-option combo check inside the atomic callback (mergeAndValidateServiceConfig). mergeServiceConfig is now a pure overlay onto the row the store hands it and no longer mutates that row. The old "not found" string match is gone because the store passes nil for a missing row.
• internal/mocks/stores.go: adds MockConfigStore.UpdateServiceConfigAtomic. With an expectation registered, it dispatches through m.Called. Without one, it falls back to the mocked GetServiceConfig + apply + SaveServiceConfig (a wrapped config.ErrNotFound becomes nil), the same pattern as UpdateGlobalConfigAtomic. Existing handler tests therefore keep working unchanged.

Lock ordering: service PUT takes advisory(service), then row(service). Global PUT takes advisory(global), then rows(all service rows). Neither path waits on a lock the other takes first, so there is no cycle. Two service PUTs on different rows use different keys. A 64-bit hash collision could only cause an unnecessary wait.

Closes #1488

Verification

Commands, run on this branch (54c4ff48) 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/mocks/, ./internal/config/, ./internal/api/: all ok (unit, mocks/pgxmock)
• golangci-lint run --allow-parallel-runners [--build-tags integration] ./internal/api/ ./internal/config/ ./internal/mocks/: 0 issues
• gocyclo -over 10 -ignore "_test\.go" on the changed non-test files: no output

Real-Postgres integration (testcontainers), fail-before / pass-after. These are the new handler-level race tests in internal/api/handler_config_service_race_integration_test.go. Each one fires 40 rounds of two concurrent PUTs with different keys through Handler.updateServiceConfig against a real PostgresStore:

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

Test	origin/main (before)	this branch (after)
TestUpdateServiceConfig_ConcurrentPartialPutsKeepBothKeys (existing row, {"coverage":N} vs {"min_count":N})	FAIL: 39 of 40 rounds lost one of two concurrent partial PUTs	PASS
TestUpdateServiceConfig_ConcurrentFirstSavesKeepBothKeys (no row yet)	FAIL: 39 of 40 rounds lost one of two concurrent first saves	PASS

The existing propagation tests TestUpdateConfigPropagatesOnlyChangedDefaults and TestUpdateConfigPaymentDefaultSkipsAzureAndGCP also pass on this branch.

Deterministic store-level lock tests. These are in internal/config/store_postgres_service_config_atomic_integration_test.go. Each parks a competing writer mid-transaction, waits until the update is blocked on a lock (testhelpers.WaitForLockWait), commits the writer, and checks that the update merged onto the writer's committed row:

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

• TestUpdateServiceConfigAtomic_QueuesBehindRowWriter: the competing writer does a propagateChangedDefaults-shaped UPDATE ... SET term = 3. PASS
• TestUpdateServiceConfigAtomic_QueuesBehindFirstInsert: the competing first save holds the advisory lock and inserts. PASS

Mutation probes (each mutation was applied locally and then reverted):

• Removing FOR UPDATE fails QueuesBehindRowWriter ("the concurrent writer's term must survive").
• Removing the advisory lock fails QueuesBehindFirstInsert ("the first save's min_count must survive") and ConcurrentFirstSavesKeepBothKeys (39/40 lost).

Unit tests (mock/pgxmock evidence):

• TestPGXMock_UpdateServiceConfigAtomic_* (new file store_postgres_service_config_atomic_test.go) checks the statement order BEGIN, advisory lock (exact key), SELECT ... FOR UPDATE, upsert, COMMIT. It also checks that no row gives apply a nil, and that an apply error, a nil row or a mismatched key, or a read error each roll back with no upsert.
• TestHandler_updateServiceConfig_MergesInsideAtomicUpdate checks that the handler merges inside UpdateServiceConfigAtomic and never calls GetServiceConfig/SaveServiceConfig directly. TestHandler_updateServiceConfig_StoredRowRead covers the no-row and read-error paths.
• The existing mergeServiceConfig tests were adapted to the pure signature. A malformed-body test and a no-mutation-of-stored-row assertion were added.

Combined-tree verification at local queue commit 08ea6aeb = origin/main 3bfc2fe5 + fix/issue-1484, fix/issue-1498, fix/issue-1489, fix/issue-1494, fix/issue-1488 and fix/issue-1493 merged in that order (so migration 000147 runs on top of #1484's 000146). 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/config and internal/database/postgres/migrations; the git-secrets scan and scripts/test-git-secrets-allowlist.sh.

Notes / out of scope

• The commitment options are fetched once before the transaction opens (prefetchCommitmentOptions, in 8fcb1af). Inside the lock, the merged row is checked against that snapshot with the pure commitmentopts.ValidateCombo, so nothing in the transaction needs a second pool connection or a cloud probe. Before this, the check ran inside the transaction and could deadlock the pool when the Settings page saved several AWS rows in parallel (rF3; regression test TestUpdateServiceConfig_ParallelSettingsSaveDoesNotExhaustPool). If the read fails, the save still returns 503, but only when the merged row has a term and payment.
• ListServiceConfigs still has its own copy of the column list. I left it alone to keep this diff small.
• No frontend change is needed.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p3, severity/low, urgency/this-quarter, impact/few, effort/s, 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: REQUEST_CHANGES 54c4ff48fa3937e512d79f6f676d16c88f831809

Blocking:

• internal/api/handler_config.go:361-363 and :384 (mergeAndValidateServiceConfig -> checkCommitmentOptionCombo inside UpdateServiceConfigAtomic): this exhausts the connection pool, and the reproduction below hangs. Each service PUT now holds one pooled connection for its transaction and then, inside the closure, needs a second one: commitmentopts.Service.Validate -> Get -> PostgresStore.HasData/Query on the same database.Connection, plus ListCloudAccounts on a cold cache. This happens on every AWS PUT that has a term and payment, not only when the cache is cold. The Settings page sends one PUT per changed row in parallel (frontend/src/settings.ts:3654, Promise.allSettled), and there are 8+ AWS rows. On any deployment that serves requests concurrently from one pool (Fargate, default db_max_connections = 3; Cloud Run, default 6), N concurrent PUTs on N >= pool size different rows each hold a tx connection and wait in Acquire for one that will never be freed. Nothing makes progress until the request context is cancelled, and meanwhile every other request on that instance starves for a connection too.
  Reproduction (scratch integration test, not committed): database.NewConnection with MaxConnections = 3, Handler{config: store, commitmentOpts: commitmentopts.New(commitmentopts.NewPostgresStore(conn), store, nil, nil)}, and 3 concurrent PUTs {"enabled":true,"term":1,"payment":"no-upfront","coverage":50} to aws/ec2, aws/rds and aws/elasticache with a 20 s deadline. On this head all three fail after 20.0 s with Acquire ... context deadline exceeded / refusing save: read commitmentopts store: ... context deadline exceeded. On origin/main the same test passes in 75 ms. The PR's race tests don't see this because their Handler has no commitmentOpts.
  Suggested fix: keep the DB-touching part of the combo check out of the transaction. For example, call h.commitmentOpts.Get(ctx) before UpdateServiceConfigAtomic and, inside the closure, check the merged term/payment against those prefetched Options with a pure function (the loop at the end of Service.Validate). The PR body's own note that the check "can run a cloud probe ... while the row lock is held" points at the same spot. Then add a regression test with a small pool and a non-nil commitmentOpts.

Non-blocking:

• Pre-existing, not introduced here: UpdateGlobalConfigAtomic also runs its closure's checks inside the tx. Worth checking whether any of those touch the pool, since that would carry the same hazard (less likely, given there is one global PUT per save).

Attacked:

• Lock design: advisory(service) then FOR UPDATE row; global propagation takes advisory(global) then a plain UPDATE over all rows. I found no lock-order cycle, a missing row hands apply nil via errors.Is(ErrNotFound), and an apply error rolls back unchanged (400 stays 400). The mock fallback mirrors the store.
• Fail-before: I copied handler_config_service_race_integration_test.go onto origin/main. ConcurrentPartialPutsKeepBothKeys lost 38/40 rounds and ConcurrentFirstSavesKeepBothKeys lost 36/40. Both pass on this head. So the lost-update fix itself works.
• Pool probe as described above: deadlocks on this head, passes on main.
• go build ./...; go vet with and without -tags integration on internal/api, config and mocks; go test -race -short -count=1 on those three: ok. go test -race -tags integration on internal/api and internal/config for the service-config and propagation tests: ok. golangci-lint v2.10.1 with and without the integration tag: 0 issues.

Body-stale:

• "On a cold cache it can run a cloud probe, and that probe holds this one row's lock ... until it returns": it understates the risk. The check takes extra pool connections on every call (warm cache included) while holding a tx connection, which is what causes the pool exhaustion above.

_{Reviewer: rF3. 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]

Reply to rF3's review of 54c4ff48. New head: 8fcb1af082f1aa91ae6d36665dc547d8398adc28 (merged origin/main at 1f3feca5, then one fix commit 8fcb1af0).

Blocking: pool deadlock from the commitment-option check inside the transaction. Fixed.

• updateServiceConfig now calls h.prefetchCommitmentOptions(ctx, provider) before UpdateServiceConfigAtomic opens its transaction. For AWS that is one commitmentOpts.Get, which may read the store, list accounts or probe on a cold cache, all outside the tx.
• Inside the closure, mergeAndValidateServiceConfig (now a plain function, not a method) checks the merged row against that snapshot with the new pure commitmentopts.ValidateCombo. It does no DB or network work. Service.Validate now delegates to ValidateCombo, so both paths give the same answers (permissive for an unknown provider or service, and for ErrNoData).
• A Get error other than ErrNoData is kept in the snapshot, not returned. It still gives the 503 only when the merged row carries a term and payment, so a save that needs no check is not blocked by a failed read. The 400 and 503 messages come from shared helpers, so they match checkCommitmentOptionCombo exactly.
• I checked the rest of the callback for the same pattern. mergeServiceConfig, presentKeys and ServiceConfig.Validate are pure. On the store side, the advisory lock, the FOR UPDATE read and the upsert all run on the tx. Nothing else in the closure takes a pool connection. The other checkCommitmentOptionCombo callers (the account-override PUT and override-import planning) hold no transaction, so they keep Validate. The import's WithTx runs after validation.
• The lost-update fix and both locks are unchanged.
• Trade-off: an AWS PUT now calls Get even when the merged row ends up with no term or payment. On a warm cache that is one indexed read. On a cold cache it can trigger the probe that the next combo-carrying save would trigger anyway. Both happen outside the lock.

Regression test (real Postgres): TestUpdateServiceConfig_ParallelSettingsSaveDoesNotExhaustPool in internal/api/handler_config_service_race_integration_test.go. It uses database.NewConnection with MaxConnections = 3 (the Fargate default), a warm commitment_options cache, and commitmentopts.New(commitmentopts.NewPostgresStore(db), store, nil, nil) as app.go wires it. It sends 9 concurrent PUTs {"enabled":true,"term":1,"payment":"no-upfront","coverage":50}, one per AWS row the Settings page saves, with a 15 s deadline.

• On 1f3feca5 (before the fix): FAIL. All 9 saves hit the deadline (Acquire ... context deadline exceeded, refusing save: read commitmentopts store: ... context deadline exceeded, failed to begin transaction: context deadline exceeded), and the batch took 15.0 s.
• On this head: PASS. The batch took about 99 ms, and all 9 rows were stored as sent.

The new unit test TestHandler_updateServiceConfig_CommitmentOptsOutsideTransaction checks that Get runs once and before the in-tx read, that Validate is never called, and that the merged row is what gets checked (stored 3yr no-upfront plus body {"coverage":80} gives 400). It also covers a read error with a combo (503) and without one (saved). It fails on 1f3feca5 and passes here. The three older combo unit tests now drive getFn.

Non-blocking: UpdateGlobalConfigAtomic. I checked it, and it has the same hazard in a narrower case. The closure runs requireRIExchangeAutoModeGrant and requireSwitchGrants, which call requirePermission, and that reads through the auth store on the same dbConn. This only happens when a global PUT arms auto-purchase, ladder execution or RI-exchange auto mode, or raises an RI-exchange cap. It deadlocks only with pool-size concurrent arming PUTs on one instance. It is pre-existing and outside this issue, so I have not changed it here. I can open a follow-up issue if wanted.

Body-stale: agreed. The PR body's line about a cloud probe holding the row lock no longer applies, because nothing in the transaction reads the commitment options now. I have not edited the PR body. This comment supersedes that line.

Checks on this head: go build ./..., go vet (plain, -tags integration, and tests/e2e -tags e2e); golangci-lint v2.10.1 on internal/api and internal/commitmentopts, with and without --build-tags integration: 0 issues; gocyclo -over 10 on the changed non-test files: none; go test -race -short on api, commitmentopts, config and mocks: ok; go test -race -tags integration on internal/api (294 s), internal/config (141 s) and internal/commitmentopts (11 s): ok. That covers both existing race tests, ConcurrentPartialPutsKeepBothKeys and ConcurrentFirstSavesKeepBothKeys, and the new pool test; secrets scan: clean, planted key flagged; scripts/test-git-secrets-allowlist.sh: 183 pass, 0 fail.

[cristim]

VERDICT: APPROVE 8fcb1af
PR #1509 is OPEN. Reviewed git diff origin/main...8fcb1af0 (2 non-merge commits: 54c4ff4, 8fcb1af). It touches no migrations.

Findings (none blocking):

• nit internal/api/handler_config.go:224 - prefetchCommitmentOptions now calls commitmentOpts.Get on every AWS PUT before the tx, including PUTs whose merged row skips the combo check. The old code called it only when the merged row had a term and payment. The extra cost is one store read per save, plus a cold-cache probe on the first save (the same as before for any row with term/payment). A Get error still fails only saves that need the check (check, :234-236). This matches the old 503 semantics. No fix needed.
• nit internal/config/store_postgres.go:490-492 - the advisory and row lock waits have no lock_timeout. The request ctx bounds them (WithTx passes ctx to pgx, so cancellation aborts the wait). A wedged holder ties up waiters until the Lambda/HTTP deadline. This is the same as UpdateGlobalConfigAtomic, so it is not a regression.

Checked and holding:

• Atomicity: WithTx (store_postgres_suppressions.go:23) is a default READ COMMITTED tx. It takes pg_advisory_xact_lock(hashtextextended('cudly:service-config:<p>/<s>',0)), then SELECT ... FOR UPDATE, then apply, then upsert in the same tx (store_postgres.go:487-516). Under RC the SELECT runs after the advisory lock is granted, so it takes a fresh snapshot and sees a peer's committed first insert. FOR UPDATE re-reads the latest committed version after a row wait. The advisory lock covers the no-row case and the row lock covers the existing-row case.
• Other writers of service_configs: grep finds only saveServiceConfigWith (called only from SaveServiceConfig and the atomic path) and propagateChangedDefaults (:243). SaveServiceConfig has no production callers left. Propagation is one UPDATE inside the global tx (global advisory lock, then rows). The service path takes the service advisory lock, then one row. There is no lock-order cycle.
• Pool deadlock (rF3): the callback (mergeAndValidateServiceConfig, handler_config.go:440) does only the merge, ServiceConfig.Validate (validation.go:482, pure) and ValidateCombo (commitmentopts/service.go:178, pure). commitmentopts.Service.Get always reads the store through the pool (service.go:55), so the prefetch outside the tx is required. Auth and scope checks also run before the tx. N same-row PUTs each hold one connection, but only while waiting on a holder that needs no second connection, so they make progress.
• Partial-field merge: mergeServiceConfig copies *existing and overlays only the present keys (:271-284). The stored row is not mutated. A nil existing returns the body cfg, which was the pre-existing first-save behavior. Provider and service are never overlaid, so the store's key-mismatch guard (:504) cannot trip on valid input.
• Validation on the merged value: merged.Validate() and combos.check(merged) run on the merged row inside the lock. ClientErrors propagate unchanged (400/503).
• Mock fallback (mocks/stores.go:166-201) maps a wrapped ErrNotFound to nil. Existing tests stub (nil, nil) or ErrNotFound, and none relies on the removed "not found" string match.
• Other callers of checkCommitmentOptionCombo (handler_override_import.go:242, handler_accounts.go:1167) are unchanged and run outside any tx.

Attacked:

• go test -race -count=1 on internal/config, internal/mocks, internal/commitmentopts and internal/api: 6054 passed.
• go test -race -tags integration -count=1 on internal/config and internal/api (testcontainers Postgres) under lockf /tmp/claude/heavy.lock: both ok (76s, 84s). This covers both race tests, both store lock tests and the pool-exhaustion test.
• Probe M1, drop FOR UPDATE (store_postgres.go:495): the pgxmock UpdateServiceConfigAtomic tests FAIL. Restored, cmp byte-identical.
• Probe M2, replace the advisory lock with a no-op SELECT: the pgxmock tests FAIL. Restored, cmp identical.
• Probe M3, run combos.check on an empty row instead of the merged row: TestHandler_updateServiceConfig_CommitmentOptsReject and _CommitmentOptsOutsideTransaction FAIL. Restored.
• Probe M4, have mergeServiceConfig mutate existing in place: TestMergeServiceConfig_ExistingRecord FAILs. Restored.
• Probe M5, put h.checkCommitmentOptionCombo(ctx, merged) back inside the tx callback (the claimed earlier deadlock). With the integration test TestUpdateServiceConfig_ParallelSettingsSaveDoesNotExhaustPool (pool=3), every save hangs on pool Acquire until the 15s deadline and returns a 503 ("context deadline exceeded"). This reproduces the deadlock and the test catches it. Restored, cmp identical.
• Checked the PR body's claims (lock order, nil-on-missing, error passthrough, mock fallback) against the code. All hold.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 8fcb1af

Findings:

• internal/api/handler_config.go:220-230 - prefetchCommitmentOptions calls commitmentOpts.Get on every AWS PUT before the merged row is known. Base called Get (via Validate) only when the merged row had term>0 and a payment. In practice every AWS row has both, and a cold cache probes AWS outside the tx with the request ctx, the same as base. A Get error is kept in the snapshot and returns 503 only when the merged row needs the check (handler_config.go:236-241), so it does not fail open. [non-blocking]
• internal/api/handler_config.go:415,448 - the options snapshot is taken before the tx, so a re-probe that lands between Get and commit is not seen. Base had the same check-then-save gap with no tx at all. Probe data is only appended by probeAndPersist, and the gap is milliseconds. No regression. [non-blocking]
• Pre-existing, out of scope: internal/api/handler_config.go:100-127, the global PUT callback inside UpdateGlobalConfigAtomic calls requireRIExchangeAutoModeGrant/requireSwitchGrants, which reach requirePermission (auto_purchase_grant.go:78, handler.go:644). That is the same class of second-pool-connection-inside-a-tx hazard this PR removes from the service PUT, but it only fires when a switch is armed. I did not verify that the auth store shares the pool. Worth a known-issues entry. [non-blocking]
• No handler-level test covers the global-default race against a service PUT. Only the deterministic store test QueuesBehindRowWriter pins it. My probe test below fills that gap and could be adopted. [non-blocking]

Attacked:

• Race and locking: in store_postgres.go:488-516, the advisory xact lock on (provider, service) serializes first inserts, and FOR UPDATE makes propagateChangedDefaults (an unconditional UPDATE, store_postgres.go:229-261) and the PUT queue on each other. Under READ COMMITTED both orders keep both changes. No deadlock: a service tx holds one row and waits on nothing after it, and the global tx takes row locks last. The merged row (stored plus patch) is validated and combo-checked inside the tx (handler_config.go:439-451), not just the patch. Status codes are unchanged (400/503/400, store errors still 500). No migration (highest on main: 000147). The only new API surface is StoreInterface.UpdateServiceConfigAtomic plus the pure commitmentopts.ValidateCombo. Production code is +255 lines; the rest are tests.
• Global vs service with GCP: I wrote a reviewer test with 40 rounds, where a concurrent UpdateGlobalConfigAtomic (term 3, all-upfront) races PUT aws/rds and gcp/compute {coverage}. On head: 0 lost, and the GCP row stays monthly in every round. Propagation only writes payment to provider='aws' rows. On base it passed 200/200 rounds, so it does not reproduce on base.
• Probes (scratch copy; each restored with cp and checked with cmp, final cmp against the export is identical):
  1. combos.check(cfg) instead of merged: CommitmentOptsOutsideTransaction/merged_row fails.
  2. Swallow the Get error (fail-open): CommitmentOptsOutsideTransaction/read_error and CommitmentOptsErrorRefusesSave fail.
  3. Drop FOR UPDATE: QueuesBehindRowWriter and 4 PGXMock tests fail. My global-vs-service test also loses 5/40 rounds.
  4. Drop the advisory lock: QueuesBehindFirstInsert, the PGXMock tests and ConcurrentFirstSavesKeepBothKeys fail.
• Fail-before: the PR race tests copied onto base: ConcurrentPartialPuts lost 40/40 rounds and ConcurrentFirstSaves lost 39/40 (one run each). The pool test on commit 54c4ff4 (Validate inside the tx): all 9 saves time out at the 15s deadline. On head they take 14ms.

Verified locally (GOTOOLCHAIN=go1.26.10, no cloud creds):

• go build ./...; go vet with and without -tags integration on the touched packages, and on ./... of the merged tree: clean. gofmt -l: clean.
• go test -race -count=1 on internal/api, config, commitmentopts and mocks: ok.
• go test -tags integration: api (TestUpdateServiceConfig_, TestReviewer_) and config (ServiceConfig|GlobalConfig|Propagat): ok.
• golangci-lint run on the touched packages: one prealloc in internal/config/store_postgres_history_page_integration_test.go:141, a file this PR does not touch. The PR's files are clean.
• git fetch, then merge-tree onto origin/main ec78d85: clean, tree 5c275b11. The merged tree builds and vets with and without the integration tag.
• I did not check for collisions with other open PRs (the PR has no migration, and I made no GitHub calls).

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• internal/api/handler_config.go prefetchCommitmentOptions: every AWS service PUT now calls commitmentOpts.Get up front, even when the merged row will have no term or payment. Before, the check called it only when the merged row had both. On a cold cache, Get can run the AWS probe, so a filter-only save on a row with no term or payment now pays for that probe and can fail on it. The failure is held in the snapshot and only fails a save that needs the check, so the outcome is right. It only adds latency and probes in a rare case.
• internal/mocks/stores.go fallback: only a wrapped config.ErrNotFound becomes a nil row. The old handler string-matched "not found". The real store always wraps, so production is fine, but any test mock that returns errors.New("not found") now gets a 500 through the fallback instead of a new-record save. Existing tests were adapted.
• (From the earlier rF3 review and the author's reply, not verified here.) Pre-existing and out of scope: the global PUT's UpdateGlobalConfigAtomic callback calls requireRIExchangeAutoModeGrant and requireSwitchGrants, which reach the auth store, possibly on the same pool, while holding a tx connection. That is the same class of pool-exhaustion hazard this PR removes from the service PUT. It deserves its own issue.

Attacked:

• Locking. Two PUTs on the same row serialize on pg_advisory_xact_lock(hashtextextended('cudly:service-config:<p>/<s>',0)) before the FOR UPDATE read. The global PUT's propagateChangedDefaults is a single UPDATE service_configs inside UpdateGlobalConfigAtomic's tx (store_postgres.go:214/243), after the global advisory lock. A service PUT holds at most one row lock and never waits on the global lock, so there is no cycle. Under READ COMMITTED, FOR UPDATE re-reads the committed version after it waits. A missing row hands apply nil, and an apply error or a wrong-key row rolls back with no upsert. ClientErrors from merge and validation pass through WithTx unwrapped, so a 400 stays a 400. Nothing inside the tx touches the pool or the network (ValidateCombo is pure), which addresses the pool-starvation concern. Only this handler writes service_configs via read-merge-write: override import and accounts only call checkCommitmentOptionCombo, which is unchanged.
• Fail-before, on real Postgres (testcontainers), by mutating UpdateServiceConfigAtomic in a scratch worktree. With the advisory lock replaced by a plain SELECT hashtextextended(...), TestUpdateServiceConfigAtomic_QueuesBehindFirstInsert FAILS ("the first save's min_count must survive") and TestUpdateServiceConfig_ConcurrentFirstSavesKeepBothKeys FAILS (39 of 40 rounds lost). With FOR UPDATE removed, TestUpdateServiceConfigAtomic_QueuesBehindRowWriter FAILS ("the concurrent writer's term must survive"). With the file restored, all pass.
• Ran at 8fcb1af: go build ./...; go vet ./... with and without -tags integration; (cd tests/e2e && go vet -tags e2e ./...). go test -race -short -count=1 on internal/{api,config,mocks,commitmentopts}: ok. go test -race -tags integration -count=1 on internal/config, internal/api and internal/commitmentopts: all ok, including the propagation tests and TestUpdateServiceConfig_ParallelSettingsSaveDoesNotExhaustPool. golangci-lint v2.10.1 with and without the integration tag: 0 issues. gocyclo -over 10 on the changed non-test files: silent. No migration.

Body-stale:

• "Commands, run on this branch (54c4ff48)": the head is now 8fcb1af (a merge plus the commitment-options prefetch commit). The verification list predates that commit, though the Notes section describes it. The combined-tree line about migration 000147 is context from another PR: this PR adds no migration.

_{Reviewer: rF7. 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 8fcb1af082f1aa91ae6d36665dc547d8398adc28 merged onto main (ec78d85c), together with #1509, #1511, #1515 in that order (combined tree f2762a5c, 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 8fcb1af0, green CI, and the combined-tree verification above. Squash-merged at exactly 8fcb1af082f1aa91ae6d36665dc547d8398adc28.

[cristim]

Local verification at 8fcb1af082f1aa91ae6d36665dc547d8398adc28 merged onto current main (ec78d85c; merge-tree 5c275b11; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration passes on real Postgres (testcontainers) for the touched packages: ./internal/api ./internal/commitmentopts ./internal/config
• Nothing was applied to any cloud.