mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
fix(runtime): a handle-gap verdict answers for its pane, not for the tab id
Taking ownership of the hazard three of us converged on and none closed: a tab id reused after retraction inherited a stale expired verdict, so the republished pane skipped its handle-gap wait entirely. Every other gap argued about on this map is conservative — drop a verdict, re-park, hold longer. This one is the #19735 direction, which is why it should not sit open. It is closed WITHOUT a fourth trigger, which is the point. Every trigger any owner of this map controls fires downstream of the moment this needs: the tab-death prune runs inside `recordExpiredWait`, so it acts on the next expiry in that environment and never fires if the id is reused before then — and once republished its predicate stops matching, because the tab is live again. A retraction trigger fails for the opposite reason: a transient rowless frame would drop a sibling's still-valid verdict, which is exactly what broke when the sweep was first widened. So this is not a prune at all. The verdict carries the environment-minted PTY binding the pane held AT PARK TIME, read through the same `parseRemoteRuntimePtyId` the liveness check uses, and `hasHostMirrorHandleWaitExpired` answers only for a pane that still holds it. A republished pane binds a newly minted PTY and gets its own wait; a genuine reattach to the same PTY inherits the verdict, which is correct, because the verdict follows the PTY rather than the tab id; a transient rowless frame touches neither, so the loop-breaker survives it. Read from the layout and not `ptyIdsByTabId` deliberately: during the handle gap the published-handle map is empty by definition — that is the gap — while the layout binding is what already licenses calling the pane unverifiable rather than dead. Park time, not expiry time. A layout rebind is not a release condition, so a pane replaced mid-wait leaves the original waiter running to term, and the verdict has to name the pane that actually did the waiting. Re-reading at expiry hands the new pane a wait it never served — a narrower window of the same bug. Mutations: ignoring the binding fails the reuse case; re-reading the binding at expiry instead of park time SURVIVED the first three cases and needed a fourth to kill it, which is the one that pins which moment the identity is captured at. Both now fail. NOTE FOR INTEGRATION: this changes the map's value from `number` to `{ generation, paneBinding }`. It is orthogonal to the combined sweep in `17d34da7aca` (that prunes entries; this adds a field and a read-time condition) but they touch adjacent lines in `recordExpiredWait` and will conflict textually. The sweep's two rules and their scopes are unaffected.
This commit is contained in:
Neil
@@ -10,6 +10,7 @@ import {
|
||||
clearRuntimeEnvironmentConnectionGenerationsForTests,
|
||||
setRuntimeEnvironmentConnectionGenerationForTests
|
||||
} from '@/store/slices/runtime-status'
|
||||
import { useAppStore } from '@/store'
|
||||
|
||||
/**
|
||||
* What the expired-verdict map actually retains, as opposed to what its comment claimed.
|
||||
@@ -26,6 +27,21 @@ describe('host-mirror handle-gap expired verdicts', () => {
|
||||
vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS + 1)
|
||||
}
|
||||
|
||||
/** The layout binding the liveness check reads — what identifies the pane behind a tab id. */
|
||||
const bindPane = (tabId: string, leafId: string, ptyId: string): void => {
|
||||
useAppStore.setState({
|
||||
terminalLayoutsByTabId: {
|
||||
...useAppStore.getState().terminalLayoutsByTabId,
|
||||
[tabId]: {
|
||||
root: { type: 'leaf', leafId },
|
||||
activeLeafId: leafId,
|
||||
expandedLeafId: null,
|
||||
ptyIdsByLeafId: { [leafId]: ptyId }
|
||||
} as never
|
||||
}
|
||||
} as never)
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers()
|
||||
resetHostMirrorHandleGapWaitsForTests()
|
||||
@@ -35,6 +51,7 @@ describe('host-mirror handle-gap expired verdicts', () => {
|
||||
afterEach(() => {
|
||||
resetHostMirrorHandleGapWaitsForTests()
|
||||
clearRuntimeEnvironmentConnectionGenerationsForTests()
|
||||
useAppStore.setState({ terminalLayoutsByTabId: {} } as never)
|
||||
vi.useRealTimers()
|
||||
})
|
||||
|
||||
@@ -85,26 +102,52 @@ describe('host-mirror handle-gap expired verdicts', () => {
|
||||
expect(countExpiredHostMirrorHandleGapVerdictsForTests()).toBe(2)
|
||||
})
|
||||
|
||||
// The residual hazard, recorded rather than fixed: the verdict is keyed on a tab id and is
|
||||
// deliberately sticky for the life of the connection, so a pane whose row is retracted keeps its
|
||||
// verdict. If the host ever republishes that same tab id on the same connection, the new pane
|
||||
// inherits "your wait already expired" and skips its own — which is the #19735 shape. Clearing on
|
||||
// re-park would remove the loop-breaker, so this is pinned as behaviour, not changed.
|
||||
// The verdict stays sticky across a retraction — that is the loop-breaker — but it answers for
|
||||
// the PANE it was about, not for whatever later holds the tab id. Tab ids are not unique over a
|
||||
// connection (`createTab` honours caller-supplied id hints, orphan adoption re-keys rows), and
|
||||
// a republished pane inheriting "your wait already expired" skips its own wait, which is the
|
||||
// #19735 direction and the one gap here that is not conservative.
|
||||
//
|
||||
// The tab-death prune does NOT close this, which both its author and I initially assumed it did;
|
||||
// they measured it and told us otherwise. The reason is the trigger, not the predicate: the prune
|
||||
// runs only inside `recordExpiredWait`, so it fires on the next expiry IN THAT ENVIRONMENT. Reuse
|
||||
// the id before then and the entry is never swept — and once the id is republished the predicate
|
||||
// stops matching it at all, because the tab is live again. So it is not even eventually
|
||||
// consistent for this case. Closing it needs a trigger that fires on row retraction itself.
|
||||
// Do not delete this test on the strength of that prune landing.
|
||||
it('keeps a retracted pane’s verdict, so a reused tab id inherits it', () => {
|
||||
// No prune can close it, which is what makes the read-time check the right shape: every trigger
|
||||
// on this map fires downstream of the moment it needs. The tab-death prune runs only inside
|
||||
// `recordExpiredWait`, so it acts on the next expiry in that environment — reuse the id before
|
||||
// then and it never fires, and once republished its predicate stops matching because the tab is
|
||||
// live again. A retraction trigger is unsafe for a different reason: a transient rowless frame
|
||||
// would drop a sibling's still-valid verdict.
|
||||
it('answers for the pane it was about, not for a new pane under the same tab id', () => {
|
||||
setRuntimeEnvironmentConnectionGenerationForTests('env-a', 1)
|
||||
bindPane('tab-a', 'leaf-1', 'remote:env-a@@term_1')
|
||||
parkUntilHostMirrorHandleLands('env-a', 'wt-1', 'tab-a', () => {})
|
||||
expire()
|
||||
expect(hasHostMirrorHandleWaitExpired('env-a', 'tab-a')).toBe(true)
|
||||
|
||||
parkUntilHostMirrorHandleLands('env-a', 'wt-1', 'tab-a', () => {})
|
||||
// The host retracts that pane and republishes a different one under the same id: a PTY it
|
||||
// newly minted. The verdict must not carry over — this pane has never waited.
|
||||
bindPane('tab-a', 'leaf-1', 'remote:env-a@@term_2')
|
||||
expect(hasHostMirrorHandleWaitExpired('env-a', 'tab-a')).toBe(false)
|
||||
|
||||
// A genuine reattach to the SAME pty inherits it, which is correct: the verdict follows the
|
||||
// PTY, not the tab id, and re-waiting on a pane that already gave up reopens the replay loop.
|
||||
bindPane('tab-a', 'leaf-1', 'remote:env-a@@term_1')
|
||||
expect(hasHostMirrorHandleWaitExpired('env-a', 'tab-a')).toBe(true)
|
||||
})
|
||||
|
||||
// The narrow window the case above does not reach: the pane is replaced BETWEEN park and expiry.
|
||||
// A layout rebind is not a release condition (`waiterIsReleased` watches the handle map and the
|
||||
// rows, not the binding), so the original waiter runs to term and records a verdict — and the
|
||||
// verdict has to name the pane that actually did the waiting. Reading the binding at expiry
|
||||
// instead of at park time attributes it to whoever holds the id by then, which hands the new
|
||||
// pane a wait it never served. Both are green without this case, so it is the one that pins
|
||||
// WHICH moment the identity is captured at.
|
||||
it('records the pane that waited, not whatever holds the tab id when the deadline fires', () => {
|
||||
setRuntimeEnvironmentConnectionGenerationForTests('env-a', 1)
|
||||
bindPane('tab-a', 'leaf-1', 'remote:env-a@@term_1')
|
||||
parkUntilHostMirrorHandleLands('env-a', 'wt-1', 'tab-a', () => {})
|
||||
|
||||
// Replaced mid-wait; nothing releases the waiter, so it still expires.
|
||||
bindPane('tab-a', 'leaf-1', 'remote:env-a@@term_2')
|
||||
expire()
|
||||
|
||||
expect(hasHostMirrorHandleWaitExpired('env-a', 'tab-a')).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { useAppStore } from '@/store'
|
||||
import { getRuntimeEnvironmentConnectionGeneration } from '@/store/slices/runtime-status'
|
||||
import { WEB_SESSION_TAB_RPC_TIMEOUT_MS } from '@/runtime/web-session-tab-rpc-timeout'
|
||||
import { parseRemoteRuntimePtyId } from '../../../shared/remote-runtime-pty-id'
|
||||
|
||||
/**
|
||||
* Per-pane park for the frame between a host's tab rows and its PTY handles.
|
||||
@@ -26,6 +27,8 @@ export const HOST_MIRROR_HANDLE_GAP_DEADLINE_MS = WEB_SESSION_TAB_RPC_TIMEOUT_MS
|
||||
type HandleGapWaiter = {
|
||||
worktreeId: string
|
||||
tabId: string
|
||||
/** Identifies the pane this wait is about; see ExpiredHandleGapVerdict. */
|
||||
paneBinding: string
|
||||
deadline: ReturnType<typeof setTimeout>
|
||||
run: () => void
|
||||
}
|
||||
@@ -43,28 +46,66 @@ const waitersByPane = new Map<string, HandleGapWaiter>()
|
||||
* re-asks about the same pane, and without a recorded verdict it would park, expire and replay
|
||||
* forever. So it is NOT cleared when the pane's row is retracted.
|
||||
*
|
||||
* The cost of that, and the reason it is written down: the key is a tab id, and a tab id is not
|
||||
* guaranteed unique over a connection — `createTab` honours caller-supplied id hints and orphan
|
||||
* adoption re-keys rows. A pane republished under a retired pane's tab id inherits "your wait
|
||||
* already expired" and skips its own wait, which is the #19735 shape. Fixing it by clearing on
|
||||
* re-park would remove the loop-breaker, so it is pinned in
|
||||
* host-mirror-handle-gap-wait-retention.test.ts rather than traded away.
|
||||
* Sticky is not the same as "applies to whatever later holds this tab id". The key is a tab id,
|
||||
* which is NOT unique over a connection — `createTab` honours caller-supplied id hints and orphan
|
||||
* adoption re-keys rows — so a pane republished under a retired pane's id used to inherit "your
|
||||
* wait already expired" and skip its own wait. That is the #19735 direction, and it is the one
|
||||
* gap on this map that is not conservative: every other one drops a verdict and re-parks, which
|
||||
* only ever holds longer.
|
||||
*
|
||||
* It is closed by identifying the PANE the verdict was about rather than by pruning, so no new
|
||||
* trigger is needed — which matters, because every trigger any owner of this map controls fires
|
||||
* downstream of the moment this hazard needs. The verdict carries the environment-minted PTY
|
||||
* binding the pane held when it parked, and only answers for a pane that still holds it:
|
||||
*
|
||||
* - a republished pane binds a PTY the host newly minted, so the binding differs and it gets its
|
||||
* own wait;
|
||||
* - a genuinely reattached pane holding the same PTY inherits the verdict, which is correct — the
|
||||
* verdict follows the PTY, not the tab id;
|
||||
* - a transient rowless frame does not touch the binding, so the verdict survives it. That is the
|
||||
* case that makes a retraction-triggered prune unsafe and this read-time check safe.
|
||||
*
|
||||
* Bounded by the panes that have parked AND expired on each environment's current connection;
|
||||
* every environment's rows are retired on its own next reconnect, by any expiry anywhere.
|
||||
*/
|
||||
const expiredGenerationByPane = new Map<string, number>()
|
||||
type ExpiredHandleGapVerdict = {
|
||||
generation: number
|
||||
/** Sorted environment-minted PTY ids the tab's leaves held at park time; '' when none. */
|
||||
paneBinding: string
|
||||
}
|
||||
const expiredGenerationByPane = new Map<string, ExpiredHandleGapVerdict>()
|
||||
let unsubscribeStore: (() => void) | null = null
|
||||
|
||||
function paneWaitKey(environmentId: string, tabId: string): string {
|
||||
return `${environmentId}\0${tabId}`
|
||||
}
|
||||
|
||||
/** True once the deadline fired for this pane on the current connection. */
|
||||
/**
|
||||
* The environment-minted PTY ids this tab's leaves are bound to, as one comparable string.
|
||||
*
|
||||
* Read from the layout, not `ptyIdsByTabId`: during the handle gap the published-handle map is
|
||||
* empty by definition — that is the gap — while the layout binding is what
|
||||
* `tabHoldsEnvironmentPtyBinding` already uses to call the pane unverifiable rather than dead.
|
||||
*/
|
||||
function paneBindingFor(tabId: string, environmentId: string): string {
|
||||
const bindings = useAppStore.getState().terminalLayoutsByTabId[tabId]?.ptyIdsByLeafId ?? {}
|
||||
return Object.values(bindings)
|
||||
.filter(
|
||||
(ptyId): ptyId is string =>
|
||||
typeof ptyId === 'string' && parseRemoteRuntimePtyId(ptyId)?.environmentId === environmentId
|
||||
)
|
||||
.sort()
|
||||
.join('')
|
||||
}
|
||||
|
||||
/** True once the deadline fired for THIS pane on the current connection. */
|
||||
export function hasHostMirrorHandleWaitExpired(environmentId: string, tabId: string): boolean {
|
||||
const verdict = expiredGenerationByPane.get(paneWaitKey(environmentId, tabId))
|
||||
return (
|
||||
expiredGenerationByPane.get(paneWaitKey(environmentId, tabId)) ===
|
||||
getRuntimeEnvironmentConnectionGeneration(environmentId)
|
||||
verdict !== undefined &&
|
||||
verdict.generation === getRuntimeEnvironmentConnectionGeneration(environmentId) &&
|
||||
// Why this and not the key alone: the key is a tab id, and the pane behind it can be replaced.
|
||||
verdict.paneBinding === paneBindingFor(tabId, environmentId)
|
||||
)
|
||||
}
|
||||
|
||||
@@ -74,13 +115,18 @@ function recordExpiredWait(environmentId: string, key: string): void {
|
||||
// removed environment, which by definition expires nothing again, retained its rows for the life
|
||||
// of the process. Each key names its own environment, so the generation it must be judged against
|
||||
// is readable from the key.
|
||||
for (const [staleKey, staleGeneration] of expiredGenerationByPane) {
|
||||
for (const [staleKey, stale] of expiredGenerationByPane) {
|
||||
const staleEnvironmentId = staleKey.slice(0, staleKey.indexOf('\0'))
|
||||
if (getRuntimeEnvironmentConnectionGeneration(staleEnvironmentId) !== staleGeneration) {
|
||||
if (getRuntimeEnvironmentConnectionGeneration(staleEnvironmentId) !== stale.generation) {
|
||||
expiredGenerationByPane.delete(staleKey)
|
||||
}
|
||||
}
|
||||
expiredGenerationByPane.set(key, getRuntimeEnvironmentConnectionGeneration(environmentId))
|
||||
// Why the waiter's park-time binding and not a fresh read: this verdict is about the pane whose
|
||||
// wait just ran out, and re-reading here would attribute it to whatever holds the id now.
|
||||
expiredGenerationByPane.set(key, {
|
||||
generation: getRuntimeEnvironmentConnectionGeneration(environmentId),
|
||||
paneBinding: waitersByPane.get(key)?.paneBinding ?? ''
|
||||
})
|
||||
}
|
||||
|
||||
function stopStoreSubscriptionIfIdle(): void {
|
||||
@@ -163,7 +209,13 @@ export function parkUntilHostMirrorHandleLands(
|
||||
recordExpiredWait(environmentId, key)
|
||||
releaseWaiter(key)
|
||||
}, HOST_MIRROR_HANDLE_GAP_DEADLINE_MS)
|
||||
waitersByPane.set(key, { worktreeId, tabId, deadline, run })
|
||||
waitersByPane.set(key, {
|
||||
worktreeId,
|
||||
tabId,
|
||||
paneBinding: paneBindingFor(tabId, environmentId),
|
||||
deadline,
|
||||
run
|
||||
})
|
||||
startStoreSubscription()
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user