test(integration): catch on-disk storage format breaks pre-merge - #4003
Merged
Conversation
hubcio
force-pushed
the
storage-compat-test
branch
from
September 1, 2026 06:58
7b6facc to
ddfb920
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4003 +/- ##
============================================
+ Coverage 84.97% 85.02% +0.04%
Complexity 1402 1402
============================================
Files 1230 1236 +6
Lines 181408 181465 +57
Branches 147703 147758 +55
============================================
+ Hits 154157 154286 +129
+ Misses 23199 23105 -94
- Partials 4052 4074 +22
🚀 New features to boost your workflow:
|
spetz
previously approved these changes
Sep 2, 2026
A change to the on-disk format could ship undetected, and every way it fails is silent. A partition whose durable state cannot be read is tombstoned while boot still exits 0. Topic options degrade per key with no log line at all. Segment files carry no version stamp, so a layout change is a misread rather than an error. The breaking:storage label already promised this check and was wired to nothing. A server built from origin/master seeds a data directory holding sealed and active segments, a forced metadata checkpoint, consumer and group offsets, a purged topic, users and permissions. The binary is then swapped to the build under test and restarted against that same directory, and everything must read back. One process and one test rather than a two-phase handoff: restart() re-reads executable_path on every call and stop() never touches the data directory, so no staging area or cross-process manifest is needed. Both binaries are built explicitly, because the integration crate has no dependency on the server and nextest alone would run whatever stale binary sits in target, comparing master against master and reporting green forever. The assertions check data, never that the server booted. Topic options are seeded non-default so a lost key cannot masquerade as a re-derived default, and the segment set is compared byte for byte to catch a truncating misparse that a readback would miss. breaking:storage skips the check, resolved from live labels rather than the webhook payload so it still works on a re-run.
Review of the compat lane found three ways it could pass with the on-disk format broken. The purged topic was empty on both sides of the swap: a purge.gen that fails to decode reads as 0 by design, the reconciler re-purges, and messages_count == 0 held either way. Users were seeded with every permission bit true, so a dropped or reordered field read back identical, and nothing authenticated with the seeded password or token after the swap. Stream recovery was a bare count, which a corrupted name survives because later reads resolve by id. Messages appended after the purge now have to survive the swap at offset 0, permissions alternate bits at every level and compare whole, fresh clients log in with the password and the raw token, and streams compare as an id-to-name map. The lane itself could also be skipped or misdirected. rust-configs owns the partition and metadata schema knobs but was not a dependency, and the module wiring files were not watched. The baseline was live origin/master, which can be newer than the master a pull_request merge ref was cut against; the script now takes the merge commit's first parent, read from the raw object because HEAD^1 does not resolve in a depth-1 checkout. A relative CARGO_TARGET_DIR is made absolute before the worktree build, and test-storage-compat no longer runs the DAG and coverage setup it never reads.
The baseline fetch passed --depth=1 unconditionally. On the CI checkout that is a no-op, but on a developer's full clone git does not skip anything: it writes the fetched tip into .git/shallow and grafts every older commit off the history, for every worktree that shares the repository. A local run left master cut at its tip and git status counting the whole old history as remote-only commits. Pass --depth=1 only when the repository is already shallow.
hubcio
force-pushed
the
storage-compat-test
branch
from
September 2, 2026 07:55
779a4ee to
52185f1
Compare
The checkpoint fired on 250 empty streams before any topic, group, user or token existed, so every rich type reached the second boot through WAL replay alone and a broken snapshot encoding passed. Messages had id 0, no user headers, one payload length and partition 0, and the readback compared a payload prefix, so a misread id, timestamp, header or batch partition id passed too. A purge generation decoding above the committed one kept the messages the test checks while silently suppressing every later purge. Seed the rich metadata before the checkpoint drive and one more of each type after it, so both encodings are exercised. Give messages explicit ids, typed headers, varying lengths and a non-zero partition, capture the baseline's own readback and compare every stored field after the swap. Purge again under the build under test and require it to take effect; that topic is excluded from the byte comparison by design. The script now passes --retries 0 and --ignore-default-filter instead of relying on the absence of --profile ci, and .config/nextest.toml requires nextest 0.9.77, the floor for those flags. The install step replaces a cache-restored binary that fails the version check instead of aborting the lane later.
spetz
approved these changes
Sep 2, 2026
mmodzelewski
approved these changes
Sep 2, 2026
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
3 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.
A change to the on-disk format could ship undetected, and every
way it fails is silent. A partition whose durable state cannot be
read is tombstoned while boot still exits 0. Topic options
degrade per key with no log line at all. Segment files carry no
version stamp, so a layout change is a misread rather than an
error. The breaking:storage label already promised this check and
was wired to nothing.
A server built from origin/master seeds a data directory holding
sealed and active segments, a forced metadata checkpoint,
consumer and group offsets, a purged topic, users and
permissions. The binary is then swapped to the build under test
and restarted against that same directory, and everything must
read back.
One process and one test rather than a two-phase handoff:
restart() re-reads executable_path on every call and stop() never
touches the data directory, so no staging area or cross-process
manifest is needed. Both binaries are built explicitly, because
the integration crate has no dependency on the server and nextest
alone would run whatever stale binary sits in target, comparing
master against master and reporting green forever.
The assertions check data, never that the server booted. Topic
options are seeded non-default so a lost key cannot masquerade as
a re-derived default, and the segment set is compared byte for
byte to catch a truncating misparse that a readback would miss.
breaking:storage skips the check, resolved from live labels
rather than the webhook payload so it still works on a re-run.