[PR #6294] test(javascript): order the caller abort by frame, not wall clock #7262

Open
opened 2026-11-01 21:46:34 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6294
Author: @cainiao1992
Created: 10/22/2026
Status: 🔄 Open

Base: main ← Head: fix/worker-order-audit-abort-timing


📝 Commits (1)

  • 90cd530 test(javascript): order the caller abort by frame, not wall clock

📊 Changes

1 file changed (+6 additions, -3 deletions)

View changed files

📝 tests/boundary/javascript/workerOrderAudit.test.ts (+6 -3)

📄 Description

Summary

Stabilizes tests/boundary/javascript/workerOrderAudit.test.ts case retains coalesced static evidence when abort follows a resource frame, which failed in CI (Test shard (3/8) of run 39743487960) with AssertionError: expected false to be true on controller.signal.aborted. The test ordered the caller abort with a 5 ms wall-clock timer; it now orders it with the worker protocol itself while keeping the coalesced static-plus-failure dispatch.

Problem and expected behavior

The regression intends to prove that an abort delivered after the worker reported a failure (reason: "resource") frame leaves that resource failure primary and keeps the already-coalesced static evidence. The fixture wrote the static frame and the resource frame in one write and armed setTimeout(() => controller.abort(null), 5) when the chunk surfaced, so the abort had to win a sub-millisecond race against the runtime's own completion path (checkpoint read, admission reply, terminal frame, temp-file removal). That path can run entirely within one event-loop iteration, while the timer is only observed in a later timers phase, so under CI load the abort arrived after analyze() had already resolved — the assertion that the abort was delivered inside the operation then failed. Nothing about the runtime behavior was wrong; the ordering was asserted with wall-clock timing.

Expected behavior after the change: the frame that establishes the ordering is the protocol frame itself, and the abort is delivered at a point the implementation defines, independent of machine speed.

Change and scope

  • The fixture still writes the static checkpoint frame and the resource failure frame in one write, so the coalesced multi-frame dispatch this case names and covers is unchanged. Only the abort ordering changed.
  • The abort is triggered by the observed failure frame through the file's existing observeWorkerFrames helper and deferred by one microtask, which places it behind the runtime's own reader for that chunk: the failure is recorded as primary before cancellation is delivered, and the frame loop still observes the cancellation before resolving. No timer, no delay constant.
  • Assertions are unchanged: abort delivered, AnalysisResourceConstraintError with reason: "resource failure before abort", retained javascript.parse_status === "complete".

Intentionally not included: the other four cases in the same file that still order aborts with setTimeout:

  • clears a consumed resource frame before the next reusable worker job (5 ms; the abort has to follow a response in a second reusable job, so the observation point is a different job's frame).
  • selects a control-frame budget failure observed before caller abort (20 ms; the abort is armed by accumulated stdout bytes past 64 KiB, where no parseable frame exists to key on and the timer is the ordering itself).
  • preserves a complete current-job failure parsed before caller abort (5 ms; convertible to the same frame ordering, but it needs its own phase-frame sequencing and is a separate change).
  • does not let a wrong-job failure frame override later caller abort (5 ms; same).

They passed in CI and are left alone rather than converted speculatively; the last two are the natural follow-up if this ordering proves stable.

Follow the contribution scope: not applicable — a test-only stabilization inside existing boundaries; no new provider and no capability involving target execution, injection, mutation, or protection recovery.

Contract and boundary impact

  • Semantic owner and earliest changed stage: JavaScript analysis worker regression test only; no production code changed.
  • Contribution scope fit and linked maintainer decision: not applicable (test stabilization; ordinary fix within existing boundaries).
  • CLI and MCP/tool-catalog contract: none.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: none; the covered contract is "resource failure recorded before cancellation stays primary and retains static evidence".
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

The test launches fixture child processes only; no analyzed-target interaction is introduced or changed.

Evidence and regression coverage

  • Tests added or updated: tests/boundary/javascript/workerOrderAudit.test.ts — retains coalesced static evidence when abort follows a resource frame reordered as described, with the single-write static-plus-failure dispatch preserved. No other case changed.
  • Base reproduction or other evidence: the failure is observed at the affected base revision in CI, CI required run 39743487960, Test shard (3/8): AssertionError: expected false to be true at expect(controller.signal.aborted).toBe(true). It was not reproduced locally before the change (30 CPU-contention loops on the whole file still passed), so the CI log is the reproduction evidence, not a local one.
  • User-visible CLI/MCP output (if applicable): not applicable.
  • Remaining proof gaps: the ordering proof is now structural rather than empirical; the previously flaky delivery point is no longer time-based. Local stress runs cannot prove the original CI timing was the only cause, only that the timing dependency is gone.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npx vitest run tests/boundary/javascript/workerOrderAudit.test.ts — 35/35 passed, on this head rebased onto 2b8fb387b
  • npx vitest run tests/boundary/javascript/workerOrderAudit.test.ts -t "retains coalesced static evidence when abort follows a resource frame" — 5/5 consecutive runs, same head
  • Ordering-sensitivity check: replacing the deferred abort with a synchronous controller.abort(null) in the same observation makes the case fail with AnalysisCancelledError instead of the resource failure, which confirms the case still asserts "failure recorded before cancellation" rather than passing vacuously; re-run on this head
  • npx oxfmt --check tests/boundary/javascript/workerOrderAudit.test.ts — formatted
  • npx oxlint tests/boundary/javascript/workerOrderAudit.test.ts — 0 warnings, 0 errors
  • npm run check:fast — passed

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable (fixture child processes only).
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no production behavior changed; the fixture child processes are spawned through the existing spawnOwnedProviderProcess owner.

Author readiness

Human owner: @cainiao1992

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/6294 **Author:** [@cainiao1992](https://github.com/cainiao1992) **Created:** 10/22/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/worker-order-audit-abort-timing` --- ### 📝 Commits (1) - [`90cd530`](https://github.com/morluto/rea/commit/90cd530a8c21cf812e0969b2134504eec1d8c1b6) test(javascript): order the caller abort by frame, not wall clock ### 📊 Changes **1 file changed** (+6 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `tests/boundary/javascript/workerOrderAudit.test.ts` (+6 -3) </details> ### 📄 Description ## Summary Stabilizes `tests/boundary/javascript/workerOrderAudit.test.ts` case `retains coalesced static evidence when abort follows a resource frame`, which failed in CI (`Test shard (3/8)` of run 39743487960) with `AssertionError: expected false to be true` on `controller.signal.aborted`. The test ordered the caller abort with a 5 ms wall-clock timer; it now orders it with the worker protocol itself while keeping the coalesced static-plus-failure dispatch. ## Problem and expected behavior The regression intends to prove that an abort delivered after the worker reported a `failure` (`reason: "resource"`) frame leaves that resource failure primary and keeps the already-coalesced static evidence. The fixture wrote the static frame and the resource frame in one write and armed `setTimeout(() => controller.abort(null), 5)` when the chunk surfaced, so the abort had to win a sub-millisecond race against the runtime's own completion path (checkpoint read, admission reply, terminal frame, temp-file removal). That path can run entirely within one event-loop iteration, while the timer is only observed in a later timers phase, so under CI load the abort arrived after `analyze()` had already resolved — the assertion that the abort was delivered inside the operation then failed. Nothing about the runtime behavior was wrong; the ordering was asserted with wall-clock timing. Expected behavior after the change: the frame that establishes the ordering is the protocol frame itself, and the abort is delivered at a point the implementation defines, independent of machine speed. ## Change and scope - The fixture still writes the static checkpoint frame and the resource `failure` frame in one write, so the coalesced multi-frame dispatch this case names and covers is unchanged. Only the abort ordering changed. - The abort is triggered by the observed `failure` frame through the file's existing `observeWorkerFrames` helper and deferred by one microtask, which places it behind the runtime's own reader for that chunk: the failure is recorded as primary before cancellation is delivered, and the frame loop still observes the cancellation before resolving. No timer, no delay constant. - Assertions are unchanged: abort delivered, `AnalysisResourceConstraintError` with `reason: "resource failure before abort"`, retained `javascript.parse_status === "complete"`. Intentionally not included: the other four cases in the same file that still order aborts with `setTimeout`: - `clears a consumed resource frame before the next reusable worker job` (5 ms; the abort has to follow a response in a second reusable job, so the observation point is a different job's frame). - `selects a control-frame budget failure observed before caller abort` (20 ms; the abort is armed by accumulated stdout bytes past 64 KiB, where no parseable frame exists to key on and the timer is the ordering itself). - `preserves a complete current-job failure parsed before caller abort` (5 ms; convertible to the same frame ordering, but it needs its own phase-frame sequencing and is a separate change). - `does not let a wrong-job failure frame override later caller abort` (5 ms; same). They passed in CI and are left alone rather than converted speculatively; the last two are the natural follow-up if this ordering proves stable. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope): not applicable — a test-only stabilization inside existing boundaries; no new provider and no capability involving target execution, injection, mutation, or protection recovery. ## Contract and boundary impact - Semantic owner and earliest changed stage: JavaScript analysis worker regression test only; no production code changed. - Contribution scope fit and linked maintainer decision: not applicable (test stabilization; ordinary fix within existing boundaries). - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none; the covered contract is "resource failure recorded before cancellation stays primary and retains static evidence". - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [ ] Read existing files or captured data. - [x] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. The test launches fixture child processes only; no analyzed-target interaction is introduced or changed. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/javascript/workerOrderAudit.test.ts` — `retains coalesced static evidence when abort follows a resource frame` reordered as described, with the single-write static-plus-failure dispatch preserved. No other case changed. - Base reproduction or other evidence: the failure is observed at the affected base revision in CI, `CI required` run 39743487960, `Test shard (3/8)`: `AssertionError: expected false to be true` at `expect(controller.signal.aborted).toBe(true)`. It was not reproduced locally before the change (30 CPU-contention loops on the whole file still passed), so the CI log is the reproduction evidence, not a local one. - User-visible CLI/MCP output (if applicable): not applicable. - Remaining proof gaps: the ordering proof is now structural rather than empirical; the previously flaky delivery point is no longer time-based. Local stress runs cannot prove the original CI timing was the only cause, only that the timing dependency is gone. For evidence-bearing changes: - [ ] Observed, derived, and inferred claims remain distinguishable. - [ ] Artifact identity, source provenance, and failed attempts remain preserved. - [ ] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run tests/boundary/javascript/workerOrderAudit.test.ts` — 35/35 passed, on this head rebased onto `2b8fb387b` - `npx vitest run tests/boundary/javascript/workerOrderAudit.test.ts -t "retains coalesced static evidence when abort follows a resource frame"` — 5/5 consecutive runs, same head - Ordering-sensitivity check: replacing the deferred abort with a synchronous `controller.abort(null)` in the same observation makes the case fail with `AnalysisCancelledError` instead of the resource failure, which confirms the case still asserts "failure recorded before cancellation" rather than passing vacuously; re-run on this head - `npx oxfmt --check tests/boundary/javascript/workerOrderAudit.test.ts` — formatted - `npx oxlint tests/boundary/javascript/workerOrderAudit.test.ts` — 0 warnings, 0 errors - `npm run check:fast` — passed ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (fixture child processes only). - Package or release metadata impact: none. - Security, privacy, process, or containment review: no production behavior changed; the fixture child processes are spawned through the existing `spawnOwnedProviderProcess` owner. ## Author readiness <!-- The human owner completes these declarations before requesting review. The PR author is the owner by default. A different owner must explicitly accept ownership on this PR. Draft PRs may leave them unchecked. Keep the description and validation evidence current as the work changes. See CONTRIBUTING.md#pull-request-ownership-and-readiness. --> Human owner: @cainiao1992 - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#7262
No description provided.