fix: keep the cost-usage database on transient SQLite failures, only rebuild on corruption (refs #2760) - #2765
Conversation
…rebuild on corruption (refs #2760) withDatabase treated every error as corruption and deleted the entire database. That destroyed user history on lock contention (app and CLI both open writable connections), disk-full, and even a plain NOT NULL constraint violation from appending rows for an unregistered path -- which then failed again on the fresh database and nuked it a second time. Classify SQLite primary result codes: transient/environment/data-shape errors (BUSY, LOCKED, FULL, NOMEM, IOERR, CONSTRAINT, READONLY, ...) now return the fallback and keep the file; corruption and schema drift (CORRUPT, NOTADB, ERROR, invalid data, incompatible schema) still rebuild. Rebuilds and preserved failures are now logged via the token-cost category so field rebuilds are observable, and a failed transaction that could not roll back drops the connection instead of leaving it wedged. Adds a failure-injection suite: constraint orphans, connection reuse after a rolled-back transaction, mid-file structural corruption, deleted -wal / stale -shm sidecars, zero-byte databases, the database path occupied by a directory, and a future schema version on disk.
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix: keep the cost-usage database on transient SQLite failures, only rebuild on corruption (refs #2760) This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 338db90cd5
ℹ️ 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".
| return opened | ||
| } catch { | ||
| self.rebuildDatabase() | ||
| self.rebuildDatabase(reason: "open failed: \(error)") |
There was a problem hiding this comment.
When a fresh CostUsageStore is opened while another app/CLI process holds the SQLite file locked, openDatabase() can throw SQLITE_BUSY from its setup/validation pragmas before any operation runs. This catch bypasses shouldRebuild(after:) and immediately calls rebuildDatabase, which removes the main DB and sidecars before the outer withDatabase classifier ever sees the transient error; CostUsageStoreAccess.read creates exactly such new store instances, so startup/read contention can still erase cost history.
Useful? React with 👍 / 👎.
1 participant
Adversarial review of the SQLite cost-store internals (#2761/#2762, refs #2760) found one real data-loss defect and an observability gap; this PR fixes them and adds a failure-injection test suite.
Defect: any SQLite error destroyed the whole database
CostUsageStore.withDatabase(default:)treated every thrown error as corruption: it deleted the database (plus-wal/-shm), recreated it, retried, and on a second failure deleted it again. Three reachable non-corruption triggers:appendTokenSnapshots/appendUsageRows/replaceBufferedLinesresolvefile_idvia(SELECT id FROM files WHERE path = ?); for an unregistered path that yields NULL into a NOT NULL column. The error nuked the DB, the retry against the fresh empty DB hit the same constraint, and the DB was nuked a second time — all history gone from one bad batch.CostUsageStoreAccess.read(used by the app, the CLI, and the project indexer) opens a writable connection. Cross-process contention past the 5s busy timeout returnedSQLITE_BUSYand deleted the database out from under the other process.SQLITE_FULLduring a write deleted the user's entire usage history, exactly when the old JSON path would have kept the previous artifact.Fix: classify the primary result code. Transient/environment/data-shape codes (
BUSY,LOCKED,NOMEM,READONLY,INTERRUPT,IOERR,FULL,PERM,TOOBIG,CONSTRAINT,MISUSE,AUTH,RANGE) return the fallback and keep the file; corruption/schema-drift codes (CORRUPT,NOTADB,ERROR,CANTOPEN, plusinvalidData/incompatibleSchema) keep the existing rebuild behavior, so the "dropped table degrades to fresh store" contract is unchanged. A preserved failure whose transaction cannot be rolled back drops the connection (file intact) instead of leaving it wedged.Observability
rebuildCountexisted but was never read outside tests — field rebuilds were invisible. Rebuilds now log a warning with the reason and running count through the existingtoken-costlog category, and preserved (non-rebuild) failures log too.Failure injection results
Known limitation (unchanged, documented in the test): value-only bit flips inside b-tree cells are undetectable without a checksumming VFS — quick_check verifies structure, not content. Budget behavior was re-audited: in-window trims under byte pressure mark catchUpPending and preserve previousReportPayload (the #2646 mitigation holds), and incremental vacuum reclaim is asserted by measurement in the existing suite.
Proof:
swift test --filter CostUsageStore(58 tests, 3 suites, all green) andmake checkclean; Codex autoreview clean.