[travsr-store][travsr-daemon] Fix #509: key embed.db freshness on file identity, not existence - #771
Open
ritikpal1122 wants to merge 3 commits into
Open
[travsr-store][travsr-daemon] Fix #509: key embed.db freshness on file identity, not existence#771ritikpal1122 wants to merge 3 commits into
ritikpal1122 wants to merge 3 commits into
Conversation
…ess on file identity, not existence embed_data_version cached a read-only Connection to .travsr/embed.db and dropped it only when path.exists() was false. Deleting embed.db to force a rebuild (a normal user recovery) and letting the sidecar recreate it leaves the path present at every poll, so the connection stayed pinned to the unlinked inode and its PRAGMA data_version was frozen for the daemon's lifetime. Every ask entry warmed before the swap kept hitting until restart. Reopen on file identity instead: unix uses (st_dev, st_ino), which is exact because the old inode cannot be reused while the connection still holds it open. Windows has no stable equivalent (MetadataExt::file_index is unstable behind windows_by_handle, rust-lang/rust#63010), so it uses the stable (creation_time, last_write_time, file_size) triple, which errs toward extra reopens rather than toward keeping a dead handle. Reopening alone is not enough: a fresh connection to the rebuilt file begins its own data_version count from a low value, so both sides of the swap read 2 and collide. The returned value is now a token mixing the identity with the pragma, compared for equality only, which is all the query cache does with it. Regression test asserts the reported version moves across a delete-and-recreate and that the reopened connection still tracks writes to the new file. It fails on the pre-fix code with "left: 2, right: 2".
…verage The delete-and-recreate test failed on windows-latest with os error 32, the file being in use. That is the platform refusing the premise: SQLite opens db files on Windows without FILE_SHARE_DELETE, so unlinking embed.db under the live read-only connection is denied rather than succeeding and stranding the connection on an unlinked inode. The failure mode the test reproduces cannot happen there, so the test is now unix only, with that reason recorded rather than a bare cfg. Writing that comment surfaced a worse problem. It claimed the Windows arm of file_identity was "covered by the identity unit tests", and there were none: that arm had no test on any platform, which is how a Windows-only regression would have reached a release. So there is one now. file_identity_changes_when_the_file_is_replaced runs everywhere, because it swaps the file before reading identity and never holds it open, which is exactly what the store-level test cannot do on Windows. It asserts the tuple differs rather than any one member: on unix the inode carries it, and on Windows NTFS tunneling can carry creation time onto a same-named replacement, leaving the other two members to carry it. It also pins identity as stable for an unchanged file, so it cannot invalidate the cache on every poll, and as absent for a missing path. Mutation checked: with file_identity returning a constant the new test fails on the replacement assertion.
CI failed on ubuntu at the replacement assertion: ext4 handed the recreated file the inode it had just released, so (dev, ino) was unchanged and the replacement looked like the original. It passed on macOS, which allocated a fresh one. Inode reuse is real, and the reason it cannot reach production is that the cached read-only Connection is open across the swap. POSIX cannot free an unlinked inode while a descriptor still refers to it, so its number cannot be reallocated to the replacement. The test held no handle, so it was asserting against a situation the code never meets, and it was the test that was wrong rather than the fix. It now holds a File across the swap, mirroring the connection, and the comment says why the pin is load-bearing rather than setup, so it is not tidied away later.
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
1 participant
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.
Closes #509.
The bug
embed_data_versioncached a read-only connection to.travsr/embed.dband dropped it only whenpath.exists()was false. A delete-and-recreate between two polls keepsexists()true, so the stale connection went on reading the unlinked inode andPRAGMA data_versionfroze for the daemon's lifetime. Warmaskentries cached before the swap kept hitting, clearable only by restarting the daemon. Deleting.travsr/embed.dbto force a rebuild is a normal user recovery, so this is reachable in ordinary use.Reopening alone does not fix it
The non-obvious half, and the reason this is not a one-line change. Detecting the swap and reopening still fails: the fresh connection to the rebuilt file starts its own
data_versioncount low, and collides with the frozen reading. Both sides read2and the cache still hits.Proved by mutation, keeping the identity check and returning the raw pragma:
So the fix is identity-based reopen plus mixing the file identity into the returned value.
Contract change
The return is now an opaque token rather than the raw pragma. Verified safe: every consumer compares it for equality only.
CacheKeyderivesPartialEq, Eq, Hashand nothing orders it, so no consumer relies on the value increasing. The doc now says the token is equality-only and carries no ordering.Windows
No stable inode equivalent exists (
file_index/volume_serial_numberare still unstable,windows_by_handle, rust-lang/rust#63010), so identity is gated per platform. Windows is not left on the old behaviour: it uses the stable(creation_time, last_write_time, file_size)triple.That is a heuristic, not an identity. NTFS tunneling can carry a deleted file's creation time onto a same-named replacement created within ~15s; in that window the other two members carry the comparison. It errs toward extra reopens (any write moves
last_write_time), never toward keeping a dead handle, which is the safe direction: a spurious reopen costs one cache miss, a missed one serves stale answers forever.Also worth noting SQLite opens db files on Windows without
FILE_SHARE_DELETE, so unlinking embed.db under a live connection fails outright there; the branch covers rename-over and already-reopened cases. Not compile-checked on Windows - onlyaarch64-apple-darwinis installed here. CI covers it.Doc correction
The comment claimed this case was already handled ("a later re-created embed.db is picked up with a fresh connection"). That only held if a poll happened to land while the file was absent. Removed.
Verification
The pre-existing test
embed_data_version_tracks_out_of_band_embed_writesstill passes, so the ordinary write path is unaffected.