Allow atomic in-place track overwrite via override - #134
Open
aviezerl wants to merge 1 commit into
Open
Conversation
emr_track.create and emr_track.import both refused to write a track that already existed in the target db, regardless of `override` - that flag only covered shadowing a track held in another db. Callers wanting to rebuild a track in place had no option but to emr_track.rm() it first, which left the track absent for the entire rebuild. Anything reading it in that window failed, which is what wiser.epi's cohort/computed-track rebuild hits. Let override mean "replace this track" in the same db too, and route every track write through a staging file that is renamed into place. rename(2) replaces the target atomically, so a reader sees either the complete previous track or the complete new one, never a missing or half-written file. emr_track.import already staged to a ".tmp" file, but unlinked the target before the move, reopening exactly the window the staging was meant to close; the unlink is unnecessary since rename replaces the target by itself. The staging path is also now suffixed with the pid so two writers of the same track cannot corrupt each other's staging file, and it is cleaned up if serialization or the rename throws. Claude-Session: https://claude.ai/code/session_01J4rGxe5SVxtXktoRHvWXsL
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.
Why
emr_track.createandemr_track.importboth refuse to write a track that already exists in the target db, regardless ofoverride- that flag only ever covered shadowing a track held in another db:So a caller wanting to rebuild a track in place has no option but to
emr_track.rm()it first. That is whatwis_cohort.build()/wis_computed_track.build()in wiser.epi do, and it leaves the track absent for the entire rebuild - every reader in that window fails. It is the root of the "rebuild a cohort and everything depending on it breaks for a few minutes" problem in wiser.What
override = TRUEnow also means "rewrite this track in its own db". Cross-db behaviour is unchanged.<track>.nrtrack.tmp.<pid>and renamed into place.rename(2)replaces the target atomically, so a concurrent reader sees either the complete previous track or the complete new one - never a missing or partial file.emr_track.importalready staged to a.tmpfile, but calledunlink(target)before the move, which reopened exactly the window the staging was there to close.renamereplaces the target by itself, so the unlink just goes away.A track is a single file (
<db>/<name>.nrtrack), so the rename really is atomic - no directory tree to swap.Callers can now drop the rm-then-rebuild dance entirely and pass
override = TRUE.Tests
New
tests/testthat/test-track.overwrite.R: same-db rewrite via both entry points, still errors withoutoverride, data actually replaced, and no staging files left behind.Heads up on the existing suite: it is flaky independently of this change. Three runs of unmodified
masterproduced three different failure sets (7, 8 and 9 failing tests, withfilters work on an overridden track,read_only is also overridden when overriding a trackandemr_track.addto works with filecoming and going). Two runs of this branch produce 597 tests with the same 8 failures as one of those baseline runs, and the 3 new tests pass. So no new failures are attributable here, but the suite is not a reliable per-test signal as it stands - probably worth a separate look, several of these tests share and mutate one on-disk db.Note on scope
.vars/ attributes are still written separately after the track file, so a reader landing between the rename and the var write sees the new track with the oldspec. That degrades towis_track_compiled()returning FALSE (at worst a redundant rebuild), not an error, and two files cannot be swapped in a single atomic operation anyway.