diff --git a/src/relay/relay-grace-branch.test.ts b/src/relay/relay-grace-branch.test.ts index 580ab7e17..e7fee5832 100644 --- a/src/relay/relay-grace-branch.test.ts +++ b/src/relay/relay-grace-branch.test.ts @@ -1,8 +1,11 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { + applyRelayGraceTimeConfiguration, decideRelayGrace, - retryDeferredShutdownAfterGraceReconfigure, - type RelayGraceDecisionInput + decideRelayGraceReconfigure, + type RelayGraceBranch, + type RelayGraceDecisionInput, + type RelayGraceReconfigureInput } from './relay-grace-branch' const EMPTY_DETACHED_STARTUP_GRACE_MS = 30_000 @@ -68,19 +71,6 @@ describe('decideRelayGrace', () => { ).toEqual({ branch: 'shutdown-deferred', timeoutMs: IDLE_RELAY_GRACE_MS }) }) - it('preserves shutdown-deferred when grace is reconfigured to zero', () => { - // Why a non-zero starting grace: configureRelayGraceTime only re-arms on an actual change, - // so the reconfiguration this models must cross a real boundary, not 0 → 0. - const current = decide({ configuredGraceMs: 10_000, retryDeferredShutdown: true }) - - expect( - decide({ - configuredGraceMs: 0, - retryDeferredShutdown: retryDeferredShutdownAfterGraceReconfigure(current.branch) - }) - ).toEqual({ branch: 'shutdown-deferred', timeoutMs: IDLE_RELAY_GRACE_MS }) - }) - it('prefers startup-empty-detached over the idle cap', () => { expect( decide({ @@ -115,3 +105,150 @@ describe('decideRelayGrace', () => { }) }) }) + +function reconfigure(overrides: Partial = {}) { + return decideRelayGraceReconfigure({ + previousConfiguredGraceMs: 10_000, + nextConfiguredGraceMs: 86_400_000, + graceTimerArmed: true, + shutdownInFlight: false, + currentBranch: 'configured', + ...overrides + }) +} + +describe('decideRelayGraceReconfigure', () => { + it('re-arms a running window so a raised grace takes effect at the new deadline', () => { + // Why: the reported bug's call site. startGrace samples the grace at arm time, so without this + // re-arm a raise landing mid-window still fires at the old deadline. + expect(reconfigure()).toEqual({ rearm: true, retryDeferredShutdown: false }) + }) + + it('preserves shutdown-deferred across the re-arm', () => { + const rearmed = reconfigure({ nextConfiguredGraceMs: 0, currentBranch: 'shutdown-deferred' }) + + expect(rearmed).toEqual({ rearm: true, retryDeferredShutdown: true }) + expect( + decide({ + configuredGraceMs: 0, + retryDeferredShutdown: rearmed.rearm && rearmed.retryDeferredShutdown + }) + ).toEqual({ branch: 'shutdown-deferred', timeoutMs: IDLE_RELAY_GRACE_MS }) + }) + + it('ignores a re-assertion of the same grace', () => { + // Why: the host re-asserts its grace on every establish; re-arming on those would keep the + // window alive indefinitely. + expect(reconfigure({ previousConfiguredGraceMs: 86_400_000 })).toEqual({ rearm: false }) + }) + + it('does not arm a window that was never running', () => { + expect(reconfigure({ graceTimerArmed: false })).toEqual({ rearm: false }) + }) + + it('does not re-arm once shutdown is in flight', () => { + expect(reconfigure({ shutdownInFlight: true })).toEqual({ rearm: false }) + }) +}) + +/** + * Models relay.ts's grace state around the configureGraceTime call site: `startGrace` re-decides the + * branch exactly as the relay does, so a dropped `retryDeferredShutdown` shows up as a branch downgrade. + */ +function relayGraceHost( + overrides: { + configuredGraceMs?: number + graceTimerArmed?: boolean + shutdownInFlight?: boolean + graceBranch?: RelayGraceBranch | null + relayIdle?: boolean + activePtyCount?: number + } = {} +) { + let configuredGraceMs = overrides.configuredGraceMs ?? 10_000 + let graceBranch: RelayGraceBranch | null = overrides.graceBranch ?? 'configured' + const startGrace = vi.fn( + (_reason: string, options?: { retryDeferredShutdown?: boolean }): void => { + graceBranch = decide({ + configuredGraceMs, + relayIdle: overrides.relayIdle ?? true, + activePtyCount: overrides.activePtyCount ?? 0, + retryDeferredShutdown: options?.retryDeferredShutdown === true + }).branch + } + ) + + return { + startGrace, + branch: () => graceBranch, + configuredGraceMs: () => configuredGraceMs, + apply: (graceTimeSeconds: unknown) => + applyRelayGraceTimeConfiguration(graceTimeSeconds, { + readConfiguredGraceMs: () => configuredGraceMs, + // Mirrors PtyHandler.setGraceTimeMs's clamp so the decision sees the stored value. + writeConfiguredGraceMs: (graceMs) => { + configuredGraceMs = Math.max(0, Math.floor(graceMs)) + }, + isGraceTimerArmed: () => overrides.graceTimerArmed ?? true, + isShutdownInFlight: () => overrides.shutdownInFlight ?? false, + readGraceBranch: () => graceBranch, + startGrace + }) + } +} + +describe('applyRelayGraceTimeConfiguration', () => { + it('re-arms a deferred shutdown without downgrading it to an ordinary configured window', () => { + // Why: the call site the branch selector alone cannot cover — dropping the startGrace option here + // would leave a refused kill with nothing left to retry it. + const host = relayGraceHost({ graceBranch: 'shutdown-deferred', configuredGraceMs: 0 }) + + expect(host.apply(86_400)).toEqual({ graceTimeMs: 86_400_000 }) + expect(host.startGrace).toHaveBeenCalledWith('grace reconfigured', { + retryDeferredShutdown: true + }) + expect(host.branch()).toBe('shutdown-deferred') + }) + + it('re-arms an ordinary window at the raised grace', () => { + const host = relayGraceHost() + + expect(host.apply(86_400)).toEqual({ graceTimeMs: 86_400_000 }) + expect(host.startGrace).toHaveBeenCalledWith('grace reconfigured', { + retryDeferredShutdown: false + }) + expect(host.branch()).toBe('configured') + }) + + it('stores the host-sleep zero and lets the idle cap own the window', () => { + const host = relayGraceHost() + + expect(host.apply(0)).toEqual({ graceTimeMs: 0 }) + expect(host.startGrace).toHaveBeenCalledTimes(1) + expect(host.branch()).toBe('idle-no-ptys') + }) + + it('does not re-arm on a re-asserted grace', () => { + const host = relayGraceHost({ configuredGraceMs: 86_400_000 }) + + expect(host.apply(86_400)).toEqual({ graceTimeMs: 86_400_000 }) + expect(host.startGrace).not.toHaveBeenCalled() + }) + + it('does not arm a window that was never running', () => { + const host = relayGraceHost({ graceTimerArmed: false }) + + expect(host.apply(86_400)).toEqual({ graceTimeMs: 86_400_000 }) + expect(host.startGrace).not.toHaveBeenCalled() + }) + + it('ignores a malformed or negative payload without touching the stored grace', () => { + for (const payload of [undefined, 'later', Number.NaN, -1]) { + const host = relayGraceHost() + + expect(host.apply(payload)).toEqual({ graceTimeMs: 10_000 }) + expect(host.configuredGraceMs()).toBe(10_000) + expect(host.startGrace).not.toHaveBeenCalled() + } + }) +}) diff --git a/src/relay/relay-grace-branch.ts b/src/relay/relay-grace-branch.ts index 52c6935df..68ba3e2ec 100644 --- a/src/relay/relay-grace-branch.ts +++ b/src/relay/relay-grace-branch.ts @@ -23,10 +23,82 @@ export type RelayGraceDecision = { timeoutMs: number } -export function retryDeferredShutdownAfterGraceReconfigure( +export type RelayGraceReconfigureInput = { + previousConfiguredGraceMs: number + nextConfiguredGraceMs: number + /** A grace window with a real deadline is running; an unlimited (deadline-less) one is not re-armed. */ + graceTimerArmed: boolean + shutdownInFlight: boolean currentBranch: RelayGraceBranch | null -): boolean { - return currentBranch === 'shutdown-deferred' +} + +export type RelayGraceReconfigureDecision = + | { rearm: false } + | { rearm: true; retryDeferredShutdown: boolean } + +/** Live relay grace state, read through accessors so the caller's `let` bindings stay authoritative. */ +export type RelayGraceTimeConfigurationInput = { + readConfiguredGraceMs: () => number + /** Writes the new grace; the value is read back so a normalizing setter wins. */ + writeConfiguredGraceMs: (graceMs: number) => void + isGraceTimerArmed: () => boolean + isShutdownInFlight: () => boolean + readGraceBranch: () => RelayGraceBranch | null + startGrace: (reason: string, options?: { retryDeferredShutdown?: boolean }) => void +} + +/** + * Applies a `relay.configureGraceTime` payload, re-arming the running window when the value changed. + * + * Why extracted from relay.ts: that file runs `main()` on import and exports nothing, so the call site + * — including the `retryDeferredShutdown` hand-off into `startGrace` — is otherwise untestable. + */ +export function applyRelayGraceTimeConfiguration( + graceTimeSeconds: unknown, + state: RelayGraceTimeConfigurationInput +): { graceTimeMs: number } { + const seconds = Number(graceTimeSeconds) + if (Number.isFinite(seconds) && seconds >= 0) { + const previousConfiguredGraceMs = state.readConfiguredGraceMs() + // Why: the host sends 0 before system sleep so live remote PTYs survive longer than the ordinary grace window. + state.writeConfiguredGraceMs(Math.floor(seconds) * 1000) + const reconfigure = decideRelayGraceReconfigure({ + previousConfiguredGraceMs, + nextConfiguredGraceMs: state.readConfiguredGraceMs(), + graceTimerArmed: state.isGraceTimerArmed(), + shutdownInFlight: state.isShutdownInFlight(), + currentBranch: state.readGraceBranch() + }) + if (reconfigure.rearm) { + state.startGrace('grace reconfigured', { + retryDeferredShutdown: reconfigure.retryDeferredShutdown + }) + } + } + return { graceTimeMs: state.readConfiguredGraceMs() } +} + +/** + * Decides whether a live grace change must re-arm the running grace timer. + * + * Why a decision rather than inline gating: `startGrace` samples the configured grace at arm time, so + * this gate is the only thing that makes a post-launch raise take effect on an already-running window. + */ +export function decideRelayGraceReconfigure( + input: RelayGraceReconfigureInput +): RelayGraceReconfigureDecision { + // Why only on an actual change: the host re-asserts the same value on every establish, and + // restarting the window on those would keep a grace alive indefinitely. + if ( + input.nextConfiguredGraceMs === input.previousConfiguredGraceMs || + !input.graceTimerArmed || + input.shutdownInFlight + ) { + return { rearm: false } + } + // Why carry the branch forward: re-arming without it downgrades a deferred shutdown to an ordinary + // configured window, and the refused kill is never retried. + return { rearm: true, retryDeferredShutdown: input.currentBranch === 'shutdown-deferred' } } /** diff --git a/src/relay/relay.ts b/src/relay/relay.ts index a0228e0cc..4bfc8bf3e 100644 --- a/src/relay/relay.ts +++ b/src/relay/relay.ts @@ -52,8 +52,8 @@ import { import { resolveSetupAgentSequenceLaunchCommand } from '../shared/setup-agent-sequencing' import { pickRemoteCliEnv } from './remote-cli-env' import { + applyRelayGraceTimeConfiguration, decideRelayGrace, - retryDeferredShutdownAfterGraceReconfigure, type RelayGraceBranch } from './relay-grace-branch' import { relayLogLine } from './relay-diagnostic-log' @@ -685,27 +685,14 @@ async function main(): Promise { }) function configureRelayGraceTime(params: Record): { graceTimeMs: number } { - const seconds = Number(params.graceTimeSeconds) - if (Number.isFinite(seconds) && seconds >= 0) { - const previousGraceMs = ptyHandler.configuredGraceTimeMs - // Why: the host sends 0 before system sleep so live remote PTYs survive longer than the ordinary grace window. - ptyHandler.setGraceTimeMs(Math.floor(seconds) * 1000) - // Why: startGrace samples the configured value at arm time, so a raise that lands while the idle - // timer is already running would still fire at the old deadline. Re-arm only on an actual change - // — the host re-asserts the same value on every establish, and restarting the window on those - // would keep a grace alive indefinitely. - if ( - ptyHandler.configuredGraceTimeMs !== previousGraceMs && - graceDeadlineAt !== null && - graceReason !== null && - !shutdownInFlight - ) { - startGrace('grace reconfigured', { - retryDeferredShutdown: retryDeferredShutdownAfterGraceReconfigure(graceBranch) - }) - } - } - return { graceTimeMs: ptyHandler.configuredGraceTimeMs } + return applyRelayGraceTimeConfiguration(params.graceTimeSeconds, { + readConfiguredGraceMs: () => ptyHandler.configuredGraceTimeMs, + writeConfiguredGraceMs: (graceMs) => ptyHandler.setGraceTimeMs(graceMs), + isGraceTimerArmed: () => graceDeadlineAt !== null && graceReason !== null, + isShutdownInFlight: () => shutdownInFlight, + readGraceBranch: () => graceBranch, + startGrace + }) } dispatcher.onNotification(SSH_RELAY_CONFIGURE_GRACE_TIME_METHOD, (params) => { diff --git a/src/renderer/src/components/activity/activity-portal-readiness-loop.react185.test.tsx b/src/renderer/src/components/activity/activity-portal-readiness-loop.react185.test.tsx index 50190295d..10d9fc2af 100644 --- a/src/renderer/src/components/activity/activity-portal-readiness-loop.react185.test.tsx +++ b/src/renderer/src/components/activity/activity-portal-readiness-loop.react185.test.tsx @@ -15,7 +15,10 @@ import { resolveActivityPortalSwap, type ActivityPortalThreadRef } from './activity-portal-thread-reconciliation' -import type { ActivityPortalReadinessStatus } from './activity-portal-readiness-oscillation' +import { + ACTIVITY_PORTAL_READINESS_MAX_FLIPS, + type ActivityPortalReadinessStatus +} from './activity-portal-readiness-oscillation' const WORKTREE_ID = 'wt-1' const TAB_ID = 'tab-react185' @@ -85,6 +88,28 @@ async function flushPortalFramesUntil( } } +// Drain MutationObserver microtasks and the readiness rAF they schedule. Reports whether the +// drain settled so a caller never reads a transition whose readiness callbacks are still queued. +async function flushPortalReadiness( + frames: ReturnType +): Promise { + for (let attempt = 0; attempt < 8; attempt += 1) { + await act(async () => { + await Promise.resolve() + }) + if (frames.pending() === 0) { + await act(async () => { + await Promise.resolve() + }) + if (frames.pending() === 0) { + return true + } + } + await frames.flush() + } + return frames.pending() === 0 +} + // Models the tab-root DOM and sibling hiding emitted by a portaled TerminalPane. function renderPortaledTerminalPane(target: HTMLElement, tabId: string, leafIds: string[]): void { const isolatedLeafId = leafIds[0] @@ -354,23 +379,47 @@ describe('Activity portal pane switching', () => { await act(async () => { root.render() }) + expect(await flushPortalReadiness(frames)).toBe(true) await flushPortalFramesUntil(frames, () => statuses.at(-1) === 'unavailable') expect(statuses.at(-1)).toBe('unavailable') - // Feed each observed DOM state separately so MutationObserver cannot coalesce the flips. - for (let flip = 0; flip < 9; flip += 1) { + // Feed each DOM state separately and keep going until sibling DOM reports latched + // unavailable. A fixed 9-flip budget flakes when CI load drops MutationObserver + // deliveries below ACTIVITY_PORTAL_READINESS_MAX_FLIPS transitions. + let sawSiblingLoading = false + let sawLatchedSibling = false + for ( + let flip = 0; + flip < ACTIVITY_PORTAL_READINESS_MAX_FLIPS * 4 && !sawLatchedSibling; + flip += 1 + ) { + const mode = flip % 2 === 0 ? 'sibling' : 'hidden' + const statusesBefore = statuses.length await act(async () => { - buildRoot(flip % 2 === 0 ? 'sibling' : 'hidden') + buildRoot(mode) await Promise.resolve() }) - await frames.flush() + expect(await flushPortalReadiness(frames)).toBe(true) + if (mode !== 'sibling') { + continue + } + // Transition-local evidence: an unlatched subscription answers sibling DOM with 'loading', + // so the latch is only proven once a sibling transition that previously emitted 'loading' + // stops doing so and leaves 'unavailable' standing. + if (statuses.slice(statusesBefore).includes('loading')) { + sawSiblingLoading = true + } else if (sawSiblingLoading && statuses.at(-1) === 'unavailable') { + sawLatchedSibling = true + } } + expect(sawLatchedSibling).toBe(true) expect(statuses.at(-1)).toBe('unavailable') await act(async () => { buildRoot('ready') await Promise.resolve() }) + expect(await flushPortalReadiness(frames)).toBe(true) await flushPortalFramesUntil(frames, () => statuses.at(-1) === 'ready') expect(statuses.at(-1)).toBe('ready') }) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts index f1faa368e..b3fdc5835 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts @@ -413,6 +413,46 @@ describe('refreshFileExplorerExpandedDirs', () => { expect(cache['/repo/b']).toMatchObject({ loading: false, children: [{ name: 'index.ts' }] }) }) + it('stops later batches after a commit callback throws', async () => { + let cache: Record = {} + const setDirCache = vi.fn((update: CacheUpdate) => { + cache = typeof update === 'function' ? update(cache) : update + }) + const commitError = new Error('commit failed') + const onDirCommitted = vi.fn((dirPath: string) => { + if (dirPath === '/repo/a') { + throw commitError + } + }) + + await expect( + refreshFileExplorerExpandedDirs({ + dirs: ['a', 'b', 'c', 'd'].map((name) => ({ dirPath: `/repo/${name}`, depth: 0 })), + worktreePath: '/repo', + dirLoadTracker: createFileExplorerDirLoadTracker(), + setDirCache, + readDirectory: async () => ({ + entries: [entry('index.ts')], + operationOwner: { kind: 'local' as const } + }), + // Two dirs per commit batch, so /repo/c and /repo/d belong to a later batch. + maxConcurrentReads: 2, + onDirCommitted + }) + ).rejects.toBe(commitError) + + // A surviving worker must not commit past the failure: callers observing the rejection + // would otherwise still get setDirCache writes and staleness clears afterwards. + expect(onDirCommitted.mock.calls.map(([dirPath]) => dirPath).sort()).toEqual([ + '/repo/a', + '/repo/b' + ]) + // One up-front loading write plus the single failed batch's result write. + expect(setDirCache).toHaveBeenCalledTimes(2) + expect(cache['/repo/c']).toMatchObject({ loading: true }) + expect(cache['/repo/d']).toMatchObject({ loading: true }) + }) + it('drops a queued directory superseded while an earlier read is blocked', async () => { const tracker = createFileExplorerDirLoadTracker() let cache: Record = {} diff --git a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts index 06c5347b4..64dc0d587 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts @@ -51,6 +51,9 @@ export async function refreshFileExplorerExpandedDirs({ const pendingResults: { dirPath: string; cache: DirCache }[] = [] let settledSinceCommit = 0 let committedDirs = 0 + // Why: forEachWithConcurrency has no cancel hook, so a failed commit must stop the surviving + // workers itself — otherwise a later batch commits after the caller already saw this reject. + let stopped = false // Why: mark every dir loading up front — FileExplorer's auto-load // effect re-runs on any `expanded` change and fans out an unbounded loadDir per @@ -67,6 +70,9 @@ export async function refreshFileExplorerExpandedDirs({ }) const commitPendingResults = (): void => { + if (stopped) { + return + } settledSinceCommit = 0 const currentResults = pendingResults .splice(0) @@ -98,6 +104,7 @@ export async function refreshFileExplorerExpandedDirs({ } } if (commitFailed) { + stopped = true throw firstCommitError } } @@ -113,6 +120,9 @@ export async function refreshFileExplorerExpandedDirs({ } await forEachWithConcurrency(uniqueDirs, maxConcurrentReads, async ({ dirPath, depth }) => { + if (stopped) { + return + } const loadToken = loadTokens.get(dirPath)! // A superseding load owns this dir now; do not spend a round trip on a result we must drop. if (!dirLoadTracker.isCurrent(loadToken)) { diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts b/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts index 0d9c42af6..d8b218b64 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerWatch.ts @@ -128,7 +128,9 @@ export function useFileExplorerWatch({ // Why: one atomic effect avoids a cleanup-ordering race that drops events on rapid worktree switches (review issue §3). useEffect(() => { - if (!worktreePath || activeRuntimeEnvironmentId === undefined) { + // Why require a worktree id: it is half of every watch key, and reconciliation already needs it — + // a null-keyed subscription would only park resync state no correctly-keyed one can consume. + if (!worktreePath || !activeWorktreeId || activeRuntimeEnvironmentId === undefined) { return } @@ -172,9 +174,6 @@ export function useFileExplorerWatch({ } function processPayload(payload: FsChangedPayload): void { - if (!currentWorktreeId) { - return - } processFileExplorerFsPayload({ payload, currentWorktreePath,