[P2] fix(runtime): stop broadcasting terminalSideEffects to clients without consumers (#11619)
* fix(runtime): stop broadcasting terminalSideEffects to clients without consumers Co-authored-by: Orca <help@stably.ai> * fix(runtime): keep mobile subscribers counted for side-effect availability Excluding phones from the consumer-availability count added a new flip edge (last desktop client leaving a phone-attached host), and the flip's tracker rebuild cancels armed stale-working-title timers — stranding a 'working' spinner on the phone. Availability counts all subscribers again; the broadcast fix stays in the per-listener fan-out skip, now applied inside the delivery callback so live-Set unsubscribe semantics and allocation-free iteration are preserved. Co-authored-by: Orca <help@stably.ai> * fix(runtime): separate mobile title tracking from side-effect scans --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Neil
GitHub
parent
6442a9f649
commit
eb35c7fa3e
|
|
@ -8468,6 +8468,81 @@ describe('OrcaRuntimeService', () => {
|
|||
expect(events).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('omits terminalSideEffects from non-consuming listeners while other events still flow', () => {
|
||||
const runtime = new OrcaRuntimeService(store)
|
||||
const desktopEvents: RuntimeClientEvent[] = []
|
||||
const mobileEvents: RuntimeClientEvent[] = []
|
||||
runtime.syncWindowGraph(HEADLESS_RUNTIME_WINDOW_ID, { tabs: [], leaves: [] })
|
||||
runtime.onClientEvent((event) => desktopEvents.push(event))
|
||||
runtime.onClientEvent((event) => mobileEvents.push(event), {
|
||||
consumesTerminalSideEffects: false
|
||||
})
|
||||
|
||||
runtime.onPtyData('pty-remote', '\x1b]0;Codex working\x07', 100)
|
||||
runtime.notifyBranchRenamed(TEST_REPO_ID)
|
||||
|
||||
expect(desktopEvents.map((event) => event.type)).toEqual([
|
||||
'terminalSideEffects',
|
||||
'worktreesChanged'
|
||||
])
|
||||
expect(mobileEvents.map((event) => event.type)).toEqual(['worktreesChanged'])
|
||||
})
|
||||
|
||||
it('keeps a phone-only host producing title state without emitting batches to it', async () => {
|
||||
vi.useFakeTimers()
|
||||
try {
|
||||
const ptyId = `${TEST_REPO_ID}::/tmp/worktree-a@@pty-a`
|
||||
const runtime = new OrcaRuntimeService(store)
|
||||
const mobileEvents: RuntimeClientEvent[] = []
|
||||
const trackerEntries = (
|
||||
runtime as unknown as {
|
||||
ptyTitleTrackersByPtyId: Map<string, { commandCodeDetector: unknown }>
|
||||
}
|
||||
).ptyTitleTrackersByPtyId
|
||||
runtime.setPtyController({
|
||||
write: () => true,
|
||||
kill: () => true,
|
||||
getForegroundProcess: async () => null,
|
||||
listProcesses: async () => [{ id: ptyId, cwd: '/tmp/worktree-a', title: 'shell' }]
|
||||
})
|
||||
runtime.syncWindowGraph(HEADLESS_RUNTIME_WINDOW_ID, { tabs: [], leaves: [] })
|
||||
runtime.onClientEvent((event) => mobileEvents.push(event), {
|
||||
consumesTerminalSideEffects: false
|
||||
})
|
||||
const unsubscribeDesktop = runtime.onClientEvent(() => {})
|
||||
|
||||
runtime.onPtyData(ptyId, '\x1b]0;Codex working\x07', 100)
|
||||
runtime.onPtyData(ptyId, 'output without a title\r\n', 101)
|
||||
// The phone is still subscribed: disposing trackers on this edge would cancel
|
||||
// its armed stale-working-title timer and strand a 'working' spinner (#1437).
|
||||
unsubscribeDesktop()
|
||||
|
||||
await vi.advanceTimersByTimeAsync(3_000)
|
||||
|
||||
expect(trackerEntries.has(ptyId)).toBe(true)
|
||||
expect(trackerEntries.get(ptyId)?.commandCodeDetector).toBeNull()
|
||||
expect((await runtime.listTerminals()).terminals[0]).toMatchObject({ title: 'Codex' })
|
||||
expect(mobileEvents.some((event) => event.type === 'terminalSideEffects')).toBe(false)
|
||||
} finally {
|
||||
vi.useRealTimers()
|
||||
}
|
||||
})
|
||||
|
||||
it('skips a listener unsubscribed mid-fan-out even with mobile exclusions active', () => {
|
||||
const runtime = new OrcaRuntimeService(store)
|
||||
const lateEvents: RuntimeClientEvent[] = []
|
||||
runtime.syncWindowGraph(HEADLESS_RUNTIME_WINDOW_ID, { tabs: [], leaves: [] })
|
||||
runtime.onClientEvent(() => {}, { consumesTerminalSideEffects: false })
|
||||
runtime.onClientEvent(() => {
|
||||
unsubscribeLate()
|
||||
})
|
||||
const unsubscribeLate = runtime.onClientEvent((event) => lateEvents.push(event))
|
||||
|
||||
runtime.onPtyData('pty-remote', '\x1b]0;Codex working\x07', 100)
|
||||
|
||||
expect(lateEvents).toEqual([])
|
||||
})
|
||||
|
||||
it('emits one batched event per chunk with facts in byte order and attribution', () => {
|
||||
const { runtime, batches } = createSideEffectRuntime()
|
||||
syncSinglePty(runtime)
|
||||
|
|
|
|||
|
|
@ -2725,6 +2725,10 @@ export class OrcaRuntimeService {
|
|||
private ptyController: RuntimePtyController | null = null
|
||||
private notifier: RuntimeNotifier | null = null
|
||||
private clientEventListeners = new Set<(event: RuntimeClientEvent) => void>()
|
||||
// Why: mobile subscribers discard terminalSideEffects; exclude them from batch delivery and production.
|
||||
private terminalSideEffectExcludedClientEventListeners = new Set<
|
||||
(event: RuntimeClientEvent) => void
|
||||
>()
|
||||
private nativeChatLaunchDraftResolutionByTabId = new Map<
|
||||
string,
|
||||
NativeChatLaunchDraftResolutionTombstone
|
||||
|
|
@ -4681,15 +4685,26 @@ export class OrcaRuntimeService {
|
|||
}
|
||||
}
|
||||
|
||||
onClientEvent(listener: (event: RuntimeClientEvent) => void): () => void {
|
||||
onClientEvent(
|
||||
listener: (event: RuntimeClientEvent) => void,
|
||||
options?: { consumesTerminalSideEffects?: boolean }
|
||||
): () => void {
|
||||
this.clientEventListeners.add(listener)
|
||||
if (options?.consumesTerminalSideEffects === false) {
|
||||
this.terminalSideEffectExcludedClientEventListeners.add(listener)
|
||||
}
|
||||
this.refreshTerminalSideEffectConsumerAvailability()
|
||||
return () => {
|
||||
this.clientEventListeners.delete(listener)
|
||||
this.terminalSideEffectExcludedClientEventListeners.delete(listener)
|
||||
this.refreshTerminalSideEffectConsumerAvailability()
|
||||
}
|
||||
}
|
||||
|
||||
private countTerminalSideEffectConsumingClientEventListeners(): number {
|
||||
return this.clientEventListeners.size - this.terminalSideEffectExcludedClientEventListeners.size
|
||||
}
|
||||
|
||||
getTerminalSleepClientEventSnapshot(): RuntimeClientEvent[] {
|
||||
const events: RuntimeClientEvent[] = []
|
||||
const sleepStates = [...this.terminalSleepStateByWorktreeId.values()].sort((a, b) =>
|
||||
|
|
@ -4747,9 +4762,25 @@ export class OrcaRuntimeService {
|
|||
}
|
||||
|
||||
private emitClientEvent(event: RuntimeClientEvent): void {
|
||||
// Why: mobile streams discard terminalSideEffects; skip excluded listeners so
|
||||
// paired phones never receive the per-OSC batch frames over the relay. Filtered
|
||||
// inside the delivery callback to keep live-Set iteration (a listener that
|
||||
// unsubscribes mid-fan-out must not be delivered to) and stay allocation-free.
|
||||
const skipExcluded =
|
||||
event.type === 'terminalSideEffects' &&
|
||||
this.terminalSideEffectExcludedClientEventListeners.size > 0
|
||||
// Why: a throwing subscriber here once escaped acquireWorktreeTerminalSpawn after it took the
|
||||
// per-worktree terminal mutation, leaking it and wedging that worktree's sleep until restart.
|
||||
notifyRuntimeListeners(this.clientEventListeners, (listener) => listener(event), 'client-event')
|
||||
notifyRuntimeListeners(
|
||||
this.clientEventListeners,
|
||||
(listener) => {
|
||||
if (skipExcluded && this.terminalSideEffectExcludedClientEventListeners.has(listener)) {
|
||||
return
|
||||
}
|
||||
listener(event)
|
||||
},
|
||||
'client-event'
|
||||
)
|
||||
}
|
||||
|
||||
notifyNativeChatLaunchDraftResolved(
|
||||
|
|
@ -9417,7 +9448,7 @@ export class OrcaRuntimeService {
|
|||
console.error('[runtime] terminal side-effect listener threw', { ptyId, err })
|
||||
}
|
||||
}
|
||||
if (this.clientEventListeners.size > 0) {
|
||||
if (this.countTerminalSideEffectConsumingClientEventListeners() > 0) {
|
||||
this.emitClientEvent({ type: 'terminalSideEffects', batch })
|
||||
}
|
||||
}
|
||||
|
|
@ -9585,33 +9616,23 @@ export class OrcaRuntimeService {
|
|||
this.retirePtyAgentLaunchAuthority(ptyId)
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: 'command-finished', exitCode })
|
||||
},
|
||||
// Why: headless serve still scans command completion to retire agent
|
||||
// launch authority; other transient facts remain desktop-only.
|
||||
...(this.terminalSideEffectConsumerAvailable
|
||||
? {
|
||||
onBell: () => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: 'bell' })
|
||||
},
|
||||
onPrLink: (link: TerminalGitHubPRLink) => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: 'pr-link', link })
|
||||
},
|
||||
// Why: hidden-delivery-gated views never see the bytes, so main
|
||||
// surfaces DECSET 2031 subscribes as facts; the theme reply is
|
||||
// still sent by the renderer (query authority stays with the view).
|
||||
onMode2031Subscribe: () => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: '2031-subscribe' })
|
||||
},
|
||||
// Why: the gated view never sees the withdrawal bytes either, so the
|
||||
// subscription registry it keeps for theme flips needs this fact to
|
||||
// stay truthful.
|
||||
onMode2031Unsubscribe: () => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: '2031-unsubscribe' })
|
||||
}
|
||||
}
|
||||
: {})
|
||||
onBell: () => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: 'bell' })
|
||||
},
|
||||
onPrLink: (link: TerminalGitHubPRLink) => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: 'pr-link', link })
|
||||
},
|
||||
// Why: hidden-delivery-gated views never see 2031 bytes; facts keep their theme registry truthful.
|
||||
onMode2031Subscribe: () => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: '2031-subscribe' })
|
||||
},
|
||||
onMode2031Unsubscribe: () => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: '2031-unsubscribe' })
|
||||
}
|
||||
},
|
||||
initialTitle !== null ? { initialTitle } : {}
|
||||
)
|
||||
tracker.setTransientSideEffectScanningEnabled(this.terminalSideEffectConsumerAvailable)
|
||||
const entry: RuntimePtyTitleTrackerEntry = {
|
||||
tracker,
|
||||
applyingChunk: false,
|
||||
|
|
@ -9624,15 +9645,7 @@ export class OrcaRuntimeService {
|
|||
// self-arms on the Command Code banner; the spawn command (when main
|
||||
// saw one) mirrors the renderer detector's startupCommand fast-arm.
|
||||
commandCodeDetector: this.terminalSideEffectConsumerAvailable
|
||||
? createCommandCodeOutputStatusDetector({
|
||||
startupCommand: this.terminalSpawnCommandsByPtyId.get(ptyId) ?? null,
|
||||
onWorking: (prompt) => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: 'command-code-working', prompt })
|
||||
},
|
||||
onDone: (prompt) => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: 'command-code-done', prompt })
|
||||
}
|
||||
})
|
||||
? this.createTerminalSideEffectCommandCodeDetector(ptyId)
|
||||
: null
|
||||
}
|
||||
this.ptyTitleTrackersByPtyId.set(ptyId, entry)
|
||||
|
|
@ -9752,18 +9765,34 @@ export class OrcaRuntimeService {
|
|||
|
||||
private refreshTerminalSideEffectConsumerAvailability(): void {
|
||||
const nextAvailable =
|
||||
this.terminalSideEffectLocalConsumerAvailable || this.clientEventListeners.size > 0
|
||||
this.terminalSideEffectLocalConsumerAvailable ||
|
||||
this.countTerminalSideEffectConsumingClientEventListeners() > 0
|
||||
if (nextAvailable === this.terminalSideEffectConsumerAvailable) {
|
||||
return
|
||||
}
|
||||
this.terminalSideEffectConsumerAvailable = nextAvailable
|
||||
// Why: optional bell/command/link scanners are selected when a tracker is
|
||||
// created. Rebuild at the window boundary so pure headless output stays cheap.
|
||||
for (const ptyId of [...this.ptyTitleTrackersByPtyId.keys()]) {
|
||||
this.disposePtyTitleTracker(ptyId)
|
||||
for (const [ptyId, entry] of this.ptyTitleTrackersByPtyId) {
|
||||
entry.tracker.setTransientSideEffectScanningEnabled(nextAvailable)
|
||||
entry.commandCodeDetector = nextAvailable
|
||||
? this.createTerminalSideEffectCommandCodeDetector(ptyId)
|
||||
: null
|
||||
}
|
||||
}
|
||||
|
||||
private createTerminalSideEffectCommandCodeDetector(
|
||||
ptyId: string
|
||||
): NonNullable<RuntimePtyTitleTrackerEntry['commandCodeDetector']> {
|
||||
return createCommandCodeOutputStatusDetector({
|
||||
startupCommand: this.terminalSpawnCommandsByPtyId.get(ptyId) ?? null,
|
||||
onWorking: (prompt) => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: 'command-code-working', prompt })
|
||||
},
|
||||
onDone: (prompt) => {
|
||||
this.recordTerminalSideEffectFact(ptyId, { kind: 'command-code-done', prompt })
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
private extractLastOsc7CwdForPty(
|
||||
ptyId: string,
|
||||
data: string
|
||||
|
|
|
|||
|
|
@ -0,0 +1,66 @@
|
|||
import { describe, expect, it, vi } from 'vitest'
|
||||
import type { RuntimeClientEvent } from '../../../../shared/runtime-client-events'
|
||||
import type { OrcaRuntimeService } from '../../orca-runtime'
|
||||
import { isStreamingMethod, type RpcContext, type RpcStreamingMethod } from '../core'
|
||||
// Why: importing client-events directly trips its module-init cycle through ipc/ssh; the index resolves it.
|
||||
import { ALL_RPC_METHODS } from './index'
|
||||
|
||||
const subscribeMethod = ALL_RPC_METHODS.find(
|
||||
(method) => method.name === 'runtime.clientEvents.subscribe' && isStreamingMethod(method)
|
||||
) as RpcStreamingMethod
|
||||
|
||||
function makeRuntime(): {
|
||||
runtime: OrcaRuntimeService
|
||||
onClientEvent: ReturnType<typeof vi.fn>
|
||||
cleanups: (() => void)[]
|
||||
} {
|
||||
const cleanups: (() => void)[] = []
|
||||
const onClientEvent = vi.fn(
|
||||
(
|
||||
_listener: (event: RuntimeClientEvent) => void,
|
||||
_options?: { consumesTerminalSideEffects?: boolean }
|
||||
) =>
|
||||
() => {}
|
||||
)
|
||||
const runtime = {
|
||||
onClientEvent,
|
||||
registerSubscriptionCleanup: (_id: string, cleanup: () => void) => {
|
||||
cleanups.push(cleanup)
|
||||
}
|
||||
} as unknown as OrcaRuntimeService
|
||||
return { runtime, onClientEvent, cleanups }
|
||||
}
|
||||
|
||||
describe('runtime.clientEvents.subscribe', () => {
|
||||
it('registers mobile subscriptions as non-consumers of terminal side effects', async () => {
|
||||
const { runtime, onClientEvent, cleanups } = makeRuntime()
|
||||
|
||||
const done = subscribeMethod.handler(
|
||||
undefined,
|
||||
{ runtime, connectionId: 'conn-1', clientKind: 'mobile' } as RpcContext,
|
||||
() => {}
|
||||
)
|
||||
|
||||
expect(onClientEvent).toHaveBeenCalledWith(expect.any(Function), {
|
||||
consumesTerminalSideEffects: false
|
||||
})
|
||||
cleanups.forEach((cleanup) => cleanup())
|
||||
await done
|
||||
})
|
||||
|
||||
it('keeps non-mobile subscriptions consuming terminal side effects', async () => {
|
||||
const { runtime, onClientEvent, cleanups } = makeRuntime()
|
||||
|
||||
const done = subscribeMethod.handler(
|
||||
undefined,
|
||||
{ runtime, connectionId: 'conn-1' } as RpcContext,
|
||||
() => {}
|
||||
)
|
||||
|
||||
expect(onClientEvent).toHaveBeenCalledWith(expect.any(Function), {
|
||||
consumesTerminalSideEffects: true
|
||||
})
|
||||
cleanups.forEach((cleanup) => cleanup())
|
||||
await done
|
||||
})
|
||||
})
|
||||
|
|
@ -16,11 +16,16 @@ export const CLIENT_EVENT_METHODS: readonly RpcAnyMethod[] = [
|
|||
defineStreamingMethod({
|
||||
name: 'runtime.clientEvents.subscribe',
|
||||
params: null,
|
||||
handler: async (_params, { runtime, connectionId }, emit) => {
|
||||
handler: async (_params, { runtime, connectionId, clientKind }, emit) => {
|
||||
await new Promise<void>((resolve) => {
|
||||
const unsubscribe = runtime.onClientEvent((event) => {
|
||||
emit(event)
|
||||
})
|
||||
// Why: mobile discards terminalSideEffects; excluding it stops the
|
||||
// per-OSC batch frames from crossing the relay.
|
||||
const unsubscribe = runtime.onClientEvent(
|
||||
(event) => {
|
||||
emit(event)
|
||||
},
|
||||
{ consumesTerminalSideEffects: clientKind !== 'mobile' }
|
||||
)
|
||||
|
||||
const seq = ++clientEventSubscriptionSeq
|
||||
const subscriptionId = `runtime-client-events-${connectionId ?? 'inproc'}-${seq}`
|
||||
|
|
|
|||
|
|
@ -98,6 +98,8 @@ export type TerminalTitleTracker = {
|
|||
* Titles are unaffected; un-suppressing resets the scanners' cross-chunk carry.
|
||||
*/
|
||||
setTransientFactScanningSuppressed: (suppressed: boolean) => void
|
||||
/** Enable consumer-only bell, PR-link, and mode-2031 scans without resetting title state. */
|
||||
setTransientSideEffectScanningEnabled: (enabled: boolean) => void
|
||||
/** Cancel the stale-title timer and clear accumulated tracker state. */
|
||||
dispose: () => void
|
||||
}
|
||||
|
|
@ -117,12 +119,13 @@ export function createTerminalTitleTracker(
|
|||
onMode2031Subscribe,
|
||||
onMode2031Unsubscribe
|
||||
} = callbacks
|
||||
const bellDetector = onBell ? createBellDetector() : null
|
||||
let bellDetector = onBell ? createBellDetector() : null
|
||||
// Why: created only when a consumer exists so headless serve never pays the per-chunk 133/URL scans.
|
||||
const commandFinishedScanner = onCommandFinished
|
||||
? createOsc133CommandFinishedScanner(onCommandFinished)
|
||||
: null
|
||||
let prLinkDetector = onPrLink ? createTerminalGitHubPRLinkDetector() : null
|
||||
let transientSideEffectScanningEnabled = true
|
||||
let transientFactScanningSuppressed = false
|
||||
let mode2031ReplyScanState = INITIAL_MODE_2031_REPLY_SCAN_STATE
|
||||
// Why: seed both so a mid-session tracker behaves as if it had observed the pane's last live title (renderer parity).
|
||||
|
|
@ -210,7 +213,7 @@ export function createTerminalTitleTracker(
|
|||
onPrLink?.(link)
|
||||
}
|
||||
}
|
||||
if (onMode2031Subscribe || onMode2031Unsubscribe) {
|
||||
if (transientSideEffectScanningEnabled && (onMode2031Subscribe || onMode2031Unsubscribe)) {
|
||||
const previousMode2031ReplyScanState = options.mode2031PendingSubscribe
|
||||
? { ...mode2031ReplyScanState, pendingSubscribe: true }
|
||||
: mode2031ReplyScanState
|
||||
|
|
@ -238,7 +241,11 @@ export function createTerminalTitleTracker(
|
|||
}
|
||||
}
|
||||
// The permission BEL rides outside the OSC title; a FRESH detector avoids touching the chunk detector's cross-chunk escape state.
|
||||
if (onBell && createBellDetector().chunkContainsBell(frame)) {
|
||||
if (
|
||||
transientSideEffectScanningEnabled &&
|
||||
onBell &&
|
||||
createBellDetector().chunkContainsBell(frame)
|
||||
) {
|
||||
onBell()
|
||||
}
|
||||
// Why: deliberately skip the 133/PR-link/2031 scanners — fabricated bytes contain none and must not perturb their cross-chunk carry.
|
||||
|
|
@ -271,6 +278,16 @@ export function createTerminalTitleTracker(
|
|||
}
|
||||
}
|
||||
},
|
||||
setTransientSideEffectScanningEnabled(enabled: boolean): void {
|
||||
if (enabled === transientSideEffectScanningEnabled) {
|
||||
return
|
||||
}
|
||||
transientSideEffectScanningEnabled = enabled
|
||||
bellDetector?.reset()
|
||||
bellDetector = enabled && onBell ? createBellDetector() : null
|
||||
prLinkDetector = enabled && onPrLink ? createTerminalGitHubPRLinkDetector() : null
|
||||
mode2031ReplyScanState = INITIAL_MODE_2031_REPLY_SCAN_STATE
|
||||
},
|
||||
dispose(): void {
|
||||
clearStaleTitleTimer()
|
||||
agentTracker?.reset()
|
||||
|
|
|
|||
Loading…
Reference in New Issue