feat: Fast delayed resubmit for Stellar insufficient-fee and TRY_AGAIN_LATER#822
Merged
Conversation
Insufficient-fee and TRY_AGAIN_LATER rejections now schedule their own delayed resubmission instead of waiting for the status checker. - submit_core schedules a delayed resubmit (5s × attempt) on txInsufficientFee (cap raised 2→6) and the first 3 TRY_AGAIN_LATER rejections; the status-checker ladder remains the fallback rescue - enqueue failures are logged and swallowed — submission outcome unchanged - transaction_submission_queue moved to the high-frequency Redis profile: its 20s enqueue_scheduled sweep quantized the delays and let the status checker double-fire (this feature is the first user of scheduled_on there) - e2e harness: chained WireMock rejection mappings + runner with a 5-scenario timing matrix, channels prod-parity profile (SQS standard/FIFO via LocalStack), and closed-loop soak with residual-based scoring Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR increases Stellar insufficient-fee retry limits and introduces fast-resubmit constants, changes ChangesFast resubmit core logic
Estimated code review effort: 4 (Complex) | ~60 minutes E2E fast-resubmit verification harness
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RPC as StellarRPC
participant SubmitCore as submit_core
participant JobQueue as JobProducer
RPC->>SubmitCore: TRY_AGAIN_LATER / ERROR(insufficient fee)
SubmitCore->>SubmitCore: record retry count
alt within fast retry limit
SubmitCore->>SubmitCore: compute delay = base_delay * retries
SubmitCore->>JobQueue: send_submit_transaction_job(scheduled_on)
JobQueue-->>SubmitCore: enqueue result (errors logged/swallowed)
else fast window closed
SubmitCore->>SubmitCore: rely on status checker fallback
end
SubmitCore-->>RPC: Ok(updated_tx, status=Sent)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## main #822 +/- ##
==========================================
+ Coverage 89.54% 89.58% +0.03%
==========================================
Files 303 303
Lines 128773 129234 +461
==========================================
+ Hits 115314 115770 +456
- Misses 13459 13464 +5
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/transaction/stellar/submit.rs (1)
257-280: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRecheck the insufficient-fee cap after the atomic retry update.
The cap check uses
tx.metadatabeforerecord_stellar_insufficient_fee_retry. If the input transaction is stale or duplicate submit jobs race, the repository can return a post-increment retry count above the cap and this branch still schedules another fast resubmit.Proposed fix
let retries = updated_tx .metadata .as_ref() .map_or(0, |metadata| metadata.insufficient_fee_retries); + + if retries > STELLAR_INSUFFICIENT_FEE_MAX_RETRIES { + STELLAR_SUBMISSION_FAILURES + .with_label_values(&["error", "tx_insufficient_fee"]) + .inc(); + return Err(TransactionError::UnexpectedError(format!( + "Transaction submission error: insufficient fee retry limit exceeded ({STELLAR_INSUFFICIENT_FEE_MAX_RETRIES})" + ))); + } + let delay_seconds = STELLAR_FAST_RESUBMIT_BASE_DELAY_SECONDS * i64::from(retries);🤖 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 `@src/domain/transaction/stellar/submit.rs` around lines 257 - 280, The insufficient-fee cap in submit.rs is checked only before calling record_stellar_insufficient_fee_retry, so a stale tx or racing submit can still reschedule after the repository increments retries past the limit. In the submit flow around record_stellar_insufficient_fee_retry and the subsequent retries/delay_seconds calculation, re-read the returned metadata.insufficient_fee_retries and enforce STELLAR_INSUFFICIENT_FEE_MAX_RETRIES again before scheduling the fast resubmit; if the updated count is over the cap, return the same TransactionError instead of continuing.
🧹 Nitpick comments (5)
src/domain/transaction/stellar/submit.rs (1)
1473-1478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the delay-escalation assertion to the exact multiplier.
The current
10..=20second window would pass for the wrong retry multiplier. Assert aroundSTELLAR_FAST_RESUBMIT_BASE_DELAY_SECONDS * 3so this test catches regressions in the escalation formula.Proposed test tightening
+ let expected_delay = STELLAR_FAST_RESUBMIT_BASE_DELAY_SECONDS * 3; mocks .job_producer .expect_produce_submit_transaction_job() - .withf(|_, scheduled_on| { - let now = Utc::now().timestamp(); - scheduled_on.is_some() - && scheduled_on.unwrap() >= now + 10 - && scheduled_on.unwrap() <= now + 20 + .withf(move |_, scheduled_on| { + let Some(scheduled_on) = scheduled_on else { + return false; + }; + let now = Utc::now().timestamp(); + *scheduled_on >= now + expected_delay - 1 + && *scheduled_on <= now + expected_delay + 1 })🤖 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 `@src/domain/transaction/stellar/submit.rs` around lines 1473 - 1478, The delay-escalation check in the stellar submit test is too broad and can miss retry multiplier regressions. Update the assertion inside the `withf` predicate to verify `scheduled_on` is centered around `STELLAR_FAST_RESUBMIT_BASE_DELAY_SECONDS * 3` instead of accepting any value in the 10..=20 second range, so the test specifically validates the escalation formula.testing/wiremock/README.md (1)
93-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame personal-looking default path appears throughout the doc.
/tmp/claude-501/codex-parity.lm3FQkand/tmp/claude-501/codex-soak4.SaB8rKlook like copy-pasted session-specific temp paths rather than illustrative examples. Since users will likely copy these commands verbatim, consider a self-explanatory placeholder (e.g./tmp/fast-resubmit-artifacts) consistent with a generalizedDEFAULT_ARTIFACT_DIRin the script.Also applies to: 110-121, 134-141, 175-175
🤖 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 `@testing/wiremock/README.md` around lines 93 - 94, The README is showing session-specific temp directories in the e2e-fast-resubmit examples, which makes the commands look personal and copy-pasted. Update the documented artifact paths to use a generic placeholder such as /tmp/fast-resubmit-artifacts, and make the examples consistent with the script’s DEFAULT_ARTIFACT_DIR and the e2e-fast-resubmit.sh usage so users can copy them safely.testing/wiremock/scripts/e2e-fast-resubmit.sh (3)
876-897: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated scenario→expected-gaps/calls tables risk silent drift.
scenario_expected_gaps()/scenario_expected_calls()and the inline jqexpected($scenario)def insidewrite_gap_table_for_file()encode the same per-scenario timing table twice. If one is updated (e.g. a new scenario or a changed delay) without updating the other, the report table and the pass/fail evaluation will silently diverge.♻️ Proposed fix — pass the expected gaps through as data instead of duplicating the table
-write_gap_table_for_file() { - local file="$1" - local label="$2" - ... - jq -r ' - def expected($scenario): - { - "if-once": [5], - "if-escalation": [5,10,15], - "if-overcap": [5,10,15,20,25,30], - "tal-window": [5,10,15], - "tal-window-closes": [5,10,15,10] - }[$scenario]; - expected(.scenario) as $expected +write_gap_table_for_file() { + local file="$1" + local label="$2" + ... + jq -r --argjson expected_map "$(jq -nc ' + reduce ("if-once","if-escalation","if-overcap","tal-window","tal-window-closes")[] as $s + ({}; . + {($s): ($s | `@base64`)}))' 2>/dev/null || true)" ' + def expected($scenario): + ($scenario) as $s + | ["if-once","if-escalation","if-overcap","tal-window","tal-window-closes"] as $keys + | {} # placeholder — simplest fix: define scenario_expected_gaps once in bash and export as JSON, then reuse here via --argjsonSimplest concrete fix: build the map once in bash (e.g.
scenario_gaps_json="$(jq -nc '{"if-once":[5], ...}')") and pass it into bothevaluate_passandwrite_gap_table_for_filevia--argjson, removing one of the two hardcoded tables.Also applies to: 1824-1863
🤖 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 `@testing/wiremock/scripts/e2e-fast-resubmit.sh` around lines 876 - 897, The scenario timing table is duplicated between scenario_expected_gaps()/scenario_expected_calls() and the inline jq expected($scenario) logic in write_gap_table_for_file(), so update the script to define the mapping once and reuse it in both places. Refactor the bash/JQ flow so a single data source (for example a shared JSON map passed via --argjson) feeds both evaluate_pass and write_gap_table_for_file, and remove the hardcoded per-scenario table from one side to keep scenario values in sync.
793-817: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
soak_scenarios_idle()check-then-arm is not atomic.Between the idle check and
arm_scenariocall insoak_injector_loop(), a concurrent worker could consume the scenario, causing a race. This is inherent to WireMock's scenario state model and only affects the soak injector's best-effort rejection-rate targeting in this test harness, not correctness-critical production code.🤖 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 `@testing/wiremock/scripts/e2e-fast-resubmit.sh` around lines 793 - 817, The idle check in soak_scenarios_idle() is race-prone because it is evaluated separately from the arm_scenario() call used by soak_injector_loop(), so update the soak injector flow to avoid relying on a check-then-arm sequence for WireMock scenarios. Use the existing scenario_state() and arm_scenario() helpers to make the decision and state transition in one best-effort path, or otherwise recheck immediately before arming, so concurrent workers cannot consume the scenario between steps.
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGeneralize the default artifact directory instead of a personal-looking session path.
/tmp/claude-501/codex-parity.lm3FQkreads like a leftover from a specific dev/AI session rather than a generic default. This also propagates into the README examples (e.g. lines 94, 113-114, 120-121, 126, 141, 175). Consider a self-explanatory default such as/tmp/oz-relayer-fast-resubmit-<pid>or amktemp -dbased path.♻️ Proposed fix
-DEFAULT_ARTIFACT_DIR="/tmp/claude-501/codex-parity.lm3FQk" +DEFAULT_ARTIFACT_DIR="/tmp/oz-relayer-fast-resubmit-$$"🤖 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 `@testing/wiremock/scripts/e2e-fast-resubmit.sh` around lines 7 - 9, The default artifact directory in e2e-fast-resubmit.sh is hardcoded to a personal-looking session path; replace it with a generic, self-explanatory default in the DEFAULT_ARTIFACT_DIR/ARTIFACT_DIR setup (for example a process-specific or mktemp-based directory) so the generated defaults and related README references no longer expose a dev-specific path. Update the default handling around VERDICTS_FILE to continue deriving from the new generic artifact directory, and keep the naming consistent wherever the default path is documented or echoed.
🤖 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 `@testing/wiremock/e2e/fast-resubmit-config.json`:
- Around line 17-29: The fast-resubmit config references a local signer keystore
that is not guaranteed to exist in matrix/default runs. Update the setup around
fast resubmit so the `local-signer.keystore` used by `fast-resubmit-config.json`
is created before those runs, or change the `signers` entry to point at a
keystore path generated by `testing/wiremock/scripts/e2e-fast-resubmit.sh`
outside soak-only mode. Make the fix in the config and/or script paths that
drive `local-signer` so the relayer always has a valid keystore available.
---
Outside diff comments:
In `@src/domain/transaction/stellar/submit.rs`:
- Around line 257-280: The insufficient-fee cap in submit.rs is checked only
before calling record_stellar_insufficient_fee_retry, so a stale tx or racing
submit can still reschedule after the repository increments retries past the
limit. In the submit flow around record_stellar_insufficient_fee_retry and the
subsequent retries/delay_seconds calculation, re-read the returned
metadata.insufficient_fee_retries and enforce
STELLAR_INSUFFICIENT_FEE_MAX_RETRIES again before scheduling the fast resubmit;
if the updated count is over the cap, return the same TransactionError instead
of continuing.
---
Nitpick comments:
In `@src/domain/transaction/stellar/submit.rs`:
- Around line 1473-1478: The delay-escalation check in the stellar submit test
is too broad and can miss retry multiplier regressions. Update the assertion
inside the `withf` predicate to verify `scheduled_on` is centered around
`STELLAR_FAST_RESUBMIT_BASE_DELAY_SECONDS * 3` instead of accepting any value in
the 10..=20 second range, so the test specifically validates the escalation
formula.
In `@testing/wiremock/README.md`:
- Around line 93-94: The README is showing session-specific temp directories in
the e2e-fast-resubmit examples, which makes the commands look personal and
copy-pasted. Update the documented artifact paths to use a generic placeholder
such as /tmp/fast-resubmit-artifacts, and make the examples consistent with the
script’s DEFAULT_ARTIFACT_DIR and the e2e-fast-resubmit.sh usage so users can
copy them safely.
In `@testing/wiremock/scripts/e2e-fast-resubmit.sh`:
- Around line 876-897: The scenario timing table is duplicated between
scenario_expected_gaps()/scenario_expected_calls() and the inline jq
expected($scenario) logic in write_gap_table_for_file(), so update the script to
define the mapping once and reuse it in both places. Refactor the bash/JQ flow
so a single data source (for example a shared JSON map passed via --argjson)
feeds both evaluate_pass and write_gap_table_for_file, and remove the hardcoded
per-scenario table from one side to keep scenario values in sync.
- Around line 793-817: The idle check in soak_scenarios_idle() is race-prone
because it is evaluated separately from the arm_scenario() call used by
soak_injector_loop(), so update the soak injector flow to avoid relying on a
check-then-arm sequence for WireMock scenarios. Use the existing
scenario_state() and arm_scenario() helpers to make the decision and state
transition in one best-effort path, or otherwise recheck immediately before
arming, so concurrent workers cannot consume the scenario between steps.
- Around line 7-9: The default artifact directory in e2e-fast-resubmit.sh is
hardcoded to a personal-looking session path; replace it with a generic,
self-explanatory default in the DEFAULT_ARTIFACT_DIR/ARTIFACT_DIR setup (for
example a process-specific or mktemp-based directory) so the generated defaults
and related README references no longer expose a dev-specific path. Update the
default handling around VERDICTS_FILE to continue deriving from the new generic
artifact directory, and keep the naming consistent wherever the default path is
documented or echoed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63cfebf5-f7f7-4e1f-a174-f0cc3ba9c19d
📒 Files selected for processing (16)
src/constants/stellar_transaction.rssrc/domain/transaction/stellar/submit.rssrc/queues/redis/queue.rstesting/wiremock/.gitignoretesting/wiremock/README.mdtesting/wiremock/e2e/fast-resubmit-config.jsontesting/wiremock/mappings/stellar/stellar-insufficient-fee-armed-2.jsontesting/wiremock/mappings/stellar/stellar-insufficient-fee-armed-3.jsontesting/wiremock/mappings/stellar/stellar-insufficient-fee-armed-4.jsontesting/wiremock/mappings/stellar/stellar-insufficient-fee-armed-5.jsontesting/wiremock/mappings/stellar/stellar-insufficient-fee-armed-6.jsontesting/wiremock/mappings/stellar/stellar-insufficient-fee-armed-7.jsontesting/wiremock/mappings/stellar/stellar-try-again-later-armed-2.jsontesting/wiremock/mappings/stellar/stellar-try-again-later-armed-3.jsontesting/wiremock/mappings/stellar/stellar-try-again-later-armed-4.jsontesting/wiremock/scripts/e2e-fast-resubmit.sh
Keep the repeatable regression core: the 5-scenario timing matrix with measured-gap assertions. Drop the one-off prod-parity and soak tooling used during development (results documented on the PR). Replace the 9 near-duplicate chained mapping files with dynamic stub registration against the WireMock admin API at arm time. The chains decrement into the existing one-shot armed state, so any rejection count works without a hardcoded ceiling. Runner: 2146 -> 565 lines. Matrix re-verified 5/5 after the slim. Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
- Recheck the insufficient-fee cap on the post-increment count before scheduling a fast resubmit. Racing submit jobs for the same transaction could both pass the pre-check; the recheck closes that window and fails a doomed transaction one cycle earlier. New unit test covers it. - Tighten the delay-escalation test to assert the exact 5s-per-attempt formula instead of a 10-20s window. - Bootstrap the e2e harness keystore when missing (generate and fund via friendbot), so the harness runs from a fresh clone. Verified by moving the local keystore aside and rerunning the matrix. Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
When two submit jobs race, the loser's post-record count exceeds the cap. Failing there would clobber the winner's already-scheduled retry, creating a failure mode the passive path does not have. Skip the enqueue instead: the cap stays airtight and the next rejection's pre-record check terminates the transaction normally. Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
- If a transaction reached a final state between the RPC rejection and the atomic retry record (racing job finalized it), the repo returns it unchanged. Both rejection arms now skip scheduling in that case instead of enqueueing a pointless resubmit. Unit tests for both arms. - The e2e runner now fails fast (exit 64) on an invalid scenario name instead of booting the stack and exiting green with zero scenarios; scenario validation happens before any infrastructure starts. Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a burst of transactions is submitted below the current surge fee, Stellar rejects them with
txInsufficientFeeorTRY_AGAIN_LATER. These rejections are transient. Congestion usually clears within one ledger, about 5 seconds, and the same envelope usually succeeds if it is simply submitted again. But nothing resubmits it quickly today. The transaction sits inSentuntil the status checker notices it, which takes 25 to 45 seconds or more. Under sustained load this lag builds a backlog of stuck transactions, and clients start timing out.This PR makes recovery active. When a submission is rejected for one of these two reasons, the relayer schedules its own retry:
TRY_AGAIN_LATERgets fast retries only for its first 3 rejections. After that the status checker takes over, same as today. Without this window, a transaction the network never accepts could retry fast for its entire 15-minute lifetime.Two areas deserve extra review attention:
sent_at, so the checker stays quiet while fast retries run. That only holds if the delayed jobs arrive on time. The Redis submission queue only swept scheduled jobs every 20 seconds, since nothing had ever scheduled a job on it before. That delay was enough for the checker to fire a duplicate, so this PR moves the queue to the high-frequency polling profile. SQS is unaffected because it supports delayed delivery natively.Follow-ups being filed separately: a resubmission that errors (for example
TxBadSeq) never refreshessent_at, so the checker retries it every cycle until it expires. And a hard-failed transaction leaves a sequence gap that can break later transactions from the same account.Testing Process
sendTransactiona chosen number of times, then lets traffic pass through to testnet. It asserts the measured time between submissions from the proxy's request journal. All 5 scenarios pass. This harness is what caught the queue bug above — unit test mocks have no delivery latency.Measured retry gaps per scenario
Checklist