fix(llm,settings): refuse post-disposal route replacement and teardown notifications
Two lifecycle holes the registry and the consumer helper left open. `AdapterRegistrationHandle.replace` had no liveness guard: after the handle's disposer ran, a replace put routes back into the registry with nothing left to release them, so the adapter leaked permanently. `owned` being empty cannot carry that fact, because `replace([])` is the legal empty-section state, so the disposer records it explicitly. `installSettingsSection`'s watcher lacked the guard its own disposer carries: a stored change landing while the consumer unloads reached `onChange`, which re-registers routes against a fiber whose resources are being released. Also documents `withFileLock` in the atomic-write README (it claimed one export), records the age-based lock takeover as a known limitation, and lists ctx.settings and ctx.credentials in the architecture capability table.
This commit is contained in:
@@ -199,6 +199,10 @@ export interface AdapterRegistrationHandle {
|
||||
* one synchronous section, so no request can observe a gap. An empty array
|
||||
* is legal here (a settings section that emptied holds zero routes while
|
||||
* staying registered), unlike an empty initial registration.
|
||||
*
|
||||
* Throws `LlmError` with code `REGISTRATION_DISPOSED` once the registration
|
||||
* has been released: its routes are gone and its disposer has already run,
|
||||
* so anything registered afterwards would have no owner left to release it.
|
||||
* @param providers - the complete next route set for this registration.
|
||||
*/
|
||||
replace(providers: string[]): void
|
||||
@@ -227,10 +231,14 @@ export class LlmService extends Service {
|
||||
// The routes this registration currently holds; `replace` rewrites it, and
|
||||
// the disposer releases whatever it holds at disposal time.
|
||||
const owned = new Set<string>()
|
||||
// The disposer has run: `owned` being empty cannot say so on its own,
|
||||
// because `replace([])` legally leaves a live registration holding none.
|
||||
let released = false
|
||||
const dispose = this.ctx.effect(function* (this: LlmService) {
|
||||
if (providers.length === 0) throw new LlmError('an adapter must register at least one provider', 'INVALID_ADAPTER')
|
||||
this.commitRoutes(owned, this.prepareRoutes(providers, adapter, owned))
|
||||
yield () => {
|
||||
released = true
|
||||
for (const provider of owned) this.adapters.delete(provider)
|
||||
owned.clear()
|
||||
}
|
||||
@@ -239,6 +247,11 @@ export class LlmService extends Service {
|
||||
// synchronous fire-and-forget — discard the (always-resolved) promise.
|
||||
const handle = (() => void dispose()) as AdapterRegistrationHandle
|
||||
handle.replace = (next: string[]): void => {
|
||||
// Registering here would leak: the effect's disposer already ran, so
|
||||
// nothing remains to release whatever this call would put in the map.
|
||||
if (released) {
|
||||
throw new LlmError('a disposed adapter registration cannot replace its routes', 'REGISTRATION_DISPOSED')
|
||||
}
|
||||
this.commitRoutes(owned, this.prepareRoutes(next, adapter, owned))
|
||||
}
|
||||
return handle
|
||||
|
||||
@@ -1381,4 +1381,32 @@ describe('LlmService', () => {
|
||||
disposeAgain()
|
||||
expect(ctx.llm.listProviders()).toEqual([])
|
||||
})
|
||||
|
||||
it('refuses to replace routes on a registration that was already released', async () => {
|
||||
// The leak this prevents: the effect's disposer has run, so a route added
|
||||
// afterwards would sit in the registry with nothing left to release it.
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(LlmService)
|
||||
|
||||
const handle = ctx.llm.registerAdapter(['m1'], new ScriptedAdapter(SCRIPT))
|
||||
handle()
|
||||
expect(() => { handle.replace(['leaked']) })
|
||||
.toThrow(/disposed adapter registration cannot replace its routes/)
|
||||
expect(ctx.llm.listProviders()).toEqual([])
|
||||
})
|
||||
|
||||
it('still allows an empty route set on a live registration', async () => {
|
||||
// `replace([])` is the settings-section-emptied case: legal, and it must
|
||||
// not be mistaken for disposal by the guard above.
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(LlmService)
|
||||
|
||||
const handle = ctx.llm.registerAdapter(['m1'], new ScriptedAdapter(SCRIPT))
|
||||
handle.replace([])
|
||||
expect(ctx.llm.listProviders()).toEqual([])
|
||||
handle.replace(['m2'])
|
||||
expect(ctx.llm.listProviders()).toEqual([{ id: 'm2', name: 'm2' }])
|
||||
handle()
|
||||
expect(ctx.llm.listProviders()).toEqual([])
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user