Skip to content

fix: retry the node start when returning to the app - #848

Open
ovitrif wants to merge 5 commits into
masterfrom
fix/retry-node-start-on-foreground
Open

ovitrif wants to merge 5 commits into
masterfrom
fix/retry-node-start-on-foreground

Conversation

@ovitrif

@ovitrif ovitrif commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Refs:

This PR retries a failed node start when the app returns to the foreground. It is a partial fix for #777 (retry on foreground only); backoff and an actionable connection state stay open there, so the issue stays open.

Description

  • Retries the node start when the app returns to the foreground from the background while the node is in the error-starting state, a wallet exists and the network is connected, so a start that failed on an Electrum timeout recovers without a relaunch. Before, a start was retried only when the network path changed, on pull-to-refresh of the node info screen or from the setup screen, none of which fires when the network path stays connected but degraded. A brief inactive phase (Control Center, notification shade, Face ID prompt) does not retry.
  • Keeps recovery mode from starting the node through this retry: the retry is not scheduled while the Recovery screen is shown, is re-checked when it runs, and if Recovery opens while the start is in flight, the node that restart started is stopped once the start completes. Not guaranteed: the node can run until an in-flight start finishes (up to the Electrum timeout), the start's follow-up work (payment sync, order watching) runs before the stop, and a failed stop is only logged.
  • Extracts the network-restored node restart into one helper that both the network handler and the foreground handler call, so the two entry points cannot drift. As a result the network-restored restart also skips recovery mode, which is the case [Bug]: Recovery mode can still start the node after a network reconnect #603 describes.
  • Adds a changelog fragment.

Out of Scope

Design

N/A — no UI changes.

Preview

N/A — no user-visible changes beyond the recovery after a failed start; no new screens.

QA Notes

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

  • Launch with Electrum unreachable so the node start times out → restore Electrum, keep the network connected → background and foreground the app → the node starts and the wallet leaves "Connection issues" without a relaunch — a degraded Electrum during node start not in Capabilities
  • regression: node running → background and foreground the app → no second start is logged — a degraded Electrum during node start not in Capabilities
  • Node in error-starting state → Recovery quick action from the app icon → background and foreground the app → the node is not started and Recovery stays shown; with Recovery opened while a retry is in flight, the node is stopped once the start completes — a degraded Electrum during node start not in Capabilities
  • Node in error-starting state → pull down Control Center or the notification shade and close it → no retry is logged and no error haptic fires — a degraded Electrum during node start not in Capabilities

Automated Checks

  • added NodeStartRetryTests.swift — retries only from the error-starting state, with a wallet and a connection, only after a return from the background, never while the Recovery screen is shown, and stops a node that started while Recovery opened
  • ran a simulator check: a node start that failed on an unreachable Electrum endpoint was retried once when the app returned from the background and started once the endpoint was reachable again; the retry also fired, and failed again, while the endpoint was still unreachable. A brief inactive phase and the Recovery race were not driven on the simulator; the gating and stop decisions are covered by unit tests only

@ovitrif ovitrif self-assigned this Sep 30, 2026
@ovitrif ovitrif added this to the 2.7.0 milestone Sep 30, 2026
@ovitrif ovitrif changed the title fix: retry a failed node start when the app returns to the foreground fix: retry the node start when returning to the app Sep 30, 2026
@ovitrif

ovitrif commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed d5dd07d: renamed the changelog fragment to this PR's number, as pr.md asks once the PR exists. No code change.

The scene-phase active handler now restarts the node when it is in errorStarting with a wallet and a connection, sharing the restart used when the network is restored.
@ovitrif
ovitrif force-pushed the fix/retry-node-start-on-foreground branch from d5dd07d to da351df Compare October 1, 2026 19:36
@ovitrif
ovitrif marked this pull request as ready for review October 1, 2026 21:57
@ovitrif
ovitrif requested review from a team, coreyphillips and pwltr and removed request for a team October 1, 2026 21:58
@ovitrif

ovitrif commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on master (0 behind) and pushed da351df: the foreground retry now also stays off while the Recovery screen is shown, with a unit test that fails without the guard. Description updated with the Recovery guard and the simulator check.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review 🔄 Running since 2026-10-01T21:58:09.149296Z da351df Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Adds automatic node restart when app returns to foreground.

The PR should not merge until the Recovery quick-action path prevents a foreground retry regardless of callback order.

Findings

  1. P1 Recovery can start the node ▶

Summary

The PR adds a foreground retry for a connected wallet whose node start has failed, reuses the network-restored start helper, and adds predicate tests and a changelog fragment.

  • The Recovery exclusion depends on the quick-action notification being handled before the scene-phase callback; that ordering is not enforced.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[App activated via Recovery quick action] --> B{Which callback runs first?}
  B -->|Quick action| C[Set showRecoveryScreen]
  C --> D[Foreground check skips retry]
  B -->|Scene phase| E[Foreground check sees Recovery as false]
  E --> F[Schedule node start]
  F --> G[Quick action shows Recovery]
  G --> H[Node start proceeds without another Recovery check]
Loading

Reviews (1) · Last reviewed commit: "fix: skip the foreground node start retr..."

Comment thread Bitkit/AppScene.swift Outdated
@ovitrif

ovitrif commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 47e0db6: the Recovery check also runs inside the restart helper when the restart executes (answers the review thread about the quick-action ordering). Because the network-restored restart shares the helper, it now also skips in recovery mode, which is the case #603 describes. Description updated; the guard test and the other NodeStartRetryTests pass on the new head.

@coreyphillips

Copy link
Copy Markdown
Contributor

Two independent reviews.

needs changing before merge

  • Recovery can still race with the node restart (Bitkit/AppScene.swift:1573). restartNode can pass its one-time recovery guard, suspend in startWallet, and continue starting the node after the Recovery quick action sets showRecoveryScreen. I confirmed the control flow: the guard runs here, WalletViewModel.start() changes the state to .starting before awaiting setup, and handleQuickAction only changes the screen flag without cancelling or stopping the start. Recovery can therefore appear while the node starts, which breaks the recovery guarantee this change introduces. The new tests only cover Recovery being visible before the retry is scheduled.

worth doing, does not block

  • Recovery check inside the Task narrows the quick-action race but does not close it (Bitkit/AppScene.swift:1570). The guard in restartNode only moves the showRecoveryScreen check one main-actor hop later. If the Recovery quick action is posted after that Task has passed the guard, startWallet() still runs and the node starts underneath the Recovery screen. SceneDelegate.windowScene(_:performActionFor:) posts .quickActionSelected independently of SwiftUI's scenePhase change (Bitkit/SceneDelegate.swift:60-81), and the two are not ordered relative to each other. I did not reproduce this on a device, so it may be rare. The PR description says the check "also runs when the restart executes", which is accurate but reads as a full fix. If recovery mode must never coexist with a running node, the start needs a check after the start completes (or recovery entry should stop the node). Otherwise the description should say this is best effort.
  • Retry runs on every inactive-to-active transition, with an error haptic per failure (Bitkit/AppScene.swift:1071). handleScenePhaseChange treats every .active as a return to the app. Pulling down Control Center or the notification shade, or a Face ID prompt for PIN unlock, moves the scene .inactive and then back to .active. While the node is in errorStarting and Electrum is still unreachable (the PR's own simulator check confirms the retry fires and fails in that state), each of these triggers a full startWallet() attempt with its Electrum timeout. Each failure ends in Haptics.notify(.error) in startWallet's catch (Bitkit/AppScene.swift:813). wallet.start() sets .starting before its first await, so concurrent starts are prevented. The cost is repeated error buzzes and log noise, not double starts. Gating on a prior .background phase, or suppressing the haptic for background retries, would avoid it.

@ovitrif

ovitrif commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Recovery can still race with the node restart (Bitkit/AppScene.swift:1573). restartNode can pass its one-time recovery guard, suspend in startWallet, and continue starting the node after the Recovery quick action sets showRecoveryScreen.

@coreyphillips Agreed, and pushed dee4b5f. After the start completes, restartNode now stops the node when Recovery is shown (shouldStopNodeStartedUnderRecovery: node running and Recovery shown). The restart only runs from stopped, initializing or error-starting, so a node running at that point was started by the restart. The unit test fails without it. What is guaranteed: a retry never leaves a node running under Recovery once its start completes. What is not: the node can run until an in-flight start finishes (up to the Electrum timeout), the start's follow-up work (payment sync, order watching) runs before the stop, and a failed stop is only logged. The description says so now and no longer reads as a full fix.

Retry runs on every inactive-to-active transition, with an error haptic per failure (Bitkit/AppScene.swift:1071).

@coreyphillips Gated on a prior background phase: the app records .background and only the next .active after it can retry, so Control Center, the notification shade and Face ID no longer trigger a start or its error haptic. The haptic stays for a retry that follows a real return to the app. NodeStartRetryTests covers the gate; I checked the background return on a simulator, but did not drive a Control Center or Face ID blip there.

@pwltr pwltr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Verified that after startup failed with Electrum paused, unpausing it and backgrounding/reopening Bitkit started the node without a relaunch. No blocking findings. Automatic retry with backoff remains tracked in #777.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants