fix(merge): write-temp-then-rename so a failed merge never destroys prior output - #15
Merged
Merged
Conversation
The merge paired pre<->post rows on (frame_idx, thread_id), where
frame_idx was each DLL's independent g_frameCounter. The two counters
stay in lockstep only under pre-upstream layer ordering and a symmetric
start -- both of which the README documents as the user's manual job
("Layer ordering is on you"; a 4th layer can slip between pre and post).
Any asymmetry (reversed ordering, a one-sided Start() throw, a one-sided
shared-memory read) shifted every subsequent pair by one, so target_us
was computed from two DIFFERENT host frames and the merge silently
emitted plausible-but-wrong numbers for the whole session.
Key on the frame's intrinsic identity instead: frameEndInfo->displayTime
(an XrTime) is the same value on both halves for a given host frame and
flows through the chain unchanged, so ordering and start-alignment stop
mattering -- a frame seen by only one side simply fails to join (one
dropped frame) instead of cascading. Renames the frame_idx CSV column to
display_time across the per-side, GPU, and merged CSVs, merge.{h,cpp},
the GPU timer surface, analyze.py, and both test suites. g_frameCounter
is retained solely to feed MergeIntoOutput's zero-frame guard.
displayTime is stored as uint64_t (XrTime is non-negative in practice),
so the merge keys and the GPU ring need no signed/unsigned churn. Adds a
frameEndInfo null guard on both sides since the record path now
dereferences it. A target layer that itself rewrites displayTime now
yields an empty merge (a loud, logged failure) instead of wrong numbers.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rior output MergeIntoOutput deleted frames-merged-<pid>.csv via removeStale() on every error path once g_frameCounter>0, and opened the destination in truncate mode before writing. A process runs many start/stop sessions (the hotkey toggles repeatedly), so a later session that recorded frames but then failed transiently -- a per-side CSV that never flushed (disk full, lost ACL), zero matched pairs, a failed open, or ENOSPC partway through the write -- destroyed a still-valid merged CSV from an earlier session and left nothing in its place. The "current session has just superseded it" assumption only held when the current session actually succeeded. Write the merge to a sibling <out>.tmp and atomically rename it over the destination only after the write fully succeeds. The previous merged CSV now survives every failure path (replaced only by a complete, valid new file), which also closes the open-in-truncate window where a disk-full mid-write corrupted the destination before the write could finish. Drops removeStale() entirely. A stale-but-valid file from an earlier session is preserved on failure -- consistent with the existing zero-frame guard, and the logs say the merge was skipped. MergeIntoOutput is OS-facing glue (paths + ofstream); like the rest of it, it is exercised through the build and the pure-function tests in test_merge.cpp rather than a unit test of its own. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
2 participants
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.
Finding #3 (code review):
removeStaledeletes a valid merged CSV on a failed later sessionMergeIntoOutputdeletedframes-merged-<pid>.csvviaremoveStale()on every error path onceg_frameCounter > 0, and opened the destination in truncate mode before writing. A process runs many start/stop sessions (the hotkey toggles repeatedly), so a later session that recorded frames but then failed transiently — a per-side CSV that never flushed (disk full, lost ACL), zero matched pairs, a failed open, or ENOSPC partway through the write — destroyed a still-valid merged CSV from an earlier session and left nothing in its place. The "current session has just superseded it" assumption only held when the current session actually succeeded.Fix
Write the merge to a sibling
<out>.tmpand atomically rename it over the destination only after the write fully succeeds:truncopen destroyed the old file first).removeStale()entirely (all 6 call sites + the lambda).Verification
MergeIntoOutputis OS-facing glue (paths +ofstream); like the rest of it, it's exercised via the build + the pure-function tests intest_merge.cpprather than a unit test of its own (matching the file's existing policy).removeStalereferences remain; no lines >120 cols.🤖 Generated with Claude Code