feat(keepers): dedup keeper submissions across invocations - #548
feat(keepers): dedup keeper submissions across invocations#548determined-001 wants to merge 3 commits into
Conversation
priorHash only tracked an unconfirmed transaction inside one invocation. A killed run (or one that exhausted its retries mid-confirmation) left the next cron tick with no memory of it, free to broadcast a second transaction while the first was still landing. For accrue() that costs a fee; for migrate_adapter it costs real slippage twice. Add a shared submission record (keeper-state.ts) written only after sendTransaction returns a hash, so a crash before broadcast leaves nothing behind to block the next run. Every run resolves an existing record against the network rather than trusting it: SUCCESS/FAILED clear it, NOT_FOUND past the transaction's own validity window ages it out, and only a genuinely in-flight one skips the target. A store or lookup failure is reported as unknown and also skips, since reading an outage as "nothing was submitted" is what produces the duplicate. Backed by the Upstash Redis the API already uses for rate limiting, over plain fetch to keep a client dependency out of the shared helper package. The migration keeper refuses to run in production without it; the accrue keeper falls back to a per-invocation store and logs that dedup is inactive, since its duplicate only wastes a fee. Also close the accrue/migrate race: the two keepers act on the same vault's adapter with no coordination, so the accrue keeper could accrue() an adapter the vault had already migrated away from, a silently ineffective call. It now runs the same live get_adapter() re-check the migration keeper already had, moved into keeper-tx.ts and shared. That re-check also covers the one window the record cannot, broadcast succeeded then the process died before the write landed. closes drydocs#515
collinsezedike
left a comment
There was a problem hiding this comment.
Two more, not anchorable since api/v1/keepers/accrue.ts and rebalance.ts aren't touched by this PR: checkRateLimit() is awaited outside the try/catch in both handlers, so an Upstash failure there escapes unhandled as a bare 500 with no [accrual-keeper]/[migration-keeper] log line, before the run (and all the careful store-outage signaling this PR adds) ever starts. Pre-existing, but worth fixing alongside this PR since it undermines the same reliability goal.
| context: Record<string, unknown> = {} | ||
| ): Promise<void> { | ||
| try { | ||
| await store.delete(key); |
There was a problem hiding this comment.
This deletes by key with no check that the stored record still matches the hash being resolved. A slow/overlapping run can delete a newer run's record: Run A's poll resolves an old hash as failed and clears the key; Run B has since written a new hash there; Run A's clear wipes it; Run C sees nothing and rebroadcasts. That's a real double migrate_adapter, double slippage, the exact bug this PR exists to prevent. Needs a conditional delete (only clear if the stored hash still matches what this call resolved).
There was a problem hiding this comment.
Fixed in dc6057e. get now returns the record together with the exact serialized bytes it was read as (revision), and both writes are conditional on it: replace and deleteIf compare-and-set against that revision, via Lua EVAL on the Upstash side since the REST API cannot express it otherwise.
Your A/B/C sequence is now a test (leaves a record another run has replaced alone instead of clearing it): B replaces the record while A is mid-lookup, A resolves its old hash as failed, and the conditional delete leaves B's record in place. A run that discovers it lost the lease stops touching the key and logs it.
|
|
||
| let record: SubmissionRecord | null; | ||
| try { | ||
| record = await store.get(key); |
There was a problem hiding this comment.
The read-check-write here is non-atomic: a plain GET, with the SET happening later after a transaction is signed and sent. Two genuinely concurrent invocations (a scheduled run overlapping a manual retry, or workflow_dispatch firing both steps) can both read "no record," both pass, and both broadcast. Needs SET NX or equivalent compare-and-set at write time, not just a read beforehand.
There was a problem hiding this comment.
Fixed in dc6057e. The read is now backed by a claim: SubmissionLease.acquire does SET NX on the key before the transaction is built, and a run that loses that race skips the target. The resolve step you see here is now only the "is there something already in flight" question; exclusivity comes from the claim, not from the read.
Claims carry a hash-less record and a much shorter window (DEFAULT_CLAIM_TTL_MS, 60s) than a submitted transaction, since nothing was signed under them and there is no transaction that could still land. A run that abandons a target without signing (stale adapter, simulation error, deadline) releases the claim in a finally rather than leaving it to expire.
| @@ -194,13 +285,20 @@ export async function submitKeeperOperation( | |||
| throw new Error("Transaction could not be submitted yet (try again later)"); | |||
There was a problem hiding this comment.
Two related gaps here. First, onSubmitted fires after this TRY_AGAIN_LATER throw, so a transaction already sitting in the mempool leaves no dedup record; the thrown message contains "try again," so isTransientKeeperError retries and rebroadcasts with no record ever written for the first attempt. Second, a few lines up, if sendTransaction itself times out in withRaceTimeout after actually reaching the network, there's no captured hash either, same outcome: the timeout is classified transient and retried, rebroadcasting a transaction that may already be landing.
There was a problem hiding this comment.
Both fixed in dc6057e, by moving the record earlier rather than adding cases.
The hash comes from the signed transaction (prepared.hash()), so it exists before the network is touched: onSigned fires there, not after sendTransaction returns. Every exit path past that point has a record.
On top of that, neither failure rebuilds any more. A send that throws (including the withRaceTimeout case you describe) and a TRY_AGAIN_LATER response both raise SubmissionInFlightError against the signed hash, so the retry rechecks that transaction instead of building a second one with a different hash. status: "ERROR" is the one case that stays a plain error, since the transaction was rejected outright and is not in flight, and it now calls onResolved to release the record rather than blocking the target until it ages out.
| ...(options.fetchImpl && { fetchImpl: options.fetchImpl }), | ||
| }); | ||
| } | ||
| if (options.requireShared && env.VERCEL_ENV === "production") { |
There was a problem hiding this comment.
Two issues with this guard: it only checks VERCEL_ENV === "production", but middleware.ts documents that preview deployments also sign real transactions off a real key. A preview deployment with Upstash unset silently falls back to an empty in-memory store, no dedup, on a real signing key. Separately, this check is actually unreachable in production: middleware.ts already throws at module load under the identical condition, before this code ever runs, so this specific guard is never exercised by any real code path.
There was a problem hiding this comment.
Fixed in dc6057e. The guard is now Boolean(env.VERCEL_ENV), so preview refuses too, with the environment named in the error. You are right that this is the case that matters: middleware.ts only fails closed on production, so preview was precisely the deployment where this check could have done something and did not.
That also answers the unreachability point: on production middleware.ts still throws first, so this guard is a backstop there rather than the primary control, but on preview it is now the only thing standing between a real signing key and a store that cannot dedup.
| continue; | ||
| } | ||
| let priorHash: string | undefined; | ||
| const submissionHooks: KeeperSubmissionHooks = { |
There was a problem hiding this comment.
The documented deps.submitMigration override point (a few lines below) bypasses these submissionHooks entirely, since hooks are only wired to the built-in submitMigrationTransaction branch. Any real caller using this injection point, which is exported public API, not just a test seam, silently gets zero cross-invocation dedup with no signal that the guard was dropped.
There was a problem hiding this comment.
Partly fixed in dc6057e, and I want to be straight about the part that is not.
deps.submitMigration now receives the hooks as a fourth argument, so an override that forwards them to submitKeeperOperation keeps full dedup. An override that ignores them cannot be forced to participate, so the run now warns at startup when one is injected, and the docs say what is lost.
What such an override still gets: the claim is taken by the keeper loop, not by the submitter, so it holds for the duration of the run, and the on-chain adapter re-check is unaffected. What it loses is the record surviving past the end of the run.
| "Refusing to run the migration keeper: UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN are required when VERCEL_ENV=production (the in-memory fallback is per-invocation and cannot prevent a duplicate migrate_adapter)" | ||
| ); | ||
| } | ||
| options.logger.info( |
There was a problem hiding this comment.
On any Upstash misconfiguration, the accrual keeper falls back to a fresh per-invocation in-memory store at info-log level only. That reinstates the exact "accepted, bounded gap" this PR's own docs now describe as closed, silently, with nothing that would page anyone.
There was a problem hiding this comment.
Fixed in dc6057e. The fallback now logs at warn with the environment attached whenever VERCEL_ENV is set, and stays at info only in local dev where the per-invocation store is the expected state. The accrue keeper still falls back rather than refusing (its duplicate costs a fee, not slippage), but it is no longer quiet about it.
|
|
||
| let lookup: { status?: string; ledger?: number } | null | undefined; | ||
| try { | ||
| lookup = await server.getTransaction(record.hash); |
There was a problem hiding this comment.
server.getTransaction(hash) here has no timeout wrapper, unlike every other keeper RPC call in this codebase (submitKeeperOperation wraps its calls in withRaceTimeout, waitForTransaction wraps its polls in withSorobanTimeout). A black-holed connection here hangs the whole run past maxDuration with no partial result returned.
There was a problem hiding this comment.
Fixed in dc6057e: the lookup goes through withRaceTimeout, using the keeper's own rpcTimeoutMs, and a timeout resolves to unknown like any other failed lookup rather than hanging the run. Covered by a test that hands it a promise that never settles.
| const url = options.url.replace(/\/+$/, ""); | ||
| const fetchImpl = options.fetchImpl ?? fetch; | ||
|
|
||
| async function command(args: (string | number)[]): Promise<unknown> { |
There was a problem hiding this comment.
Same issue for the Upstash REST call itself, no AbortSignal/timeout. A hung KV request stalls the run with no ceiling, and this can happen right after a transaction is broadcast, the worst possible moment for a stall.
There was a problem hiding this comment.
Fixed in dc6057e: every Upstash command is wrapped in withRaceTimeout and also carries an AbortSignal.timeout, so the socket is released rather than left hanging, bounded by DEFAULT_STORE_TIMEOUT_MS (5s) and overridable. Same test approach, a fetch that never settles now rejects instead of stalling.
| export function parseSubmissionTtlMs( | ||
| env: Record<string, string | undefined> | ||
| ): number { | ||
| return parsePositiveInt( |
There was a problem hiding this comment.
parseSubmissionTtlMs only enforces >= 1, no validation against the actual transaction validity window (.setTimeout(300) in keeper-tx.ts). An operator setting a TTL shorter than 300s turns the "safe to retry, provably dead" expiry logic into a duplicate-submission generator, since the original transaction can still land after the record ages out.
There was a problem hiding this comment.
Fixed in dc6057e. TX_VALIDITY_WINDOW_MS is now exported from keeper-tx.ts (and is what setTimeout is built from, so the two cannot drift), DEFAULT_SUBMISSION_TTL_MS is derived from it, and parseSubmissionTtlMs rejects anything below it with an error that says why.
|
Thanks for the depth here — the concurrency findings in particular are correct and I'd missed them. Ack on all 13 (11 inline + the two handler ones in your review body). Working through them now; I'll reply on each thread as I go rather than in one lump, and push a single follow-up commit. Two I want to flag my intended direction on early, since they change the design rather than patch it, and I'd rather hear now if you disagree:
On the fail-open contradiction you caught in the accrue keeper ( |
Review found the record-based dedup had holes that could still produce the duplicate it was meant to stop. Nothing was written before the network call, so "no record" was not a claim on the target: two concurrent invocations could both read nothing and both broadcast, and a transaction that reached the mempool before a timeout or a TRY_AGAIN_LATER left no record at all. Replace the bare record with a lease: SET NX a hash-less claim before the transaction is built, then compare-and-set the real hash in as soon as the transaction is signed, before sendTransaction rather than after it returns. A crash between signing and broadcasting now leaves a record for a transaction that never went out, which ages out at the transaction's own validity window; that is a bounded delay rather than a duplicate migration. Claims carry a much shorter window, since nothing was signed under them, and are released when a run abandons a target without signing. Every write after the claim is conditional on the exact record this run put there, so a slow run can no longer clear a record a newer run replaced and hand a third run a clean slate to rebroadcast into. A run that loses its lease stops touching the key. Send failures no longer rebuild: a timed-out send or TRY_AGAIN_LATER now raises SubmissionInFlightError against the signed hash, so the retry rechecks that transaction instead of broadcasting a second one with a different hash. A transaction rejected outright at submission releases its record instead of blocking the target until it ages out. Also from review: - The shared-store requirement covers preview as well as production; preview signs real transactions, and middleware.ts only fails closed on production, so the previous check was unreachable where it mattered. - Falling back to the per-invocation store warns on any deployment instead of logging at info level. - MERIDIAN_KEEPER_SUBMISSION_TTL_MS is rejected below the 300s transaction validity window, where the expiry rule would generate duplicates. - Both the status lookup and the Upstash request are time-bounded. - The accrue keeper proceeds (and warns) instead of halting all accrual when the store cannot be read; only the migration keeper stops, since only its duplicate costs slippage. - deps.submitAccrual/submitMigration receive the lease hooks, and a run using an injected submitter warns that dedup depends on it forwarding them. - checkRateLimit is wrapped in both keeper handlers, so an Upstash outage returns a logged 503 rather than escaping as a bare unhandled 500.
|
@determined-001 is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
All 13 addressed in dc6057e — replies are on each thread. Summary of what changed, since several of them turned out to be one root cause: The record became a lease. The trade I flagged earlier stands: a crash between signing and broadcasting now leaves a record for a transaction that never went out, cleared when it ages out at the transaction's own validity window. Bounded delay, not a lockup, and not a duplicate. Every write after the claim is conditional on the exact bytes this run wrote (compare-and-set / compare-and-delete, Lua Send failures no longer rebuild. A timed-out send and The rest: preview is covered by the shared-store refusal (that was the deployment where the guard could actually do something); the fallback warns on any deployment instead of info-logging; the TTL is rejected below the 300s validity window; the status lookup and the Upstash request are both time-bounded; the accrue keeper proceeds and warns on Verification: all packages green ( One thing worth your call: the two keepers now behave differently on an unreadable store — migration stops, accrue proceeds and warns. That is deliberate and documented in both keeper docs, but it is the one place their execution models diverge, and I would rather you sign off on the divergence than discover it later. |
|
Heads up: the CI run for dc6057e is sitting at action_required — GitHub is waiting on a maintainer to approve the workflow run for this fork. Nothing I can do from my side. The run on the previous commit (81305ff) passed everything except the Vercel deploy check, which needs deploy authorization on your side too. Everything in it is green locally — full |
| } | ||
| this.held = next; | ||
| } catch (err) { | ||
| this.logger.warn("[keeper-state] could not record submission", { |
There was a problem hiding this comment.
One new gap from this fix itself. If store.replace throws here (a transient Upstash blip at exactly this moment), the error is swallowed and this.held keeps pointing at the original claim, which still has hash: null and is still aging out on the short claim TTL, not the transaction's real ~300s validity window. A concurrent run reading this key after the claim TTL elapses but before the actual transaction resolves sees expired/none and takes the lease itself, broadcasting a duplicate for the same target, which is the exact failure this PR exists to prevent. Needs a bounded retry on this write before giving up, or the claim's own TTL extended once a hash exists so a swallowed write failure doesn't shorten the record's real lifetime.
There was a problem hiding this comment.
Fixed in 10c9d2f. write() now retries store.replace (bounded, exponential backoff via the shared withRetry) instead of swallowing the first failure. Added a test (retries recording the signed hash instead of giving up on the first transient failure) that fails twice then succeeds and asserts the hash lands with no warning logged; the existing always-fails test still asserts the eventual-warning path after retries are exhausted.
|
@determined-001 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
SubmissionLease.write() caught a store.replace() failure on the first try and left the short-TTL claim in place instead of the signed hash. A concurrent run reading that claim after the claim TTL (but well before the transaction's real ~300s validity window) would see it as expired and rebroadcast, the exact duplicate this module exists to prevent. Retry the write a bounded number of times before giving up, so a transient store blip doesn't silently shorten the record's real lifetime.
closes #515
Summary
priorHash(keeper-tx.ts) only tracked an unconfirmed transaction inside one invocation. A killed run, or one that exhausted its retries mid-confirmation, left the next cron tick with no memory of it and free to broadcast a second transaction while the first was still landing. Foraccrue()that costs a fee; formigrate_adaptereach call is its own slippage-bounded transaction, so a double-migration costs real slippage twice.This adds the cross-invocation state both keepers were missing, plus the accrue/migrate coordination gap folded into this issue's scope.
Design decision: where the state lives
Upstash Redis — the same
UPSTASH_REDIS_REST_URL/UPSTASH_REDIS_REST_TOKENpairapi/_lib/middleware.tsalready requires in production for distributed rate limiting, so production deployments have it configured. Spoken over plainfetchrather than@upstash/redis:stellar-sdk-helpersis the shared library the web build also imports, and three Redis commands don't justify a client dependency there.The on-chain-signal alternative from the issue was not taken: it is protocol-specific and can only ever answer "did the adapter change", never "is my transaction still in flight".
The state machine
The record holds only the submitted hash and the time it was broadcast, and is written only after
sendTransactionreturns a hash. There is deliberately no "about to send" state, so a crash before broadcast leaves nothing behind that could block the next run.An existing record is never trusted on its word — every run resolves it against the network:
SUCCESSFAILEDSo a record either resolves to a real outcome or ages out; nothing waits on a human.
MERIDIAN_KEEPER_SUBMISSION_TTL_MSdefaults to360000— the 300s.setTimeout(300)validity window plus 60s of clock-skew margin — and is also applied as a Redis-side expiry, so a run that dies before clearing a record can't leave one behind past the point where its transaction could still land.Skips land in
skipped[], notfailures[]: these are benign races, and returning HTTP 500 for them would page someone for correct behavior.Fallback, per risk profile
The migration keeper refuses to run in production without a shared store — a per-invocation fallback cannot dedup across invocations at all, and its duplicate costs slippage. The accrue keeper falls back and logs that dedup is inactive for the run, since a duplicate
accrue()only re-syncs a cached value from live state.Accrue vs. migrate race (folded in from the #512 review round)
The two keepers act on the same vault's adapter independently, so the accrue keeper could read
get_adapter()at discovery, have the migration keeper switch the vault mid-run, and thenaccrue()the detached adapter — a silently ineffective call, since a detached adapter is still a valid contract. It now runs the same liveget_adapter()re-check the migration keeper already had; the guard moved tokeeper-tx.ts(assertAdapterUnchanged) and is shared by both.That re-check also covers the one window the record cannot: broadcast succeeded, then the process died before the record was written. Complementary, not redundant — which is why both guards exist.
Changes
packages/stellar-sdk-helpers/src/keeper-state.ts(new) — record shape, store interface, in-memory and Upstash implementations,resolvePriorSubmissionstate machine, env loading.keeper-tx.ts—onSubmitted/onResolvedsubmission hooks (invoked defensively: a throwing hook must never surface as a submission error, or the retry loop answers it with a second broadcast); sharedStaleAdapterError/assertAdapterUnchanged.migration-keeper.ts— per-vault dedup check before evaluation (a blocked vault doesn't spend rate lookups or deadline budget), record/clear hooks,submissionTtlMsconfig.accrual-keeper.ts— same wiring, plus the adapter re-check and askipped[]entry for the detached-adapter case.migration-keeper.md"accepted, bounded gap" framing replaced with the state machine and the two-guard rationale;accrual-keeper.mdupdated with its own record keying and the race section;environment-variables.mdgainsMERIDIAN_KEEPER_SUBMISSION_TTL_MSand theUPSTASH_REDIS_REST_*rows.Verification
npm run test— all packages green (shared 4 files, api-core 3, api 3, api-local 1, stellar-sdk-helpers 14 files / 227 tests, web 16 files / 90 tests).npm run coverage— passes every package's thresholds;stellar-sdk-helpersbranch coverage 84.53% (threshold 76). New/changed files:keeper-state.ts94.5% branches,keeper-tx.ts93.75%,accrual-keeper.ts93.3%,migration-keeper.ts88%.npm run typecheck,npm run typecheck:api,npm run lint,npx prettier --check .— all clean.PXexpiry, malformed values, HTTP failure reported by status without echoing the token), the production refusal, and end-to-end runs of both keepers covering in-flight skip, landed-then-proceed, age-out, "record written before the confirmation wait, cleared after", store-outage skip, and the detached-adapter skip.Not verified on testnet: the migration keeper still has nothing live to act on (#514's vault predates
migrate_adapter, and the rate source is inert per #511), so a real double-submission cannot be staged there yet. The dedup path is exercised end-to-end in tests instead.closes #515