ci: bound the test step, correct the macos-x86 accel claim, and ground #79's www mechanism - #108
Conversation
…s www mechanism Three things, all in service of #76 and #79. **Step-level timeout (#76).** The integration test step had no cap of its own, only the job budget. When a job timeout fires GitHub tears the runner down and `if: always()` never runs — so the reap, metrics, summary and artifact-upload steps are all skipped and the leg produces literally no output. That is why macos-x86 has resisted diagnosis for weeks. The step now caps at 10 minutes under the job budget, which kills only the step and leaves the job alive to report. It does not rescue a runner that loses communication with the server; nothing in-job can. **Accel misclassification (#76).** macos-15-intel is a bare-metal Intel runner where detectAccel() returns hvf (timeoutFactor 1.5, boots 30-46 s at cpu-load 0-1 %), but the platform table hardcoded tcg — and the "Accel hint" summary line printed that hardcoded value one row below the detectAccel line contradicting it. The table now records the expected accelerator separately from the empirical run-time budget, so a claim about the runner is no longer entangled with a measurement of how long its suite takes. The summary reports the *detected* accelerator and raises a ::warning:: on drift, so the table cannot go stale silently again. macos-x86 keeps the extended budget: its full suite has never finished inside 60 min, which is an open question, not an accel fact. Verified by running the plan script locally across dispatch shapes (all / gating / single-platform, with and without tcg-smoke and test-filter): every leg's timeout and smoke eligibility is unchanged. **#79's mechanism, grounded (test/lab/www-abort-damage/).** The root-cause comment on #79 inferred "aborted probe -> wedged www" from one socket table. Reproduced locally on RouterOS 7.21.5, Intel Mac, x86 under HVF and arm64 under cross-arch TCG: - A lone aborted request is harmless at any probe delay from 0-3000 ms. - 2-5 consecutive aborts with 0-2000 ms gaps are harmless. - An aborted request FOLLOWED BY A COMPLETED ONE, repeated, is the trigger. www then resets incoming connections, and a surviving connection is answered with the status computed for the *aborted* request — an admin: probe carrying a valid empty password comes back 401. Two runs byte-identical. Health is checked with curl from a separate process, so this is guest-side, not Bun node:http socket reuse; and it reproduces on x86 as well as arm64, so it is not an arm64 quirk. waitForBoot() emits exactly that pattern whenever a probe overruns its 3 s deadline and the next one succeeds — our probe damages the service it is waiting for, and the damage is a WRONG answer, not just a lost one (the #69 surface). Recorded honestly in DESIGN.md and qemu.instructions.md, including what did NOT reproduce: www recovers within ~6 s here, so the permanent wedge remains an inference, and nothing local explains what made CI's first probe exceed 3 s (a failed login answers in ~125 ms even at cpu-load 100). Also confirmed locally that clean() leaves state.user naming the deleted user — but that alone is survivable, since waitForBoot() accepts 401 as proof REST responded. No fix to the probe itself here; that is #79's own change. Refs #76, #79, #69, #106 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:
📝 WalkthroughWalkthroughThe PR adds a RouterOS REST abort-damage lab and related design documentation, then updates integration CI to separate accelerator from runtime budget, calculate step timeouts, detect accelerator drift, and document the changes. ChangesRouterOS REST abort-damage investigation
Integration workflow runtime planning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AbortLab
participant RouterOS_www
participant curlProbe
AbortLab->>RouterOS_www: start REST request
AbortLab->>RouterOS_www: destroy request socket
AbortLab->>RouterOS_www: send completed REST request
RouterOS_www-->>curlProbe: return health status
curlProbe-->>AbortLab: report result
Possibly related PRs
Suggested reviewers: 🚥 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.
Pull request overview
This PR improves integration CI diagnostics (especially for macos-x86) by adding a step-level timeout below the job timeout, corrects/grounds accelerator reporting for macos-x86, and adds a lab + durable documentation capturing the reproduced RouterOS www corruption mechanism related to aborted REST probes (issue #79 context).
Changes:
- CI: add a per-step timeout for the integration test step (10 minutes under the job timeout) so
if: always()teardown/metrics/artifacts still run when tests hang. - CI: correct macos-x86 accel classification/reporting by separating “expected accel” from “budget class” and reporting detected accel with drift warnings.
- Labs/docs: add a new lab (
test/lab/www-abort-damage/) and record the reproduced “abort then complete”wwwcorruption mechanism in durable docs.
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 |
|---|---|
.github/workflows/integration.yml |
Adds step-level timeout, separates accel vs budget in the plan matrix, and improves platform/accel reporting for diagnosis. |
test/lab/www-abort-damage/www-abort-damage.test.ts |
New lab test formalizing the reproduced www corruption pattern with node:http + curl health checks. |
test/lab/www-abort-damage/REPORT.md |
Lab report documenting experiments, positive/negative results, and bounds of the reproduction. |
test/lab/README.md |
Registers the new lab in the lab index. |
.github/instructions/qemu.instructions.md |
Records the “aborting a probe damages www” finding and resulting operational rules. |
DESIGN.md |
Adds design decision capturing the mechanism and constraints/bounds. |
CHANGELOG.md |
Notes CI timeout and macos-x86 accel reporting changes. |
project-words.txt |
Adds steptimeout to the cSpell project dictionary. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@CHANGELOG.md`:
- Around line 50-56: Update the changelog entry to remove the inaccurate claim
that no leg’s smoke eligibility changed: document that macos-arm64, now
classified as tcg|standard, no longer qualifies for tcg-smoke, or narrow the
claim specifically to macos-x86.
In `@test/lab/www-abort-damage/www-abort-damage.test.ts`:
- Around line 36-37: Update the lab gating and port initialization near SKIP and
PORT: parse QUICKCHR_INTEGRATION using explicit enabled values so “0” does not
enable the test, and validate QUICKCHR_LAB_PORT as an integer within the valid
1–65535 port range before allowing execution.
- Around line 83-84: Update the settle helper in www-abort-damage.test.ts to
poll for www recovery until a bounded deadline instead of sleeping a fixed six
seconds. Return or record the actual elapsed recovery time, and ensure the
report uses that measured duration while preserving independent settling between
patterns.
- Around line 111-126: Update the assertions in the pairs/flat validation to
verify each completed admin request is tied to its corresponding bogus request,
requiring the documented tuple pattern including ["ABORT", "401"] rather than
only checking that flat contains "401". Enforce the exact reset positions and
sequence documented for this scenario, while preserving the existing four-pair
setup and connection ordering.
- Around line 49-80: Update connect() and curlProbe() to distinguish
guest-induced resets from unrelated failures instead of mapping every error or
nonzero curl exit to "RST". Handle response close/aborted events separately,
preserve normal completion, and add a bounded timeout for requests that never
complete; classify connection refusal, timeout, missing curl, and other failures
with distinct non-RST outcomes so reset assertions cannot pass spuriously.
🪄 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: 8059c325-65f3-41d8-b237-4f825e02fc39
📒 Files selected for processing (8)
.github/instructions/qemu.instructions.md.github/workflows/integration.ymlCHANGELOG.mdDESIGN.mdproject-words.txttest/lab/README.mdtest/lab/www-abort-damage/REPORT.mdtest/lab/www-abort-damage/www-abort-damage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Unit Tests (windows-latest)
🧰 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/www-abort-damage/www-abort-damage.test.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/www-abort-damage/www-abort-damage.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/lab/www-abort-damage/www-abort-damage.test.ts
🧠 Learnings (1)
📚 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
🪛 LanguageTool
.github/instructions/qemu.instructions.md
[style] ~151-~151: Consider using “the surrounding budget”.
Context: ... deadline with the accel factor**, like the budget around it. Under TCG the honest fix for a slow an...
(NOUN_AROUND_IT)
🪛 zizmor (1.28.0)
.github/workflows/integration.yml
[warning] 350-350: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 371-371: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 373-373: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 397-397: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (8)
.github/workflows/integration.yml (4)
23-57: LGTM!Also applies to: 87-87
157-194: LGTM!Also applies to: 223-253
346-398: LGTM!
481-486: LGTM!project-words.txt (1)
374-374: LGTM!.github/instructions/qemu.instructions.md (1)
130-160: LGTM!test/lab/README.md (1)
19-19: LGTM!test/lab/www-abort-damage/www-abort-damage.test.ts (1)
55-59: 🗄️ Data Integrity & IntegrationNo action needed.
restGet()usesnode:httpwithagent: falseandConnection: close, matching the probe’s connection semantics.
…signal Addresses the PR #108 review. **The lab's assertions were too weak for the claim they support.** CodeRabbit was right that `flat.toContain("401")` does not demonstrate wrong-response delivery — a `401` in the *abort* slot is just the bogus request answering normally. The assertion now works on the pair: some pair must be `["ABORT", "401"]`, i.e. the request we let COMPLETE while sending `admin:` with its valid empty password came back 401. Likewise the reset count now looks only at the completed slot of each pair. **Failures are no longer collapsed into "RST".** `connect()` mapped every error and `curlProbe()` every non-zero curl exit to `RST`, so a refusal, a stall, or a missing curl could have satisfied the guest-reset assertions — in a lab whose entire point is that *the guest* resets the connection. Both now classify: ECONNRESET/56 → RST, ECONNREFUSED/7 → REFUSED, 28 → TIMEOUT, 127 → NO-CURL, everything else labelled with its code. A completed request also gets a 20 s stall guard so it cannot hang the suite. **Recovery is polled, not slept.** A fixed 6 s wait was fragile under TCG scheduling variance and, worse, was reported as the recovery time. It is polled with a bounded deadline and logs the real figure: measured at 1344 / 2301 / 2309 / 2376 ms across four runs. Every "~6 s" claim in DESIGN.md, qemu.instructions.md, REPORT.md and the test header is corrected to the measured 1.3-2.4 s range — the 6 s was my settle interval, not a measurement. **The accel-drift warning no longer fires on an expected fallback.** Copilot was right: a Linux leg dropping kvm->tcg because the runner has no writable /dev/kvm is a runner condition, not a stale table, and "update the platform table" is the wrong advice. That case is now a ::notice::; ::warning:: is reserved for genuine drift, so it stays meaningful on macos-x86/windows-x86. **Also hardened:** QUICKCHR_LAB_PORT is validated as a real port, so the file cannot silently "pass" against nothing. The QUICKCHR_INTEGRATION gate is left matching every other test file in the repo. Not changed: CodeRabbit's CHANGELOG finding claimed macos-arm64 loses tcg-smoke eligibility. It does not — eligibility now follows `budget`, and macos-arm64 was ineligible before (tcg=false) and after (budget=standard). Verified by running the plan script from HEAD~1 and HEAD with tcg-smoke=true: byte-identical smoke/timeout for all five legs. Verified: lab 5/5 on x86/HVF and arm64 cross-arch TCG with the stricter assertions; biome, markdownlint, cspell clean; actionlint finding count unchanged from main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.github/workflows/integration.yml:22
- Spelling: “comma-separate” should be “comma-separated”.
# Platform ids (comma-separate to combine, or use a group alias):
Picks up items 1 and 4 from the #79/#76 triage, plus the two things the last session left as judgement calls: recording the mechanism in the durable docs, and attempting the local repro. Items 2 and 3 (the probe fix and the #106 envelope retune) are deliberately not here.
1. Step-level timeout on the test step (#76)
The test step had no cap of its own — only the job budget. When a job timeout fires, GitHub tears the runner down and
if: always()never runs, so the reap, metrics, summary and artifact-upload steps are all skipped and the leg produces no output at all. That is the reason macos-x86 has resisted diagnosis for weeks.The step now caps 10 min under the job budget (
steptimeout = timeout - 10), which kills only the step and leaves the job alive to report.Honest limit: this does not rescue a runner that loses communication with the server, which is how both macos-x86 legs actually died at ~60 min. Nothing in-job can. It converts the hang case into an honest failure with a report; #76's per-file cap is still the follow-up for the starvation case.
2. macos-x86 accel misclassification (#76)
macos-15-intelis a bare-metal Intel runner wheredetectAccel()returns hvf (timeoutFactor 1.5, boots 30–46 s atcpu-load: 0–1%). The platform table hardcodedtcg, and the "Accel hint" summary line printed that hardcoded value one row below thedetectAccelline contradicting it.accel(a claim about the runner) separately frombudget(the measured run-time class). Entangling them is what produced the wrong value.::warning::on drift, so the table cannot go stale silently again.Verified by running the plan script locally across dispatch shapes (
all/gating/ single platform, with and withouttcg-smokeandtest-filter): every leg's timeout and smoke eligibility is unchanged.3. #79's mechanism, reproduced locally (
test/lab/www-abort-damage/)#79's root-cause comment inferred "aborted probe → wedged www" from one socket table. Reproduced on RouterOS 7.21.5, Intel Mac, x86 under HVF and arm64 under cross-arch TCG:
200throughoutwwwresets incoming connections; a surviving one gets the ABORTED request's responseConcretely — probe as a non-existent user, abort mid-flight, then probe as
admin:with its valid empty password, and get401: the status belonging to the aborted request. Two runs of the discriminating experiment were byte-identical.Health is checked with
curlfrom a separate process, so this is guest-side rather than Bunnode:httpsocket reuse. And it reproduces on x86 as well as arm64, which is the load-bearing part of the claim.waitForBoot()emits exactly that pattern whenever a probe overruns its 3 s deadline and the next one succeeds. Our probe damages the service it is waiting for — and the damage is a wrong answer, not merely a lost one (the #69 surface).What did NOT reproduce, stated plainly
wwwrecovers within ~6 s here. "After five abortswwwstops accepting SYNs forever" remains an inference from CI's socket table.cpu-load: 100, and the fullclean()→ relaunch →waitForBoot()scenario on arm64/TCG resolved in 51 s. The entry point to the CI failure is still unexplained.clean()leavesstate.usernaming the deleted user (defect A) — but that alone is survivable, sincewaitForBoot()accepts401as proof REST responded. Defect B is the one that matters, as CI: post-clean() relaunch never becomes REST-ready on Linux hosts (long-term / 7.21.5) #79 already argued.Recorded in
DESIGN.md(design decision 7) andqemu.instructions.md, both including the negative results.Not in this PR
clean(). The lab now says backing off matters because it is the abort/complete alternation, not the abort count, that does the damage.Verification
actionlintfinding count unchanged frommain(2 pre-existingSC2016infos).🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
CI Improvements
Tests