Repository navigation
fix(utils): recover corrupt SQLite stores whose corruption surfaces as another error - #13530
Conversation
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
P1: correct, narrowly scoped root-cause fix. After a failed init, revealHiddenCorruption sends quick_check-detected damage to the existing quarantine path. The probe runs only when recoverCorruption is set and init has already failed, and busy and corruption errors skip it. I checked the regression test: it fails against origin/main's sqlite.ts (no such table: added) and passes on the branch (8/8).
Before merge: the changelog entry has no #13530/@Hunter-124 attribution, and packages/utils/CHANGELOG.md, where the change actually lives, has no entry.
Should-fix: the regression test exercises the probe only while oven-sh/bun#37415 is unfixed. A case that doesn't depend on Bun's behavior would keep the probe covered after the Bun fix ships. One small nit on the shape of the synthesized error.
Thanks @Hunter-124 for the detailed root-cause write-up and the end-to-end check against a real corrupt agent.db.
|
|
||
| ### Fixed | ||
|
|
||
| - Fixed a corrupt `agent.db` crashing every launch with `no such table` (for example `hint_usage`) instead of being preserved and recreated ([#13530](https://github.com/can1357/oh-my-pi/pull/13530) by [@Hunter-124](https://github.com/Hunter-124)) |
There was a problem hiding this comment.
should-fix: two changelog gaps.
- External contributions need attribution per
AGENTS.md: append([#13530](https://github.com/can1357/oh-my-pi/pull/13530) by [@Hunter-124](https://github.com/Hunter-124)). The PR checklist says attribution was added, but this line has none. - The code change is in
packages/utils/src/sqlite.ts, andpackages/utils/CHANGELOG.md## [Unreleased]has no entry. The fix also covers therecoverCorruptionstores inpi-ai(sqlite-credential-store.ts) andpi-catalog(model-cache.ts), not onlyagent.db, so api-utilsentry such as "openSqliteDatabase/openSqliteDatabaseSyncwithrecoverCorruptionnow recover stores whose corruption surfaces as a non-corruption init error" would describe it accurately.
There was a problem hiding this comment.
Fixed in d58b5e7 and 32cc839: the pi-coding-agent entry now carries the #13530 / @Hunter-124 attribution, and packages/utils/CHANGELOG.md has a [Unreleased] → Fixed entry describing the recoverCorruption behavior for all stores.
| const initialize = (db: Database) => { | ||
| // Bun reports only the final statement's step error, so the failed CREATE is dropped here... | ||
| db.run("CREATE TABLE IF NOT EXISTS added (value TEXT); CREATE TABLE IF NOT EXISTS existing (value TEXT);"); | ||
| // ...and resurfaces as SQLITE_ERROR, which on its own would not trigger recovery. | ||
| db.prepare("SELECT value FROM added").finalize(); |
There was a problem hiding this comment.
should-fix: this regression test only reaches revealHiddenCorruption while oven-sh/bun#37415 is unfixed. After oven-sh/bun#37418 ships, the multi-statement db.run() at line 222 will throw SQLITE_CORRUPT directly. The test will keep passing through the old isSqliteCorruptionError path, and nothing will cover the probe.
A companion case that does not depend on Bun's behavior would pin the new contract: use the same freelist-damaged fixture, and have the initializer allocate nothing and just throw new Error("init failed"). That checks that a healthy-looking read path plus a failing quick_check still rotates the store, and the ok-branch is already covered by opt-in recovery never rotates a non-corruption SQLite failure. (Before-fix check: this test fails against origin/main's sqlite.ts with no such table: added and passes on the branch, 8/8.)
There was a problem hiding this comment.
Added in 32cc839: a non-corruption init failure on a store that fails quick_check is preserved as corruption uses the same freelist-damaged fixture with an initializer that allocates nothing and throws init failed on the first attempt. It asserts the store is rotated (single backup, byte-identical), the second attempt runs on the fresh file, and the preserved error is SQLITE_CORRUPT with the init error as cause. It fails against origin/main's sqlite.ts and passes on the branch.
| const original = error instanceof Error ? error.message : String(error); | ||
| return Object.assign( | ||
| new Error(`database disk image is malformed (${problem}); initialization failed: ${original}`), | ||
| { | ||
| code: "SQLITE_CORRUPT", | ||
| errno: 11, | ||
| cause: error, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
nit: Object.assign(new Error(msg), { cause }) sets cause as an ordinary enumerable property instead of using ErrorOptions. new Error(msg, { cause: error }) followed by assigning code/errno is the standard form. Separately, when the probe itself throws corruption (line 76), the original init error is dropped entirely, but the synthesized branch keeps it as cause. Wrapping both the same way would keep the original failure visible when triaging preserved stores.
There was a problem hiding this comment.
Fixed in 32cc839: the synthesized error is now new Error(msg, { cause }) with code/errno assigned afterwards. A probe that itself throws corruption is wrapped the same way, keeping the probe error's code/errno and message and attaching the original init error as cause.
3152598 to
fa0a90a
Compare
…s another error Bun's multi-statement db.run() only reports the final statement's step error (oven-sh/bun#37415). A corrupt agent.db made the hint_usage CREATE fail with SQLITE_CORRUPT mid-script; Bun dropped it, the later prepare failed with "no such table: hint_usage" (SQLITE_ERROR), and recoverCorruption never fired, so omp crashed on every launch. When an initializer with recoverCorruption fails for any non-corruption, non-busy reason, run PRAGMA quick_check on the still-open handle; if the file is damaged, route it through the existing quarantine-and-recreate path. Healthy stores pay nothing: the probe only runs after a failure.
…ruption Build the synthesized corruption error with ErrorOptions instead of an enumerable cause property, and wrap a probe that itself throws corruption the same way so the original initialization failure stays attached. Add a regression case that does not depend on Bun dropping mid-script step errors: a plain initializer failure on a store whose quick_check fails must still be preserved and recreated. Add the pi-utils changelog entry with PR attribution.
fa0a90a to
32cc839
Compare
3 participants
What
I hit this on my own machine: omp crashed on every launch with
no such table: hint_usagebecause a corruptagent.dbnever reached the recovery path, and this change makes that corruption get detected so the store is preserved and recreated the wayrecoverCorruptionintends.When a store opened with
recoverCorruptionfails to initialize with a non-corruption, non-busy error,openSqliteDatabase/openSqliteDatabaseSyncnow runPRAGMA quick_check(1)on the still-open handle. If the file is damaged, the failure is routed through the existing quarantine-and-recreate path instead of surfacing as a hard crash.Why
My
~/.omp/agent/agent.dbhad a damaged freelist (Freelist: invalid page number 218103808, out-of-order rowids inusage_history). On 18.3.5 every launch crashed with:AgentStorage.#initializeSchemacreates its tables in one multi-statementdb.run(). Allocating a page for the newhint_usagetable hit the bad freelist and failed withSQLITE_CORRUPT, but Bun only reports the step error of the final statement in a multi-statementrun()(oven-sh/bun#37415, fix pending in oven-sh/bun#37418; still present in Bun 1.4.2). The trailingCREATE TABLE IF NOT EXISTS schema_versionsucceeded as a no-op, so the error was dropped, and the nextpreparefailed with a plainSQLITE_ERROR.isSqliteCorruptionErroris false for that, sorecoverCorruptionnever fired.Fixing it centrally in
pi-utilscovers everyrecoverCorruptionstore (agent.db, auth credentials, model cache, history), all of which use multi-statementdb.run()DDL, and keeps working on Bun versions without the upstream fix. The probe only runs after an initializer has already failed, so the happy path is unchanged. On an unrelated init failure against a healthy store it costs onequick_check(1)pass before the original error is rethrown.Testing
packages/utils/test/sqlite.test.tsreproduces the real corruption (freelist trunk pointer past EOF, which only a page allocation trips) behind a multi-statement script. It fails onmainwithno such table: addedand passes with the fix. The recreated store is usable.packages/utils/test/sqlite.test.ts8/8, agent-storage + history-storage suites 38/38,auth-storage-sqlite-busy7/7.bun run checkinpackages/utilsis clean (oxlint, oxfmt, tsgo).agent.dbwithPI_CODING_AGENT_DIRpointed at a temp dir andomp -p --no-session --no-tools "hi":no such table: hint_usageagent.db.corrupt-<ts>-<uuid>(+ sidecars), recreatesagent.db(integrityok,hint_usagepresent), and answers the promptbun checkpasses (packages/utils)