fix(ci): kill orphaned QEMU processes on macOS-x86 step timeout to prevent runner death - #10
Merged
Merged
Conversation
Both x86_64 Extended Verification failures (run 27919557586) were CI-harness, not product: Windows integration passed 6/6 and only the artifact upload failed; macOS-x86 lost the runner to TCG starvation near 60min (no HVF on hosted runners). - verify-extended: write the Windows integration log under $HOME so upload-artifact stops erroring on the cross-drive least-common-ancestor (C: state dir vs D: workspace); mark macOS-x86 best-effort/non-gating (continue-on-error + job timeout) - workflows: declare least-privilege `permissions: contents: read` (the publish job keeps id-token: write) — clears the CodeQL missing-workflow-permissions findings - test/lab/mndp: move network-derived srcMac() out of the console.log format string (CodeQL js/tainted-format-string) and guard udpLen>=8 in ethToUdpPayload - tests: tighten qemu-args anchors and add an empty-body resolveVersion case (folds in the sound parts of AI-findings PRs #6/#8/#9, now closed) The 8 by-design js/clear-text-logging alerts (CLI surfacing generated CHR dev credentials to the operator's terminal) are dismissed separately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 42 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ChangesCI Hardening, Security Fixes, and Test Strengthening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 Jun 22, 2026
This was referenced Jun 22, 2026
There was a problem hiding this comment.
Pull request overview
This PR hardens the CI harness for Extended Verification (especially Windows artifact uploads and macOS-x86 timeouts) and resolves CodeQL workflow-permissions findings, while tightening a few unit tests and addressing lab-only tainted-format-string findings.
Changes:
- Make Extended Verification more reliable: Windows integration logs are written under
$HOMEto avoid cross-drive artifact upload failures; macOS-x86 is marked best-effort with a job timeout. - Add least-privilege workflow defaults (
permissions: contents: read) across CI/publish/verify workflows (publish job retainsid-token: write). - Strengthen unit tests for QEMU arg invariants and add an empty-body
resolveVersiontest; harden lab MNDP probes (udpLen >= 8, avoid tainted format-string position).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/verify-extended.yml |
Fix Windows artifact upload path LCA issue; mark macOS-x86 as best-effort with timeout; set default read-only permissions. |
.github/workflows/ci.yml |
Add default least-privilege permissions for CI workflow. |
.github/workflows/publish.yml |
Add default least-privilege permissions; publish job keeps id-token: write. |
test/unit/qemu-args.test.ts |
Tighten QEMU arg assertions (single -M, mem/cpu, -display none, indexed -netdev/-drive, ensure TCG branch exercised). |
test/unit/versions.test.ts |
Add test asserting empty upgrade-server body maps to INVALID_VERSION. |
test/lab/mndp/stream-unix-probe.ts |
Guard invalid UDP length and avoid tainted format-string usage in logging. |
test/lab/mndp/socket-connect-probe.ts |
Guard invalid UDP length and avoid tainted format-string usage in logging. |
CHANGELOG.md |
Document CI harness changes and security hardening (no published-package change). |
BACKLOG.md |
Record the Extended Verification triage and the folded-in AI-findings fixes. |
Addresses Copilot review nit on #10. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Root cause: when the 50-min integration test step times out on macos-15-intel (TCG, no HVF), bash exits but unref()'d QEMU processes keep running at 100% CPU. These orphaned processes overwhelm the runner over the next ~7 minutes until it loses communication with GitHub, leaving the "Write summary" and "Upload logs" steps permanently pending. Fix (macos-x86 job only): - Add trap _cleanup_qemu EXIT INT TERM in the test step — pkill TERM then KILL qemu-system-x86_64 when bash exits for any reason. This is the primary fix: QEMU is killed immediately when the step is killed, freeing the runner to continue with cleanup steps. - Add an if: always() "Kill QEMU processes" step before Write summary and Upload logs as a belt-and-suspenders secondary cleanup. The workflow-level conclusion was already "success" (continue-on-error), but the job itself was dying hard, preventing any cleanup from running.
Copilot
AI
changed the title
fix(ci): green Extended Verification + 0 code-scanning alerts
fix(ci): kill orphaned QEMU processes on macOS-x86 step timeout to prevent runner death
Jun 22, 2026
![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: 1
🤖 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/verify-extended.yml:
- Around line 408-416: For future defense-in-depth hardening, consider moving
the template variable `${{ steps.files.outputs.list }}` used in the for loop
into an environment variable before the run block, then reference that
environment variable in the for loop with proper quoting. This approach reduces
potential template injection surface area, though the current implementation is
acceptable given the upstream validation at lines 395-396 and the
workflow_dispatch access controls already in place.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 259157f3-3f3c-4744-80c1-93eb4a979657
📒 Files selected for processing (9)
.github/workflows/ci.yml.github/workflows/publish.yml.github/workflows/verify-extended.ymlBACKLOG.mdCHANGELOG.mdtest/lab/mndp/socket-connect-probe.tstest/lab/mndp/stream-unix-probe.tstest/unit/qemu-args.test.tstest/unit/versions.test.ts
Comment on lines
+408
to
+416
| # Write the log under $HOME (the C: user profile) so it shares a drive | ||
| # with the machine state dir (~/AppData/Local). upload-artifact computes a | ||
| # least-common-ancestor across all paths; a workspace-relative path lives on | ||
| # D:\ and has no common ancestor with C:\…\AppData → the upload errors. | ||
| : > "$HOME/integration-output.txt" | ||
| fail=0 | ||
| for f in ${{ steps.files.outputs.list }}; do | ||
| echo "::notice::Running $f" | ||
| if ! QUICKCHR_INTEGRATION=1 bun test "$f" 2>&1 | tee -a integration-output.txt; then | ||
| if ! QUICKCHR_INTEGRATION=1 bun test "$f" 2>&1 | tee -a "$HOME/integration-output.txt"; then |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Path fix correctly addresses cross-drive artifact upload issue.
The $HOME-relative path ensures the log file and machine state directory share a common ancestor on Windows.
Regarding the static analysis hint (template-injection at line 414): the ${{ steps.files.outputs.list }} is validated upstream via the existence check at lines 395-396, and workflow_dispatch already requires write access to trigger. The risk is minimal, but for defense-in-depth, consider using an environment variable with proper quoting in a future hardening pass:
env:
TEST_FILES: ${{ steps.files.outputs.list }}
run: |
for f in $TEST_FILES; doThis is informational only—the current implementation is acceptable given the access controls in place.
🧰 Tools
🪛 zizmor (1.25.2)
[info] 414-414: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for 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.
In @.github/workflows/verify-extended.yml around lines 408 - 416, For future
defense-in-depth hardening, consider moving the template variable `${{
steps.files.outputs.list }}` used in the for loop into an environment variable
before the run block, then reference that environment variable in the for loop
with proper quoting. This approach reduces potential template injection surface
area, though the current implementation is acceptable given the upstream
validation at lines 395-396 and the workflow_dispatch access controls already in
place.
Source: Linters/SAST tools
…er TCG The full integration suite (10 files) starves the hosted Intel runner under TCG (no HVF) — even with the orphan-QEMU cleanup, the runner loses communication near ~64 min before any cleanup/upload step runs. Scope the empty-test-filter default on macos-x86 to a single smoke file (anchor.test.ts: one CHR boot + REST field-presence across 6 endpoints) so the job finishes and goes green while still exercising the core Intel-macOS boot/REST path. An explicit test-filter input overrides for full/targeted runs. Tighten the step timeout to 30 min — a single TCG boot needs far less, and a tight ceiling kills a hung boot before the runner is badly starved, so the cleanup/upload steps can actually run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mobileskyfi
added a commit
that referenced
this pull request
Jul 27, 2026
…override (#99) Refixes #97. The `FEAT_SSBS=0 → TCG` fallback merged in #98 keyed on the wrong axis. It is still **unreleased** (last tag `v0.4.5`), so this corrects it in place rather than deprecating it. ## SSBS was a coincident marker, not the mechanism M4 is the first Apple chip to report `hw.optional.arm.FEAT_SSBS = 0`, which is why the reporter's diagnostics correlated. But Linux 5.6 treats SSBS as an **optional** mitigation and boots fine without it — `cortex-a53` and `neoverse-n1` don't implement it either. SSBS-absence cannot produce `No working init found`. ## Actual root cause: the CHR image requires AArch32, Apple Silicon has none Current arm64 CHR images pair an **AArch64 kernel with a 32-bit ARM userspace**: | CHR image | `/init` type | |---|---| | 7.20.8 arm64 | `ELF 32-bit LSB ARM, EABI5, static` | | 7.22.1 arm64 | `ELF 32-bit LSB ARM, EABI5, static` | | 7.23beta5 arm64 | `ELF 32-bit LSB ARM, EABI5, static` | The 7.22.1 `system` package holds **101 more ARM32 executables and 18 ARM32 shared objects**; the only AArch64 executables are `kexec` and `vmcore-dmesg`. An AArch64 `/init` alone would not fix it — the failure would just move later. Apple Silicon implements **no AArch32 at any exception level** (`ID_AA64PFR0_EL1` is AArch64-only for EL0/EL1), and QEMU under HVF passes that hardware register straight through — `hvf_arch_init_vcpu()` re-reads the live vCPU register and edits only the GIC bit, so **the `-cpu` model is inert**. The guest kernel never sets `ARM64_HAS_32BIT_EL0`, `compat_elf_check_arch()` rejects the `EM_ARM` `/init` with `-ENOEXEC`, the initramfs has no fallback init, and Linux panics at t≈0.076 s. Confirmed by the guest's own panic-time capability bitmap, decoded against Linux 5.6 `cpucaps.h` (`ARM64_HAS_32BIT_EL0 == 13`): | Guest | Bitmap | Caps | `ARM64_HAS_32BIT_EL0` | |---|---|---:|---| | M4, `-accel hvf -cpu host` — panics | `0x20012,28000230` | 8 | **absent** | | `-accel tcg -cpu cortex-a710` — boots | `0x20013,28402230` | 11 | present | The failing set is a strict subset; the only other differences are `ARM64_SVE` and `ARM64_HAS_STAGE2_FWB`, neither of which participates in `execve()`. ## Consequences for the fix 1. **Scope is every Apple Silicon generation, not M4+.** No Apple CPU since 2020 implements AArch32, so the SSBS predicate left **M1/M2/M3 on HVF and panicking** — a live bug, not merely a narrow gate. 2. **A QEMU version floor is not a valid restore signal.** `Hypervisor.framework` exposes only `hv_vcpu_config_get_feature_reg()` — there is no setter — so no macOS VMM can present a feature the silicon lacks (UTM's fork behaves identically). The deferred `getQemuVersion()` guard is **dropped, not postponed**. 3. **The restore signal is the guest artifact:** a future arm64 CHR whose appended `/init` *and* required system-package executables/shared objects are all AArch64, confirmed by a real HVF boot. That check is mechanical and could become a release-time guard. 4. Standard server ARM (Ampere, Graviton) *does* implement AArch32 EL0, which is why arm64 CHR runs there under KVM. This is an Apple-Silicon-plus-CHR-artifact interaction, not a RouterOS-on-ARM defect. Full chain, evidence table, and reproduction: `docs/m4-hvf-arm64-investigation.md`. Reported upstream to MikroTik (request: ship an ARM32-free arm64 userspace, or document that the image requires AArch32 EL0 despite its AArch64 kernel). ## Second bug found while fixing this: x86 CHR was broken on Apple Silicon Not a slowdown — a hard failure, pre-existing and unrelated to SSBS. `detectAccel("x86")` returned `hvf` whenever `kern.hv_support=1`, **including on Apple Silicon**, so `qemu-system-x86_64` was launched with `-accel hvf` on an arm64 host. HVF is compiled into a QEMU binary only when the emulated target matches the physical host, so that binary has no `hvf` accelerator at all and QEMU exits. Verified by the mirror-image test on an Intel Mac, where the same asymmetry is observable locally: ```console $ qemu-system-x86_64 -accel help # target matches host Accelerators supported in QEMU binary: tcg hvf $ qemu-system-aarch64 -accel help # target does not match host Accelerators supported in QEMU binary: tcg ``` `detectAccel` now selects TCG for **both** guest arches on Apple Silicon, and `isAppleSiliconHost()` uses `sysctl.proc_translated` so a Rosetta process (which reports `process.arch === "x64"`) is still recognized as an arm64 host. `isCrossArchEmulation()` became symmetric as a result, so x86-on-arm64 finally gets the cross-arch timeout factor. Repo docs already disagreed with the code here — `qemu.instructions.md` said x86 CHR "must use `accel=tcg`" on Apple Silicon while `DESIGN.md`'s table claimed HVF. The table was wrong; both now match the code. ## Changes - `detectAccel("arm64")` returns `tcg` on **all** macOS hosts. `hostLacksSsbs()`/`ssbsTcgWarning()` → `isAppleSiliconHost()`/`accelNote()`. - **New escape hatch** — `--accel <auto|tcg|hvf|kvm>` on `start`/`add`, plus an `accel` setting and `QUICKCHR_ACCEL` env var (precedence: flag > env > `quickchr.env` > `auto`, matching every other setting). Anything but `auto` is passed to QEMU verbatim and bypasses detection entirely, so HVF can be tested against a future AArch64-only image with no code change. Forcing `--accel hvf` for an arm64 guest on Apple Silicon still prints the panic caveat rather than obeying silently. - `doctor` marks the acceleration row when an override is in effect (otherwise the row reads as a capability report it isn't), and the launch note **names the tier** that set the accelerator (`--accel` / `QUICKCHR_ACCEL` / `quickchr.env`) — a stale `accel=tcg` in the settings file is otherwise an unexplained slowdown. - `integration.yml`'s header claimed `macos-arm64 → arm64 CHR HVF` and printed `Accel hint: HVF (expected)`. Both were false and are corrected — see Verification. - Docs corrected: `DESIGN.md` #10, `CHANGELOG.md` (replaced the unreleased SSBS entry rather than stacking a correction on it), `MANUAL.md`, `.github/instructions/qemu.instructions.md`, `.github/copilot-instructions.md`, and the investigation doc's implications section. ### One structural change worth a look Wiring `platform.ts` to `settings.ts` created an import cycle (`settings → cache → state → network → platform`) that loaded eagerly on **every** CLI start and pushed `cli-settings.test.ts` past its 5 s timeout. The `quickchr.env` file tier is extracted into a leaf module `src/lib/settings-file.ts` (node:fs + paths.ts only); `settings.ts` re-exports it, so the public API is unchanged. `platform.ts` imports the leaf, not `settings.ts`. ## Verification Local (Intel x86_64 Mac): - `bun run check` clean — Biome, `tsc --noEmit`, markdownlint, cspell, examples, shellcheck. - `bun test test/unit/` — 719 pass, 18 skip, 0 fail. - Real x86 CHR booted and answered REST under **both** auto (`-accel hvf`) and `--accel tcg` (`-accel tcg,tb-size=256`), confirming the override reaches QEMU's argv and the launch path is intact. - The Apple Silicon branch is exercised by mocking `process.platform`/`process.arch`. - `QUICKCHR_INTEGRATION=1 bun test` over `start-stop`, `library-api`, `forward-cli`, `settings-secure-login-cli` — 14 pass, 0 fail (326 s, real CHR boots). **CI cannot validate the HVF path, and its docs claimed otherwise.** The `macos-arm64` gating leg runs on hosted `macos-15` runners that are themselves VMs (`Apple M1 (Virtual)`) reporting `kern.hv_support=0`; its own platform log reads `quickchr detectAccel(arm64): tcg` (run [29669706663](https://github.com/tikoci/quickchr/actions/runs/29669706663)). So that leg has **never** exercised arm64 HVF, and a green macOS run is not evidence about #97 either way. The workflow header said it was HVF; that's fixed in this PR so the next reader isn't misled. **Grounding boundary, unchanged and important:** this is **not reproduced locally** — dev and CI hosts are Intel x86_64, and the failure needs Apple Silicon. The M4 evidence is the reporter's (tikoci/mikropkl#11), and the exact `Failed to execute /init (error -8)` line remains unobserved. What's verified here is accelerator selection, the override, and the x86 boot path. **A reviewer on Apple Silicon confirming that `--accel hvf` on an arm64 guest still panics — and that the default TCG path boots — would close the last gap.** Refs #97. Downstream: tikoci/mikropkl#11. 🤖 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 accelerator controls through the `--accel` option, `QUICKCHR_ACCEL`, and the `accel` setting. * Supported modes include automatic selection, TCG, HVF, and KVM, with clear override precedence. * Added status messaging when acceleration is explicitly configured. * **Bug Fixes** * Apple Silicon now reliably uses TCG for CHR guests where HVF is incompatible, improving boot reliability. * **Documentation** * Updated manuals, design guidance, changelog, and troubleshooting information for acceleration behavior and overrides. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 31, 2026
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.
Root cause: When the 50-minute integration test step times out on
macos-15-intel(TCG, no HVF on hosted runners), bash exits but theunref()'d detached QEMU processes keep running at 100% CPU. Over the next ~7 minutes these orphaned QEMU TCG processes overwhelm the runner until it loses communication with GitHub, leaving theWrite integration summaryandUpload integration logssteps permanently pending. The workflow-level conclusion was alreadysuccess(due tocontinue-on-error: true), but the job itself was dying hard with no cleanup.Changes Made
In the
integration-macos-x86job of.github/workflows/verify-extended.yml:trap _cleanup_qemu EXIT INT TERMinside "Run integration tests (sequential per-file)" that runspkill -TERM/-KILL qemu-system-x86_64when bash exits for any reason (timeout, error, or cancellation). This is the primary fix — QEMU is killed immediately when the step ends, freeing the runner before cleanup steps start.if: always()"Kill QEMU processes" step before "Write integration summary" that runs the samepkillsequence, ensuring QEMU is dead even if the trap does not fully fire.These two changes together ensure the cleanup steps (
Write integration summary,Upload integration logs) actually execute after the test step times out, rather than the runner dying mid-job.Summary by CodeRabbit
Security
Bug Fixes