Skip to content

fix(wal): recover a connection that has fallen behind more than one WAL generation - #349

Open
darshanp40 wants to merge 2 commits into
rhashimoto:masterfrom
darshanp40:fix/skiptx-permanent-desync
Open

darshanp40 wants to merge 2 commits into
rhashimoto:masterfrom
darshanp40:fix/skiptx-permanent-desync

Conversation

@darshanp40

Copy link
Copy Markdown

Fixes #345.

The bug

#skipTx only ever accepted a broadcast whose WAL file generation (salt1) was exactly one ahead of the connection's own — #followFileChange checks salt1 + 1 and nothing else. A connection more than one swap behind throws invalid WAL file.

That alone would be recoverable, except #advanceTxId deletes the transaction from #mapIdToPendingTx before #skipTx can throw. Once it does, the id is gone for good — nothing else ever re-adds a given id once its broadcast has been seen — so #txId can never advance past it. Every later broadcast then falls through to #readTx(), gets null, and #activateTx(null) dereferences it. #advanceTxId/rejoin() are both plain synchronous functions, so when that crash happens during rejoin() or isolateForWrite()'s own catch-up call, it propagates synchronously into jUnlock/jLock's try/catch and surfaces as disk I/O error.

A quieter variant of the same gap produces database disk image is malformed instead of a throw: at exactly one generation behind, the +1 check can match a real file by coincidence — the wrong one relative to what the transaction actually names — and #skipTx never verifies the adopted file against tx.waSalt1. It silently continues with #activeHandle pointing at one file and #activeOffset describing a position in a different one. The next real page read pulls real, checksum-valid bytes from the wrong file at the wrong offset, and SQLite's own page-structure check reports corruption.

The fix

Three changes in src/examples/WriteAhead.js, all interdependent:

  1. #skipTx adopts by verified salt match, not by generation hop. A new #adoptFileForSalt1(targetSalt1) checks both physical WAL files' real on-disk headers for the one that actually holds the target salt, instead of assuming a single hop forward. There are only ever two physical files, so if the transaction is still recoverable from disk at all, one of them names it exactly — this is what closes the malformed case, since it never adopts on a coincidental match anymore, only a confirmed one.
  2. The pending-map delete moves to after #skipTx succeeds. If it still throws (neither file matches — genuinely unrecoverable from disk), the id stays queued instead of being lost forever, so a retry can make progress instead of repeating the identical failure at that #txId indefinitely.
  3. #advanceTxId never calls #activateTx(null). If #readTx() finds nothing at the current position, it stops advancing for that call instead of crashing; the pending entries stay queued for the next broadcast or the backstop's readToCurrent pass.

Reproduction and verification

repro-345/ has a harness that reproduces all three symptoms deterministically — no real multi-tab reload race, no timing luck. It engineers the exact internal precondition each one needs and delivers it through the real, unmodified #handleMessage path via a genuine BroadcastChannel message (same shape a real peer's broadcast takes; nothing forged beyond ordinary public fields, no SDK internals called directly).

Results from this session:

Scenario Result
invalid WAL file 5/5 runs
disk I/O error 2/2 runs
database disk image is malformed 3/3 runs
Fix, real broadcasts only (no synthetic data) sawThrow=false sawUncaught=null readError=null writerRows=500 victimSees=500

repro-345/vendor/ is the unpatched code plus a handful of test-only fault-injection hooks (not part of the fix) that make the reproduction deterministic; repro-345/vendor-fixed/ is the same with this PR's fix applied. repro-345/README.md has the full writeup and exact steps to run it (python3 -m http.server + open the HTML pages in a Chromium browser).


@simolus3's reproduction repo and root-cause writeup on the issue is what let us pin down the exact mechanism — this PR follows directly from that.

…AL generation

#skipTx only ever accepted a broadcast whose file generation was exactly
one ahead of the connection's own (#followFileChange checked salt1 + 1
and nothing else), and #advanceTxId deleted the pending transaction from
#mapIdToPendingTx before #skipTx could throw -- so a connection more than
one swap behind lost the transaction permanently and could never advance
past that txId. Every later broadcast then fell through to #readTx(),
returned null, and #activateTx(null) dereferenced it.

A quieter variant of the same gap produced silent corruption instead of a
throw: at exactly one generation behind, the salt1 + 1 check can match a
real file by coincidence -- the wrong one relative to what the incoming
transaction actually names -- and #skipTx never verified the file it
adopted against the transaction's own salt.

Three changes:
 - #skipTx now adopts whichever physical WAL file's real on-disk header
   actually matches the transaction's salt (a new #adoptFileForSalt1),
   verified by reading it, instead of assuming a single generation hop.
   There are only ever two physical files, so if the transaction is still
   recoverable from disk at all, one of them names it exactly.
 - the pending-map delete happens only after #skipTx succeeds, so a throw
   leaves the id queued for a retry instead of losing it forever.
 - #advanceTxId stops advancing instead of calling #activateTx(null) when
   #readTx() finds nothing at the current position.

Fixes rhashimoto#345.

Reproduced and the fix verified deterministically; see repro-345/ in this
PR for the harness and full writeup.
…fication

Reproduces all three reported symptoms -- invalid WAL file, disk I/O
error, database disk image is malformed -- without a real multi-tab
reload race: engineers the exact internal precondition each one needs
and delivers it through the real, unmodified #handleMessage path via a
genuine BroadcastChannel message.

repro-345/vendor/       unpatched WriteAhead.js + OPFSWriteAheadVFS.js,
                        with test-only fault-injection hooks (not part
                        of the fix) -- demonstrates the bug.
repro-345/vendor-fixed/ the same, with the fix applied -- demonstrates
                        it resolving.
repro-345/harness/      the reproduction pages themselves.

See repro-345/README.md for the mechanism, how to run it, and results
from this session: invalid WAL file 5/5, disk I/O error 2/2, malformed
3/3, and a clean end-to-end run against the fix with zero synthetic
data (a connection misses a real swap and every real transaction while
paused, then self-heals with the correct row count once unpaused).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant