fix(storage): converge concurrent fresh WAL initialization - #2003
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Review: MERGE-READY ✅ (independent deepseek-v4-flash review)
Reviewed the full diff (+103/−18, 3 files, all in packages/storage) and reproduced the race locally.
- Fix verified effective: on main's old logic (busy_timeout + single journal-mode switch), a two-process probe failed 3/12 rounds with
database is locked; with the PR'sensureWalJournalMode+ bounded 5s retry, 0/12 failures, convergence ≤269ms. The mechanism matches the repo's existing precedent from #1770 (sqlite-long-term-memory-schema.tsensureWalJournalMode), and journal-mode conversion is serialized by SQLite's header lock — no corruption/lost-update window; fail-closed preserved (non-BUSY errors throw immediately). - Test quality good: new real-child-process × production entry (
acquireOperationalStateDatabase()) test with file-barrier sync, 12 rounds, asserting both process exit codes,journal_mode='wal',user_version=8; passes on CI (test_workspaces). No schema/protocol change, clean single-commit revert. - Storage jobs all green (
test_workspaces,test_runtime_host,test_headless, typecheck).
Non-blocking P3 (optional): (1) ensureWalJournalMode is now duplicated in two schema files (maintenance-drift risk — consider a shared helper or cross-referencing comments); (2) the race test is probabilistic and doesn't assert that a round actually hit contention; (3) the 'memory' early-return is a dead branch for file DBs.
Merge prerequisite: the only red check (e2e_shard (1/2) — plan-reminders.spec.ts:86 toBeFocused()) is a known pre-existing flake: the same test fails identically on main (run 30811251917), the repo's playwright.config.ts:13-16 documents toBeFocused() failing on window-blur with retries: 0 by design, and this PR touches zero desktop code (shard 2/2 is green). Please re-run the e2e job (or apply the repo's established flake handling) before merging; the PR itself needs no changes.
|
I reviewed the approval and reran the failed E2E shard as requested. The rerun reproduced the exact same pre-existing desktop focus flake:
This PR changes only three storage files, and all storage/runtime/typecheck checks remain green. I am leaving the three optional P3 observations out of this PR so the WAL initialization fix keeps a single invariant and a clean revert boundary. No production change is warranted in #2003 for this rerun result; the remaining red status is the repository's existing UI focus flake. 中文说明已按 review 建议重跑失败的 E2E shard,结果再次复现完全相同的既有 Desktop 焦点 flaky:
本 PR 只修改三个 storage 文件,storage/runtime/typecheck 检查仍全部通过。三个可选 P3 不并入本 PR,以保持 WAL 初始化修复只有一个主要不变量和清晰回滚边界。 因此此次重跑结果不需要修改 #2003 的生产代码;剩余红灯来自仓库既有 UI focus flaky。 |
|
Merged — thanks for the fix! Heads-up on why the CI was red: it was never this PR's code. The branch was based on a pre-fix main snapshot (11:39, before #2001 merged at 11:58), so the PR's CI kept running the stale merge-ref — e2e_shard (1/2) failed on One optional follow-up from the independent review (pre-existing, not blocking this PR): the same concurrent first-open WAL race still lives in |
Summary
SQLITE_BUSY/ locked failures within the existing 5-second initialization boundacquireOperationalStateDatabase()Invariant and boundary
Compatibility
upstream/main(72027d2bbat implementation time)Verification
database is locked@maka/storagebuild and formatting checks passedThe full storage suite was also attempted on local Windows. Unrelated tests hit the repository's existing parallel temp-cleanup
EBUSYfailures while deleting openruntime.sqlite[-shm]files; the focused tests above completed cleanly.Fixes #1544
中文说明
变更摘要
database is locked的问题SQLITE_BUSY/ locked 做有界重试,沿用 5 秒初始化上限acquireOperationalStateDatabase()同时起跑,共重复 12 轮不变量与边界
兼容性验证
upstream/main平铺实现测试
database is locked本机 Windows 也尝试了完整 storage suite;若干无关测试在并行删除仍被占用的
runtime.sqlite[-shm]临时文件时触发既有EBUSY,不属于本次 WAL 初始化回归。上述定向测试均已通过。