mobile: prevent wallet stalls on external I/O - #1131
Conversation
Mobile hosts cannot pass caller contexts through gomobile. They may retain stale transports across OS suspension. Bound repeatable reads and invoice creation without retrying an uncertain receive. Keep the local satoshi balance available when optional remote credit enrichment stalls.
Mobile wallet recovery asks the chain backend to filter remote blocks. The v0.18.0 wallet holds its database writer while that backend call is in flight, which can stall unrelated wallet operations on a slow path. Update to the merged btcwallet recovery change. It builds requests under a short read transaction, performs filter I/O without an open transaction, and atomically persists each batch while serializing address derivation. This keeps recovery progress atomic without blocking unrelated database writers during remote filter calls. The iOS binding build and the full wallet-runtime unit suite pass against the new version.
3e9504c to
a4575fe
Compare
bhandras
changed the title
📚 Doc gardening advisoryThis PR's Go changes leave diff --git a/sdk/wavewalletdk/mobile/AGENTS.md b/sdk/wavewalletdk/mobile/AGENTS.md
index b83bfdaa..65b71ee8 100644
--- a/sdk/wavewalletdk/mobile/AGENTS.md
+++ b/sdk/wavewalletdk/mobile/AGENTS.md
@@ -27,12 +27,19 @@ application-facing wallet API.
`applyMobileConfig` into a `wavewalletdk.Config`; validated (non-negative
durations/counts, `uint32`-safe recovery window) before merging onto
`wavewalletdk.DefaultConfig()`.
+- `mobileReceiveRequest` — unexported wire struct for `Receive`, decoded by
+ `decodeReceiveRequest`. It carries the SDK's `AmountSat`/`Memo` plus a
+ mobile-only `TimeoutSeconds`, and is projected onto a
+ `wavewalletdk.ReceiveRequest` so the SDK DTO stays unchanged. Older
+ bindings that never send the field still decode.
- RPC verbs (`GetInfo`, `CreateWallet`, `UnlockWallet`, `Balance`, `Deposit`,
`Receive`, `PrepareSend`, `SendPrepared`, `List`, `Exit`, `ExitStatus`,
`ExitSummary`, `GetExitPlan`, `SweepWallet`, `Status`, `Subscribe`,
`OpenWalletFromPasskey`) — each dereferences the singleton `wavewalletdk.Client`
via `activeClient()`, decodes a JSON request into the matching
`wavewalletdk.*Request`, and marshals the `wavewalletdk.*Result` response.
+ `Receive` is the one verb whose request is not the SDK DTO verbatim: it
+ decodes `mobileReceiveRequest` to pick up the mobile-only deadline.
- Scalar conveniences (`ConfirmedBalanceSat`, `PendingInboundSat`,
`WalletReady`, `IsRunning`) — avoid a JSON round trip for hot-path reads;
`IsRunning` never blocks on an RPC.
@@ -57,6 +64,23 @@ application-facing wallet API.
can succeed (e.g. after OS suspend/resume).
- `startEmbedded` separates its daemon-readiness deadline from the lifecycle
context used to open or recover the wallet. `Stop` cancels both.
+- gomobile cannot carry a caller `context.Context`, so this package owns every
+ per-call deadline rather than letting a verb inherit the daemon lifetime.
+ Repeatable reads (`Balance`, `List`, `Status`) use `readContext`'s
+ `defaultReadTimeout` (10s) — safe to bound because the host can simply ask
+ again. `Receive` uses the caller's `TimeoutSeconds`, falling back to
+ `defaultReceiveTimeout` (20s) when zero or omitted and capped by
+ `maxReceiveTimeout` (5m).
+- `decodeReceiveRequest` rejects negative and over-cap `TimeoutSeconds`
+ instead of clamping them, so malformed host input cannot restore the
+ effectively unbounded receive this field exists to avoid.
+- A `Receive` deadline error does not prove the receive session was never
+ created. The host must reconcile the authoritative Activity view and recover
+ any matching invoice before deliberately requesting another one; the bounded
+ reads carry no such ambiguity.
+- Every bounded per-call context derives from the daemon-lifetime parent that
+ `activeClient()` returns, so `Stop` still cancels an in-flight verb
+ immediately rather than waiting out its deadline.
- Startup through `startEmbedded` and `Subscription.Next` recover panics into
a returned `error`; those are the entry points documented to survive a panic
without crossing the gomobile boundary and killing the host process.
@@ -70,6 +94,9 @@ application-facing wallet API.
## Deep Docs
+- [docs/wavewalletdk_mobile.md](../../../docs/wavewalletdk_mobile.md) — Binding
+ ABI reference: JSON field-name casing rules, per-verb deadlines, and the
+ host-side background/resume contract.
- [sdk/wavewalletdk/CLAUDE.md](../CLAUDE.md) — Wrapped SDK; see for full DTO and
RPC method detail.
- [ARCHITECTURE.md](../../../ARCHITECTURE.md) — System-wide package map.
diff --git a/sdk/wavewalletdk/mobile/CLAUDE.md b/sdk/wavewalletdk/mobile/CLAUDE.md
index b83bfdaa..65b71ee8 100644
--- a/sdk/wavewalletdk/mobile/CLAUDE.md
+++ b/sdk/wavewalletdk/mobile/CLAUDE.md
@@ -27,12 +27,19 @@ application-facing wallet API.
`applyMobileConfig` into a `wavewalletdk.Config`; validated (non-negative
durations/counts, `uint32`-safe recovery window) before merging onto
`wavewalletdk.DefaultConfig()`.
+- `mobileReceiveRequest` — unexported wire struct for `Receive`, decoded by
+ `decodeReceiveRequest`. It carries the SDK's `AmountSat`/`Memo` plus a
+ mobile-only `TimeoutSeconds`, and is projected onto a
+ `wavewalletdk.ReceiveRequest` so the SDK DTO stays unchanged. Older
+ bindings that never send the field still decode.
- RPC verbs (`GetInfo`, `CreateWallet`, `UnlockWallet`, `Balance`, `Deposit`,
`Receive`, `PrepareSend`, `SendPrepared`, `List`, `Exit`, `ExitStatus`,
`ExitSummary`, `GetExitPlan`, `SweepWallet`, `Status`, `Subscribe`,
`OpenWalletFromPasskey`) — each dereferences the singleton `wavewalletdk.Client`
via `activeClient()`, decodes a JSON request into the matching
`wavewalletdk.*Request`, and marshals the `wavewalletdk.*Result` response.
+ `Receive` is the one verb whose request is not the SDK DTO verbatim: it
+ decodes `mobileReceiveRequest` to pick up the mobile-only deadline.
- Scalar conveniences (`ConfirmedBalanceSat`, `PendingInboundSat`,
`WalletReady`, `IsRunning`) — avoid a JSON round trip for hot-path reads;
`IsRunning` never blocks on an RPC.
@@ -57,6 +64,23 @@ application-facing wallet API.
can succeed (e.g. after OS suspend/resume).
- `startEmbedded` separates its daemon-readiness deadline from the lifecycle
context used to open or recover the wallet. `Stop` cancels both.
+- gomobile cannot carry a caller `context.Context`, so this package owns every
+ per-call deadline rather than letting a verb inherit the daemon lifetime.
+ Repeatable reads (`Balance`, `List`, `Status`) use `readContext`'s
+ `defaultReadTimeout` (10s) — safe to bound because the host can simply ask
+ again. `Receive` uses the caller's `TimeoutSeconds`, falling back to
+ `defaultReceiveTimeout` (20s) when zero or omitted and capped by
+ `maxReceiveTimeout` (5m).
+- `decodeReceiveRequest` rejects negative and over-cap `TimeoutSeconds`
+ instead of clamping them, so malformed host input cannot restore the
+ effectively unbounded receive this field exists to avoid.
+- A `Receive` deadline error does not prove the receive session was never
+ created. The host must reconcile the authoritative Activity view and recover
+ any matching invoice before deliberately requesting another one; the bounded
+ reads carry no such ambiguity.
+- Every bounded per-call context derives from the daemon-lifetime parent that
+ `activeClient()` returns, so `Stop` still cancels an in-flight verb
+ immediately rather than waiting out its deadline.
- Startup through `startEmbedded` and `Subscription.Next` recover panics into
a returned `error`; those are the entry points documented to survive a panic
without crossing the gomobile boundary and killing the host process.
@@ -70,6 +94,9 @@ application-facing wallet API.
## Deep Docs
+- [docs/wavewalletdk_mobile.md](../../../docs/wavewalletdk_mobile.md) — Binding
+ ABI reference: JSON field-name casing rules, per-verb deadlines, and the
+ host-side background/resume contract.
- [sdk/wavewalletdk/CLAUDE.md](../CLAUDE.md) — Wrapped SDK; see for full DTO and
RPC method detail.
- [ARCHITECTURE.md](../../../ARCHITECTURE.md) — System-wide package map.How to apply Save the diff above to a file and Either way, keep This check is advisory only — it never fails the build. Workflow run · doc-gardening PR advisory |
|
Gateway review context for head
Validation on this exact head:
For each proposed finding, require a concrete production trigger and execution trace, consequence, PR attribution, existing guards or recovery, smallest useful fix, and fix regression risk. Classify it as blocker, follow-up, or not worth changing. Do not block on speculative, unreachable, harmless, or pre-existing conditions. |
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 5 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 5 Minor · 🔵 0 Nit
Summary
The change is well-scoped and the central judgement call — bounding repeatable reads with a binding-owned deadline while treating Receive as a distinct, uncertain-outcome operation — is the right one. decodeReceiveRequest rejects rather than clamps out-of-range input and checks the negative and over-cap cases before the time.Duration multiply, so the overflow path the test names is genuinely unreachable. The swapwallet side is a clean two-second bound on optional enrichment that degrades to the authoritative local balance, and TestServiceBalanceBoundsStalledCreditRead pins the incident shape rather than the implementation.
What I'd tighten falls into three buckets: the mobile Receive DTO is hand-copied rather than embedded, so the SDK request type and the binding can drift apart silently; the "repeatable read" rule is applied to three verbs but not to the other read-only ones in the same file; and the new TimeoutSeconds contract is described as clamping in docs/wavewalletdk_mobile.md while the code rejects. None of these is a correctness failure on the paths the tests cover.
Nothing here changes the deadline semantics themselves, so this is COMMENT. Separately, sdk/wavewalletdk/mobile/CLAUDE.md / AGENTS.md still describe the pre-PR "every request decodes into the matching wavewalletdk.*Request DTO" rule; the doc-gardening bot already posted a patch for that, so I'm not duplicating it as a finding.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| // mobileReceiveRequest extends the SDK receive DTO with a mobile-only request | ||
| // deadline. The extra JSON field is backwards-compatible with older bindings, | ||
| // whose encoding/json decoder ignores it. | ||
| type mobileReceiveRequest struct { |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F1 (Minor) — Mobile Receive drops any ReceiveRequest field beyond two · sdk/wavewalletdk/mobile/wallet.go:43
mobileReceiveRequest hand-copies AmountSat and Memo rather than embedding wavewalletdk.ReceiveRequest, so any other field on that DTO — today or after a future SDK change — is silently dropped at the binding instead of failing to compile; I cannot enumerate the DTO's fields because sdk/wavewalletdk/types.go is not in the loaded context, so this may be latent rather than live. Embedding (struct { wavewalletdk.ReceiveRequest; TimeoutSeconds int64 }) preserves the flat PascalCase wire shape, since encoding/json promotes embedded struct fields, and lets Receive forward the whole DTO.
There was a problem hiding this comment.
Not worth changing. wavewalletdk.ReceiveRequest currently has exactly AmountSat and Memo (sdk/wavewalletdk/types.go:191). No current field is dropped. This binding maps the mobile-only timeout at the owning boundary. Embedding would silently opt future SDK fields into the mobile wire ABI instead of forcing deliberate binding review, so there is no concrete defect to fix here.
|
|
||
| // GetInfo returns the daemon readiness snapshot as JSON (wavewalletdk.Info). | ||
| func GetInfo() ([]byte, error) { | ||
| client, ctx, err := activeClient() |
There was a problem hiding this comment.
🟡 F2 (Minor) — GetInfo/ExitStatus/ExitSummary keep the unbounded deadline · sdk/wavewalletdk/mobile/wallet.go:84
GetInfo, ExitStatus, and ExitSummary are read-only and just as safe for a host to request again, but they still pass activeClient()'s daemon-lifetime context straight through, so they stall on a frozen transport exactly the way Balance did before this PR. Either route them through readContext as well, or note in the binding doc why readiness polling is deliberately exempt.
There was a problem hiding this comment.
Fixed in 5183f572. readContext now bounds GetInfo, ExitStatus, ExitSummary, and GetExitPlan, plus the scalar balance/readiness helpers. Balance, List, and Status remain bounded as before. Focused tests, changed-file lint, docs, module checks, and the iOS XCFramework build pass.
| PascalCase rule. | ||
| PascalCase rule. `Receive` additionally accepts `TimeoutSeconds`: zero or an | ||
| omitted field selects a 20-second default, and positive values are capped at | ||
| five minutes. A deadline error does not prove that the receive session was not |
There was a problem hiding this comment.
🟡 F3 (Minor) — Doc says over-cap TimeoutSeconds is clamped, not rejected · docs/wavewalletdk_mobile.md:57
decodeReceiveRequest returns an error when TimeoutSeconds exceeds 300 (sdk/wavewalletdk/mobile/wallet.go:67), but this line tells host authors that positive values are "capped at five minutes", which reads as clamping — a host that sends 600 gets a failed Receive, not a 300-second one.
| five minutes. A deadline error does not prove that the receive session was not | |
| omitted field selects a 20-second default, and values are rejected above |
There was a problem hiding this comment.
Fixed in 5183f572. The binding documentation now says values above five minutes are rejected, matching decodeReceiveRequest and its negative/excessive/overflow tests.
| defer cancel() | ||
|
|
||
| res, err := client.Receive(ctx, req) | ||
| if err != nil { |
There was a problem hiding this comment.
🟡 F4 (Minor) — Timed-out Receive is indistinguishable from other errors · sdk/wavewalletdk/mobile/wallet.go:198
The deadline error is returned verbatim, so a host cannot separate "timed out, outcome uncertain, reconcile Activity first" from "rejected, no session created" without matching on an error string that crosses gomobile as free text; whether the SDK already tags deadline errors is not visible here, since sdk/wavewalletdk is not in the loaded context. Wrapping the parentCtx.Err() == nil && ctx.Err() == context.DeadlineExceeded case in a stable, documented message would make the PR's stated safety rule mechanically checkable by the host rather than prose-only.
There was a problem hiding this comment.
Fixed in 5183f572. A binding-owned deadline now returns the stable receive timed out; outcome uncertain; reconcile Activity before retrying prefix while wrapping the original cause. Parent lifecycle cancellation remains unchanged. Tests cover both branches, and the ABI documentation names the prefix.
| } | ||
|
|
||
| creditCtx, cancel := context.WithTimeout( | ||
| ctx, s.creditReadTimeout, |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F5 (Minor) — Zero-value creditReadTimeout disables credit enrichment · swapwallet/service.go:620
creditReadTimeout is populated only in newService, so a Service constructed any other way gets 0, and context.WithTimeout(ctx, 0) yields an already-expired context — every balance read then silently omits the credit fields with no error surfaced. The package's other wallet-level knobs resolve through Deps (resolveMaxListLimit, resolveLog); a resolveCreditReadTimeout() falling back to defaultCreditReadTimeout matches that convention and fails safe.
There was a problem hiding this comment.
Not worth changing. The production path is swapwallet.Register → newService, which always sets creditReadTimeout; all package service fixtures also use newService. The dependencies are private, and a zero-value Service has nil dependencies and cannot serve a balance request. There is no production path to an otherwise initialized service with a zero timeout, so a fallback would defend an unreachable state.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
|
/gateway dismiss F1 |
|
🚫 Dismissed F1 (minor) by @bhandras — no reason given Open findings on this PR: 🟡 F2 (minor) · 🟡 F3 (minor) · 🟡 F4 (minor) · 🟡 F5 (minor) |
|
/gateway dismiss F5 |
|
🚫 Dismissed F5 (minor) by @bhandras — no reason given Open findings on this PR: 🟡 F2 (minor) · 🟡 F3 (minor) · 🟡 F4 (minor) |
|
Gateway re-review context for head
Verification on the new head:
Please verify the addressed findings and review only the new delta for concrete, reachable regressions. Do not re-raise dismissed speculative states without a production path. Classify any new item as blocker, follow-up, or not worth changing, including fix risk. |
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 1 finding
🔴 0 Blocker · 🟠 0 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
All three actionable prior findings are addressed at 5183f572. readContext now bounds every repeatable read the doc claims it does — GetInfo, Balance, List, ExitStatus, ExitSummary, GetExitPlan, Status, and the three scalar helpers in convenience.go — so no read verb still inherits the daemon lifetime. The ABI doc's TimeoutSeconds wording now matches decodeReceiveRequest's reject-don't-clamp behavior, and binding-owned Receive deadlines carry the stable receiveTimeoutErrorPrefix while preserving the wrapped cause.
One gap remains in the new receiveError contract: it marks the deadline path as uncertain but deliberately leaves the lifecycle-cancellation path unmarked, even though a Stop racing an in-flight Receive is uncertain for the same reason. That is a minor documentation/contract inconsistency, not a defect in the deadline logic itself.
The btcwallet bump, credit-enrichment deadline, and their tests are unchanged from the prior head.
Status of prior findings
- F2 addressed:
GetInfo(sdk/wavewalletdk/mobile/wallet.go:105),ExitStatus(:318),ExitSummary(:342), andGetExitPlan(:366) now derive fromreadContext, as doConfirmedBalanceSat,PendingInboundSat, andWalletReadyinconvenience.go. No read verb passes the daemon-lifetime context through any more. Resolved. - F3 addressed:
docs/wavewalletdk_mobile.md:55now reads "values above five minutes are rejected", matchingdecodeReceiveRequest's error return and itsexcessive/overflowtest cases. Resolved. - F4 addressed:
receiveTimeoutErrorPrefixis a documented constant,receiveErrorwraps with%wso the cause surviveserrors.Is, and both branches are pinned byTestReceiveErrorMarksUncertainTimeout/TestReceiveErrorPreservesLifecycleCancellation. The prefix is named in the ABI doc, so a host can match it without depending on transport error text. Resolved.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| // owned by this binding as an uncertain outcome. A parent cancellation means | ||
| // the daemon is stopping, so it keeps the lifecycle error unchanged. | ||
| func receiveError(parentCtx, callCtx context.Context, err error) error { | ||
| if err == nil || parentCtx.Err() != nil || |
There was a problem hiding this comment.
🟡 F6 (Minor) — Receive cancelled by Stop returns an unmarked uncertain outcome · sdk/wavewalletdk/mobile/wallet.go:50
receiveError returns the raw error whenever parentCtx.Err() != nil, but a Stop that cancels an in-flight Receive leaves exactly the same uncertain outcome as a deadline — the daemon may already have created the receive session — so a host following the new background/resume guidance (Stop, then Start and Receive again on resume) sees a plain lifecycle error rather than receiveTimeoutErrorPrefix and can create a second invoice for one user intent. Either extend the uncertain-outcome marking to lifecycle cancellation of Receive, or state in the ABI doc that a Receive error observed across Stop carries the same reconcile-before-retry obligation.
There was a problem hiding this comment.
Fixed in a41cc55f.
Receive now treats cancellation from either its request deadline or wallet Stop as an uncertain outcome. Both paths return the stable receive outcome uncertain; reconcile Activity before retrying prefix while preserving the original cancellation cause. Tests cover deadline and lifecycle cancellation separately.
Apply the binding-owned read deadline to every repeatable mobile read. Return a stable uncertain-outcome prefix when Receive reaches its own deadline. Document that receive timeouts over five minutes are rejected.
5183f57 to
038df79
Compare
Mark Receive cancellation by either its request deadline or wallet Stop with the same stable uncertain-outcome prefix. This tells mobile hosts to reconcile Activity before retrying after foreground timeouts and lifecycle cancellation.
|
Gateway re-review context for exact head
Please re-check only concrete, reachable defects introduced or materially worsened by this PR. Treat pre-existing debt and speculative future API drift as follow-ups or not worth changing. Stop when the intended invariants are proven and no material defect remains. |
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 1 finding
🔴 0 Blocker · 🟠 0 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
F6 is fixed. receiveError now keys off the derived call context (sdk/wavewalletdk/mobile/wallet.go:50), so a Receive cancelled by its own deadline or by Stop cancelling the daemon-lifetime parent both return the stable receive outcome uncertain; reconcile Activity before retrying prefix with the cause preserved under errors.Is; both branches are pinned by tests, and the prefix is named in the binding ABI doc. F2, F3, and F4 remain fixed at this head — the repeatable-read set in docs/wavewalletdk_mobile.md matches the call sites that actually route through readContext, including the scalar helpers and GetExitPlan.
The remaining item is on the daemon side and is not a mobile-binding concern: the new 2-second credit bound in fetchBalance turns a slow-but-healthy credit endpoint into a silent omission, with no log line and no response signal separating it from "zero credits". That is a follow-up, not a blocker — the local satoshi balance, which is the invariant this PR set out to protect, is correct either way.
Unbounded verbs that remain (Deposit, PrepareSend, SendPrepared, Exit, SweepWallet) are pre-existing and out of the stated scope; I am not raising them.
Status of prior findings
- F2 addressed:
readContextnow boundsGetInfo,ExitStatus,ExitSummary, andGetExitPlanalongsideBalance/List/Statusand the three scalar helpers inconvenience.go. Resolved. - F3 addressed:
docs/wavewalletdk_mobile.md:57now says values above five minutes are rejected, matchingdecodeReceiveRequest's error return rather than implying a clamp. Resolved. - F4 addressed:
receiveUncertainErrorPrefix(sdk/wavewalletdk/mobile/wallet.go:31) gives the host a stable, documented marker for the uncertain-outcome case, and%wkeeps the cause inspectable. Resolved. - F6 addressed: Fixed at
a41cc55f.receiveErrortestscallCtx.Err()rather than only the request deadline, so aStopthat cancels an in-flightReceivecarries the same reconcile-before-retry marker as a timeout;TestReceiveErrorMarksLifecycleCancellationpins the parent-cancellation branch specifically. Resolved.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| return resp, nil | ||
| } | ||
|
|
||
| creditCtx, cancel := context.WithTimeout( |
There was a problem hiding this comment.
🟡 F7 (Minor) — Credit timeout is indistinguishable from zero credits · swapwallet/service.go:619
A credit lookup that exceeds the new 2-second bound returns resp, nil with CreditAvailableSat/CreditReservedSat left at zero and nothing logged, so a slow-but-healthy credit endpoint reads identically to a wallet holding no credits — where before this change the same endpoint at 3s latency returned the real figures. Log the elided enrichment via s.deps.resolveLog() on that branch, as replayEvents does for a skipped row, so an operator can tell degradation from a genuine zero; I cannot see rpc/wavewalletrpc's proto in the provided context, so whether the response shape could also carry an explicit "credit unknown" signal is unverified here.
There was a problem hiding this comment.
Fixed in 502fa87d.
The bounded credit lookup still degrades to the authoritative local balance, but the fallback now emits Credit balance enrichment skipped with the original error. The regression test verifies both the valid local response and the operator-visible warning.
Log the error when optional credit enrichment is omitted from an otherwise valid local balance response. This lets operators distinguish a degraded remote read from a genuine zero-credit wallet without failing the local refresh.
|
Gateway re-review context for exact head
Please re-check only concrete, reachable defects introduced or materially worsened by this PR. Treat pre-existing debt and speculative future API drift as follow-ups or not worth changing. Stop when the intended invariants are proven and no material defect remains. |
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review
No findings.
Status of prior findings
- F2 addressed: Unchanged at this head:
readContextboundsGetInfo,Balance,List,ExitStatus,ExitSummary,GetExitPlan,Status, and the scalar helpers inconvenience.go. Resolved. - F3 addressed:
docs/wavewalletdk_mobile.md:57states values above five minutes are rejected, matchingdecodeReceiveRequest's error return atsdk/wavewalletdk/mobile/wallet.go:79. Resolved. - F4 addressed:
receiveErrorreturnsreceiveUncertainErrorPrefixwrapping the original cause whenever the call context is done, and the prefix is documented as stable binding ABI. Resolved. - F6 addressed: Lifecycle cancellation takes the same branch as a binding-owned deadline —
receiveErrorkeys oncallCtx.Err() != nil, which is set by bothStopand the timeout — so aReceiveinterrupted byStopcarries the same reconcile-before-retry marker.TestReceiveErrorMarksLifecycleCancellationpins it. Resolved. - F7 addressed: Fixed at
swapwallet/service.go:625: the enrichment-failure branch logsCredit balance enrichment skippedwith the error vias.deps.resolveLog().WarnS, using the parentctxrather than the expiredcreditCtx, before returning the authoritative local balance. Resolved.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
Release the Postgres fixture semaphore after Docker startup and store migrations finish instead of retaining it until test cleanup. This preserves bounded initialization while preventing parallel tests from exhausting every slot and blocking the remaining test barrier.
|
Gateway re-review context for exact head
Please re-check the fixture change for a concrete resource leak, unbounded initialization path, or cleanup regression. Stop when the initialization bound and release invariant are proven and no material defect remains. |
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 1 finding
🔴 0 Blocker · 🟠 0 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
The prior findings are all closed: every repeatable mobile read now derives from readContext (F2), the ABI doc says over-cap TimeoutSeconds is rejected rather than clamped (F3), receiveError marks both deadline and lifecycle cancellation with receiveUncertainErrorPrefix while preserving the cause via %w (F4, F6), and the credit-enrichment fallback logs Credit balance enrichment skipped with the original error (F7). F1 and F5 stay dismissed.
The only new code in this head is the Postgres fixture-slot change. The hold-and-wait it targets is genuinely removed: all three fixture-creating helpers now release an idempotent slot at return, after NewPostgresStore (and ExecuteMigrations), so no test holds a slot while waiting on another test that needs one. TearDown remains a correct failure-path fallback, and NewTestPgFixture's existing success guard still releases on a require.NoError Goexit. The coarse caller index shows NewTestPgFixture referenced from three files — its definition plus the two updated helper files — so no call site looks to have been missed.
One consequence of the narrowed boundary is worth a decision rather than a fix.
Status of prior findings
- F2 addressed:
readContextnow wrapsGetInfo,Balance,List,ExitStatus,ExitSummary,GetExitPlan,Status(sdk/wavewalletdk/mobile/wallet.go) and the three scalar helpers inconvenience.go. Resolved. - F3 addressed:
docs/wavewalletdk_mobile.mdnow reads "values above five minutes are rejected", matchingdecodeReceiveRequest's error return. Resolved. - F4 addressed:
receiveErroratsdk/wavewalletdk/mobile/wallet.go:50prefixes cancellation with the stablereceiveUncertainErrorPrefixand wraps the cause with%w; the prefix is documented as binding ABI and covered byTestReceiveErrorMarksUncertainTimeout. Resolved. - F6 addressed: The guard is
callCtx.Err() != nil, which is also true when the daemon-lifetime parent is cancelled, so aStopracing an in-flightReceivecarries the same reconcile-before-retry marker.TestReceiveErrorMarksLifecycleCancellationpins that branch specifically. Resolved. - F7 addressed:
swapwallet/service.gonow emitsWarnS(ctx, "Credit balance enrichment skipped", err)before returning the local snapshot, andTestServiceBalanceDegradesWhenCreditsAreUnavailableasserts both the degraded response and the warning text including the underlying error. Resolved.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| // runs. The db package marks most tests parallel, and unbounded docker startup | ||
| // can starve CI runners enough that stores observe partially initialized | ||
| // schemas. | ||
| // acquireTestPgFixtureSlot bounds concurrent Postgres fixture initialization. |
There was a problem hiding this comment.
🚫 Dismissed by @bhandras
🟡 F8 (Minor) — Live Postgres containers are no longer bounded · db/postgres_fixture.go:127
The semaphore previously bounded live fixtures at four, because the slot was held until TearDown; it now bounds only the initialization window, so the number of simultaneously running Postgres containers is capped by -parallel (GOMAXPROCS by default) rather than by testPgFixtureParallelism. On an 8- or 16-CPU runner that is 2–4× the containers PR #772 was sized for, and the starvation it was added to prevent — "stores observe partially initialized schemas" — is memory/IO pressure from concurrently running containers as much as from concurrent startup. The rewritten comment and the db/AGENTS.md invariant both describe the new, narrower guarantee accurately, so this is a resource-envelope decision rather than a mismatch between code and docs; the local timings in the PR description show the dev host absorbs it, not that a constrained CI runner does. If you want the old ceiling back without reintroducing hold-and-wait, a second, larger semaphore acquired for the container's lifetime (sized independently of the init bound) keeps the two concerns separate. Accepting the change as-is is also defensible — the CI signal on this head is what settles it.
|
F8 decision for exact head
|
|
/gateway dismiss F8 |
|
🚫 Dismissed F8 (minor) by @bhandras — no reason given All findings on this PR are addressed or dismissed. |
|
/gateway re-review |
|
No new commits since the last gateway review (HEAD is still |
bhandras
added
the
backport-v0.1.x-branch
|
Successfully created backport PR for |
…ranch [v0.1.x-branch] Backport #1131: mobile: prevent wallet stalls on external I/O
2 participants
The problem
Mobile hosts call the embedded Wavelength wallet through gomobile, which cannot carry a caller
context.Context. If iOS or Android suspends the process while an external connection is using an old network path, wallet reads and Lightning invoice creation can wait for the embedded daemon's full lifetime instead of returning control to the app.Wallet recovery can cause a second stall. btcwallet v0.18.0 keeps its database writer open while the chain backend filters recovery blocks. The lightweight backend may perform remote I/O during that call, so unrelated wallet operations can wait behind a slow recovery request.
Read calls are safe to repeat. Invoice creation is different: a deadline or lifecycle cancellation may race durable receive-session creation, so cancellation does not prove that no invoice exists.
The fix
GetInfo,Balance,List,ExitStatus,ExitSummary,GetExitPlan,Status, and the scalar convenience helpers.Receivean optionalTimeoutSecondsfield. Older hosts receive a 20-second default. Values above five minutes are rejected.Receiveas an uncertain outcome with a stable reconcile-before-retry error prefix.Safety rule
The host may retry timed-out reads. It must not blindly retry a canceled
Receive; it must first reconcile the authoritative Activity view and recover any matching invoice.Recovery keeps address-index operations serialized and commits each completed batch atomically, while unrelated database writers remain available during remote filter I/O.
What does not change
TimeoutSeconds.Tests
make lint-changed-localmake tidy-module-checkmake unit-swapruntimego test -tags='mobile wavewalletrpc swapruntime' ./sdk/wavewalletdk/mobilego test -tags='wavewalletrpc swapruntime' ./swapwallet -run '^TestServiceBalance' -count=1go test -tags=test_postgres ./waved -count=1 -timeout=30mgo test -tags=test_postgres ./db -count=1 -timeout=30mmake mobile-iosThe tests cover default and explicit mobile deadlines, invalid deadline values, repeatable read deadlines, uncertain
Receivecancellation, healthy credit enrichment, credit timeout fallback and logging, and the Postgres fixture-slot lifecycle. The full wallet-runtime suite and iOS xcframework build pass against the updated btcwallet dependency.Exact-head CI passes all 20 required checks, including Postgres unit and system tests, race tests, SQLite tests, lint, static checks, and every cross-build.
Compatibility and rollout
TimeoutSecondsis additive at the mobile JSON boundary. Older Wavelength bindings ignore the extra field. Updated bindings apply the bounded default even when an older host omits it.The btcwallet fix is pinned to
v0.18.1-0.20260826052527-33c252f3b4d6, the Go pseudo-version for merged commit33c252f3b4d6. No btcwallet v0.18.1 tag exists yet.After this PR merges, publish a Wavelength release containing both changes. wavelength-mobile#6 can then consume and validate the release xcframework.