fix: prevent swap autoclaim race between service worker and UI#558
fix: prevent swap autoclaim race between service worker and UI#558
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughUpdates: change lint script quoting in package.json; add a swapManagerProcessing flag in the Boltz Swap UI to reflect SwapManager.isProcessing(), disable the primary action while processing, adjust button label to "Claiming...", and have the button handler early-exit when SwapManager reports processing. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Swap UI
participant Manager as SwapManager
participant Backend as Backend/Blockchain
User->>UI: Click Claim/Refund button
UI->>Manager: query isProcessing(swapId)
alt already processing
Manager-->>UI: true
UI-->>User: button disabled / no-op (show "Claiming...")
else not processing
Manager-->>UI: false
UI->>Manager: initiate claim/refund(swapId)
Manager->>Backend: submit transaction
Backend-->>Manager: tx result
Manager-->>UI: update swap status
UI-->>User: update UI (success/failure)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying wallet-bitcoin with
|
| Latest commit: |
4a6ca3e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://67c741cf.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-swap-autoclaim-race.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
4a6ca3e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://83f2f75b.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-swap-autoclaim-race.arkade-wallet.pages.dev |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — fix: prevent swap autoclaim race between service worker and UI
Good fix. The core approach — claimingRef + claimingSwapIds state with mark/unmark in finally — is the right pattern for this race. Three issues to address:
🔴 Bug: waitAndClaim / waitAndClaimArk missing idempotency guard
src/providers/swaps.tsx:261-275 (diff lines)
claimVHTLC, claimArk, and claimBtc all check claimingRef.current.has(swap.id) and bail early if already claiming. But waitAndClaim and waitAndClaimArk do NOT have this guard — they unconditionally markClaiming and proceed.
This matters because waitAndClaim is the long-lived poller. If React's useEffect in QrCode.tsx re-fires (StrictMode, dep changes, component remount), two concurrent arkadeSwaps.waitAndClaim() calls will race on the same swap — exactly the bug this PR is fixing.
// current (no guard):
const waitAndClaim = async (swap: BoltzReverseSwap): Promise<void> => {
if (!arkadeSwaps) return
markClaiming(swap.id)
...
// should be:
const waitAndClaim = async (swap: BoltzReverseSwap): Promise<void> => {
if (!arkadeSwaps || claimingRef.current.has(swap.id)) return
markClaiming(swap.id)
...Same for waitAndClaimArk.
🟡 Incomplete: lint:fix script still uses single quotes
package.json:43
lint was fixed to double quotes for Windows, but lint:fix was not:
"lint:fix": "eslint 'src/**/*.{ts,tsx,js,jsx}' --fix",Should be \"src/**/*.{ts,tsx,js,jsx}\" for consistency.
🟡 Test mocks not updated
src/test/screens/mocks.ts:87-114
mockSwapsContextValue is missing the three new context fields: waitAndClaim, waitAndClaimArk, and claimingSwapIds. This will cause TypeScript errors if any test references the full context type, and will break if tests for QrCode.tsx or Swap.tsx are added that exercise the new behavior.
Add:
waitAndClaim: async () => {},
waitAndClaimArk: async () => {},
claimingSwapIds: new Set<string>(),Minor observations (no action needed)
- The
useEffectinQrCode.tsx(line ~208) has no cleanup for in-flightwaitAndClaim/waitAndClaimArkpromises. If the user changes amount, old pollers keep running. Pre-existing issue, not introduced here, but the idempotency guard above would at least prevent the new poller from double-claiming. - Context value object is recreated every render (no
useMemo). Also pre-existing.
Verdict: Fix the missing idempotency guard on waitAndClaim/waitAndClaimArk — that's the same race this PR exists to solve. The rest is minor cleanup.
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
4a6ca3e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4c8f3654.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-swap-autoclaim-race.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
Use SwapManager.isProcessing() to check the service worker's actual swapsInProgress state before allowing a manual claim from the UI. The button reactively shows "Claiming..." and disables when the SwapManager is already processing the swap (auto-claim, autonomous action, etc).
Kukks
force-pushed
the
fix/swap-autoclaim-race
branch
from
3467fdd to
5995fc6
Compare
There was a problem hiding this comment.
Code Review — fix: prevent swap autoclaim race (re-review after force-push)
The approach changed completely from the previous version (client-side claimingRef Set in SwapsProvider) to querying the service worker's SwapManager.swapsInProgress Set via isProcessing(). This is architecturally better — single source of truth in the service worker.
Overall: solid improvement, minor issues only. Approving.
🟡 TOCTOU race in buttonHandler (low practical risk)
src/screens/Apps/Boltz/Swap.tsx:164-167
if (swapManager) {
const alreadyProcessing = await swapManager.isProcessing(swapInfo.id)
if (alreadyProcessing) return
}
// ... gap where service worker can start processing ...
await claimVHTLC(swapInfo)isProcessing() crosses the service worker boundary (async message), then there's a window before claimVHTLC executes where the SwapManager could start its own auto-claim via executeAutonomousAction. Both would call claimVHTLC concurrently.
Practical risk is low because:
- The button is already reactively disabled via
swapManagerProcessingstate — user needs precise timing - Even if both fire, the on-chain VHTLC claim is atomic — double-claim yields a "VHTLC is already spent" error, not fund loss
- The error is caught and displayed cleanly (line 192-194)
No action required, just documenting the limitation. A proper fix would be an atomic tryStartProcessing(swapId) method on SwapManager that checks + locks in one call.
🟡 lint:fix script still uses single quotes
package.json:46
Same issue I flagged in the previous review — lint was fixed to double quotes for Windows but lint:fix was not:
"lint:fix": "eslint 'src/**/*.{ts,tsx,js,jsx}' --fix",Should be:
"lint:fix": "eslint \"src/**/*.{ts,tsx,js,jsx}\" --fix",✅ What looks good
- Querying
SwapManager.swapsInProgress(the service worker's canonical lock) instead of maintaining a parallel client-side Set — eliminates state drift between UI and worker cancelledflag in useEffect cleanup prevents stale state updates- Button reactively shows "Claiming..." and disables during auto-claim
- Swap update subscription re-checks
isProcessingon each status change — keeps UI in sync - No protocol-critical changes — this is purely UI-layer race prevention; the actual claim logic in
arkade-swaps.tsandswap-manager.tsis untouched
LGTM with the lint:fix nit. ✅
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 45-46: The "lint:fix" npm script still uses single quotes which
breaks on Windows shells; update the "lint:fix" script (the "lint:fix" entry in
package.json) to use the same quoting as "lint" (use double quotes around the
glob or remove quotes entirely) so the eslint command eslint
"src/**/*.{ts,tsx,js,jsx}" --fix runs cross-platform; ensure you edit the
"lint:fix" value to match the Windows-safe form used by "lint".
In `@src/screens/Apps/Boltz/Swap.tsx`:
- Around line 159-167: The guard that calls swapManager.isProcessing should only
run for claim flows (when isClaimable is true) and when it detects
alreadyProcessing it must update swapManagerProcessing to keep the UI in sync
before returning; update the buttonHandler (and the analogous handler around the
isClaimable/refund branch at the other spot) to: 1) only call
swapManager.isProcessing when isClaimable is true, and 2) if alreadyProcessing
is true set swapManagerProcessing (the state/flag used to disable the UI)
appropriately (e.g., set to true) before returning so the button label/disabled
state reflects the ongoing manager processing.
- Around line 63-72: The two calls to swapManager.isProcessing (inside the
initial check and inside the subscribeToSwapUpdates callback) lack error
handling; extract a helper like safeCheckProcessing(swapId) that calls
swapManager.isProcessing(swapId).then(p => { if (!cancelled)
setSwapManagerProcessing(p) }).catch(err => { console.error('isProcessing
failed', err, swapId); if (!cancelled) setSwapManagerProcessing(false) }) and
replace both direct calls with safeCheckProcessing(swapInfo.id) so failures are
logged and swapManagerProcessing is reset to a safe default.
🪄 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: f84d8766-2905-4216-905d-d63fb140d4ac
📒 Files selected for processing (2)
package.jsonsrc/screens/Apps/Boltz/Swap.tsx
| swapManager.isProcessing(swapInfo.id).then((p) => { | ||
| if (!cancelled) setSwapManagerProcessing(p) | ||
| }) | ||
|
|
||
| swapManager | ||
| .subscribeToSwapUpdates(swapInfo.id, (updatedSwap) => { | ||
| setSwapInfo(updatedSwap) | ||
| swapManager.isProcessing(updatedSwap.id).then((p) => { | ||
| if (!cancelled) setSwapManagerProcessing(p) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual file and its imports
head -50 src/screens/Apps/Boltz/Swap.tsxRepository: arkade-os/wallet
Length of output: 2284
🏁 Script executed:
# Read the specific lines mentioned in the review (63-72) and surrounding context
sed -n '50,85p' src/screens/Apps/Boltz/Swap.tsxRepository: arkade-os/wallet
Length of output: 1106
🏁 Script executed:
# Check all imports in the file to verify consoleError is available
grep -n "^import" src/screens/Apps/Boltz/Swap.tsx | head -20Repository: arkade-os/wallet
Length of output: 1187
🏁 Script executed:
# Search for other similar unhandled promise patterns in this file
rg -n "\.then\(" src/screens/Apps/Boltz/Swap.tsx | head -20Repository: arkade-os/wallet
Length of output: 218
Handle failed processing-state checks with proper error containment.
Both isProcessing() calls (lines 63–65 and 70–72) lack .catch() handlers, which leaves unhandled promise rejections and stale swapManagerProcessing state if the swap-manager check fails. This can freeze the claim/refund button UI. Extract a helper function to centralize error handling, log the failure, and reset state to a safe default.
🛡️ Proposed fix
- swapManager.isProcessing(swapInfo.id).then((p) => {
- if (!cancelled) setSwapManagerProcessing(p)
- })
+ const refreshSwapManagerProcessing = (id: string) => {
+ swapManager
+ .isProcessing(id)
+ .then((p) => {
+ if (!cancelled) setSwapManagerProcessing(p)
+ })
+ .catch((error) => {
+ if (!cancelled) {
+ consoleError(error, `Error checking swap processing state ${id}`)
+ setSwapManagerProcessing(false)
+ }
+ })
+ }
+
+ refreshSwapManagerProcessing(swapInfo.id)
swapManager
.subscribeToSwapUpdates(swapInfo.id, (updatedSwap) => {
setSwapInfo(updatedSwap)
- swapManager.isProcessing(updatedSwap.id).then((p) => {
- if (!cancelled) setSwapManagerProcessing(p)
- })
+ refreshSwapManagerProcessing(updatedSwap.id)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| swapManager.isProcessing(swapInfo.id).then((p) => { | |
| if (!cancelled) setSwapManagerProcessing(p) | |
| }) | |
| swapManager | |
| .subscribeToSwapUpdates(swapInfo.id, (updatedSwap) => { | |
| setSwapInfo(updatedSwap) | |
| swapManager.isProcessing(updatedSwap.id).then((p) => { | |
| if (!cancelled) setSwapManagerProcessing(p) | |
| }) | |
| const refreshSwapManagerProcessing = (id: string) => { | |
| swapManager | |
| .isProcessing(id) | |
| .then((p) => { | |
| if (!cancelled) setSwapManagerProcessing(p) | |
| }) | |
| .catch((error) => { | |
| if (!cancelled) { | |
| consoleError(error, `Error checking swap processing state ${id}`) | |
| setSwapManagerProcessing(false) | |
| } | |
| }) | |
| } | |
| refreshSwapManagerProcessing(swapInfo.id) | |
| swapManager | |
| .subscribeToSwapUpdates(swapInfo.id, (updatedSwap) => { | |
| setSwapInfo(updatedSwap) | |
| refreshSwapManagerProcessing(updatedSwap.id) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/Apps/Boltz/Swap.tsx` around lines 63 - 72, The two calls to
swapManager.isProcessing (inside the initial check and inside the
subscribeToSwapUpdates callback) lack error handling; extract a helper like
safeCheckProcessing(swapId) that calls swapManager.isProcessing(swapId).then(p
=> { if (!cancelled) setSwapManagerProcessing(p) }).catch(err => {
console.error('isProcessing failed', err, swapId); if (!cancelled)
setSwapManagerProcessing(false) }) and replace both direct calls with
safeCheckProcessing(swapInfo.id) so failures are logged and
swapManagerProcessing is reset to a safe default.
| const buttonLabel = swapManagerProcessing ? 'Claiming...' : isClaimable ? 'Complete swap' : 'Refund swap' | ||
| const refunded = swapInfo.status === 'transaction.refunded' | ||
|
|
||
| const buttonHandler = async () => { | ||
| try { | ||
| if (swapManager) { | ||
| const alreadyProcessing = await swapManager.isProcessing(swapInfo.id) | ||
| if (alreadyProcessing) return | ||
| } |
There was a problem hiding this comment.
Scope the guard to claims and sync stale UI on early return.
The current swapManagerProcessing handling also disables/short-circuits refund flows, even though this guard is meant for auto-claim races. Also, when the handler discovers alreadyProcessing, it returns without setting swapManagerProcessing, so a stale UI can remain enabled.
🐛 Proposed fix
- const buttonLabel = swapManagerProcessing ? 'Claiming...' : isClaimable ? 'Complete swap' : 'Refund swap'
+ const claimInProgress = swapManagerProcessing && !isRefundable
+ const buttonLabel = claimInProgress ? 'Claiming...' : isClaimable ? 'Complete swap' : 'Refund swap'
const refunded = swapInfo.status === 'transaction.refunded'
const buttonHandler = async () => {
try {
- if (swapManager) {
+ if (isClaimable && swapManager) {
const alreadyProcessing = await swapManager.isProcessing(swapInfo.id)
- if (alreadyProcessing) return
+ if (alreadyProcessing) {
+ setSwapManagerProcessing(true)
+ return
+ }
}- {!success && (isRefundable || isClaimable || swapManagerProcessing) ? (
+ {!success && (isRefundable || isClaimable || claimInProgress) ? (
<ButtonsOnBottom>
- <Button onClick={buttonHandler} label={buttonLabel} disabled={processing || swapManagerProcessing} />
+ <Button onClick={buttonHandler} label={buttonLabel} disabled={processing || claimInProgress} />
</ButtonsOnBottom>
) : null}Also applies to: 233-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/Apps/Boltz/Swap.tsx` around lines 159 - 167, The guard that calls
swapManager.isProcessing should only run for claim flows (when isClaimable is
true) and when it detects alreadyProcessing it must update swapManagerProcessing
to keep the UI in sync before returning; update the buttonHandler (and the
analogous handler around the isClaimable/refund branch at the other spot) to: 1)
only call swapManager.isProcessing when isClaimable is true, and 2) if
alreadyProcessing is true set swapManagerProcessing (the state/flag used to
disable the UI) appropriately (e.g., set to true) before returning so the button
label/disabled state reflects the ongoing manager processing.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
2 participants
Summary
SwapManager.isProcessing(swapId)on mount and after each status update to reactively track whether the service worker is already claiming/refunding the swapisProcessingagain at button-press time as a final guard before calling claim/refundProblem
When the service worker's
SwapManagerauto-claims a swap (viaexecuteAutonomousAction), the user can simultaneously tap "Complete swap" on the swap detail screen. Both paths callclaimVHTLCconcurrently, causing a double-claim race.The
SwapManageralready tracks in-progress operations in itsswapsInProgressSet and exposesisProcessing(swapId)via the service worker message protocol. This PR queries that source of truth from the UI rather than building a parallel tracking system in the main thread.Test plan
Summary by CodeRabbit
Bug Fixes
Chores