Skip to content

test(computer-use): synchronize session cancellation - #2345

Merged
Astro-Han merged 1 commit into
apache:mainfrom
liugddx:test/computer-use-cancel-synchronization
Aug 7, 2026
Merged

test(computer-use): synchronize session cancellation#2345
Astro-Han merged 1 commit into
apache:mainfrom
liugddx:test/computer-use-cancel-synchronization

Conversation

@liugddx

@liugddx liugddx commented Aug 6, 2026

Copy link
Copy Markdown
Member

Summary

  • make the session-clear test wait for a same-pipe round trip before cancelling the silent request
  • have the mock executor acknowledge cancellation and await that causal response before reading its log
  • remove two fixed 80ms sleeps while retaining state, generation, and cancel-log assertions

Why

clearSession() synchronously sends $/cancel; the CI flake came from reading the child-process log after a fixed delay that did not guarantee the child had consumed stdin and appended the record. The mock records cancel before sending its response, so awaiting that response is a deterministic acknowledgement instead of a scheduling guess.

Validation

  • npm --workspace @maka/computer-use run build
  • target test repeated 30 times
  • node --test packages/computer-use/dist/__tests__/maka-cu-service.test.js (17 pass)
  • Biome and git diff --check

@liugddx
liugddx force-pushed the test/computer-use-cancel-synchronization branch from f158063 to 85ce4c0 Compare August 7, 2026 00:20

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the barrier-based synchronization is genuinely causal, not another timing guess: the stdin FIFO guarantees window.list is answered only after the mock consumed the observe line, and await call waits for the cancel reply (and its cleanup of the kill timer). 30 runs including a CPU-saturated pass all green, and the old test was arguably hollow (mock never answered cancel, so the child was doomed to SIGKILL at +2s anyway — the assertions passed right before the behavior they claimed to verify broke). The new test strengthens rather than compromises the semantics.

One follow-up suggestion (not blocking, P3): the sibling file maka-cu-backend.test.ts:1165-1168 still waits a fixed delay(120) for the fire-and-forget session.end — the exact CI-flake mechanism this PR's description names. The same barrier trick (a round-trip await after clearSession) would make it deterministic too.

Merging.

@Astro-Han
Astro-Han merged commit 04c2b1f into apache:main Aug 7, 2026
11 checks passed
Astro-Han pushed a commit that referenced this pull request Aug 8, 2026
…llation coverage (#2394)

* test(computer-use): remove the remaining fixed-delay races from cancellation 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 #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 #2332

* test(computer-use): address adversarial review of the cancellation sync

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.

* test(computer-use): close the remaining fixed-delay races the review 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 #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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants