lab(ci): local HVF full-suite resource trend — cost is flat (B7 of #110) - #115
Conversation
#76 left two candidate explanations for the macos-x86 runner loss: per-file cost inflating as the suite progresses, or flat cost with the time going somewhere else. B7 distinguishes them on the one machine that can run the CI-matching HVF baseline locally. Adds test/lab/full-suite-resource-trend/ — a sampler plus a runner that reproduces the CI per-file loop (one `bun test` process per file, sequentially, same env knobs), because the shape is part of what is being measured. Running the suite as a single process would rule out per-process growth by construction and answer a question CI is not asking. Result: 12/12 pass in 30.4 min, and cost is flat. Against windows-x86 — the slowest CI platform that completes — the VM-only ratio has a slope of +0.002x per file across 10 files. No memory accumulates either: free+inactive moves -194 MB over 30 min, wired+compressed -84 MB, swap falls, and memory pressure never leaves level 1. The one apparent outlier was a cold download, not leakage. provisioning.test.ts ran 1.98x windows-x86 at position 10 of 12 — exactly where a leakage story would want it — but 619 s of its 992 s elapsed with zero QEMU processes, downloading the pinned 7.20.7/7.20.8 images with two retries each. Excluding that leaves 373 s of VM work, 0.74x, in line with every other file. That single file is a locally measured demonstration of the #104/B3 confound and direct support for B3 landing before B11 collects timing samples. This rules out a deterministic local Intel/HVF full-suite defect. It does not rule out a hosted-runner limit, and the laptop's 8 cores / 64 GiB do not characterize the runner's — the pressure result must not be carried across. The ~30 minutes unaccounted for on CI remain unaccounted for; that is B8. Refs #76, #110. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdded a macOS lab that runs the full integration suite, samples host resources, analyzes per-file and suite-wide trends, and documents a B7 HVF report. Added sampler-related terms to the project dictionary. ChangesFull-suite resource trend lab
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant run.sh
participant sample.sh
participant IntegrationTests
participant analyze.ts
run.sh->>sample.sh: Start resource sampling
run.sh->>IntegrationTests: Run integration files sequentially
sample.sh-->>run.sh: Write NDJSON resource samples
run.sh->>sample.sh: Stop sampling after the suite
analyze.ts->>analyze.ts: Parse timings and samples
analyze.ts-->>run.sh: Emit resource and timing reports
Possibly related issues
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 |
…ement The CHR Image Caching section explained cache key mechanics but not that a miss is large enough to dominate a per-file timing comparison. B7 (#110) measured it: provisioning.test.ts ran 1.98x its windows-x86 CI cost purely because two pinned images were uncached, and 62% of the file elapsed with no QEMU running at all. At position 10 of 12 that is exactly where a cumulative- leakage story would want its evidence, so the discriminator — was any qemu-system process alive? — is worth stating where the caching rules live. Refs #76, #104, #110. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new lab under test/lab/ to reproduce CI’s per-file integration-suite execution shape locally (Intel macOS/HVF) while sampling host resource metrics, producing an evidence report for CI issue #76 / umbrella #110.
Changes:
- Add a new
full-suite-resource-trend/lab with a per-file runner, NDJSON sampler, analyzer script, and written report. - Document the new lab in
test/lab/README.md. - Extend the cSpell project dictionary with lab/metrics vocabulary.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
test/lab/README.md |
Adds the new lab entry and explains the exception to the usual *.test.ts lab shape. |
test/lab/full-suite-resource-trend/sample.sh |
macOS-only resource sampler emitting NDJSON records keyed by the currently-running file marker. |
test/lab/full-suite-resource-trend/run.sh |
CI-shape per-file integration runner (one bun test per file), wiring the sampler and output artifacts. |
test/lab/full-suite-resource-trend/analyze.ts |
Reads timing + samples and produces trend tables (position-vs-cost and monotonic resource climb). |
test/lab/full-suite-resource-trend/README.md |
Usage and interpretation documentation for running and reading the lab output. |
test/lab/full-suite-resource-trend/REPORT.md |
Captures the measured results and caveats for the referenced run. |
project-words.txt |
Adds cSpell dictionary words used by the new lab/report. |
Two review findings on #115. project-words.txt is lowercase throughout — APFS was the only uppercase entry in 479 words, and cspell is case-insensitive here, so the entry works lowercase and no longer risks a case-variant duplicate. sample.sh interpolated the marker's contents into a JSON string unescaped. The marker only ever holds a test filename or a "(...)" sentinel today, so nothing was broken, but analyze.ts drops unparseable lines silently — an invalid sample would be indistinguishable from a missing one, which is precisely the failure mode this lab exists to avoid. Now escapes backslash and double-quote and strips control characters; verified round-tripping a marker containing both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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 `@project-words.txt`:
- Line 24: Replace the uppercase APFS entry with lowercase apfs in the project
words dictionary, preserving its existing sorted position and the file’s
lowercase acronym convention.
In `@test/lab/full-suite-resource-trend/README.md`:
- Around line 26-34: Update the `current-file` artifact description in the
README to include the `(after <file>)` marker between each completed file and
the next file, preserving the existing `(startup)` and `(done)` states and
clarifying that this intermediate value may appear in `samples.ndjson`.
In `@test/lab/full-suite-resource-trend/sample.sh`:
- Around line 34-39: Replace the eval-based assignment in the vm_stat parsing
block with direct variable assignment: have awk emit the four calculated values
as space-separated fields, then read them into VM_FREE, VM_INACTIVE, VM_WIRED,
and VM_COMPRESSED using read. Preserve the existing page-size conversion and
variable ordering without evaluating generated shell code.
In `@test/lab/README.md`:
- Line 21: Update the documentation link in the full-suite-resource-trend row of
test/lab/README.md to reference the existing
.github/instructions/ci.instructions.md document instead of the incorrect
relative ci.instructions.md path.
🪄 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: 5541282a-47c3-4285-a89f-ddc0076291f8
📒 Files selected for processing (7)
project-words.txttest/lab/README.mdtest/lab/full-suite-resource-trend/README.mdtest/lab/full-suite-resource-trend/REPORT.mdtest/lab/full-suite-resource-trend/analyze.tstest/lab/full-suite-resource-trend/run.shtest/lab/full-suite-resource-trend/sample.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Unit Tests (windows-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/lab/full-suite-resource-trend/analyze.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/lab/full-suite-resource-trend/analyze.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/lab/full-suite-resource-trend/analyze.ts
🪛 ast-grep (0.45.0)
test/lab/full-suite-resource-trend/sample.sh
[error] 33-38: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "$(vm_stat | awk -v p="$PAGE" '
/Pages free/ {gsub(/./,"",$3); printf "VM_FREE=%d\n", $3p/1048576}
/Pages inactive/ {gsub(/./,"",$3); printf "VM_INACTIVE=%d\n", $3p/1048576}
/Pages wired down/ {gsub(/./,"",$4); printf "VM_WIRED=%d\n", $4p/1048576}
/occupied by compressor/ {gsub(/./,"",$5); printf "VM_COMPRESSED=%d\n",$5p/1048576}
')"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🔇 Additional comments (7)
test/lab/full-suite-resource-trend/run.sh (1)
1-87: LGTM!test/lab/full-suite-resource-trend/sample.sh (1)
1-33: LGTM!Also applies to: 40-58
test/lab/full-suite-resource-trend/analyze.ts (1)
1-347: LGTM!test/lab/README.md (1)
22-31: LGTM!test/lab/full-suite-resource-trend/README.md (1)
1-25: LGTM!Also applies to: 35-77
test/lab/full-suite-resource-trend/REPORT.md (1)
1-137: LGTM!project-words.txt (1)
210-210: LGTM!Also applies to: 232-233, 249-249, 293-293, 309-313, 401-401
Two more review findings on #115. sample.sh built shell assignments in awk and fed them to eval. vm_stat's output is not attacker-controlled so this was not exploitable, but static analysis flags it as CWE-78 and the pattern becomes an injection point the moment it is copied onto a source that is. awk now emits four plain numbers consumed by `read`. Verified against an independent vm_stat call: inactive and compressed match exactly, free and wired differ under 1% from being sampled a moment apart. The lab README listed the marker sequence as (startup) -> file -> (done), but run.sh also parks it at "(after <file>)" between files — the fix that closes the "QEMU at exit" blind spot. A reader grouping samples.ndjson by `file` needs to expect that value, so both the marker row and the samples row now name it. Declined: the SKILL Reference column in test/lab/README.md was flagged for naming `ci.instructions.md` without its .github/instructions/ path. That column holds bare code spans, not links, and every existing row uses a bare filename — including qemu.instructions.md, which lives in that same directory. Changing only the new row would make the table inconsistent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Addressed in
Declined — |
B7 of #110. Evidence-producing bite for #76: does per-file cost inflate as the suite progresses (cumulative leakage), or stay flat?
Answer: flat. Full report in
test/lab/full-suite-resource-trend/REPORT.md.Result
12/12 files pass, 1818 s test time, 30.4 min wall, Intel Mac / x86_64 guest under HVF (CI-matching accel), QEMU 11.0.3, SHA
21ac47f, targetstable.Ratios against
windows-x86— the slowest CI platform that actually completes its job (run30507484030) — over the 10 files carrying timing signal:No resource accumulates either, across 343 samples at 5 s:
Memory pressure stayed at level 1 (normal) for every sample. Peak QEMU concurrency never exceeded 1, and the settled post-suite sample shows 0
qemu-systemprocesses — no file leaked QEMU into its successor.The outlier was a cold download, not leakage
provisioning.test.tsran 1.98× windows-x86 at position 10 of 12 — exactly where a cumulative-leakage story would want its evidence. It isn't. 619 s of its 992 s elapsed with zero QEMU processes running: it was downloading the pinned 7.20.7/7.20.8 images, each needing two retries. Excluding that leaves 373 s of VM work — 0.74×, in line with every other file — and it accounts for essentially the entire raw slope.That makes this a locally measured demonstration of the #104/B3 confound, and direct support for B3 landing before B11 collects timing samples: with cold download uncontrolled, B11 would carry variance of this size and attribute it to whatever it varied on purpose.
What this does not show
Deliberately narrow, per B7's own instructions:
macos-15-intelrunner is not, and that gap is uncharacterized here. The "no memory pressure" result must not be carried across to the runner.Contents
run.sh— reproduces the CI per-file loop (onebun testprocess per file, sequentially, same env knobs). Deliberately notbun test test/integration/: running the suite as one process would rule out per-process growth by construction and answer a question CI is not asking.sample.sh— 5 s NDJSON resource sampler, attributing every sample to the running file via a marker file.analyze.ts— folds timing + samples into the two questions; embeds the CI reference with thegit show origin/ci-data:…command that produced it.REPORT.md— findings, and the measurement caveats (APFS purgeable reclamation makesdisk_freeunusable in this run; per-file "QEMU at exit" was blind and orphan detection uses peak concurrency instead).Verification
bun run checkgreen (biome, tsc, markdownlint, cspell, examples, shellcheck).bun test test/unit/841 pass / 0 fail. No CI legs consumed — this bite is local by design.Refs #76, #110.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation