mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
fix(runtime): prune a handle-gap verdict when its pane dies, not only on reconnect
`expiredGenerationByPane` was pruned by one rule: same environment, *different* connection generation. The generation only advances on reconnect, and a tab id is never reissued — so on a connection that never drops, every pane that ever hit the handle-gap deadline left an entry that nothing could ever remove. The module comment claimed the prune kept the map "bounded by the panes parked on the current connection". It did not. Measured directly: 500 park-and-expire cycles on one environment at a fixed generation leave 500 entries, one per closed tab, growing monotonically for as long as the session lives. Sizing, because this should not be mis-attributed to the open OOM report: an entry is a short string plus a number, so 100k tab-opens is roughly 10 MB, and reaching this path at all requires a mirrored pane that a local-only user never touches. It is a genuine unbounded-growth bug and it is not that OOM. The fix adds a second prune rule — drop any verdict whose tab id is no longer a published row — so the bound is live panes plus one. The scan is a Set built once per record, and records only happen on a deadline expiry, never on a frame. Both directions are tested, because over-pruning is its own defect: a verdict dropped while its pane is still live re-parks a wait that had already answered. Also pinned here: waiters and their deadline timers drain to zero (`vi.getTimerCount()`), and the store subscription's lifetime is exactly the waiters' — released early it would strand a parked pane forever. Mutation-tested: dropping the liveness rule fails the churn test 500 vs 1; making the prune unconditional fails the live-verdict test; unsubscribing on any waiter drain fails the subscription test. Carries a note on the sibling module: `hydratedGenerationByWorktree` has the same retention class (pre-existing, bounded by worktrees ever hydrated rather than by tab churn, so a far smaller ceiling) and is deliberately left alone.
This commit is contained in:
@@ -0,0 +1,158 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { useAppStore, type AppState } from '@/store'
|
||||
import {
|
||||
clearRuntimeEnvironmentConnectionGenerationsForTests,
|
||||
setRuntimeEnvironmentConnectionGenerationForTests
|
||||
} from '@/store/slices/runtime-status'
|
||||
import {
|
||||
HOST_MIRROR_HANDLE_GAP_DEADLINE_MS,
|
||||
countExpiredHostMirrorHandleGapVerdictsForTests,
|
||||
countParkedHostMirrorHandleGapPanesForTests,
|
||||
hasHostMirrorHandleWaitExpired,
|
||||
parkUntilHostMirrorHandleLands,
|
||||
resetHostMirrorHandleGapWaitsForTests
|
||||
} from './host-mirror-handle-gap-wait'
|
||||
|
||||
// What this pins: the two module-level maps here must return to baseline after churn.
|
||||
// `waitersByPane` is drained by its deadline, but the expired-verdict map was pruned ONLY
|
||||
// by connection generation — and a tab id is never reissued, so on a connection that never
|
||||
// drops (a session left open for days, the ordinary case) every pane that ever timed out
|
||||
// left a permanent entry. Both directions matter: under-pruning is the leak, and
|
||||
// over-pruning drops a live pane's verdict and re-parks a wait that already answered.
|
||||
|
||||
const ENV_A = 'env-retention-a'
|
||||
const ENV_B = 'env-retention-b'
|
||||
const WORKTREE = 'repo-1::worktree-retention'
|
||||
const initialAppStoreState = useAppStore.getState()
|
||||
|
||||
function setLiveTabs(tabIdsByWorktree: Record<string, string[]>): void {
|
||||
const tabsByWorktree: Record<string, unknown[]> = {}
|
||||
for (const [worktreeId, tabIds] of Object.entries(tabIdsByWorktree)) {
|
||||
tabsByWorktree[worktreeId] = tabIds.map((id) => ({ id, title: id, ptyId: null }))
|
||||
}
|
||||
useAppStore.setState({ tabsByWorktree, ptyIdsByTabId: {} } as unknown as AppState)
|
||||
}
|
||||
|
||||
/** Parks a pane and lets its deadline fire, which is what records an expired verdict. */
|
||||
function parkAndExpire(environmentId: string, worktreeId: string, tabId: string): void {
|
||||
parkUntilHostMirrorHandleLands(environmentId, worktreeId, tabId, () => {})
|
||||
vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS + 1)
|
||||
}
|
||||
|
||||
describe('host mirror handle gap wait retention', () => {
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers()
|
||||
useAppStore.setState(initialAppStoreState, true)
|
||||
resetHostMirrorHandleGapWaitsForTests()
|
||||
clearRuntimeEnvironmentConnectionGenerationsForTests()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
resetHostMirrorHandleGapWaitsForTests()
|
||||
clearRuntimeEnvironmentConnectionGenerationsForTests()
|
||||
vi.useRealTimers()
|
||||
})
|
||||
|
||||
it('drains every waiter and its deadline timer once the budget expires', () => {
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1)
|
||||
setLiveTabs({ [WORKTREE]: Array.from({ length: 50 }, (_, i) => `tab-${i}`) })
|
||||
for (let index = 0; index < 50; index += 1) {
|
||||
parkUntilHostMirrorHandleLands(ENV_A, WORKTREE, `tab-${index}`, () => {})
|
||||
}
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(50)
|
||||
expect(vi.getTimerCount()).toBe(50)
|
||||
|
||||
vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS + 1)
|
||||
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0)
|
||||
expect(vi.getTimerCount()).toBe(0)
|
||||
})
|
||||
|
||||
it('keeps every live pane verdict on the current connection', () => {
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1)
|
||||
setLiveTabs({ [WORKTREE]: ['tab-a', 'tab-b', 'tab-c'] })
|
||||
parkAndExpire(ENV_A, WORKTREE, 'tab-a')
|
||||
parkAndExpire(ENV_A, WORKTREE, 'tab-b')
|
||||
parkAndExpire(ENV_A, WORKTREE, 'tab-c')
|
||||
|
||||
// Every one of these rows is still published, so every verdict is still answerable.
|
||||
expect(hasHostMirrorHandleWaitExpired(ENV_A, 'tab-a')).toBe(true)
|
||||
expect(hasHostMirrorHandleWaitExpired(ENV_A, 'tab-b')).toBe(true)
|
||||
expect(hasHostMirrorHandleWaitExpired(ENV_A, 'tab-c')).toBe(true)
|
||||
expect(countExpiredHostMirrorHandleGapVerdictsForTests()).toBe(3)
|
||||
})
|
||||
|
||||
it('does not retain an expired verdict for a tab that closed on the same connection', () => {
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1)
|
||||
// A long-lived connection: the generation never advances while tabs churn.
|
||||
for (let index = 0; index < 500; index += 1) {
|
||||
setLiveTabs({ [WORKTREE]: [`tab-${index}`] })
|
||||
parkAndExpire(ENV_A, WORKTREE, `tab-${index}`)
|
||||
}
|
||||
// Only the one pane still published may hold a verdict; the other 499 tab ids are
|
||||
// closed and will never be reissued.
|
||||
expect(countExpiredHostMirrorHandleGapVerdictsForTests()).toBe(1)
|
||||
expect(hasHostMirrorHandleWaitExpired(ENV_A, 'tab-499')).toBe(true)
|
||||
})
|
||||
|
||||
it('drops an environment verdict once that environment reconnects', () => {
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1)
|
||||
setLiveTabs({ [WORKTREE]: ['tab-a', 'tab-b'] })
|
||||
parkAndExpire(ENV_A, WORKTREE, 'tab-a')
|
||||
expect(hasHostMirrorHandleWaitExpired(ENV_A, 'tab-a')).toBe(true)
|
||||
|
||||
// Both rows survive the reconnect, so only the generation rule may drop tab-a.
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 2)
|
||||
expect(hasHostMirrorHandleWaitExpired(ENV_A, 'tab-a')).toBe(false)
|
||||
parkAndExpire(ENV_A, WORKTREE, 'tab-b')
|
||||
expect(countExpiredHostMirrorHandleGapVerdictsForTests()).toBe(1)
|
||||
})
|
||||
|
||||
it('holds exactly one store subscription for exactly as long as a waiter exists', () => {
|
||||
let active = 0
|
||||
const realSubscribe = useAppStore.subscribe.bind(useAppStore)
|
||||
vi.spyOn(useAppStore, 'subscribe').mockImplementation(((listener: never) => {
|
||||
active += 1
|
||||
const unsubscribe = realSubscribe(listener)
|
||||
return () => {
|
||||
active -= 1
|
||||
unsubscribe()
|
||||
}
|
||||
}) as never)
|
||||
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1)
|
||||
setLiveTabs({ [WORKTREE]: ['tab-a', 'tab-b'] })
|
||||
expect(active).toBe(0)
|
||||
|
||||
parkUntilHostMirrorHandleLands(ENV_A, WORKTREE, 'tab-a', () => {})
|
||||
parkUntilHostMirrorHandleLands(ENV_A, WORKTREE, 'tab-b', () => {})
|
||||
expect(active).toBe(1)
|
||||
|
||||
// The handle lands for one pane. The other is still parked, so releasing the
|
||||
// subscription here would strand it: nothing would ever notice its handle.
|
||||
useAppStore.setState({ ptyIdsByTabId: { 'tab-a': ['pty-a'] } } as unknown as AppState)
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(1)
|
||||
expect(active).toBe(1)
|
||||
|
||||
vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS + 1)
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0)
|
||||
expect(active).toBe(0)
|
||||
expect(vi.getTimerCount()).toBe(0)
|
||||
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
it('returns both maps to baseline across a full connect/churn/disconnect loop', () => {
|
||||
for (let round = 0; round < 400; round += 1) {
|
||||
const environmentId = round % 2 === 0 ? ENV_A : ENV_B
|
||||
const worktreeId = `repo-1::worktree-${round}`
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(environmentId, round + 1)
|
||||
setLiveTabs({ [worktreeId]: [`tab-${round}`] })
|
||||
parkAndExpire(environmentId, worktreeId, `tab-${round}`)
|
||||
}
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0)
|
||||
expect(vi.getTimerCount()).toBe(0)
|
||||
// Only the final round's pane is still published.
|
||||
expect(countExpiredHostMirrorHandleGapVerdictsForTests()).toBe(1)
|
||||
})
|
||||
})
|
||||
@@ -52,12 +52,29 @@ export function hasHostMirrorHandleWaitExpired(environmentId: string, tabId: str
|
||||
)
|
||||
}
|
||||
|
||||
function liveTabIds(): Set<string> {
|
||||
const tabIds = new Set<string>()
|
||||
for (const tabs of Object.values(useAppStore.getState().tabsByWorktree)) {
|
||||
for (const tab of tabs) {
|
||||
tabIds.add(tab.id)
|
||||
}
|
||||
}
|
||||
return tabIds
|
||||
}
|
||||
|
||||
function recordExpiredWait(environmentId: string, key: string): void {
|
||||
const generation = getRuntimeEnvironmentConnectionGeneration(environmentId)
|
||||
// Why: a verdict from a previous connection is dead weight; drop it so the map
|
||||
// stays bounded by the panes parked on the current connection.
|
||||
// Why two prune rules: a verdict from a previous connection is dead weight, and so is
|
||||
// one for a pane whose row is gone. Generation alone does not bound the map — a tab id
|
||||
// is never reissued, so a connection that never drops (the ordinary case for a session
|
||||
// left open for days) kept one entry for every pane that ever timed out.
|
||||
const prefix = `${environmentId}\0`
|
||||
const liveTabs = liveTabIds()
|
||||
for (const [staleKey, staleGeneration] of expiredGenerationByPane) {
|
||||
if (!liveTabs.has(staleKey.slice(staleKey.indexOf('\0') + 1))) {
|
||||
expiredGenerationByPane.delete(staleKey)
|
||||
continue
|
||||
}
|
||||
if (staleKey.startsWith(prefix) && staleGeneration !== generation) {
|
||||
expiredGenerationByPane.delete(staleKey)
|
||||
}
|
||||
@@ -153,6 +170,10 @@ export function countParkedHostMirrorHandleGapPanesForTests(): number {
|
||||
return waitersByPane.size
|
||||
}
|
||||
|
||||
export function countExpiredHostMirrorHandleGapVerdictsForTests(): number {
|
||||
return expiredGenerationByPane.size
|
||||
}
|
||||
|
||||
export function resetHostMirrorHandleGapWaitsForTests(): void {
|
||||
for (const waiter of waitersByPane.values()) {
|
||||
clearTimeout(waiter.deadline)
|
||||
|
||||
@@ -14,6 +14,10 @@ import { getRuntimeEnvironmentConnectionGeneration } from '@/store/slices/runtim
|
||||
type ParkedMirrorWaiter = { environmentId: string; worktreeId: string; run: () => void }
|
||||
|
||||
const hydratedGenerationByEnvironment = new Map<string, number>()
|
||||
/** Pruned only by `clearHostSessionMirrorHydration`, so a worktree deleted while its environment
|
||||
* stays connected keeps its entry — the same retention class as the handle-gap verdict map
|
||||
* (host-mirror-handle-gap-wait.ts), which prunes on tab death for exactly this reason. Bounded by
|
||||
* worktrees ever hydrated, not by tab churn, so it is a far smaller ceiling and left as-is. */
|
||||
const hydratedGenerationByWorktree = new Map<string, number>()
|
||||
const parkedWaitersByWorktree = new Map<string, ParkedMirrorWaiter>()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user