Skip to content

The Windows fork-storm containment test passes every time - #451

Open
avifenesh wants to merge 5 commits into
mainfrom
tower-crane/T465-the-windows-fork-storm-containment-test
Open

avifenesh wants to merge 5 commits into
mainfrom
tower-crane/T465-the-windows-fork-storm-containment-test

Conversation

@avifenesh

@avifenesh avifenesh commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

The Windows test finite nested fork storm fails the gate command and leaves another attempt running failed on the main push at 7938043 (shard 2/4) with resource_reason undefined. It now gets a memory cap the storm cannot reach, and the Windows repeat job runs it ten times.

Cause: sampler side (the storm ended between outer samples), not reporting and not a containment gap

  • The failed test took 4.4 s. That is under the gate's 10 s timeout and the storm's 15 s lifetime, so the gate did not time out, and the harness wrote nothing to stderr. The storm was stopped early by something that did not report to the outer watchdog.
  • nested-storm launches the storm as an inner contained attempt: 128 tasks, 512 MiB. On Windows its watchdog sums the working sets of the storm's tree. A probe on a hosted runner (run 39490666666, temporary job T465 probe) measured the storm's 13 Node processes at 492 to 495 MiB, about 38 MiB each. That is 17 MiB under the inner cap.
  • So the inner watchdog can stop the storm on resource-memory-max before an outer snapshot counts it above 4 tasks. By design a nested attempt's cap does not fail its parent, so the outer correctly records no reason, and the gate command fails with plain exit 1.
  • Reporting works: whenever the outer watchdog saw the storm, it recorded resource-tasks-max. That held in 40 of 40 probe rounds, detected 2.0 to 5.4 s after launch (CIM snapshot 0.3 s warm, 2.6 s cold).
  • The probe did not reproduce the miss itself: 0 of 20 rounds missed at 512 MiB. The first dispatch's 30-round loop with the old cap lost its log, because the job hit its limit. The cause is inferred from the 4.4 s duration, the empty stderr and the measured 17 MiB margin. With this change, a future miss prints the inner attempt's reason.

Change

  • test/fixtures/containment-harness.js: the nested-storm inner attempt gets a 4 GiB memory cap, about 8 times the measured storm, so only the outer task cap can stop the storm. The harness prints inner attempt: <reason> when the inner attempt hits a cap.
  • test/containment.test.js: the assertion message carries the exit status and stderr.
  • .github/workflows/ci.yml: repeat (windows-latest, node 26) runs the test ten times, right after the wakeup step, with a 15-minute step limit so a hung pass still leaves the job log. test/ci-required.test.js and docs/testing.md list the step.
  • No retries, no skip, no change in lib/.

Verification

  • Run 39490666666 (workflow_dispatch at f5a14f6, same test code as this head except a harness comment): repeat (windows-latest, node 26) passed Finite nested fork storm 10/10 (3.2 to 3.6 s each). Windows shards 2/4, 3/4 and 4/4 passed.
  • In that run, Windows shard 1/4 failed only in test/isolation.test.js:3138 (ladder args allowlist, actual: ''), which this branch does not touch. Run 39477442480 also failed shard 1/4 in test/mutants.test.js, also untouched here.
  • Locally on Linux: npm test -- test/containment.test.js test/ci-required.test.js 53/53 pass; npm run check:shared -- --base origin/main exit 0.

Limits

  • PR CI does not run the repeat job, so the Windows evidence is the dispatched run above. Main pushes and daily runs keep running the step.

Pre-submit review

  • Head: d2ebbd7
  • Fresh context: no (no subagent tool in this dispatch)
  • Acceptance: "10 in a row on Windows CI": run 39490666666, job 118862674597, Finite nested fork storm: pass 1/10 to 10/10, all passed, at f5a14f6; since then the test code changed only by a harness comment (git diff f5a14f6 d2ebbd7 -- test/containment.test.js test/fixtures/containment-harness.js). "names the cause and fixes it": Cause section above; fix at test/fixtures/containment-harness.js (nested-storm, 4 GiB inner memory cap).
  • Rework coverage: none (first revision)
  • Revuto at head: capped at its 2-round limit (nonblocking); both earlier findings (changelog wording, temporary probe job) are fixed at this head and no inline finding is open
  • CI at head: checks (shared files) and CodeQL pending; local npm run check:shared -- --base origin/main exit 0
  • Left open: none in this change; Windows shard 1/4 failures in isolation.test.js and mutants.test.js are outside this branch
  • Result: pass

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cursor

cursor Bot commented Oct 20, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: c08fb169-6d53-4fff-97ca-850303c715f9)

@revuto-review revuto-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is an auto review done by revuto.


This PR isn't ready to merge yet. Two blockers:

  1. The new T465 temporary probe test always ends in assert.fail on win32. npm test runs it on all four Windows shards for every main push, so Windows CI would fail on every run, not one in three.
  2. changelog.d/T465.md says the test "no longer fails", but nothing here fixes it. The harness change only prints the inner attempt's reason to stderr, and the assertion change only adds diagnostics.

The new 30-pass repeat step also exits 1 on any failure. Until the real fix lands, the repeats job will keep failing on main.

Comment thread test/containment.test.js Outdated
await child.containmentDone;
report.push(`inner round ${round}: ${child.resourceReason} after ${Date.now() - started} ms`);
}
assert.fail(report.join('\n'));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is an auto review done by revuto.


This probe always fails on Windows: it ends in an unconditional assert.fail(report.join('\n')), and the only skip is for non-win32. npm test runs on the four windows-latest shards for every push to main (ci.yml lines 54-64), so merging this would turn a one-in-three flake into a failure on every run. That's fine as a measurement commit on the branch, but remove the probe (or gate it behind an opt-in env var) before merge.

Comment thread changelog.d/T465.md Outdated
@@ -0,0 +1 @@
- Windows CI: the nested fork storm containment test no longer fails when its inner attempt stops the storm first.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is an auto review done by revuto.


This entry says the nested fork storm test "no longer fails when its inner attempt stops the storm first", but the PR has no fix for that. The harness change only writes inner attempt: <reason> to stderr. The outer watchdog and result.resource_reason don't change, so the assert.equal(result.resource_reason, 'resource-tasks-max', ...) at line 589 still fails whenever the inner cap fires first. Either add the fix, for example by having the outer attempt read the inner attempt's reason or by changing the assertion, or hold this entry until the fix lands.

…on Windows (temporary job)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cursor

cursor Bot commented Oct 20, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: 87bad439-4106-4adf-9810-470ccf8bf7bd)

@revuto-review revuto-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is an auto review done by revuto.


Both earlier findings are fixed:

  • The always-failing T465 temporary probe test is gone from test/containment.test.js.
  • The changelog entry now has a real fix behind it. The inner attempt's memory cap in nested-storm is raised to 4 GiB. Its tasks cap of 128 is already well above the storm's process count. The outer watchdog checks the task count before memory (lib/containment.js:640-641), so the outer tasks_max: 4 now wins.

One blocker remains. The probe moved into a new t465-probe CI job and test/fixtures/t465-probe.js. The fixture describes itself as "Temporary … removed before submit", but both would land on main. Details are inline.

Comment thread .github/workflows/ci.yml Outdated

t465-probe:
name: T465 probe (temporary)
if: github.event_name != 'pull_request'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is an auto review done by revuto.


This job is temporary. Its name says so, and test/fixtures/t465-probe.js:3 says "removed before submit". But merging this PR puts both on main.

There, if: github.event_name != 'pull_request' runs the job on every main push and on every daily run. Each run holds a windows-latest runner for up to 30 minutes and runs 40 nested-storm gate runs plus 3 CIM snapshots.

The same guard keeps it from running in this PR's CI, so it can't collect its evidence before merge either. It only runs if someone dispatches the workflow manually on this branch.

The probe never sets a failing exit code unless it throws, so leaving it in would cost runner time without anyone noticing. Get the probe's data from a manual dispatch on this branch, then remove the job and the fixture before merging. The 10-pass repeat step plus the 4 GiB inner cap are enough to show the fix.

avifenesh and others added 2 commits October 21, 2026 00:06
…changelog

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cursor

cursor Bot commented Oct 20, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: a3688e11-4ae2-4ddf-afbc-0c6398b3acc2)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cursor

cursor Bot commented Oct 20, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: 8b516e3d-7b8a-4f27-8940-745714395158)

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.

1 participant