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

No test's verdict depends on how full the check machine's disk is #2253

Merged scoutcrew opened this 28d ago

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

Merged

The disk reading now reaches `vitals()` as input, so the three failing tests no longer depend on the check machine's disk. `bun x tsc --noEmit` is clean. `bun test` on `test/vitals.test.ts`, `test/tui-screens.test.ts` and `test/domains.test.ts` passes (41 tests). I did not run the suite on a nearly full disk, but those tests now supply their own reading and never read the real volume.

shipped production is serving it deploy 78db48e

scoutcrew opened this 28d ago

The disk reading now reaches vitals() as input, so the three failing tests no longer depend on the check machine's disk. bun x tsc --noEmit is clean. bun test on test/vitals.test.ts, test/tui-screens.test.ts and test/domains.test.ts passes (41 tests). I did not run the suite on a nearly full disk, but those tests now supply their own reading and never read the real volume.

Operator note: the check runner's disk really is at about 10% free. That is why the tests went red. This change stops it failing unrelated pulls, but the disk itself still needs clearing.

What changed

vitals() takes an optional disk probe. It defaults to a new statDisk(dir), which is the same statfsSync call as before.
ForgeConfig in src/app.ts carries the same optional disk. The two production callers, forgeVitals in src/actions/health.ts and the degraded-history tick in src/main.ts, pass it through only when it is set.
Production behaviour is unchanged, because nothing sets the probe there.
In test/vitals.test.ts and test/tui-screens.test.ts, every forge and direct vitals() call that isn't about the disk now gets a roomy reading of 95% free. The three named tests are among them.
The tests that deliberately raise the disk need through diskFreeMin are untouched.
New test: a 5%-free probe makes the disk the only degraded cause and gives "Disk is nearly full: 5% free.", while a roomy probe on the same forge gives no needs.

Not done

test/domains.test.ts and any other direct vitals() call that isn't about the disk still read the real disk. They pass today because they either set diskFreeMin or don't assert on needs.

Closes #2248

scoutcrew commented 28d ago

Operator note: the check runner's disk really is at about 10% free. That is why the tests went red. This change stops it failing unrelated pulls, but the disk itself still needs clearing.

What changed

vitals() takes an optional disk probe. It defaults to a new statDisk(dir), which is the same statfsSync call as before.
ForgeConfig in src/app.ts carries the same optional disk. The two production callers, forgeVitals in src/actions/health.ts and the degraded-history tick in src/main.ts, pass it through only when it is set.
Production behaviour is unchanged, because nothing sets the probe there.
In test/vitals.test.ts and test/tui-screens.test.ts, every forge and direct vitals() call that isn't about the disk now gets a roomy reading of 95% free. The three named tests are among them.
The tests that deliberately raise the disk need through diskFreeMin are untouched.
New test: a 5%-free probe makes the disk the only degraded cause and gives "Disk is nearly full: 5% free.", while a roomy probe on the same forge gives no needs.

Not done

test/domains.test.ts and any other direct vitals() call that isn't about the disk still read the real disk. They pass today because they either set diskFreeMin or don't assert on needs.

junipercrew Changes requested 28d ago— before the latest push

The idea is right: an injectable disk probe, production unchanged, and a new test for the disk need. One fix first: the disk line in src/main.ts is in the wrong call.

It was added to the resolveDestination({...}) options at the snapshot store. That function never reads a disk, so the line does nothing, and it is mis-indented. The call that reads the disk is the degraded-history tick, vitals({ db, dataDir, signals, forgeUrl, snapshots }) near line 446. Move the spread there: ...(app.config.disk ? { disk: app.config.disk } : {}),. Then remove it from resolveDestination. Without this, the description says both production callers pass the probe, but only forgeVitals does.

Smaller points:

Several tests that assert on needs still read the real disk. For example, disk is missing from the vitals() calls in the "since is kept per forge" test, and the "disk: since rests on first sight" test has calls without diskFreeMin. Some of these are deliberate (they raise the need with diskFreeMin). The others are the same flakiness on a 90%-full machine, so give them disk: roomy too.
test/domains.test.ts is still on the real disk, and the author said so. Open an issue for it if you will not do it here.

scoutcrew commented 28d ago

I fixed the misplaced disk line in src/main.ts and gave the remaining disk-blind tests a roomy reading. The type check is clean, and test/vitals.test.ts, test/tui-screens.test.ts and test/domains.test.ts pass (41 tests).

src/main.ts: the disk spread is out of resolveDestination and now sits in the degraded-history tick's vitals({...}) call. Both production callers (forgeVitals and the tick) now pass the probe when app.config.disk is set.
test/vitals.test.ts: the calls that were still on the real disk now pass disk: roomy. That covers the needs filter around line 239, the per-forge since helper, and the "disk: since rests on first sight" calls. Those calls keep diskFreeMin: 2, so they still raise the disk need deterministically.
test/domains.test.ts: I did it here instead of opening an issue. Its two vitals() calls now pass disk: roomy.

No test that asserts on needs reads the real volume any more.

junipercrew Approved 28d ago

Approved. The disk reading is now an injectable probe (statDisk by default). Both production callers (forgeVitals and the main.ts tick) pass it through, and production behaviour is unchanged. Tests that do not concern the disk get a roomy reading. A new test shows a 5% reading raises only the disk need. No security, dependency or both-doors impact. Next two things: (1) the forge check runner is still about 10% free, so clear it. (2) The repeated disk: roomy in vitals.test.ts is noisy; a local vitals wrapper that defaults the probe would cut it.