fix: exempt consensus recovery reads from the 30s MDBX read-txn timeout

[procdump]

Problem

A validator crash-looped into an unrecoverable "database is corrupted" state.
Root cause was a transient storage stall, not a logic bug.

Timeline from the incident logs:

• ~15:10:38 — the node's storage stalls; execution freezes (block stuck at
  4776429 for ~78s while consensus rounds keep advancing).
• 15:11:08 / 15:11:24 / 15:11:56 — three separate read transactions each hit
  the 30s max_read_transaction_duration cap ("Long-lived read transaction has
  been timed out"). Multiple unrelated readers timing out at once is an I/O
  stall signature, not a slow query.
• 15:11:56 — the DAG-rebuild read in construct_dag_from_cert_store
  (after_round) is one of them. Its .expect("database available") turns the
  timeout into a panic, killing the process mid-write.
• 15:11:57 onward — on restart MDBX open() returns MDBX_CORRUPTED (write
  map + SafeNoSync, killed mid-writeback during the stall). Crash loop with
  exponential backoff, never recovers.

The scan that panicked was tiny (~600 rounds; a healthy rebuild does 500 rounds
in ~76ms), which rules out "large scan" and confirms the stall.

Fix

The read-txn timeout exists to stop leaked/hung readers from pinning the MVCC
free-list and starving the writer — worth keeping. But it should not guillotine
a bounded, one-shot recovery scan that legitimately runs long under I/O
pressure. The repo already has disable_long_read_safety() for exactly this
(used by the epoch-manager tally walk); after_round just never opted in.

• Add a self-documenting ReadTimeout { Enforced, Exempt } param to
  after_round.
• Both recovery callers pass Exempt:
  • construct_dag_from_cert_store (the crash path) — keeps its .expect,
    which now only fires on a real DB fault (panic-at-boot is correct there).
  • prime_consensus — now returns the correct cert_store_round instead of
    degrading to a stale value on timeout.
• The 30s cap still guards every other reader in the system.

[github-actions]

Claude finished @procdump's task in 2m 29s —— View job

---

Code Review

• Gather context and understand the request
• Read changed files and git diff
• Analyze code for correctness, safety, and Rust best practices
• Post review findings

---

Overall: This is a clean, well-reasoned fix that correctly addresses the root cause. The design is minimal and targeted — it exempts exactly the two recovery-path callers that need it, leaves everything else enforced, and follows the established disable_long_read_safety() pattern already used in epoch_manager/utils.rs and rewards/src/lib.rs.

---

What the change does

1. Adds a self-documenting ReadTimeout { Enforced, Exempt } enum to certificate_store.rs.
2. Threads it through CertificateStore::after_round (trait + impl).
3. Both recovery callers pass Exempt:
   • construct_dag_from_cert_store in state.rs — the crash path.
   • prime_consensus in state-sync/src/lib.rs — was silently degrading to a stale cert_store_round on timeout.
4. The test correctly uses Enforced (normal, latency-sensitive path).

---

Findings

Confirmed correct

• All callsites covered. A grep over the codebase shows exactly three callers of after_round: both recovery callers use Exempt, the test uses Enforced. No missed sites. origins_after_round (called from certificate_fetcher.rs) is a live-operation path and is correctly left alone.

• Timing of disable_long_read_safety() is right. It is called inside with_read_txn's closure, after the MDBX transaction is open but before any cursor/read operations begin — the only valid window.

• LayeredDbTxMut panic unreachable here. layered_db.rs:305-306 panics on disable_long_read_safety() for write transactions, but after_round calls with_read_txn, which yields a read-only LayeredDbTx. That impl correctly delegates to self.db.disable_long_read_safety().

• MDBX emits a warn! on each exemption ("Disabling long read safety for database transaction"), consistent with the existing callers. Good for operator observability.

• prime_consensus error handling is now correct. Before this PR, a 30 s timeout caused after_round to return Err, the warning path was taken, and cert_store_round was left at its stale prior value. With Exempt, that Err branch is only reachable on a genuine DB fault, which is the right behavior.

• .expect("database available") in construct_dag_from_cert_store is now safe to keep. It previously could fire on a transient stall; with the timeout exempt, it only fires on a real database error, where a panic-at-boot is the correct response.

---

Nit: doc comment says "potentially large" — the issue is latency, not size

certificate_store.rs:94

/// `timeout` controls whether this (potentially large) scan is subject to the read-transaction
/// timeout; recovery-path callers pass [`ReadTimeout::Exempt`].

The incident showed a ~600-round scan hitting the 30 s cap during an I/O stall — clearly not a "large" scan. The framing "potentially large" might mislead future readers into thinking only big scans need exemption. A tighter description would be: "a bounded, one-shot recovery scan that may outlast the timeout under I/O pressure". The per-variant doc on Exempt ("Opt out of the timeout for a bounded, latency-insensitive scan") already gets this right; the trait doc is slightly inconsistent.

Fix this →

---

Minor: ReadTimeout is defined in certificate_store.rs but is conceptually DB-layer

This is a question of placement, not correctness. The enum is only used by after_round today, so collocating it with that method is fine. If disable_long_read_safety() exemptions spread to other scan methods (origins_after_round, last_two_rounds_certs, etc.), promoting ReadTimeout to the storage crate root or a dedicated traits.rs would avoid circular references. For now, the current location is acceptable.

---

Question: should prime_consensus log a warning on Err specifically mentioning this changed behavior?

state-sync/src/lib.rs:97-104:

Err(e) => {
    warn!(
        target: "rayls-consensus-state-sync",
        err = ?e,
        "prime_consensus: after_round failed; cert_store_round left at prior value"
    );
    (None, 0)
}

The Err path is now only reachable on a real DB fault (not a timeout). The message "cert_store_round left at prior value" is accurate but understates the severity — if we're here, the DB itself failed. This is subjective, but a warn might be worth upgrading to an error since this is no longer "the harmless timeout degradation path" but a genuine storage failure. Not a blocker.

---

Summary

The fix is correct and production-ready. The ReadTimeout API is clean and self-documenting, the two recovery callers are correctly exempted, and the approach is consistent with the existing disable_long_read_safety() pattern in the codebase. One minor doc nit and one optional log-level consideration; neither is a blocker.

[github-actions]

Claude finished @procdump's task in 2m 12s —— View job

---

PR Review: fix: exempt consensus recovery reads from the 30s MDBX read-txn timeout

• Gather context from PR description and changed files
• Read changed files and analyze code
• Provide comprehensive review feedback

---

Summary

This is a clean, minimal, well-motivated fix for a real production incident. The root cause analysis is thorough and the solution is correctly scoped. The design choices (an explicit enum over a bool, using the existing disable_long_read_safety() primitive) are good.

No blocking issues. A few observations below.

---

Correctness

ReadTimeout enum — correct design choice.
Using an enum over bool avoids call-site ambiguity (after_round(x, true) would be opaque). The Enforced/Exempt naming reads clearly at every call site. No issue.

disable_long_read_safety() is called in the right place.
In certificate_store.rs:263-266, the call happens immediately after the transaction opens, before any iterator or DB access. This is correct — the timeout can't fire between the txn open and the safety disable if the call is first.

.expect("database available") in construct_dag_from_cert_store (state.rs:110).
With Exempt, the only errors that can now reach this .expect are genuine DB faults (corruption, I/O failure, etc.), not timeout resets. Panicking at boot on a real DB fault is the right behavior — continuing with a corrupt DAG would be worse.

prime_consensus error handling (state-sync/src/lib.rs:97-105).
Before this PR, a 30s timeout would cause after_round to return Err, hit the warn! arm, and silently leave cert_store_round at its prior (possibly stale) value. With Exempt, any Err is now a genuine DB error. The graceful-degrade path (warn + leave cert_store_round unchanged) is still acceptable for startup — crashing the whole node on a cert-store-round read failure would be disproportionate — but worth documenting that this degrades the rejoin gate accuracy.

with_read_txn scoping.
disable_long_read_safety() is scoped to the single with_read_txn closure; it doesn't affect any other concurrent reader or future transactions. Correct.

---

Design observations

ReadTimeout is defined in certificate_store.rs — minor placement concern.
The enum is a cross-cutting concern (it describes an MDBX txn property, not a certificate domain concept). It works here because pub use certificate_store::* in stores/mod.rs re-exports it crate-wide. If more methods across other stores need similar control in future, it might be worth moving to a shared types module. Not blocking for this PR.

origins_after_round also does a range scan — not touched.
certificate_store.rs:302-326 scans CertificateDigestByRound but doesn't receive the timeout param. This is intentional (it isn't in the recovery path), but it's worth noting for future callers who might add it to a recovery path.

---

Test coverage

The test at storage_tests.rs:383 correctly uses ReadTimeout::Enforced — the test is not a recovery path and should remain subject to the timeout. No issue there.

There's no test exercising the Exempt path specifically. Testing actual disable_long_read_safety() behavior would require injecting an MDBX timeout and verifying the scan completes — that's difficult with the current test infrastructure and not expected here. A comment in the test file pointing to the incident would help future maintainers understand why the Exempt variant exists.

---

Nits

certificate_store.rs:96 — the doc comment on the trait method uses [ReadTimeout::Exempt] (backtick-bracket syntax). This is fine but slightly inconsistent with some other doc comments in the file.

---

Verdict

Approve. The fix is correct, minimal, and well-documented. The PR description's incident timeline is an excellent addition to the project history — it makes the "why" obvious to anyone who reads this code later.

[github-actions]

Claude finished @procdump's task in 2m 11s —— View job

---

PR Review: fix: exempt consensus recovery reads from the 30s MDBX read-txn timeout

• Gather context and understand the request
• Read changed files and git diff
• Analyze code for correctness, safety, and Rust best practices
• Post review findings

---

Summary

This PR is correct and production-ready. The two commits are well-structured: the first applies the fix (adds ReadTimeout, threads it through after_round, exempts both recovery callers), and the second refactors ReadTimeout to the DB types layer — which directly addresses the placement concern raised in the prior reviews. No blocking issues.

---

What changed since the prior reviews

Second commit (f24cd9b) — ReadTimeout moved to database_traits.rs

This is a clean improvement. ReadTimeout now lives at rayls_infrastructure_types::ReadTimeout alongside the other DB traits (DbTx, DbTxMut, Database), which is the right home for it. The export chain is clean:

• types/src/lib.rs re-exports database_traits::* → exposes ReadTimeout crate-wide
• storage/src/lib.rs re-exports ReadTimeout for consumers (pub use rayls_infrastructure_types::{error::StoreError, ReadTimeout})
• All three call sites import from rayls_infrastructure_storage, as before

This resolves the "defined in certificate_store.rs but conceptually DB-layer" concern from the prior reviews.

---

Confirmed correct

• All callers covered. Exactly three callers of after_round: state.rs:110 (Exempt), state-sync/src/lib.rs:91 (Exempt), storage_tests.rs:383 (Enforced). No missed sites.
• disable_long_read_safety() timing is right. Called inside with_read_txn's closure at certificate_store.rs:248-252, before any cursor operations — the only valid window.
• Exemption propagates through LayeredDatabase. layered_db.rs:253-257 correctly delegates to self.db.disable_long_read_safety(). The regression test at layered_db.rs:1388 proves the call actually reaches the inner MDBX txn and a walk straddling the timeout deadline completes in full.
• LayeredDbTxMut panic unreachable. layered_db.rs:305-306 panics on disable_long_read_safety() for write transactions, but after_round calls with_read_txn, which yields LayeredDbTx (read-only). Safe.
• .expect("database available") in construct_dag_from_cert_store is now safe. With Exempt, the only errors reaching it are genuine DB faults — panic-at-boot is correct there.
• mem_db and redb impls are no-ops. Neither backend enforces a read-txn timeout, so their disable_long_read_safety being a no-op is correct.

---

Outstanding nits from prior reviews (still not fixed)

Nit: trait doc says "potentially large" — the issue is I/O latency, not size

certificate_store.rs:79

/// `timeout` controls whether this (potentially large) scan is subject to the read-transaction
/// timeout; recovery-path callers pass [`ReadTimeout::Exempt`].

The incident was a ~600-round scan timing out under I/O pressure — clearly not large. "Potentially large" risks future maintainers assuming only big scans need exemption. The per-variant doc on Exempt ("Opt out of the timeout for a bounded, latency-insensitive scan") already gets this right; the trait doc is slightly inconsistent with it.

Fix this →

Optional: prime_consensus error path is still warn!

state-sync/src/lib.rs:97-104:

Err(e) => {
    warn!(..., "prime_consensus: after_round failed; cert_store_round left at prior value");
    (None, 0)
}

With Exempt, this branch is only reachable on a genuine DB failure, not a transient timeout. A warn! previously made sense as "harmless timeout degradation"; now it understates the severity. Not blocking, but worth an upgrade to error!.

---

Verdict

Approve. The refactor commit is a good follow-up that puts ReadTimeout in the right architectural layer. The core fix is correct, minimal, and consistent with the existing disable_long_read_safety() pattern across the codebase. The two outstanding nits are cosmetic and don't affect correctness.

[github-actions]

Claude finished @procdump's task in 1m 48s —— View job

---

PR Review: fix: exempt consensus recovery reads from the 30s MDBX read-txn timeout

• Gather context and understand the request
• Read changed files and git diff
• Analyze code for correctness, safety, and Rust best practices
• Post review findings

---

Overall: correct, minimal, and well-motivated. The root-cause analysis in the PR description is thorough, and the fix is precisely scoped to the two callsites that actually need exemption. The subsequent refactor commit (dc6b6a3) moving ReadTimeout to database_traits.rs is the right call — it belongs at the DB-layer, not alongside CertificateStore.

---

What the change does

1. Adds ReadTimeout { Enforced, Exempt } to database_traits.rs (DB types layer), exported crate-wide via pub use database_traits::* and re-exported from the storage crate.
2. Threads it through CertificateStore::after_round (trait + impl).
3. construct_dag_from_cert_store (state.rs:110) and prime_consensus (state-sync/src/lib.rs:91) both pass Exempt; the integration test passes Enforced.
4. disable_long_read_safety() is called immediately after the txn opens, before any iterator or fetch — the only valid window.

---

Findings

Confirmed correct

• disable_long_read_safety() placement is right. It's the first call inside the with_read_txn closure (certificate_store.rs:251), before the skip_to / iter call. The timeout cannot fire between txn open and the disable call.

• Both phases of the scan are covered. The implementation collects digests via skip_to::<CertificateDigestByRound>, then fetches full certs via multi_get_with_tx::<Certificates>. Both happen inside the same with_read_txn closure, so both benefit from the exemption.

• LayeredDbTxMut panic is unreachable. with_read_txn yields a read-only LayeredDbTx (line 253 of layered_db.rs), which correctly delegates to self.db.disable_long_read_safety(). The DbTxMut variant that panics is not involved here.

• All callsites covered. Exactly three callers of after_round exist in the codebase. Both recovery callers use Exempt; the test uses Enforced. origins_after_round (a live, latency-sensitive path in certificate_fetcher.rs) is correctly left unchanged.

• ReadTimeout placement is now correct. Moved to database_traits.rs — a DB-layer concern, not a certificate-domain concern. Re-exported cleanly from the storage crate root.

• .expect("database available") in construct_dag_from_cert_store is safe to keep. With Exempt, only a genuine DB fault reaches that branch, and panicking at boot on corruption is the correct response.

---

Minor: warn! level in prime_consensus error path is now undersized

state-sync/src/lib.rs:97-104:

Err(e) => {
    warn!(
        target: "rayls-consensus-state-sync",
        err = ?e,
        "prime_consensus: after_round failed; cert_store_round left at prior value"
    );
    (None, 0)
}

Before this PR, a 30 s timeout would reach this arm — a transient condition worth a warn. With Exempt, the only way to reach this arm is a genuine DB error (corruption, I/O failure). warn understates the severity; the node is degraded and cert_store_round will be stale. error! is more appropriate and will alert operators who filter by level. Not a blocker, but worth addressing before production load.

Fix this →

---

Nit: trait doc says "potentially large" — the issue is latency, not size

certificate_store.rs:79:

/// `timeout` controls whether this (potentially large) scan is subject to the read-transaction
/// timeout; recovery-path callers pass [`ReadTimeout::Exempt`].

The incident was a ~600-round scan (~76 ms under normal conditions) that timed out due to an I/O stall. "Potentially large" implies size is the motivating factor, which could mislead future readers. The per-variant doc on Exempt already gets this right: "bounded, latency-insensitive scan". Aligning the trait doc would make the two consistent.

Fix this →

---

Summary

The fix is correct and production-ready. One minor log-level concern (warn → error in the prime_consensus fallback, now that timeout no longer reaches it) and one doc nit; neither is a blocker.