test(integration): remove flaky ACP recall timing check - #10703
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! Note on the trajectory: this PR moved through three shapes — a production preload ( Template looks good ✓ Problem: Observed failure — main E2E run 33501336475 / job 99835166630 failed the ACP first-request auto-memory assertion three times on Direction: Aligned, with one honest reservation named below. Removing an assertion that measures shared-runner scheduling rather than a stable contract is a legitimate flake fix. The reservation: this test was also the only one driving the real recall pipeline (scan → parse → select → inject) end-to-end through a real ACP process. I checked the replacement coverage the PR body claims, and it exists and bites: Size: Test-only deletion, −109 net lines in one integration-test file. Stage 0 core-module gate does not apply. Approach: Minimal — the diff excises exactly the one Risk: No high-risk-path matches (Stage 1e). Reviewers should know the changed suite never executes in PR CI (the full CLI integration lane is merge_group-only and no merge queue is enabled; the no-AK lane doesn't include this file) — the confirmation that the flake is gone lands with post-merge main E2E. The flake removal itself is by construction: the timing-sensitive assertion no longer exists. Moving on to code review. 🔍 中文说明感谢贡献! 关于演变过程: 本 PR 经历了三种形态——生产侧预载( 模板完整 ✓ 问题: 已观测到的失败——主 E2E run 33501336475 / job 99835166630 在 方向: 对齐,但有一个诚实的保留意见见下。删除一个实际测量共享 runner 调度、而非稳定产品契约的断言,是正当的 flake 修复。保留意见是:这个测试也是唯一一个在真实 ACP 进程中端到端驱动真实 recall 管线(扫描 → 解析 → 选择 → 注入)的测试。我核对了 PR 正文声称的替代覆盖,确实存在且有力度: 规模: 纯测试删除,单个集成测试文件净 −109 行。Stage 0 核心模块门禁不适用。 方案: 最小化——diff 精确切除那一个 风险: 无高风险路径命中(Stage 1e)。评审者应知晓:改动的套件不会在 PR CI 中执行(完整 CLI 集成通道仅 merge_group 且当前未启用合并队列;no-AK 通道不含此文件)——flake 已消除的确认将随合并后的 main E2E 落地。flake 消除本身是构造性的:时序敏感断言已不复存在。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the current head after the ponytail pass. The final diff only removes the flaky wall-clock E2E; fake-timer client tests still cover bounded initial delivery, and ACP Session tests cover initial and post-tool refined delivery. No production behavior or concurrency changes. Focused Prettier and ESLint pass; PR CI is pending.
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head ffd4871f.
- The final shape is a pure removal of the flaky assertion (+0/−78, one test): the E2E asserted the recalled-memory marker unconditionally on the first ACP model request, which encoded a real 100 ms wall-clock race with real filesystem scanning — exactly the kind of check that legitimately loses on loaded shared runners. The claim in the PR description and the review comment matches the code.
- Contract coverage is not dropped: the initial-window fast path stays pinned by the fake-timer client unit tests in
packages/core/src/core/client.test.ts, and both delivery paths of the conditional contract (recall before the initial send; refined recall after tool responses) have assertions in the ACPSession.test.ts. No production behavior or concurrency change; remaining imports in the trimmed test file are still used. - No prior open reviews or threads; CI on this head has no failures (Test and the review pipeline still running); per the channel convention the call is on the review itself.
yiliang114
left a comment
There was a problem hiding this comment.
Review findings (self-PR, cannot self-approve):
- The wall-clock race is genuinely removed, not papered over: the fake server now orchestrates the recall lifecycle deterministically — the selector request (non-stream) completes its 250ms budget and resolves a barrier; the first streamed main request awaits that barrier and answers with a
read_filetool call for the actual memory file, so the marker enters the conversation through the recall path regardless of how fast the filesystem scan ran. The assertion relaxes from 'the first main request contains the marker' to 'at least two main requests and one of them contains the marker', which matches the new two-request shape. - Honest scoping in the comment: the 100ms scan-budget behavior itself stays covered by the fake-timer client tests, this E2E only pins delivery.
fakeToolCall(name, args, id)exists in fake-openai-server.ts with a matching signature;memoryFileis assigned before the prompt is submitted and only read at request time, so no initialization race.- Change is confined to the single
itblock; no product code touched.
Verified locally on 33ed51f: built the bundle and ran the target test — 'delivers managed auto-memory through the ACP recall lifecycle' passes (3.6s). No blocking issues found.
chiga0
left a comment
There was a problem hiding this comment.
Review — test(integration): remove flaky ACP recall timing check
Tier: Standard (test deletion — feature coverage impact)
What the change does at the new head (ffd4871)
Pure deletion of 'injects managed auto-memory into the first ACP model request' (+0 / −78). No replacement is added.
The deleted test:
// selector delayed 250ms (outside 100ms initial recall window)
if (body['stream'] !== true) await delay(250);
return { content: 'done' };
// …
const mainRequest = fakeServer.requests.find(…);
expect(JSON.stringify(mainRequest?.body['messages'])).toContain(marker);The flakiness was structural: the selector was forced outside the 100ms budget, yet the assertion still required the marker to appear in the single first main request — a race the test could not win deterministically.
Removing it is the correct short-term fix; the test was negative-value CI signal.
Coverage impact
After this PR the ACP managed auto-memory recall lifecycle has no integration test coverage. The deleted test was the only E2E check on the path from selector → memory injection → model context. A replacement that drives the delivery deterministically (e.g. via Promise coordination rather than wall-clock) is worth a follow-up.
This is noted but is not a blocker — deleting a reliably-flaky test is a valid trade-off.
CI status
All jobs at ffd4871 are SKIPPED; review-pr is still in progress. No test evidence for or against the change (expected for a pure deletion).
Verdict
No blockers. The change is correct, the deletion matches the PR title exactly, and the flakiness root cause is sound. Approving with the recommendation to add a deterministic replacement in a follow-up.
Code reviewIndependent proposal first: for a flaky E2E that asserts more than the product contract guarantees, the sane options are (a) make the production path fast enough — the reverted preload, which buys the guarantee at the cost of a scan on every refresh; (b) rewrite the test to be deterministic while keeping end-to-end coverage — the intermediate commit here; (c) remove the test and lean on the deterministic seams. I'd have landed on (b); the PR lands on (c). Having verified the seam coverage, (c) is defensible — the diff is honest about the tradeoff and the claims check out. What I verified:
No blockers. Standing reservation (non-blocking, named in Stage 1): nothing now drives the real recall pipeline end-to-end through a real ACP process; the deterministic rewrite at Test evidence (PR's own CI, via API)CI on
No sandboxed-lane line this run: the PR carries no behavioural claim to settle — it removes a test, and CI (plus the structural argument above) is what there is to check. If a maintainer later wants the end-to-end coverage back, the rewrite at 中文说明代码审查先给独立方案:对于一个断言超出产品契约保证的 flaky E2E,合理选项是 (a) 让生产路径足够快——即被回退的预载,代价是每次刷新都要扫描;(b) 把测试重写为确定性、同时保留端到端覆盖——即本 PR 的中间版本;(c) 删除测试、依靠确定性接缝覆盖。我会选 (b);PR 落在 (c)。在核实接缝覆盖之后,(c) 站得住——diff 对取舍是诚实的,各项声称也都查证属实。 我核实的内容:
无阻塞问题。长期保留意见(非阻塞,已在 Stage 1 点名):现在没有任何测试在真实 ACP 进程中端到端驱动真实 recall 管线;如需在不要墙钟依赖的前提下取回该覆盖, 测试证据(来自 PR 自身 CI,经 API 获取)审查时 本次不给沙箱验证行:PR 没有需要了结的行为性论断——它删除一个测试,可核对的就是 CI 与上述结构性论证。若维护者之后想取回端到端覆盖, — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — the flake is real, the deleted assertion measured host scheduling rather than a product contract, and every coverage claim in the PR body checked out; the one reservation is a coverage-policy call the author already made with eyes open. The honest reflection here is about the pivot, because it changes what this PR is. It opened as a production fix (preload the snapshot), then rewrote the test, then deleted it. Reading the final state against the code: the old E2E asserted an unconditional first-request guarantee that the recall design never promises — the initial delivery point is a bounded 100 ms wait, and the model-selected result has a second, guaranteed delivery point at the tool result. So the three-time failure on the loaded ECS host was the test measuring storage and event-loop tail latency, not the product breaking. Once that's established, the remaining question is only how much test coverage the fix should keep. I'd have kept the deterministic rewrite ( Standing follow-ups, none blocking: if auto-memory recall regresses end-to-end (real wiring, not unit seams), nothing will fail until it surfaces on main E2E or in telemetry; the rewrite is in this PR's history if that gap ever hurts. The flake fix itself is by construction — the timing-sensitive assertion no longer exists — and post-merge main E2E will confirm issue #10701 closes clean. CI is still running on this commit, so approval is deferred until CI lands green on 中文说明置信度:4/5 —— flake 真实存在,被删断言测量的是宿主机调度而非产品契约,PR 正文中的每一项覆盖声称都查证属实;唯一的保留意见是一个作者已在知情下做出的覆盖策略决定。 这里诚实的反思在于方案转变,因为它改变了这个 PR 的性质。它以生产侧修复(预载快照)开出,随后重写测试,最后删除测试。把最终状态对照代码来看:旧 E2E 断言的是无条件的首请求保证,而 recall 设计从不承诺这一点——初始投递点是有限的 100 ms 等待,模型选择结果还有第二个保证送达的投递点(工具结果处)。因此负载较高的 ECS 主机上连续三次失败,是测试在测量存储与事件循环尾延迟,而不是产品出了故障。确立这一点后,剩下的问题只是修复应保留多少测试覆盖。 我本会保留确定性重写( 遗留跟进事项,均非阻塞:如果 auto-memory recall 在端到端层面(真实接线,而非单测接缝)回归,在 main E2E 或遥测暴露之前不会有任何测试失败;如果这个缺口将来造成困扰,重写版本就在本 PR 历史里。flake 修复本身是构造性的——时序敏感断言已不存在——合并后的 main E2E 将确认 issue #10701 干净关闭。 该 commit 的 CI 仍在运行,因此批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
Review @
|
|
Released in v0.23.0. |
What this PR does
This PR removes the bundled ACP auto-memory E2E that requires a real filesystem scan to publish a deterministic result inside the production 100 ms initial-recall window.
The production behavior remains covered at the two deterministic seams that own the contract: fake-timer client tests verify the 100 ms wait, early fast-result delivery, timeout behavior, cancellation, and deduplication; ACP Session tests verify recall before the initial send and refined recall after tool responses. No production memory, configuration, timeout, or test-concurrency code changes.
Why it's needed
Main E2E run 33501336475, job 99835166630 failed this assertion three times on
ecs-qwen-hk3-21, while the same commit passed on macOS. The job already had one Vitest worker after #10567.#9992 defines the first-request behavior conditionally: recalled memory is included when fast Recall completes inside the bounded initial window. The removed E2E turns that into an unconditional real-time guarantee and therefore measures shared-runner scheduling and filesystem tail latency rather than a stable product contract. Replacing it with another orchestrated E2E would duplicate the existing deterministic tests with more moving parts.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: the E2E failed whenever a loaded runner did not finish memory scanning within 100 ms, even though missing that bounded window is a supported path.
After: the wall-clock assertion is gone; the same behavior remains protected by deterministic tests at the client and ACP Session boundaries.
Tested on
Environment (optional)
Local isolated worktree on Node.js 22.22.0. The reused dependency tree is stale against current
main, so clean-install execution is delegated to PR CI.Risk & Scope
Linked Issues
Fixes #10701
Related: #9992, #10567
中文说明
本 PR 做了什么
本 PR 删除 bundled ACP auto-memory E2E 中依赖真实文件系统扫描在生产 100 ms 初始 Recall 窗口内发布 deterministic result 的用例。
生产行为仍由两个真正拥有该契约的确定性测试层覆盖:client fake-timer 单测验证 100 ms 等待、fast result 提前交付、超时、取消和去重;ACP Session 单测验证首轮发送前 recall,以及工具响应后的 refined recall。本 PR 不修改生产 memory、配置、timeout 或测试并发代码。
为什么需要
Main E2E run 33501336475 的 job 99835166630 在
ecs-qwen-hk3-21上连续三次失败,而同一 commit 在 macOS 上通过。该 job 在 #10567 之后已经只有一个 Vitest worker。#9992 对首个请求的定义是有条件的:fast Recall 在有限初始窗口内完成时才注入 recalled memory。被删除的 E2E 把它变成无条件实时保证,因此实际测量的是共享 runner 调度和文件系统尾延迟,而不是稳定的产品契约。再用另一套编排式 E2E 替代,只会用更多状态重复已有的确定性测试。
Reviewer Test Plan
如何验证
前后证据
修改前:只要高负载 runner 没有在 100 ms 内完成 memory scan,E2E 就失败,即使错过有限窗口本身是受支持路径。
修改后:墙钟断言被删除;同一行为仍由 client 与 ACP Session 边界上的确定性测试保护。
测试环境
环境(可选)
Node.js 22.22.0 的本地隔离 worktree。复用依赖树相对当前
main已过期,因此 clean-install 执行交给 PR CI。风险与范围
关联 Issue
Fixes #10701
Related: #9992, #10567