Make explicit desktop take-back always release the mobile presence lock (#8200)
Previously a failed resize during reclaimTerminalForDesktop left the override/lock in place (#7588 semantics), but for an explicit "take back all terminals" gesture that could strand banners on background panes whose resize can't converge. Now the take-back unconditionally releases the driver and clears any held fit-override via a new releaseDesktopTakeBack helper, while auto-restore and phone-initiated paths keep the original keep-lock-on-failure behavior. Also extract the local mapWithConcurrency helper out of workspace-cleanup.ts into a shared, tested src/shared/map-with-concurrency.ts and use it to bound concurrent desktop-fit reclaims in terminal-fit-restore.ts.
This commit is contained in:
Jinjing
GitHub
parent
5a8078e755
commit
a916a93df3
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<boolean> {
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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<GlobalSettings, 'activeRuntimeEnvironmentId'> | 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<boolean> {
|
||||
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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<T, R>(
|
||||
items: readonly T[],
|
||||
limit: number,
|
||||
fn: (item: T) => Promise<R>
|
||||
): Promise<R[]> {
|
||||
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<string>()
|
||||
if (state.activeWorktreeId) {
|
||||
|
|
|
|||
|
|
@ -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])
|
||||
})
|
||||
})
|
||||
|
|
@ -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<T, R>(
|
||||
items: readonly T[],
|
||||
limit: number,
|
||||
fn: (item: T, index: number) => Promise<R>
|
||||
): Promise<R[]> {
|
||||
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
|
||||
}
|
||||
Loading…
Reference in New Issue