fix(agent-session): mark stopped pending messages as paused

[kangfenmao]

🚨 Branch strategy — read before opening this PR

The v2 refactor has merged into main, so main is the default branch for active development (v1 and v2 code currently coexist there — expect large, breaking changes).

• Active development (features, refactors, optimizations, fixes for the current codebase) → target main (the default base).
• v1 maintenance (hotfixes and subsequent v1 releases) → branch from and target v1, not main.

A v1 fix does not auto-carry to main: if the same bug exists on main, open a separate forward-port PR targeting main. Before touching subsystems being replaced, read docs/references/data/ and watch for @deprecated markers — they flag code being deleted.

What this PR does

Before this PR:

When a user clicked stop after sending an agent session message, the assistant placeholder could remain pending in the database if the stop request arrived before the stream was created.

After this PR:

Stop requests now mark matching empty pending assistant placeholders as paused and persist the paused terminal status back to the existing placeholder row, so the database no longer keeps the stopped message in pending.

Fixes # N/A

Why we need it and why it was done in this way

The following tradeoffs were made:

The fix is scoped to agent-session topics and only applies the deferred stop request to the matching assistant placeholder id. A short TTL prevents stale stop requests from affecting later turns.

The following alternatives were considered:

Creating a new assistant message on empty paused terminal events was avoided because agent sessions already create a placeholder before streaming starts; final status should update that row instead.

Links to places where the discussion took place: N/A

Breaking changes

If this PR introduces breaking changes, please describe the changes and the impact on users.

None.

Special notes for your reviewer

The change is intentionally limited to agent-session placeholder persistence and stop handling before stream creation.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

• Branch: This PR targets the correct branch — main for active development, v1 for v1 maintenance fixes
• PR: The PR description is expressive enough and will help future contributors
• Code: Write code that humans can understand and Keep it simple
• Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• Documentation: A user-guide update was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior.
• Self-review: I have reviewed my own code (e.g., via /gh-pr-review, gh pr diff, or GitHub UI) before requesting review from others

Release note

Fix agent sessions so stopping a message send no longer leaves the assistant message pending in the database.

[ousugo]

LGTM

[DeJeune]

This comment was translated automatically.

Code Review · PR #16720 · Request changes

Update: Confirmed by maintainer, pre-stream abort must use a unified AbortController, no longer introducing the pendingAborts parallel "deferred abort" buffer. Design items previously marked as Notice are now upgraded to must fix.

The target behavior of the changes is correct (deferred-abort can be hit, paused persisted to DB, canPersistEmptyTerminal, assistantMessageId finalize targets all verified), but the implementation approach should be consolidated as follows.

---

🔴 Must Fix — Use turn-level AbortController to unify pre-stream abort, remove pendingAborts/TTL/messageId

Problem: pre-stream stop currently relies on pendingAborts map at AiStreamManager.ts:203 + PENDING_ABORT_TTL_MS=5000(:47) + messageId set matching(consumePendingAbort :577-586) to catch it. This is a parallel specialized state machine alongside the existing AbortController abort primitive. The controller used by execution isn't created until send() at :1001, so pre-stream can only rely on a bypass buffer.

Must change to: Let the turn own an AbortController, execution acquires it, and pre-stream abort directly trips this controller — this way pendingAborts, 5s TTL, messageId matching, and the synchronous DB write in abort() can all be deleted (controller is naturally isolated by turn, no staleness exists, thus no need for TTL/messageId fallback).

Implementation要点:

1. AgentSessionRuntimeService creates new AbortController() when creating a turn in beginTurn / startNextTurn(:797-807)/ startContinuationTurn(:887-897), attached to entry.currentTurn.abortController.
2. SendModelSpec adds optional abortController; createAndLaunchExecution(:989-1024) acquires spec.abortController ?? new AbortController(), no longer unconditionally creating new(:1001). Agent-session's model spec is filled by AgentChatContextProvider / startNextTurn with the turn's controller.
3. AiStreamManager.abort(): when no live stream and is agent-session, delegate to AgentSessionRuntimeService.abortPendingTurn(sessionId, reason) → call currentTurn.abortController.abort(reason).
4. After send() creates exec, if the acquired controller is already aborted, synchronously set stream.status='aborted' (replacing consumePendingAbort); subsequently runExecutionLoop reads the already aborted signal → openTurnStream(AiService.ts → AgentSessionRuntimeService.ts:335) immediately teardown → onExecutionPaused follows normal path to persist paused.
5. Keep canPersistEmptyTerminal + AgentSessionMessageBackend.assistantMessageId (these two are correct); Delete pendingAborts / consumePendingAbort / PENDING_ABORT_TTL_MS / markPendingAssistantPlaceholdersPaused(AgentSessionMessageService.ts:251) — paused persistence is now done via onExecutionPaused's normal persistence path, no longer needing to pre-flip DB in abort.

Coverage window is equivalent to current approach (controller must exist before send(), i.e., beginTurn has run), not a regression.

🟡 Resolved by above — Uncaught DB transaction in abort()

Location: AiStreamManager.ts:553 markPendingAssistantPlaceholdersPaused → withWriteTx without try/catch, while runAgentTask.ts:204-210's onRunAbort assumes "abort doesn't throw" (throwing would skip rejectExecution, leaking executionDone). After unified approach deletes this DB write, this risk surface disappears entirely. If any DB write in abort() is ultimately kept, it must have try/catch + logger.error (refer to AgentSessionRuntimeService.reconcileStalePendingMessages :188-197).

🔵 Notice — Test adjustments needed

AiStreamManager.test.ts:270-302 has two test cases written around markPendingAssistantPlaceholdersPaused mock, after refactoring should change to: pre-stream abort() → subsequent send() acquires already aborted turn controller → exec immediately paused with placeholder row persisted as paused; and add a turn controller isolation test case (old turn abort doesn't affect new turn).

---

_{🤖 Automated Review (reviewer–verifier adversarial) · /gh-pr-review}

---

Original Content

代码评审 · PR #16720 · Request changes

更新：经维护者确认,pre-stream abort 必须用统一的 AbortController 处理,不再引入 pendingAborts 这套并行的"延迟中止"缓冲。原先列为 Notice 的设计项现升级为必须修改。

改动的目标行为是对的(deferred-abort 能命中、paused 落库、canPersistEmptyTerminal、assistantMessageId finalize 目标都验证过),但实现方式请按下面收敛。

---

🔴 必须修改 — 用 turn 级 AbortController 统一 pre-stream abort,移除 pendingAborts/TTL/messageId

问题：pre-stream stop 现在靠 AiStreamManager.ts:203 的 pendingAborts map + PENDING_ABORT_TTL_MS=5000(:47)+ messageId 集合匹配(consumePendingAbort :577-586)来接住,这是与既有 AbortController abort 原语并行的一套专用状态机。execution 用的 controller 要到 send() 里 :1001 才 new,于是 pre-stream 只能靠旁路缓冲补。

必须改为：让 turn 拥有一个 AbortController,execution 领用它,pre-stream abort 直接 trip 这个 controller —— 这样 pendingAborts、5s TTL、messageId 匹配、以及 abort() 里的同步 DB 写全部可以删掉(controller 天然按 turn 隔离,不存在 staleness,也就不需要 TTL/messageId 兜底)。

落地要点：

1. AgentSessionRuntimeService 在 beginTurn / startNextTurn(:797-807)/ startContinuationTurn(:887-897)创建 turn 时 new AbortController(),挂到 entry.currentTurn.abortController。
2. SendModelSpec 增加可选 abortController;createAndLaunchExecution(:989-1024)领用 spec.abortController ?? new AbortController(),不再无条件新建(:1001)。agent-session 的 model spec 由 AgentChatContextProvider / startNextTurn 填入 turn 的 controller。
3. AiStreamManager.abort():无 live stream 且是 agent-session 时,委托 AgentSessionRuntimeService.abortPendingTurn(sessionId, reason) → 对 currentTurn.abortController.abort(reason)。
4. send() 建好 exec 后,若领用的 controller 已 aborted,同步把 stream.status='aborted'(替代 consumePendingAbort);随后 runExecutionLoop 读到已 aborted 的 signal → openTurnStream(AiService.ts → AgentSessionRuntimeService.ts:335)立即 teardown → onExecutionPaused 走正常路径落 paused。
5. 保留 canPersistEmptyTerminal + AgentSessionMessageBackend.assistantMessageId(这两项是对的);删除 pendingAborts / consumePendingAbort / PENDING_ABORT_TTL_MS / markPendingAssistantPlaceholdersPaused(AgentSessionMessageService.ts:251)—— paused 落库改由 onExecutionPaused 的正常持久化路径完成,不再需要在 abort 里预先 flip DB。

覆盖窗口与现方案等价(controller 需在 send() 前已存在,即 beginTurn 已跑),不是退化。

🟡 由上面一并解决 — abort() 里未捕获的 DB 事务

位置：AiStreamManager.ts:553 markPendingAssistantPlaceholdersPaused → withWriteTx 无 try/catch,而 runAgentTask.ts:204-210 的 onRunAbort 按"abort 不抛"来用(抛出会跳过 rejectExecution,泄漏 executionDone)。统一方案删掉这次 DB 写后,该风险面直接消失。若最终仍保留任何在 abort() 内的 DB 写,则必须 try/catch + logger.error(参照 AgentSessionRuntimeService.reconcileStalePendingMessages :188-197)。

🔵 Notice — 测试同步调整

AiStreamManager.test.ts:270-302 两个用例是围绕 markPendingAssistantPlaceholdersPaused mock 写的,重构后应改为:pre-stream abort() → 后续 send() 领用已 aborted 的 turn controller → exec 立即 paused 且占位行落 paused;并补一条 turn controller 隔离性用例(旧 turn abort 不影响新 turn)。

---

_{🤖 自动评审(reviewer–verifier 对抗式)· /gh-pr-review}

[DeJeune]

This review was translated automatically.

pre-stream abort must be handled using a unified turn-level AbortController. Remove pendingAborts/TTL/messageId matching and DB pre-flip in abort(). Keep canPersistEmptyTerminal + assistantMessageId. See review comments for details: #16720 (comment)

---

Original Content

pre-stream abort 必须用统一的 turn 级 AbortController 处理,移除 pendingAborts/TTL/messageId 匹配及 abort() 里的 DB 预 flip;保留 canPersistEmptyTerminal + assistantMessageId。详见评审评论:#16720 (comment)

[eeee0717]

This review was translated automatically.

I found one non-blocking type invariant issue that is separate from the existing DeJeune feedback.

• Suggestion: src/main/ai/agentSession/AgentSessionRuntimeService.ts:58 still declares assistantMessageId optional, but the new createPersistenceListener() path now requires it and throws when it is missing. Because beginTurn() mutates runtime entry state before listener creation in the new-entry path, a caller can compile successfully while leaving the session runtime partially installed if it omits the id. Please either make assistantMessageId required on BeginAgentSessionTurnInput / AgentSessionTurn, or validate it before mutating entries.

After fixing, please request eeee0717 for further review.

No local pnpm lint/test/format was run, per instruction.

---

Original Content

I found one non-blocking type invariant issue that is separate from the existing DeJeune feedback.

• Suggestion: src/main/ai/agentSession/AgentSessionRuntimeService.ts:58 still declares assistantMessageId optional, but the new createPersistenceListener() path now requires it and throws when it is missing. Because beginTurn() mutates runtime entry state before listener creation in the new-entry path, a caller can compile successfully while leaving the session runtime partially installed if it omits the id. Please either make assistantMessageId required on BeginAgentSessionTurnInput / AgentSessionTurn, or validate it before mutating entries.

修复后请 request eeee0717 再进行后续 review。

No local pnpm lint/test/format was run, per instruction.

[zhangjiadi225]

LGTM. Verified the latest head replaces the pendingAborts/DB pre-flip path with a turn-level AbortController, removes markPendingAssistantPlaceholdersPaused, and makes assistantMessageId required. CI basic-checks and general-test are passing. Note: PR is still behind main, so please update before merge.

dismissing

[eeee0717]

No substantive issues found.

CI checked via GitHub: visible checks are passing or intentionally skipped. Local pnpm lint/test/format not run per repository review instructions.

[0xfullex]

a reviewer is still requesting review