ci: bound each integration test file, and name the one that wedged (B4 of #110) - #122
Merged
Conversation
…ged (B4 of #110) `integration.yml` runs test files sequentially under one step cap set 10 min under the job budget (#108). On the extended-budget platforms that cap is 290 minutes, so a single hung file can burn a whole leg and the failure arrives as "the step ran out of time" with no name attached. This puts the bound at file granularity, where the answer is. Each file now runs under `scripts/ci-file-watchdog.ts`: - a deterministic, checked-in cap per file; - a second bound from what remains of the step budget, holding a 300 s reserve back so the reap/metrics/summary/upload steps always still run; - on expiry: SIGTERM → SIGKILL the test process, reap QEMU, count again to verify, write `~/watchdog-<file>.json` and a job-summary block; - exit codes that separate "red, keep going" from "stop, the environment may be poisoned". Caps come from a NAMED ci-data window — runs 30657533896 and 30665449265, both at `2899be4`, 14 legs and 168 test-file records — and are `clamp(observed × 2, 600 s, 1200 s)`. Never derived at runtime: that would let a hang inflate its own next deadline (#110 rule 4). The window is post-#116 on purpose, or durations carry download-retry inflation rather than cost. `macos-x86` is absent from that window and cannot be added while #76 stands, so the caps are deliberately generous rather than tight — the tightest is 1.83× its worst observed run. A watchdog that killed a healthy file on the one platform under investigation would manufacture exactly the masked signal this program exists to remove. Outcome vocabulary (#77 item 4) covers only what the watchdog observes first-hand: pass, test-failure, file-watchdog-timeout, not-run. `runner-lost` and `attempted-incomplete` need the server-visible ledger and stay with B5. Records carry both the new `outcome` and the original binary `status`, so tested-versions.json and historical runs/*.ndjson stay comparable. Adds a `watchdog-cap` experiment lever so the timeout path can be exercised on a real runner without committing a test that hangs on purpose. It can only SHORTEN a cap, validated in `plan`, with a warning when set. This does not make a lost runner diagnosable — #76's legs die at ~62 min holding a 290-minute step budget and nothing in-job survives that, this script included. Refs #77, #110. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthroughThe PR adds a Bun watchdog for per-file integration tests. It derives bounded deadlines, cleans up timed-out processes, records structured outcomes, and integrates reports and exit codes into the CI workflow. ChangesIntegration watchdog
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
This was referenced Aug 1, 2026
There was a problem hiding this comment.
Pull request overview
Adds a per-file watchdog to the integration workflow so a single wedged integration test file is bounded, attributed by filename, and still leaves post-failure evidence (timing, host snapshot, QEMU reap verification) instead of consuming an entire step budget.
Changes:
- Introduces
scripts/ci-file-watchdog.tsand wires it into.github/workflows/integration.ymlto enforce deterministic per-file caps plus a step-deadline clamp, with awatchdog-capexperiment lever. - Extends CI timing parsing/metrics (
scripts/ci-metrics.ts) to record both binarystatusand a widenedoutcomevocabulary, preserving historical comparability. - Adds unit coverage for watchdog cap derivation/deadline math/serialization and updates CI documentation describing the watchdog behavior and outcomes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
test/unit/ci-metrics.test.ts |
Updates timing parsing tests for <file> <n>s <outcome> and legacy normalization. |
test/unit/ci-file-watchdog.test.ts |
Adds anchor unit tests for cap selection, step-deadline clamp, and timing-line serialization. |
scripts/ci-metrics.ts |
Extends timing parsing to include outcome while keeping status binary; warns on unknown outcomes. |
scripts/ci-file-watchdog.ts |
New watchdog runner: enforces per-file caps, kills on expiry, reaps QEMU, writes watchdog reports, and emits timing lines. |
project-words.txt |
Updates cSpell dictionary (but header/comment block and casing need cleanup). |
.github/workflows/integration.yml |
Runs each integration file via the watchdog; validates watchdog-cap; uploads watchdog reports and surfaces them in the step summary. |
.github/instructions/ci.instructions.md |
Documents the per-file watchdog design, caps window, outcomes, artifacts, and experiment lever. |
Suppressed comments (1)
project-words.txt:181
project-words.txtis kept sorted and lowercase (cSpell matches case-insensitively), butIMAGENAMEwas added in uppercase. Use the lowercase form so the dictionary stays consistent and easy to scan.
IMAGENAME
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/instructions/ci.instructions.md:
- Line 113: Update the “Per-file watchdog” heading to level four so it remains
nested under the “Integration (integration.yml)” section, and update the
re-derive jq snippet to print both .status and .outcome, preserving the existing
refresh behavior while exposing detailed outcomes for filtering.
In @.github/workflows/integration.yml:
- Around line 925-937: Update the integration summary’s success condition near
the existing all-tests-passed message to also require that no file watchdog
timeout is recorded in $HOME/integration-timing.txt. Reuse the existing
$watchdog_fired state for the watchdog report block so a watchdog kill cannot
produce a success message before its timeout details.
In `@project-words.txt`:
- Around line 3-14: Reorder and repair the introductory comments in
project-words.txt so they form a coherent explanation: begin with the project
dictionary purpose, describe the vocabulary and exclusions, explain sorting and
cSpell/editor usage, then mention the relevant commands and configuration
references. Remove the unmatched parenthesis and restore the intended complete
sentences without changing the dictionary content.
In `@scripts/ci-file-watchdog.ts`:
- Around line 207-225: Update the Windows command in qemuProcessCount to include
both qemu-system-x86_64.exe and qemu-system-aarch64.exe, or use a wildcard
image-name filter compatible with tasklist. Preserve the existing qemu-system
substring filtering and Unix behavior so surviving aarch64 processes are
counted.
- Line 61: Replace the report-file usage of writeFileSync with Bun.write(),
while retaining appendFileSync for the timing file’s append behavior; update the
import accordingly and adjust the report-writing logic in the watchdog flow to
await Bun.write() if needed.
In `@scripts/ci-metrics.ts`:
- Around line 64-69: Replace the object-based OUTCOME_ALIASES lookup with a Map
so tokens such as “constructor” cannot resolve through Object.prototype. Update
the alias access in the timing-line parsing flow around the outcome
normalization logic to use Map lookup while preserving the existing
fail-to-test-failure normalization.
In `@test/unit/ci-file-watchdog.test.ts`:
- Around line 25-50: Add a unit test in the “capSecondsFor — the checked-in cap
table” suite covering the optional overrideS parameter: verify an override below
the calculated cap shortens the result, while an override above it does not
lengthen the cap. Use the existing capSecondsFor test patterns and constants.
🪄 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: dee08b3f-ca63-4ad7-b819-c6564cb2acba
📒 Files selected for processing (7)
.github/instructions/ci.instructions.md.github/workflows/integration.ymlproject-words.txtscripts/ci-file-watchdog.tsscripts/ci-metrics.tstest/unit/ci-file-watchdog.test.tstest/unit/ci-metrics.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Unit Tests (windows-latest)
- GitHub Check: Unit Tests & Coverage
- GitHub Check: Analyze (javascript-typescript)
- 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-file-watchdog.test.tstest/unit/ci-metrics.test.tsscripts/ci-metrics.tsscripts/ci-file-watchdog.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-file-watchdog.test.tstest/unit/ci-metrics.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-file-watchdog.test.tstest/unit/ci-metrics.test.tsscripts/ci-metrics.tsscripts/ci-file-watchdog.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-file-watchdog.test.tstest/unit/ci-metrics.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-file-watchdog.test.tstest/unit/ci-metrics.test.tsscripts/ci-metrics.tsscripts/ci-file-watchdog.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
📚 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-metrics.tsscripts/ci-file-watchdog.ts
🔇 Additional comments (10)
project-words.txt (1)
181-181: LGTM!scripts/ci-file-watchdog.ts (2)
143-196: LGTM!
310-327: 🩺 Stability & AvailabilityNo change needed.
await proc.exitedreturns a non-zero numeric code when an external signal terminates the subprocess, so the non-timeoutcode === 0branch is safe.test/unit/ci-file-watchdog.test.ts (1)
79-135: LGTM!Also applies to: 137-167
.github/workflows/integration.yml (3)
132-135: LGTM!Also applies to: 175-178, 236-236, 324-335
775-809: 🩺 Stability & Availability | ⚡ Quick winConfirm
rccapture underpipefailfor every watchdog exit code.
bun scripts/ci-file-watchdog.ts … | tee -a … || rc=$?relies onpipefailto surface the watchdog code instead oftee's. Ifteealso fails,rcbecomestee's status and codes 3 and 4 lose their "stop the loop" meaning. ConsiderPIPESTATUS[0]for an exact read:♻️ Proposed change
- rc=0 - bun scripts/ci-file-watchdog.ts \ - --file "$f" \ - --timing "$HOME/integration-timing.txt" \ - --report-dir "$HOME" \ - --deadline "$deadline" 2>&1 | tee -a "$HOME/integration-output.txt" || rc=$? + rc=0 + set +e + bun scripts/ci-file-watchdog.ts \ + --file "$f" \ + --timing "$HOME/integration-timing.txt" \ + --report-dir "$HOME" \ + --deadline "$deadline" 2>&1 | tee -a "$HOME/integration-output.txt" + rc=${PIPESTATUS[0]} + set -e
968-968: LGTM!Also applies to: 988-988
scripts/ci-metrics.ts (1)
12-12: LGTM!Also applies to: 71-86, 88-99
test/unit/ci-metrics.test.ts (1)
14-43: LGTM!.github/instructions/ci.instructions.md (1)
121-135: LGTM!Also applies to: 142-174, 293-293
Seven findings, all valid. Two were more than their grade suggests, and one was a regression this PR introduced. **The summary claimed success above a watchdog kill (Major).** A watchdog kill leaves NO `(fail)` line in integration-output.txt — bun is killed mid-file, so the log simply stops. Verified against run 30681180342's own artifact: zero `(fail)` lines, so that run's summary printed "✅ all selected integration tests passed" directly above the "⏱ File watchdog fired" block. Exactly the false green this program exists to remove, shipped by the change that adds the block. The success line is now gated on the timing file too. **Windows cleanup could report a false clean bill of health.** `reapQemu` kills both qemu-system-x86_64.exe and qemu-system-aarch64.exe, but qemuProcessCount filtered on the x86_64 image alone, so a surviving aarch64 emulator would still produce `cleanup_verified: true` — and that flag decides whether the file loop keeps going. Dropped the image filter and let the existing substring match cover both; it also covers tasklist's "no tasks" line without a special case. **project-words.txt header scrambled — my regression.** The helper that added IMAGENAME re-sorted every non-empty line, comments included, shredding the dictionary's header into fragments. Restored to its original bytes; dropping the image-name filter above means the word was never needed. Also: Bun.write() for the report file (house rule — appendFileSync stays where append semantics are required); OUTCOME_ALIASES is a Map, because the token regex accepts `constructor` and an object literal resolves that through Object.prototype to a function; the docs heading nests under Integration (integration.yml) at h4; and the re-derive jq snippet prints `.outcome` with a note to take only `pass` rows — folding a killed file back in would let the caps ratchet upward off their own timeouts. Two regression tests added: the watchdog-cap lever shortens but never lengthens, and an outcome token cannot resolve through Object.prototype. Refs #77, #110. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
Review round — 7 findings, all valid, all fixed (
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 905-910: Update the result-handling logic around the
watchdog_fired check and the steps.run outcome so the existing watchdog-specific
branch remains first, but the success message is emitted only when
steps.run.outcome is success and the timing data indicates no failure. Add a
generic failure branch for startup/import failures, watchdog process errors, or
any non-pass timing result before the success else path.
In `@test/unit/ci-file-watchdog.test.ts`:
- Around line 69-82: Update capSecondsFor and its invalid-value test cases so
positive fractional overrides that floor below one second, such as 0.5, are
rejected and fall back to the table cap. Validate the floored override is at
least one before accepting it, while preserving existing behavior for valid
overrides and other invalid values.
🪄 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: 0b88a65c-6347-4640-a5a9-f25a7ee1000c
📒 Files selected for processing (6)
.github/instructions/ci.instructions.md.github/workflows/integration.ymlscripts/ci-file-watchdog.tsscripts/ci-metrics.tstest/unit/ci-file-watchdog.test.tstest/unit/ci-metrics.test.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Integration / 3_Plan matrix (platforms × targets).txt: Integration
Conclusion: failure
##[group]Run set -eo pipefail
�[36;1mset -eo pipefail�[0m
�[36;1m# id → label | runner | accel | budget�[0m
�[36;1m#�[0m
�[36;1m# `accel` is the accelerator this hosted runner is EXPECTED to give�[0m
�[36;1m# detectAccel(). It is a claim about the runner, checked at runtime:�[0m
�[36;1m# the Log platform step compares it against the detected value and�[0m
�[36;1m# emits a ::warning:: on drift. It is reporting only — it never�[0m
�[36;1m# changes what the tests do.�[0m
�[36;1m#�[0m
�[36;1m# `budget` is the empirical run-time class, kept separate on purpose:�[0m
�[36;1m# "slow leg" is measured, not derived from the accelerator. It drives�[0m
�[36;1m# the job/step timeouts and tcg-smoke eligibility.�[0m
�[36;1m#�[0m
�[36;1m# accel grounding:�[0m
�[36;1m# linux-x86/linux-arm64 kvm — /dev/kvm on hosted runners; either�[0m
�[36;1m# may fall back to tcg when the runner�[0m
�[36;1m# ships without a writable /dev/kvm.�[0m
�[36;1m# That fallback is reported as a�[0m
�[36;1m# ::notice::, not drift — it is a�[0m
�[36;1m# runner condition, not a stale table.�[0m
�[36;1m# macos-arm64 tcg — hosted macos-15 is itself a VM�[0m
�[36;1m# ("Apple M1 (Virtual)"), kern.hv_support=0�[0m
�[36;1m# (run 29669706663), and quickchr forces�[0m
�[36;1m# TCG on Apple Silicon anyway (`#97`).�[0m
�[36;1m# macos-x86 hvf — macos-15-intel is bare metal: detectAccel�[0m
�[36;1m# returns hvf (timeoutFactor 1.5), boots�[0m
�[36;1m# take 30–46 s at cpu-load 0–1 %. This row�[0m
�[36;1m# read `tcg` until `#76`; the summary line�[0m
�[36;1m# printed that hardcoded claim ...
GitHub Actions: Integration / 1_Integration (linux_x86_64 · stable).txt: Integration
Conclusion: failure
##[group]Run set -eo pipefail
�[36;1mset -eo pipefail�[0m
�[36;1mFILTER="${TEST_FILTER// /}"�[0m
�[36;1mif [ -z "$FILTER" ] && [ "$SMOKE" = "true" ]; then�[0m
�[36;1m # tcg-smoke was requested: run the curated subset that exercises�[0m
�[36;1m # boot + REST end-to-end in minutes instead of hours.�[0m
�[36;1m FILTER="anchor.test.ts"�[0m
�[36;1m echo "::notice::TCG smoke subset: $FILTER (tcg-smoke input; omit it for the full suite)"�[0m
�[36;1mfi�[0m
�[36;1mif [ -z "$FILTER" ]; then�[0m
�[36;1m ALL=""�[0m
�[36;1m for f in test/integration/*.test.ts; do ALL="$ALL $f"; done�[0m
�[36;1m echo "list=$ALL" >> "$GITHUB_OUTPUT"�[0m
�[36;1m # scope=full is what lets a run mark a RouterOS version "tested" in�[0m
�[36;1m # ci-data/tested-versions.json — filtered/smoke runs never do.�[0m
�[36;1m echo "scope=full" >> "$GITHUB_OUTPUT"�[0m
�[36;1m exit 0�[0m
�[36;1mfi�[0m
�[36;1mecho "scope=filtered" >> "$GITHUB_OUTPUT"�[0m
�[36;1mFILES=""�[0m
�[36;1mIFS=',' read -ra PARTS <<< "$FILTER"�[0m
�[36;1mfor f in "${PARTS[@]}"; do�[0m
�[36;1m [ -z "$f" ] && continue�[0m
�[36;1m if [ -f "test/integration/$f" ]; then�[0m
�[36;1m FILES="$FILES test/integration/$f"�[0m
�[36;1m else�[0m
�[36;1m echo "::warning::Skipping unknown test file: $f"�[0m
�[36;1m fi�[0m
�[36;1mdone�[0m
�[36;1mif [ -z "${FILES// /}" ]; then�[0m
�[36;1m echo "::error::test-filter '$FILTER' matched no known files in test/integration/"�[0m
🧰 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-metrics.test.tsscripts/ci-metrics.tstest/unit/ci-file-watchdog.test.tsscripts/ci-file-watchdog.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-metrics.test.tstest/unit/ci-file-watchdog.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-metrics.test.tsscripts/ci-metrics.tstest/unit/ci-file-watchdog.test.tsscripts/ci-file-watchdog.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-metrics.test.tstest/unit/ci-file-watchdog.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-metrics.test.tsscripts/ci-metrics.tstest/unit/ci-file-watchdog.test.tsscripts/ci-file-watchdog.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-metrics.tsscripts/ci-file-watchdog.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
🔇 Additional comments (5)
scripts/ci-file-watchdog.ts (1)
40-45: LGTM!Also applies to: 65-65, 212-219, 360-360
.github/workflows/integration.yml (1)
936-936: LGTM!Also applies to: 976-976, 996-996
scripts/ci-metrics.ts (1)
68-70: LGTM!Also applies to: 89-101
test/unit/ci-metrics.test.ts (1)
38-45: LGTM!.github/instructions/ci.instructions.md (1)
113-113: LGTM!Also applies to: 139-144, 297-297
**The success line could still print after the step failed (Major).** The previous round gated it on the watchdog outcomes only, which leaves two other ways to fail without a `(fail)` line: a startup/import error, where bun exits non-zero having printed no test result at all, and the watchdog script itself dying, which leaves no timing row to inspect. The first is visible in the timing file, the second only in the step outcome — so the success line is now gated on both, with a generic failure branch after the watchdog-specific one so timeout details stay specific. Verified across all six shapes this step can end in, each rendered through the real summary block: all-green, ordinary test failure, watchdog kill, budget exhausted, startup/import error, watchdog script died. Only the first prints ✅; the two that motivated this finding now say the step failed. **A fractional lever value produced a 0-second cap (Minor).** `capSecondsFor` accepted any `overrideS > 0` and then floored it, so `--cap 0.5` yielded a cap of 0 — a deadline already expired when the file starts, which would report a wedge that never happened. Now requires `>= 1`, with `0.5` and `0.999` added to the invalid-value cases and `1` pinned as the smallest honored value. Refs #77, #110. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Second round closed ( All 11 review threads resolved; Final end-to-end verify on the merge candidate — run 30748983353, forced 45 s cap on |
This was referenced Aug 2, 2026
mobileskyfi
added a commit
that referenced
this pull request
Aug 2, 2026
) **B5 of #110** — part 2 of #77, the half B4 explicitly could not do. B4 bounded a *file* that hangs; this makes a *lost runner* diagnosable. A leg whose runner stops talking left **nothing** behind: no artifact, no metrics record, no row anywhere. The 2026-07-31 sweep planned 15 integration legs and `ci-data` received 12 ndjson files, with no marker for the missing three. A reader comparing platforms saw silence and had to guess whether `macos-x86` had never run or had died trying. ## Both open design decisions got a maintainer call first Recorded on [#110](#110 (comment)) before any code was written, per the bite's instruction not to pick silently. **Decision 1 — a separate ledger, not `tested-versions.json`.** The version scheduler reads that file as a *presence* test (`ros-versions.yml:99`), so writing `incomplete` into `conclusion` would make an aborted run look tested and **silently stop rescheduling that version, forever, with no error**. Today's `macos-x86` case would not trip it (the scheduler only reads `linux-x86`), but the contract is one platform away from breaking. So: a new `ci-data/attempted-legs.json`, and that jq is untouched. `foldTestedVersions`'s `scope:"full"` rule is unchanged. **Decision 2 — a per-leg check run.** Rejected alternatives, one of them on evidence gathered before claiming. ## What survives losing a runner — measured, not assumed The test loop already prints `::notice::Running <file>` per file, so scraping the job log would have cost nothing. **It does not survive.** On run [30665449265](https://github.com/tikoci/quickchr/actions/runs/30665449265): ```text job 91271309625 macos/x86_64 · stable → BlobNotFound job 91271309655 macos/x86_64 · testing → BlobNotFound job 91271309687 macos/x86_64 · long-term → BlobNotFound job 91271309635 linux-x86 (green) → 868 lines job 91271309686 windows-x86 (RED, normal) → 790 lines ``` The contrast with the *normally failing* windows leg is the load-bearing half: **failure preserves the log; runner loss destroys it.** The jobs API step ledger, by contrast, *does* survive — a vanished leg still records its test step as `in_progress` with a start time. That is free and now used, but it stops at step granularity and can never name the file. This table is recorded in `DESIGN.md` so the next instrument does not re-derive it. ## The design | | | |---|---| | `scripts/ci-leg-checkpoint.ts` | Opens a check run per leg, PATCHes it after every file — last completed file, what is running now, outcome/duration/cap, and a resource sample (free memory, load, free disk, live QEMU count). A leg that dies leaves it un-closed; **the absence of a terminal is the signal.** | | `scripts/ci-leg-ledger.ts` | Runs in `aggregate`: compares `plan`'s matrix against the legs that reached `ci-data/runs/`, writes `attempted-legs.json`, annotates the run summary, and closes check runs the runner never got to. | | `scripts/ci-host-snapshot.ts` | The host readings, extracted from B4's watchdog into **one home** — same rule `COLD_DOWNLOAD_FLOOR_BYTES_PER_S` got in B13. These numbers get compared *across* the two instruments, so a second copy free to drift would make the comparison noise. | **The artifact outranks both instruments.** A leg is `complete` because it produced a metrics record — never because a check run says so. The checkpoint is best-effort (warns and exits 0 on any API failure, so a fork PR's read-only token or a dropped PATCH cannot red a leg), which means trusting it first would let *instrument failure fabricate a `runner-lost` for a leg that finished cleanly*. There is an anchor test for exactly that. Verdicts: `complete`, `runner-lost`, `attempted-incomplete`, `not-started`. That last one is **not** in #77's vocabulary and is deliberate — without it a leg that died in `Install QEMU` is indistinguishable from one whose runner vanished mid-suite, and mislabelling a setup failure as `runner-lost` sends #76 chasing a mechanism that was never involved. ## Verification **1. Replay against the real sweep (committed as an anchor test).** The classifier, fed the actual job records from run 30665449265 — 15 planned, 12 complete, the three `macos-x86` legs named `runner-lost`, all 15 jobs matched by name. Fixture trimmed to the fields classification reads. This is the only test driven by a genuine runner-loss event rather than one written to match its own expectations. **2. Happy path on a real runner** — [run 30750868899](https://github.com/tikoci/quickchr/actions/runs/30750868899), green. Check run posted, marked and closed; ledger wrote `1/1 legs complete` and committed to `ci-data` with no `incomplete` map. **3. An intentionally killed runner** — the bite's stated exit criterion, which cannot be verified locally. [Run 30750979859](https://github.com/tikoci/quickchr/actions/runs/30750979859), dispatched from a temporary commit that `sudo halt -f -p`s the runner after the first file. **That commit was dropped and the branch force-pushed before this PR was opened** — it is not in the diff, and no such lever ships. The reproduction matched #76 exactly: step 17 stuck `in_progress`, steps 18+ never started, `if: always()` never fired, no artifact, and the job log blob returns `BlobNotFound` — a second, self-controlled confirmation of the log finding above. What the ledger recorded anyway: ```json "linux-x86|stable": { "terminal": "runner-lost", "last_file": "disk.test.ts", "current_file": "exec.test.ts", "last_checkpoint_ts": "2026-08-02T13:57:25.584Z", "files_reported": 1, "files_planned": 2, "stalled_step": "Run integration tests (sequential per-file)", "job_matched": true, "job_elapsed_s": 2760 } ``` and the aggregate closed the orphaned check run as `runner-lost — last file disk.test.ts`. ### The gap between the wedge and the verdict is ~45 minutes, and the checkpoint closes it This run measured something #110 had listed as uncharacterized. The runner was halted at **13:57:26**; GitHub declared the job failed at **14:42:03**. The service waited **~44.6 minutes** before deciding the runner was gone. `last_checkpoint_ts` is 13:57:25 — **within two seconds of the actual wedge**, and 44.6 minutes earlier than `completed_at`. That is the whole value of the instrument: `job_elapsed_s: 2760` is an upper bound that overstates the wedge by ~16x here, and the checkpoint replaces it with a timestamp. **Do not carry the 44.6 min figure across platforms or loss modes** (#110 rule 6). It is one measurement, on `ubuntu-latest`, of a *clean halt* — a silent network partition or a wedged-but-alive runner may well get a different verdict latency. What it does establish is that the interval is **tens of minutes, not seconds**, so reading `completed_at` as the wedge time is wrong by a lot. Whether #76's `macos-x86` legs go quiet far earlier than their 62-65 min `completed_at` is now a *measurable* question rather than an unanswerable one — B8a is the bite that should answer it, and it now has an instrument that can. ## Notes for review - **Permissions cross a reusable-workflow boundary.** The integration job needs `checks: write`; aggregate needs `checks: write` + `actions: read`. Reusable-workflow permissions are capped by the calling job, so `main.yml` and `sweep.yml` grant both. A caller that forgets them gets warnings and an empty ledger, not a red run. - **One silent-degradation risk, deliberately fenced.** The jobs API exposes no matrix values, so joining a job to a planned leg goes through the display-name string. `integrationJobName()` is its one home and is asserted against production job names by a test; if `integration.yml`'s `name:` is edited without it, every entry degrades to `job_matched: false`. - **`hostUptimeS` replaces B4's `uptimeS`**, which reported `process.uptime()` — the watchdog process's own life, duplicating its `elapsed_s` and reading ~0 in every checkpoint. Renamed rather than reused so nothing reads a checkpoint's number as if it meant the watchdog's. - **No CHANGELOG entry** — CI-internal, nothing user-facing, same as B4 (#122) and B3 (#117). - **`ci-data` README updated** — done, [`54a2192`](54a2192) on the data branch. `attempted-legs.json` has been written by the aggregate job since run 30750868899 but was undocumented; the README now carries the four terminals, the artifact-outranks-the-instrument rule, why `last_checkpoint_ts` rather than `job_elapsed_s` is the timestamp to read, and a query recipe verified against the real file. ## This PR is deliberately linked to #77 — check that before merging Per the `#112` → `#79` hazard, a PR should link its sub-issue **only** when it satisfies that issue's whole done-when. This one does, so the link is intentional rather than accidental: | #77 done-when | Bite | |---|---| | A hung file fails before the runner-loss boundary with name, cap, host snapshot, cleanup result | B4 (#122) ✅ | | Timeout cleanup verified; CI does not continue through a poisoned environment | B4 (#122) ✅ | | Runner disappearance leaves a server-visible `runner-lost` and last completed/current file | **this PR** ✅ | | Per-file timeout outcomes flow into the rollup, **and expected incomplete legs are visible** | B4 (first half) + **this PR** (second half) ✅ | | Caps derived from cited post-`2899be4` data, checked in, documented | B4 (#122) ✅ | **The one judgement call is resolved: the link stays, and the residue is filed as #125.** #77's *Proposed approach* §4 also lists `operation-timeout`, `infra-download` / `infra-cache` and `cancelled` / `superseded`, which are **not** implemented — they need output classification neither bite does, and `ci-file-watchdog.ts:113` says so rather than leaving it silent. They are not in the done-when, and #110's rule is that a PR links its sub-issue when it satisfies that issue's **whole done-when** — which this does, all five rows above. So closing #77 here is correct, and #125 exists so the unbuilt half does not evaporate with it. Closes #77. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added CI progress checkpoints for individual test files, with persistent status and timing information. * Added recovery tracking for interrupted or incomplete integration runs. * Added per-run reporting that distinguishes completed, interrupted, attempted-incomplete, and not-started test legs. * CI now captures available host and accelerator details during execution. * **Documentation** * Added guidance for investigating lost-runner and incomplete-run scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
B4 of #110 — the per-file watchdog half of #77. The ledger (B5) is deliberately not here.
integration.ymlruns test files sequentially under one step cap set 10 min under the job budget (#108). On the extended-budget platforms that cap is 290 minutes, so one hung file can burn a whole leg and the failure arrives as "the step ran out of time" with no name attached. This puts the bound at file granularity, where the answer is.Two bounds, because timeouts nest
A file is bounded by its own cap and by what remains of the step budget, whichever is smaller (#110 rule 5):
so the reap / metrics / summary / upload steps always still run. When the remainder cannot fit a viable attempt the file is recorded
not-runrather than launched into a cap that guarantees a meaningless timeout.The caps, and why they are loose on purpose
Checked in, deterministic, never fetched at runtime — a runtime lookup is how a hang inflates its own next deadline (#110 rule 4).
OBSERVED_MAX_Sholds the worst healthy duration per file over a named window: runs 30657533896 and 30665449265, both at2899be4— 14 legs, 168test-filerecords, one non-pass (theresolveVersionflake now filed as #121). Cap isclamp(observed × 2, 600 s, 1200 s).The window is post-
2899be4per the retired-provisionality note on #77: before #116 a flat 120 s download deadline aborted healthy transfers and re-downloaded from zero, so durations carried retry inflation rather than cost (B7 measuredprovisioning.test.tsat 992 s of which 619 s had no QEMU alive at all). It now reads 654 s.Two deliberate choices that keep this from becoming a masking device:
macos-x86is absent from the window and cannot be added while CI: macos-x86 full suite wedges at position 10 in provisioning.test.ts — host-level runner loss, mechanism unexplained (HVF) #76 stands — it has never completed a full suite. So the caps are generous rather than tight; the tightest isprovisioning.test.tsat 1.83× its worst observed run. Killing a healthy file on the one platform under investigation would manufacture exactly the signal this program exists to remove.macos-x86has no cache writer while CI: macos-x86 full suite wedges at position 10 in provisioning.test.ts — host-level runner loss, mechanism unexplained (HVF) #76 stands (CI: per-run cache rotation writes ~5 GB per full dispatch and exceeds the 10 GB quota #104/B3), so any of its files may pay a cold download the window never measured — up to 465 s of legitimate transfer budget (Download deadlines are flat totals, so a healthy large transfer fails as a timeout instead of classifying #116). A 2× cap on a 51-second file would kill that healthy download outright.The suite-deadline clamp, not a tight per-file cap, is what bounds the total.
Evidence on expiry
SIGTERM → SIGKILL the test process, reap QEMU, count again to verify, then write
~/watchdog-<file>.jsoninto the artifact and the job summary. Exit codes drive the loop:0pass,1test failure,2timed out but reaped clean (keep going, leg red),3timed out with QEMU surviving,4budget spent. 3 and 4 stop the loop — continuing through a possibly poisoned environment turns one root cause into a string of misleading failures (#77 §2).Killing
bunalone does not stop QEMU; it is spawned detached. Measured, not assumed — see below.Outcome vocabulary
Only what the watchdog observes first-hand:
pass,test-failure,file-watchdog-timeout,not-run.runner-lostandattempted-incompleteare not implemented — they need the server-visible ledger, which is B5.infra-download/infra-cache/operation-timeoutwould need output classification this script deliberately does not do; it leaves stdout/stderr inherited so the workflow'steekeeps producing byte-identical logs.Records carry both the new
outcomeand the original binarystatus. That is not redundancy:tested-versions.jsonand every storedruns/*.ndjsonare built on pass/fail, and a widened vocabulary must add detail rather than silently reclassify stored results. A legacyfailline normalizes totest-failure, so a pre-B4 record folds identically to a post-B4 one.Verification
bun run checkgreen. Unit 910 pass / 0 fail (19 new intest/unit/ci-file-watchdog.test.tscovering cap selection, remaining-budget math and outcome serialization; 2 new inci-metrics).Local — the reap is real. A test that boots CHR 7.23.2 under HVF and then hangs, capped at 75 s:
So the reap is load-bearing, not belt-and-braces.
CI — forced timeout, run 30681180342 (
linux-x86,exec.test.ts,watchdog-cap=45). This is the dispatch B4's tick condition asks for. The test step went red and everyif: always()step still ran — Kill leftover QEMU, Assemble metrics, Write integration summary, Upload integration logs, all green — and the artifact carries:{ "kind": "file-watchdog-timeout", "file": "exec.test.ts", "elapsed_s": 45, "cap_s": 45, "cap_source": "file-table", "qemu_before_reap": 1, "qemu_after_reap": 0, "cleanup_verified": true, "host": { "cpuCount": 4, "totalMemMiB": 15994, "freeMemMiB": 14967, "loadAvg": [0.58, 0.24, 0.09], "df": [...] } }with
integration-timing.txtreadingexec.test.ts 45s file-watchdog-timeoutandmetrics.ndjsoncarryingstatus: "fail"+outcome: "file-watchdog-timeout".CI — control, run 30681189163 (same leg, no lever). Green, caps taken from the table (600 s each), and the timing lines are byte-identical to the old format:
Both dispatches ran
collect-metrics=falseso a synthetic timeout cannot pollute the very window the caps are derived from.A bug the lever found
The viability floor was checked before the file's own cap, so a cap deliberately set below
MIN_VIABLE_CAP_Swas refused as "budget exhausted" with 99 s affordable and a 3 s cap. Found by running the lever, not by reading it — it would have broken the CI proof above. Fixed, with a regression test.watchdog-capexperiment leverAdded so the timeout path can be exercised on a real runner without committing a test that hangs on purpose. It can only shorten a cap — a value at or above the checked-in one is ignored, so it cannot buy a hang more rope. Validated in
planlikeqemu-version/accel(a lever that silently does nothing is worse than a typo) and it emits a::warning::when set. No normal run sets it.What this does NOT do
It does not make a lost runner diagnosable. #76's
macos-x86legs die at ~62 min holding a 290-minute step budget; nothing running inside the job survives that, this script included — the evidence from run 30665449265 is explicit about it. A green watchdog is not progress on #76. That is B5's ledger, and B8a's bounded groups.Out of scope
The incomplete-leg ledger and both of its open design decisions (B5). Boot envelopes (#106). Retuning the caps from a wider window (B11/B12 — this PR gives them a checked-in table and a cited window to retune from).
Refs #77, #110.
Summary by CodeRabbit
New Features
Documentation
Tests