fix(web-shell): quiet background task polling failures - #7923
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献!
进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewI formed my own baseline before reading the diff: add an optional
One non-blocking reservation: No correctness bugs, security issues, or convention violations (ESM, TestingThis is an unattended CI run, so I did not build or execute the PR — the signal below is the PR's own CI, read via the API. The primary unit suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Real-scenario (TUI) testing: N/A — this is non-visual notice-dispatch logic, fully exercised by the unit tests above; there is no on-screen behavior to capture. The sandboxed 中文说明代码审查我在看 diff 之前先独立给出了自己的方案:给
一个非阻塞的保留点: 无正确性 bug、安全问题或规范违规(ESM、用 测试这是无人值守的 CI 运行,因此我没有构建或执行 PR——下方信号来自 PR 自身的 CI,通过 API 读取。主单元测试套件( 真实场景(TUI)测试:N/A——这是非视觉的 notice 派发逻辑,已由上面的单元测试完整覆盖,没有可截屏的界面行为。沙箱化的 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — a small, well-tested fix that does exactly what the tracking issue asked; the only reservation is a minor design choice, not a defect. Stepping back: this is the kind of PR that's easy to approve for the right reasons. The problem is verified independently of the PR's own framing — #7834 is a maintainer-triaged The code is straightforward and isn't trying too hard — a small classifier, a backward-compatible dedupe hook in I could not run the change myself (unattended CI never executes PR code), so the live signal is the PR's own CI plus its focused unit tests, which cover all four dispatch paths and the hook wiring. The primary unit suite is still running on the reviewed commit with nothing red, so approval is deferred until CI lands green on 中文说明置信度:4/5 —— 一个小巧、测试充分的修复,恰好做了跟踪 issue 要求的事;唯一的保留是一个轻微的设计取舍,而非缺陷。 退一步看:这是一个容易"因为正确的理由"而批准的 PR。问题独立于 PR 自身的叙述得到了验证——#7834 是维护者 triage 过的 代码直白、没有用力过猛——一个小分类器、 我无法亲自运行该改动(无人值守 CI 从不执行 PR 代码),所以现场信号来自 PR 自身的 CI 及其聚焦单元测试——后者覆盖了全部四条派发路径和 hook 接线。主单元测试套件在受审 commit 上仍在运行且无红,因此批准推迟到 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
| setAttachSessionNonce, | ||
| setNewSessionNonce, | ||
| }: CreateDaemonSessionActionsArgs): DaemonSessionActions { | ||
| const silentHardFailureNoticeKeys = new Set<string>(); |
There was a problem hiding this comment.
[Suggestion] silentHardFailureNoticeKeys is never cleared on session change, reconnection, or clear — it grows monotonically for the provider's lifetime, permanently suppressing repeat notices of the same hard error across different sessions.
Failure scenario: a user in session A encounters a non-transient getTasks error (e.g. "Malformed response"). The notice fires once and the key "load_tasks:Malformed response" is added to the set. The user then switches to session B or the daemon reconnects — clearActiveSessionState() runs but does not touch this set. Session B encounters the same error, but dispatchActionError sees the key already present and silently suppresses the notice. The user has no UI-visible indication the error persists.
| const silentHardFailureNoticeKeys = new Set<string>(); | |
| const silentHardFailureNoticeKeys = new Set<string>(); | |
| // NOTE: consider clearing in clearActiveSessionState() so a new session | |
| // gets a fresh dedup window. |
中文说明
[Suggestion] silentHardFailureNoticeKeys 在 session 切换、重连或清理时从未被清空——它随着 provider 的生命周期单调增长,在不同 session 间永久压制相同硬性错误的重复通知。
失败场景:用户在 session A 遇到非瞬时 getTasks 错误(如 "Malformed response"),通知触发一次并将 key 加入集合。随后用户切换到 session B 或 daemon 重连——clearActiveSessionState() 执行但不会清空此集合。Session B 遇到同样的错误,但 dispatchActionError 发现 key 已存在,静默压制通知。用户看不到任何 UI 可见的错误提示。
建议在 clearActiveSessionState() 中添加 silentHardFailureNoticeKeys.clear()。
— qwen3.7-max via Qwen Code /review
| function extractHttpStatus(error: unknown): number | undefined { | ||
| if (error instanceof DaemonHttpError) return error.status; | ||
| if (isRecord(error) && typeof error['status'] === 'number') { | ||
| return error['status']; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
[Suggestion] extractHttpStatus and isRecord are duplicated character-for-character from DaemonSessionProvider.tsx (lines 3232–3242), a file in the same directory.
Concrete cost: two identical private copies of the same helper in adjacent files. A future change to the error-shape contract (e.g. supporting a statusCode field or a new SDK error class) must be made in both places independently, and a miss in one silently diverges the HTTP-status detection behavior between provider-level error handling and action-level error handling.
Suggested fix: export both helpers from a shared module in the same directory (e.g. error-utils.ts) and import them where needed.
中文说明
[Suggestion] extractHttpStatus 和 isRecord 与同目录的 DaemonSessionProvider.tsx(第 3232–3242 行)中的实现完全一致。
具体成本:相邻文件中存在两份相同的私有工具函数副本。未来对错误形状的修改(如支持 statusCode 字段或新的 SDK 错误类)必须在两处独立修改,遗漏一处会导致 provider 层和 action 层的 HTTP 状态检测行为静默分歧。
建议将两个工具函数提取到同目录的共享模块中(如 error-utils.ts),并在需要的地方导入。
— qwen3.7-max via Qwen Code /review
|
Addressed the latest review suggestions in 34d573a. clearActiveSessionState() now resets the silent hard-failure dedupe set, with a regression test covering the same hard getTasks({ silent: true }) failure after clearSession(). I also extracted the duplicated extractHttpStatus / isRecord helpers into httpErrors.ts and reused them from both action-level and provider-level error handling. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
| function isTransientActionError(error: unknown): boolean { | ||
| if (isAbortError(error)) return true; | ||
| const status = extractHttpStatus(error); | ||
| if (status !== undefined) { | ||
| return status >= 500 || status === 408 || status === 429; | ||
| } |
There was a problem hiding this comment.
[Suggestion] isTransientActionError has several untested branches: status codes 408 and 429, the isAbortError branch, and message patterns 'timed out', 'network error', and 'networkerror'. Only HTTP 500 and 'failed to fetch' are exercised by the current test suite.
Failure scenario: a future edit that inverts the >= 500 comparison or drops 429 from the transient set would not be caught — the existing test covering only HTTP 500 would still pass, and the regression would ship silently.
Consider adding tests for: (a) DaemonHttpError(408) with silent: true → no notice; (b) DaemonHttpError(429) with silent: true → no notice; (c) an abort-style DOMException('AbortError') with silent: true → no notice.
中文说明
[Suggestion] isTransientActionError 有多个未经测试的分支:状态码 408 和 429、isAbortError 分支,以及消息模式 'timed out'、'network error' 和 'networkerror'。当前测试套件仅覆盖了 HTTP 500 和 'failed to fetch'。
失败场景:未来的修改如果反转 >= 500 比较或从瞬时集合中移除 429,将不会被捕获——现有的仅覆盖 HTTP 500 的测试仍会通过,回归将静默发布。
建议添加测试:(a) DaemonHttpError(408) + silent: true → 不派发通知;(b) DaemonHttpError(429) + silent: true → 不派发通知;(c) DOMException('AbortError') + silent: true → 不派发通知。
— qwen3.7-max via Qwen Code /review
| if (isAbortError(error)) return true; | ||
| const status = extractHttpStatus(error); | ||
| if (status !== undefined) { | ||
| return status >= 500 || status === 408 || status === 429; |
There was a problem hiding this comment.
[Suggestion] No test positively asserts that a non-retryable HTTP status (e.g., 401, 403, 404) in silent mode IS surfaced as a notice. The hard-failure test uses a generic Error('Malformed response'), never a DaemonHttpError with a 4xx status.
Failure scenario: if the comparison were accidentally broadened to status >= 400, a DaemonHttpError(401, …) would be classified as transient and silently swallowed by the background poller — the user would see no indication that authentication failed, while the UI continued showing stale task data.
Consider adding a test: DaemonHttpError(403) with silent: true → addNotice IS called once, and the error propagates.
中文说明
[Suggestion] 没有测试正面断言不可重试的 HTTP 状态码(如 401、403、404)在 silent 模式下会作为通知派发。硬性失败测试使用的是普通的 Error('Malformed response'),从未使用带 4xx 状态码的 DaemonHttpError。
失败场景:如果比较被意外扩大为 status >= 400,DaemonHttpError(401, …) 将被归类为瞬时错误并被后台轮询静默吞掉——用户看不到任何认证失败的提示,而 UI 继续显示过期的任务数据。
建议添加测试:DaemonHttpError(403) + silent: true → addNotice 被调用一次,且错误继续传播。
— qwen3.7-max via Qwen Code /review
| export function extractHttpStatus(error: unknown): number | undefined { | ||
| if (error instanceof DaemonHttpError) return error.status; | ||
| if (isRecord(error) && typeof error['status'] === 'number') { | ||
| return error['status']; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The extracted httpErrors.ts module has no dedicated test file. The DaemonHttpError branch of extractHttpStatus is exercised indirectly via actions.test.ts, but the duck-typed isRecord + status branch (for plain { status: 500 } errors) is never exercised anywhere.
Failure scenario: a refactor that renames the status property check or drops the duck-type branch would not be caught — callers throwing a non-DaemonHttpError object with a status field would stop being classified correctly.
中文说明
[Suggestion] 提取的 httpErrors.ts 模块没有专门的测试文件。extractHttpStatus 的 DaemonHttpError 分支通过 actions.test.ts 间接覆盖,但鸭子类型化的 isRecord + status 分支(用于处理普通 { status: 500 } 错误对象)从未被任何测试覆盖。
失败场景:如果重构将 status 属性检查重命名或移除了鸭子类型分支,将不会被捕获——抛出非 DaemonHttpError 但带有 status 字段的错误对象的调用方将不再被正确分类。
— qwen3.7-max via Qwen Code /review
|
Addressed the latest coverage suggestions in 484d733. Added focused tests for retryable HTTP 408/429, abort-style silent task failures, transient timeout/network message matching, hard HTTP 403 notice behavior in silent mode, and dedicated httpErrors helper coverage for DaemonHttpError, duck-typed { status }, invalid status values, and record detection. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| const session = sessionRef.current; | ||
| if (!session) { | ||
| throw new Error('Daemon session is not connected'); |
There was a problem hiding this comment.
[Suggestion] This new inline no-session guard (which replaces requireSessionForAction) has no test coverage — the only "not connected" test in actions.test.ts targets attachSession. — Concrete cost: this path is a behaviour change (when silent, the not-connected error now routes through dispatchActionError and is deduped to a single notice instead of noticing on every call), and if a future edit makes isTransientActionError match this message or reorders the guard, a permanently disconnected session would surface zero user feedback and nothing in the suite would fail. Consider adding a case with sessionRef.current undefined that asserts await expect(actions.getTasks({ silent: true })).rejects.toThrow('Daemon session is not connected') and addNotice called once across two silent calls, plus a non-silent variant.
— qwen3.8-max-preview via Qwen Code /review
| if (opts?.silent && isTransientActionError(error)) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
[Suggestion] A persistently transient-classified failure is silenced indefinitely with no escalating signal. — Failure scenario: a daemon that consistently exceeds the action timeout (or sits behind a broken proxy) rejects every 3s poll with e.g. Get tasks timed out after 30000ms; each failure is transient, so it is rethrown silently and useBackgroundTasks only console.warns — without session id or failure count. The tasks panel freezes with zero UI feedback, indistinguishable from "no active tasks". Consider tracking consecutive transient failures and enriching the warn with sessionId/a counter, and/or after N consecutive transient failures surfacing a single notice or downgrading to a non-silent getTasks() so a persistent condition eventually becomes visible.
— qwen3.8-max-preview via Qwen Code /review
| function clearActiveSessionState() { | ||
| silentHardFailureNoticeKeys.clear(); |
There was a problem hiding this comment.
[Suggestion] silentHardFailureNoticeKeys is cleared only on clearSession()/newSession(), never on a successful poll. — Failure scenario: a hard failure (e.g. HTTP 403 Forbidden) dispatches a notice once and stores load_tasks:Forbidden; the cause is then fixed and polling stays healthy long enough for the user to dismiss the stale notice; when the same failure later recurs, the key is still in the set, so the notice is suppressed and useBackgroundTasks only console.warns — the panel silently stops updating with zero UI feedback. Since only getTasks uses this set, consider clearing it on a successful getTasks resolve so a failure recurring after recovery is surfaced again.
— qwen3.8-max-preview via Qwen Code /review
|
@qwen-code /resolve |
…no-session guard Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge summary — PR #7923Root causeMain's PR #7929 (commit What each side did
Resolution (semantic merge)Kept the PR's async getTasks(opts) {
const session = sessionRef.current;
if (!session) throw new Error('Daemon session is not connected');
try {
return await withActionTimeout(session.tasks(), 'Get tasks timed out');
} catch (error) {
if (error instanceof Error &&
error.message === 'Daemon session is not connected') throw error;
if (opts?.silent && isTransientActionError(error)) throw error;
throw dispatchActionError(/* …load_tasks, silent once-only notice… */);
}
},In the test file the sides added different tests, so both were kept: the PR's What is load-bearing
What I could not verifyNo build/typecheck/tests run (out of scope). Behavior is covered by tests on both 中文说明根因main 的 PR #7929(commit 双方改动
解决方式保留本 PR 的 关键点
无法验证本命令不运行 build/typecheck/测试。行为双方均有测试覆盖,但“silent + 无会话” |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| @@ -1435,20 +1452,53 @@ function dispatchActionError( | |||
| return abortError; | |||
| } | |||
| const message = error instanceof Error ? error.message : String(error); | |||
There was a problem hiding this comment.
[Suggestion] The new getActionErrorMessage(error) helper duplicates this exact inline expression. The helper is used by getActionErrorNoticeKey and isTransientActionError, but this pre-existing copy in dispatchActionError was left in place. — Concrete cost: the error→message normalization rule now lives in two places in the same file; if a future change handles a new error shape in one copy it must mirror the other, or the dedupe key (getActionErrorNoticeKey) and the displayed notice message diverge, so a hard failure could be deduped under a different string than the one shown.
| const message = error instanceof Error ? error.message : String(error); | |
| const message = getActionErrorMessage(error); |
— qwen3.8-max-preview via Qwen Code /review
| const message = getActionErrorMessage(error).toLowerCase(); | ||
| return ( | ||
| message.includes('timed out') || | ||
| message.includes('failed to fetch') || | ||
| message.includes('network error') || | ||
| message.includes('networkerror') | ||
| ); |
There was a problem hiding this comment.
[Suggestion] For a non-HTTP error (plain Error, no numeric status), classification falls through to this substring check, so a permanent hard failure whose message merely contains one of these tokens is classified transient. — Failure scenario: a daemon/SDK bug throwing e.g. Error("failed to fetch model config: invalid response") or Error("NetworkError: bad certificate") on every 3s poll is classified transient forever → getTasks({ silent: true }) rethrows with no notice and no dedupe tracking, and the useBackgroundTasks poller only console.warns and keeps polling, so a permanently broken daemon produces zero user-facing notices — defeating the PR's "surface hard failures once" goal. HTTP-status errors are immune (the status branch returns first). This is distinct from the open thread at line 1147 (which objects to the lack of escalation); here the classification itself mislabels a hard failure. — Suggested fix: prefer a typed signal over message sniffing for the non-HTTP case, or route message-matched errors through the same dedupe set so a never-resolving "transient" failure surfaces once.
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM!✅
— qwen3.8-max-preview via Qwen Code /review
|
Released in v0.21.3. |






What this PR does
Adds a silent mode for
getTasksand uses it from the web-shell background task poller. In silent mode, transient task refresh failures such as aborts, timeouts, network fetch failures, and retryable HTTP responses are rethrown without dispatching user-facing notices, while hard failures still dispatch at most one notice per error identity so they are not lost or repeated every poll.Why it's needed
useBackgroundTasksrefreshes tasks on a 3-second interval, butgetTaskspreviously routed every failure throughdispatchActionError, which could produce repeated error notices during background polling. This keeps foregroundgetTasks()calls reporting normally while making the polling path quiet for retryable failures and non-spammy for hard failures.Reviewer Test Plan
How to verify
Run the focused webui action tests and confirm default
getTasks()failures still dispatch notices, silent transient failures do not dispatch notices, silent retryable HTTP failures do not dispatch notices, and silent hard failures dispatch only once.Run the focused web-shell hook test and confirm
useBackgroundTaskspasses{ silent: true }for immediate refreshes, session-switch refreshes, and interval refreshes.Commands run locally:
npm test --workspace=@qwen-code/webui -- src/daemon/session/actions.test.ts,npm test --workspace=@qwen-code/web-shell -- client/hooks/useBackgroundTasks.test.tsx,npm run typecheck --workspace=@qwen-code/webui,npx prettier --check packages/webui/src/daemon/session/types.ts packages/webui/src/daemon/session/actions.ts packages/webui/src/daemon/session/actions.test.ts packages/web-shell/client/hooks/useBackgroundTasks.ts packages/web-shell/client/hooks/useBackgroundTasks.test.tsx.Evidence (Before & After)
N/A — non-visual action and hook behavior covered by unit tests.
Tested on
Environment (optional)
Windows local workspace with Vitest, TypeScript, and Prettier. I also rebuilt
@qwen-code/webuilocally before checking the web-shell type surface because web-shell typecheck reads the local webui dist declarations.Risk & Scope
getStatsis left unchanged because triage identified it as user-initiated rather than a background poll.getTasks()remains backward compatible because the new options argument is optional.Linked Issues
Refs #7834.
中文说明
What this PR does
为
getTasks增加 silent 模式,并让 web-shell 的后台任务轮询使用这个模式。在 silent 模式下,abort、timeout、网络 fetch 失败、可重试 HTTP 响应等瞬时任务刷新失败会原样抛出但不派发用户可见 notice;硬性失败仍会按错误身份最多派发一次 notice,避免错误完全丢失,也避免每次轮询重复弹出。Why it's needed
useBackgroundTasks每 3 秒刷新一次任务,但此前getTasks的每次失败都会经过dispatchActionError,后台轮询失败时可能产生重复 error notice。这个改动保持前台getTasks()调用正常报错,同时让轮询路径对可重试失败保持安静,并让硬性失败不再刷屏。Reviewer Test Plan
How to verify
运行聚焦的 webui action 测试,确认默认
getTasks()失败仍会派发 notice,silent 瞬时失败不会派发 notice,silent 可重试 HTTP 失败不会派发 notice,silent 硬性失败只派发一次 notice。运行聚焦的 web-shell hook 测试,确认
useBackgroundTasks在立即刷新、session 切换刷新、interval 刷新时都会传入{ silent: true }。本地运行命令:
npm test --workspace=@qwen-code/webui -- src/daemon/session/actions.test.ts、npm test --workspace=@qwen-code/web-shell -- client/hooks/useBackgroundTasks.test.tsx、npm run typecheck --workspace=@qwen-code/webui、npx prettier --check packages/webui/src/daemon/session/types.ts packages/webui/src/daemon/session/actions.ts packages/webui/src/daemon/session/actions.test.ts packages/web-shell/client/hooks/useBackgroundTasks.ts packages/web-shell/client/hooks/useBackgroundTasks.test.tsx。Evidence (Before & After)
N/A —— 这是非视觉的 action 和 hook 行为变更,已由单元测试覆盖。
Tested on
Environment (optional)
Windows 本地工作区,使用 Vitest、TypeScript 和 Prettier。我还在本地重新构建了
@qwen-code/webui后再检查 web-shell 的类型表面,因为 web-shell typecheck 会读取本地 webui dist 声明。Risk & Scope
getStats保持不变,因为 triage 已指出它是用户主动触发,不是后台轮询。getTasks()保持向后兼容,因为新增 options 参数是可选的。Linked Issues
Refs #7834.