mirror of
https://gh-proxy.org/https://github.com/klinsmaya/multica.git
synced 2026-11-11 02:46:45 +08:00
Xichang(Seacen) Zhao
cdf93e4914
* fix(chat): cancel queued tasks when a session is archived, and stop telling non-Slack sessions they have no channel Two channel-agnostic fixes found while auditing WeCom; both help Lark and DingTalk equally. **Archiving left queued tasks claimable.** SetChatSessionArchived deletes the channel binding but never cancelled the session's in-flight tasks, and ClaimAgentTask does not read chat_session.status. So the daemon still ran the turn — and with the binding row already gone, the runtime brief described a private web chat while the channel adapter still held the chat id and posted the answer into the group. The person who archived the conversation never saw it happen. The cancel now runs first, in the same transaction. DeleteChatSession has always done this, for the reason its own query comment gives — "so the daemon doesn't keep running work whose result has nowhere to land" — and archiving means the same thing. **The empty-history note lied to every non-Slack session.** The reader is Slack-only, so WeCom, Lark and DingTalk all land on "this conversation is not connected to a chat channel", which is false. The reader is an agent deciding who can see its answer: told it is in a web-only conversation it reasons differently than told it is in a group whose backlog it cannot read. The note now names the platform, and the response carries channel_type. Written channel-agnostically — a per-platform lookup here would go blind the next time a channel is added, which is how this came to be wrong. Two tests against a real database. Both drive the real paths, not the helpers: task status after archive = "queued", want cancelled — the daemon will still run this turn, and with the channel binding already gone its answer goes to the room while the brief says private a WeCom-bound session was told it is not on a channel: "This conversation is not connected to a chat channel, so there is no channel history to read." * fix(chat): broadcast the archive cancellation instead of only writing it Review follow-up on the archive-cancel path. SetChatSessionArchived dropped the rows CancelAgentTasksByChatSession returns, so the cancellation reached the database and stopped there. None of the post-commit lifecycle ran: no captureTaskCancelled (the tasks' 24h mat_ tokens stayed live), no ReconcileAgentStatus (an agent whose only task was cancelled kept showing 'working'), no task:cancelled (other clients kept the turn on screen until something else forced a refresh), and no notifyTasksFinished (the runtime waited for the daemon's next poll before claiming the next task). The query is not limited to never-started work — it also matches dispatched, running, waiting_local_directory and deferred — so this is most visible when a running turn is cancelled. Keep the rows and hand them to TaskService.BroadcastCancelledTasks after the commit, which is what DeleteChatSession has always done twenty lines below. The empty-history response also read the binding twice, once for channel_type and once inside noHistoryNote. An archive landing between the two reads could answer channel_type "wecom" next to a note saying the conversation is not connected to a chat channel. Resolve the channel type once and derive both from it; noHistoryNote becomes a pure function of that value, so the two cannot disagree. TestArchivingAChatSessionCancelsItsQueuedTasks now queues a running turn alongside the queued one, marks the agent 'working', mints a task token, and asserts the post-commit side as well as the status column: agent back to 'idle', task:cancelled for both rows, token revoked. Without the broadcast it fails on all three. The history test drives the real response path on both legs and pins channel_type against the note. * fix(chat): scope the archive cancel to sessions that were on a channel Review follow-up. SetChatSessionArchived cancelled on every archive, with no check for whether the session had ever been bound to a channel. That is wider than the bug: the failure this fixes is a channel-bound session answering into a group room after its binding was dropped, and a plain web chat has no room, no adapter and no late answer. Cancelling there was a new product behaviour riding along with the fix, and it contradicts what the UI says archive is. A session row offers Stop or Archive, never both (chat-thread-list.tsx:364-377). Stop is `danger` behind a confirmation row; Archive fires immediately with none. Archive is the reversible step: unarchive exists, and hard delete is offered only once a chat is archived (chat-session-header.tsx:169-186). The two also cost the user different things. Stop goes through CancelTaskByUser, which hands the typed prompt back as cancelled_chat_message.restore_to_input or a durable chat_draft_restore row; CancelAgentTasksByChatSession is a bare UPDATE with neither, and BroadcastCancelledTasks does capture, reconciliation, the event and the runtime wakeup but no draft restore. So an unconditional cancel let one unconfirmed click destroy a prompt that unarchiving cannot bring back. The cancel now runs only when GetChannelChatSessionBindingBySessionAny finds a binding, read before DeleteChannelChatSessionBindingBySession erases it. A read error that is not ErrNoRows fails the archive rather than silently skipping the cancel — "not bound" and "cannot tell" must not take the same branch. This also settles the header's Archive item having no isRunning gate: archiving a running web chat from the dropdown now cancels nothing, so it no longer bypasses the confirmation the Stop button requires. Gating that item is a frontend question and stays out of this PR. The tests are one fixture archived twice. The bound leg is the previous test with the binding it was always describing — its failure message named an answer reaching the room, but it never created a room. It keeps every post-commit assertion (queued + running cancelled, agent back to idle, task:cancelled for both, token revoked) and adds that the binding is still deleted. The web-only leg asserts the opposite: both turns still in flight, agent still working, no task:cancelled, token still live, session still archived. Dropping the cancel fails the first; making it unconditional again fails the second; reading the binding after the delete fails the first. * fix(chat): refuse a debounced chat enqueue onto an archived session Review follow-up. The archive cancel only reaches tasks that already exist, and the channel run trigger is debounced by DefaultChatRunBatchWindow: the message row is persisted on arrival, the task row is created a window later when flushChatRun reloads the session and calls EnqueueChatTask. Archive inside that window and the cancel finds nothing, drops the binding and commits — then the flush enqueues onto a conversation the user closed. ClaimAgentTask does not read chat_session.status either, so the daemon runs it: runtime and quota spent on a closed conversation, assistant messages written into an archived chat, the agent flipped back to 'working', and with the binding already deleted the brief describes the run as a private web chat. EnqueueChatTask was the one enqueue path without the guard the direct-send path and OpenMikaOnboardingChat already take: lock the chat_session, re-read it, refuse a status that is not 'active' with ErrChatSessionArchived. It now takes it as its first statement, so the chat_session -> agent_task_queue order is unchanged. The lock is LockChatSessionForEnqueue (FOR NO KEY UPDATE), not the FOR UPDATE the send, delete and draft paths take, and the difference is deliberate. Those paths want to block INSERTs that reference the row: FOR UPDATE conflicts with the FOR KEY SHARE an FK insert takes on its parent, which is how LockChatSessionForDelete keeps a send from slipping a task in between its cancel and its delete. Here that conflict is the cost, not the point — every inbound channel message is an INSERT into chat_message FK'd to this row, so FOR UPDATE would make a room's next message wait for the previous message's enqueue. Measured on pg17 while the row is held: an append blocks under FOR UPDATE and does not under FOR NO KEY UPDATE, while SetChatSessionArchived's UPDATE (status is not a key column) blocks against FOR NO KEY UPDATE in both directions. Serialisation against the archive is kept, ingestion is not serialised behind it. flushChatRun needs no change: it clears the typing indicator before the error switch, and this error is not one of the two the switch names, so it falls to the default log branch and posts nothing into a room the archive just unbound. Three tests. The handler one is the case the existing archive tests cannot reach — message persisted, debounce not yet flushed, archive, then flush — asserting no task row and no task:queued; removing the status check fails it naming the run. The service one holds the lock mode from inside the enqueue transaction: a concurrent archive must block, a concurrent append must not. The router one pins that an archived-session flush clears typing and says nothing. Also carried over from review: sessionChannelType returned "" for every binding-lookup failure, so a transient database error answered 200 with the "not connected to a chat channel" note this PR removes — telling a WeCom or Lark session it is web-only because a query failed. It returns (string, error) now, with only pgx.ErrNoRows mapped to empty, and the caller reports the rest. And the bug's own description was wider than the bug: the outbound senders resolve their destination through the binding row the archive deletes, so a late answer has no route left and does not reach the room. The handler comment, the test file header and the failing assertion now say what actually happens — a wasted, mis-briefed run against a closed conversation.