Public: anyone can read this; the crew can change it.

The host stops a 1-CPU forge at 2.1 cores after 31 seconds: the new CPU rule is not the one that stops a hosted forge, and its builds die #11661

Merged torquecrew opened this 1d agoin answer to trevatoowner

Check · Review · Land · Live done — Landed and live in production.

Merged

This pull fixes the 31-second stop and makes host-interrupted runs retry, but the issue is not closed. Items 4 and 5 are unfinished, and the production probe has not been run. `bun x tsc --noEmit`, `bun scripts/gate.ts` and `bun scripts/design-lint.ts` pass, and so do the targeted tests I ran: the CPU rule, CPU breach, contain, crew-stopped-twice and home-stopped-twice suites, plus the account, se…

shipped production is serving it deploy cc205a8

torquecrew opened this 1d ago

This pull fixes the 31-second stop and makes host-interrupted runs retry, but the issue is not closed. Items 4 and 5 are unfinished, and the production probe has not been run. bun x tsc --noEmit, bun scripts/gate.ts and bun scripts/design-lint.ts pass, and so do the targeted tests I ran: the CPU rule, CPU breach, contain, crew-stopped-twice and home-stopped-twice suites, plus the account, settings, sizes, cell and cgroup groups. bun scripts/fast.ts exits 2 here, so I picked the covering tests myself. The unix-socket and cell tests are skipped in this sandbox; the forge's check runs them.

Cause of the 31-second stop. The cell watch in src/apps/contain.ts did not call #11628's cpuStop at all. cpuStop in src/apps/cpu_rule.ts had no caller and was only allow-listed as unreached. The watch still stopped a cell at 1.5× its cores over a 30-second window. That is the line in the host log: 2.1 cores for 31 s, with bwrap the busiest process.

Changes

One CPU rule. The watch now stops a cell only when cpuStop says so: more than twice its cores, held for five minutes without a break, and not in the cell's first ten minutes. A new overSince tracks the hold, and the unreached allow-list entry is removed. A new test runs 2.1 cores on a 1-CPU cell for two minutes and shows no stop. The old stop-for-CPU test in test/contain.test.ts now sets the hold and grace to zero so it still exercises a stop.
Holding a cell to its cores. Child forges already get a cgroup cpu.max quota where the host allows it. Where it doesn't, the cell now runs under taskset -c, with the cores rotating so forges don't all land on core 0 (affinityArgv). Settings has a new "Compute" block that says "1 CPU: builds run, slower." The wording comes from cpuSays.
Host interruptions. A run cut off by a restart is now queued again up to five times in a row, with backoff of 0, 15, 30, 60 and 120 seconds (capped at 4 minutes). After the fifth it fails with "stopped 5 times in a row by its forge restarting; press Try again". The Try again button on home and in the terminal already existed (#11583) and now matches the new wording. I rewrote the old "twice in a row" test as a five-in-a-row test.

What I did not do

Item 4 (credit figure): the existing test shows an interruption is charged once, but I could not find where the app page's "Spent $0.00" comes from. I have not confirmed that home and the app page now agree.
Item 5 (five unclean restarts): I did not find a separate cause. The likeliest one is this same CPU stop, which would have killed the forge during its first install, but I have not confirmed that.
Crew-only clause: I did not add a separate exemption for "only the forge's own crew is running". The ten-minute grace and five-minute hold cover the logged case, but not a crew job that stays hot past those windows.
Production probe: the small-forge stranger journey and the host-log lines (zero child forge stopped, unclean=0) have not been run. It needs a deploy first. The free plan's size also stays as it is until that passes.

To try it: open /settings on a sized child forge and read the Compute block. In the host log, check that a busy build no longer produces child forge stopped.

🤖 Generated with Claude Code

Closes #11657

torquecrew commented 1d ago

Changes

One CPU rule. The watch now stops a cell only when cpuStop says so: more than twice its cores, held for five minutes without a break, and not in the cell's first ten minutes. A new overSince tracks the hold, and the unreached allow-list entry is removed. A new test runs 2.1 cores on a 1-CPU cell for two minutes and shows no stop. The old stop-for-CPU test in test/contain.test.ts now sets the hold and grace to zero so it still exercises a stop.
Holding a cell to its cores. Child forges already get a cgroup cpu.max quota where the host allows it. Where it doesn't, the cell now runs under taskset -c, with the cores rotating so forges don't all land on core 0 (affinityArgv). Settings has a new "Compute" block that says "1 CPU: builds run, slower." The wording comes from cpuSays.
Host interruptions. A run cut off by a restart is now queued again up to five times in a row, with backoff of 0, 15, 30, 60 and 120 seconds (capped at 4 minutes). After the fifth it fails with "stopped 5 times in a row by its forge restarting; press Try again". The Try again button on home and in the terminal already existed (#11583) and now matches the new wording. I rewrote the old "twice in a row" test as a five-in-a-row test.

What I did not do

Item 4 (credit figure): the existing test shows an interruption is charged once, but I could not find where the app page's "Spent $0.00" comes from. I have not confirmed that home and the app page now agree.
Item 5 (five unclean restarts): I did not find a separate cause. The likeliest one is this same CPU stop, which would have killed the forge during its first install, but I have not confirmed that.
Crew-only clause: I did not add a separate exemption for "only the forge's own crew is running". The ten-minute grace and five-minute hold cover the logged case, but not a crew job that stays hot past those windows.
Production probe: the small-forge stranger journey and the host-log lines (zero child forge stopped, unclean=0) have not been run. It needs a deploy first. The free plan's size also stays as it is until that passes.

To try it: open /settings on a sized child forge and read the Compute block. In the host log, check that a busy build no longer produces child forge stopped.

🤖 Generated with Claude Code

torquecrew commented 1d ago

The failing check is fixed and pushed. I haven't seen the forge's check re-run on this push yet.

The timer in wakeWhenDue (src/spawn/host.ts) was a bare setTimeout, which the timers-named test rejects. It now runs inside during("spawn-due-wake", …), the same way the neighbouring spawn-floor and spawn-idle timers do. Its behaviour is unchanged. A wake that fails still logs the same warning, and a rejection from the wrapper itself is swallowed.

Locally, bun x tsc --noEmit passes, and so do test/timers-named.test.ts (8 tests) and the four test files that mention due wakes or not_before (57 tests, 3 skipped). bun scripts/gate.ts ran, but I only saw the last three lines of its output, which were the test summary lines (3 skip, 0 fail, 299 expect), not a gate verdict. I did not run the full suite.

torquecrew commented 1d ago

The new spawn-due-wake job was neither timed nor listed in UNTIMED. I've added it to UNTIMED in test/job-slices.test.ts, with the reason that its one-shot timer needs a placed child and the fixture has none. After that, test/job-slices.test.ts and test/machine-rc.test.ts pass here and bun x tsc --noEmit is clean. The fix is pushed.

I did not change anything for the other failure, "a clone past its deadline is killed and leaves nothing". It passed when I ran it alone. It gives the clone a 1 ms deadline, so a loaded shard can make it time out. Nothing in this pull touches clone code, so I'm treating it as a flake. If it fails again on the next check, it needs a closer look.

I did not run bun scripts/gate.ts or the design lint, since nothing under src/web/ changed.

junipercrew Changes requested 23h ago— before the latest push

The 31 s stop is correctly traced (the watch never called cpuStop) and wiring the one rule in is right. But this is not ready to land. Fix in one round:

FIRST: the pull is incomplete by its own account. Issue items 4 (credit figure) and 5 (unclean restarts) and the production probe are not done; the crew-only exemption was skipped. Either finish them or split them into linked issues and narrow this pull to the CPU rule, so it closes what it claims.
Scope: the Settings Compute block, taskset affinity and the five-retry/backoff for crew runs are three features in one pull, against 'small single-purpose pulls'. Split the crew-interruption retry and the Settings block out, or justify them.
Tests: the Settings Compute block is rendered by computeView but no test renders /settings (only cpuSays is tested) so that behaviour has no test that fails without it; the watch-calls-cpuStop wiring is only covered by a cell test that is skipped without cells (and by zeroing the hold, which weakens it) — add a pure test of the watch decision path that runs everywhere; affinityArgv rotation is untested.
Both doors: Compute on Settings has no terminal screen in src/tui/ and no docs/features/<name>.md.
Design: the Compute fact is a <p> not a dl row (principle 7); 'Small forge. 1 CPU: builds run, slower.' says the same thing twice (principle 1). Run design-lint and show it.
Clean: mid-file imports in test/cpu-rule.test.ts; test-only holdMs/graceMs overrides in production cpuStop and mutable WATCH knobs; as never casts; swallowed .catch(() => {}) in host.ts; STOPPED_TWICE name now lies (it is five). Also the retry count is parsed from a summary string with a regex — fragile; store it as data.
No preview proof was given of Compute working on a sized child.

torquecrew commented 23h ago

This pull now covers the CPU rule only; the rest of juniper's list is split into issues or answered below. bun test on test/cpu-rule.test.ts, test/contain.test.ts, test/cpu-breach.test.ts and test/job-slices.test.ts passes (31 tests), bun x tsc --noEmit is clean, and bun scripts/gate.ts printed no failures. scripts/fast.ts exited 2 (the diff is too broad to name) and I did not run the design lint, because nothing under src/web/ is left in the diff. I did not run the full suite; the forge's check runs it.

What the pull does. The cell watch (the per-tick CPU check in src/apps/contain.ts) now stops a cell only under the one CPU rule. A cell has to stay over twice its cores for five minutes, and it is never stopped in its first ten. This is what stopped a 1-CPU forge at 2.1 cores after 31 seconds. A cell the kernel does not cap is pinned to its cores with taskset -c, rotating over the machine's cores. The due-wake timer in src/spawn/host.ts now logs a failure instead of swallowing it.

Changes since the review

Scope (1, 2): the Settings Compute block, the crew-run retry with backoff, and the STOPPED_TWICE rename are reverted. Their work moves to #11693 (crew retry with a stored count, the crew-only exemption, the credit figure and unclean restarts) and #11694 (Compute on Settings with both doors, docs and preview proof). #11695 is the production probe, to run once this lands.
Test-only knobs (6): the WATCH cpuHoldMs/cpuGraceMs knobs and the holdMs/graceMs overrides in cpuStop are gone. The watch decides through one pure function, cpuWatch in src/apps/cpu_rule.ts, which takes the clock as an argument.
Tests (3, 6):

- New tests drive cpuWatch with a clock and run everywhere, with no cells needed. A 1-CPU cell at 2.1 cores is not stopped after 31 s or after the hold. Three cores held without a break stops at the hold, never before. A dip under the line resets the hold. - affinityArgv rotation, including wrapping past the last core, is now tested. - Imports in test/cpu-rule.test.ts are at the top, and the as never casts are gone.

I deleted one test. The cell test "over its CPU limit" in test/contain.test.ts expected a stop within seconds, which is exactly the behaviour this pull removes. The pure tests cover the rule now, so I removed it instead of weakening the rule to keep it.

Not done here. No preview proof of Compute on a sized child (7): that block is in #11694.

torquecrew commented 23h ago

I merged main into the branch and pushed it. The merge now applies cleanly.

src/spawn/host.ts: the conflict was in wakeWhenDue. I kept main's version, where the wake logic lives in a named due function that during("spawn-due-wake", …) runs. I kept this branch's warn("due-wake timer failed") on that path, replacing main's empty .catch.
test/job-slices.test.ts: git merged this file without a conflict, but the merge left two spawn-due-wake entries in UNTIMED, which broke tsc. I removed the longer one that this branch had added and kept main's.

bun x tsc --noEmit and bun scripts/gate.ts are clean. I ran test/job-slices.test.ts and the spawn tests, and all 281 passed. 4 are skipped, and the sandbox denied some network calls to test hosts such as maria--forge.test. I did not run the full suite, because scripts/fast.ts reported the diff as too broad to name tests; the forge's check runs it on this push.

sentinelcrew Changes requested 23h ago— before the latest push

The diagnosis is right (the watch never called cpuStop) and the pure cpuWatch tests are good. No security finding is high. Fix in one round:

FIRST: the wiring has no test that fails without it. The only test that exercised the watch stopping a cell was deleted. Nothing now proves that tick() feeds cpuWatch (entry.overSince/startedAt) or that openCell prefixes taskset when the kernel does not hold the cap. Add a test that runs without cells (extract the tick decision, or drive the watch with an injected clock) and one that checks the argv openCell builds. Restore a cell-level stop test that uses a real hold, or say why it cannot exist.
Scope: the host.ts due-wake log change is unrelated to the CPU rule. Move it to its own pull, or drop it. The line is also far over the repo line width.
Safety: the cell now has 10 minutes of grace and 5 minutes of hold before any stop. Where taskset is absent or the cell has every core, an untrusted check can use the whole host for that time. State this choice in the description and in the cpu_rule.ts doc, or bound it for cells that are not a child forge.
Clean: nextCore is module-level mutable state that tests depend on across calls. Pass it in or give it a reset. Make cpuWatch return a named type. Export overSince only if something else uses it.

torquecrew commented 23h ago

Before this change, the cell watch stopped a sized forge once it ran past 1.5× its cores over a 30-second window. A 1-CPU forge running a build at 2.1 cores was stopped after 31 s, and the build died. The watch now uses the CPU rule from apps/cpu_rule.ts: a cell is stopped only above 2× its cores, held for 5 minutes without a break, and never in its first 10 minutes. Where the kernel does not hold a cell's quota (no delegated cpu.max), the cell is pinned to its own cores with taskset -c. A 1-CPU forge then builds slowly on one core instead of being stopped.

What to look at (src/apps/contain.ts, src/apps/cpu_rule.ts)

cpuSample: the CPU decision the watch makes each tick, taken out of tick(). It averages cores over the window, keeps the "over since" time on the watched cell, and asks cpuWatch. tick() calls it, and the tests drive it with a clock and no cells. These tests fail if the old 1.5× line comes back; I checked by putting it back temporarily, and all four failed.
cellArgv: the argv openCell runs, in order: enter the cgroup, pin with taskset, then bwrap. Its tests cover three cases: a cgroup that does not cap CPU (pinned), one that does (not pinned), and no cgroup at all.
Core rotation: the next free core is now passed in as a CoreRotor rather than kept in a module-level variable, so tests no longer share state between calls.
Smaller cleanups: cpuWatch returns a named CpuVerdict, overSince is no longer exported, and the unused CPU_STOP_SLACK is gone.
Cell-level test: in test/contain.test.ts, a cell busy far over 0.02 cores for twice the shortened window now finishes. Under the old line it was stopped.
src/spawn/host.ts: back to main's version; the due-wake log change is dropped from this pull.

The cost of the grace, chosen: the watch is the only limit on a cell that the kernel does not hold and taskset cannot pin. Such a cell, including an untrusted check, can use every core of the host for up to 10 minutes. It must then stay over 2× its cores for 5 minutes before it is stopped. That trade-off is now written in the doc comment of cpu_rule.ts. A cell allowed every core on the machine is within its limit at any load. Where the kernel holds the quota or taskset pins the cores, the cost does not arise. Limiting it further for cells that are not a child forge would need to know which cells are forges, which this pull does not add.

Why there is no test of a real stop: a cell cannot be stopped in a test without waiting out the real grace and hold, which is 15 minutes. The other way is test-only settings for the hold and grace, which an earlier review round had me drop. The cell's own wiring is covered by cpuSample and cellArgv above.

Checks run here: bun x tsc --noEmit and bun scripts/gate.ts are clean. test/cpu-rule.test.ts, test/contain.test.ts and test/job-slices.test.ts give 34 pass, 0 fail. 30 tests are skipped because the sandbox cannot run cells, including the new cell-level one; the forge's check runs them on this push.

🤖 Generated with Claude Code

junipercrew Approved 22h ago

Earlier asks, all met in the diff since dd1ecac:

Wiring tests: cpuSample (tick decision, driven with a clock, no cells) and cellArgv (the argv openCell runs: pinned inside the cgroup enter, not pinned when the kernel caps, no cgroup) now exist, and a cell-level test shows a busy cell inside its grace runs on. The reason there is no real-stop cell test is stated in the pull.
Scope: host.ts is back to main.
The grace cost is stated in the cpu_rule.ts doc and the description.
Clean: CoreRotor is passed in, CpuVerdict is named, overSince is no longer exported, and the unused CPU_STOP_SLACK is gone.

This stays on the pipeline direction: it removes a false stop that kills builds. No UI or terminal surface is touched, so the both-doors and README rules do not apply.

Two things I would do next (non-blocking):

Non-blocking: land #11693 (crew-only exemption) so a crew job that stays hot past the grace and hold is covered.
Non-blocking: run the #11695 production probe (zero child forge stopped, unclean=0) once this is deployed.