feat(requester): auto-heal stuck nonce markers via reconciliation loop - #985
feat(requester): auto-heal stuck nonce markers via reconciliation loop#985vishalchangrani wants to merge 11 commits into
Conversation
…once markers
Adds a background reconciliation loop to TxMemPool that polls the most recent
wrapping Cadence tx for each active EOA and clears the in-flight nonce marker
when the wrapper cannot advance the on-chain nonce. Bounds wedge duration to
one sealing window (~6-8s) instead of the full idleQueueRetention (~1 minute).
Motivation: the DFNS silent-drop incident (2026-07-29T03:09:11Z, EOA
0xdEA9...58B3, nonce 25882) showed the wrapping Cadence tx sealing with
evm_error=nonce too high after intra-block Collection Node reordering. In
that case the mempool has already advanced `lastConsecutivelySubmitted` on
successful send, so subsequent retries got ErrInFlightNonce until the
idle-eviction retention expired — that is the wedge this loop closes.
Implementation:
* eoaQueue gains lastFlowTxID, set on every successful submission (fast
path in Add, and reconcileSubmission after a background submitWork).
* reconcileSubmission now takes the flowTxID so it can persist it on the
queue alongside markSubmitted (existing behavior otherwise unchanged).
* nonceTracker.resetSubmissionState clears both submission markers so the
next Add re-classifies against on-chain state.
* reconcileLoop ticks at TxReconcileInterval, snapshots per-EOA
(flowTxID, lastSubmittedAt) under the lock, calls GetTransactionResult
outside the lock, and resets state only on concrete evidence:
SEALED-with-error, or unsealed past TxReconcileStaleAfter. Transient AN
errors do not touch state. A short critical section around the reset
re-checks that the same flowTxID still owns the queue.
* Injectable getTxResult field mirrors the existing submitBatch pattern
so tests can drive reconciliation without a live Access Node.
* New metric TxPoolReconcileReset(reason) with labels
{wrapper-reverted, unsealed-past-threshold}.
* New config knobs TxReconcileInterval (default 1s) and
TxReconcileStaleAfter (default 30s), both validated > 0 when
--tx-mempool-mode=true.
Test file change is minimal: only satisfies the new reconcileSubmission
signature (added flow.Identifier{} argument at four existing call sites)
and defaults getTxResult in newTestPool to a no-op fake. No new tests are
added here — a follow-up subagent covers behavior.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds seven table-tests exercising TxMemPool.reconcileOnce, covering every
branch of the recovery path (behavior-spec case 13):
- wrapper reverted (Sealed + non-nil Error) clears the marker
- wrapper unsealed past TxReconcileStaleAfter clears the marker
- wrapper Sealed with no error leaves the marker in place
- wrapper unsealed and fresh leaves the marker in place
- queue without an outstanding marker is skipped (no getTxResult call)
- fresher submission superseding lastFlowTxID between snapshot and
reset is respected (no wrongful reset)
- end-to-end: post-reconcile, a same-nonce retry is accepted rather
than rejected as in flight
All tests drive reconcileOnce synchronously through a fake clock plus
injected submitBatch/getTxResult, so no goroutines and no wall-clock
sleeps are involved. testPoolConfig() now seeds TxReconcileInterval and
TxReconcileStaleAfter to match the CLI defaults (1s / 30s).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend the tx_mempool.go BEHAVIOR SPEC with a Recovery section describing the reconciliation loop's ticks, reset triggers, concurrency model, and observability signals. Fix a doc-vs-code inconsistency in the reconcileLoop docstring: it listed three reset cases, but only two are implemented. The "chain advanced past highestSent" case is redundant, because a successfully-sealed wrapper is progressed by the next legitimate submission, so no wedge needs clearing. Add the two new CLI flags (tx-reconcile-interval, tx-reconcile-stale-after) to the README Configuration Flags table. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address two blockers surfaced during the code-review pass on the initial implementation (f46029c3): 1. Panic in NewTxMemPool when a programmatic caller (e.g. the e2e test config in tests/tx_mempool_test.go) leaves TxReconcileInterval or TxReconcileStaleAfter at their zero values: time.NewTicker(0) panics. Fix: backfill both fields to their CLI-default values (1s, 30s) inside NewTxMemPool before starting the reconcile loop. 2. Behavioral race in reconcileOnce that could clobber a fresh in-flight submission. After the snapshot-outside-the-lock pattern, if a concurrent background flush called markSubmitting(newHigh) between snapshot and reset, the reconciler saw an unchanged lastFlowTxID and would reset — clearing submitting for the newer batch. That would let a client retry duplicate the in-flight nonce, reintroducing the exact duplicate-wrapper failure this loop is meant to eliminate. Fix: after the lastFlowTxID re-check under the lock, also require q.nonces.inFlight() to be false; a matching lastFlowTxID guarantees the previous batch's markSubmitted has returned, so any submitting we observe now must belong to a strictly newer batch. Also enriches the WARN log with elapsed-since-submit duration and, when known, the wrapper's Cadence error message. Adds Test_TxMemPool_ReconcileSkipsWhenFreshBatchInFlight; verified to FAIL without the fix and PASS with it. Verified: go build/vet clean; unit tests pass under -race; e2e Test_TxMemPool suite now passes cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #984's review consolidated the log field name from flow-tx-id to flow_tx_id (underscores are safer for Grafana filter syntax). The reconciliation loop's WARN log and its associated behavior-spec comment still used the old dashed name after rebase; align them so a downstream grep on 'flow_tx_id' catches both the submission and the reconciler reset log lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe transaction mempool now polls submitted Flow transactions, clears markers for reverted or stale wrappers, preserves newer submissions, and records reset metrics. Polling and stale thresholds are configurable through the CLI and configuration. ChangesTransaction reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TxMemPool
participant FlowAccessAPI
participant EOAQueue
participant MetricsCollector
TxMemPool->>FlowAccessAPI: Poll transaction result for Flow transaction ID
FlowAccessAPI-->>TxMemPool: Return sealed or unsealed transaction result
TxMemPool->>EOAQueue: Revalidate and clear submission markers
TxMemPool->>MetricsCollector: Increment reset counter with reason
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // staleness threshold is exceeded. | ||
| sealed := err == nil && result != nil && result.Status >= flow.TransactionStatusSealed | ||
| reverted := sealed && result.Error != nil | ||
| stale := now.Sub(s.lastSubmittedAt) > t.config.TxReconcileStaleAfter |
There was a problem hiding this comment.
The stale check ignores the poll result. A wrapper that sealed successfully with no follow-up submission within 30s is reset with reason unsealed-past-threshold (verified with a scratch test on this branch). Every idle-after-submit EOA emits a spurious WARN and counter increment, and the reset drops the ErrInFlightNonce guard while the index may still lag the seal. Gate staleness on not-landed, e.g. stale := !(sealed && result.Error == nil) && now.Sub(s.lastSubmittedAt) > t.config.TxReconcileStaleAfter, and you could add a regression test for sealed-success past the threshold.
| // path below. But avoid touching state on transient AN errors — only | ||
| // reset if we have concrete evidence (SEALED-with-error) OR the | ||
| // staleness threshold is exceeded. | ||
| sealed := err == nil && result != nil && result.Status >= flow.TransactionStatusSealed |
There was a problem hiding this comment.
Status >= Sealed also matches Expired (5 > 4). An expired wrapper will never land and should reset immediately, but here it counts as sealed with a nil Error, so it only heals via the stale path and a naive !sealed fix of the stale bug would make it never reset.
There was a problem hiding this comment.
With the reconciliation loop, the same scenario clears in ~7s:
Within 6-8 seconds the transaction will likely have a status of TransactionStatusFinalized and not TransactionStatusSealed, so it's worth mentioning that the reconciliation loop won't be able to recover a wedged transaction within 6-8 seconds. I'm not sure if we want to check against a result.Status == flow.TransactionStatusFinalized to expedite the recovery. But then again, I wonder if GetTransactionResultsByBlockID or GetTransactionResult can fetch finalized blocks/transactions, or if they will be missing from AN, until they become sealed.
There was a problem hiding this comment.
Good catch 👌 Updated in 0b45280, to check specifically for result.Status == flow.TransactionStatusSealed
| Str("flow_tx_id", s.flowTxID.Hex()). | ||
| Str("reason", reason). | ||
| Dur("elapsed-since-submit", elapsed) | ||
| if reverted && result.Error != nil { |
There was a problem hiding this comment.
nit: reverted already implies result.Error != nil (line 1261). if reverted { suffices.
|
|
||
| now := t.now() | ||
| for _, s := range snaps { | ||
| result, err := t.getTxResult(ctx, s.flowTxID) |
There was a problem hiding this comment.
nit: this call has no deadline, so one hung AN call stalls the whole reconcile loop indefinitely and silently disables healing. Wrap each poll in context.WithTimeout (e.g. the tick interval), mirroring the fastPathSubmitTimeout rationale.
There was a problem hiding this comment.
I agree with that. Moreover, to minimize the AN calls, I would instead fetch the transaction results of an entire block, e.g.:
txResults, err = k.client.GetTransactionResultsByBlockID(ctx, blockID)We would likely have to track which block to fetch next, for inspecting the transaction results. And ideally, we need to give sufficient time, so that the block is sealed, not just finalized.
snaps could be turned to a map, and then we just iterate over the txResults:
type snapshot struct {
from gethCommon.Address
lastSubmittedAt time.Time
}
var snaps [flow.Identifier]snapshot
for _, txResult := range txResults {
s, ok := snaps[txResult.TransactionID]
...
}This way we fetch all the transaction results of a single block, which is 1 AN cal, instead of the potentially many GetTransactionResult calls that we would have to do by querying each EOA individually.
There was a problem hiding this comment.
Added a context.WithTimeout as a first step: 5f21e92 .
zhangchiqing
left a comment
zhangchiqing
left a comment
There was a problem hiding this comment.
looks good, other than what Janez's comments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/requester/tx_mempool.go (1)
930-954: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStamp
lastSubmittedAton successful background submissions.
collectPrefixstampslastSubmittedAtbefore the submit work is detached, whilereconcileSubmissiononly updateslastFlowTxIDon success. The reconciliation snapshot uses that queue value, so an in-flight wrapper can appear stale if the network call takes longer thanTxReconcileStaleAfter; stamp the queue again in the successful branch or pass the collection timestamp through as the submission timestamp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/requester/tx_mempool.go` around lines 930 - 954, Update reconcileSubmission so the successful submission branch stamps q.lastSubmittedAt in addition to q.lastFlowTxID, using the current submission time or the collection timestamp propagated into the method. Keep the timestamp unchanged on failed submissions and preserve the existing nonce rollback/marking behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@services/requester/tx_mempool.go`:
- Around line 930-954: Update reconcileSubmission so the successful submission
branch stamps q.lastSubmittedAt in addition to q.lastFlowTxID, using the current
submission time or the collection timestamp propagated into the method. Keep the
timestamp unchanged on failed submissions and preserve the existing nonce
rollback/marking behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e38b236d-6e2b-4a49-ae16-dc2226b4ec1f
📒 Files selected for processing (7)
README.mdcmd/run/cmd.goconfig/config.gometrics/collector.gometrics/nop.goservices/requester/tx_mempool.goservices/requester/tx_mempool_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/requester/tx_mempool.go`:
- Around line 1262-1264: Update the reconciliation logic around
reconcileOnce/getTxResult so flow.TransactionStatusExpired is treated as a
terminal result alongside sealed transactions. Add an explicit expired branch
that immediately clears the submission markers and records the appropriate
expired reset reason, without waiting for TxReconcileStaleAfter; preserve
existing sealed and stale handling for other statuses.
- Line 1264: Update the stale condition in the transaction reconciliation logic
to use the equivalent simplified check !sealed || result.Error != nil,
preserving the existing time comparison and relying on result being non-nil
whenever sealed is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 993cf01f-f955-4135-a70d-bbe201a8b01f
📒 Files selected for processing (2)
services/requester/tx_mempool.goservices/requester/tx_mempool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/requester/tx_mempool_test.go
| sealed := err == nil && result != nil && result.Status == flow.TransactionStatusSealed | ||
| reverted := sealed && result.Error != nil | ||
| stale := !(sealed && result.Error == nil) && now.Sub(s.lastSubmittedAt) > t.config.TxReconcileStaleAfter |
There was a problem hiding this comment.
Handle flow.TransactionStatusExpired as a terminal result.
Line 1262 recognizes only flow.TransactionStatusSealed. An expired wrapper therefore resets only after TxReconcileStaleAfter, although it cannot land later. Add an explicit expired branch, clear the markers immediately, and record the correct reset reason.
This repeats the previous review finding; the equality check fixes misclassification but not delayed recovery.
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'TransactionStatusExpired|TransactionStatusSealed|reconcileOnce|getTxResult' \
--glob '*.go' .🧰 Tools
🪛 GitHub Check: Lint
[failure] 1264-1264:
QF1001: could apply De Morgan's law (staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/requester/tx_mempool.go` around lines 1262 - 1264, Update the
reconciliation logic around reconcileOnce/getTxResult so
flow.TransactionStatusExpired is treated as a terminal result alongside sealed
transactions. Add an explicit expired branch that immediately clears the
submission markers and records the appropriate expired reset reason, without
waiting for TxReconcileStaleAfter; preserve existing sealed and stale handling
for other statuses.
4 participants
Summary
TxMemPoolthat auto-heals stuck nonce markers when a wrapping Cadence tx reverts or silently disappears, instead of waiting the full idle-eviction window (60s).GetTransactionResult(lastFlowTxID)for each active EOA and clearinglastConsecutivelySubmitted/submitting/lastFlowTxIDwhen the wrapper is sealed-with-error or unsealed pastTxReconcileStaleAfter.--tx-reconcile-interval(default1s) and--tx-reconcile-stale-after(default30s).TxPoolReconcileReset{reason}and a WARN log line at every reset for observability.Why now
Real production incident on 2026-07-29 at 03:09:11Z (foundation gateway, EOA
0xdEA93da411CFB1f7d676BF81A686ED36C1d358B3, EVM nonce 25882):assert()inrun.cdcreverted withevm_error=nonce too high(Cadence txb946d5c3ea1a80bc165d26824c7504b28dd84c930f182cd048fef52d5c2ca16d).EVM.TransactionExecutedevent fired for the 25882 EVM tx — the mempool never learned it had failed.lastConsecutivelySubmittedstayed stuck at 25882. Every subsequent retry returnedErrInFlightNonce. The wedge cleared only after ~65 seconds via the 60s idle-eviction of the empty queue.With the reconciliation loop, the same scenario clears in ~7s: the reverted wrapper seals with a non-nil
Error, the next tick observes it, and the marker is reset so the client's very next resubmission is accepted and re-submitted with the correct on-chain nonce.Full analysis is on the issue thread: #983 (comment).
What changed
eoaQueue.lastFlowTxID(flow.Identifier) — the Flow tx ID of the most recent Cadence submission, populated by the fast path andreconcileSubmission, cleared on reset.TxMemPool.getTxResult— injectablefunc(ctx, id) (*flow.TransactionResult, error), defaults toclient.GetTransactionResult, injected in tests to run without a live AN.nonceTracker:resetSubmissionState()clears bothlastConsecutivelySubmittedandsubmitting.reconcileLoopticks onTxReconcileInterval, callsreconcileOnce. Snapshot taken underqueueMux, network call issued outside the lock, reset re-acquires the lock and re-checks theflow_tx_idstill matches and thatq.nonces.inFlight()is false, so neither a superseding ack'd submission nor a freshly-in-flight one is ever clobbered.TxReconcileInterval(default1s),TxReconcileStaleAfter(default30s) inconfig/config.gowith corresponding CLI flags and validators incmd/run/cmd.go.NewTxMemPoolalso defensively backfills these to the CLI defaults when zero, so programmatic constructors (the e2e test config) don't triggertime.NewTicker(0).TxPoolReconcileReset(reason)— Prometheus counter withreasonlabel (wrapper-reverted|unsealed-past-threshold). Nop implementation added.services/requester/tx_mempool.goextended with a new "Recovery" section (case 13); README's Configuration Flags table gains the two new flags.Commits
feat(requester): add mempool reconciliation loop to auto-heal stuck nonce markers— implementation.test(requester): unit tests for mempool reconciliation loop— focused tests covering revert, staleness, healthy sealed, unsealed-fresh, no-marker skip, superseded-flow-tx-id skip, and end-to-end unwedging.docs(requester): reconciliation loop behavior spec and README.fix(requester): reviewer-flagged blockers in reconciliation loop— defensive defaults inNewTxMemPoolfor zero durations (fixes e2etime.NewTicker(0)panic), and aninFlight()guard inreconcileOnceto prevent clobbering a fresh in-flight submission (guarded by a regression test).chore(requester): align reconcile-loop with PR #984 flow_tx_id naming— small alignment after the parent PR's naming decision.Base branch
Targets
mpeter/poc-index-finalized-block-results. PR #984 has been squash-merged into this base, so this PR now sits directly on top with no stacking dependency.Test plan
go build ./...— clean.go vet ./services/requester/...— clean.go test ./services/requester/ -count=1 -race— passes; 8 reconcile-loop unit tests included:Test_TxMemPool_ReconcileClearsMarkerWhenWrapperRevertedTest_TxMemPool_ReconcileClearsMarkerWhenWrapperStaleTest_TxMemPool_ReconcileLeavesMarkerWhenWrapperSealedSuccessfullyTest_TxMemPool_ReconcileLeavesMarkerWhenWrapperUnsealedAndFreshTest_TxMemPool_ReconcileSkipsQueueWithoutMarkerTest_TxMemPool_ReconcileSkipsSupersededFlowTxIDTest_TxMemPool_ReconcileSkipsWhenFreshBatchInFlight(regression guard on the fix commit)Test_TxMemPool_ReconcileClearsAllowsSubsequentAddToBeAcceptedTest_TxMemPoolintests/— passes (~33s).TxPoolReconcileResetcounter stays at zero under normal load; simulate a reverted wrapper and confirm the reset log fires within one tick.Config recommendation
Deploy with the defaults (
--tx-reconcile-interval=1s --tx-reconcile-stale-after=30s). Both defaults are conservative:1spolling is cheap (one ANGetTransactionResultcall per active EOA per second, only for EOAs with an outstanding marker).30sstale-after gives Flow's sealing latency (~6-8s) a very wide margin so the reconciler cannot false-positive on healthy-but-slow-to-seal submissions.After deployment, monitor:
TxPoolReconcileReset{reason}counter (should be near zero; a non-zero rate onwrapper-revertedindicates real intra-block reordering; a non-zero rate onunsealed-past-thresholdindicates AN issues).reconciliation clearing stuck in-flight marker).Once we have a week or two of production data characterising sealing latency and reordering rates, we can tune
TxReconcileStaleAfterdown (e.g. to15s) to shrink the worst-case wedge duration on silent AN drops.Summary by CodeRabbit