diff --git a/src/main/runtime/mobile-presence-lock.test.ts b/src/main/runtime/mobile-presence-lock.test.ts index a38084c51..eddb3f299 100644 --- a/src/main/runtime/mobile-presence-lock.test.ts +++ b/src/main/runtime/mobile-presence-lock.test.ts @@ -574,9 +574,13 @@ describe('mobile presence lock — issue #7588 held-modal restore convergence', expect(listenerEvents.some((e) => e.mode === 'desktop-fit')).toBe(true) }) - // Scenario 4: a failing resize during a HELD (no-subscriber) restore returns - // false and keeps the override — no lying, no phantom desktop-fit event. - it('held restore with a failing resize returns false and keeps the override', async () => { + // Scenario 4: an explicit take-back on a HELD (no-subscriber) override must + // ALWAYS release, even when the desktop resize fails. The override is cleared + // optimistically with a paired desktop-fit 0×0 so the banner can't linger; the + // desktop renderer refits the PTY on its next settled frame. (The gesture + // deliberately overrides #7588's keep-lock-on-failed-resize rule, which still + // governs the auto-restore and phone-initiated paths.) + it('held restore with a failing resize still releases and clears the override', async () => { const { runtime, fitOverrideEvents, setResizeSucceeds } = createRuntime(null) const listenerEvents: { mode: string; cols: number; rows: number }[] = [] runtime.subscribeToFitOverrideChanges('pty-1', (e) => listenerEvents.push(e)) @@ -594,18 +598,17 @@ describe('mobile presence lock — issue #7588 held-modal restore convergence', const restored = await runtime.reclaimTerminalForDesktop('pty-1') - expect(restored).toBe(false) - expect(runtime.getTerminalFitOverride('pty-1')).not.toBeNull() - expect(fitOverrideEvents.slice(notifierBefore).some((e) => e.mode === 'desktop-fit')).toBe( - false - ) - expect(listenerEvents.slice(listenerBefore).some((e) => e.mode === 'desktop-fit')).toBe(false) + expect(restored).toBe(true) + expect(runtime.getTerminalFitOverride('pty-1')).toBeNull() + expect(fitOverrideEvents.slice(notifierBefore).some((e) => e.mode === 'desktop-fit')).toBe(true) + expect(listenerEvents.slice(listenerBefore).some((e) => e.mode === 'desktop-fit')).toBe(true) }) - // Scenario 5: a failing resize during an ACTIVE-SUBSCRIBER take-back returns - // false, keeps the override, leaves the driver on its mobile lock, and - // restores the prior display mode ('auto') — the fix #3 P1 correction. - it('active-subscriber take-back with a failing resize returns false and preserves the mobile lock', async () => { + // Scenario 5: an explicit ACTIVE-SUBSCRIBER take-back with a failing resize + // must still release — driver → desktop, override cleared, banner dismissed, + // mode reset to 'auto'. This is the "take back all terminals" guarantee: a + // background PTY that can't converge must not strand its banner. + it('active-subscriber take-back with a failing resize still releases the lock', async () => { const { runtime, fitOverrideEvents, setResizeSucceeds } = createRuntime() const listenerEvents: { mode: string; cols: number; rows: number }[] = [] runtime.subscribeToFitOverrideChanges('pty-1', (e) => listenerEvents.push(e)) @@ -619,43 +622,30 @@ describe('mobile presence lock — issue #7588 held-modal restore convergence', const restored = await runtime.reclaimTerminalForDesktop('pty-1') - expect(restored).toBe(false) - expect(runtime.getTerminalFitOverride('pty-1')).not.toBeNull() - // Driver stays mobile (lock retained) and mode is not left lying at 'desktop'. - expect(runtime.getDriver('pty-1')).toEqual({ kind: 'mobile', clientId: 'phone-A' }) + expect(restored).toBe(true) + // Lock released and banner dismissed despite the failed resize. + expect(runtime.getDriver('pty-1')).toEqual({ kind: 'desktop' }) + expect(runtime.getTerminalFitOverride('pty-1')).toBeNull() expect(runtime.getMobileDisplayMode('pty-1')).toBe('auto') - expect(fitOverrideEvents.slice(notifierBefore).some((e) => e.mode === 'desktop-fit')).toBe( - false - ) - expect(listenerEvents.slice(listenerBefore).some((e) => e.mode === 'desktop-fit')).toBe(false) + expect(fitOverrideEvents.slice(notifierBefore).some((e) => e.mode === 'desktop-fit')).toBe(true) + expect(listenerEvents.slice(listenerBefore).some((e) => e.mode === 'desktop-fit')).toBe(true) }) - // Scenario 5b: after a FAILED active-subscriber take-back, wasResizedToPhone - // must be re-armed so a later unsubscribe under a finite auto-restore setting - // still schedules its timer and eventually clears the override. Without the - // re-arm the flag would be stuck false and the phone-fit would strand. - it('failed take-back re-arms wasResizedToPhone so a later unsubscribe still auto-restores', async () => { - // Finite auto-restore (5s default from the rig store). - const { runtime, ptySizes, setResizeSucceeds } = createRuntime() + // Scenario 5b: the failed-resize take-back leaves NO stranded phone-fit — + // driver released to desktop, override cleared, mode reset to 'auto'. (The + // pre-revision behavior kept the lock and relied on a later auto-restore; the + // explicit gesture now releases unconditionally.) + it('failed take-back leaves no stranded override or lock', async () => { + const { runtime, setResizeSucceeds } = createRuntime() await runtime.handleMobileSubscribe('pty-1', 'phone-A', { cols: 45, rows: 20 }) - expect(ptySizes.get('pty-1')).toEqual({ cols: 45, rows: 20 }) - // Take-back fails → false, flag re-armed, mode rolled back to 'auto'. setResizeSucceeds(false) - expect(await runtime.reclaimTerminalForDesktop('pty-1')).toBe(false) - expect(runtime.getTerminalFitOverride('pty-1')).not.toBeNull() + expect(await runtime.reclaimTerminalForDesktop('pty-1')).toBe(true) - // Phone then leaves the terminal. Resize works again for the auto-restore. - setResizeSucceeds(true) - runtime.handleMobileUnsubscribe('pty-1', 'phone-A') - // Soft-leave grace, then the finite auto-restore timer. - await vi.advanceTimersByTimeAsync(300) - await vi.advanceTimersByTimeAsync(5_000) - - // The scheduled auto-restore fired: override cleared, PTY back to desktop. + expect(runtime.getDriver('pty-1')).toEqual({ kind: 'desktop' }) expect(runtime.getTerminalFitOverride('pty-1')).toBeNull() - expect(ptySizes.get('pty-1')).toEqual({ cols: 150, rows: 40 }) + expect(runtime.getMobileDisplayMode('pty-1')).toBe('auto') }) // Scenario 6: a phone-initiated setDisplayMode('desktop') against a stale diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 88a8f69b8..b7e4c1d2c 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -8197,42 +8197,35 @@ export class OrcaRuntimeService { } // Why: invoked from `runtime:restoreTerminalFit` IPC (the desktop "Take - // back" / "Restore" button). Forces the PTY back to desktop dims and - // flips the driver to `desktop`, suppressing further mobile-driven dim - // changes until a mobile actor takes the floor again. Two cases: - // 1. Active mobile subscriber: route through applyMobileDisplayMode so - // the existing 'resized' event reaches the phone. - // 2. Held with no mobile subscriber (post-indefinite-hold): no inner - // subscriber to notify; resolve restore target and enqueueLayout - // directly. applyLayout is the SOLE writer of terminalFitOverrides on - // the normal path, so the held branch defers the mutation to it — the - // one exception is the dead-pty orphan cleanup below, which mirrors - // onPtyExit's sanctioned delete. See docs/mobile-fit-hold.md. + // back" / "Restore" button). Forces the PTY back to desktop dims and flips + // the driver to `desktop`, suppressing further mobile-driven dim changes + // until a mobile actor takes the floor again. Three cases, each ending in + // releaseDesktopTakeBack: + // 1. Active mobile subscriber: route through applyMobileDisplayMode so the + // existing 'resized' event reaches the phone. + // 2. Held override, no subscriber (post-indefinite-hold): resolve the + // restore target and enqueueLayout directly. + // 3. Stale mobile driver, no subscriber and no override: nothing to resize, + // just drop the lock. See docs/mobile-fit-hold.md. // - // Returns `true` only when the pty ends converged (no fit-override held); - // `false` when a restore was attempted but the resize failed (#7588), or - // when there was nothing to reclaim. On a failed-restore `false`, driver/mode - // are left unchanged so a driving phone keeps its lock and the modal - // truthfully stays — the user can retry. + // Why: explicit desktop take-back is a user command to reclaim input control + // NOW. Unlike the auto-restore timer and phone-initiated setDisplayMode paths + // (which keep the lock when a resize can't converge, #7588), this gesture + // ALWAYS drops the presence lock and banner. "Take back all terminals" + // reclaims several PTYs at once; a background pane whose desktop resize can't + // converge must not strand its banner on the other terminals. The resize is + // best-effort — the desktop renderer refits the PTY on its next settled + // frame. Returns `true` whenever there was a lock to reclaim, `false` only + // when there was nothing to reclaim. async reclaimTerminalForDesktop(ptyId: string): Promise { if (this.isMobileSubscriberActive(ptyId)) { - // Why (#7588): capture the prior mode so a failed restore can roll the - // routing-only 'desktop' write back — driver/mode transitions must be - // gated on convergence, not committed before we know the resize took. - const priorMode = this.getMobileDisplayMode(ptyId) this.setMobileDisplayMode(ptyId, 'desktop') - const converged = await this.applyMobileDisplayMode(ptyId) - if (!converged) { - // Resize failed — override still held. Leave the driver on its mobile - // lock and undo the mode write so nothing is left lying at 'desktop'. - this.setMobileDisplayMode(ptyId, priorMode) - return false - } - this.setDriver(ptyId, { kind: 'desktop' }) - // Why: a desktop-initiated reclaim is "I'm taking over right now", - // not a sticky preference. The next mobile subscribe (e.g. user - // switches back to the terminal tab on the phone) must default to - // phone-fit again, not stay in passive desktop-watch mode. + await this.applyMobileDisplayMode(ptyId) + this.releaseDesktopTakeBack(ptyId) + // Why: a desktop-initiated reclaim is "I'm taking over right now", not a + // sticky preference. The next mobile subscribe (e.g. user switches back to + // the terminal tab on the phone) must default to phone-fit again, not stay + // in passive desktop-watch mode. this.setMobileDisplayMode(ptyId, 'auto') return true } @@ -8251,36 +8244,38 @@ export class OrcaRuntimeService { const renderer = this.lastRendererSizes.get(ptyId) const cols = renderer?.cols ?? heldOverride.previousCols ?? fallback.cols const rows = renderer?.rows ?? heldOverride.previousRows ?? fallback.rows - const result = await this.enqueueLayout(ptyId, { kind: 'desktop', cols, rows }) - if (result.ok) { - this.setDriver(ptyId, { kind: 'desktop' }) - // Why: a desktop-initiated reclaim is "I'm taking over right now", - // not a sticky preference. Reset to auto so the next mobile subscribe - // re-enters phone-fit. (Held-PTY branch may not have an entry, but - // calling setMobileDisplayMode('auto') is a no-op deletion in that - // case — safe and idempotent.) - this.setMobileDisplayMode(ptyId, 'auto') - return true - } - // Why (#7588): the layout entry is gone (pty exited) but the override is - // still held — unreachable via public APIs today (onPtyExit deletes both - // in lockstep), but reporting success while stranding the override would - // re-show the modal on the next hydrate. Run the same cleanup onPtyExit - // does (delete override + paired desktop-fit 0×0) so the renderer - // converges; nothing to resize on a dead pty. - if (result.reason === 'pty-exited' && this.terminalFitOverrides.has(ptyId)) { - this.terminalFitOverrides.delete(ptyId) - this.notifier?.terminalFitOverrideChanged(ptyId, 'desktop-fit', 0, 0) - this.notifyFitOverrideListeners(ptyId, 'desktop-fit', 0, 0) - return true - } - // resize-failed: the override remains held; report the truth so the - // modal correctly stays and the user can retry. Driver/mode untouched. - return false + await this.enqueueLayout(ptyId, { kind: 'desktop', cols, rows }) + this.releaseDesktopTakeBack(ptyId) + this.setMobileDisplayMode(ptyId, 'auto') + return true + } + // Why: a stale lock — driver still reads mobile with no active subscriber + // and no held override (e.g. reclaimed inside the soft-leave grace, or a + // subscriber that dropped without a clean unsubscribe). Release it so the + // banner can't linger; there is nothing to resize. + if (this.getDriver(ptyId).kind === 'mobile') { + this.releaseDesktopTakeBack(ptyId) + return true } return false } + // Why: the shared "banner must be gone now" step for an explicit desktop + // take-back. Releases the presence lock (driver → desktop) and, if the + // best-effort resize left a fit-override held (resize didn't converge), + // clears it optimistically with a paired desktop-fit 0×0 — the same signal + // onPtyExit emits — so neither the presence-lock banner nor the held-fit + // banner can survive the reclaim. The desktop renderer refits the PTY to real + // dims on its next settled frame. + private releaseDesktopTakeBack(ptyId: string): void { + this.setDriver(ptyId, { kind: 'desktop' }) + if (this.terminalFitOverrides.has(ptyId)) { + this.terminalFitOverrides.delete(ptyId) + this.notifier?.terminalFitOverrideChanged(ptyId, 'desktop-fit', 0, 0) + this.notifyFitOverrideListeners(ptyId, 'desktop-fit', 0, 0) + } + } + // Why: read-side clamp for mobileAutoRestoreFitMs. `null` means // indefinite hold (no auto-restore timer). A finite value is clamped // to [MIN, MAX] to defend against bad config — the smallest useful diff --git a/src/renderer/src/components/terminal-pane/terminal-fit-restore.ts b/src/renderer/src/components/terminal-pane/terminal-fit-restore.ts index bdf152a35..54738f590 100644 --- a/src/renderer/src/components/terminal-pane/terminal-fit-restore.ts +++ b/src/renderer/src/components/terminal-pane/terminal-fit-restore.ts @@ -1,4 +1,5 @@ import type { GlobalSettings } from '../../../../shared/types' +import { mapWithConcurrency } from '../../../../shared/map-with-concurrency' import { callRuntimeRpc } from '@/runtime/runtime-rpc-client' import { getRemoteRuntimePtyEnvironmentId, @@ -7,6 +8,13 @@ import { type TerminalFitRestoreSettings = Pick | undefined +// Why: "take back all terminals" can target a phone that controls hundreds of +// PTYs. Fanning out one IPC/RPC reclaim per PTY unbounded would burst the +// runtime transport; cap the in-flight reclaims so a huge session degrades to +// steady throughput instead of a thundering herd. Each reclaim is a short +// round-trip, so a modest pool keeps latency low without overwhelming it. +const RESTORE_FIT_CONCURRENCY = 8 + const restoreFailedResult = (): { restored: boolean } => { // Why: terminal fit restore is best-effort when mobile/remote transports disappear. return { restored: false } @@ -37,8 +45,8 @@ export async function restoreTerminalFitsToDesktop( settings: TerminalFitRestoreSettings ): Promise { const uniquePtyIds = [...new Set(ptyIds)] - const results = await Promise.all( - uniquePtyIds.map((ptyId) => restoreTerminalFitToDesktop(ptyId, settings)) + const results = await mapWithConcurrency(uniquePtyIds, RESTORE_FIT_CONCURRENCY, (ptyId) => + restoreTerminalFitToDesktop(ptyId, settings) ) return results.some(Boolean) } diff --git a/src/renderer/src/store/slices/workspace-cleanup.ts b/src/renderer/src/store/slices/workspace-cleanup.ts index ed2e12d49..bffe5fb93 100644 --- a/src/renderer/src/store/slices/workspace-cleanup.ts +++ b/src/renderer/src/store/slices/workspace-cleanup.ts @@ -21,6 +21,7 @@ import { type WorkspaceCleanupScanProgress, type WorkspaceCleanupScanResult } from '../../../../shared/workspace-cleanup' +import { mapWithConcurrency } from '../../../../shared/map-with-concurrency' import { classifyTitleActivity, isExplicitAgentStatusFresh } from '@/lib/pane-agent-evidence' import { translate } from '@/i18n/i18n' @@ -553,26 +554,6 @@ function getWorkspaceCleanupProgressCandidateIndex( } } -async function mapWithConcurrency( - items: readonly T[], - limit: number, - fn: (item: T) => Promise -): Promise { - const results: R[] = [] - let nextIndex = 0 - const workerCount = Math.min(limit, items.length) - await Promise.all( - Array.from({ length: workerCount }, async () => { - while (nextIndex < items.length) { - const index = nextIndex - nextIndex += 1 - results[index] = await fn(items[index]) - } - }) - ) - return results -} - function getInitialWorkspaceCleanupGitDeferrals(state: AppState): string[] { const ids = new Set() if (state.activeWorktreeId) { diff --git a/src/shared/map-with-concurrency.test.ts b/src/shared/map-with-concurrency.test.ts new file mode 100644 index 000000000..fb81bc0ed --- /dev/null +++ b/src/shared/map-with-concurrency.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from 'vitest' +import { mapWithConcurrency } from './map-with-concurrency' + +describe('mapWithConcurrency', () => { + it('preserves input order in the result array regardless of settle order', async () => { + // Later items resolve sooner, so a naive push-on-resolve would reorder. + const results = await mapWithConcurrency([30, 20, 10], 3, async (ms, index) => { + await new Promise((resolve) => setTimeout(resolve, ms)) + return index + }) + expect(results).toEqual([0, 1, 2]) + }) + + it('never exceeds the concurrency limit', async () => { + let inFlight = 0 + let peak = 0 + const items = Array.from({ length: 50 }, (_, i) => i) + await mapWithConcurrency(items, 8, async () => { + inFlight += 1 + peak = Math.max(peak, inFlight) + await new Promise((resolve) => setTimeout(resolve, 1)) + inFlight -= 1 + }) + expect(peak).toBeLessThanOrEqual(8) + // With 50 items and limit 8, the pool should actually saturate. + expect(peak).toBe(8) + }) + + it('processes every item exactly once', async () => { + const seen: number[] = [] + await mapWithConcurrency([1, 2, 3, 4, 5], 2, async (n) => { + seen.push(n) + }) + expect(seen.sort((a, b) => a - b)).toEqual([1, 2, 3, 4, 5]) + }) + + it('returns an empty array for no items without spawning workers', async () => { + let calls = 0 + const results = await mapWithConcurrency([], 4, async () => { + calls += 1 + }) + expect(results).toEqual([]) + expect(calls).toBe(0) + }) + + it('clamps a limit below one to a single worker instead of stalling', async () => { + const results = await mapWithConcurrency([1, 2, 3], 0, async (n) => n * 2) + expect(results).toEqual([2, 4, 6]) + }) +}) diff --git a/src/shared/map-with-concurrency.ts b/src/shared/map-with-concurrency.ts new file mode 100644 index 000000000..5930ddbed --- /dev/null +++ b/src/shared/map-with-concurrency.ts @@ -0,0 +1,24 @@ +// Why: bounded-concurrency map. `Promise.all(items.map(fn))` fans out every +// item at once — fine for a handful, but a burst of hundreds of concurrent +// IPC/RPC round-trips can swamp the transport or its call queue. This runs at +// most `limit` calls in flight via a fixed worker pool while preserving input +// order in the result array (results[i] corresponds to items[i]). +export async function mapWithConcurrency( + items: readonly T[], + limit: number, + fn: (item: T, index: number) => Promise +): Promise { + const results: R[] = [] + let nextIndex = 0 + const workerCount = Math.max(1, Math.min(limit, items.length)) + await Promise.all( + Array.from({ length: workerCount }, async () => { + while (nextIndex < items.length) { + const index = nextIndex + nextIndex += 1 + results[index] = await fn(items[index], index) + } + }) + ) + return results +}