-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(core): report broadcast delivery failures in send_message #10081
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 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -88,6 +88,14 @@ const debug = createDebugLogger('AGENTS_TEAM_MANAGER'); | |||||||||||||||||||
| // imported it from this module keep compiling. | ||||||||||||||||||||
| export type { TeamAgentHandle }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** Delivery outcome of a {@link TeamManager.broadcast} call. */ | ||||||||||||||||||||
| export interface BroadcastResult { | ||||||||||||||||||||
| /** Number of recipients the broadcast attempted (sender excluded). */ | ||||||||||||||||||||
| total: number; | ||||||||||||||||||||
| /** Names of recipients whose delivery was rejected. */ | ||||||||||||||||||||
| failedRecipients: string[]; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** Configuration for spawning a teammate. */ | ||||||||||||||||||||
| export interface TeammateSpawnConfig { | ||||||||||||||||||||
| /** Human-readable name (will be sanitized). */ | ||||||||||||||||||||
|
|
@@ -670,31 +678,38 @@ export class TeamManager { | |||||||||||||||||||
| /** | ||||||||||||||||||||
| * Broadcast a message to all teammates and the leader | ||||||||||||||||||||
| * (except the sender). | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * Returns the delivery outcome so the caller can distinguish complete | ||||||||||||||||||||
| * success from partial/total failure instead of assuming every | ||||||||||||||||||||
| * delivery landed. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| async broadcast(message: string, fromName: string): Promise<void> { | ||||||||||||||||||||
| const promises = this.teamFile.members | ||||||||||||||||||||
| async broadcast(message: string, fromName: string): Promise<BroadcastResult> { | ||||||||||||||||||||
| const recipients = this.teamFile.members | ||||||||||||||||||||
| .filter((m) => m.name.toLowerCase() !== fromName.toLowerCase()) | ||||||||||||||||||||
| .map((m) => this.sendMessage(m.name, message, fromName)); | ||||||||||||||||||||
| .map((m) => m.name); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Also deliver to leader inbox if sender is not the leader. | ||||||||||||||||||||
| if (fromName.toLowerCase() !== LEADER_NAME) { | ||||||||||||||||||||
| promises.push(this.sendMessage(LEADER_NAME, message, fromName)); | ||||||||||||||||||||
| recipients.push(LEADER_NAME); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // allSettled, not all: a single recipient that terminated between | ||||||||||||||||||||
| // the member snapshot and the send throws (its queue is gone), and | ||||||||||||||||||||
| // Promise.all would reject the whole broadcast — making the leader | ||||||||||||||||||||
| // think every recipient failed when the rest were delivered fine. | ||||||||||||||||||||
| const results = await Promise.allSettled(promises); | ||||||||||||||||||||
| const failures = results.filter( | ||||||||||||||||||||
| (r): r is PromiseRejectedResult => r.status === 'rejected', | ||||||||||||||||||||
| const results = await Promise.allSettled( | ||||||||||||||||||||
| recipients.map((name) => this.sendMessage(name, message, fromName)), | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| const failedRecipients = recipients.filter( | ||||||||||||||||||||
| (_, i) => results[i]?.status === 'rejected', | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| if (failures.length > 0) { | ||||||||||||||||||||
| if (failedRecipients.length > 0) { | ||||||||||||||||||||
| debug.warn( | ||||||||||||||||||||
| `Broadcast: ${failures.length}/${results.length} send(s) failed ` + | ||||||||||||||||||||
| `Broadcast: ${failedRecipients.length}/${results.length} send(s) failed ` + | ||||||||||||||||||||
| `(recipient likely terminated).`, | ||||||||||||||||||||
| ); | ||||||||||||||||||||
|
Comment on lines
707
to
710
Collaborator
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. [Suggestion] The warning prints only the failure count, but the failed recipients' names are available at the log site —
Suggested change
中文说明该警告只打印失败数量,但失败收件人的名字在打日志的位置就可以拿到—— — qwen3.8-max via Qwen Code /review (v0.22.0) |
||||||||||||||||||||
| } | ||||||||||||||||||||
| return { total: recipients.length, failedRecipients }; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2025 Qwen | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| /** | ||
| * Broadcast delivery-outcome contract for send_message(to: "*") — #10072. | ||
| * | ||
| * Uses the real TeamManager (via TeamCoordinationHarness) so a delivery | ||
| * rejects the same way it does in production: a recipient terminates | ||
| * between the member snapshot and the send, its per-agent queue is | ||
| * dropped, and sendMessage refuses the delivery. | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, afterEach } from 'vitest'; | ||
| import { SendMessageTool } from './send-message.js'; | ||
| import { BackgroundTaskRegistry } from '../agents/background-tasks.js'; | ||
| import type { ApprovalMode, Config } from '../config/config.js'; | ||
| import type { TeamManager } from '../agents/team/TeamManager.js'; | ||
| import { TeamCoordinationHarness } from '../agents/team/test-utils/coordination-harness.js'; | ||
|
|
||
| // Mock Storage so all file I/O uses the harness's temp dir. | ||
| vi.mock('../config/storage.js', async (importOriginal) => { | ||
| const original = | ||
| await importOriginal<typeof import('../config/storage.js')>(); | ||
| let mockGlobalDir = ''; | ||
| return { | ||
| ...original, | ||
| Storage: { | ||
| ...original.Storage, | ||
| getGlobalQwenDir: () => mockGlobalDir, | ||
| __setMockGlobalDir: (dir: string) => { | ||
| mockGlobalDir = dir; | ||
| }, | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| import { Storage } from '../config/storage.js'; | ||
|
|
||
| function setMockDir(dir: string): void { | ||
| ( | ||
| Storage as unknown as { | ||
| __setMockGlobalDir: (d: string) => void; | ||
| } | ||
| ).__setMockGlobalDir(dir); | ||
| } | ||
|
|
||
| function makeConfig(teamManager: TeamManager): Config { | ||
| return { | ||
| getTeamManager: () => teamManager, | ||
| getBackgroundTaskRegistry: () => new BackgroundTaskRegistry(), | ||
| getApprovalMode: () => 'default' as ApprovalMode, | ||
| } as unknown as Config; | ||
| } | ||
|
|
||
| describe('SendMessageTool — broadcast delivery outcomes (#10072)', () => { | ||
| let harness: TeamCoordinationHarness | undefined; | ||
|
|
||
| afterEach(async () => { | ||
| await harness?.cleanup(); | ||
| harness = undefined; | ||
| }); | ||
|
|
||
| async function createHarness(): Promise<TeamCoordinationHarness> { | ||
| const h = await TeamCoordinationHarness.create(); | ||
| setMockDir(h.tmpDir); | ||
| harness = h; | ||
| return h; | ||
| } | ||
|
|
||
| function broadcastInvocation(h: TeamCoordinationHarness) { | ||
| const tool = new SendMessageTool(makeConfig(h.teamManager)); | ||
| return tool.build({ to: '*', message: 'sync for everyone' }); | ||
| } | ||
|
|
||
| it('does not claim complete success when a delivery is rejected', async () => { | ||
| const h = await createHarness(); | ||
| await h.spawnTeammate('alice'); | ||
| const bob = await h.spawnTeammate('bob'); | ||
|
|
||
| // bob terminates between the member snapshot and the send: its | ||
| // queue is dropped, so its delivery rejects while alice's lands. | ||
| await bob.shutdown(); | ||
|
|
||
| const result = await broadcastInvocation(h).execute( | ||
| new AbortController().signal, | ||
| ); | ||
|
|
||
| expect(result.error).toBeUndefined(); | ||
| // Must not claim that every teammate received the message… | ||
| expect(result.llmContent).not.toBe('Message broadcast to all teammates.'); | ||
| // …and must name the recipient that was not reached. | ||
| expect(String(result.llmContent)).toContain('bob'); | ||
| }); | ||
|
|
||
| it('still reports complete success when every delivery lands', async () => { | ||
| const h = await createHarness(); | ||
| const alice = await h.spawnTeammate('alice'); | ||
| await h.spawnTeammate('bob'); | ||
|
|
||
| const result = await broadcastInvocation(h).execute( | ||
| new AbortController().signal, | ||
| ); | ||
|
|
||
| expect(result.error).toBeUndefined(); | ||
| expect(result.llmContent).toBe('Message broadcast to all teammates.'); | ||
| await h.waitForMessages('alice', 1); | ||
| await h.waitForMessages('bob', 1); | ||
| expect(alice.getReceivedMessages()).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('reports failure when no delivery lands', async () => { | ||
| const h = await createHarness(); | ||
| const alice = await h.spawnTeammate('alice'); | ||
| const bob = await h.spawnTeammate('bob'); | ||
| await alice.shutdown(); | ||
| await bob.shutdown(); | ||
|
|
||
| const result = await broadcastInvocation(h).execute( | ||
| new AbortController().signal, | ||
| ); | ||
|
|
||
| expect(result.error).toBeDefined(); | ||
| expect(result.llmContent).not.toBe('Message broadcast to all teammates.'); | ||
| expect(String(result.llmContent)).toContain('alice'); | ||
| expect(String(result.llmContent)).toContain('bob'); | ||
| }); | ||
| }); |
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.
[Suggestion]
failedRecipients: string[]collapses the four distinctsendMessagerejection causes — terminated recipient, unknown recipient, leader-inbox write failure, and backpressure — into bare names, and the only semantic annotation of "rejected" in this module (the warn just above the return) asserts "(recipient likely terminated)", which is wrong for three of the four. Backpressure is transient: its direct-send error explicitly tells the caller to wait for the backlog to drain, and the queue flushes the moment the recipient goes IDLE. Concrete trigger: teammatebobis ACTIVE withMAX_PENDING_MESSAGES(50) pending; the leader runssend_message(to: "*"); bob's delivery rejects — the tool then reportsdelivery failed for: bob. The listed recipients did not receive the message., byte-identical to bob having terminated. The leader model gets no hint that waiting and retrying would succeed, and can act as though bob is gone — reassigning bob's tasks or spawning a duplicate teammate — dropping a message that would have landed on bob's next idle flush. Consider carrying the rejection reason alongside the name (built from thePromiseRejectedResult.reasonvalues in theallSettledresults), surfacing the reason in the partial/total-failure text, and fixing or dropping the "(recipient likely terminated)" clause — or minimally, keep the shape but stop asserting a cause the data doesn't carry.中文说明
failedRecipients: string[]把sendMessage的四种不同 reject 原因——收件人已终止、收件人不存在、leader 收件箱写入失败、背压——压缩成了纯名字,而模块中唯一对「rejected」的语义标注(return 上方那行 warn)断言 "(recipient likely terminated)",对其中三种原因来说都是错的。背压是瞬时的:直接发送的错误信息明确提示调用方等待积压消化,且收件人一旦进入 IDLE 队列就会被清空。具体场景:teammatebob处于 ACTIVE 且积压了MAX_PENDING_MESSAGES(50)条消息,leader 执行send_message(to: "*"),bob 的投递被 reject——工具报告delivery failed for: bob. The listed recipients did not receive the message.,与 bob 已终止时的输出逐字节相同。leader 模型得不到「稍等重试即可成功」的提示,可能表现得像 bob 已经消失一样——重新分配 bob 的任务或复制一个新 teammate——从而丢掉一条本会在 bob 下次 idle flush 时送达的消息。建议把 reject 原因随名字一起携带(从allSettled结果的PromiseRejectedResult.reason取值),在部分/全部失败文案中呈现原因,并修正或去掉 "(recipient likely terminated)" 从句——最小的改法是保持类型不变,但不再断言数据中不存在的原因。— qwen3.8-max via Qwen Code /review (v0.22.0)