-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(agent-core-v2): suppress task notifications before loop quiescence on remove #3720
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
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 |
|---|---|---|
|
|
@@ -205,6 +205,7 @@ export class AgentTaskService extends Disposable implements IAgentTaskService { | |
| declare readonly _serviceBrand: undefined; | ||
|
|
||
| private readonly tasks = new Map<string, ManagedTask>(); | ||
| private exitSuppressionArmed = false; | ||
| private readonly buildingNotificationKeys = new Set<string>(); | ||
| private readonly pendingNotificationRequests = new Map<string, LoopNotifyHandle>(); | ||
| private readonly persistence: AgentTaskPersistence; | ||
|
|
@@ -779,21 +780,16 @@ export class AgentTaskService extends Disposable implements IAgentTaskService { | |
| return results.filter((info): info is AgentTaskInfo => info !== undefined); | ||
| } | ||
|
|
||
| async suppressAllTerminalNotifications(): Promise<void> { | ||
| this.exitSuppressionArmed = true; | ||
|
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.
If a detached task has already reached Useful? React with 👍 / 👎. |
||
| for (const [, request] of Array.from(this.pendingNotificationRequests)) { | ||
| request.drop(); | ||
| } | ||
| } | ||
|
|
||
| async stopAllOnExit(reason: string): Promise<readonly AgentTaskInfo[]> { | ||
| await this.suppressAllTerminalNotifications(); | ||
| if (this.keepAliveOnExit()) return []; | ||
| const active = this.list(true); | ||
| await Promise.allSettled( | ||
| active | ||
| .filter((task) => task.detached === true) | ||
| .map((task) => | ||
| this.suppressTerminalNotification(task.taskId).catch((error: unknown) => { | ||
| this.log.error('terminal notification suppression failed', { | ||
| taskId: task.taskId, | ||
| error, | ||
| }); | ||
| }), | ||
| ), | ||
| ); | ||
| return this.stopAll(reason); | ||
| } | ||
|
|
||
|
|
@@ -834,6 +830,10 @@ export class AgentTaskService extends Disposable implements IAgentTaskService { | |
| return this.sessionEventBus.isAgentActive(this.scopeContext.agentContext); | ||
| } | ||
|
|
||
| private marksTerminalNotificationSuppressed(entry: ManagedTask): boolean { | ||
| return this.exitSuppressionArmed && !this.keepAliveOnExit() && this.isDetached(entry); | ||
| } | ||
|
|
||
| async wait( | ||
| taskId: string, | ||
| timeoutMs = 30_000, | ||
|
|
@@ -1035,12 +1035,22 @@ export class AgentTaskService extends Disposable implements IAgentTaskService { | |
| entry.timeoutHandle = undefined; | ||
| } | ||
| const foregroundRelease = entry.foregroundRelease; | ||
| if (this.marksTerminalNotificationSuppressed(entry)) { | ||
| entry.terminalNotificationSuppressed = true; | ||
| } | ||
| if (entry.outputPersistStarted) { | ||
| await this.persistLive(entry); | ||
| } else { | ||
| entry.pendingOutput = []; | ||
| entry.pendingOutputBytes = 0; | ||
| } | ||
| if ( | ||
| this.marksTerminalNotificationSuppressed(entry) && | ||
| entry.terminalNotificationSuppressed !== true | ||
| ) { | ||
| entry.terminalNotificationSuppressed = true; | ||
| await this.persistLive(entry); | ||
| } | ||
| this.fireTerminalEffects(entry); | ||
| foregroundRelease?.resolve('terminal'); | ||
| this.resolveWaiters(entry); | ||
|
|
@@ -1095,7 +1105,7 @@ export class AgentTaskService extends Disposable implements IAgentTaskService { | |
| if (!this.lifecycleActive()) return; | ||
| const context = await this.buildAgentTaskNotificationContext(info); | ||
| if (context === undefined) return; | ||
| if (!this.lifecycleActive()) return; | ||
| if (!this.lifecycleActive() || this.isTerminalNotificationSuppressed(info.taskId)) return; | ||
| const key = notificationKey(context.origin); | ||
| if (this.deliveredNotificationKeys.has(key)) return; | ||
| const handle = this.loop.notify({ | ||
|
|
@@ -1302,6 +1312,7 @@ export class AgentTaskService extends Disposable implements IAgentTaskService { | |
|
|
||
| private isTerminalNotificationSuppressed(taskId: string): boolean { | ||
| return ( | ||
| this.exitSuppressionArmed || | ||
|
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 Useful? React with 👍 / 👎. |
||
| this.tasks.get(taskId)?.terminalNotificationSuppressed === true || | ||
| this.ghosts.get(taskId)?.terminalNotificationSuppressed === true | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -388,6 +388,7 @@ export class AgentLifecycleService extends Disposable implements IAgentLifecycle | |
| managed.closing = true; | ||
| this.onWillCloseEmitter.fire(agent); | ||
| const handle = managed.handle; | ||
| await handle.accessor.get(IAgentTaskService).suppressAllTerminalNotifications(); | ||
|
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.
This fixes observable CLI behavior by preventing background-task notifications from initiating work while an agent is being removed, but the commit contains no AGENTS.md reference: AGENTS.md:L86-L86 Useful? React with 👍 / 👎. |
||
| const loop = handle.accessor.get(IAgentLoopService); | ||
| const compaction = handle.accessor.get(IAgentFullCompactionService).compacting; | ||
| const compactionSettled = compaction?.promise.catch(() => undefined) ?? Promise.resolve(); | ||
|
|
||
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.
When a detached task finishes just before
remove()andnotifyAgentTask()is already awaiting its output snapshot, the task's terminal state was recorded before this flag was armed. After the await,buildAgentTaskNotificationContext()checks only the task/ghostterminalNotificationSuppressedfield, notexitSuppressionArmed, so it can still callloop.notify()and start a spurious turn or keeploop.settled()pending during teardown. Make the global flag visible to notification construction as well as settlement.Useful? React with 👍 / 👎.