test(sdk-java): widen the SSE idle-watchdog margins in the slow-line test - #7617
Conversation
…test slowSseLineBytesKeepIdleWatchdogAlive drove the event out in 20-byte steps 50ms apart against a 150ms idle timeout, so any observed gap above 150ms tripped the watchdog. Each idle close restarts the whole event from the handler, and the 3s observation budget only covered about three attempts, so a few slow steps exhausted the reconnects. The test failed this way on macOS runners repeatedly, most recently on main. Send the event in 8-byte steps and raise the idle timeout to 500ms, which keeps the property under test intact: the event now needs about a second of 50ms steps to arrive, so a watchdog fed only by whole frames still expires well inside the run. Raise the observation timeout so a single slow step is absorbed by a reconnect instead of ending the prompt. Locally the old test starts failing once steps reach 260ms; the new one survives 400ms steps and still fails, as it should, when SseReader stops reporting per-byte activity.
|
Thanks for the PR — the root-cause analysis here is excellent. Template: the sections don't match the repo template headings exactly ( Problem: observed and well-evidenced. Four CI runs linked (30025946135 on Direction: test stability fix, squarely in scope. No product behavior changes. Size: not applicable — single test file in Approach: the scope is exactly right. Three small changes that each address a specific part of the failure mode: smaller steps (more frequent activity signals), wider idle timeout (10× margin instead of 3×), and a longer observation budget so one slow step gets absorbed by a reconnect instead of ending the prompt. The ratio-based argument for why the test property is preserved is sound — and the "neutering SseReader" counter-check in the description is a nice touch that proves the test still guards what it should. Moving on to code review. 🔍 中文说明感谢贡献——根因分析写得非常好。 模板:章节标题和仓库模板不完全一致( 问题:已观测到的真实 flake,有四个 CI run 链接为证(30025946135 在 方向:测试稳定性修复,完全在范围内,不涉及产品行为变更。 规模:不适用——仅改动 方案:范围恰到好处。三处小改动分别针对失败模式的不同环节:更小的步进(更频繁的活动信号)、更宽的 idle 超时(10 倍余量取代 3 倍)、更长的观测预算让偶发慢步可以被一次重连吸收。基于比例而非绝对值的论证是成立的——描述中"把 SseReader 改成按帧上报"的反向验证也很好,证明测试仍然守得住它该守的回归。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: for a flaky test where a 50ms step interval races a 150ms idle watchdog, I'd widen the timeout-to-step ratio (the 3× margin is the root cause), shrink the step size so activity signals arrive more frequently, and give the observation budget enough headroom that a single reconnect doesn't exhaust it. Comparison with the diff: the PR does exactly this, and the execution is tight. Three changes, each addressing a distinct part of the failure mode:
The added comment explaining why the test property is preserved (event delivery >1s, watchdog at 500ms, so a frame-only watchdog still expires) is helpful and accurate. No correctness issues, no regressions, no scope creep. The change follows the existing test style. Nothing to flag. Real-Scenario TestingThis is a test-only change in the Java SDK ( Java/Maven is not available in this review environment, so I could not run The logic checks out on inspection: with 8-byte steps at 50ms intervals, the 173-byte event takes ~1.1s to deliver against a 500ms idle timeout — the watchdog is fed by partial-line bytes throughout delivery, and would still expire if it only counted whole frames. 中文说明代码审查独立方案: 对于 50ms 步进与 150ms idle 看门狗竞争导致的 flaky 测试,我会拉大超时与步进的比值(3 倍余量是根因)、缩小步进让活动信号更频繁、并给观测预算留出足够余量使单次重连不会耗尽它。 与 diff 的对比: PR 正是这样做的,而且执行得很干净。三处改动分别针对失败模式的不同环节:
新增的注释解释了为什么测试属性得以保留(事件传输 >1 秒,看门狗 500ms,因此只按整帧计数的看门狗仍会超时),准确且有帮助。 无正确性问题、无回归、无范围蔓延。改动遵循现有测试风格。无需标记的问题。 真实场景测试这是一个纯测试改动(Java SDK 的 本审查环境没有 Java/Maven,无法直接运行 从代码逻辑上看:8 字节步进、50ms 间隔,173 字节事件约需 1.1 秒传输,idle 超时 500ms——看门狗在传输全程被不完整行的字节喂活,如果只按整帧计数则仍会超时。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — textbook flaky-test fix: real CI evidence, precise root cause, minimal change, and the test still guards what it should. The PR matches my independent proposal exactly — widen the timeout-to-step ratio, shrink the step size, and give the observation budget headroom. What stands out is the quality of the analysis: the root cause is nailed down to the 3× margin being too tight for loaded runners, the fix is three targeted changes with no drive-by edits, and the "neutering SseReader" counter-check proves the test isn't vacuous after the change. The decision to leave the other two one-off flakes alone (different root causes, can't reproduce) shows good judgment rather than patching on a guess. Nothing to maintain in six months — the numbers are self-explanatory and the comment captures the invariant. 中文说明置信度:5/5 —— 教科书级的 flaky test 修复:真实的 CI 证据、精确的根因、最小的改动,而且测试仍然守得住它该守的回归。 PR 与我的独立方案完全一致——拉大超时与步进的比值、缩小步进、给观测预算留余量。突出的是分析质量:根因精确定位到 3 倍余量对负载高的 runner 太紧,修复是三处针对性改动、没有顺手编辑,"把 SseReader 改成按帧上报"的反向验证证明修改后测试不是空断言。对另外两个各失败过一次的 flake 不做猜测性修改(根因不同、本地无法复现),体现了好的判断力。 六个月后维护也没有负担——数值不言自明,注释记录了不变量。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Straightforward flaky test fix — widens SSE idle-watchdog margins (150ms → 500ms) and adds explicit promptObservationTimeout. Chunk size reduced from 20 to 8 bytes for more granular line splitting.
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Problem
DaemonSessionClientTest.slowSseLineBytesKeepIdleWatchdogAlivefails intermittently, most recently onmain(job):It has failed the same way on
macos-latest / Java 21in several earlier runs (30012191939, 30011211178, 29980041650). This one is test-side timing tolerance, not a product defect.Root cause
The test writes the 173-byte terminal event in 20-byte steps 50ms apart, against
sseIdleTimeout(150ms). The margin between one step and the watchdog is 3×, so any step the client observes more than 150ms apart — server thread scheduling, a flush, a GC pause on a loaded runner — trips the idle watchdog.That alone would be survivable, except an idle close restarts the whole event: the handler replays it from byte 0, so every attempt costs another full delivery. With
maximumReconnectAttempts(2)and the 3s observation budget fromclientBuilder(), only about three attempts fit, and a handful of slow steps exhausts them.Locally the current test starts failing once the steps reach 260ms, and 200ms steps already take 2.4s of the 3s budget.
Fix
Send the event in 8-byte steps and raise the idle timeout to 500ms. The property under test is preserved, because it depends on the ratio rather than the absolute numbers: a step must stay under the idle timeout while the whole event must take longer than it. The event now needs ~1.1s of 50ms steps to arrive, so a watchdog fed only by whole frames still expires well inside the run — and a slow runner only stretches the delivery, which strengthens that direction.
Also raise the observation timeout for this test so one slow step is absorbed by a reconnect instead of ending the prompt.
Verification
mvn clean test— 108 tests, green;mvn checkstyle:check— 0 violations.SseReaderso activity is reported per frame instead of per byte makes the new test fail with the exact CI error, so it still catches the regression it exists for.Not covered here
Two other tests in this class have failed once each in CI —
rejectsClientWidePromptCapacityBeforeMutation(detach timeout, ubuntu/11) andacceptanceContinuationDoesNotHoldSessionLifecycleLock(windows/21). Neither reproduces locally and neither shares this root cause, so they are left alone rather than patched on a guess.中文说明
问题
DaemonSessionClientTest.slowSseLineBytesKeepIdleWatchdogAlive间歇性失败,最近一次直接挂在main上(job):此前在
macos-latest / Java 21上已经以同样方式失败过多次(30012191939、30011211178、29980041650)。这一个属于测试自身的时间容忍度问题,不是产品缺陷。根因
测试把 173 字节的 terminal 事件按 20 字节一步、每步间隔 50ms 写出,而客户端
sseIdleTimeout(150ms)。一步与看门狗之间只有 3 倍余量,因此只要客户端观测到的某一步间隔超过 150ms —— 服务端线程调度、一次 flush、负载高时的一次 GC 停顿 —— 就会触发 idle 看门狗。单次触发本来还能扛过去,问题在于 idle 关流会让整个事件重来:handler 从第 0 字节重新发一遍,于是每次重试都要付出一次完整的传输时间。而
clientBuilder()给的是maximumReconnectAttempts(2)加 3 秒观测预算,总共只够约三次尝试,几步慢一点就把重试次数耗光。本地实测:当前这个测试在步长间隔达到 260ms 时开始失败;间隔 200ms 时已经吃掉 3 秒预算里的 2.4 秒。
修复
改成 8 字节一步,并把 idle 超时提到 500ms。被测性质得以保留,因为它依赖的是比例而非绝对数值:单步必须小于 idle 超时,而整个事件必须长于 idle 超时。现在整个事件需要约 1.1 秒的 50ms 步进才能送完,因此一个只按整帧喂食的看门狗仍然会在测试跑完之前超时 —— 而机器越慢,传输只会越长,这个方向反而更稳。
同时为这个测试放宽观测超时,让偶发的一步慢可以由一次重连吸收,而不是直接结束 prompt。
验证
mvn clean test—— 108 个测试全绿;mvn checkstyle:check—— 0 violations。SseReader改成按帧而非按字节上报活跃度后,新版测试会以和 CI 完全一致的错误失败,说明它仍然守得住它该守的回归。本 PR 未覆盖的部分
这个测试类里另有两个测试各在 CI 上失败过一次 ——
rejectsClientWidePromptCapacityBeforeMutation(detach 超时,ubuntu/11)和acceptanceContinuationDoesNotHoldSessionLifecycleLock(windows/21)。两者本地都无法复现,也都不是本次这个根因,因此不做猜测性修改,留待单独排查。