You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(hrr): stop the archive reader at the clean-shutdown trailer - #13173
load_archive() set complete when it read a valid clean-shutdown trailer, and then carried on reading. A record after the trailer was replayed as part of a complete archive, and the trailer's event count did not cover it. Once #12258 lands the capture writer appends nothing after the trailer, but an archive from an earlier runtime can hold a few records there, from calls still in flight at shutdown.
This change makes the trailer the end of the event stream and checks the count it claims. Everything that decides whether an archive is complete now gives the same answer as the loader: --repair and --info on an archive root, --info on one archive, and the capture writer when it resumes an archive.
Technical Details
projects/hrr/playback/hrr_reader.cpp: load_archive() stops at the first valid trailer. It counts the whole records after it, prints [HRR] Ignored N records after the clean-shutdown trailer once, and does not replay them. The archive is complete only when the trailer's total_events equals the number of records before it. A mismatch is reported on stderr and leaves the archive incomplete, so hrr-playback --repair rewrites it.
projects/hrr/playback/hrr_reader.h: Archive gains skipped_after_trailer, trailer and trailer_events, so a trailer that miscounts is told apart from a missing one. kMaxRecordBytes (64 MiB) moves here from the reader source.
hrr::has_clean_trailer() (new, in the reader) walks the record headers and seeks over the payloads. It gives the answer load_archive() would give, with the same framing checks and the same 64 MiB cap, without reading the payloads. It seeks with _fseeki64 on Windows and fseeko elsewhere, so an events.bin over 2 GiB is walked correctly where long is 32 bits.
projects/hrr/playback/hrr_playback.cpp:
--repair on an archive root uses hrr::has_clean_trailer() to skip complete sub-archives. Before, it skipped any sub-archive that ended in a valid trailer, even one whose count was wrong.
--info on an archive root shows a process as complete only when its manifest says so and the trailer counts its records.
--info on one archive prints NO (shutdown trailer counts N events; M precede it) for a miscounting trailer, and an Ignored: line when records follow the trailer.
The --repair comment and usage string say it rewrites any incomplete archive: torn tail, missing trailer or miscounting trailer.
projects/clr/hipamd/src/hrr/hip_capture_writer.cpp: scan_events_for_resume() counts a record only once its body fits in the file. Before, a torn last record was cut off on resume but stayed in the count, so the resumed trailer claimed one record too many.
projects/hrr/DESIGN.md and projects/hrr/README.md describe all of the above.
Behaviour changes:
An archive whose trailer count does not match its records is no longer reported as complete, and --repair rewrites it, also on an archive root.
An archive with records after a matching trailer is complete. The records are not replayed, and --repair leaves the archive as it is.
Unit_HRR_Recovery_RecordAfterTrailer: 5 records, a trailer that counts them, 2 more records. The archive is complete with exactly the 5 records, has_clean_trailer agrees, and --info shows the 2 ignored records.
Unit_HRR_Recovery_TrailerCountMismatch and Unit_HRR_Recovery_TrailerOvercounts: a trailer that claims 4 or 6 of 5 records. The archive is not complete, has_clean_trailer agrees, and --info reports the count instead of a missing trailer.
Unit_HRR_Recovery_TornMiddleRecord and Unit_HRR_Recovery_OversizeRecordBeforeTrailer: a record shorter than a header, or one byte over 64 MiB, before a trailer that counts it. The loader takes the record as torn, and has_clean_trailer agrees.
Unit_HRR_Recovery_RepairRootRewritesMiscountedTrailer: --repair on a root rewrites the sub-archive whose trailer miscounts and skips the clean one.
Unit_HRR_Recovery_RootInfoChecksTrailerCount: --info on a root shows the miscounting sub-archive as not complete.
projects/hrr/tests/integration/hrr_capture_access_test.cc: Unit_HRR_CaptureResumeCountsOnlyWholeRecords resumes an archive that ends in a torn record. The result loads complete, with a trailer that counts its records.
Negative controls: each test against the code with its fix reverted or its check removed.
The 64-bit seek has no test: the failure needs a Windows C runtime and a file over 2 GiB, and on Linux fseek is already 64-bit.
Built amdhip64 with -DHIP_HRR_CAPTURE_ENABLED, hrr-playback, hrr-unit-tests and hrr-integration-tests for gfx950 from this branch at cdad319. Ran every Catch2 case, hidden ones included, one at a time on an MI350X.
Test Result
Check
With this change
Without it
hrr-unit-tests, MI350X
30 of 30 pass
the record-after-trailer, overcount, oversize-record and root --info cases fail when the matching check is removed or reverted
hrr-integration-tests default cases, MI350X
106 of 106 pass
Unit_HRR_CaptureResumeCountsOnlyWholeRecords fails with the writer change reverted: the trailer counts 187 events and 186 precede it
Hidden integration cases, MI350X
103 of 126 pass. The other 23 end the same way on develop at 83201fc: the 14 that fail there (Unit_HRR_FirstMallocRoundtrip, Unit_HRR_ApiMatrix_T2_Roundtrip, Unit_HRR_ApiMatrix_T4_Roundtrip and the 11 Unit_HRR_DiskSpace_*_Direct workloads), and 9 workloads that need the environment their own test sets
load_archive() set complete on a valid hrr_eof_record and kept reading, so
a record after the trailer was replayed as part of a complete archive and
was not covered by the trailer's event count. A runtime older than the
writer fix can leave such records from calls still in flight at shutdown.
The trailer now ends the event stream. Whole records after it are counted
in Archive::skipped_after_trailer, reported once on stderr, and not
replayed. The archive is complete only when the trailer's total_events
equals the number of records before it; a mismatch is reported and leaves
the archive incomplete, so --repair rewrites it.
Unit tests cover a record after the trailer and a trailer whose count is
wrong. DESIGN.md says what the reader does with both.
…hive
--repair on an archive root skipped any sub-archive whose events.bin ended
in a valid trailer, without checking the trailer's total_events. A
sub-archive whose trailer miscounts its records, which load_archive does not
treat as complete, was reported as already complete and never rewritten.
has_clean_trailer() now walks the record headers, seeking over the payloads,
and accepts the archive only when the trailer is the last record and counts
the records before it.
A trailer with the wrong count was also reported by --info as "no shutdown
trailer; capture likely crashed", because Archive only kept complete and
truncated. Archive now keeps trailer and trailer_events, and --info prints
the trailer's count against the records that precede it.
DESIGN.md describes --repair in terms of incomplete archives: a torn tail, a
missing trailer and a miscounting trailer are all rewritten.
Unit_HRR_Recovery_RepairRootRewritesMiscountedTrailer covers the root
repair; Unit_HRR_Recovery_TrailerCountMismatch now checks the new fields and
the --info line.
Root info fails to validate trailer counts for multi-process archives
projects/hrr/playback/hrr_playback.cpp:309
This status branch is bypassed for a multi-process archive root: main() returns from print_root_info() at lines 1433–1436, and that function prints each process's manifest complete flag without validating its trailer count. Consequently, hrr-playback <root> --info still reports a sub-archive with a miscounting trailer as complete, despite the new completeness definition. Please make root info validate each sub-archive's trailer/count (either through load_archive() or a count-aware lightweight probe) before rendering its status.
The reason will be displayed to describe this comment to others. Learn more.
Needs work: two blocking items, the rest are nits.
Blocking
The PR body doesn't match the head. It omits the second commit (rewritten has_clean_trailer in hrr_playback.cpp:1037-1080, the new --info line, Archive::trailer), and "2 new tests / 25 of 25" can't cover the third test. Describe the change and restate the results on this head; no HRR check has run on it.
hrr_recovery_test.cc:376-402 - the only root-repair test has a miscounted trailer at EOF, so the new has_clean_trailer conditions are untested. By inspection, dropping offset + total == size or the total bounds checks leaves every test green. Add trailer-then-record, torn middle record and overcounting trailer cases.
Non-blocking
3. hrr_playback.cpp:1062-1065 - no 64 MiB record cap like read_raw_record (hrr_reader.cpp:125-131); an oversize record plus a counted trailer would read as clean here but torn in the loader.
4. hip_capture_writer.cpp:559-571 (outside the diff) - scan_events_for_resume counts a torn record before the fit check, so a resumed capture's trailer can count one too many and now be flagged incomplete.
5. DESIGN.md:85, DESIGN.md:427, hrr_playback.cpp:918-921 and the --repair usage string still say "crash-truncated"; print_info (hrr_playback.cpp:306-318) doesn't show skipped_after_trailer.
Question:--repair leaves records after a matching trailer in place. Deliberate? AIRUNTIME-2848 says --repair keeps its behaviour; please confirm the root-repair change is intended.
Basis: read-only review; nothing was built or run. Findings 2-4 (the test-coverage verdict, the record cap, the resume count) come from reading the code, not from a test or mutation run.
hrr-playback <root> --info showed each process as complete from its
manifest alone, so a sub-archive whose trailer miscounts its records was
listed as complete although load_archive does not take it as complete. The
root view now also requires the trailer probe to pass.
The probe, has_clean_trailer(), moves into the reader as
hrr::has_clean_trailer and uses the reader's framing: the same 64 MiB
record cap (kMaxRecordBytes, now in hrr_reader.h) and the same stop at the
first trailer. Before, a record over the cap followed by a counted trailer
passed the probe while the loader took the record as torn, and a trailer
followed by more records failed the probe while the loader took the archive
as complete. The probe now gives the loader's answer in both cases. The
check that a record fits in the file is gone: seeking past the end makes
the next header read fail.
--info shows records ignored after the trailer, and the --repair comment,
usage string, README and DESIGN.md describe repair of any incomplete
archive instead of a crash-truncated one.
Unit tests: Unit_HRR_Recovery_RootInfoChecksTrailerCount,
Unit_HRR_Recovery_TrailerOvercounts, Unit_HRR_Recovery_TornMiddleRecord,
Unit_HRR_Recovery_OversizeRecordBeforeTrailer, and probe and --info checks
added to the complete, record-after-trailer and count-mismatch cases.
scan_events_for_resume() counted a record before checking that its body
fits in the file. A torn last record was cut off on resume but stayed in
the event count, so the resumed capture's trailer claimed one record too
many, and the reader now takes such an archive as incomplete. The record is
counted only once it fits.
Unit_HRR_CaptureResumeCountsOnlyWholeRecords resumes an archive that ends
in a torn record and checks that the result loads complete, with a trailer
that counts its records.
Re the previously missed finding "Root info fails to validate trailer counts for multi-process archives": fixed in 99e2ee9. print_root_info() now shows a process as complete only when its manifest says so and hrr::has_clean_trailer() finds a trailer that counts the records before it, the same test load_archive() applies. The probe moved into the reader so --repair and --info on a root share it.
Covered by Unit_HRR_Recovery_RootInfoChecksTrailerCount: a root with two sub-archives whose manifests both say complete, one with a trailer that counts 3 of 4 records; --info on the root shows that one as NO and the other as yes. The test fails with the manifest-only status restored.
has_clean_trailer() skipped payloads with fseek, which takes a long. On
Windows that is 32 bits, and an msvcrt fseek fails once the file position
passes 2 GiB even for a small relative offset, so a valid events.bin over
2 GiB could read as incomplete: root --info would show it as not complete
and root --repair would rewrite it. The walk now seeks with _fseeki64 on
Windows and fseeko elsewhere.
No test: the failure needs a Windows C runtime and a file over 2 GiB, and
on Linux fseek is already 64-bit, so a test here passes with or without the
change.
@danielbolsson thanks for the review. All of it is addressed on the head at 8ab2e31:
The PR body is rewritten for this head: all the commits, the shared trailer check, the new --info lines, Archive::trailer, and results restated from a fresh MI350X run.
Fixed in 99e2ee9. The trailer check is now hrr::has_clean_trailer in the reader, and it gives the same answer as load_archive: same framing checks, stop at the first trailer. Two conditions are gone. "Trailer at end of file" made the check disagree with the loader for a trailer followed by records. "Record fits in the file" could not change the answer, because seeking past the end makes the next header read fail. New cases: TrailerOvercounts, TornMiddleRecord, OversizeRecordBeforeTrailer, and a check-against-loader assertion in RecordAfterTrailer. They fail with the end-of-file check put back, with == changed to <=, and with the 64 MiB cap removed. The shorter-than-a-header check does not change the result on Linux, where the unsigned wrap seeks past the end. It matters only where long is 32-bit, so TornMiddleRecord documents that case and has no failing negative control here.
Fixed in 99e2ee9. kMaxRecordBytes is in hrr_reader.h, and the trailer check and the loader share it. Unit_HRR_Recovery_OversizeRecordBeforeTrailer writes a record one byte over the cap before a counted trailer. It fails with the cap removed from the check.
Confirmed and fixed in 8ab2e31. scan_events_for_resume now counts a record only after its body fits. Unit_HRR_CaptureResumeCountsOnlyWholeRecords resumes an archive ending in a torn record. With the old scan, the trailer counts 187 events and 186 precede it.
Fixed in 99e2ee9. DESIGN.md:85, DESIGN.md:427, the --repair comment, the usage string and the README row describe repair of any incomplete archive. --info prints Ignored: N records after the shutdown trailer (not replayed) when N is not zero, and RecordAfterTrailer checks it.
On the question: yes, --repair leaves records after a matching trailer in place, as it did before this PR. What changed is the miscounting trailer: it now makes the archive incomplete, so single and root --repair rewrite it and root --info agrees. That is intended: a trailer that miscounts is not a clean shutdown, and AIRUNTIME-2848's "keeps its behaviour" was about archives that were already whole.
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed cdad319. Both earlier blocking points are fixed: the body now describes has_clean_trailer, the new --info lines and the trailer fields, and the new branches have tests (RecordAfterTrailer, TornMiddleRecord, TrailerOvercounts, OversizeRecordBeforeTrailer, RepairRootRewritesMiscountedTrailer).
Non-blocking follow-ups:
skills/decode-and-triage/SKILL.md:323,331 and skills/enable-recording/scripts/inspect_archive.py:694 still describe every incomplete archive as "crash-truncated / no clean-shutdown trailer"; a miscounting trailer is now incomplete too.
hip_capture_writer.cpp:523scan_events_for_resume has no 64 MiB record cap and seeks with long, so a record over the cap is counted there but torn in the loader, which contradicts "all deciders agree".
@danielbolsson one correction to item 2: with the 64-bit seek from a later Copilot thread (cdad319), the shorter-than-a-header check no longer changes the result on any platform, Windows included; it stays as a guard that matches read_raw_record.
…onfigs
The seven Unit_HRR_Recovery_* cases in hrr_recovery_test.cc go in
config/unit.yaml and Unit_HRR_CaptureResumeCountsOnlyWholeRecords in
config/integration.yaml, which develop requires to list every case.
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
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.
danielbolsson
left a comment
Motivation
load_archive()setcompletewhen it read a valid clean-shutdown trailer, and then carried on reading. A record after the trailer was replayed as part of a complete archive, and the trailer's event count did not cover it. Once #12258 lands the capture writer appends nothing after the trailer, but an archive from an earlier runtime can hold a few records there, from calls still in flight at shutdown.This change makes the trailer the end of the event stream and checks the count it claims. Everything that decides whether an archive is complete now gives the same answer as the loader:
--repairand--infoon an archive root,--infoon one archive, and the capture writer when it resumes an archive.Technical Details
projects/hrr/playback/hrr_reader.cpp:load_archive()stops at the first valid trailer. It counts the whole records after it, prints[HRR] Ignored N records after the clean-shutdown traileronce, and does not replay them. The archive is complete only when the trailer'stotal_eventsequals the number of records before it. A mismatch is reported on stderr and leaves the archive incomplete, sohrr-playback --repairrewrites it.projects/hrr/playback/hrr_reader.h:Archivegainsskipped_after_trailer,trailerandtrailer_events, so a trailer that miscounts is told apart from a missing one.kMaxRecordBytes(64 MiB) moves here from the reader source.hrr::has_clean_trailer()(new, in the reader) walks the record headers and seeks over the payloads. It gives the answerload_archive()would give, with the same framing checks and the same 64 MiB cap, without reading the payloads. It seeks with_fseeki64on Windows andfseekoelsewhere, so anevents.binover 2 GiB is walked correctly wherelongis 32 bits.projects/hrr/playback/hrr_playback.cpp:--repairon an archive root useshrr::has_clean_trailer()to skip complete sub-archives. Before, it skipped any sub-archive that ended in a valid trailer, even one whose count was wrong.--infoon an archive root shows a process as complete only when its manifest says so and the trailer counts its records.--infoon one archive printsNO (shutdown trailer counts N events; M precede it)for a miscounting trailer, and anIgnored:line when records follow the trailer.--repaircomment and usage string say it rewrites any incomplete archive: torn tail, missing trailer or miscounting trailer.projects/clr/hipamd/src/hrr/hip_capture_writer.cpp:scan_events_for_resume()counts a record only once its body fits in the file. Before, a torn last record was cut off on resume but stayed in the count, so the resumed trailer claimed one record too many.projects/hrr/DESIGN.mdandprojects/hrr/README.mddescribe all of the above.Behaviour changes:
--repairrewrites it, also on an archive root.--repairleaves the archive as it is.Issue Tracking
JIRA ID: AIRUNTIME-2848
Test Plan
projects/hrr/tests/unit/hrr_recovery_test.cc, on theTmpArchivefixture Daniel suggested in his review of fix(hrr): swap the capture shims into the dispatch table slot by slot #12258:Unit_HRR_Recovery_RecordAfterTrailer: 5 records, a trailer that counts them, 2 more records. The archive is complete with exactly the 5 records,has_clean_traileragrees, and--infoshows the 2 ignored records.Unit_HRR_Recovery_TrailerCountMismatchandUnit_HRR_Recovery_TrailerOvercounts: a trailer that claims 4 or 6 of 5 records. The archive is not complete,has_clean_traileragrees, and--inforeports the count instead of a missing trailer.Unit_HRR_Recovery_TornMiddleRecordandUnit_HRR_Recovery_OversizeRecordBeforeTrailer: a record shorter than a header, or one byte over 64 MiB, before a trailer that counts it. The loader takes the record as torn, andhas_clean_traileragrees.Unit_HRR_Recovery_RepairRootRewritesMiscountedTrailer:--repairon a root rewrites the sub-archive whose trailer miscounts and skips the clean one.Unit_HRR_Recovery_RootInfoChecksTrailerCount:--infoon a root shows the miscounting sub-archive as not complete.projects/hrr/tests/integration/hrr_capture_access_test.cc:Unit_HRR_CaptureResumeCountsOnlyWholeRecordsresumes an archive that ends in a torn record. The result loads complete, with a trailer that counts its records.fseekis already 64-bit.amdhip64with-DHIP_HRR_CAPTURE_ENABLED,hrr-playback,hrr-unit-testsandhrr-integration-testsfor gfx950 from this branch at cdad319. Ran every Catch2 case, hidden ones included, one at a time on an MI350X.Test Result
hrr-unit-tests, MI350X--infocases fail when the matching check is removed or revertedhrr-integration-testsdefault cases, MI350XUnit_HRR_CaptureResumeCountsOnlyWholeRecordsfails with the writer change reverted: the trailer counts 187 events and 186 precede itdevelopat 83201fc: the 14 that fail there (Unit_HRR_FirstMallocRoundtrip,Unit_HRR_ApiMatrix_T2_Roundtrip,Unit_HRR_ApiMatrix_T4_Roundtripand the 11Unit_HRR_DiskSpace_*_Directworkloads), and 9 workloads that need the environment their own test setsSubmission Checklist