fix(live): latch only the swept episodes under per-episode latch files - #1254
Open
cgycorey wants to merge 1 commit into
Open
fix(live): latch only the swept episodes under per-episode latch files#1254cgycorey wants to merge 1 commit into
cgycorey wants to merge 1 commit into
Conversation
Follow-up to the merged HKUDS#1244 (a7d00ab). Reviewer (HOLD) found two real latch races at mark time in the merged code: 1. mark_sweep_fired re-read the sentinels AFTER the sweep. A clear + re-trip mid-sweep (ep1 -> ep2) recorded ep2 as fired although no ep2 sweep ran; the next runner then skipped ep2 and the kill action was silently lost (reproduced). 2. The latch was a shared read-merge-write record: two concurrent completions for different episodes (allowed — claims are episode-keyed) could lose one another's entry (reproduced). Fix, per the reviewer's direction, with a stronger mechanism than the suggested lock: - halt_snapshot() reads both sentinels in ONE coherent pass and returns (active_episodes, newest). The runner captures this BEFORE claiming; the claim, the already-fired check, every mark and the release all bind to that snapshot. - mark_sweep_fired(broker, episodes) takes the snapshot and never re-reads the sentinels. - Latch files are per-episode (FLATTEN_FIRED-<sha16(episode)>), each an independent O_CREAT|O_EXCL atomic create. There is NO shared mutable record to race on — concurrent completions cannot lose an entry by construction, so no inter-process lock is needed at all. The legacy single-record file is still consulted for compat. Rebased on origin/main (e90b6c6), preserving the upstream no-side-effect re-check audit (637791b). Regressions (mutated-fails / fixed-passes): - test_mid_sweep_retrip_records_only_swept_episode - test_concurrent_latch_updates_both_episodes_recorded 96 live tests pass; ruff clean.
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.
What && why
Follow-up to merged #1244. The halt-sweep latch had two mark-time races found in re-review:
mark_sweep_firedre-read the halt sentinels after the sweep. A clear + re-trip mid-sweep (ep1→ep2) recorded ep2 as fired although no ep2 sweep ran; the next runner then skipped ep2 — the kill action was silently lost (reproduced).Fix
halt_snapshot(broker)reads both sentinels in one coherent pass →(active_episodes, newest). The runner captures this before claiming; the claim, the already-fired check, everymark_sweep_firedcall and the release all bind to that snapshot. No sentinel is ever re-read at mark time.mark_sweep_fired(broker, episodes)takes the snapshot explicitly.FLATTEN_FIRED-<sha16(episode)>), each an independent atomicO_CREAT|O_EXCLcreate. There is no shared mutable record to race on: concurrent completions cannot lose an entry by construction, so the suggested inter-process lock is unnecessary. The legacy single-record file is still read for compatibility.Tests
test_mid_sweep_retrip_records_only_swept_episode— ep1 swept + ep2 re-tripped mid-sweep → ep1 latched, ep2 NOT → next runner sweeps ep2.test_concurrent_latch_updates_both_episodes_recorded— barrier-synced concurrent marks for two episodes → both survive.Both regression tests mutated-fail against the old behavior and pass on the fix. 96 live tests pass; ruff clean.
Known boundary (pre-existing, unchanged by this PR): if the halt state changes between the snapshot and the claim (a clear/re-trip in that microsecond gap), the sweep may claim the previous episode (
"unknown"when the snapshot is empty). The claim is still held and released correctly; the latched set is the snapshot's. This is inherited from the merged #1244 logic and is not a regression — documented rather than silently assumed.