Skip to content

fix: make redis sender-manager wallet pool self-healing - #43

Open
arpan-jain wants to merge 2 commits into
mainfrom
fix/sender-manager-wallet-recovery
Open

fix: make redis sender-manager wallet pool self-healing#43
arpan-jain wants to merge 2 commits into
mainfrom
fix/sender-manager-wallet-recovery

Conversation

@arpan-jain

Copy link
Copy Markdown
Contributor

The wallet pool is a Redis list that never re-seeds itself unless it is
completely empty, and markWalletProcessed cleared local bookkeeping
before the LPUSH that returns the wallet. When the shared Redis hit
maxmemory on 2026-07-24 (~12:20 UTC), every return-push failed as an
unhandled rejection and Base permanently lost 8 of 10 executor wallets.
All traffic funnelled through the remaining 2, whose nonce races put
~5% of bundles into the ~10s stuck-replacement cycle, pushing p95
inclusion latency from ~1.8s to ~13.4s.

  • checkout (RPOP) now also stamps a checked-out marker ZSET in a single Lua script, so a held wallet is never invisible to reconciliation
  • release (LPOS-guarded LPUSH + ZREM) is atomic and duplicate-proof, runs BEFORE local bookkeeping is cleared, retries 3x and never throws; persistent failures park the wallet for background retry
  • a 60s maintenance loop retries failed releases and re-stamps markers for held wallets (quarantine can hold one indefinitely), so a marker older than 30min genuinely means its holder died
  • startup reconciliation replaces the old seed-only-when-empty logic: seeds on first boot, restores wallets absent from both queue and fresh markers, prunes markers of unconfigured wallets
  • drop the every-10-min 'utility wallet has insufficient balance' error log; executors are funded externally now (gauge metric stays)

Requires Redis >= 6.0.6 (LPOS).

  The wallet pool is a Redis list that never re-seeds itself unless it is
  completely empty, and markWalletProcessed cleared local bookkeeping
  before the LPUSH that returns the wallet. When the shared Redis hit
  maxmemory on 2026-07-24 (~12:20 UTC), every return-push failed as an
  unhandled rejection and Base permanently lost 8 of 10 executor wallets.
  All traffic funnelled through the remaining 2, whose nonce races put
  ~5% of bundles into the ~10s stuck-replacement cycle, pushing p95
  inclusion latency from ~1.8s to ~13.4s.

  - checkout (RPOP) now also stamps a checked-out marker ZSET in a single
    Lua script, so a held wallet is never invisible to reconciliation
  - release (LPOS-guarded LPUSH + ZREM) is atomic and duplicate-proof,
    runs BEFORE local bookkeeping is cleared, retries 3x and never
    throws; persistent failures park the wallet for background retry
  - a 60s maintenance loop retries failed releases and re-stamps markers
    for held wallets (quarantine can hold one indefinitely), so a marker
    older than 30min genuinely means its holder died
  - startup reconciliation replaces the old seed-only-when-empty logic:
    seeds on first boot, restores wallets absent from both queue and
    fresh markers, prunes markers of unconfigured wallets
  - drop the every-10-min 'utility wallet has insufficient balance'
    error log; executors are funded externally now (gauge metric stays)

  Requires Redis >= 6.0.6 (LPOS).
@arpan-jain
arpan-jain requested a review from SahilVasava July 27, 2026 08:36

@SahilVasava SahilVasava left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Adversarial review (incl. empirical Lua/OOM verification against Valkey 8.1 in Docker — prod runs 8.1.4). The Lua scripts themselves are solid: CHECKOUT is genuinely atomic even under OOM (RPOP is non-denyoom, and a script's first write unlocks subsequent denyoom writes — verified 4/4 with direct ZADD failing on the same connection), and RELEASE fails cleanly with zero partial effects. The design direction is right. But the reconciliation restore path can duplicate a live-held wallet in three realistic scenarios — and duplication means two pods signing with the same key = nonce collisions, strictly worse than the leak this fixes.

Blocking (all at createRedisSenderManager.ts, PR head 939ca32):

  1. CONFIRMED — rolling deploy of this PR duplicates every wallet held by old-code pods (:110-120). Old code checks out via plain RPOP with no marker; during rollout an old pod's held wallet is in neither structure → a booting new pod restores it → another pod pops it while the old pod still bundles with it; the old pod's eventual unguarded lpush adds a second queue copy. Near-certain on base under traffic. Also fires between two new-code pods: the snapshot (:95-97) and restore (:115) aren't atomic and RELEASE_WALLET_LUA re-validates nothing. Needs an atomic restore script that re-checks marker-absent-or-stale inside Lua, plus a two-phase rollout (stamp markers first, enable restore in a second deploy).

  2. CONFIRMED — post-outage recovery mass-restores actively-held wallets (:100-106, :152) — the exact incident scenario this PR targets. Re-stamp uses ZADD, which is denyoom (verified: OOM command not allowed while over maxmemory) — during a ≥30min Redis OOM every held wallet's marker goes stale; a pod booting in the recovery gap restores live-held (including quarantined-with-unconfirmed-nonce) wallets.

  3. CONFIRMED — boot crash-loop under Redis write failure (:115, uncaught; handler.ts:270, uncaught). Under OOM the release script rejects at LPUSH (first write, denyoom — verified). Mid-incident, every pod restart dies at boot: old code degraded to read-only; new code turns "2 wallets left" into "total outage on any restart". Wrap reconciliation in try/catch and retry from the maintenance loop.

Non-blocking:

  1. CONFIRMED — self-healing only runs at boot: a crashed pod's wallets stay lost until some future pod boots >30min later. The stale-marker scan belongs in maintain() (:131-159) once finding 1's atomicity is fixed.
  2. PLAUSIBLE — at-least-once release retry (:216-224) can duplicate a wallet re-checked-out by another pod (LPOS proves "not queued", not "still mine") and its trailing ZREM deletes the new holder's marker. Fix: ownership-conditional release (if ZREM == 1 then LPUSH) — idempotent under retry; keep a separate stale-checked script for reconciliation.
  3. LOW — config-removed wallets are pruned from the marker zset but never LREM'd from the queue; popping one throws "wallet not found" out of getWallet into fire-and-forget callers. Opportunistic LREM in reconciliation.

Verified clean: checkout atomicity under OOM; release zero-partial-effects; LPOS nil-mapping + availability on Valkey 8.1; markWalletProcessed ordering/idempotency incl. un-awaited call sites and the shutdown sweep; quarantine re-stamp coverage (60s tick vs 30min staleness = 30× margin, outage case aside); re-stamp-vs-release race is harmless; first-boot double-seed safe; redisKeys prefix no-collision. tsc --noEmit passes at head.

Tests: nothing covers this code (e2e compose has no Redis, exercises the memory sender manager only). Given three of the findings are races this PR class exists to prevent, please add one ioredis integration test against a throwaway Redis: checkout/release round-trip, boot reconciliation fresh-vs-stale markers, release-when-already-queued no-op.

One ops note: if any BetterStack alert keys on the removed "utility wallet has insufficient balance" error log (validateAndRefill.ts) rather than the gauge, update it before merge.

  - release is ownership-conditional (ZREM gates LPUSH): at-least-once
    retries after a client-side timeout can no longer double-queue a
    wallet another pod has since checked out
  - restores moved off the boot path into the 60s maintenance loop,
    behind a 10-min continuously-missing grace window, via an atomic
    Lua script that re-validates queue/marker state; boot now only
    prunes unconfigured wallets (queue + markers) and seeds first boot
  - restore scan gated on a write-health probe: while Redis writes fail
    (e.g. OOM) holders cannot re-stamp markers, so staleness proves
    nothing; the missing-clock resets and holders get a full grace
    period after recovery
  - boot reconciliation never fails startup; re-stamps skip wallets
    pending release retry
  - add ioredis integration tests (vitest in src workspace, gated on
    REDIS_URL): checkout/release round-trip, ownership no-ops,
    restore fresh/stale/queued/absent, boot prune + first-boot seed
@arpan-jain

Copy link
Copy Markdown
Contributor Author

Adversarial review (incl. empirical Lua/OOM verification against Valkey 8.1 in Docker — prod runs 8.1.4). The Lua scripts themselves are solid: CHECKOUT is genuinely atomic even under OOM (RPOP is non-denyoom, and a script's first write unlocks subsequent denyoom writes — verified 4/4 with direct ZADD failing on the same connection), and RELEASE fails cleanly with zero partial effects. The design direction is right. But the reconciliation restore path can duplicate a live-held wallet in three realistic scenarios — and duplication means two pods signing with the same key = nonce collisions, strictly worse than the leak this fixes.

Blocking (all at createRedisSenderManager.ts, PR head 939ca32):

  1. CONFIRMED — rolling deploy of this PR duplicates every wallet held by old-code pods (:110-120). Old code checks out via plain RPOP with no marker; during rollout an old pod's held wallet is in neither structure → a booting new pod restores it → another pod pops it while the old pod still bundles with it; the old pod's eventual unguarded lpush adds a second queue copy. Near-certain on base under traffic. Also fires between two new-code pods: the snapshot (:95-97) and restore (:115) aren't atomic and RELEASE_WALLET_LUA re-validates nothing. Needs an atomic restore script that re-checks marker-absent-or-stale inside Lua, plus a two-phase rollout (stamp markers first, enable restore in a second deploy).
  2. CONFIRMED — post-outage recovery mass-restores actively-held wallets (:100-106, :152) — the exact incident scenario this PR targets. Re-stamp uses ZADD, which is denyoom (verified: OOM command not allowed while over maxmemory) — during a ≥30min Redis OOM every held wallet's marker goes stale; a pod booting in the recovery gap restores live-held (including quarantined-with-unconfirmed-nonce) wallets.
  3. CONFIRMED — boot crash-loop under Redis write failure (:115, uncaught; handler.ts:270, uncaught). Under OOM the release script rejects at LPUSH (first write, denyoom — verified). Mid-incident, every pod restart dies at boot: old code degraded to read-only; new code turns "2 wallets left" into "total outage on any restart". Wrap reconciliation in try/catch and retry from the maintenance loop.

Non-blocking:

  1. CONFIRMED — self-healing only runs at boot: a crashed pod's wallets stay lost until some future pod boots >30min later. The stale-marker scan belongs in maintain() (:131-159) once finding 1's atomicity is fixed.
  2. PLAUSIBLE — at-least-once release retry (:216-224) can duplicate a wallet re-checked-out by another pod (LPOS proves "not queued", not "still mine") and its trailing ZREM deletes the new holder's marker. Fix: ownership-conditional release (if ZREM == 1 then LPUSH) — idempotent under retry; keep a separate stale-checked script for reconciliation.
  3. LOW — config-removed wallets are pruned from the marker zset but never LREM'd from the queue; popping one throws "wallet not found" out of getWallet into fire-and-forget callers. Opportunistic LREM in reconciliation.

Verified clean: checkout atomicity under OOM; release zero-partial-effects; LPOS nil-mapping + availability on Valkey 8.1; markWalletProcessed ordering/idempotency incl. un-awaited call sites and the shutdown sweep; quarantine re-stamp coverage (60s tick vs 30min staleness = 30× margin, outage case aside); re-stamp-vs-release race is harmless; first-boot double-seed safe; redisKeys prefix no-collision. tsc --noEmit passes at head.

Tests: nothing covers this code (e2e compose has no Redis, exercises the memory sender manager only). Given three of the findings are races this PR class exists to prevent, please add one ioredis integration test against a throwaway Redis: checkout/release round-trip, boot reconciliation fresh-vs-stale markers, release-when-already-queued no-op.

One ops note: if any BetterStack alert keys on the removed "utility wallet has insufficient balance" error log (validateAndRefill.ts) rather than the gauge, update it before merge.

@arpan-jain arpan-jain closed this Jul 29, 2026
@arpan-jain

Copy link
Copy Markdown
Contributor Author
  1. Rolling-deploy duplication + non-atomic restore.
    Fixed, but with a different mechanism than the two-phase rollout you suggested — open to pushback here. Boot no longer restores anything. Restores now live in the maintenance loop and only fire after a wallet has been observed continuously missing for a 10-minute grace window.
    Old-code pods overlap for ~2-4 min during a Render rolling deploy (measured on the Jul 24 deploys), so they either release the wallet (it appears in the queue, clock resets) or die (genuinely leaked,
    restore is correct) well inside the grace. The restore itself is a new RESTORE_WALLET_LUA that re-validates queue + marker state atomically inside the script, which closes the snapshot-vs-restore race between two new-code pods.
    Known residual: an old-code pod holding one wallet longer than the grace during the overlap (e.g. quarantined at deploy instant) can still be duplicated once — bounded by the nonce-too-low refetch.

  2. Post-outage mass-restore.
    Fixed — and the grace window alone was not enough, since reads keep working during OOM so the missing-clock would have accrued through the outage and raced holders' re-stamps at recovery. The restore scan is now gated on a write-health probe: each tick does a sentinel SET first; if it fails, the scan is skipped and the missing-clock resets. Staleness is only trusted while holders are demonstrably able to re-stamp. After recovery, holders re-stamp within one 60s tick vs a fresh 10-min grace for restorers — 10x margin.

  3. Boot crash-loop. Fixed. Boot reconciliation (now just prune + first-boot seed) is wrapped in try/catch; on failure it logs and starts anyway, and the maintenance loop heals whatever it missed.

  4. Healing only at boot. Fixed as suggested — the stale-marker scan runs in maintain() every 60s.

  5. At-least-once release duplication.
    Fixed exactly as you proposed: release is now ownership-conditional (if ZREM == 1 then LPUSH), so a retry after a server-side-success/client-side-timeout is a no-op. Restore keeps its own stale-checked script. Related: re-stamps now skip wallets pending release retry, so we never refresh a marker that may already belong to a new holder.

  6. Unconfigured wallets left in the queue. Fixed — boot prunes them from the queue (LREM) as well as the marker zset.

Tests. Added createRedisSenderManager.test.ts — 8 ioredis integration tests against a real Redis (vitest in the src workspace, same version as e2e; suite skips when REDIS_URL is unset so CI without Redis stays green). Covers the three you asked for plus restore fresh/stale/queued/absent and first-boot seeding. 8/8 pass against redis:7-alpine. Not covered: the time-based grace/probe behavior — faking time over a real connection was too fragile, so that's verified by review only.

Ops note. Checking the BetterStack alerts before merge; alerting will key on the executorWalletsBalances gauge rather than the removed log line.
(both structures empty at boot while old pods hold everything).

@arpan-jain arpan-jain reopened this Jul 29, 2026
@arpan-jain
arpan-jain requested a review from SahilVasava July 29, 2026 14:44
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