mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 16:03:49 +00:00
fix(windows): stamp the provider cache on every confirmation, not on change
My own previous commit was wrong, and wrong in the direction #9258 exists to prevent. Review caught it; the test in this commit reproduces it first. I stamped ptyLastRecognizedForegroundAt only when the recognized name CHANGED. That makes the value the time of first recognition, so the age measures how long the agent has been running rather than how long since we last confirmed it. For a live agent recognized as the same name every cycle the stamp never moved, the age crossed 30s and stayed there, and the short-circuit died permanently. Two consequences, the second serious: - every getForegroundProcess call on a >30s-old agent pane ran the whole-table scan, defeating the exact optimization the branch exists for; - with the short-circuit off, one available-but-agentless snapshot was enough to delete a LIVE agent's identity, because paneMembershipUnavailable is false in this state so the degraded-scan substitution does not engage. That is the false "agent done" this code's own comment warns about. The daemon path was already right -- it re-stamps refreshedAt on every positive recognition -- so the same constant meant two different things in the two files. Now both mean "time since we last saw the agent", which turns the bound from "disable the short-circuit after 30s" into "force one revalidating scan every 30s": ~16-31ms per pane per 30s via the native process table. Test asserts the scan count stops incrementing after the revalidation, and fails against the stamp-on-change form. Also correct the shared docstring, which had dropped the invariant the whole design rests on, and stop calling this a WSL bug: the trigger is a persistent console-detached job member plus a fallback that reads as a shell. wsl.exe is not in SHELL_NAMES, so a plain WSL pane does not even reach this code -- WSL is just where it was measured.
This commit is contained in:
@@ -281,6 +281,43 @@ describe('LocalPtyProvider', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('keeps short-circuiting a live agent instead of scanning on every call', async () => {
|
||||
// The age bound must mean "time since we last confirmed the agent", not
|
||||
// "how long the agent has run". Stamping only on a CHANGE of name makes a
|
||||
// steadily-recognized agent age out permanently: the short-circuit dies
|
||||
// after 30s, every call runs the whole-table scan, and one available-but-
|
||||
// agentless snapshot then deletes a LIVE agent -- the false "agent done"
|
||||
// #9258 exists to prevent.
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
|
||||
mockProc.process = 'powershell.exe'
|
||||
resolveAgentForegroundProcessMock.mockResolvedValue({
|
||||
available: true,
|
||||
processName: 'claude'
|
||||
})
|
||||
readWindowsPtyJobProcessIdsMock.mockReturnValue(new Set([12345, 999]))
|
||||
vi.useFakeTimers({ toFake: ['Date'] })
|
||||
try {
|
||||
vi.setSystemTime(1_000_000)
|
||||
const { id } = await provider.spawn({ cols: 80, rows: 24 })
|
||||
|
||||
await expect(provider.getForegroundProcess(id)).resolves.toBe('claude')
|
||||
expect(resolveAgentForegroundProcessMock).toHaveBeenCalledTimes(1)
|
||||
|
||||
// Past the bound: one revalidating scan is expected, and it re-confirms.
|
||||
vi.setSystemTime(1_000_000 + 40_000)
|
||||
await expect(provider.getForegroundProcess(id)).resolves.toBe('claude')
|
||||
expect(resolveAgentForegroundProcessMock).toHaveBeenCalledTimes(2)
|
||||
|
||||
// That confirmation must restart the clock, so the short-circuit resumes.
|
||||
vi.setSystemTime(1_000_000 + 45_000)
|
||||
await expect(provider.getForegroundProcess(id)).resolves.toBe('claude')
|
||||
await expect(provider.getForegroundProcess(id)).resolves.toBe('claude')
|
||||
expect(resolveAgentForegroundProcessMock).toHaveBeenCalledTimes(2)
|
||||
} finally {
|
||||
vi.useRealTimers()
|
||||
}
|
||||
})
|
||||
|
||||
it('falls through to the scan when the job holds only the shell', async () => {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
|
||||
mockProc.process = 'powershell.exe'
|
||||
|
||||
@@ -1473,9 +1473,10 @@ export class LocalPtyProvider implements IPtyProvider {
|
||||
lastRecognizedAgent
|
||||
)
|
||||
if (stable.lastRecognizedAgent) {
|
||||
if (ptyLastRecognizedForeground.get(id) !== stable.lastRecognizedAgent) {
|
||||
ptyLastRecognizedForegroundAt.set(id, Date.now())
|
||||
}
|
||||
// Every confirmation restarts the clock: the bound means "time since we
|
||||
// last saw the agent", not "how long it has run". Stamping only on a
|
||||
// change would age out a live agent that keeps being recognized.
|
||||
ptyLastRecognizedForegroundAt.set(id, Date.now())
|
||||
ptyLastRecognizedForeground.set(id, stable.lastRecognizedAgent)
|
||||
} else {
|
||||
ptyLastRecognizedForeground.delete(id)
|
||||
|
||||
@@ -5,9 +5,16 @@ import { isShellProcess } from '../../shared/shell-process-detection'
|
||||
*
|
||||
* The job is a SUPERSET of the console: it keeps console-detached descendants,
|
||||
* so "something besides the shell is alive" cannot tell a working agent from a
|
||||
* leftover. A WSL pane always has some, so treating that as proof of life pins
|
||||
* a dead agent's name forever (#9258's bug, reached by a new route). Age is the
|
||||
* tiebreak. 5x the renderer's 6s confirm ladder.
|
||||
* leftover. Any pane that keeps one -- and whose fallback name reads as a shell
|
||||
* -- would otherwise pin a dead agent's name forever (#9258's bug, reached by a
|
||||
* new route). Age is the tiebreak.
|
||||
*
|
||||
* The invariant both callers must preserve: every successful recognition resets
|
||||
* the clock, so this can only expire an identity no scan has confirmed for this
|
||||
* long. It is never a timeout on a live agent.
|
||||
*
|
||||
* 5x the renderer's 6s confirm ladder, and bounded above by the fact that a
|
||||
* stale cache also pins the refresh at the 1s TTL until it clears.
|
||||
*/
|
||||
export const WINDOWS_DETACHED_DESCENDANT_IDENTITY_MAX_AGE_MS = 30_000
|
||||
|
||||
|
||||
Reference in New Issue
Block a user