mirror of
https://github.com/stablyai/orca.git
synced 2026-09-26 00:02:34 +00:00
fix(native-chat): stop the router reporting a stop it never observed
`closeAll` cleared the route table and set one boolean, after which that boolean was the only surviving evidence about any session. Two call sites then spent it: `releaseAcquisition` and the stop path each turned a route-lookup MISS into reported success. Eviction reads a `true` from the stop path as proof the provider child is gone and releases the durable lease on it, so a session the router never routed could have its lease handed back on the strength of "I have no record, but everything is closed." Loss of contact is not evidence of process death. The fix keeps the evidence instead of the inference: adapter shutdown only resolves once every child is proven stopped, so `closeAll` now marks each routed session `stopped` rather than forgetting it. A routed session still answers `true` from its own retained proof; a session with no route answers `false`, which leaves it indexed for a real retry. `releaseAcquisition` drops its short-circuit and asks the adapters, which answer from their own session maps. The acquire-side latch is unchanged: once closed, the router stays closed and refuses new work. Behaviour that changed: a post-`closeAll` stop for a session the router never routed, or one the host already acknowledged as released, now reports unproven instead of proven. That matches what the same call already answered before `closeAll`, and no real flow reaches it — quit evicts every owned session before `closeAll` runs, and eviction only asks the adapter for sessions whose provider child this host acquired through the router.
This commit is contained in:
+22
-5
@@ -187,7 +187,7 @@ describe('StructuredAgentSessionAdapterRouter.closeAll', () => {
|
||||
expect(acquire).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('publishes one global close proof without retaining session ids', async () => {
|
||||
it('keeps a per-session stop proof and reports no stop for a session it never routed', async () => {
|
||||
const claude = adapterOf(vi.fn(async () => true))
|
||||
const closeAdapters = vi.fn(async () => undefined)
|
||||
const router = new StructuredAgentSessionAdapterRouter(
|
||||
@@ -202,14 +202,30 @@ describe('StructuredAgentSessionAdapterRouter.closeAll', () => {
|
||||
|
||||
await router.closeAll()
|
||||
|
||||
// The routed session carries the shutdown's own exit proof; the other two are sessions this
|
||||
// router has no record of, and an absent record is not a stop it can report.
|
||||
await expect(router.closeSession('session-1')).resolves.toBe(true)
|
||||
await expect(router.closeSession('never-routed')).resolves.toBe(true)
|
||||
await expect(router.closeSession('never-routed')).resolves.toBe(false)
|
||||
router.acknowledgeSessionRelease('session-1')
|
||||
await expect(router.closeSession('session-1')).resolves.toBe(true)
|
||||
await expect(router.closeSession('session-1')).resolves.toBe(false)
|
||||
await router.closeAll()
|
||||
expect(closeAdapters).toHaveBeenCalledOnce()
|
||||
})
|
||||
|
||||
it('asks the adapters to release an unrouted session rather than answering from the close proof', async () => {
|
||||
const claudeRelease = vi.fn(async () => true)
|
||||
const codexRelease = vi.fn(async () => false)
|
||||
const router = new StructuredAgentSessionAdapterRouter(
|
||||
{ claude: adapterOf(claudeRelease), codex: adapterOf(codexRelease) },
|
||||
async () => undefined
|
||||
)
|
||||
await router.closeAll()
|
||||
|
||||
await expect(router.releaseAcquisition({ sessionId: 'never-routed' })).resolves.toBe(true)
|
||||
expect(claudeRelease).toHaveBeenCalledWith({ sessionId: 'never-routed' })
|
||||
expect(codexRelease).toHaveBeenCalledWith({ sessionId: 'never-routed' })
|
||||
})
|
||||
|
||||
it('retains live routes and publishes no global proof when closeAll fails', async () => {
|
||||
const failure = new Error('adapter shutdown failed')
|
||||
const claude = adapterOf(vi.fn(async () => true))
|
||||
@@ -268,9 +284,10 @@ describe('StructuredAgentSessionAdapterRouter.closeAll', () => {
|
||||
await router.closeAll()
|
||||
resolveAcquire(acquisition(2, 'spawn-2'))
|
||||
|
||||
// The route is NOT published behind a closed adapter, so nothing routes back out to it.
|
||||
// The route is NOT published behind a closed adapter, so nothing routes back out to it — and
|
||||
// with no route the router has nothing to stop and no stop to report.
|
||||
await expect(acquiring).rejects.toThrow('router is closed')
|
||||
await expect(router.closeSession('session-1')).resolves.toBe(true)
|
||||
await expect(router.closeSession('session-1')).resolves.toBe(false)
|
||||
expect(closeSession).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -51,9 +51,6 @@ export class StructuredAgentSessionAdapterRouter implements StructuredAgentSessi
|
||||
this.routes.delete(input.sessionId)
|
||||
}
|
||||
}
|
||||
if (this.allAdaptersClosed) {
|
||||
return true
|
||||
}
|
||||
let released = false
|
||||
for (const candidate of Object.values(this.adapters)) {
|
||||
released = (await candidate.releaseAcquisition?.(input)) === true || released
|
||||
@@ -145,7 +142,10 @@ export class StructuredAgentSessionAdapterRouter implements StructuredAgentSessi
|
||||
): Promise<boolean> {
|
||||
const route = this.routes.get(sessionId)
|
||||
if (!route) {
|
||||
return this.allAdaptersClosed
|
||||
// No route is loss of contact, never proof of a stop. Answering `true` here would hand a
|
||||
// caller a receipt for a session this router never acted on — and the caller spends that
|
||||
// receipt by releasing the durable lease.
|
||||
return false
|
||||
}
|
||||
if (route.state === 'stopped') {
|
||||
return true
|
||||
@@ -169,7 +169,13 @@ export class StructuredAgentSessionAdapterRouter implements StructuredAgentSessi
|
||||
this.closePromise = (async () => {
|
||||
try {
|
||||
await this.closeAdapters()
|
||||
this.routes.clear()
|
||||
// Adapter shutdown only resolves once every child is PROVEN stopped, so each routed
|
||||
// session inherits that proof and keeps it per session. Clearing the map instead would
|
||||
// leave one boolean as the only surviving evidence, and an empty map cannot tell a
|
||||
// session this router stopped from one it never saw.
|
||||
for (const route of this.routes.values()) {
|
||||
route.state = 'stopped'
|
||||
}
|
||||
this.allAdaptersClosed = true
|
||||
} finally {
|
||||
this.closePromise = null
|
||||
|
||||
Reference in New Issue
Block a user