fix: make the six registration methods atomic under a throwing change-listener (P1-1)
llm.registerAdapter, agents.register, sessions.create, systemPrompt.section, systemPrompt.tools, and tools.register each mutated state, emitted a change event, then returned the disposer. In Cordis a synchronous throw before the effect returns its disposer leaves nothing for the fiber to collect, so a throwing change-listener leaked the registry entry permanently — HMR/dispose could not clean it, and the duplicate-name/already-exists check stayed wedged until restart. Convert each to the generator-effect pattern already proven in AgentLoop.create: mutate state, `yield` the disposer that undoes it (collected before the next step runs, so it is torn down if a later step throws), THEN emit the change event. The existing duplicate-name throws are unchanged — they fire before any mutation, so they correctly leak nothing. No public API change: generator effects are still synchronous SyncEffects and register() keeps returning its fire-and-forget disposer wrapper. Tests: a listener-throw rollback test for all six methods — register with a change-listener that throws, assert the call throws AND the registry is clean (entry absent; a subsequent listener-free register of the same name succeeds and contributes exactly once). For systemPrompt (no duplicate-name check) the two tests assert assembly is clean. Verified each fails against the pre-fix emit-before-return-disposer form.
This commit is contained in:
@@ -140,17 +140,22 @@ export class ToolRegistry extends Service {
|
||||
* with the calling fiber. Emits `tools/change` on register/unregister.
|
||||
*/
|
||||
register(definition: ToolDefinition): () => void {
|
||||
const dispose = this.ctx.effect(() => {
|
||||
const dispose = this.ctx.effect(function* (this: ToolRegistry) {
|
||||
if (this.store.has(definition.name)) {
|
||||
throw new Error(`tool "${definition.name}" is already registered`)
|
||||
}
|
||||
this.store.set(definition.name, definition)
|
||||
this.ctx.emit('tools/change')
|
||||
return () => {
|
||||
// Yield the rollback BEFORE emitting `tools/change`: a generator effect
|
||||
// collects each yielded disposer before the next step runs, so a throwing
|
||||
// `tools/change` listener removes the tool instead of leaking it (a leak
|
||||
// would wedge the duplicate-name check until restart). The duplicate
|
||||
// throw above fires before any mutation — it leaks nothing.
|
||||
yield () => {
|
||||
this.store.delete(definition.name)
|
||||
this.ctx.emit('tools/change')
|
||||
}
|
||||
}, 'tools.register()')
|
||||
this.ctx.emit('tools/change')
|
||||
}.bind(this), 'tools.register()')
|
||||
// ctx.effect's disposer returns Promise<void>; our disposer API is
|
||||
// synchronous fire-and-forget — discard the (always-resolved) promise.
|
||||
return () => void dispose()
|
||||
|
||||
@@ -147,6 +147,27 @@ describe('ToolRegistry', () => {
|
||||
dispose()
|
||||
expect(ctx.tools.schemas().map(t => t.name)).toEqual(['echo'])
|
||||
})
|
||||
|
||||
it('rolls back the tool entry when a tools/change listener throws (P1-1)', async () => {
|
||||
const ctx = await setup()
|
||||
|
||||
let threw = false
|
||||
ctx.on('tools/change', () => {
|
||||
if (!threw) { threw = true; throw new Error('boom change listener') }
|
||||
})
|
||||
|
||||
// The throwing emit must roll the entry back, not leak it.
|
||||
expect(() => ctx.tools.register(echoTool)).toThrow('boom change listener')
|
||||
expect(ctx.tools.get('echo')).toBeUndefined() // rolled back, not leaked
|
||||
expect(ctx.tools.schemas()).toHaveLength(0)
|
||||
|
||||
// A subsequent listener-free register of the SAME name succeeds and is
|
||||
// exposed exactly once (the duplicate-name check is not wedged).
|
||||
const dispose = ctx.tools.register(echoTool)
|
||||
expect(ctx.tools.schemas().map(t => t.name)).toEqual(['echo'])
|
||||
dispose()
|
||||
expect(ctx.tools.get('echo')).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
describe('defineTool / schema DSL', () => {
|
||||
|
||||
Reference in New Issue
Block a user