[IAmJSD]

Problem

Writes to account_service_overrides from the single PUT, the single DELETE and the bulk import (#1486) left no audit record. The config_audit_events table that issue #9 asks the import to "match" did not exist anywhere in the schema or the code.

Fix

Schema: migration 000147_config_audit_events. 000146 is reserved for #1484. On this branch alone previousMigrationVersion resolves 147 to 145.

New table config_audit_events:

Column	Meaning
id	event ID
seq	identity column used for ordering. All events written in one transaction share created_at.
event_type	one of account_service_override.upsert, .delete, .import
parent_event_id	for an imported row, the import event it belongs to
actor	email, or the admin API key actor. Never empty.
actor_user_id, actor_api_key_id	set only when they are real UUIDs
account_id, provider, service	which override changed
before, after	JSONB snapshot of the override settings
details	JSONB, e.g. import mode and counts
created_at	timestamp

CHECK constraints:

• event_type must be one of the three values above.
• actor must not be empty.
• An override event must name its account, provider and service.
• An upsert must have an after.
• A delete must have a before and no after.
• An import event must not have a parent.

There are no foreign keys to users or accounts, so the trail outlives the account (whose overrides cascade away). This follows the same pattern as ri_exchange_direct_executions (000131). The down migration drops the table.

Store (internal/config/store_postgres_config_audit.go):

• SaveAccountServiceOverrideAuditedTx(ctx, tx, override, AuditContext) runs in this order:
  1. Takes the advisory lock hashtextextended('cudly:account-service-override:<account>/<provider>/<service>').
  2. Reads the stored row FOR UPDATE.
  3. Upserts with RETURNING.
  4. Records before (the locked row) and after (the row read back, so coverage shows the DECIMAL(5,2) value actually stored).
  5. Skips the event if the settings didn't change.
     Both locks are needed. The advisory lock serializes two first creates, which a row lock can't do because the row isn't visible yet. The row lock covers writers that don't take the advisory lock, such as a concurrent DELETE. Without both, an event could report the wrong before.
• DeleteAccountServiceOverrideAuditedTx runs DELETE ... RETURNING and records the deleted row as before. Deleting a missing override records nothing.
• SaveConfigAuditEventTx records the import's own event.
• SaveAccountServiceOverrideTx is replaced by the audited variant. The import was its only caller.

Handlers:

• PUT /api/accounts/:id/service-overrides/:provider/:service and the matching DELETE now write through the audited methods inside WithTx.
• The actor comes from the session via configAuditActor, which uses the same identity rules as the RI-exchange direct-execute audit. A failed audit insert fails the request, so a write never commits without its event.
• POST /api/accounts/service-overrides/import (applied, not dry-run) does the following in its existing single transaction:
  1. Records an account_service_override.import event with {mode, summary, rows}.
  2. Writes each insert/update row through the audited method, with parent_event_id pointing at that event and details.import_row set to the row's 1-based position in the file.
  3. Writes rows in (account, provider, service) order instead of file order. Each write holds its key's locks until commit, so two overlapping imports could otherwise deadlock. The report and its row numbers are unchanged.
• Dry runs and rejected strict imports return before the transaction, so they write nothing. A store failure rolls back the rows and every event, including the import event.

Closes #1493

Verification

All commands were run on fix/issue-1493 (723c88fc) with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

Static checks:

• go build ./..., go vet ./..., go vet -tags integration ./...: clean
• golangci-lint run --allow-parallel-runners [--build-tags integration] on ./internal/api/ ./internal/config/ ./internal/mocks/ ./internal/database/postgres/migrations/: 0 issues
• gocyclo -over 10 -ignore "_test\.go" on the changed non-test files: no output

Unit tests, one package at a time:

• go test -count=1 on ./internal/mocks/, ./internal/config/, ./internal/api/, ./internal/database/postgres/migrations/: all ok

Real-Postgres integration tests (testcontainers), fail-before / pass-after. Handler-level tests in internal/api/handler_override_audit_integration_test.go:

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

• TestOverrideAudit_SinglePutAndDelete: PUT insert, PUT update, no-op PUT, DELETE, and DELETE of a missing override. Checks exactly 3 events, the actor, the target, and the before/after chain.
• TestOverrideAudit_Import: a rejected strict import and a dry run write no events. A best-effort import writes one import event with mode and counts plus linked row events with before/after and import_row. A DB trigger that fails part way rolls back both rows and events.
• TestOverrideAudit_ConcurrentFirstSavesChain: 20 rounds of two concurrent PUTs creating the same override. The second event's before must equal the first event's after.

On origin/main all three fail with relation "config_audit_events" does not exist. Main has no audit trail, so this fail-before only shows the feature was absent. On this branch all three pass. The existing TestImportOverrides_StrictIsAllOrNothingPostgres also passes.

Store-level deterministic lock tests in internal/config/store_postgres_config_audit_integration_test.go. Each parks a competing writer, waits on WaitForLockWait, then commits it:

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

• SaveQueuesBehindDelete: covers the FOR UPDATE read. PASS.
• SaveQueuesBehindFirstCreate: covers the advisory lock. PASS.
• AfterIsTheStoredRow: coverage 33.333 is recorded as the stored 33.33. PASS.

Migration test:

go test -tags integration -count=1 -run TestMigration_ConfigAuditEvents ./internal/database/postgres/migrations/

PASS. It covers up from the previous version, valid inserts, seven rejected shapes, down dropping the table, and re-up.

Mutation probes (each applied locally, then reverted):

Mutation	Result
drop FOR UPDATE	SaveQueuesBehindDelete fails
drop the advisory lock	SaveQueuesBehindFirstCreate fails, and ConcurrentFirstSavesChain fails in 20 of 20 rounds
drop the no-op skip	SinglePutAndDelete fails

Unit tests (mock / pgxmock / in-memory store evidence):

• TestPGXMock_SaveOverrideAudited_* and TestPGXMock_DeleteOverrideAudited: statement order is lock, FOR UPDATE, upsert RETURNING, event. Also covers no event on a no-op and no event on a missing delete.
• TestPGXMock_SaveConfigAuditEvent_RequiresActor
• TestOverrideSnapshotJSON
• Handler tests in handler_override_audit_test.go:
  • PUT and DELETE go through the audited Tx methods with the session actor and never through the unaudited ones.
  • An audit failure fails the save.
  • An import records the linked events in key order.
  • Dry run, strict rejection and store failure leave no events.
  • A failure to write the import event writes no rows.
  • configAuditActor handles the admin API key sentinel and the API-key ID.

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.

Merge order: this adds migration 000147, so it must merge after #1505 (#1484, migration 000146).

Left out of scope

• No read API or UI for the trail. The issue only asks for the events to be recorded. A GET endpoint and an audit view would be a follow-up.
• The override PUT still merges on an unlocked read. saveAccountServiceOverride builds the row from the request plus existing.ID/created_at, read outside the transaction. The PUT is a full replace of the settings, so a stale read only affects the returned ID and created_at. The upsert keys on (account, provider, service), and the response now returns the stored row's ID. The audit before is always the row actually replaced, because it is read under the locks.
• The unaudited SaveAccountServiceOverride and DeleteAccountServiceOverride store methods are kept. No production code calls them any more. The mocks' fallbacks and the existing tests use them, so removing them would mean churning those tests.
• Deadlock risk with account deletion. An account delete cascades over that account's override rows without the advisory lock. An import, a PUT and an account delete that interleave in an unlucky order could deadlock. Postgres detects the deadlock and aborts one transaction; the import or PUT then fails with a 500 and can be retried. I judged this unlikely enough not to add more locking.

Maintainer: please mirror the issue's triage labels on this PR: priority/p3, type/feat. The issue has no severity/*, urgency/*, impact/*, effort/* or 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 723c88fc7eab71b67d4e6d58824e520c3e10eea5

Blocking:

• none

Non-blocking:

• internal/api/handler_override_import.go:333: sort.Slice is unstable. That's harmless today only because the planner rejects duplicate keys (duplicate of row %d, :216). sort.SliceStable would keep it safe if that rule is ever relaxed.
• Overrides removed by an account delete's ON DELETE CASCADE leave no config_audit_events row. The body calls this out of scope. If the trail is meant to answer "what happened to this account's overrides", an account-delete event is a natural follow-up.
• actor stores the user's email in a table with no retention or FK cascade, by design. Worth a line in whatever data-retention notes exist.

Attacked:

• Locking and audit correctness: PUT takes advisory(key), then FOR UPDATE, then upsert RETURNING. DELETE uses DELETE ... RETURNING (row lock only). The import writes in sorted key order inside one tx. I found no cycle between PUT, DELETE and import. A concurrent DELETE makes the queued save see before = nil. Every DB call inside the WithTx closures goes through tx (no nested pool acquire). A missing-row DELETE still returns success, as on main. Dry-run and strict-reject return before the tx. I found no other production writer of account_service_overrides.
• Migration 000147 matches the reservation. It only creates a new table and indexes, so there are no locks on existing tables, and down drops it. fix(db): make purchase_history.ramp_step and purchase_executions.error NOT NULL #1505 (000146) merged at 20:43Z. I merged origin/main into my scratch copy and ran: go build ./..., go vet -tags integration ./internal/..., the migrations package integration tests (including TestMigration_ConfigAuditEvents with 146 present) and TestOverrideAudit_: all ok.
• Fail-before / mutations (each reverted): PUT calling the unaudited SaveAccountServiceOverride fails TestSaveAccountServiceOverride_WritesThroughAuditedTx and _AuditFailureFailsTheSave. Dropping the import key sort fails TestImportOverrides_RecordsLinkedAuditEvents. Never inserting the event fails all 3 handler and all 3 store TestOverrideAudit_* integration tests.
• On the head: go build ./...; go vet with and without -tags integration on internal/api, config, mocks and migrations; go test -race -short -count=1 on those 4: ok. go test -race -tags integration for the override, import and migration tests in api, config and migrations: ok. golangci-lint v2.10.1 with and without the integration tag: 0 issues. gocyclo -over 10 on the changed non-test files: clean.

Body-stale:

• "it must merge after fix(db): make purchase_history.ramp_step and purchase_executions.error NOT NULL #1505": that condition is now met, since fix(db): make purchase_history.ramp_step and purchase_executions.error NOT NULL #1505 merged at 2026-10-26T20:43Z. The branch hasn't had main merged in yet, so on the branch alone previousMigrationVersion(147) is still 145. Merging main in makes it 146; I checked that combination passes.

_{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]

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

Blocking:

• none

Non-blocking:

• internal/api/handler_override_import.go (applyOverrideImport): the import event's summary comes from the plan built outside the transaction, so a concurrent PUT between planning and apply can make it disagree with the linked row events. For example, a row counted as update whose write turns out to be a no-op records no row event. The row events themselves are exact because they are read under the locks. A short note in the import event's details would help anyone reading the trail.
• 000147 up: config_audit_events has no retention or pruning, and the down migration drops the whole trail (the file says so). Both are acceptable for now. A retention policy belongs with the read API follow-up.
• The PR is a single 1306-line commit, above the repo's 400-line-per-commit guideline. It would only matter if it is squash-merged as is.
• An account delete cascading over override rows does not take the advisory lock, so it can deadlock with an import or PUT. The PR body already says so. Postgres aborts one side with a 500 and nothing is half-written.
• (Also raised in an earlier review.) An account delete that cascades over override rows leaves no config_audit_events row. If the trail must show every override disappearance, a follow-up issue should cover it.

Attacked:

• Lock correctness, reasoned through against Postgres READ COMMITTED: save vs delete, two first creates, two deletes, a save racing a create that has not committed yet, and import vs import in key order. I found no wrong before/after and no deadlock between two of these writers. Duplicate keys in one file are rejected during planning, so the unstable sort.Slice cannot change which row wins. A session with an empty email and user ID fails closed, because the audit insert refuses an empty actor. Tenant scope is unchanged: PUT and DELETE still call requireAccountAccess, and the import still uses scopedAccounts.
• Mutation probes (applied locally, then reverted). Each one failed its test:
  • Dropping the advisory lock failed SaveQueuesBehindFirstCreate.
  • Dropping FOR UPDATE failed SaveQueuesBehindDelete.
  • Dropping the no-op skip failed TestOverrideAudit_SinglePutAndDelete.
  • Fail-before, by putting main's unaudited SaveAccountServiceOverride back in the PUT handler: TestSaveAccountServiceOverride_WritesThroughAuditedTx, _AuditFailureFailsTheSave and the integration test TestOverrideAudit_SinglePutAndDelete all failed.
  • Writing the import in file order instead of key order failed TestImportOverrides_RecordsLinkedAuditEvents.
• At head 723c88f: go build ./...; go vet with and without -tags integration; go test -race -short -count=1 on api, config, mocks and migrations; go test -race -tags integration on the targeted tests; golangci-lint v2.10.1 with and without the integration tag (0 issues); gocyclo -over 10 on the changed files (clean).
• On a local merge of origin/main into head, which now has fix(db): make purchase_history.ramp_step and purchase_executions.error NOT NULL #1505's 000146 so 147 follows 146 for real: go test -race -tags integration -count=1 on all of internal/database/postgres/migrations (282s), internal/config, internal/api and internal/mocks. All passed. The branch merges cleanly.

Body-stale:

• "this adds migration 000147, so it must merge after fix(db): make purchase_history.ramp_step and purchase_executions.error NOT NULL #1505": fix(db): make purchase_history.ramp_step and purchase_executions.error NOT NULL #1505 merged at 2026-10-26T20:43Z, so the ordering constraint is already met. The branch is behind main but merges cleanly, and I verified 145 -> 146 -> 147 on the merged tree.

_{Reviewer: rF4. 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 723c88fc7eab71b67d4e6d58824e520c3e10eea5 merged onto main (3317da78), together with #1510 in that order (combined tree 10d8ea76, 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 723c88fc (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 723c88fc7eab71b67d4e6d58824e520c3e10eea5.