morluto/rea

Fork 0

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

[PR #4577] [CLOSED] test(process): give the cancellation poll the running-line window #6318

New issue

Closed

opened 2026-10-31 01:13:32 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:13:32 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4577
Author: @Kenmege
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/process-capture-cancel-poll-window

📝 Commits (1)

4b44731 test(process): give the cancellation poll the running-line window

📊 Changes

1 file changed (+5 additions, -1 deletions)

View changed files

📝 tests/boundary/process/processCaptureProgress.test.ts (+5 -1)

📄 Description

Summary

tests/boundary/process/processCaptureProgress.test.ts > "reports timeout and cancellation cleanup through MCP progress" fails intermittently on a loaded host with Matcher did not succeed in time on toContain("disposition=cancelled"). The test's 1 s default poll window is shorter than the time the product is allowed to take for that line. This aligns the poll with the 4 s window that the running-line poll in the same case already uses. Test-only change; the assertion is unchanged.

Problem and expected behavior

After cancellation, the terminal progress line (which carries disposition=) is sent only after owned-process cleanup verification. src/process/capture/ProcessCaptureLifecycle.ts bounds that verification and pending delivery with PROCESS_CLEANUP_VERIFICATION_GRACE_MS = 1_000. docs/process-capture.md says the terminal line is always emitted, and states no latency bound for it.

The test polls for the line with expect.poll's default 1 s window. That window is no longer than the product's own grace, so on a host under load the poll can expire before a line the product is still allowed to send. The earlier poll in the same case (running:) already uses { timeout: 4_000 }.

Change and scope

One line in the test: the disposition=cancelled poll gets { timeout: 4_000 }, with a comment giving the reason. No product code changes and no assertion changes. The following owned_process_group=cleaned assertion and the output checks are untouched.

Contribution scope: an ordinary test fix within existing boundaries; a maintainer scope decision is not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: the process-boundary test only.
Contribution scope fit and linked maintainer decision: not applicable (test-only fix).
CLI and MCP/tool-catalog contract: none.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details: unchanged. Cleanup is still asserted as cleaned.
Generated metadata, package, or installation impact: none.

Evidence and regression coverage

Test updated: the one poll above.
Reproduction: macOS 27 arm64, 10 cores, Node 24.18.0 from .nvmrc. npx vitest run --project process-boundary tests/boundary/process/processCaptureProgress.test.ts -t "timeout and cancellation cleanup", repeated with 10 CPU-bound yes processes running alongside, with the 1-minute load average recorded for each run.
- At 5eaef92e5 (base): 10 of 10 runs failed with Matcher did not succeed in time on disposition=cancelled.
- At this branch: 10 of 10 passed, on the same host under the same added load.
The same change, measured more widely on the same host, bucketed by 1-minute load:

Load ≤ 60 Load > 60

Before 5 of 16 runs failed; the lowest failing load was about 9 15 of 34 failed

After 0 of 21 failed 3 of 59 failed

The three failures after the change came only under heavy artificial overload (load 77–171 on 10 cores). One was the existing 4 s running: poll, one was this poll, and one was the owned_process_group=cleaned assertion. That last one did not recur in 30 later runs at higher load.
Every one of those 30 later runs listed surviving test children (ps) after the test, and found none.
Remaining proof gaps: not run on Linux or Windows locally. The change only widens a test wait, and CI covers both.

	Load ≤ 60	Load > 60
Before	5 of 16 runs failed; the lowest failing load was about 9	15 of 34 failed
After	0 of 21 failed	3 of 59 failed

Validation performed

npm run check:fast: passed (2/2 tasks).
npm run check: passed (6/6 tasks; architecture guards passed across 1459 source files).
npm run verify:test-discovery: discovered all 1439 test files exactly once.
The stress loop above at 5eaef92e5 and at this branch.

None found; this was seen as an intermittent CI failure on a loaded macOS host.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: macOS host only, as above.
Package or release metadata impact: none (tests only).
Security, privacy, process, or containment review: not applicable; no product code changes.

Author readiness

Human owner: @Kenmege (the declarations below are left for the human owner, per the template)

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.

🤖 Generated with Claude Code

_{🔄 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/4577 **Author:** [@Kenmege](https://github.com/Kenmege) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/process-capture-cancel-poll-window` --- ### 📝 Commits (1) - [`4b44731`](https://github.com/morluto/rea/commit/4b447310e103128f265274ab9476a4e93aefb544) test(process): give the cancellation poll the running-line window ### 📊 Changes **1 file changed** (+5 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `tests/boundary/process/processCaptureProgress.test.ts` (+5 -1) </details> ### 📄 Description ## Summary `tests/boundary/process/processCaptureProgress.test.ts` > "reports timeout and cancellation cleanup through MCP progress" fails intermittently on a loaded host with `Matcher did not succeed in time` on `toContain("disposition=cancelled")`. The test's 1 s default poll window is shorter than the time the product is allowed to take for that line. This aligns the poll with the 4 s window that the running-line poll in the same case already uses. Test-only change; the assertion is unchanged. ## Problem and expected behavior After cancellation, the terminal progress line (which carries `disposition=`) is sent only after owned-process cleanup verification. `src/process/capture/ProcessCaptureLifecycle.ts` bounds that verification and pending delivery with `PROCESS_CLEANUP_VERIFICATION_GRACE_MS = 1_000`. `docs/process-capture.md` says the terminal line is always emitted, and states no latency bound for it. The test polls for the line with `expect.poll`'s default 1 s window. That window is no longer than the product's own grace, so on a host under load the poll can expire before a line the product is still allowed to send. The earlier poll in the same case (`running:`) already uses `{ timeout: 4_000 }`. ## Change and scope One line in the test: the `disposition=cancelled` poll gets `{ timeout: 4_000 }`, with a comment giving the reason. No product code changes and no assertion changes. The following `owned_process_group=cleaned` assertion and the output checks are untouched. Contribution scope: an ordinary test fix within existing boundaries; a maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: the process-boundary test only. - Contribution scope fit and linked maintainer decision: not applicable (test-only fix). - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: unchanged. Cleanup is still asserted as `cleaned`. - Generated metadata, package, or installation impact: none. ## Evidence and regression coverage - Test updated: the one poll above. - Reproduction: macOS 27 arm64, 10 cores, Node 24.18.0 from `.nvmrc`. `npx vitest run --project process-boundary tests/boundary/process/processCaptureProgress.test.ts -t "timeout and cancellation cleanup"`, repeated with 10 CPU-bound `yes` processes running alongside, with the 1-minute load average recorded for each run. - At 5eaef92e5 (base): 10 of 10 runs failed with `Matcher did not succeed in time` on `disposition=cancelled`. - At this branch: 10 of 10 passed, on the same host under the same added load. - The same change, measured more widely on the same host, bucketed by 1-minute load: | | Load ≤ 60 | Load > 60 | |---|---|---| | Before | 5 of 16 runs failed; the lowest failing load was about 9 | 15 of 34 failed | | After | 0 of 21 failed | 3 of 59 failed | The three failures after the change came only under heavy artificial overload (load 77–171 on 10 cores). One was the existing 4 s `running:` poll, one was this poll, and one was the `owned_process_group=cleaned` assertion. That last one did not recur in 30 later runs at higher load. - Every one of those 30 later runs listed surviving test children (`ps`) after the test, and found none. - Remaining proof gaps: not run on Linux or Windows locally. The change only widens a test wait, and CI covers both. ## Validation performed - `npm run check:fast`: passed (2/2 tasks). - `npm run check`: passed (6/6 tasks; architecture guards passed across 1459 source files). - `npm run verify:test-discovery`: discovered all 1439 test files exactly once. - The stress loop above at 5eaef92e5 and at this branch. ## Related issue None found; this was seen as an intermittent CI failure on a loaded macOS host. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: macOS host only, as above. - Package or release metadata impact: none (tests only). - Security, privacy, process, or containment review: not applicable; no product code changes. ## Author readiness Human owner: @Kenmege (the declarations below are left for the human owner, per the template) - [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. - [ ] 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:13:32 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:46:42 +00:00

[PR #6318] [CLOSED] fix(deps): pin vite to 8.3.5 so npm audit stops reporting the launch-editor advisories #7279

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

No description provided.

Rows
Columns