mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
fix(runtime): keep the handle-gap verdict sweep inside its own environment
Reconciliation with nwparker/adv2-concurrency-fixes, which reviewed the tab-death
prune in de90e89 and found a composition hazard I had got wrong.
Row absence is TRANSIENT. During a rehydration a worktree's rows can be missing
for a frame and land again on the same connection generation. The prune swept
every environment, so an unrelated environment's deadline expiring during that
frame read it as "the pane is gone" and dropped a verdict that was still valid.
The pane then re-parked on a fresh full budget — the spurious-release-resets-the-
timer hazard that host-mirror-handle-gap-resume.test.ts warns about, in a new
place. Reproduced before fixing: A expires while B is briefly rowless, and B's
verdict reads false after B's rows return.
Both rules are now scoped to the recording environment. What licenses the
inference at all is that the caller's OWN row is published at this moment — the
deadline only records while its waiter is still parked — so "no row" means
retracted rather than not-yet-republished. That evidence covers this environment
and no other. It is the same inference `waiterIsReleased` already makes, so this
is the module's existing contract rather than a new assumption.
This does not weaken the leak fix: the 500-entry measurement was 500 panes on ONE
environment, which this still drains. It narrows what the sweep reaches, so the
churn loop now settles at one verdict per environment rather than one overall.
Removed environments are explicitly NOT this rule's job — they belong to a
teardown hook, which is adv2-concurrency-fixes' finding. The two are
complementary: theirs fires deterministically on removal, mine bounds tab churn
on an environment that never tears down. Neither subsumes the other.
Note for whoever merges the two: after this change the sweep is env-scoped, so
their comment's "does not drain" wording is accurate again — my earlier global
sweep would have drained their orphans opportunistically, and it no longer does.
Their inertness argument (a stale row cannot match, because removing an
environment advances its generation) holds either way.
Mutation-tested: widening the sweep back across environments fails the new
isolation test.
This commit is contained in:
@@ -95,6 +95,26 @@ describe('host mirror handle gap wait retention', () => {
|
||||
expect(hasHostMirrorHandleWaitExpired(ENV_A, 'tab-499')).toBe(true)
|
||||
})
|
||||
|
||||
it('never drops another environment verdict for a row that is only mid-rehydration', () => {
|
||||
// Row absence is transient: during a rehydration a worktree's rows can be missing for a
|
||||
// frame before landing again on the SAME generation. A sweep triggered by an unrelated
|
||||
// environment must not read that frame as "the pane is gone" — the verdict would vanish and
|
||||
// the pane would re-park on a fresh full budget, which is the spurious-release-resets-the-
|
||||
// timer hazard in a new place.
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1)
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(ENV_B, 1)
|
||||
setLiveTabs({ 'repo-1::wt-b': ['tab-b'], 'repo-1::wt-a': ['tab-a'] })
|
||||
parkAndExpire(ENV_B, 'repo-1::wt-b', 'tab-b')
|
||||
expect(hasHostMirrorHandleWaitExpired(ENV_B, 'tab-b')).toBe(true)
|
||||
|
||||
// B is briefly rowless while A's pane times out and records its own verdict.
|
||||
setLiveTabs({ 'repo-1::wt-a': ['tab-a'] })
|
||||
parkAndExpire(ENV_A, 'repo-1::wt-a', 'tab-a')
|
||||
|
||||
setLiveTabs({ 'repo-1::wt-b': ['tab-b'], 'repo-1::wt-a': ['tab-a'] })
|
||||
expect(hasHostMirrorHandleWaitExpired(ENV_B, 'tab-b')).toBe(true)
|
||||
})
|
||||
|
||||
it('drops an environment verdict once that environment reconnects', () => {
|
||||
setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1)
|
||||
setLiveTabs({ [WORKTREE]: ['tab-a', 'tab-b'] })
|
||||
@@ -152,7 +172,8 @@ describe('host mirror handle gap wait retention', () => {
|
||||
}
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0)
|
||||
expect(vi.getTimerCount()).toBe(0)
|
||||
// Only the final round's pane is still published.
|
||||
expect(countExpiredHostMirrorHandleGapVerdictsForTests()).toBe(1)
|
||||
// One per environment: each sweeps only its own rows, so each keeps the newest verdict it
|
||||
// recorded and nothing older. 400 rounds, 2 environments, 2 entries.
|
||||
expect(countExpiredHostMirrorHandleGapVerdictsForTests()).toBe(2)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -68,14 +68,19 @@ function recordExpiredWait(environmentId: string, key: string): void {
|
||||
// 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.
|
||||
// Why both rules stay inside this environment: the caller's own row is published right now
|
||||
// (the deadline only records while its waiter is parked), which is what makes "no row" mean
|
||||
// "retracted" rather than "not re-published yet" — the same inference `waiterIsReleased`
|
||||
// already makes. That evidence covers only this environment. Sweeping others would drop a
|
||||
// verdict belonging to an environment that is merely mid-rehydration, and its pane would
|
||||
// re-park on a fresh full budget. Removed environments are left to teardown, not to this.
|
||||
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)
|
||||
if (!staleKey.startsWith(prefix)) {
|
||||
continue
|
||||
}
|
||||
if (staleKey.startsWith(prefix) && staleGeneration !== generation) {
|
||||
if (!liveTabs.has(staleKey.slice(prefix.length)) || staleGeneration !== generation) {
|
||||
expiredGenerationByPane.delete(staleKey)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user