Repository navigation
Heartbeat from a child process, and commit only a key still held (0.6.8) - #61
Merged
Merged
Conversation
DataVault's downstream job failed 3 times in 15 identical runs, each on one of these assertions: - test_run_loop_busy.jl: `elapsed >= 3.0` (stale_after) failed at 2.85 s and 2.90 s. DataVault writes `heartbeat=` truncated to the second, so a lock reads up to 1 s older than it is and is reclaimed up to 1 s before stale_after. Measured: a lock written at .95 s is reclaimable 2.07 s later. Now `>= stale_after - 1`. With the busy wait disabled (the bug this pins), run_loop! returns in 0.96-1.01 s with done == 0, so the new bound still catches it. - test_run_deadline.jl: a deadline 0.3 s ahead passed before `run!` handed out its first key (started == 0); a run-start observation takes most of a second. The deadline is now 5 s ahead and the first key runs until it has passed, so the test no longer depends on how fast `run!` starts. Same cause as e207f53. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The key heartbeat was a task in the master. Under a work_fn that does not yield it never beat, with -t 1, -t 2 and -t 2,1 alike: a sibling took the live key after stale_after, and the master, whose `lost` flag only that task could raise, saved and committed it as well. The heartbeat is now DataVault.start_heartbeat's child process, and ownership is checked where it matters: before save!, and by committing with mark_done!(vault, key, owner), which refuses a key a sibling took meanwhile. The release is owner-checked, so it runs unconditionally. Needs DataVault 0.8.9. Tests: - test_run_busy_holder.jl (new): a master whose work_fn never yields, under -t 1 and -t 2, in its own process, against a sibling polling from this one; and a master whose key is taken mid-work_fn commits nothing. - test_run_loop_busy.jl: back to `elapsed >= stale_after`, now measured from before the marker is written. The 0.6.8 allowance of 1 s (#60) was for DataVault's whole-second heartbeat, which 0.8.9 replaces. - test_run_deadline.jl: the first key runs until a 5 s deadline has passed, so the test no longer depends on how fast run! starts (from #60). - test_double_run_safety.jl, test_run_adversarial.jl: simulate the loss by a sibling's lock, and a crashed holder by backdating both heartbeat lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sotashimozono
marked this pull request as ready for review
September 29, 2026 03:26
Contributor
|
📚 Docs preview: https://qatlashub.github.io/SweepRunner.jl/previews/PR61/ (updates on each push to this PR) |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Sep 29, 2026
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.
Draft until DataVault 0.8.9 is registered (QAtlasHub/DataVault.jl#75). This PR raises the compat floor to 0.8.9, so CI cannot resolve its dependencies before that. Supersedes #60.
What was wrong (measured)
The key heartbeat was a
Threads.@spawntask inside the master. Whilework_fncomputed without yielding, it never beat, with-t 1,-t 2and-t 2,1alike. A control run whosework_fnyielded (sleep) beat normally.A sibling therefore took the live master's key after
stale_after. The master'slostflag could only be raised by that starved task, so the master then saved and committed the key as well.Change
DataVault.start_heartbeat, a childshprocess. It stops when the master dies, or when the lock's name no longer points at the file it opened (the key was released or taken).running_owner == tokbeforesave!mark_done!(vault, key, tok), which refuses a key a sibling took in the meantimeclear_running!(vault, key, tok)) is owner-checked, so it now runs unconditionally.Tests
test_run_busy_holder.jlwork_fnnever yields for 4 s, run in its own process under-t 1and-t 2. A sibling in this process tries to take the key every 0.1 s (stale_after = 1). It must never get the key before the master commits, and the master must commit. The test also checks the master really computed for ≥ 3.5 s and that there were ≥ 25 attempts, so a master that dies at once cannot pass it.work_fncommits nothing and leaves the sibling's lock in place.test_run_loop_busy.jl: back toelapsed >= stale_after. It is now timed from before the stale lock is written, so no allowance is needed. The 1 s allowance in Two run tests assumed timings a hosted runner does not give (0.6.8) #60 was for DataVault's whole-second heartbeat, which 0.8.9 replaces.test_run_deadline.jl: the first key runs until a deadline 5 s ahead has passed, so the test no longer depends on how fastrun!starts (from Two run tests assumed timings a hosted runner does not give (0.6.8) #60).test_double_run_safety.jl/test_run_adversarial.jl: a lost key is simulated with a sibling's lock, and a crashed holder by making both heartbeat lines old.Run locally against DataVault#75 (both branches loaded in a throwaway env), the touched and related files pass:
busy_holder,double_run_safety,loop_busy,deadline,adversarial,kill_recovery,liveness,minimal,loop,result_digest,stop_flagkeylockandartifacts, after two DataVault-side fixes that they uncovered:keylockcomputed 2 of its keys twice, because of the order in which DataVault's owner-checked commit wrote the marker; fixed in DataVaultb4fecc8.artifactscalled a DataVault internal I had renamed, which showed up as a 600 s wait; the old name is kept.🤖 Generated with Claude Code