ci: sample host state from inside the file where the wedge happens (B8d of #110) - #136
Conversation
…8d of #110) B5's checkpoint marks file boundaries, which is what named #76's wedge — every known leg dies in `provisioning.test.ts` — and is structurally unable to describe it, because the freeze is 3.5-7.6 min inside that file. Free disk, free memory, load and qemuCount have never been read anywhere in that window; B8a's "111 GB free, qemuCount 0" is a boundary reading from a leg that never reached position 10. The disk and memory hypotheses are open because nothing has looked. `scripts/ci-leg-heartbeat.ts` samples the host on an interval and posts a trailing window to a check run, attributing each sample to the file the leg is inside and how deep into it the reading is. It owns a SEPARATE check run rather than extending B5's. Two read-modify-write writers of one payload would race, and the worst case is a stale PATCH dropping the newest file record — the durable evidence B5 exists to preserve, lost exactly when the runner dies. Its external_id carries a fifth segment, and parseExternalId takes exactly four, so the ledger skips heartbeats with no special case and can never build a runner-lost from one. That is asserted. Opt-in via `heartbeat-interval`, and `plan` errors above 3 legs: GITHUB_TOKEN allows 1000 requests/hour/repository, shared with the checkpoint and the aggregate. 30 s is 120 writes/hour per leg — fine for the 1-3 leg dispatches that chase #76, ~1800/hour across a sweep, which would throttle the durable record to feed a diagnostic. Host snapshot gains macOS memory pressure and swap: os.freemem() cannot separate purgeable cache from swapping to death, and both report a small number. #77's checkpoint list said "free memory / memory pressure" and only the first half was ever built. It also gains free disk on the quickchr data-dir volume, which is not the workspace volume on Windows. Refs #110, #76.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe PR adds optional in-file CI heartbeat sampling. The sampler records host metrics and checkpoint progress, publishes bounded updates to a dedicated GitHub check run, and uploads local samples and logs. The workflow validates intervals and limits enabled runs to three legs. CI heartbeat diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IntegrationStep
participant ciLegHeartbeat
participant CheckpointState
participant ciHostSnapshot
participant GitHubCheckRun
IntegrationStep->>ciLegHeartbeat: Start with interval and deadline
ciLegHeartbeat->>CheckpointState: Read file progress
ciLegHeartbeat->>ciHostSnapshot: Collect host metrics
ciLegHeartbeat->>GitHubCheckRun: Publish bounded sample payload
IntegrationStep->>ciLegHeartbeat: Signal stop on exit
ciLegHeartbeat->>GitHubCheckRun: Complete heartbeat check run neutrally
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Not ready to approve
It contains a confirmed Windows free-disk parsing bug (empty output becomes 0 MiB) plus a stray “XX” doc-comment artifact, both of which should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adds an opt-in “in-file heartbeat” instrument for the integration workflow to periodically sample host state (disk/memory/load/QEMU process count) from inside the currently running test file and publish a trailing window via a dedicated GitHub check run, specifically to help diagnose the macos-x86 runner-loss wedge in provisioning.test.ts (#76 / B8d of #110).
Changes:
- Add
scripts/ci-leg-heartbeat.tsto post periodic host snapshots to a dedicatedhb:check run, attributing each sample to the current test file + in-file depth. - Extend
scripts/ci-host-snapshot.tswithfreeDataDiskMiBand macOS-onlymemFreePct/swapUsedMiB, plus support for measuring disk free space for an arbitrary path. - Wire the instrument into
integration.ymlbehind a newheartbeat-intervalexperiment lever (validated and capped to ≤3 legs) and document it in CI instructions; add unit tests.
File summaries
| File | Description |
|---|---|
test/unit/ci-leg-heartbeat.test.ts |
New unit tests for heartbeat external_id behavior, payload round-trip, trimming/byte budget, and checkpoint-based file attribution. |
scripts/ci-leg-heartbeat.ts |
New heartbeat sampler that posts to a dedicated check run and maintains a bounded trailing window payload. |
scripts/ci-host-snapshot.ts |
Adds data-dir disk free measurement + macOS memory-pressure/swap readings; generalizes disk-free probing to an arbitrary path. |
.github/workflows/integration.yml |
Adds heartbeat-interval input, validates/caps it in plan, and runs the heartbeat as a background process during the per-file integration loop. |
.github/instructions/ci.instructions.md |
Documents the new heartbeat lever and its operational constraints. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
A cold leg does not create ~/.local/share/quickchr until its first machine, and `df` on a path that does not exist fails — so the first samples of a leg carried no `freeDataDiskMiB` at all. Observed live on run 30858745297. A hole there reads like a failed measurement rather than "same volume, nothing written yet", which is the opposite of what a disk-exhaustion hypothesis wants from its baseline. Refs #110, #76.
1. Windows free-disk could report a fabricated 0. A failed PowerShell expression
leaves stdout empty, and Number("") is 0, which Number.isFinite accepts — so
an unreadable volume reported "0 MiB free" instead of "not measured". That is
a manufactured disk-exhaustion signal on the one platform where the workspace
and quickchr volumes genuinely differ, feeding the exact hypothesis this
snapshot exists to test. Absent now stays absent.
2. The sample file is appended to, not rewritten. Re-encoding the whole array
each tick is O(n^2) over a leg, so the sampler's own cost would grow with
elapsed time and perturb the late-in-the-leg window it exists to measure —
accumulation it caused itself. It also truncates any stale file at startup,
since appending now makes a leftover prefix another leg's samples.
3. Repaired a mangled sentence in the header (a bad in-place edit left an 'XX').
Refs #110, #76.
3/3 wedged again — position 10 now 7/7. Disk exhaustion and memory exhaustion are refuted from inside the fatal window rather than inferred from boundaries: ~108 GB free, kernel memory pressure flat at 89%, zero swap, one QEMU process, and the leg dies during a VM's life. Also retires three assumptions. '~16-20 min' is a range, not a deadline — no constant on any of three clocks, so there is no hidden runner timer to hunt. Suite growth is why legs reach position 10, not why they die there: windows-x86 and macos-arm64 both run LONGER on the same commit and finish. And a load precursor visible on one leg does not replicate on the other two, which is worth writing down because opening one wedged leg makes it look like the answer. Refs #110, #76.
|
The dispatch this instrument was built for has run, and it worked. Evidence: #76 (comment) Three full-sequence What it found: disk exhaustion and memory exhaustion are both refuted, measured inside the fatal window instead of inferred from file boundaries. ~108 GB free on the quickchr data volume,
Findings recorded in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/integration.yml:
- Around line 351-355: Update the HEARTBEAT validation in the workflow’s
numeric-check block to reject positive values below 10, matching the stated
message and the 10-second minimum used by ci-leg-heartbeat. Preserve acceptance
of whole-number intervals from 10 upward and the existing invalid-input error
path.
In `@scripts/ci-host-snapshot.ts`:
- Around line 15-16: Replace Node compatibility APIs with Bun or web-standard
equivalents across the affected sites: in scripts/ci-host-snapshot.ts lines
15-16, update existsSync and dirname usage; in scripts/ci-leg-heartbeat.ts lines
53-54, replace existsSync, readFileSync, and rmSync while retaining
appendFileSync; and in scripts/ci-leg-heartbeat.ts line 139 plus
test/unit/ci-leg-heartbeat.test.ts lines 148-157, measure UTF-8 byte length with
Blob.prototype.size or TextEncoder instead of Buffer.byteLength.
In `@scripts/ci-leg-heartbeat.ts`:
- Around line 279-285: Update readCheckpoint to validate the parsed JSON before
returning it: require startedAt to be a string, planned to be an array of
strings, and records to be an array; return undefined for any schema mismatch
while preserving the existing parse-error behavior.
- Around line 144-146: Update the title construction around the samples count to
use singular “sample” when samples.length is one and plural “samples” otherwise,
while preserving the existing no-samples text and file/elapsed details. Update
the corresponding assertion to expect the singular wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 24b27098-2c84-417f-abbe-f5d2ac2e0ec1
📒 Files selected for processing (5)
.github/instructions/ci.instructions.md.github/workflows/integration.ymlscripts/ci-host-snapshot.tsscripts/ci-leg-heartbeat.tstest/unit/ci-leg-heartbeat.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Unit Tests (windows-latest)
- GitHub Check: Unit Tests & Coverage
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In Bun-based TypeScript code, use
Bun.spawn(),Bun.write(),Bun.sleep(),bun:test, and ESM imports with.tsextensions.
Files:
test/unit/ci-leg-heartbeat.test.tsscripts/ci-leg-heartbeat.tsscripts/ci-host-snapshot.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not turn a red integration test green by broadening timeouts, skipping it, or platform-gating it before reproducing and root-causing the failure.
Files:
test/unit/ci-leg-heartbeat.test.ts
**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.ts: Use Bun APIs and tooling rather than Node.js equivalents:Bun.spawn(),Bun.write(),Bun.sleep(),bun test, andbun:test. Use ESM with.tsextensions in imports; do not use CommonJS.
For ARM64virtmachines, never useif=virtiofor drives; use an explicit-device virtio-blk-pci,drive=drive0configuration.
When using HVF acceleration, use-cpu host, notcortex-a710.
For arm64 guests on macOS, automatically select TCG with-cpu cortex-a710; HVF cannot run the CHR image's 32-bit ARM userspace on Apple Silicon.--accelandQUICKCHR_ACCELmust override this selection for testing.
UEFI pflash code and vars units must be identical in size.
QGA is x86-only; do not assume the guest agent starts for arm64 CHR.
Use tabs for indentation.
Do not add unnecessary comments to obvious code.
Errors must be thrown asQuickCHRError(code, message, installHint?).
Preserve the documented public API types and behavior:QuickCHR.start(opts)returnsChrInstance;ChrInstanceprovidesstop(),remove(),rest(),monitor(),serial(), andqga(); andMachineStaterepresents persistedmachine.jsonstate.
Files:
test/unit/ci-leg-heartbeat.test.tsscripts/ci-leg-heartbeat.tsscripts/ci-host-snapshot.ts
test/unit/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Unit tests must be fast and must not require QEMU.
Files:
test/unit/ci-leg-heartbeat.test.ts
🧠 Learnings (4)
📚 Learning: 2026-07-31T11:56:40.455Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 117
File: scripts/ci-cache-key.ts:68-68
Timestamp: 2026-07-31T11:56:40.455Z
Learning: In Bun TypeScript files, do not flag use of `node:fs.appendFileSync` when append semantics are required and `Bun.write()` cannot provide them. This applies to cases such as appending multiple entries to `$GITHUB_OUTPUT` or writing to the boot log; use append-capable file operations rather than overwriting existing content.
Applied to files:
test/unit/ci-leg-heartbeat.test.tsscripts/ci-leg-heartbeat.tsscripts/ci-host-snapshot.ts
📚 Learning: 2026-07-31T11:56:38.870Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 117
File: scripts/ci-cache-key.ts:110-110
Timestamp: 2026-07-31T11:56:38.870Z
Learning: In Bun CI scripts under scripts/, use a plain Error for validation failures when the script catches the error and renders it as a GitHub ::error:: annotation. QuickCHRError is intended for programmatic library callers and is not required for this CLI-only error path.
Applied to files:
scripts/ci-leg-heartbeat.tsscripts/ci-host-snapshot.ts
📚 Learning: 2026-07-28T00:12:59.340Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 101
File: .github/workflows/integration.yml:342-342
Timestamp: 2026-07-28T00:12:59.340Z
Learning: For tikoci/quickchr, review workflow changes to ensure GitHub Actions are pinned to immutable commit SHAs as a repository-wide policy. If a PR introduces SHA pinning for only part of a workflow (or only some workflows) without extending the same policy across the relevant workflow files, flag it. When adding/changing SHA pinning, also verify Dependabot is configured to update the `github-actions` ecosystem so action version bumps are managed consistently (e.g., in the repo’s Dependabot configuration), rather than doing an isolated pinning change in an unrelated PR.
Applied to files:
.github/workflows/integration.yml
📚 Learning: 2026-07-31T11:56:27.561Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 117
File: .github/workflows/integration.yml:188-188
Timestamp: 2026-07-31T11:56:27.561Z
Learning: For the tikoci/quickchr repository, do not request isolated SHA pinning of first-party GitHub Actions in a single workflow or pull request. Treat action pinning as repository-wide work: first configure Dependabot for the github-actions ecosystem, then mechanically pin all workflow action references, and finally decide whether to enforce zizmor in CI. Apply this guidance to workflow files while issue `#118` tracks the rollout.
Applied to files:
.github/workflows/integration.yml
🪛 OpenGrep (1.26.0)
scripts/ci-host-snapshot.ts
[ERROR] 133-133: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 143-143: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
1. A parseable-but-partial checkpoint is now rejected rather than cast. That file is written by ANOTHER process while the sampler reads it, so a valid-JSON object missing `records` or `planned` is reachable — and locate() reads .length on both, so it would throw and take the sampler down at the one moment it is the only instrument still reporting. locate()'s own doc promised a torn read costs a sample's attribution and never the sample; parseCheckpoint() is what makes that true, and it is pure so the guard is tested directly. 2. heartbeat-interval now rejects 1-9 instead of silently running them at 10. The error message already claimed a 10s minimum and the script floors there, so a dispatch could be labelled with an interval it never used — the same flavour of quiet lie the other levers are validated against. 3. '1 samples' -> '1 sample'. Declined: replacing node:fs sync calls and Buffer.byteLength with Bun/web APIs. node:fs sync imports are the established pattern in all eight scripts/*.ts, including ci-leg-checkpoint.ts which this file pairs with and reads from — converting one file would break the consistency that makes the pair legible. Buffer.byteLength also does not allocate, where TextEncoder would copy the whole ~60 KB payload on every tick of a sampler whose cost must not grow. Refs #110, #76.
|
All four review findings addressed — three fixed, one declined with a reason. Fixed
Copilot's three from the earlier round were fixed in Declined: replacing
Full unit suite green (982 pass / 0 fail), biome / tsc / cspell / markdownlint clean, all 41 workflow |
…ver has A one-factor contrast: three macos-x86 legs identical to the B8d dispatch except accel=tcg. One completed 12/12 all-pass in 1939 s — the first recorded full-suite completion on this platform — with provisioning.test.ts passing AT POSITION 10 in 604 s, and a second leg passing the same file at the same position in 810 s. HVF is 0/7 through that position; TCG is 2/2. The speed confound does not apply, which is what makes the contrast usable: TCG per-file cost is within noise of HVF here (nine files, 910 s vs 769/883/937 s), because this suite is not CPU-bound — it is I/O and RouterOS startup waits, so emulation costs ~7%. The elapsed-load condition was reproduced and the same ten in-file VM boots performed; only the accelerator differed. Triangulated against B7, which bounds the claim to something narrower than 'HVF is broken': the maintainer's Intel Mac runs the same suite under HVF to completion in 1818 s. It is HVF ON THE HOSTED RUNNER. Records the rule that follows: do not pin accel=tcg on macos-x86 to green the platform. That deletes the only reproduction of a real host-level defect — the move this file already forbids for quarantining provisioning.test.ts, arriving through a different door — and costs the project its only HVF coverage. Refs #110, #76.
B8d of #110. Refs #110, #76. Not linked to close either — this lands an instrument, and per
operating rule 7 an instrument is not a diagnosis. The dispatch that consumes it is running against
this branch; its result goes to #76 as evidence.
The gap this closes
B5's checkpoint marks file boundaries. That is what named #76's wedge — all four known legs die in
provisioning.test.ts— and it is structurally unable to describe it: B8b's live log capture (the onlycopy, since runner loss destroys the blob) puts the freeze 3.5–7.6 min inside that file, at its
5th/6th test. So free disk, free memory, load and
qemuCounthave never been read anywhere in thewindow the leg actually dies in. B8a's reassuring "111 GB free,
qemuCount0" is a boundary readingfrom a leg that never reached position 10. The maintainer's disk-space hypothesis, and the memory one,
are open because nothing has looked — not because anything cleared them.
scripts/ci-leg-heartbeat.tssamples the host on an interval and posts a trailing window to a checkrun, attributing every sample to the file the leg is inside and how deep into it the reading is —
which is what makes a sample comparable to the 3.5–7.6 min figure at all.
Three decisions worth reviewing
It owns a second check run instead of extending B5's. The obvious implementation adds rows to the
existing checkpoint. It is wrong:
markand a heartbeat would both be read-modify-write writers of onepayload, and the interleaving — heartbeat reads state,
markwrites a record and PATCHes, heartbeatPATCHes its stale copy — drops the newest file record from the server-side output. The window is a
second wide and self-heals on the next tick, except in the one case this program exists for: the
runner dying inside it. Not worth taking to save a check run.
The
external_idcarries a fifth segment (…/hb) andparseExternalIdtakes exactly four, soci-leg-ledgerskips heartbeats with no special case and can never fabricate arunner-lostfrom one.test/unit/ci-leg-heartbeat.test.tsasserts that across both modules rather than leaving it to thereader.
It is opt-in, and
planerrors above 3 legs.GITHUB_TOKENis 1000 requests/hour/repository,shared with the checkpoint and the aggregate. At 30 s a leg posts 120 writes/hour — fine for the 1–3 leg
dispatches that chase #76 (every known wedge is bounded to ~16–20 min, so those are cheap), and
~1800/hour across a 15-leg sweep, which would throttle the durable record to feed a diagnostic.
That is a refusal rather than a warning because a throttled checkpoint is invisible until someone needs it.
Two
HostSnapshotfields that were specified and never built. #77's per-checkpoint list said "freememory / memory pressure" and only the first half exists. On macOS the first half barely means
anything alone:
os.freemem()counts only genuinely free pages, so a host with gigabytes of purgeablecache and a host swapping to death report the same small number. macOS legs now carry
memFreePct(
memory_pressure -Q) andswapUsedMiB(vm.swapusage) — measured working on the maintainer's IntelMac, where
freemem()said 2758 MiB while the kernel reported 86% free, which is the whole point.freeDataDiskMiBis likewise separate fromfreeDiskMiBbecause on Windows the workspace is onD:and quickchr's state is on
C:, and it is the quickchr volume that fills with images and machine disks.Safety
ci-leg-checkpoint.ts: every API failure is a::warning::and exit 0, and theworkflow ignores the process's status. An instrument that reds the leg it measures manufactures the
masked signal this program exists to stop.
attributable to the instrument rather than misread as the host going quiet.
trap … EXITstops it however the file loop ends(clean exit, red file, or the
breakafter an unclean reap). Without that trap it would keep samplingthe idle host through the forensic steps and overwrite the last rows before a wedge with reassuring ones.
output.textis bounded against the checks API's 65535-char limit by rendered bytes, not just bysample count — a rejected PATCH here would not fail loudly, it would silently stop updating and leave
the wedge unsampled. There is a test that adding a
HostSnapshotfield cannot break that bound.Verification
biome,tsc,cspell,markdownlint, shell and example lints all clean.its index, in-file depth, and every new host reading.
run:step inintegration.ymlre-checked withbash -nafter the edit (41 steps).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests