-
Notifications
You must be signed in to change notification settings - Fork 331
fix(desktop): recover Pi after offline startup (#3382) #3394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
qsoyq
wants to merge
19
commits into
makecindy:main
Choose a base branch
from
qsoyq:fix-issue-3382-pi-network-recovery
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+832
−52
Open
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
3d24b5c
fix(desktop): recover Pi after offline startup (#3382)
qsoyq 2ddab1f
fix(device-link): sync recovered Pi roster (#3394)
qsoyq cf8c5b3
fix(mobile): refresh recovered Pi roster (#3394)
qsoyq b7c46e7
fix(pi): preserve memory setting after recovery (#3394)
qsoyq 51703a7
fix(pi): stop retries for permanent prepare errors (#3394)
qsoyq 75a6d0d
fix(mobile): subscribe to Pi roster updates (#3394)
qsoyq b7ed4d8
fix(pi): stop retries after permanent recovery failure (#3394)
qsoyq 73f674a
fix(agents): refresh capabilities after Pi recovery (#3394)
qsoyq c4de0d0
fix(mobile): retain roster subscription across reconnects (#3394)
qsoyq 6f491bc
fix(mobile): cancel stale roster resubscriptions (#3394)
qsoyq 72850c5
Merge origin/main into fix-issue-3382-pi-network-recovery
qsoyq 32884c2
fix(mobile): refresh Pi capabilities after roster changes
qsoyq 2f26745
Merge remote-tracking branch 'origin/main' into qsoyq/fix-issue-3382-…
qsoyq 7465b5d
test(pi): tolerate slow Windows PowerShell cleanup
qsoyq 2fad4b0
Merge remote-tracking branch 'origin/main' into qsoyq/fix-issue-3382-…
qsoyq b8c598d
fix(desktop): ignore stale agent roster responses (#3394)
qsoyq be6464d
fix(device-link): refresh agent rosters after reconnect (#3394)
qsoyq 14662e8
fix(desktop): deduplicate agent roster refreshes (#3394)
qsoyq 1321b7c
fix(desktop): centralize agent roster subscriptions (#3394)
qsoyq File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
107 changes: 107 additions & 0 deletions
107
apps/desktop/src/main/agent-binaries/__tests__/pi-runtime-recovery.test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { createPiRuntimeRecovery } from '../pi-runtime-recovery.js'; | ||
|
|
||
| describe('Pi runtime recovery', () => { | ||
| it('retries after the network returns and registers Pi once', async () => { | ||
| let online = false; | ||
| let prepareCalls = 0; | ||
| let registered = false; | ||
| const onRegistered = vi.fn(); | ||
| const recovery = createPiRuntimeRecovery({ | ||
| isOnline: () => online, | ||
| prepare: async () => { | ||
| prepareCalls += 1; | ||
| return { ready: true, path: '/tmp/pi' }; | ||
| }, | ||
| register: () => { | ||
| if (registered) return false; | ||
| registered = true; | ||
| return true; | ||
| }, | ||
| onRegistered, | ||
| retryDelayMs: 60_000, | ||
| setTimeout: (() => 0) as unknown as typeof setTimeout, | ||
| clearTimeout: (() => undefined) as unknown as typeof clearTimeout, | ||
| }); | ||
|
|
||
| recovery.markUnavailable('manifest_failed'); | ||
| expect(await recovery.retryNow('offline')).toBe(false); | ||
| online = true; | ||
| expect(await recovery.retryNow('online')).toBe(true); | ||
| expect(await recovery.retryNow('duplicate')).toBe(false); | ||
| expect(prepareCalls).toBe(1); | ||
| expect(onRegistered).toHaveBeenCalledOnce(); | ||
| expect(recovery.isDisabled()).toBe(false); | ||
| recovery.dispose(); | ||
| }); | ||
|
|
||
| it('deduplicates concurrent recovery and keeps retryable failure disabled', async () => { | ||
| let resolvePrepare!: (value: { ready: boolean; path?: string; error?: string }) => void; | ||
| const prepare = vi.fn( | ||
| () => new Promise<{ ready: boolean; path?: string; error?: string }>((resolve) => { | ||
| resolvePrepare = resolve; | ||
| }), | ||
| ); | ||
| const recovery = createPiRuntimeRecovery({ | ||
| isOnline: () => true, | ||
| prepare, | ||
| register: () => true, | ||
| onRegistered: vi.fn(), | ||
| retryDelayMs: 60_000, | ||
| setTimeout: (() => 0) as unknown as typeof setTimeout, | ||
| clearTimeout: (() => undefined) as unknown as typeof clearTimeout, | ||
| }); | ||
|
|
||
| recovery.markUnavailable('manifest_failed'); | ||
| const first = recovery.retryNow(); | ||
| const second = recovery.retryNow(); | ||
| expect(first).toBe(second); | ||
| expect(prepare).toHaveBeenCalledOnce(); | ||
| resolvePrepare({ ready: false, error: 'still_offline' }); | ||
| expect(await first).toBe(false); | ||
| expect(recovery.isDisabled()).toBe(true); | ||
| recovery.dispose(); | ||
| }); | ||
|
|
||
| it('does not schedule retries for permanent prepare errors', async () => { | ||
| const prepare = vi.fn(async () => ({ ready: true, path: '/tmp/pi' })); | ||
| const schedule = vi.fn(() => 0); | ||
| const recovery = createPiRuntimeRecovery({ | ||
| isOnline: () => true, | ||
| prepare, | ||
| register: () => true, | ||
| onRegistered: vi.fn(), | ||
| setTimeout: schedule as unknown as typeof setTimeout, | ||
| clearTimeout: (() => undefined) as unknown as typeof clearTimeout, | ||
| }); | ||
|
|
||
| recovery.markUnavailable('asset_missing'); | ||
| expect(schedule).not.toHaveBeenCalled(); | ||
| expect(await recovery.retryNow('permanent')).toBe(false); | ||
| expect(prepare).not.toHaveBeenCalled(); | ||
| recovery.dispose(); | ||
| }); | ||
|
|
||
| it('stops an existing retry loop when a later prepare becomes permanent', async () => { | ||
| const prepare = vi.fn(async () => ({ ready: false, error: 'asset_missing' })); | ||
| const schedule = vi.fn(() => 0); | ||
| const cancel = vi.fn(); | ||
| const recovery = createPiRuntimeRecovery({ | ||
| isOnline: () => true, | ||
| prepare, | ||
| register: () => true, | ||
| onRegistered: vi.fn(), | ||
| setTimeout: schedule as unknown as typeof setTimeout, | ||
| clearTimeout: cancel as unknown as typeof clearTimeout, | ||
| }); | ||
|
|
||
| recovery.markUnavailable('manifest_failed'); | ||
| expect(schedule).toHaveBeenCalledOnce(); | ||
| expect(await recovery.retryNow('permanent-after-transient')).toBe(false); | ||
| expect(cancel).toHaveBeenCalledOnce(); | ||
| expect(schedule).toHaveBeenCalledOnce(); | ||
| expect(recovery.isDisabled()).toBe(true); | ||
| recovery.dispose(); | ||
| }); | ||
| }); |
145 changes: 145 additions & 0 deletions
145
apps/desktop/src/main/agent-binaries/pi-runtime-recovery.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| import type { PrepareResult } from './types.js'; | ||
|
|
||
| /** Retry delay for an optional Pi runtime that missed the startup network. */ | ||
| export const PI_RUNTIME_RECOVERY_RETRY_MS = 30_000; | ||
|
|
||
| /** Only errors that may change when connectivity returns are worth retrying. */ | ||
| export function isRetryablePiPrepareError(error?: string): boolean { | ||
| return error === 'manifest_failed' | ||
| || error === 'NETWORK' | ||
| || error === 'HTTP_5XX' | ||
| || error === 'ABORTED'; | ||
| } | ||
|
|
||
| export interface PiRuntimeRecoveryOptions { | ||
| isOnline: () => boolean; | ||
| prepare: () => Promise<PrepareResult>; | ||
| register: () => boolean; | ||
| onRegistered: () => void; | ||
| logWarn?: (message: string, error?: unknown) => void; | ||
| retryDelayMs?: number; | ||
| setTimeout?: typeof globalThis.setTimeout; | ||
| clearTimeout?: typeof globalThis.clearTimeout; | ||
| } | ||
|
|
||
| export interface PiRuntimeRecovery { | ||
| /** Mark the startup prepare as unavailable and begin background recovery. */ | ||
| markUnavailable(error?: string): void; | ||
| /** Try recovery immediately; returns true only when Pi was registered. */ | ||
| retryNow(reason?: string): Promise<boolean>; | ||
| /** Stop future retries during app shutdown or test cleanup. */ | ||
| dispose(): void; | ||
| isDisabled(): boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Owns the small recovery state machine for an optional Pi runtime. | ||
| * | ||
| * The CDN policy remains unchanged: every retry calls the managed prepare path, | ||
| * so a local runtime is accepted only after the manifest and verification rules | ||
| * have succeeded. Concurrent focus/timer signals share one prepare promise. | ||
| */ | ||
| export function createPiRuntimeRecovery(options: PiRuntimeRecoveryOptions): PiRuntimeRecovery { | ||
| const retryDelayMs = options.retryDelayMs ?? PI_RUNTIME_RECOVERY_RETRY_MS; | ||
| const schedule = options.setTimeout ?? globalThis.setTimeout; | ||
| const cancel = options.clearTimeout ?? globalThis.clearTimeout; | ||
| let disabled = false; | ||
| let disposed = false; | ||
| let retryable = false; | ||
| let retryTimer: ReturnType<typeof globalThis.setTimeout> | null = null; | ||
| let inFlight: Promise<boolean> | null = null; | ||
|
|
||
| const logWarn = (message: string, error?: unknown): void => { | ||
| options.logWarn?.(message, error); | ||
| }; | ||
|
|
||
| const scheduleRetry = (): void => { | ||
| if (disposed || !disabled || retryTimer !== null) return; | ||
| retryTimer = schedule(() => { | ||
| retryTimer = null; | ||
| void recovery.retryNow('timer'); | ||
| }, retryDelayMs); | ||
| const unref = (retryTimer as unknown as { unref?: () => void }).unref; | ||
| unref?.call(retryTimer); | ||
| }; | ||
|
|
||
| const recovery: PiRuntimeRecovery = { | ||
| markUnavailable(error) { | ||
| disabled = true; | ||
| retryable = isRetryablePiPrepareError(error); | ||
| if (!retryable && retryTimer !== null) { | ||
| cancel(retryTimer); | ||
| retryTimer = null; | ||
| } | ||
| if (error) { | ||
| logWarn( | ||
| retryable | ||
| ? 'Pi runtime unavailable; scheduling recovery' | ||
| : 'Pi runtime unavailable; recovery not scheduled for permanent prepare error', | ||
| error, | ||
| ); | ||
| } | ||
| if (retryable) scheduleRetry(); | ||
| }, | ||
|
|
||
| retryNow(reason = 'manual') { | ||
| if (disposed || !disabled || !retryable) return Promise.resolve(false); | ||
| if (inFlight) return inFlight; | ||
|
|
||
| let online = false; | ||
| try { | ||
| online = options.isOnline(); | ||
| } catch (error) { | ||
| logWarn('Pi runtime network state probe failed', error); | ||
| } | ||
| if (!online) { | ||
| scheduleRetry(); | ||
| return Promise.resolve(false); | ||
| } | ||
|
|
||
| const attempt = (async (): Promise<boolean> => { | ||
| try { | ||
| const result = await options.prepare(); | ||
| if (!result.ready || !result.path) { | ||
| logWarn(`Pi runtime recovery prepare failed (${reason})`, result.error); | ||
| recovery.markUnavailable(result.error); | ||
| return false; | ||
| } | ||
| if (!options.register()) { | ||
| scheduleRetry(); | ||
| return false; | ||
|
qsoyq marked this conversation as resolved.
|
||
| } | ||
| disabled = false; | ||
| retryable = false; | ||
| options.onRegistered(); | ||
| return true; | ||
| } catch (error) { | ||
| logWarn(`Pi runtime recovery threw (${reason})`, error); | ||
| recovery.markUnavailable(error instanceof Error ? error.message : String(error)); | ||
| return false; | ||
| } | ||
| })(); | ||
| inFlight = attempt; | ||
| void attempt.then(() => { | ||
| if (inFlight === attempt) inFlight = null; | ||
| }, () => { | ||
| if (inFlight === attempt) inFlight = null; | ||
| }); | ||
| return attempt; | ||
| }, | ||
|
|
||
| dispose() { | ||
| disposed = true; | ||
| if (retryTimer !== null) { | ||
| cancel(retryTimer); | ||
| retryTimer = null; | ||
| } | ||
| }, | ||
|
|
||
| isDisabled() { | ||
| return disabled; | ||
| }, | ||
| }; | ||
|
|
||
| return recovery; | ||
| } | ||
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.