Skip to content

More robust ledger and snapshot invariants#8035

Merged
achamayou merged 13 commits into
microsoft:mainfrom
eddyashton:erroneous_snapshot_ledger_contiguity
Jul 8, 2026
Merged

More robust ledger and snapshot invariants#8035
achamayou merged 13 commits into
microsoft:mainfrom
eddyashton:erroneous_snapshot_ledger_contiguity

Conversation

@eddyashton

Copy link
Copy Markdown
Member

This PR makes a previous edge case that was surprisingly consistently triggered in #8007 into a deterministic repro, and then fixes the relevant test cases.

On network shutdown we now (since #7902) check some invariants about ledger and snapshot files:

        LOG.info("All nodes stopped")
        if check_file_invariants:
            self.ledger_files_invariant(**kwargs)
            self.snapshot_files_invariants(**kwargs)

One thing these invariant checks all have in common is that they're completely oblivious to "future snapshots". If we have a node that ran from seqnos 0 to 50, and a snapshot at 1000, we ignore the snapshot entirely. When we create artificial snapshots (to test empty, corrupt, or ??? handling), we had a habit of calling it snapshot_1000_1500.committed, which was well past the end of the relevant services. The branch above extended one of these ledgers slightly, such that it crossed seqno 1000, and these artificial snapshots were suddenly considered by the invariant checks, causing failures.

The first change is to make these failures consistently reproduced, by changing the magic filename to something that's actually considered. This was annoyingly trial-and-error - there's a risk that we match a real snapshot index, or are still past the service end? But this drop to snapshot_500_510.committed has triggered all the tests I've looked at.

The second is to fix the invariant checks, in 2 ways:

  • When parsing the log to work out what snapshot a node started at (the base point we expect the ledger to be contiguous from), we handle a few more kinds of "I'm not using this snapshot" error. Previously had a single regex, but we have other kinds of corruption that produce different logs.
  • When testing an empty snapshot, we manually delete it in the same test before getting anywhere near invariant checks. This is a little unsatisfying, it feels like it would be neater to rename the empty file to .ignored (as we do for corrupt snapshots). But that's a surprisingly awkward (and wide-blast-radius) change, so we punt on it for now.

There is a check_file_invariants flag, that we could use to disable all invariant checks on this network. But I think that would lose a huge amount of beneficial coverage, given these tests are running within the operations.run_file_operations test. I think we want to be explicit about exactly which file operations/tamperings retain these invariants, rather than blindly assuming they all break them (clearly they mostly don't!).

Copilot AI review requested due to automatic review settings July 7, 2026 12:40
@eddyashton
eddyashton requested a review from a team as a code owner July 7, 2026 12:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the operations/e2e test suite and test-infra invariant parsing so that ledger/snapshot file invariant checks deterministically exercise (and correctly handle) previously “future snapshot” edge cases triggered by artificial snapshot filenames.

Changes:

  • Adjusts test-created snapshot/ledger filenames so they are consistently considered by invariant checks (rather than being ignored as “future” files).
  • Improves ledger_files_invariant() startup-seqno detection by recognizing additional “snapshot rejected” log patterns.
  • Removes the deliberately empty staged snapshot file in test_empty_snapshot to prevent shutdown-time snapshot invariants from treating it as a real committed snapshot.

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/infra/network.py Broadens log parsing in get_startup_seqno() to better detect when a local snapshot was not actually used.
tests/e2e_operations.py Makes snapshot/ledger tampering tests deterministic under invariant checks by using “in-range” filenames and removing the empty staged snapshot before teardown.

Comment thread tests/infra/network.py Outdated
Comment thread tests/infra/network.py
@achamayou
achamayou merged commit e22b759 into microsoft:main Jul 8, 2026
18 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.

3 participants