Repository navigation
fix(hrr): start a fresh archive instead of resuming an earlier one - #13339
Merged
Merged
Conversation
A process whose pid-<pid> already held an archive resumed it: it scanned events.bin or trusted writer_state.json, cut off the trailer or a torn tail, indexed the blobs there and carried the counters on. Per-process archives made that path dead for its original purpose, and in practice it only ran when a new process reused an earlier one's pid with the same HIP_HRR_CAPTURE_OUTPUT, which then added its records to that archive. Remove resume and writer_state.json. When writer::open() finds an archive at pid-<pid>, it renames the directory to the first free pid-<pid>.prev-<n>, says so on stderr and starts a fresh archive. If the rename fails, capture is disabled for the process and the earlier archive stays as it was. The root manifest, the reader, hrr-playback and inspect_archive.py take only pid-<decimal pid> directories for process archives, so a moved archive is not part of the capture. Tests: Unit_HRR_CaptureResumeTrustsOnlyItsOwnFiles, Unit_HRR_CaptureResumeCountsOnlyWholeRecords, the hidden Unit_HRR_CaptureTrimFails_Direct workload and the two resume sections of Unit_HRR_CaptureActiveMarker are removed. Unit_HRR_CaptureMovesEarlierArchiveAside is added. The registered case set changes, and integration.yaml with it.
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Archive relocation can clobber a raced destination, and tooling has inconsistent PID-directory handling and messaging.
3 open findings
What changed in this PR
Removes HRR archive resumption, preserving same-PID archives under .prev-<n> while starting fresh captures.
Changes:
- Replaces resume/checkpoint-state logic with archive relocation.
- Excludes moved archives from root discovery and playback.
- Updates tests and documentation for the new behavior.
| File | Description |
|---|---|
projects/clr/hipamd/src/hrr/hip_capture_writer.cpp |
Starts fresh archives and relocates earlier ones. |
projects/hrr/playback/hrr_reader.h |
Exposes strict PID-directory parsing. |
projects/hrr/playback/hrr_reader.cpp |
Excludes moved archives during resolution. |
projects/hrr/playback/hrr_playback.cpp |
Applies strict discovery to info and repair. |
projects/hrr/tests/include/hrr_test_common.hh |
Updates archive test discovery. |
projects/hrr/tests/integration/hrr_capture_access_test.cc |
Replaces resume tests with relocation coverage. |
projects/hrr/tests/config/integration.yaml |
Updates registered integration cases. |
projects/hrr/skills/enable-recording/scripts/inspect_archive.py |
Excludes .prev-<n> directories. |
projects/hrr/skills/enable-recording/tests/test_inspect_archive.py |
Tests moved-archive exclusion. |
projects/hrr/skills/enable-recording/scripts/hrr_capture.sh |
Revises existing-output guidance. |
projects/hrr/skills/enable-recording/tests/test_hrr_capture.py |
Updates capture-merging terminology. |
projects/hrr/skills/enable-recording/SKILL.md |
Removes writer-state documentation. |
projects/hrr/producers/README.md |
Removes resume references. |
projects/hrr/producers/pytorch/hrr_torch_regions.py |
Updates producer behavior documentation. |
projects/hrr/producers/pytorch/tests/test_hrr_torch_regions.py |
Updates test commentary. |
projects/hrr/DESIGN.md |
Documents fresh archives and moved-aside directories. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…rchive aside The run command's comment, its --force note and its refusal still said that a second run into the same --output simply reads as one merged capture. A process of that run that reuses an earlier pid now moves that pid-<pid> to pid-<pid>.prev-<n>, which readers of the archive root leave out. Say so there and in the skill.
mangupta
reviewed
Oct 13, 2026
danielbolsson
approved these changes
Oct 14, 2026
danielbolsson
left a comment
danielbolsson
left a comment
Contributor
There was a problem hiding this comment.
Read the diff and the surrounding code (the open() double-call guard, the reader and inspector pid-directory filters); the Python suites pass and the new inspect_archive test fails with its filter removed. I did not build or run the C++/GPU tests.
Non-blocking: hip_capture_writer.cpp:1083 - on Windows _O_TRUNC follows a link at events.bin (the removed branch did too); refuse a non-regular events.bin in a follow-up.
This was referenced Oct 14, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
andraghetti
added a commit
that referenced
this pull request
Oct 16, 2026
* fix(hrr): remove the capture writer's resume leftovers writer::checkpoint() has no caller since capture resume was removed in #13339; the periodic flush and fsync live in append_event_locked(). Remove it and its declaration. open() runs once at init, or in a forked child after atfork_child() has cleared the incomplete flag, so its own clear was only needed for a second archive in the same process. Drop it: a record a child dropped before its first open() now keeps the child's archive marked incomplete. Rewrite the comments that still described a second open() in the same process and a blob check against the disk; the blob check is in memory and covers this archive only. * test(hrr): a forked child that marks its archive incomplete stays incomplete Unit_HRR_ForkedChildStaysIncomplete runs a workload that forks while a stream capture is live. The child makes the overflowing hipMemcpy3DAsync of Unit_HRR_OverflowingMemcpy3D_Direct, which capture drops and marks incomplete, before its first record opens its archive. The child's manifest must say "complete": false and the parent's true. Adds Unit_HRR_ForkedChildStaysIncomplete and its _Direct workload to tests/config/integration.yaml. * test(hrr): require exactly one child archive in the fork incomplete test Unit_HRR_ForkedChildStaysIncomplete passed when neither manifest named the other process as its parent. Count the child archives, require exactly one, and check that the parent's manifest says "complete": true.
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.
Motivation
The capture writer resumes an archive it finds at
pid-<pid>/. Since per-process archives landed, that only happens when a new process reuses an earlier process's pid with the sameHIP_HRR_CAPTURE_OUTPUT, which is common in containers. The new process then adds its records to the earlier process's archive. The HRR team decided to remove resume: per-process archives made it unnecessary.Technical Details
writer::open()no longer resumes. Removed:scan_events_for_resumeandScanResult,try_load_writer_stateandsave_writer_state_locked,index_existing_blobs_locked,resumed_file_is_ours,open_events_file,hrr_ftruncate_fd,hrr_seek_end,HRR_OPEN_APPEND, trailer stripping and torn-tail trimming on open, and the carried-over sequence, event and blob counts.writer_state.jsonis gone. Checkpoints only flush and fsyncevents.bin.pid-<pid>/holds an earlier archive (anevents.binthat is a regular file with one link, owned by the capturing user),open()renames the directory to the first freepid-<pid>.prev-<n>/. It does this after the usual private-directory check.rename(2)moves the entry itself and never follows a link. One line on stderr says where the archive went. The process then records a fresh archive.Capture disabledline. The earlier archive stays where it was, unchanged.pid-<pid>/events.bin(a symbolic link, a hard link, another user's file) is not moved. The open refuses it, as before.hrr-playback --infoand--repairon a root, andinspect_archive.pytake onlypid-<decimal pid>directories as process archives. Before this change they matched anypid-prefix, so a.prev-<n>directory would have been listed twice under the same pid, and a root would have read as two processes. A moved archive can still be read by pointinghrr-playbackat it.--repairare unchanged. They handle crashed archives, not resume.pid-<pid>.prev-<n>/, instead of having them continued by the new image.writer_state.json.Issue Tracking
JIRA ID: AIRUNTIME-2915
Test Plan
Unit_HRR_CaptureResumeTrustsOnlyItsOwnFiles,Unit_HRR_CaptureResumeCountsOnlyWholeRecords, the hiddenUnit_HRR_CaptureTrimFails_Directworkload, and the two resume sections ofUnit_HRR_CaptureActiveMarker.integration.yamllists the new case set.Unit_HRR_CaptureMovesEarlierArchiveAside. It captures a workload, copies that archive into the next run'spid-<pid>/before the run starts, and runs it again. The earlier archive must end up byte for byte inpid-<pid>.prev-1/. The new archive must start at sequence 0 and load complete. The root manifest must list one process, and the reader must resolve the root to the new archive. A second section makes the base directory read-only: capture must be disabled andpid-<pid>/must keep the earlier archive unchanged.hip_capture_writer.cppmust fail.test_an_archive_moved_aside_is_not_a_process_of_the_capturetotest_inspect_archive.py. It fails with develop'sinspect_archive.py.Test Result
MI350X (gfx950), GPUs 0 and 1. Each build is tested on its own and compared case by case with a develop baseline built from the same commit. The integration binary links in every run.
Unit_HRR_CaptureMovesEarlierArchiveAsidefailsUnit_HRR_CaptureTrimFails_Direct, a hidden workload that fails when it runs alone on develop too. The two removed resume tests passed on develop.Unit_HRR_CaptureMovesEarlierArchiveAsidecounts as a skip in the gate because the gate runs as root: "moved aside" passes (40 assertions), and "cannot be moved" skips because root can rename in a read-only directory. Run as a non-root user, both sections pass (34 assertions).pid-<pid>.prev-1, capture is not disabled when the archive cannot be moved, and the earlier archive is changed.test_inspect_archive.py: 98 passed, 1 skipped. The new test fails with develop'sinspect_archive.py.test_hrr_capture.py: 29 passed, 27 skipped. PyTorch producer tests: 17 passed, 1 skipped.Submission Checklist