fix: wrap the cost-store save cycle in one transaction so a crash mid-save is all-or-nothing (refs #2760)

[steipete]

Follow-up to the SQLite cost-store cutover (refs #2760), fixing defect D3 from the adversarial review on #2765: saveCodexCache performed many per-table writes without an enclosing transaction, so a crash mid-save could leave files upserted while day_aggregates stayed stale until the next scan cycle rewrote them. The old JSON path was atomic via single-file replace; the SQLite path now matches it.

What changed

• saveCodexCache's mutation phase (removed-file deletes → per-file persists → global day aggregates → metadata/discovery/lookback singletons) now runs inside one BEGIN IMMEDIATE/COMMIT via a new withSaveTransaction seam on the store.
• Nested transactions are flattened: inTransaction becomes a no-op wrapper when sqlite3_get_autocommit reports an open transaction, so the existing per-write transactions join the outer one instead of failing with a nested BEGIN (which would previously have been classified as corruption and nuked the database).
• Inside a save transaction, nested withDatabase calls join the outer connection scope and the first inner failure aborts the rest of the cycle. Without this, a failed statement that auto-rolled-back the transaction would let the remaining writes commit individually in autocommit mode — exactly the partial state the transaction exists to prevent. The original error propagates to the outer scope, which keeps the existing rebuild-vs-preserve classification from fix: keep the cost-usage database on transient SQLite failures, only rebuild on corruption (refs #2760) #2765.
• Budget enforcement stays outside the transaction by design: it runs PRAGMA wal_checkpoint(TRUNCATE) and incremental_vacuum, which SQLite forbids inside an open transaction. Its destructive path already marks catch-up pending, so interruption there self-heals on the next scan.

Kill-mid-save proof

New CostUsageStoreCrashSafetyTests spawns a real subprocess (CodexBarCostStoreCrashProbe, a test-only executable target) that seeds a store, then re-saves an updated cache and SIGKILLs itself deterministically inside the save transaction — after the first file's table writes were issued, before aggregates and metadata — via a test-only checkpoint hook. The test then reopens the database from the parent process and asserts the previous state survived byte-for-byte in shape (per-file days, file set, and global day aggregates).

Verified the test catches the bug: with the transaction wrapper temporarily removed, it fails showing exactly D3's torn state — one file updated to the new tallies, the removed file already deleted, and day_aggregates still holding the stale totals.

Proof

• swift test --filter CostUsageStore — 69 tests in 4 suites passed (includes the new crash-safety suite plus the existing store, failure-injection, and cutover suites).
• make check — clean.
• Codex autoreview (structured helper, local mode) — clean, no accepted findings.

[clawsweeper]

Codex review: needs changes before merge. Reviewed August 8, 2026, 4:28 PM ET / 20:28 UTC.

ClawSweeper review

What this changes

The branch wraps the Codex SQLite cost-cache save cycle in one transaction, adds a subprocess SIGKILL recovery test, and lengthens timeout-test delays.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Blocked by patch quality or review findings - 7 items remain

Keep this owner-authored PR open: its crash-path proof is useful, but both prior transaction-failure blockers remain at the unchanged current head.

Priority: P1
Reviewed head: ba2350c64d04eb7fb64dc4bfd0223448624efb8a

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The crash test targets the intended behavior well, but unhandled transaction failures leave the patch unsafe to merge.
Proof confidence	🌊 off-meta tidepool	Not applicable: The external-contributor proof gate does not apply to this owner-authored PR; its described subprocess SIGKILL/reopen check is nevertheless relevant runtime evidence.
Patch quality	🦪 silver shellfish (2/6)	2 actionable review findings remain.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The external-contributor proof gate does not apply to this owner-authored PR; its described subprocess SIGKILL/reopen check is nevertheless relevant runtime evidence.
Evidence reviewed	5 items	Outer transaction failure remains unhandled: The transaction-start helper discards its Boolean result; on a preserved BUSY or LOCKED failure it leaves no active transaction, while the following save mutations continue through ordinary database calls. Post-rollback writes remain possible: The save code ignores endSaveTransaction() and always runs budget enforcement and possible report-metadata writes after it, so a rolled-back transaction can still be followed by destructive or derived writes. Prior findings are still at the reviewed revision: The current PR head is the same ba2350c revision reviewed previously, so the two earlier blockers have not been changed away.
Findings	2 actionable findings	[P1] Abort the save when the outer transaction cannot start [P2] Skip post-save mutations after transaction failure
Security	None	None.

How this fits together

Codex usage scans save per-session data, aggregates, and scan metadata in a local SQLite cache that later feeds app and CLI cost reports. Budget enforcement runs after the main save cycle to retain the cache within its limits.

flowchart LR
A[Session scan] --> B[Cost cache save]
B --> C[SQLite transaction]
C --> D[Per-file records]
C --> E[Aggregates and metadata]
E --> F[Budget enforcement]
F --> G[App and CLI reports]

Loading

Before merge

• Abort the save when the outer transaction cannot start (P1) - beginSaveTransaction() discards its failure result. On a preserved BUSY or LOCKED error it never installs activeTransactionDatabase, but the following deletes and writes run through normal autocommit calls; if the lock clears mid-cycle, this reintroduces the partial-save outcome. Return success and exit before the first mutation when BEGIN fails.
• Skip post-save mutations after transaction failure (P2) - The result of endSaveTransaction() is ignored. After an inner write or COMMIT failure rolls back the main cycle, enforceBudgets can still prune rows and the report path can persist metadata derived from the unsaved cache. Gate both paths on transaction success to preserve the previous state.
• Resolve merge risk (P1) - If BEGIN IMMEDIATE fails transiently and the lock clears during the save, subsequent writes can commit independently and recreate the torn state this PR is intended to prevent.
• Resolve merge risk (P1) - After an inner write or commit failure, budget enforcement and report metadata writes can still alter the previously intact database outside the rolled-back save transaction.
• Improve patch quality - Return and honor transaction-start and transaction-end success.
• Improve patch quality - Add regression tests for a failed BEGIN IMMEDIATE and a failed inner write, verifying no budget or metadata mutation follows.
• Improve patch quality - Run the repository-required focused store suite, full test suite, and check after the repair.

Findings

• [P1] Abort the save when the outer transaction cannot start — Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift:35
• [P2] Skip post-save mutations after transaction failure — Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift:55-67

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production versus test delta	production +199/-2; tests +107/-9; changelog +1	Most growth is a dedicated crash harness and coverage, while the persistence path itself changes in three core source files.
Patch surface	9 files changed; 307 added, 11 removed	The atomic-save repair also introduces a test-only executable and adjusts nine service timeout tests.

Merge-risk options

Maintainer options:

1. Repair failure gating before merge (recommended)
   Stop the save before any mutation when the outer transaction cannot start, and skip budget/report writes whenever the transaction ends unsuccessfully.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Return transaction-start and transaction-end success to saveCodexCache, gate all post-save writes on success, and add BUSY/LOCKED-start plus nested-write failure regression coverage.

Technical review

Best possible solution:

Return and honor transaction-start and transaction-end success, skipping all later save-cycle mutations on failure; add focused BUSY/LOCKED-start and nested-write-failure regression coverage.

Do we have a high-confidence way to reproduce the issue?

Yes. The subprocess harness establishes the crash path, and a second SQLite writer can force BEGIN IMMEDIATE to fail while focused failure injection can exercise an inner write failure.

Is this the best way to solve the issue?

No. The enclosing transaction is the right design, but its failed-start and failed-end results must stop all later mutations for the save to be all-or-nothing.

Full review comments:

• [P1] Abort the save when the outer transaction cannot start — Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift:35
beginSaveTransaction() discards its failure result. On a preserved BUSY or LOCKED error it never installs activeTransactionDatabase, but the following deletes and writes run through normal autocommit calls; if the lock clears mid-cycle, this reintroduces the partial-save outcome. Return success and exit before the first mutation when BEGIN fails.
  Confidence: 0.99
• [P2] Skip post-save mutations after transaction failure — Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift:55-67
  The result of endSaveTransaction() is ignored. After an inner write or COMMIT failure rolls back the main cycle, enforceBudgets can still prune rows and the report path can persist metadata derived from the unsaved cache. Gate both paths on transaction success to preserve the previous state.
  Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4c2b9217af72.

Labels

Label changes:

• add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🌊 off-meta tidepool and patch quality is 🦪 silver shellfish.
• remove rating: 🧂 unranked krab: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

• P1: Failure handling can still leave persisted cost-session state partially updated after a transaction cannot start or rolls back.
• merge-risk: 🚨 compatibility: The branch changes save behavior for existing on-disk SQLite cost caches.
• merge-risk: 🚨 session-state: The affected save cycle must keep per-file session data, aggregates, and metadata mutually consistent.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🌊 off-meta tidepool and patch quality is 🦪 silver shellfish.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The external-contributor proof gate does not apply to this owner-authored PR; its described subprocess SIGKILL/reopen check is nevertheless relevant runtime evidence.

Evidence

Acceptance criteria:

• [P1] swift test --filter CostUsageStore.
• [P1] make test.
• [P1] make check.

What I checked:

• Outer transaction failure remains unhandled: The transaction-start helper discards its Boolean result; on a preserved BUSY or LOCKED failure it leaves no active transaction, while the following save mutations continue through ordinary database calls. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift:35, ba2350c64d04)
• Post-rollback writes remain possible: The save code ignores endSaveTransaction() and always runs budget enforcement and possible report-metadata writes after it, so a rolled-back transaction can still be followed by destructive or derived writes. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift:55, ba2350c64d04)
• Prior findings are still at the reviewed revision: The current PR head is the same ba2350c revision reviewed previously, so the two earlier blockers have not been changed away. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore.swift:198, ba2350c64d04)
• Cost-store ownership history: The SQLite cutover and its subsequent transient-failure and retention fixes were merged from commits authored by Peter Steinberger, establishing a direct history trail for this persistence surface. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore.swift:154, 5205dd1e003c)
• Current main does not contain this transaction wrapper: The current main save path has no beginSaveTransaction/endSaveTransaction calls, so this PR's central atomic-save behavior is not already implemented or released. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift:35, 4c2b9217af72)

Likely related people:

• steipete: This author owns the merged SQLite cutover and the subsequent persistence-safety fixes on the current cost-store paths. (role: introduced and recently maintained the SQLite cost-store behavior; confidence: high; commits: 26fd0bbd7eed, 5205dd1e003c, 38ca30ab93bc; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (7 earlier review cycles)

• reviewed 2026-08-08T17:25:18.282Z sha 13aae64 :: needs maintainer review before merge. :: none
• reviewed 2026-08-08T17:37:46.198Z sha 3c93eab :: needs changes before merge. :: [P2] Skip post-save writes after a failed transaction
• reviewed 2026-08-08T17:52:17.192Z sha 5e3caed :: needs changes before merge. :: [P2] Gate post-save writes on transaction success
• reviewed 2026-08-08T18:22:28.601Z sha 5e3caed :: needs changes before merge. :: [P2] Gate post-save writes on transaction success
• reviewed 2026-08-08T18:30:27.463Z sha 4164d33 :: needs changes before merge. :: [P1] Abort when the outer transaction cannot start | [P2] Gate post-save writes on transaction success
• reviewed 2026-08-08T19:07:09.128Z sha 9788707 :: needs changes before merge. :: [P1] Abort the save when the outer transaction cannot start | [P2] Skip post-save writes when the transaction fails
• reviewed 2026-08-08T20:24:31.061Z sha ba2350c :: needs changes before merge. :: [P1] Abort the save when the outer transaction cannot start | [P2] Skip post-save writes when the transaction fails

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 13aae64b74

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Skip budget writes after a failed save transaction

When the enclosing save transaction returns false after BEGIN IMMEDIATE or a nested write fails with a preserved error, this result is discarded and the method still runs budget enforcement and the catch-up metadata write. If the existing store is over the row/byte budget in that failure path, those follow-up writes can prune rows or persist a previousReportPayload derived from the unsaved in-memory cache, so the previous on-disk state is no longer the all-or-nothing rollback state this transaction is meant to preserve. Gate the budget/report phase on the transaction succeeding.

Useful? React with 👍 / 👎.