Skip to content

Wait for backup ledger chunks before reading - #8258

Merged
Amaury Chamayou (achamayou) merged 4 commits into
mainfrom
achamayou-review-pr-8242
Sep 2, 2026
Merged

Wait for backup ledger chunks before reading#8258
Amaury Chamayou (achamayou) merged 4 commits into
mainfrom
achamayou-review-pr-8242

Conversation

@achamayou

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

Copy link
Copy Markdown
Member

Summary

Use the existing ledger recovery timeout for local-only ledger downloads so backups can receive and flush forced chunks before they are read.

Closes #8248

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 24512232-71af-4193-b26d-1b8acbb02d5a
Copilot AI lite review requested due to automatic review settings September 2, 2026 06:47
@achamayou
Amaury Chamayou (achamayou) requested a review from a team as a code owner September 2, 2026 06:47

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 new wait uses the recovery timeout, but the subsequent get_ledger_from_api() call still uses its 5s default, so the flake may persist under slow/perturbed runs.

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

Pull request overview

Fixes a race in the governance history end-to-end test by synchronising on ledger chunk availability per node before attempting local ledger reads, reducing flakiness in slower CI configurations.

Changes:

  • Wait for each node (primary + backups) to expose the forced ledger chunk before reading its local ledger.
  • Use the existing ledger recovery timeout for the new per-node wait.

Custom instructions used:

  • None (no .github/copilot-instructions.md / .github/instructions/* files were loaded during this review)
File summaries
File Description
tests/governance_history.py Adds per-node wait_for_ledger_chunk() to synchronise backups before reading local ledger chunks.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/governance_history.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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 added wait_for_ledger_chunk call can follow redirects and may not actually guarantee the chunk is available locally on backups, undermining the intended synchronization.

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

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

Comment thread tests/governance_history.py Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@achamayou
Amaury Chamayou (achamayou) merged commit 35036ac into main Sep 2, 2026
13 checks passed
@achamayou
Amaury Chamayou (achamayou) deleted the achamayou-review-pr-8242 branch September 2, 2026 10:07
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.

Flaky test: governance_history.test_ledger_is_readable does not wait for backups before reading their ledger

3 participants