darepod: unlock wallet VTXOs on OOR cleanup timeout

[Roasbeef]

The bug

cleanupSubmittedOORStartWithTimeout in darepod/rpc_server.go builds a
detached, bounded cleanup context:

cleanupCtx, cancel := context.WithTimeout(
    context.WithoutCancel(ctx), timeout, // submittedOORCleanupTimeout (10m)
)

and then future.Await(cleanupCtx). When the detached OOR start future never
completes, Await returns because cleanupCtx hit its deadline, so
cleanupCtx is now expired. The very next statement passed that same expired
cleanupCtx to unlockSelectedVTXOsBestEffort, which flows to
walletRef.Tell(cleanupCtx, &wallet.UnlockVTXOsRequest{...}).

ChannelMailbox.Send rejects an already-expired context before enqueue, so the
unlock was silently dropped and the wallet-selected VTXOs stayed pinned in
SpendingState — potentially indefinitely.

The custom-input release path was unaffected (its callback is a pure in-memory
map delete, context-independent). Only the wallet-selected VTXO unlock at
timeout was broken.

The fix

In the timeout branch, derive a fresh bounded context from the detached base
(context.WithTimeout(context.WithoutCancel(ctx), submittedOORUnlockTimeout),
30s, with its own defer cancel()) and use that for the unlock. The
completion and actor-failure branches keep using the still-live cleanupCtx,
and the custom-input release still happens in every branch. A name-prefixed
comment explains why the fresh context is required.

The test

TestSubmittedOORCleanupTimeoutReleasesCustomInput passed nil for locked
and only checked the in-memory custom-input callback, which is why the gap went
unnoticed. This adds TestSubmittedOORCleanupTimeoutReleasesSelectedVTXOs,
which drives the timeout path with a non-nil locked set against a recording
wallet actor (sendOORTestWallet) and asserts an UnlockVTXOsRequest is
actually delivered on timeout. Verified that the new test fails on the
unpatched code (VTXOs never unlocked) and passes with the fix. The existing
test is unchanged.

Fixes #387

[Roasbeef]

@claude review this

[gemini-code-assist]

Code Review

This pull request addresses an issue where wallet-selected VTXOs could remain locked if the OOR cleanup waiter timed out. When a timeout occurs, the original context is expired, causing the wallet actor to reject the unlock message. To fix this, a fresh context with a 30-second timeout is now derived and used for the unlock operation. A corresponding unit test has been added to verify this behavior. I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[claude]

Claude finished @Roasbeef's task in 1m 29s —— View job

---

Review: darepod: unlock wallet VTXOs on OOR cleanup timeout

• Read the PR diff
• Analyze cleanupSubmittedOORStartWithTimeout context handling
• Review the new test
• Post review feedback

The fix is correct and well-targeted. Nice catch on a genuinely subtle bug.

Correctness — confirmed

The root cause is real: in the err != nil branch, future.Await(cleanupCtx) can return precisely because cleanupCtx hit its deadline. The original code then passed that same expired cleanupCtx to unlockSelectedVTXOsBestEffort → unlockVTXOs → wRef.Tell(cleanupCtx, ...). Since ChannelMailbox.Send checks the context before enqueue, the unlock Tell was rejected and the wallet-selected VTXOs stayed pinned. The fresh context.WithTimeout(context.WithoutCancel(ctx), 30s) cleanly fixes that.

A few things I verified to be sure the fix is sound:

• defer freshCancel() lifetime is safe. unlockVTXOs calls wRef.Tell(...), which only needs the context to survive the enqueue. Tell returns once the message is enqueued; the wallet actor processes it independently afterward (darepod/rpc_server.go:2900). The deferred cancel runs when the goroutine returns — well after the enqueue — so the context is live for the only moment it matters. 30s is a generous margin for an enqueue. ✅
• The timeout discriminator is reliable. cleanupCtx has exactly one cancellation source (its own WithTimeout over a WithoutCancel base), so after Await returns with err != nil, cleanupCtx.Err() != nil unambiguously means the deadline fired vs. a real actor failure. ✅
• The other branches correctly keep cleanupCtx. The type-assertion failure (darepod/rpc_server.go:2865) and resp.Existing (:2874) paths are only reachable when err == nil — i.e. the await completed with a result — so cleanupCtx is still live there. No change needed, and the PR body's reasoning matches. ✅
• context.WithoutCancel(ctx) base. Correct — the original RPC ctx is already (presumably) cancelled by the time we're in detached cleanup, so detaching is essential for the fresh ctx to actually be usable. ✅

Test — good

TestSubmittedOORCleanupTimeoutReleasesSelectedVTXOs drives the exact gap the prior test missed: it passes a non-nil locked set, never completes the promise (forcing the timeout branch), and asserts an UnlockVTXOsRequest actually lands on the recording wallet actor. This is a real regression guard — the PR notes it fails on unpatched code, which is consistent with the bug. Leaving the existing custom-input test untouched is the right call.

Minor / optional (no changes requested)

1. Helper reuse. There's an existing isAwaitContextError(ctx, err) helper (darepod/rpc_server.go:2788) that captures essentially this "did the await end due to ctx?" question. The direct cleanupCtx.Err() != nil check here is arguably clearer and slightly more robust (it doesn't depend on the error wrapping context.DeadlineExceeded), so I'd lean toward keeping it as-is — just flagging the parallel helper for awareness.

2. No error log on real actor failure. The ErrorS log is gated on the timeout case only; a genuine actor failure (err != nil, cleanupCtx.Err() == nil) unlocks silently. That's pre-existing behavior, not introduced here, but if detached actor failures are worth surfacing it could be a small follow-up.

Both are optional. The change is correct, minimal, and the comment explaining why the fresh context is required is exactly the kind of WHY-comment the codebase favors. LGTM. ✅
• oor-cleanup-unlock-fresh-ctx