fix(wallet,lib,create): lock/permit leak on abort (#389), structural recipient existence check (#391), contract-recipient guard (#392), one shared withTimeout (#393) - #432
Merged
Jaydbrown merged 4 commits intoAug 31, 2026
Conversation
…l#393) An identical `withTimeout<T>(promise, ms, label)` was defined verbatim in app/create/page.tsx and contexts/WalletContext.tsx, with a third, more capable copy inside lib/soroban.ts. The copies had already drifted: only the Soroban one understood an AbortSignal or validated `ms`, and only the two UI ones produced a message a user could read. Add lib/with-timeout.ts as the single implementation. It keeps Soroban's positional `(promise, ms, label, signal)` shape so those call sites are untouched, and accepts an options object with `onTimeout` so the create form and the wallet keep their own user-facing wording. Move the operation error classes to lib/errors.ts so with-timeout can throw OperationTimeoutError/OperationAbortedError without importing safe-operations (which pulls in the Stellar SDK — WalletContext is mounted in the root layout and should not drag the SDK into that bundle). lib/safe-operations.ts re-exports them, so every existing import path and `instanceof` check is unaffected, and its own private copy of withTimeout is gone too. lib/indexer.ts keeps its AbortController: it genuinely cancels the in-flight fetch rather than racing it, which is the better tool there. Tests: lib/with-timeout.test.ts covers resolve/reject pass-through, the labelled default error, a caller-supplied error, abort before and at call time, listener cleanup, and deadline validation.
…equeue (conduit-protocol#389) `_release()` shifts the next waiter off the queue and hands it the release function *instead of* incrementing `_available` / clearing `_locked`. The waiter then re-checks its abort signal and, if it fired, rejected without ever calling `release()` — so that permit, or the mutex lock, was gone for good. After `maxConcurrentOperations` of these races the semaphore is permanently exhausted and every signTx hangs; for the Mutex a single one deadlocks connect(). The race is not theoretical: disconnect() aborts one controller that both cancels queued work and frees in-flight holders, so a holder's release() runs from an abort listener and dequeues a waiter whose signal has already fired. Release before rejecting on that path, in both primitives, so the lock or permit passes to the next waiter (or goes back to the pool). Also reject up front in Mutex.acquire() when the caller hands in an already-aborted signal. Such a signal never fires an 'abort' event, so the waiter was queued with no listener to remove it and sat there until _release() woke it — the same leak reached from the other side. Semaphore already had this guard. Tests: four regression cases in contexts/WalletContext.test.tsx — the release-from-abort-listener race for both primitives, the already-aborted signal, and a three-permit burst that verifies every permit comes back. All four fail on the unfixed code (two by deadlock, two by lost permits).
|
@jayteemoney 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! 🚀 |
…r text (conduit-protocol#391) checkRecipientExists() returned `false` — "this recipient does not exist" — for any failure whose message matched /not found|404/i. Three unrelated failures match that: a JSON-RPC `Method not found` (-32601) from a wrong RPC version, an HTTP 404 from a mistyped NEXT_PUBLIC_SOROBAN_RPC_URL, and a proxy or gateway 404. All three made the create form tell the user "Recipient account not found" when the recipient was fine and the RPC configuration was not. The substring match also never caught the case it was written for: stellar-sdk's getAccount() rejects with a plain `{ code: 404, message }` object rather than an Error, so `String(err)` was "[object Object]" and a genuinely missing account fell through to the rethrow. Ask the RPC for the recipient's ledger entry directly — the account entry for a G… key, the persistent instance entry for a C… contract — and treat an empty `entries` array, the ledger's own answer, as the only evidence for `false`. Everything that throws is "couldn't check" and propagates. A response with no entries array is malformed, not absence, so it throws too. On the create form, pass the AbortController's signal into the check (it was created per effect run and never handed to anything, so the documented cancellation never happened) and let the check own its deadline instead of racing a second local timer. Cancellation no longer flips the field to the error state, and the error copy now says the address couldn't be checked rather than implying something about the recipient. Tests: lib/soroban-recipient.test.ts covers account and contract lookups in both directions, the three misreported RPC failures, a malformed payload, an invalid address, cancellation, and the timeout.
conduit-protocol#392) The recipient field accepted any valid Stellar address, so a `C…` contract passed validation silently. `create_stream` will happily set a contract as recipient, but only an address that can *call* DripStream::withdraw as the recipient can ever pull the streamed funds out. A SAC, a plain token contract, or a multisig/vault without that call path means the deposit is streamed into a stream nobody can withdraw from, and `transfer_recipient` can only be called by the current recipient or sender. Nothing on-chain distinguishes those contracts ahead of time, so state the risk and make the user answer it: a contract recipient now renders a warning naming the consequence, plus a checkbox confirming the contract can call withdraw(). Submit stays disabled until it is ticked, the Zod schema rejects the form without it, and onSubmit re-checks before signing — the same belt-and-braces shape as the existing recipientStatus guards. Editing the address clears the acknowledgement, since it was about one specific contract. The on-chain existence line is contract-aware too: a found contract now says the contract exists and that its ability to withdraw cannot be verified, rather than "Account verified on-chain". Tests: five cases in app/create/__tests__/page.test.tsx covering the warning, the blocked and forced submit, the acknowledged happy path, the reset on address change, and that a G… recipient is unaffected.
jayteemoney
force-pushed
the
fix/389-391-392-concurrency-recipient-guards
branch
from
August 30, 2026 18:46
d5556ef to
e7c5e2e
Compare
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this PR do?
Fixes the concurrency lock/permit leak in
WalletContext, stopscheckRecipientExistsfrom misreporting an RPC failure as "recipient does not exist", warns before a contract recipient can lock a deposit, and collapses the four copies ofwithTimeoutinto one shared helper.Type of change
Related issue
Closes #389
Closes #391
Closes #392
Closes #393
Changes
contexts/WalletContext.tsxMutex/Semaphorerelease the lock/permit before rejecting a waiter aborted at dequeue;Mutex.acquire()rejects an already-aborted signal up front instead of queueing an unwakeable waiterlib/soroban.tscheckRecipientExistsreads the recipient's ledger entry viagetLedgerEntriesand treats only an emptyentriesarray as "does not exist"; every failure propagates as "couldn't check". #393 — localwithTimeoutremoved in favour of the shared oneapp/create/page.tsxwithTimeoutremovedlib/with-timeout.tswithTimeout,AbortSignal-aware, deadline-validating, listener-clean, with a caller-supplied error hooklib/errors.tswith-timeoutdoesn't pull the Stellar SDK into the wallet bundle; re-exported fromlib/safe-operations.tsso no import path changeslib/safe-operations.tswithTimeoutlib/with-timeout.test.ts,lib/soroban-recipient.test.tscontexts/WalletContext.test.tsx,app/create/__tests__/page.test.tsxCHANGELOG.md[Unreleased]#389 — aborting a queued waiter as it is dequeued leaked the lock/permit
_release()shifts the next waiter off the queue and hands it the release function instead of incrementing_available/ clearing_locked. The waiter then re-checked its abort signal and, if it had fired, rejected without ever callingrelease()— so that permit, or the mutex lock, was gone for good.The race is reachable in normal use:
disconnect()aborts one controller that both cancels queued work and frees in-flight holders, so a holder'srelease()runs from an abort listener and dequeues a waiter whose signal has already fired. AftermaxConcurrentOperationsof those, everysignTxhangs; one is enough to deadlockconnect().Both primitives now pass the lock/permit on before rejecting.
Mutex.acquire()also rejects an already-aborted signal before queueing — such a signal never fires anabortevent, so the waiter had no listener to remove it and sat there until_release()woke it and it rejected: the same leak from the other side.Semaphorealready had that guard.The four new tests fail on the unfixed code — two by deadlock (5s timeout), two on lost permits.
#391 — an unrelated RPC 404 was reported as "recipient does not exist"
The old code returned
falsefor any error message matching/not found|404/i. Three unrelated failures match: a JSON-RPCMethod not found(-32601), an HTTP 404 from a mistypedNEXT_PUBLIC_SOROBAN_RPC_URL, and a proxy/gateway 404 — all of which told the user their recipient didn't exist when the RPC configuration was the actual problem.Worth flagging: the substring match also never caught the case it was written for.
stellar-sdk'sgetAccount()rejects with a plain{ code: 404, message }object, not anError, so a genuinely missing account stringified to"[object Object]", matched nothing, and was rethrown.Rather than pattern-matching a better set of shapes, the check now asks the RPC for the ledger entry itself — the account entry for a
G…key, the persistent instance entry for aC…contract, the same keysgetAccount()/getContractData()build internally. An emptyentriesarray is the ledger's own answer and the only evidence accepted forfalse; anything that throws is "couldn't check". A response with noentriesarray is malformed, not absence, so it throws too.On the create form the
AbortControllerwas created per effect run and never handed to anything, so the cancellation its comment described never happened — the signal is now passed through, the check owns its deadline instead of racing a second local timer, and cancellation no longer flips the field into the error state. The error copy now says the address couldn't be checked rather than implying something about the recipient.#392 — a
C…recipient could silently lock the depositcreate_streamaccepts a contract as recipient, but only an address that can callDripStream::withdrawas the recipient can pull the funds out; a SAC, a token contract, or a vault without that call path strands the deposit, andtransfer_recipientis callable only by the current recipient or sender.Nothing on-chain distinguishes those contracts beforehand, so the form states the risk and makes the user answer it: a warning naming the consequence plus a checkbox confirming the contract can call
withdraw(). Submit is disabled until it's ticked, the Zod schema rejects the form without it, andonSubmitre-checks before signing (matching the existingrecipientStatusguards). Editing the address clears the acknowledgement, since it was about one specific contract. The existence line is contract-aware too — a found contract no longer reports "Account verified on-chain".I went with an inline, blocking acknowledgement rather than a
window.confirmor an advanced toggle: it is visible before the user commits, testable, and consistent with how the form already surfaces the zero-rate and not-found guards.#393 — one
withTimeoutlib/with-timeout.tskeeps the Soroban positional shape(promise, ms, label, signal)so those call sites are untouched, and takes an options object withonTimeoutwhere the create form and the wallet need their own user-facing wording. It validates the deadline, rejects withOperationAbortedErroron (or before) abort, and always removes its abort listener — the same leak class as #390.The error classes moved to
lib/errors.tsso the helper can throw them without importingsafe-operations, which pulls in the Stellar SDK;WalletContextis mounted in the root layout and shouldn't drag the SDK into that bundle. They are re-exported fromlib/safe-operations.ts, so every existing import path andinstanceofcheck is unchanged (shared First Load JS stays at 99.2 kB).lib/indexer.tsdeliberately keeps itsAbortController: it genuinely cancels the in-flight fetch rather than racing it, which is the better tool there.Checklist
npm run typecheck— no errorsnpm run lint— no warningsnpm test— 439 passed, 4 todo (32 files); 28 new testsnpm run build— production build succeedstext-red-600/border-red-200error stylingalert())'use client'added only where genuinely neededCHANGELOG.mdupdated under[Unreleased]Screenshots
Create form, contract (
C…) recipient — new state:withdraw()requirement, and a confirmation checkbox; submit disabled until tickedNotes for reviewers
mainand fast-forwards cleanly (git merge-base --is-ancestor upstream/main HEADpasses), so it merges without conflicts.lib/soroban.tsandapp/create/page.tsx. If either lands first, say the word and I'll rebase immediately.withTimeouthelper reimplemented verbatim inapp/create/page.tsxandcontexts/WalletContext.tsx#393 comes first because the other three build on the shared helper.