Skip to content

fix(taiko-client-rs): abort event sync when the execution engine rewinds below derived state - #22004

Draft
davidtaikocha wants to merge 4 commits into
mainfrom
fix/driver-execution-rewind-escape
Draft

fix(taiko-client-rs): abort event sync when the execution engine rewinds below derived state#22004
davidtaikocha wants to merge 4 commits into
mainfrom
fix/driver-execution-rewind-escape

Conversation

@davidtaikocha

Copy link
Copy Markdown
Collaborator

The bug

After an unclean execution-engine restart (OOMKill), reth rewinds to its last persisted block, losing the in-memory tail — while head_l1_origin still points above the new head. The running event syncer then retries the same proposal forever:

WARN proposal derivation failed; retrying err=Sync(Derivation(BlockUnavailable(16211429)))
WARN orphan-proof ancestry walk exceeds cap; keeping proposal log retryable

The missing parent can never reappear on its own — event derivation is the only producer of that range and it is stuck behind this proposal; gossip drops the gap blocks as stale, and the safe resume-head resolution (resolve_resume_head_block_number, checkpoint path) only runs at driver startup. The node freezes silently (and keeps reporting Ready) until an operator restarts the pod.

Two production incidents on hoodi l2-node-reth-0: 2026-08-02 (~14.5h frozen) and 2026-08-07 (~4.5h frozen, chain head alert), both ended by a manual pod delete. The rewind trigger (an alethia-reth memory leak on sub-5-CPU nodes) is being removed by taikoxyz/k8s-configs#1478, but any future EL rewind under a running driver reproduces the freeze.

The fix

When a proposal fails with BlockUnavailable(n), probe the execution head. Three consecutive observations of n strictly above the live head classify the failure as a rewind: abort the event-sync run (SyncError::ExecutionEngineRewound) instead of retrying. The whitelist runner already treats an event-syncer exit as fatal, so the driver process exits and restarts (k8s restarts only the driver container — the EL is untouched), re-resolves a safe resume head (checkpoint-synced, or min(head, head_l1_origin)), and re-derives the lost range. This is exactly the manual remedy from both incidents, automated.

Deliberately out of scope: re-resolving the resume head in-place. Startup resolution is the single tested authority for choosing a safe resume point; duplicating it mid-run would fork that logic. The abort-and-restart path reuses it wholesale.

Conservative by construction:

  • a failed head probe proves nothing → ordinary retry stays in charge;
  • BlockUnavailable at/below the head is not a rewind → streak resets, existing retry/canonicality classification untouched;
  • the three-observation streak filters head-probe races around an in-flight insert (~1s of backoff).

Adds driver_event_execution_rewind_aborts_total.

Tests

TDD: the abort test was written first and failed on main by looping exactly like production (virtual-clock timeout with the same retry warns), then the fix made it pass.

  • process_log_batch_aborts_after_persistent_block_unavailable_above_execution_head — bounded abort at the threshold with ExecutionEngineRewound { missing_block, execution_head }
  • process_log_batch_keeps_retrying_block_unavailable_at_or_below_execution_head — boundary guard (not a rewind → retry then succeed)
  • process_log_batch_keeps_retrying_when_execution_head_probe_fails — missing evidence must not abort

cargo test -p driver --lib: 129 passed. Both CI clippy passes clean; nightly rustfmt applied.

🤖 Generated with Claude Code

davidtaikocha and others added 2 commits August 7, 2026 23:06
…nds below derived state

After an unclean execution-engine restart (e.g. OOMKill), the engine can
rewind below blocks derivation already produced while head_l1_origin
still points above the head. The running event syncer then retries the
same proposal forever with BlockUnavailable(parent): the missing parent
can never reappear on its own because event derivation itself is the
only producer of that range, and the safe resume-head resolution only
runs at driver startup. The node silently freezes until an operator
restarts the pod (hoodi l2-node-reth-0 incidents on 2026-08-02 and
2026-08-07, ~4h+ of frozen head each).

Probe the execution head when a proposal fails with BlockUnavailable.
Three consecutive observations of the missing block sitting strictly
above the live head classify the failure as a rewind and abort the
event-sync run instead of retrying. The runner already treats an
event-syncer exit as fatal, so the driver restarts and re-resolves a
safe resume head (checkpoint-synced, or min(head, head_l1_origin)),
which re-derives the lost range — exactly the manual pod-restart
remedy, automated, and without touching the execution engine container.

A failed head probe or a missing block at/below the head keeps the
ordinary retry and canonicality classification in charge. Adds the
driver_event_execution_rewind_aborts_total counter.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@davidtaikocha
davidtaikocha marked this pull request as draft August 7, 2026 14:11
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🐋 DeepSeek Code Review

🔴 Critical Issues

None.

🟡 Warnings

  • Ordering mismatch with design spec: The code classifies a rewind before checking whether the proposal log is orphaned. The PR description and design spec state that “Permanently orphaned L1 proposal logs are still skipped before rewind classification”. If a proposal is orphaned and the execution engine is simultaneously observed to be below the derived range, the current ordering will abort event sync (―safe, but unnecessary restart) rather than silently skipping the orphaned log. This is not a correctness bug, but it violates the stated invariant and could cause avoidable driver restarts if orphaned logs coincide with a genuinely rewound engine.

  • Rewind streak is not reset when the execution head cannot be queried: The design spec says “If the head cannot be queried … the consecutive observation count resets”, but the code leaves the streak untouched on a probe failure (probe_execution_rewind returns None and the streak remains). This makes the rewind abort slightly more aggressive than specified: a transient execution‑engine hiccup will not wipe the streak, so the threshold of three above‑head observations might be reached after fewer uninterrupted successful probes. In practice this is safe because the streak only increments on a successful head query returning a block below the missing number, but it deviates from the documented intent. Either the code should reset the streak on probe failure, or the spec should be updated to match.

  • False‑positive abort possible under extreme load: If the execution engine is genuinely inserting the missing block but the insertion is slow (e.g., heavy I/O), the head might lag behind for several retry cycles, accumulating three consecutive above‑head observations and triggering an abort. The restart is not harmful, but it may cause a brief pause in event sync when the engine would have caught up on its own. The threshold of three is deliberately low to catch genuine rewinds fast; this trade‑off is acceptable but worth noting for operators.

🔵 Suggestions

  • Consider resetting the rewind streak when the head probe fails, to align with the spec and to require unbroken evidence of a post‑crash rewind before aborting.
  • The ProposalRetryError::Abort path currently discards the original derivation error information. If you later want to diagnose why the engine rewound, consider logging the original BlockUnavailable details alongside the new ExecutionEngineRewound error.
  • The new metric driver_event_execution_rewind_aborts_total could be accompanied by a gauge recording the last observed (missing_block, execution_head) gap for easier post‑mortem analysis.

🟢 What Looks Good

  • The new SyncError::ExecutionEngineRewound variant cleanly separates rewind aborts from retryable errors, and the error message includes the critical block numbers.
  • The three‑consecutive‑observation threshold avoids false alarms from momentary engine head races while reacting promptly to a genuine rewind.
  • Each proposal log maintains its own AtomicU32 streak, avoiding cross‑log interference.
  • The head‑probe failure path (None return) correctly prevents an abort when evidence is missing, keeping the ordinary retry alive.
  • Test coverage is thorough: three focused tests cover the abort scenario, the boundary case (missing_block == execution_head), and the probe‑failure case, all with virtual time so they run deterministically. The mock production path (UnavailableFor / UnavailableOnceFor) gives clean control.
  • The design spec is comprehensive and traces the decision loop precisely, making the intent clear for future readers.
  • No observable race conditions, no resource leaks, and all edge cases appear to be handled in a monotonic, safe manner.

Automatically triggered on PR update • model: deepseek-v4-pro

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d406fb0b74

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/taiko-client-rs/crates/driver/src/sync/event.rs
Comment thread packages/taiko-client-rs/crates/driver/src/sync/event.rs Outdated
Comment thread packages/taiko-client-rs/crates/driver/src/sync/event.rs Outdated
davidtaikocha and others added 2 commits August 8, 2026 09:36
…hans, and mixed evidence

Address three review findings on the execution-rewind escape hatch:

- decode_log_to_event_context reused BlockUnavailable to carry an L1
  block number when the proposal's source block is missing from the L1
  provider view, so on chains where L1 height exceeds the L2 head three
  transient L1 misses would terminate the driver in a restart loop.
  Introduce DerivationError::SourceBlockUnavailable for that path; the
  rewind probe only ever sees L2 numbers now.

- The probe ran before proposal_log_canonicality, so a finalized-orphan
  log whose derivation also reported an above-head parent would abort
  the driver instead of being skipped. Orphanhood now settles first;
  rewind evidence only accumulates on non-orphaned logs.

- The streak counter ignored WHICH block was missing, letting
  observations of different blocks pool toward the threshold. Replace
  it with an identity-keyed RewindStreak: a different missing block
  restarts the count. A failed head probe still deliberately leaves the
  streak intact — a flaky probe must not indefinitely postpone escape
  from a real rewind.

New coverage: finalized orphan is skipped (not escalated), missing L1
source blocks keep retrying past the threshold, and streak identity
semantics.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.

1 participant