Skip to content

metaUtils: make the (de)compression chunk size settable, and cover the trailer case - #148

Merged
dzenanz merged 1 commit into
Kitware:masterfrom
hjmjohnson:add-chunk-size-api-and-trailer-test
Sep 5, 2026
Merged

metaUtils: make the (de)compression chunk size settable, and cover the trailer case#148
dzenanz merged 1 commit into
Kitware:masterfrom
hjmjohnson:add-chunk-size-api-and-trailer-test

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Makes MET_MaxChunkSize settable so the input-chunking path can be reached by a cheap test, and adds a regression test for the deferred-trailer case.

Depends on #147; stays draft until then. The new test exercises the defect #147 fixes, so it fails against master as it stands. The plan is to merge #147 first, rebase this branch on top of it, and only then mark this ready — CI here is not expected to be green before that.

The two changes are otherwise independent: they touch different regions of metaUtils.cxx, share no commits, and merge cleanly in either order (verified both directions).

Why the API change is needed for the test

MET_MaxChunkSize was a file-local constexpr of 1 GiB, so the only way to reach the chunk boundary was to allocate several gigabytes: the smallest reproduction I could build against the shipped constant needed a 1073741829-byte compressed stream, found by binary-searching the raw size until the boundary landed inside the 8-byte trailer. That is not something to put in CI.

With a settable chunk size the test simply chooses the boundary — MET_SetMaxChunkSize(compressedSize - 4) places the split inside the trailer deterministically, with 64 KiB of data and no search.

metaUtils.h already exports extern bool META_DEBUG, so a settable knob is not a new pattern here. Values of zero or less are ignored, and the test saves and restores the previous value so test ordering is unaffected.

What the test asserts

testMeta15UncompressChunkBoundary, 0.01 s:

  1. A valid stream whose trailer falls in a later input chunk is accepted, and the decompressed bytes match the original.
  2. A stream with a corrupt CRC trailer is still rejected.

The second case matters: without it, the test could be satisfied by simply weakening the stream-end requirement rather than by consuming the trailer.

Verified by disabling only the fix loop from #147 while keeping this API, so the test is known to detect that specific defect rather than an unrelated build difference:

test 15 rest of suite
master (no fix) FAILzlib error -5 14/14 pass
with #147 pass 14/14 pass

@hjmjohnson
hjmjohnson marked this pull request as ready for review September 4, 2026 22:55
@hjmjohnson

Copy link
Copy Markdown
Contributor Author

@dzenanz would you review this one as well? It should merge after #147, and I don't have merge rights here.

The red GitHub Actions check is the intended failure and will clear once #147 merges. This PR adds the regression test for the defect #147 fixes, so against master as it stands the test correctly reports that defect. The CircleCI failure pattern is the pre-existing project-wide one described in #147.

93% tests passed, 1 tests failed out of 15
The following tests FAILED:
	 15 - testMeta15UncompressChunkBoundary (Failed)

Exactly one test fails, and it is the new one. The other 14 pass.

Proposed sequence
  1. Merge metaUtils: consume the trailer when the output buffer fills first #147 (the fix).
  2. I rebase this branch on the new master.
  3. Re-run CI here; the new test should then pass and the PR goes green.

The two changes touch different regions of metaUtils.cxx, share no commits, and merge cleanly in either order — but the test only passes with the fix present, so the order above is the one that produces a green result.

Verification that the test detects this specific defect

I disabled only the fix loop from #147 with #if 0, keeping the new chunk-size API, and re-ran:

test 15 rest of suite
without the fix FAILzlib error -5, valid stream rejected 14/14 pass
with #147 pass 14/14 pass

So the failure tracks the fix rather than an unrelated build difference. The test uses 64 KiB of data and runs in 0.01 s.

Happy to drop the test, or move MET_SetMaxChunkSize behind a private header, if the added public API is not welcome — the fix in #147 stands on its own either way.

…e trailer case

MET_MaxChunkSize was a file-local constexpr of 1 GiB, so the input-chunking
path in MET_PerformCompression and MET_PerformUncompression could only be
reached by a test allocating several gigabytes. Expose MET_SetMaxChunkSize
and MET_GetMaxChunkSize so the boundary can be placed where a test needs it,
following the existing META_DEBUG precedent for a settable diagnostic knob.
Values of zero or less are ignored.

testMeta15UncompressChunkBoundary uses this to put a chunk boundary inside
the 8-byte gzip trailer with 64 KiB of data, and asserts both that a valid
stream is accepted and that a corrupt CRC trailer is still rejected. The
test runs in 0.01 s.

The accepted case fails on master today; it passes with the fix in Kitware#147.
This change is independent of that one and can merge in either order.

Assisted-by: Claude Code -- test design and verification
@hjmjohnson
hjmjohnson force-pushed the add-chunk-size-api-and-trailer-test branch from 9a10b9a to bab6acf Compare September 4, 2026 23:10
@hjmjohnson

Copy link
Copy Markdown
Contributor Author

Thanks for merging #147, @dzenanz. Rebased on top of it — GitHub Actions is green now (15/15 tests pass, including the new testMeta15UncompressChunkBoundary), so this is ready for review and merge whenever you have a moment.

The remaining red CircleCI check is the pre-existing project-wide one: /usr/src/MetaIO/test/ci/test.sh reports canceled with failed_reason: None on every recent build including master. Details in #147.

@dzenanz
dzenanz merged commit 11606dc into Kitware:master Sep 5, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants