Skip to content

Conversation

@joaquim-verges
Copy link
Member

@joaquim-verges joaquim-verges commented Nov 23, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • Completed transactions now automatically expire after a configurable time period (default: 1 day), enabling automatic cleanup of old transaction data.
  • Configuration

    • Added configuration option for transaction expiration duration with a 1-day default setting.
    • Updated cleanup script defaults to operate in dry-run mode for safety.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 23, 2025

Walkthrough

The changes add TTL (Time To Live) configuration for completed transactions throughout the EOA executor system. A new completed_transaction_ttl_seconds field is added to configuration, store structs, and handlers. TTL expiration is applied to transaction data and attempts lists in failure scenarios. Configuration defaults are set to 86400 seconds (1 day), and cleanup script defaults are made more conservative.

Changes

Cohort / File(s) Summary
Store Module Configuration
executors/src/eoa/store/mod.rs, executors/src/eoa/store/atomic.rs, executors/src/eoa/store/borrowed.rs, executors/src/eoa/store/submitted.rs
Added completed_transaction_ttl_seconds field to EoaExecutorStore, CleanSubmittedTransactions, and ProcessBorrowedTransactions structs. Updated constructor parameters to accept TTL value. Integrated TTL expiration logic into transaction failure and confirmation paths.
Handler/Worker Configuration
executors/src/eoa/worker/mod.rs, server/src/queue/manager.rs, server/src/execution_router/mod.rs
Propagated completed_transaction_ttl_seconds through EoaExecutorJobHandler struct and threaded TTL value into EoaExecutorStore::new() calls during initialization.
Server Configuration
server/configuration/server_base.yaml, server/src/config.rs
Added completed_transaction_ttl_seconds: 86400 to queue configuration with default provider function returning 1-day TTL in seconds.
Diagnostic Routes
server/src/http/routes/admin/eoa_diagnostics.rs
Updated EoaExecutorStore::new() call sites to pass TTL parameter from configuration or handler state.
Cleanup Scripts
scripts/redis-cleanup/index.tsx, scripts/simple-redis-cleanup.ts
Changed default dryRun from false to true. Updated shouldClean safety window from 1 minute to 1 month. Replaced per-transaction dry-run logs with aggregated batch-level summaries.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Store module changes: While spread across 4 files, these follow a consistent pattern of adding the same field and threading it through constructors. Review each file's TTL expiration logic in the Fail/confirmation branches for correctness.
  • Cleanup script changes: The redis-cleanup/index.tsx script has behavioral changes beyond configuration (log aggregation, extended safety window from 1 minute to 1 month). Verify that the 1-month window is intentional and that aggregated logging doesn't hide important transaction details.
  • Configuration propagation: Verify all call sites of EoaExecutorStore::new() are updated with the TTL parameter (particularly in diagnostic routes).

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the primary change: adding TTL expiration for completed transactions in the EOA executor, which is fully reflected in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch Add_TTL_expiration_for_completed_transactions_in_EOA_executor

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@joaquim-verges joaquim-verges marked this pull request as ready for review November 23, 2025 23:41
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
scripts/simple-redis-cleanup.ts (1)

3-3: Use Bun.redis instead of ioredis.

Based on coding guidelines and learnings, scripts under scripts/**/*.{ts,tsx,js,jsx} should use Bun.redis for Redis connections rather than the ioredis library.

Apply this pattern to migrate to Bun's native Redis client:

-import Redis from "ioredis";
+// Bun.redis provides native Redis support

Then update the constructor and usage accordingly:

constructor() {
  this.redis = Bun.redis(CONFIG.redisUrl);
}

Note: Verify the Bun.redis API for any differences in method signatures (scan, pipeline, etc.) compared to ioredis.

scripts/redis-cleanup/index.tsx (1)

3-3: Migrate from ioredis to Bun.redis (requires substantial refactoring).

File matches the scripts/**/*.tsx pattern and must use Bun.redis per coding guidelines. However, this migration is not a simple import swap—Bun.redis does not provide an explicit pipeline() API, requiring refactoring of batch operations at lines 111, 272, and 315.

Required changes:

  1. Line 3: Replace import Redis from "ioredis" with import { redis } from "bun"
  2. Line 59: Replace this.redis = new Redis(CONFIG.redisUrl) with this.redis = redis(CONFIG.redisUrl)
  3. Lines 111–120 (hmget batch): Replace explicit pipeline with Promise.all() for concurrent hmget() calls
  4. Lines 272–283 (del batch): Replace explicit pipeline with Promise.all() for concurrent del() calls
  5. Lines 315–354 (batch delete): Replace explicit pipeline with Promise.all() for concurrent del() calls across all transactions

Bun.redis automatically pipelines concurrent commands, so restructuring to use Promise.all() achieves the same batching behavior without explicit pipeline management.

🧹 Nitpick comments (2)
executors/src/eoa/store/mod.rs (1)

100-104: TTL field added to EoaExecutorStore is consistent with design

Adding pub completed_transaction_ttl_seconds: u64 on EoaExecutorStore gives downstream modules a single source for TTL configuration. This aligns with how other store metadata is exposed.

If this field is only ever used internally by the store modules, you could drop pub and/or add a short doc comment clarifying that the unit is seconds to prevent future misuse.

server/src/http/routes/admin/eoa_diagnostics.rs (1)

126-132: Diagnostics now honor the same TTL configuration as the worker

Each admin handler now builds EoaExecutorStore with eoa_queue.handler.completed_transaction_ttl_seconds, so diagnostics and manual reset logic see a store configured identically to the live executor. This is the right place to source TTL from and keeps behavior consistent across read and write paths.

Given the repeated pattern of:

  • grabbing redis_conn, namespace, and completed_transaction_ttl_seconds from eoa_queue.handler, and
  • constructing EoaExecutorStore::new(...)

consider extracting a small helper (e.g., fn make_eoa_store(state: &EngineServerState, eoa: Address, chain_id: u64) -> EoaExecutorStore) to reduce duplication and keep future TTL/config changes in one place.

Also applies to: 215-217, 269-272, 333-336, 379-382, 422-425

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5c473ef and 7c6c7e2.

📒 Files selected for processing (12)
  • executors/src/eoa/store/atomic.rs (4 hunks)
  • executors/src/eoa/store/borrowed.rs (2 hunks)
  • executors/src/eoa/store/mod.rs (3 hunks)
  • executors/src/eoa/store/submitted.rs (2 hunks)
  • executors/src/eoa/worker/mod.rs (2 hunks)
  • scripts/redis-cleanup/index.tsx (3 hunks)
  • scripts/simple-redis-cleanup.ts (1 hunks)
  • server/configuration/server_base.yaml (1 hunks)
  • server/src/config.rs (2 hunks)
  • server/src/execution_router/mod.rs (1 hunks)
  • server/src/http/routes/admin/eoa_diagnostics.rs (6 hunks)
  • server/src/queue/manager.rs (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
scripts/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (scripts/.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)

scripts/**/*.{ts,tsx,js,jsx}: Use bun <file> instead of node <file> or ts-node <file> to run JS/TS files
Do not use dotenv; Bun automatically loads .env files
Use Bun.serve() for HTTP/WebSocket servers; do not use express
Use bun:sqlite for SQLite; do not use better-sqlite3
Use Bun.redis for Redis; do not use ioredis
Use Bun.sql for Postgres; do not use pg or postgres.js
Use built-in WebSocket; do not use ws
Prefer Bun.file over node:fs readFile/writeFile for file IO
Use Bun.$ (shell) instead of execa

Files:

  • scripts/redis-cleanup/index.tsx
  • scripts/simple-redis-cleanup.ts
scripts/**/*.{html,ts,tsx,css}

📄 CodeRabbit inference engine (scripts/.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)

Use bun build <file.html|file.ts|file.css> instead of webpack or esbuild

Files:

  • scripts/redis-cleanup/index.tsx
  • scripts/simple-redis-cleanup.ts
🧠 Learnings (5)
📚 Learning: 2025-09-20T06:58:40.230Z
Learnt from: d4mr
Repo: thirdweb-dev/engine-core PR: 48
File: executors/src/eoa/store/submitted.rs:229-230
Timestamp: 2025-09-20T06:58:40.230Z
Learning: The diff in executors/src/eoa/store/submitted.rs shows correct brace structure - the first closing brace closes the remove_transaction_from_redis_submitted_zset method and the second closing brace closes the impl CleanSubmittedTransactions block. The change only adds whitespace formatting.

Applied to files:

  • executors/src/eoa/store/borrowed.rs
  • executors/src/eoa/store/submitted.rs
  • server/src/execution_router/mod.rs
  • executors/src/eoa/worker/mod.rs
  • executors/src/eoa/store/atomic.rs
  • executors/src/eoa/store/mod.rs
  • server/src/http/routes/admin/eoa_diagnostics.rs
  • server/src/queue/manager.rs
📚 Learning: 2025-09-20T05:30:35.171Z
Learnt from: joaquim-verges
Repo: thirdweb-dev/engine-core PR: 48
File: executors/src/eoa/worker/send.rs:20-21
Timestamp: 2025-09-20T05:30:35.171Z
Learning: In executors/src/eoa/worker/send.rs, there is a critical bug where HEALTH_CHECK_INTERVAL is defined as 300 seconds but compared against millisecond timestamps, causing balance checks to occur every 300ms instead of every 5 minutes (1000x more frequent than intended).

Applied to files:

  • executors/src/eoa/worker/mod.rs
  • server/src/http/routes/admin/eoa_diagnostics.rs
  • server/src/queue/manager.rs
📚 Learning: 2025-09-20T07:40:58.898Z
Learnt from: CR
Repo: thirdweb-dev/engine-core PR: 0
File: scripts/.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-09-20T07:40:58.898Z
Learning: Applies to scripts/**/*.{ts,tsx,js,jsx} : Use `Bun.redis` for Redis; do not use `ioredis`

Applied to files:

  • scripts/redis-cleanup/index.tsx
  • scripts/simple-redis-cleanup.ts
📚 Learning: 2025-07-06T15:44:13.701Z
Learnt from: d4mr
Repo: thirdweb-dev/engine-core PR: 5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: The EOA executor store uses comprehensive WATCH-based coordination where every Redis state mutation watches the lock key and validates ownership before proceeding. If the lock is lost during any operation, the transaction fails with LockLost error. This makes aggressive lock acquisition safe because only the actual lock holder can successfully perform state mutations, regardless of who claims the lock.

Applied to files:

  • executors/src/eoa/store/mod.rs
  • server/src/http/routes/admin/eoa_diagnostics.rs
📚 Learning: 2025-07-06T15:44:13.701Z
Learnt from: d4mr
Repo: thirdweb-dev/engine-core PR: 5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.

Applied to files:

  • executors/src/eoa/store/mod.rs
  • server/src/http/routes/admin/eoa_diagnostics.rs
🧬 Code graph analysis (1)
server/src/http/routes/admin/eoa_diagnostics.rs (2)
executors/src/eoa/store/mod.rs (2)
  • new (113-119)
  • new (297-313)
executors/src/eoa/store/atomic.rs (1)
  • chain_id (89-91)
🔇 Additional comments (20)
scripts/simple-redis-cleanup.ts (1)

13-13: Good safety improvement.

Changing the default to dryRun: true is a conservative and safe choice that prevents accidental data deletion. Users must now explicitly opt-in to actual deletion operations.

server/configuration/server_base.yaml (1)

27-43: TTL config default looks correct

completed_transaction_ttl_seconds: 86400 is a sensible 1‑day default and consistent with the field name and comment. No issues from the config side.

server/src/execution_router/mod.rs (1)

451-459: EOA store TTL wiring is consistent with worker configuration

Passing self.eoa_executor_queue.handler.completed_transaction_ttl_seconds into EoaExecutorStore::new ensures the router uses the same TTL as the EOA worker, keeping behavior consistent for completed transactions.

executors/src/eoa/worker/mod.rs (2)

162-168: Scoped store now correctly receives TTL

Passing self.completed_transaction_ttl_seconds into EoaExecutorStore::new ensures the worker’s scoped store uses the configured TTL for completed transactions. This keeps TTL behavior centralized on the handler.


129-132: All handler construction sites properly initialize completed_transaction_ttl_seconds from queue config; no action needed.

The verification confirms that the single handler construction site in server/src/queue/manager.rs:264 correctly sets completed_transaction_ttl_seconds: queue_config.completed_transaction_ttl_seconds, which is populated from configuration with a default of 86400 seconds (1 day). The field is never uninitialized or defaulted to zero.

executors/src/eoa/store/mod.rs (1)

297-303: Constructor update cleanly threads in completed_transaction_ttl_seconds

Extending EoaExecutorStore::new to take completed_transaction_ttl_seconds and store it on the struct is straightforward and keeps existing call‑sites explicit about TTL configuration.

Also applies to: 311-312

server/src/config.rs (2)

54-55: LGTM!

The new TTL configuration field is properly structured with a serde default and follows the existing configuration patterns in the file.


76-78: LGTM!

The default TTL value of 86400 seconds (1 day) is reasonable for completed transaction cleanup, providing a balance between data retention and storage efficiency.

executors/src/eoa/store/borrowed.rs (2)

44-44: LGTM!

The field addition properly threads the TTL configuration through the borrowed transaction processing flow.


230-236: LGTM!

The TTL expiration is correctly applied to failed transactions. The Success path appropriately omits TTL here since those transactions transition to submitted state and are handled by the cleanup logic in submitted.rs. The Nack path correctly omits TTL as transactions return to pending state.

executors/src/eoa/store/submitted.rs (2)

203-203: LGTM!

The field addition properly enables TTL configuration for submitted transaction cleanup operations.


364-367: LGTM!

The TTL expiration is correctly applied to confirmed transactions, ensuring automatic cleanup after the configured duration. The logic appropriately applies TTL only to completed transactions (SCENARIO 1), while replaced transactions (SCENARIO 2) correctly omit TTL as they transition back to pending state.

server/src/queue/manager.rs (1)

264-264: LGTM!

The TTL configuration is correctly propagated from QueueConfig to EoaExecutorJobHandler, completing the configuration chain from config files to the executor system.

executors/src/eoa/store/atomic.rs (4)

592-598: LGTM!

The TTL expiration is correctly applied to failed pending transactions, ensuring both the transaction data and attempts list are automatically cleaned up after the configured duration.


668-675: LGTM!

The batch failure operation correctly applies the same TTL logic as the single-transaction case, ensuring consistency across both code paths.


734-734: LGTM!

The TTL is correctly threaded from the store into the CleanSubmittedTransactions operation.


752-752: LGTM!

The TTL is correctly threaded from the store into the ProcessBorrowedTransactions operation, maintaining consistency with the CleanSubmittedTransactions pattern.

scripts/redis-cleanup/index.tsx (3)

15-17: LGTM: More conservative defaults improve safety.

The updated defaults are appropriate:

  • dryRun: true prevents accidental deletions
  • Larger batchSize improves efficiency
  • Higher progressInterval reduces log noise

222-229: Question: Does 1-month minimum align with the 1-day TTL?

The safety window was increased from 1 minute to 1 month, but the PR introduces a 1-day TTL (86400 seconds) for completed transactions. This means:

  • TTL should auto-expire transactions after 1 day
  • This cleanup script won't touch anything under 30 days old
  • Gap: Transactions that survive TTL expiration would accumulate for 29 days before cleanup

Is this 30-day buffer intentional, or should it be closer to the TTL duration (e.g., 2-3 days) to catch TTL failures sooner?


307-309: LGTM: Aggregated logging reduces noise.

Replacing per-transaction dry-run logs with a single summary improves readability and performance while still providing the necessary information.

@joaquim-verges joaquim-verges merged commit 196bf34 into main Nov 24, 2025
4 checks passed
@joaquim-verges joaquim-verges deleted the Add_TTL_expiration_for_completed_transactions_in_EOA_executor branch November 24, 2025 00:22
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.

2 participants