Skip to content

Fix shuffled ledger readability after recovery - #8266

Merged
Amaury Chamayou (achamayou) merged 11 commits into
mainfrom
achamayou-probable-garbanzo
Sep 3, 2026
Merged

Fix shuffled ledger readability after recovery#8266
Amaury Chamayou (achamayou) merged 11 commits into
mainfrom
achamayou-probable-garbanzo

Conversation

@achamayou

@achamayou Amaury Chamayou (achamayou) commented Sep 2, 2026

Copy link
Copy Markdown
Member

Summary

Suite tests reuse networks across operations including disaster recovery. governance_history.test_ledger_is_readable previously inferred the start of locally readable history from startup_seqno, but that field only records the startup snapshot.

That inference fails after ledger-based recovery. Committed chunks from the previous service may be supplied through read-only ledger directories, while /node/ledger_chunk deliberately serves only the main ledger directory. A recovered primary can therefore report startup_seqno: 0 without being able to serve seqno 1. Conversely, a snapshot-less joiner replays the complete ledger into its main directory and can serve from seqno 1.

Select the local-only download watermark from the node lifecycle already tracked by the test infrastructure:

  • Nodes started from a snapshot begin at startup_seqno + 1.
  • A primary launched with StartType.recover and no snapshot begins at /node/network.current_service_create_txid, the first transaction written to its main ledger for the latest recovery.
  • Genesis nodes and snapshot-less joiners begin at seqno 1.

The selection now lives in the shared local-only ledger download helper, so callers get consistent recovery-safe behavior. It is constant-time and requires no REST API change.

Failure sequence

Long Test run 33634419271 shuffled full_test_suite with seed 1788356297. This placed recovery.test_recover_service_aborted immediately before governance_history.test_ledger_is_readable.

The recovered primary, node 10, reported startup_seqno: 0 and had committed through seqno 415. The test nevertheless began at seqno 1. That chunk belonged to previous-service read-only recovery input, so node 10 returned ResourceNotFound repeatedly for 20 seconds before the test failed. This was not replication lag; the current chunk containing the target seqno was already committed.

With the new watermark, the same ordering reads recovered primary 10 from its current service creation transaction while recovery joiner 11 still reads its locally served history from seqno 1.

Testing

  • SHUFFLE_SUITE_SEED=1788356297 ./tests.sh --timeout 600 -R '^full_test_suite$' -VV - 44/44 passed; recovered primary started at the latest DR transaction and recovery joiner started at seqno 1
  • SHUFFLE_SUITE_SEED=15 ./tests.sh --timeout 600 -R '^full_test_suite$' -VV - 44/44 passed; genesis nodes started at seqno 1 and the snapshot joiner started at startup_seqno + 1
  • scripts/ci-checks.sh -f

Follow-up to #8248.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 2, 2026 14:44
@achamayou
Amaury Chamayou (achamayou) requested a review from a team as a code owner September 2, 2026 14:44

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.

🟡 Changes recommended

The recovery-aware “start seqno” watermark is implemented only in this test, while the shared local-only ledger download helper’s default start calculation remains inconsistent with ledger-based recovery and may still cause similar failures in other call sites.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates the governance_history end-to-end test so it reads only the portion of each node’s ledger that is actually served by the /node/ledger_chunk API after ledger-based disaster recovery, avoiding repeated 404s when earlier (read-only) recovery input chunks are not API-readable.

Changes:

  • Adjusts the test’s intent/wording to focus on “API-readable” ledger chunks.
  • Computes a per-node starting seqno watermark using /node/state.startup_seqno (snapshot-started) or /node/network.current_service_create_txid (ledger-started after recovery).
  • Passes start_seqno into get_ledger_from_api(..., local_only=True) so the test reads the locally served suffix rather than attempting to read from seqno 1.

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md
File summaries
File Description
tests/governance_history.py Updates test_ledger_is_readable to derive a node-specific readable start seqno and pass it to the local ledger download helper.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/governance_history.py Outdated
Comment thread tests/governance_history.py Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread tests/infra/node.py Outdated
@achamayou
Amaury Chamayou (achamayou) merged commit c87ece6 into main Sep 3, 2026
13 checks passed
@achamayou
Amaury Chamayou (achamayou) deleted the achamayou-probable-garbanzo branch September 3, 2026 21:58
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