test(computer-use): remove the remaining fixed-delay races from cancellation coverage - #2394
Conversation
…llation coverage The abort-path cancellation tests waited a fixed 120ms/80ms before aborting, and asserted on the mock executor's log with a single read. The caller's settlement on the unanswered-cancel path is driven by the host's CANCEL_GRACE_MS timer and SIGKILL, which is unsynchronized with the child's appendFileSync — under parallel CI load a single read can observe the log before the record lands. Replace the pre-abort delays with the window.list barrier round trip that apache#2345 established for the session-clearing test (in-order pipe + in-order mock processing prove observe reached the executor), and poll the log with a bounded 2s deadline where settlement is host-timer-driven (abort grace and deadline paths). Tests whose settlement is driven by the executor's own cancel answer keep the single read: the mock logs before it responds, so the record is on disk before the host resolves. Closes apache#2332
An independent adversarial pass on the previous commit found three
defects, none affecting what the tests can catch but all real:
- The barrier's ordering guarantee silently depends on ensureStarted
having settled (with a signal-carrying call and a bare call, the
not-ready path reorders the stdin writes — reproduced 4/4 with the
pre-warm removed). Document the invariant where the pattern first
appears.
- A failing barrier assertion left the in-flight observe call an
unhandled rejection (reproduced under node:test). Settle it with
void call.catch(() => {}) in all three barrier tests.
- waitForRecord's deadline branch re-evaluated the predicate on the
same array inside assert.ok — a tautologically false assert with an
unreachable return. Write it as assert.fail.
Suite: 134 pass. Stability: 20 runs at 4-way concurrency, 0 failures.
|
Ran an independent adversarial review against the first commit (a reviewer agent instructed to refute every claim, with access to the SUT source). Outcome, and what 67894be changes: Claims that survived active refutation attempts:
Findings fixed in 67894be:
Re-verified after the fixes: 134 pass, biome clean, 20 runs at 4-way concurrency with 0 failures. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the barrier replacements are genuinely causal (stdin FIFO + mock line-order processing, verified), the bounded waitForRecord is a real improvement over fixed delays, and 9 runs under CPU load were all green. Mutation checks confirm the tests still catch the old failure modes.
One P2 before claiming the issue done: maka-cu-backend.test.ts:1166 still has the delay(120) race for session.end — the exact spot #2345's review flagged, and this PR's 'Closes #2332' claim doesn't cover it (the 'Deliberately unchanged' list only names the service file's three delays). clearSession's fire-and-forget void service.call('session.end') has no causal order with the read; under parallel CI load the record can arrive after the 120ms window and the assert fails — same shape as the original #2332 flake. Either await a session.end round-trip (have clearSession return the promise, or barrier + waitForRecord), or record the deferral explicitly in the PR. P3s: the delay(80) at :487 can vacuous-pass a retry regression (window is small but it's the same timing-guess class), and the delay(200) at :235 is failure-direction (can flake under load) though it's already tracked in #2389. Approving.
…flagged Review follow-up (P2 + P3s): - session.end after clearSession (the P2): clearSession fires session.end without exposing the round-trip, and the delay(120) before reading the child's log was the same shape as the original apache#2332 flake. The backend test now waits on the delivered record itself via the same bounded waitForRecord the service test uses. - service_mismatch spawn accounting: the delay(80) could vacuous-pass a retry regression whose respawn lands after the window. Dropped in favor of the causal ordering that was already there: a wrongly-retried spawn writes its `start` before replying to the handshake the awaited preflight consumed, and the retry path would surface as restart_exhausted, failing the rejects matcher. Documented at the call site. - The two delay(200)s in the service test: the stray-null check now anchors on a subsequent window.list round-trip (the stray line sits ahead of the response on stdout, so it was processed before the call returned), and the junk-line teardown polls the state fact with a bounded deadline. 134/134 pass after a clean build; the two touched files stay green across 5 repeated runs.
|
P2 and both P3s addressed in 3c2339b:
134/134 after a clean build; the two touched files stay green across 5 repeated runs. With these, the delay-based waits left in the package are only the mock-internal scheduled behaviors, so I'd read #2332's "audit the neighboring cancellation tests" clause as satisfied now. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the previous P2 and P3 findings are addressed, and the overall direction is solid. Replacing timing guesses with observable synchronization is the right seam here. Approving with one non-blocking test-quality suggestion:
P3 — In the afterHello: 'null' test, the new window.list round trip is not yet a causal barrier for the stray line. The mock emits AFTER_HELLO from a 30 ms timer after the handshake, so window.list may be answered before null is written. The assertion can therefore run before the behavior under test occurs. A small observable marker emitted when AFTER_HELLO is written, followed by the round trip, would establish the intended ordering without restoring a fixed wait.
This is test-only and non-blocking; the production surface is unchanged. Approved.
简体中文
感谢更新——之前的 P2 和 P3 已经解决,整体方向也正确。用可观察的同步机制替代固定等待,是这里合适的方案。Approve,另留一个不阻塞合并的测试建议:
P3 — 在 afterHello: 'null' 测试中,新的 window.list round trip 还不能证明 null 已经被处理。Mock 在握手结束后通过 30 ms 定时器输出 AFTER_HELLO,因此 window.list 可能先返回,断言会在真正被测行为发生前通过。可以在输出 AFTER_HELLO 时写入一个可观察 marker,测试等待 marker 后再做 round trip,从而建立完整的因果顺序,而不必恢复固定等待。
这只影响测试质量,不涉及生产代码,因此不阻塞合并。Approve。
…rker (#2471) Post-merge review follow-up on #2394: the window.list round trip was claimed as a causal anchor for the stray `null` line, but the mock emits AFTER_HELLO from a 30ms timer after the handshake — the round trip could be answered before the stray line was ever written, and the assertion could pass before the behavior under test occurred. The mock now logs an `after-hello` marker once the stray lines are on stdout, and the test waits for that marker (bounded waitForRecord) before the round trip. With the line provably ahead of the response on the pipe, stream ordering makes the anchor real: the stray line was processed — and would have thrown in the data listener — before the call returned. 17/17 service tests green across 5 repeated runs after a clean build.
Summary
Closes #2332.
#2345 already replaced the fixed delays in the test the issue names (
clears one session without ending the others) with awindow.listbarrier — this PR finishes the issue's remaining ask: audit the neighboring cancellation tests for the same fixed-delay pattern, and remove the two instances that survive.Two distinct races were still present in the
cancelling a delivered request (§7.2/§7.3)suite:settles an aborted delivered request…waited 120 ms andlets the executor answer a cancel…waited 80 ms beforecontroller.abort(), guessing thatobservehad reached the executor. Both now use the same barrier round trip test(computer-use): synchronize session cancellation #2345 established: the pipe is ordered and the mock processes stdin lines in order, so an answeredwindow.listproves the earlierobservewas received and logged.asks before killing on the deadline…), the caller settles when the host'sCANCEL_GRACE_MStimer SIGKILLs the child (maka-cu-service.tsrequestCancel/kill) — nothing orders that settlement after the child'sappendFileSyncof thecancelrecord, and a SIGKILLed process can still get a last scheduler slice. A singlereadRecords()after the rejection can therefore miss the record under parallel CI load — the same class of failure that broketest_workspacesonmainafter feat(cli): add Runtime Host-backed TUI sessions #2308. These assertions now poll with a bounded 2 s deadline (waitForRecord), which adds zero time to passing runs (first read hits) and still fails promptly, with the log contents in the message, on a real regression.Deliberately unchanged:
answerCancelthe mock logs before it responds, so the record is on disk strictly before the host resolves the caller — that read is deterministic.delay()calls outside the cancellation suite (uncaught-exception settle, teardown-after-bad-lines, dispose purge) are absence/settling waits of a different shape; they belong to the broader perf(test): replace fixed waits with observable completion #2389 sweep, not this issue.Verification
npm --workspace @maka/computer-use run test— 134 pass, 0 fail.maka-cu-service.test.jsat 4-way concurrency — 0 failures.npx biome checkon the changed file — clean.