Skip to content

fix(query-history): recover from corruption + atomic writes#253

Merged
debba merged 3 commits into
mainfrom
fix/query-history-corruption-recovery
May 25, 2026
Merged

fix(query-history): recover from corruption + atomic writes#253
debba merged 3 commits into
mainfrom
fix/query-history-corruption-recovery

Conversation

@debba
Copy link
Copy Markdown
Collaborator

@debba debba commented May 25, 2026

Summary

  • The per-connection query history JSON could be corrupted by concurrent addEntry calls (one per statement in multi-statement scripts) racing on fs::write. Once corrupted, serde_json::from_str rejected 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_history now 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_history is now atomic — write to a per-write temp file in the same directory, then rename onto the target — so concurrent or crashed writes can never leave the target half-written.
  • A per-connection async mutex (QueryHistoryState) serialises read-modify-write sequences so concurrent add_query_history_entry calls can't lose updates either.
  • get_query_history returns the new QueryHistoryResponse { 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 — clean
  • pnpm lint — clean
  • pnpm test --run — 2425 tests pass
  • pnpm build — clean
  • Manual: corrupt ~/Library/Application Support/tabularis/query_history/<id>.json by appending garbage, open the app → banner appears with the backup path, new queries get recorded
  • Manual: run a multi-statement script (5+ statements) on MySQL, verify every statement is recorded and the file stays valid JSON

debba added 2 commits May 25, 2026 20:23
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.
@kilo-code-bot
Copy link
Copy Markdown

kilo-code-bot Bot commented May 25, 2026

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The previous WARNING regarding missing per-connection lock protection in backfill_missing_database_for_connection and remove_history_for_connection has been resolved in commit 68dcbd5.

  • Both functions are now async and acquire the QueryHistoryState lock before file I/O.
  • Callers in commands.rs now properly .await these operations.
Files Reviewed (2 files)
  • src-tauri/src/query_history.rs — previous issue resolved
  • src-tauri/src/commands.rs.await added for history operations

Reviewed by kimi-k2.6-20260420 · 107,691 tokens

@debba debba merged commit c2b5598 into main May 25, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant