Fix local ledger downloads after snapshot recovery

[Eddy Ashton (eddyashton)]

Bug

This fixes an actual test-infrastructure failure, not code readability or production recovery behavior. In the original #8117 ASAN run, recovery node 18 loaded snapshot 423, replayed the old service through 436, and created the new service at 437. The local-only downloader nevertheless started at 424 and received 404 responses for 20 seconds.

startup_seqno identifies the startup snapshot, not the beginning of locally HTTP-readable history. Ledger::committed_ledger_path_with_idx searches only the main ledger directory and excludes recovery-marked files. The test infrastructure supplies already-committed old-service files through read-only recovery directories. If the relevant old-service chunk is still current at shutdown, it instead goes into the main directory and can later be served; this is why the bad watermark sometimes works.

The original incident used the default, unshuffled suite order. A plain recovery/readability pair reproduced both failure and success in a normal Debug build: ASAN is not required, but chunk commitment timing matters.

Fix

Check StartType.recover before checking the startup snapshot. For local-only downloads, recovery-start nodes begin at current_service_create_txid; snapshot joiners retain startup_seqno + 1, and genesis/snapshot-less joiners retain 1. The recovery bound is conservative: earlier history may be locally available, but is not guaranteed to be. Explicit starting-sequence requests and normal non-local-only downloads are unchanged. No production endpoint or timeout changes.

Regression coverage

Removed the standalone e2e test, CMake registration, and CI bucket entry. A 16-line recovery variant now:

1. Copies committed snapshots using the existing helper.
2. Forces and waits for a committed ledger chunk beyond the copied snapshot.
3. Recovers using that unchanged snapshot copy.

This reliably places the first post-snapshot entry in read-only recovery input. The variant is included alongside the existing recovery cases in full_test_suite, and a named recovery_ledger two-step ordering runs it immediately before governance_history.test_ledger_is_readable. No private-helper or hard-coded 404 assertions remain.

Validation

All executions used build-directory tests.sh:

• Reduced two-step sequence, normal Debug: old helper failed 3/3 with the original download timeout; fixed helper passed 3/3.
• Final full suite, default order: 44 passed, 1 existing quorum-requirement skip (test_node_replacement).
• Final full suite with SHUFFLE_SUITE=1 and SHUFFLE_SUITE_SEED=1, 2, and 3: 45/45 passed for each seed; the new recovery variant executed successfully in all runs.
• scripts/ci-checks.sh -f: all checks passed.
• Independent review of the lightweight replacement: no significant issues found.

Earlier normal and ASAN runs verified the helper fix using the now-removed standalone repro; the replacement suite coverage above was validated in the normal build.

Follow-up to #8266; related to #8248.

[Copilot]

🟡 Changes recommended

The new test asserts a 404 for /node/ledger_chunk?since=... where the server logic should typically return a permanent redirect (308) when other nodes exist, making the test likely to fail.

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

Pull request overview

This PR fixes how the test infrastructure determines a node’s locally-servable ledger “watermark” during service recovery (so recovery nodes don’t incorrectly use a startup snapshot boundary), and adds a focused end-to-end test to prevent regressions in recovery-ledger readability.

Changes:

• Adjust Node._get_local_ledger_start_seqno() to prioritise recovery semantics (use current_service_create_txid for recovery nodes) before considering startup_seqno.
• Add a new recovery_ledger_readability e2e test covering snapshot and non-snapshot recovery paths and verifying expected watermark behaviour.
• Register the new test in CMake and CI bucket configuration.

Custom instructions used

• None (no repository instruction files from .github/copilot-instructions.md or .github/instructions/ were loaded via tools in this review).

File summaries

File	Description
tests/recovery_ledger.py	New e2e test validating recovery ledger readability/watermarks across snapshot/no-snapshot scenarios.
tests/infra/node.py	Fixes local ledger start seqno selection for recovery nodes to avoid using snapshot-derived startup seqno.
tests/ci-buckets.txt	Adds the new recovery ledger readability test to bucket_b.
CMakeLists.txt	Registers the new recovery_ledger_readability e2e test target.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

---

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

[Amaury Chamayou (achamayou)]

I do not understand what this PR aims to do, is it only improving readability (not a bug), or fixing an actual test failure that was surface with ASAN as claimed at the bottom of the description? What is the test failure?

I see two changes, a new test, and a change to _get_local_ledger_start_seqno() that does not seem right to me. I am unable to reconcile them with the description.

[Eddy Ashton (eddyashton)]

I do not understand what this PR aims to do, is it only improving readability (not a bug), or fixing an actual test failure that was surface with ASAN as claimed at the bottom of the description? What is the test failure?

I see two changes, a new test, and a change to _get_local_ledger_start_seqno() that does not seem right to me. I am unable to reconcile them with the description.

Here's the ASAN failure: https://github.com/microsoft/CCF/actions/runs/34121620500/job/101740738617

It's fixing a "ledger readability" bug, as in "what is readable from the ledger". Not code readability. Still iterating on a more sensible repro.