fix(query-history): recover from corruption + atomic writes#253
Merged
Conversation
Concurrent addEntry calls (one per statement in multi-statement scripts) raced on fs::write and could leave the per-connection history JSON with a trailing tail from a longer prior write. Once corrupt, serde_json::from_str refused to parse and every subsequent read/write failed silently — the History panel went empty and no new entries were ever recorded. - read_history now backs up corrupt files to <id>.json.corrupt-<ts> and returns an empty list so the app self-heals instead of staying mute. - write_history is atomic: write to a per-write temp file, then rename onto the target so concurrent writes never interleave. - Per-connection async mutex serialises read-modify-write sequences. - get_query_history returns the backup path so the sidebar can surface a banner explaining what happened and where the data was preserved.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The previous WARNING regarding missing per-connection lock protection in
Files Reviewed (2 files)
Reviewed by kimi-k2.6-20260420 · 107,691 tokens |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
addEntrycalls (one per statement in multi-statement scripts) racing onfs::write. Once corrupted,serde_json::from_strrejected the trailing tail and every subsequent read/write failed silently, leaving the History panel empty and never recording new entries again — reproduced locally on macOS/APFS.read_historynow self-heals: a parse failure renames the bad file aside as<id>.json.corrupt-<timestamp>and returns an empty list, so the app keeps working instead of staying mute.write_historyis now atomic — write to a per-write temp file in the same directory, thenrenameonto the target — so concurrent or crashed writes can never leave the target half-written.QueryHistoryState) serialises read-modify-write sequences so concurrentadd_query_history_entrycalls can't lose updates either.get_query_historyreturns the newQueryHistoryResponse { entries, recoveredBackupPath }. The sidebar surfaces a dismissible amber banner with the backup file path so users know what happened and where their data went.Background
While debugging "history doesn't work anymore" on a Mac install I found the on-disk file ended with valid JSON
]followed by 46 bytes of leftover content (]ll,\n "database": ...) — clearly the tail of a prior longer write that wasn't truncated. The user-visible symptom was a permanently empty History panel because the parse error short-circuited both reads and writes.Test plan
cargo test --lib query_history— 7 tests pass (3 new: atomic truncation, no leftover tmp on success, corrupt-file backup naming)pnpm typecheck— cleanpnpm lint— cleanpnpm test --run— 2425 tests passpnpm build— clean~/Library/Application Support/tabularis/query_history/<id>.jsonby appending garbage, open the app → banner appears with the backup path, new queries get recorded