fix: Prevent transactions stranded in non-final Redis status indexes#823
Conversation
Status transitions in the Redis transaction repository wrote the body and its status indexes in two separate round-trips, so a transient failure after the body committed left confirmed transactions stranded in the sent/submitted sorted sets. The final-state guard rejected retries and the cleanup job only scans final-status indexes, so ghosts were never repaired or deleted, permanently inflating pending counts. - Maintain status indexes inside the partial_update Lua script so body and index changes commit atomically; finalization also purges the id from all non-final indexes, healing stale entries from earlier hops - Add reconcile_stale_status_indexes to sweep non-final indexes for entries older than 1h whose body is missing or already final, called per relayer from the cleanup cron under the existing distributed lock so already-deployed ghosts heal on the first run after deploy Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
- Skip status sorted-set writes in partial_update's follow-up index pipeline: the Lua script already moves them atomically with the body, and replaying them from a stale snapshot could race a concurrent transition and re-add a just-removed entry - Run reconciliation before final-status cleanup so repaired expired ghosts are deleted in the same cron run; a reconcile failure is surfaced but no longer blocks deletion 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:
WalkthroughRedis transaction status indexes are now updated atomically during partial updates and can be reconciled for stale entries. Repository abstractions expose reconciliation, and relayer cleanup reports reconciliation failures without stopping deletion processing. ChangesTransaction index consistency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CleanupWorker
participant TransactionRepository
participant RedisTransactionRepository
CleanupWorker->>TransactionRepository: reconcile_stale_status_indexes(relayer_id)
TransactionRepository->>RedisTransactionRepository: delegate reconciliation
RedisTransactionRepository->>RedisTransactionRepository: scan and repair stale status indexes
RedisTransactionRepository-->>TransactionRepository: repair count or error
TransactionRepository-->>CleanupWorker: reconciliation result
CleanupWorker->>TransactionRepository: process transaction cleanup
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (3)
src/jobs/handlers/transaction_cleanup_handler.rs (1)
651-656: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the required import grouping.
Move the external
chronoimport before the localcrate/superimports. As per coding guidelines, imports must be grouped as std, external crates, then local crates.🤖 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/jobs/handlers/transaction_cleanup_handler.rs` around lines 651 - 656, Reorder the imports in the transaction cleanup handler so the external chrono import appears before all local crate or super imports, preserving the required grouping order of standard library, external crates, then local modules.Source: Coding guidelines
src/repositories/transaction/transaction_redis.rs (2)
2064-2064: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip building index metadata when the patch has no status change.
partial_update_index_metadataserializes two 8-entry maps plus a JSON string on everypartial_update, but the Lua script only readsARGV[5]whenpatch["status"]is present. Frequent non-status updates (set_sent_at,update_network_data,priced_at, retry counters) pay this cost for nothing. Gate the computation onupdate.status.is_some()and pass an empty arg otherwise.♻️ Proposed change
- let index_metadata_json = self.partial_update_index_metadata(&tx_id, &update)?; + let index_metadata_json = if update.status.is_some() { + self.partial_update_index_metadata(&tx_id, &update)? + } else { + String::new() + };The Lua path is already guarded by
patch["status"], so an emptyARGV[5]is never decoded when status is absent.🤖 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/repositories/transaction/transaction_redis.rs` at line 2064, In the partial update flow around partial_update_index_metadata, only build the index metadata when update.status.is_some(); otherwise use an empty string for the Lua script argument corresponding to ARGV[5]. Preserve the existing metadata serialization and script behavior for status-changing patches.
498-511: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffRepeated re-fetching of skipped entries within a run.
When a scan window mixes repairable entries with skipped ones (legit in-flight / non-final mismatches), the offset stays put after a repair while the skipped entries — being the lowest scores — remain at the front and get re-fetched via
get_transactions_by_idson each subsequent iteration until they finally push pastSTALE_INDEX_PAGE_SIZE. For relayers with many old in-flight transactions this wastes I/O every cron cycle and can approach theMAX_STALE_INDEX_RECONCILE_ITERATIONS_PER_STATUScap. Behavior is correct; consider tracking already-inspected ids (or advancing offset past skipped members) to bound rescans.🤖 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/repositories/transaction/transaction_redis.rs` around lines 498 - 511, Update the stale-status reconciliation loop around get_transactions_by_ids and the repaired_on_page branch to avoid re-fetching entries already inspected but skipped as legitimate in-flight or non-final mismatches. Track inspected/skipped IDs or otherwise advance the scan position past them while preserving repair behavior, ensuring each skipped entry is bounded to a single inspection per run.
🤖 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 `@src/jobs/handlers/transaction_cleanup_handler.rs`:
- Around line 255-282: Update the cleanup failure handling after
reconcile_stale_status_indexes and the corresponding later failure path to
preserve reconcile_error when status cleanup also fails. Combine the
reconciliation and cleanup errors in the combined-failure result, while
retaining the existing behavior when only one operation fails.
---
Nitpick comments:
In `@src/jobs/handlers/transaction_cleanup_handler.rs`:
- Around line 651-656: Reorder the imports in the transaction cleanup handler so
the external chrono import appears before all local crate or super imports,
preserving the required grouping order of standard library, external crates,
then local modules.
In `@src/repositories/transaction/transaction_redis.rs`:
- Line 2064: In the partial update flow around partial_update_index_metadata,
only build the index metadata when update.status.is_some(); otherwise use an
empty string for the Lua script argument corresponding to ARGV[5]. Preserve the
existing metadata serialization and script behavior for status-changing patches.
- Around line 498-511: Update the stale-status reconciliation loop around
get_transactions_by_ids and the repaired_on_page branch to avoid re-fetching
entries already inspected but skipped as legitimate in-flight or non-final
mismatches. Track inspected/skipped IDs or otherwise advance the scan position
past them while preserving repair behavior, ensuring each skipped entry is
bounded to a single inspection per run.
🪄 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: 83973171-d023-47dd-b1b8-4aed395c79dc
📒 Files selected for processing (3)
src/jobs/handlers/transaction_cleanup_handler.rssrc/repositories/transaction/mod.rssrc/repositories/transaction/transaction_redis.rs
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 #823 +/- ##
==========================================
- Coverage 89.54% 89.13% -0.42%
==========================================
Files 303 307 +4
Lines 128773 130752 +1979
==========================================
+ Hits 115314 116544 +1230
- Misses 13459 14208 +749
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a Redis consistency failure mode where a transaction body can reach a final state while its ID remains stranded in a non-final status index (e.g., submitted), preventing retries and inflating pending counts. It makes status-index transitions atomic with the body update and adds a reconciliation pass to heal already-deployed “ghost” entries during the cleanup cron.
Changes:
- Extend the
partial_updateLua script to atomically move status sorted-set entries alongside the transaction body write, and purge IDs from all non-final indexes on finalization. - Add repository reconciliation (
reconcile_stale_status_indexes) to scan non-final indexes for stale entries and repair/remove them. - Call reconciliation from the transaction cleanup job and surface reconciliation errors without blocking cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/repositories/transaction/transaction_redis.rs | Makes partial status transitions index-atomic via Lua; adds stale index reconciliation and Redis integration tests. |
| src/repositories/transaction/mod.rs | Extends TransactionRepository with a default no-op reconciliation method and wires it through storage + mocks. |
| src/jobs/handlers/transaction_cleanup_handler.rs | Runs reconciliation before final-status cleanup and preserves reconciliation errors in results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
LGTM
Thanks
In the follow up PR we should remove methods: set_confirmed_at and set_sent_at methods given no usage and given that they do not play well with current Lua script.
Extend stale-index reconciliation to sweep final status indexes in a purge-only mode: entries whose transaction body is gone are removed, while present bodies are left untouched and never re-added. This reaps body-less dangles that no other job cleans up, including the rare entry a concurrent delete could strand during reconciliation. Also address review nits: - Skip building partial_update index metadata when the patch has no status change (the Lua script only reads it on status transitions). - Move ALL_TRANSACTION_STATUSES to src/constants/transactions.rs. - Fix import grouping in the cleanup handler test module. Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
Summary
A status transition in the Redis transaction repository wrote the body and its status indexes in two separate round-trips. A transient failure between them (crash, dropped connection) left the body in a final state while the ID stayed in a non-final index like
submitted. The final-state guard rejected any retry and the cleanup job only scans final-status indexes, so these ghost entries were never repaired or deleted and permanently inflated pending transaction counts.Two changes:
partial_updateLua script now moves the status sorted-set entries in the same atomic invocation as the body write. When a transaction reaches a final state, the script also purges its ID from every non-final index, so an entry stranded by an earlier failure heals at finalization. The follow-up index pipeline no longer touches status sets, which removes a stale-snapshot race between concurrent transitions.reconcile_stale_status_indexesrepository method sweeps non-final indexes for entries older than one hour whose body is missing or already final, and repairs them. The cleanup cron calls it per relayer under the existing distributed lock, so ghosts already present in deployed environments heal automatically on the first run after deploy.Testing Process
Redis-gated integration tests (
cargo test --lib repositories::transaction::transaction_redis -- --ignoredagainst a local Redis) cover the atomic index move, the finalization purge of fabricated stale entries, and reconciliation healing ghosts, removing dangling IDs, and skipping young or legitimately in-flight entries. All 76 ignored tests and the 25 cleanup-handler tests pass.Checklist
Summary by CodeRabbit
Bug Fixes
Tests