fix(ingest): CRC-validate the primary ingest read, not just the resume path - #462
Conversation
…e path Hebbian-Robotics#429 fixed the LeRobot resume path; the same hole was open one stage earlier, on the primary path every ingest takes. transform.py's read of the source file (open_reader) never asked for CRC validation, so a structurally valid MCAP with a damaged chunk payload transcoded without complaint and got a fresh, true receipt over corrupt bytes. open_reader and PythonMcapEpisodeReader gain validate_crcs (default False); only the ingest read in transform.py opts in, so the other three call sites reading HFlow's own canonical output are unaffected. The transcode already decodes every chunk in one pass, so this piggybacks a check on a read that already happens: measured 1.113x on a 30MB source and 1.035x on a 75MB one. CRCValidationError subclasses ValueError, not McapError, so classify_ingest_failure needed its own branch for it -- without one it silently fell through to INFRASTRUCTURE, blaming the platform for a damaged recording. It classifies to the same SOURCE_UNREADABLE kind as the not-MCAP case; error_type (InvalidMagic vs CRCValidationError) keeps the two distinguishable in the ledger without a new enum member. Refs Hebbian-Robotics#431 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
👋 Hi @Sravan1011 — thank you so much for your first contribution to HFlow! A maintainer will review your pull request as soon as possible. In the meantime:
💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game. We are excited to have you here and appreciate your help making the project better! 🙌 |
kstonekuan
left a comment
There was a problem hiding this comment.
The classification branch is the part that makes this correct rather than merely present, and the comment explains exactly why it is needed. I checked the claim rather than taking it:
MRO: ['CRCValidationError', 'ValueError', 'Exception', 'BaseException', 'object']
is McapError subclass: False
So without that branch a payload-damaged recording would have fallen through to INFRASTRUCTURE and blamed the machine for a fact about the file. Reusing SOURCE_UNREADABLE and letting error_type carry InvalidMagic vs CRCValidationError is the right call: the operator's decision is the same, and the detail is one column over for anyone who needs it.
Measuring at ingest scale instead of inheriting #429's number was the other thing asked for, and 1.113x on 30 MB and 1.035x on 75 MB is the answer that settles it. The ratio falling as the file grows is what you would expect when the CRC rides a decode that was already happening.
Validated: full gate clean, 1639 passed / 6 skipped. Both halves independently held:
validation turned off at the ingest read FAILED test_ingest_refuses_a_source_with_a_damaged_chunk_payload
FAILED test_a_payload_damaged_source_is_classified_the_same_as_unreadable
CRC failure falls through to INFRASTRUCTURE FAILED test_classify_crc_validation_error_as_source_unreadable
FAILED test_a_payload_damaged_source_is_classified_the_same_as_unreadable
Blast radius is contained, which was the third thing the issue asked you to decide. validate_crcs defaults to False and only transform.py:580 opts in, so episode.py:311, app.py:1390 and lerobot.py:230 keep reading canonical files at the old cost. Putting the flag on open_reader rather than turning validation on inside it is what makes that possible, and the comment says why this one call site is different: a source nobody has trusted yet.
Merging. That closes the family #426 opened: resume verifies reused bytes, ingest verifies incoming ones.
Closes #431.
Summary
#429 fixed the LeRobot resume path. The same hole was open one stage earlier, on the primary path every ingest takes:
transform.py's read of the source file (open_reader) never asked for CRC validation, so a structurally valid MCAP with a damaged chunk payload transcoded without complaint and got a fresh, true receipt over corrupt bytes — #426's finding, one stage upstream.What changed
open_reader/PythonMcapEpisodeReader(reader.py) gainvalidate_crcs: bool = False, threaded straight into the existingmake_reader(..., validate_crcs=...)call — the same shapedoctor.pyand fix(import): CRC-validate reused landing episodes before stamping receipts #429's fix already use.False, soapp.py:1390,episode.py:311, andruntime/_templates.py:717are unaffected: those read canonical files HFlow already produced and already identifies by content hash. Onlytransform.py's ingest read opts in withvalidate_crcs=True, since that's the one call reading someone else's file for the first time.ingest_ledger.classify_ingest_failuregets a new branch formcap.stream_reader.CRCValidationError. This is not cosmetic:CRCValidationErrorsubclassesValueError, notMcapError, so without this branch a payload-damaged source would silently fall through toIngestFailureKind.INFRASTRUCTURE— blaming the platform for a bad recording. It classifies to the sameSOURCE_UNREADABLEkind as the not-MCAP case;error_type(InvalidMagicvsCRCValidationError) already stays stored verbatim beside the classification, so the two stay distinguishable in the ledger without a newIngestFailureKindmember.Cost
The transcode already decodes every chunk over one full read (
reader.iter_batches()), sovalidate_crcs=Truechecks a CRC on a pass that was already happening rather than adding one. Measured directly on ingest-scale sources (not inherited from #429's smaller fixture number):validate_crcs=Falsevalidate_crcs=TrueBest-of-5 wall time, filesystem cache warmed identically before each series.
Tests
tests/test_ingest_ledger.py::test_classify_crc_validation_error_as_source_unreadable— pins the classifier branch directly.tests/test_ingest_ledger.py::test_ingest_refuses_a_source_with_a_damaged_chunk_payload— a source with a damaged chunk payload (via the sharedflip_chunk_payload_bytesfixture from fix(import): CRC-validate reused landing episodes before stamping receipts #429) raises throughwrite_canonical_episodeand produces no output file.tests/test_ingest_in_process.py::test_a_payload_damaged_source_is_classified_the_same_as_unreadable— end-to-end throughhflow ingest: ledger row lands withfailure_kind = source-unreadable,error_type = CRCValidationError.InvalidMagic→source-unreadable) are unchanged and still pass, confirming the structurally-invalid case keeps its current classification.Validation