-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(agent-core-v2): keep agent lifecycle context active through scope teardown #3206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0da99b7
7b88c60
b7c6d0f
1bbb48a
48e0d27
8f6f9ca
8905a60
6a601d6
55e2566
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@moonshot-ai/kimi-code": patch | ||
| --- | ||
|
|
||
| Fix repeated server crashes when resuming a session that was interrupted in the middle of a turn. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -487,6 +487,10 @@ export class InstantiationService implements IInstantiationService { | |
| return this._ledger.register(disposer, label); | ||
| } | ||
|
|
||
| anchorKernelFinalizer(disposer: Disposer, label: string): LedgerEntry { | ||
| return this._ledger.registerFinalizer(disposer, label); | ||
| } | ||
|
|
||
| private _getFiberHost(): FiberHost { | ||
| this._fiberHost ??= { | ||
| mintUid: () => ++this._root()._nextUnitUid, | ||
|
|
@@ -648,18 +652,31 @@ export class InstantiationService implements IInstantiationService { | |
| return new InstantiationService(services, this._strict, this, this._enableTracing); | ||
| } | ||
|
|
||
| private _disposePromise: Promise<void> | undefined; | ||
|
|
||
| dispose(): void { | ||
| void this.disposeAsync(); | ||
| } | ||
|
|
||
| disposeAsync(): Promise<void> { | ||
| this._disposePromise ??= this.disposeCore(); | ||
| return this._disposePromise; | ||
| } | ||
|
|
||
| private disposeCore(): Promise<void> { | ||
| if (this._disposed) { | ||
| return; | ||
| return Promise.resolve(); | ||
| } | ||
| this._disposed = true; | ||
|
|
||
| const childTeardowns: Promise<void>[] = []; | ||
| let teardown: void | Promise<void> = undefined; | ||
| try { | ||
| for (const child of Array.from(this._children)) { | ||
| child.dispose(); | ||
| childTeardowns.push(child.disposeAsync()); | ||
| } | ||
| this._children.clear(); | ||
| void this._ledger.teardown('scope-close'); | ||
| teardown = this._ledger.teardown('scope-close'); | ||
|
Comment on lines
675
to
+679
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in this revision is that AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L24-L27 Useful? React with 👍 / 👎. |
||
| this._services.dispose(); | ||
| this.cascade.dispose(); | ||
| for (const view of this._collectionViews.values()) { | ||
|
|
@@ -674,6 +691,7 @@ export class InstantiationService implements IInstantiationService { | |
| this._parent._children.delete(this); | ||
| } | ||
| } | ||
| return Promise.all([...childTeardowns, Promise.resolve(teardown)]).then(() => undefined); | ||
| } | ||
|
|
||
| private _createInstance<T>(ctor: any, args: unknown[], _trace: Trace, unit?: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ export function watchScopeUnits(container: InstantiationService, kind: ScopeKind | |
| const foldLedger = new Ledger(`scope-units:${kind}`); | ||
| container.anchorKernelEntry((reason) => foldLedger.teardown(reason), `scope-units:${kind}`); | ||
|
|
||
| const materialized = new Map<number, () => void>(); | ||
| const materialized = new Map<number, () => void | Promise<void>>(); | ||
|
|
||
| const materialize = (record: StoredRecord): void => { | ||
| const recipe = record.value as ServiceRecipe; | ||
|
|
@@ -32,7 +32,7 @@ export function watchScopeUnits(container: InstantiationService, kind: ScopeKind | |
| if (isClassRecipe(recipe)) { | ||
| const instance = host.constructService(recipe, undefined) as Partial<IDisposable>; | ||
| unitLedger.register(() => { | ||
| instance.dispose?.(); | ||
| return instance.dispose?.(); | ||
| }, `unit:${name}`); | ||
| } else { | ||
| const facade = new FiberRuntime( | ||
|
|
@@ -57,23 +57,23 @@ export function watchScopeUnits(container: InstantiationService, kind: ScopeKind | |
| } | ||
|
|
||
| let retracted = false; | ||
| const retract = (): void => { | ||
| const retract = (): void | Promise<void> => { | ||
| if (retracted) { | ||
| return; | ||
| return undefined; | ||
| } | ||
| retracted = true; | ||
| materialized.delete(record.id); | ||
| void unitLedger.teardown('unload'); | ||
| return unitLedger.teardown('unload'); | ||
| }; | ||
| if (!record.providerBook.isActive) { | ||
| retract(); | ||
| void retract(); | ||
| return; | ||
| } | ||
| record.providerBook.register(() => { | ||
| retract(); | ||
| void retract(); | ||
| }, `scope-units:${kind}`); | ||
| foldLedger.register(() => { | ||
| retract(); | ||
| return retract(); | ||
| }, `record:${name}`); | ||
| materialized.set(record.id, retract); | ||
| }; | ||
|
|
@@ -92,7 +92,7 @@ export function watchScopeUnits(container: InstantiationService, kind: ScopeKind | |
| } | ||
| for (const [id, retract] of Array.from(materialized)) { | ||
| if (!seen.has(id)) { | ||
| retract(); | ||
| void retract(); | ||
|
Comment on lines
93
to
+95
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a Feature is hot-unprovided or replaced, AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L24-L27 Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,6 +222,7 @@ export class AgentLifecycleService extends Disposable implements IAgentLifecycle | |
| eventBus?.activateAgent(agent); | ||
| let managed: ManagedAgent | undefined; | ||
| let didCreate = false; | ||
| let finalizerArmed = false; | ||
| try { | ||
| const handle = createScopedChildHandle( | ||
| this.instantiation, | ||
|
|
@@ -237,13 +238,17 @@ export class AgentLifecycleService extends Disposable implements IAgentLifecycle | |
| }], | ||
| ], | ||
| configureContainer: (container) => { | ||
| container.anchorKernelFinalizer(() => { | ||
| eventBus?.deactivateAgent(agent); | ||
| }, 'agent-event-bus-deactivate'); | ||
|
Comment on lines
+241
to
+243
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a post-creation step such as AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L16-L16 Useful? React with 👍 / 👎.
Comment on lines
+241
to
+243
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in the current revision shows that this finalizer still does not cover asynchronous AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L24-L26 Useful? React with 👍 / 👎.
Comment on lines
+241
to
+243
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in this revision is that AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L26-L26 Useful? React with 👍 / 👎. |
||
| finalizerArmed = true; | ||
| this.adopt({ | ||
| id: agentId, | ||
| kind: LifecycleScope.Agent, | ||
| accessor: { | ||
| get: (id) => container.invokeFunction((accessor) => accessor.get(id)), | ||
| }, | ||
| dispose: () => { container.dispose(); }, | ||
| dispose: () => container.disposeAsync(), | ||
| }); | ||
| managed = this.roster.get(agentId); | ||
| }, | ||
|
|
@@ -274,10 +279,10 @@ export class AgentLifecycleService extends Disposable implements IAgentLifecycle | |
| await managed.runtimeSet.close().catch(() => undefined); | ||
| managed.killSpace(); | ||
| try { | ||
| managed.handle.dispose(); | ||
| await managed.handle.dispose(); | ||
| } catch { } | ||
| } | ||
| eventBus?.deactivateAgent(agent); | ||
| if (!finalizerArmed) eventBus?.deactivateAgent(agent); | ||
| if (didCreate) this.onDidCloseEmitter.fire(agent); | ||
| throw error; | ||
| } | ||
|
|
@@ -446,10 +451,7 @@ export class AgentLifecycleService extends Disposable implements IAgentLifecycle | |
| await Promise.all([loop.settled(), compactionSettled, prompt.drain(reason)]); | ||
| await managed.runtimeSet.close(); | ||
| managed.killSpace(); | ||
| handle.dispose(); | ||
| this.instantiation.invokeFunction((accessor) => | ||
| (accessor.get(ISessionEventBus) as ISessionEventBus | undefined)?.deactivateAgent(agent), | ||
| ); | ||
| await handle.dispose(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in the current revision is that retaining the roster entry through this await still does not reserve the ID: AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L24-L27 Useful? React with 👍 / 👎. |
||
| if (this.roster.get(agent.agentId) === managed) this.roster.delete(agent.agentId); | ||
| this.onDidCloseEmitter.fire(agent); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in the new async-disposal implementation is that a second
disposeAsync()call returns an already-resolved promise while the first ledger teardown may still be suspended. If a parent scope starts disposal and another lifecycle path then awaits the same handle, that caller proceeds before contributed units and finalizers finish, reintroducing the teardown race this change is intended to close. Cache and return the first teardown promise for every subsequent call.AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L24-L27
Useful? React with 👍 / 👎.