morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #4961] commandFinalizationCancellation fails on main CI depending on which worker runs it #3073

New issue

Closed

opened 2026-10-31 00:35:59 +00:00 by bsakaguchi · 4 comments

bsakaguchi commented

2026-10-31 00:35:59 +00:00

Owner

Originally created by @Adolanium on GitHub (Oct 18, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/4961

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

Other

Observed behavior

tests/boundary/cli/commandFinalizationCancellation.test.ts fails intermittently on main CI. Three of its five cases fail: interrupt, primary-failure, and interrupt-cleanup-failure. The same commit has both passing and failing CI runs: 47d3c6b1a passed in run 39090487270 and failed in run 39090782662, and 18aa9af2b failed in run 39094214574 and passed in run 39091913983.

FAIL cli-boundary tests/boundary/cli/commandFinalizationCancellation.test.ts > finishes the registered WASM command through final close: interrupt
AssertionError: expected { …(16) } to match object { Object (code, details) }
-   "code": "cancelled",
FAIL ... primary-failure
AssertionError: expected undefined to be 130
FAIL ... interrupt-cleanup-failure
AssertionError: expected undefined to be 130

Reproduction

Executed on Linux (WSL Ubuntu, Node 24.18.0) at 2c4338db1:

npx vitest run --project cli-boundary tests/boundary/cli/commandFinalizationCancellation.test.ts: 5 passed, three runs out of three.
npx vitest run --project cli-boundary --maxWorkers=1 --no-file-parallelism tests/boundary/cli/cliInputSchemaRequired.test.ts tests/boundary/cli/commandFinalizationCancellation.test.ts: the same three cases fail as on CI.

Expected behavior

The test passes regardless of which other CLI test files share its worker.

Impact and scope

Main CI and every PR's CI fail on some runs for reasons unrelated to the change under test. Only the test is affected. Production code is not.

REA version

6.6.0, revision 2c4338db1

Runtime and platform

GitHub Actions Linux runners, reproduced on WSL Ubuntu with Node 24.18.0.

Target and artifact context

No target.

Interface

Other

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

See the observed failures above. The test asserts host.listeners.size is 0, which also passes when the listeners went to the real process, so that assertion does not catch the problem.

Typed outcome or recovery expected

Not applicable.

Analysis or suspected cause

The cli-boundary project in vitest.config.ts sets isolate: false, and its comment says "module mocks belong in process-global". This test uses vi.mock on src/cli/commandCancellation.js to inject a fake signal host. When another file in the same worker has already loaded src/cli.ts (which imports wasmCommands and the real commandCancellation), the cached module is reused and the mock does not apply. withCommandCancellation then registers its listeners on the real process, host.listeners.get("SIGINT")?.() does nothing, no interrupt happens, and host.exitCode stays undefined. Whether the test fails depends on worker scheduling, which explains why the same commit both passes and fails.

Additional context

Moving the test to tests/process-global/ fixes it. With the move, the same pairing passes in three runs out of three, and npx vitest run --project cli-boundary --project process-global passes 934 tests across 140 files after npm run build:cached. The fix is commit github.com/Adolanium/rea@e3112897ad, and github.com/Adolanium/rea@d32d79eeb adds a guard so a bypassed mock fails every case. Both are on branch test/finalization-cancellation-process-global. Upstream's 3-open-PR limit for outside contributors blocks opening the PR right now. Feel free to cherry-pick them; otherwise I'll open the PR when a slot frees.

Originally created by @Adolanium on GitHub (Oct 18, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/4961 ### Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ### Area Other ### Observed behavior `tests/boundary/cli/commandFinalizationCancellation.test.ts` fails intermittently on main CI. Three of its five cases fail: `interrupt`, `primary-failure`, and `interrupt-cleanup-failure`. The same commit has both passing and failing CI runs: 47d3c6b1a passed in run 39090487270 and failed in run 39090782662, and 18aa9af2b failed in run 39094214574 and passed in run 39091913983. ``` FAIL cli-boundary tests/boundary/cli/commandFinalizationCancellation.test.ts > finishes the registered WASM command through final close: interrupt AssertionError: expected { …(16) } to match object { Object (code, details) } - "code": "cancelled", FAIL ... primary-failure AssertionError: expected undefined to be 130 FAIL ... interrupt-cleanup-failure AssertionError: expected undefined to be 130 ``` ### Reproduction Executed on Linux (WSL Ubuntu, Node 24.18.0) at 2c4338db1: 1. `npx vitest run --project cli-boundary tests/boundary/cli/commandFinalizationCancellation.test.ts`: 5 passed, three runs out of three. 2. `npx vitest run --project cli-boundary --maxWorkers=1 --no-file-parallelism tests/boundary/cli/cliInputSchemaRequired.test.ts tests/boundary/cli/commandFinalizationCancellation.test.ts`: the same three cases fail as on CI. ### Expected behavior The test passes regardless of which other CLI test files share its worker. ### Impact and scope Main CI and every PR's CI fail on some runs for reasons unrelated to the change under test. Only the test is affected. Production code is not. ### REA version 6.6.0, revision `2c4338db1` ### Runtime and platform GitHub Actions Linux runners, reproduced on WSL Ubuntu with Node 24.18.0. ### Target and artifact context No target. ### Interface Other ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence See the observed failures above. The test asserts `host.listeners.size` is 0, which also passes when the listeners went to the real `process`, so that assertion does not catch the problem. ### Typed outcome or recovery expected Not applicable. ### Analysis or suspected cause The `cli-boundary` project in `vitest.config.ts` sets `isolate: false`, and its comment says "module mocks belong in process-global". This test uses `vi.mock` on `src/cli/commandCancellation.js` to inject a fake signal `host`. When another file in the same worker has already loaded `src/cli.ts` (which imports `wasmCommands` and the real `commandCancellation`), the cached module is reused and the mock does not apply. `withCommandCancellation` then registers its listeners on the real `process`, `host.listeners.get("SIGINT")?.()` does nothing, no interrupt happens, and `host.exitCode` stays undefined. Whether the test fails depends on worker scheduling, which explains why the same commit both passes and fails. ### Additional context Moving the test to `tests/process-global/` fixes it. With the move, the same pairing passes in three runs out of three, and `npx vitest run --project cli-boundary --project process-global` passes 934 tests across 140 files after `npm run build:cached`. The fix is commit https://github.com/Adolanium/rea/commit/e3112897ad33e757f93691c1303125c3b7bfb55a, and https://github.com/Adolanium/rea/commit/d32d79eeb adds a guard so a bypassed mock fails every case. Both are on branch [`test/finalization-cancellation-process-global`](https://github.com/Adolanium/rea/tree/test/finalization-cancellation-process-global). Upstream's 3-open-PR limit for outside contributors blocks opening the PR right now. Feel free to cherry-pick them; otherwise I'll open the PR when a slot frees.

bsakaguchi closed this issue

2026-10-31 00:35:59 +00:00

bsakaguchi commented

2026-10-31 00:36:00 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 18, 2026):

Solid root-cause analysis — with isolate: false and process-global module caching, vi.mock only applies if the mocked module is the first thing loaded in the worker, which is inherently scheduler-dependent. Moving the test to tests/process-global is the right fix and matches how the project already documents that mocks belong there. Two suggestions: (1) add a sentinel to the fake signal host (e.g. a symbol property marking it as the fake) and assert the interrupt case actually ran against the fake, so the current blind spot — the "listeners went to the real process" path still passing the size-0 assertion — becomes a hard failure; (2) add a short comment in the moved test explaining why it must live in process-global (cached src/cli.ts reuse), so nobody "cleans up" by moving it back to cli-boundary and reintroducing the flake. +1 on cherry-picking e311289 to unblock main CI.

@XinwangTao commented on GitHub (Oct 18, 2026): Solid root-cause analysis — with isolate: false and process-global module caching, vi.mock only applies if the mocked module is the first thing loaded in the worker, which is inherently scheduler-dependent. Moving the test to tests/process-global is the right fix and matches how the project already documents that mocks belong there. Two suggestions: (1) add a sentinel to the fake signal host (e.g. a symbol property marking it as the fake) and assert the interrupt case actually ran against the fake, so the current blind spot — the "listeners went to the real process" path still passing the size-0 assertion — becomes a hard failure; (2) add a short comment in the moved test explaining why it must live in process-global (cached src/cli.ts reuse), so nobody "cleans up" by moving it back to cli-boundary and reintroducing the flake. +1 on cherry-picking e311289 to unblock main CI.

bsakaguchi commented

2026-10-31 00:36:01 +00:00

Author

Owner

@Adolanium commented on GitHub (Oct 18, 2026):

Thanks, both suggestions are now in github.com/Adolanium/rea@d32d79eeb, on the same branch as e3112897a.

Bypassed mock. The three interrupt cases already failed hard, because exitCode stayed undefined. control and cleanup-failure could still pass against the real process. The fake host now counts on registrations, and every case asserts at least one, so no symbol sentinel is needed. Placing a copy of the updated test back in tests/boundary/cli/ and running it with cliInputSchemaRequired.test.ts in one worker now fails all 5 cases at that assertion.
Placement. A comment above vi.mock says the file must stay in process-global because a shared cli-boundary worker may already have loaded src/cli.

On Linux (WSL Ubuntu, Node 24.18.0), the moved test paired with cliInputSchemaRequired.test.ts passes 11 of 11 tests in three runs out of three. --project cli-boundary --project process-global passes 934 tests across 140 files. The issue body now links both commits.

@Adolanium commented on GitHub (Oct 18, 2026): Thanks, both suggestions are now in https://github.com/Adolanium/rea/commit/d32d79eeb, on the same branch as e3112897a. 1. Bypassed mock. The three interrupt cases already failed hard, because `exitCode` stayed `undefined`. `control` and `cleanup-failure` could still pass against the real `process`. The fake host now counts `on` registrations, and every case asserts at least one, so no symbol sentinel is needed. Placing a copy of the updated test back in `tests/boundary/cli/` and running it with `cliInputSchemaRequired.test.ts` in one worker now fails all 5 cases at that assertion. 2. Placement. A comment above `vi.mock` says the file must stay in `process-global` because a shared `cli-boundary` worker may already have loaded `src/cli`. On Linux (WSL Ubuntu, Node 24.18.0), the moved test paired with `cliInputSchemaRequired.test.ts` passes 11 of 11 tests in three runs out of three. `--project cli-boundary --project process-global` passes 934 tests across 140 files. The issue body now links both commits.

bsakaguchi commented

2026-10-31 00:36:01 +00:00

Author

Owner

@morluto commented on GitHub (Oct 18, 2026):

Closing as completed by 8cbaf6c70 on main.

The five registered WASM finalization cases were moved from the shared CLI worker to the existing per-file isolated process-global project, preserving their outcome, exit-code, listener and cleanup assertions. This is the same cached-module/mock placement failure tracked in #4959, which is already closed.

This is source/history reconciliation using the existing validation record; no new local test or provider run is claimed.

@morluto commented on GitHub (Oct 18, 2026):  Closing as completed by [8cbaf6c70](https://github.com/morluto/rea/commit/8cbaf6c7021b97dd4de6b5771c35fbed06b240e9) on `main`. The five registered WASM finalization cases were moved from the shared CLI worker to the existing per-file isolated process-global project, preserving their outcome, exit-code, listener and cleanup assertions. This is the same cached-module/mock placement failure tracked in #4959, which is already closed. This is source/history reconciliation using the existing validation record; no new local test or provider run is claimed.

bsakaguchi commented

2026-10-31 00:36:01 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 19, 2026):

Scope looks right — keeping it to registry-state.mjs and only adding failure classification + bounded backoff around the existing-version GET preserves the unknown-state contract. One thing to watch: after retries still time out, the job must still abort before authentication rather than proceed with a guessed state, so the backoff only heals short blips and never flips unknown into a decision.

@XinwangTao commented on GitHub (Oct 19, 2026): Scope looks right — keeping it to registry-state.mjs and only adding failure classification + bounded backoff around the existing-version GET preserves the unknown-state contract. One thing to watch: after retries still time out, the job must still abort before authentication rather than proceed with a guessed state, so the backoff only heals short blips and never flips unknown into a decision.

bsakaguchi referenced this issue

2026-10-31 01:07:50 +00:00

[PR #3073] [CLOSED] fix(adb): observe daemon startup notices emitted on stderr #5592

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#3073

No description provided.

Rows
Columns