From b190840e1d85f806575f58635827502d714febcf Mon Sep 17 00:00:00 2001 From: hathach Date: Thu, 27 Aug 2026 15:26:43 +0700 Subject: test/hil: drop the sysfs blindness subsystem and derive the recovery reserve Two layers whose cost was a contract to reason about rather than an outcome. SYSFS_UNKNOWN was a three-valued return five files had to keep apart, and misreading unknown as absence was silent: a healthy board reported as a firmware regression. What it guarded is real -- `serial` is served by usb_string_attr, which takes usb_lock_device_interruptible (v6.12.96 sysfs.c:141-143), the same lock a wedged usbfs ioctl holds -- so the BOUND stays, on every caller by default. usb_scan reads `serial` on every device matching the VID, and hil_lock's controller_of does that on essentially every board, so one wedged DUT would otherwise stall every worker, not one. What goes is the third value. read_sysfs now returns str or None, and the question the third value existed to answer is asked directly instead, by two predicates that say which question they answer: sysfs_stranded() is process-wide and sticky, for hil_pool_check's footer ("could anything here be the tool losing sight of healthy hardware?"), and path_stranded(path) is per-device, which is what usbtest needs to tell a DUT whose `serial` is held under device_lock from one that genuinely left the bus -- that difference decides whether it performs driver-registry writes that take the uninterruptible device_lock. Gone: _SysfsUnknown, SYSFS_UNKNOWN, sysfs_blind, sysfs_blind_note, note_sysfs_strand, the cross-process blindness publishing and its report banner, usb_scan's (list, bool) return, usbtest's inconclusive abort, _blind_note's slot in the result tuple, and bounded_open, whose last caller went in the previous commit. The strand memo is rewritten around the one invariant that makes it safe to reuse: it is keyed by the path's kernfs inode, captured BEFORE the read. A busport does not change when a board returns to the same physical port, so a path-only blacklist outlives the wedge and hil_pool_check's own recovery flow -- reset, reflash, wait_device polling that busport -- would never look at the board again. A re-enumeration destroys the kernfs node and makes a new one, so a changed inode is the all-clear. Two ceilings bound different things: per path (_PATH_STRAND_MAX) for a board that flaps while still wedged, and per process (_STRAND_MAX) as a backstop against RLIMIT_NOFILE, counted per PATH rather than per reader because hil_pool_check runs four poll threads over one bus. A board the pool guard never reached is now reported as run-aborted rather than pool-timed-out, and outranks a stale board-locked cell for the same reason the pool-timeout cell does. Both predicates answer conservatively where they are consulted before something irreversible. path_stranded() covers the paths read_sysfs answered None for WITHOUT reading -- past _STRAND_MAX it declines to start another reader, and vouching for a path nobody looked at hands usbtest's fail-CLOSED guard a fabricated all-clear, running remove_id/unbind against a wedged device. usbtest's startup lookup carries the same caveat hil_test's absent arm already did, because its stderr is relayed verbatim into the report cell. strand_note() survives the removal for the same reason master had it: every caller that can say "not found" needs the same sentence, and the one site left to re-invent it got missed -- a wedged-but-enumerated printer was reported as an enumeration failure, sending a maintainer after firmware. The two predicates are not interchangeable, and usbtest needs both. Its per-case verdict is per-DUT -- a peer that stranded at case 2 must not make our board report wedged at case 29 -- but the finally block's cleanup is process-wide: remove_id plus an unbind of EVERY interface under the driver, including that peer's, each taking the uninterruptible device_lock. So the verdict uses path_stranded() and the global cleanup stays gated on sysfs_stranded(). USBTEST_RECOVERY_BUDGET was an independent 250s that could not actually contain the ladder it reserved for, which is why usbtest.py carried a _time_left() gate re-deciding before every step -- with a bare '- 35' for costs paid downstream that nobody could re-derive. Between them the two produced a recovery that skipped its own steps for most real hangs. The reserve is now derived from the bounds usbtest itself declares, per flasher and per target: a probe reset, a reflash, and the Rescue-DP POR plus retry a wedged RP DAP needs, plus the settles and hil_util.REAP_GRACE for each bounded step. The Rescue-DP legs are openocd-only and gated on the RP target cfg, and a stub reset is screened out, so the reserve tracks each board's real ladder instead of one fleet number: 390s for the two RP boards -- whose ladder the old 250 could not contain, which is exactly why the gates skipped their steps -- 190s for the other seventeen probe-reset boards, and 150s for esptool and lm4flash, whose reset is a no-op. Changing a bound in usbtest moves the reserve with it, and a unit test asserts it covers the ladder. With the room actually reserved, the child runs the ladder straight through: recovery_steps, _time_left, the three per-step gates and the parsed-but-unused --outer-timeout are gone. What stays is what decides outcomes -- the convoy_safe gate, reset-before-reflash, the no_op screen so a stub that resets nothing is not claimed, and wedged_pids() as the arbiter, because a clean flash only proves the probe wrote the MCU. hil_util.py 616 -> 514 lines. --- .../followup/pr3840-mret-board-result.md | 9 ++ .../2026-07-30-hil-usbtest-fleet-wedge-design.md | 109 ++++++++++++++++++++- 2 files changed, 117 insertions(+), 1 deletion(-) (limited to 'docs/superpowers') diff --git a/docs/superpowers/followup/pr3840-mret-board-result.md b/docs/superpowers/followup/pr3840-mret-board-result.md index 7b8da7b9c..77b76b605 100644 --- a/docs/superpowers/followup/pr3840-mret-board-result.md +++ b/docs/superpowers/followup/pr3840-mret-board-result.md @@ -3,6 +3,15 @@ **Origin:** split out of PR #3840 (making `hil_report.md` a rendering of `hil_report.json`). Delete this file when its own PR lands. +> **SUPERSEDED IN PART (2026-08-26).** Written against a 7-field tuple whose index 5 was +> `blind`. The sysfs blindness subsystem is gone: `test_board` now returns **6** fields with +> `stray` at index 5, and its board-locked early return is 5 wide. The problem described +> below is unchanged and still worth fixing — three producers, three widths, and +> `len(r) > 5 and r[5]` reads a WRONG SLOT rather than raising. But drop the `blind` field +> from the proposed NamedTuple and re-derive every index from `hil_test.test_board` before +> executing, or `_stray_note` starts reading a duration as a stray count. +> `StrayNoteSurvivesTheTupleWidth` pins the current shape. + ## What is established `test_board()` returns a bare tuple that three producers build and fourteen call sites read diff --git a/docs/superpowers/specs/2026-07-30-hil-usbtest-fleet-wedge-design.md b/docs/superpowers/specs/2026-07-30-hil-usbtest-fleet-wedge-design.md index 3ed0c1519..cc1c95d53 100644 --- a/docs/superpowers/specs/2026-07-30-hil-usbtest-fleet-wedge-design.md +++ b/docs/superpowers/specs/2026-07-30-hil-usbtest-fleet-wedge-design.md @@ -17,7 +17,9 @@ skill win, never this document. - **Job ceilings.** 90/90/120 min (build.yml), not 60/60/90 and not the 85/115 below. They must clear the 3600s guard plus the pre-pool checkout/artifact merge and the post-guard sweep and report upload. No job pins `HIL_POOL_TIMEOUT`. -- **Battery budgets.** `USBTEST_BATTERY_BUDGET` 260s, `USBTEST_RECOVERY_BUDGET` 250s. +- **Battery budgets.** `USBTEST_BATTERY_BUDGET` 260s. The recovery reserve is no longer a + constant: `usbtest.recovery_reserve(flasher)` derives it per flasher (RP-target openocd 350s, + other openocd/jlink/stlink 150s, esptool 110s) — see the trim addendum. The 200s-with-a-197s-floor derivation recorded here was never shipped; the floor assertion was removed with it. - **HUNG recovery.** Reflash of the DUT through its roster flasher @@ -234,3 +236,108 @@ a stuck run and explain it without anyone touching the rig. the board or flashing `board_test` by hand resolves it without any code change. - **An unattended PVE watchdog** that detects the wedge and power-cycles the host. Declined: more moving parts, and it can cut a running CI job. + +--- + +## Trim addendum — 2026-08-25 + +The containment above grew past what one maintainer could hold. This records what was +removed and, more importantly, the rule that decided it, so the next reader does not +re-derive the deleted layers from the incident above. + +### The dividing principle + +**The CI job ceiling bounds how long a run can burn. It does nothing about state that +outlives the run.** Cut what the ceiling contains; keep what it does not. + +- Contained by the ceiling: a worker blocked on a wedged device. `drain_pool` keeps the + boards that finished, `_write_failed_spec` names the one in flight, `_abandon_exit` + writes and uploads the report, and the job dies at `timeout-minutes` regardless. The + cost is one pool slot. +- **Not** contained: a D-state holder left on a usbfs node, or an unswept stray still + holding a probe. The job dies and those survive it, on a self-hosted runner, into the + next run. That is the original incident. + +### Removed + +- **The sysfs blindness subsystem.** `SYSFS_UNKNOWN`, the `_SysfsUnknown` sentinel, the + path→inode strand memo with its `_STRAND_MISS` miss-sentinel, the four-credit blindness + cap, `sysfs_blind()`/`sysfs_blind_note()`, `note_sysfs_strand()`, `bounded_open()`, + `usb_scan`'s `(list, bool)` return, usbtest's `inconclusive` abort, and `_blind_note`'s + report banner. `read_sysfs` is an ordinary `open().read()` returning `str | None`. + + It was a three-valued contract five files had to reason about, and misreading unknown as + absence was silent — a healthy board reported as a firmware regression. It existed for + exactly one attribute that can block. Verified against v6.12.96 `sysfs.c`: only + `usb_string_attr` (`product`/`manufacturer`/`serial`, sysfs.c:141-143) takes + `usb_lock_device_interruptible`; `idVendor`, `idProduct`, `bcdDevice`, `busnum`, + `devnum` and `speed` are lock-free `sysfs_emit` from cached fields. Two of the five + `read_sysfs` call sites read attributes that cannot block at all. + + **The bound stayed, and it is not opt-in.** An early cut of this trim made `read_sysfs` + unbounded on the theory that a blocked worker costs one pool slot. That is false: + `usb_scan` reads `serial` on every device matching the VID to find the one it wants, and + `hil_lock.controller_of` does exactly that from `controller_permit`, on essentially every + board — so one wedged DUT would stall *every* worker and the pool guard would take the + whole run. `read_sysfs` and `usb_scan` are bounded by `SYSFS_READ_GRACE` by default; + three call sites forgot an opt-in version within a single sitting, and a unit test now + pins the default. + + What is gone is the *contract*, not the bound: no third value, no process-wide blindness + latch, no `(list, bool)` return, no report banner. A give-up reads as None like any + unreadable attribute, and the cost is confined to the device that is actually wedged. + + **`hil_pool_check` is why the memo has to be exact.** It is a standalone + ThreadPoolExecutor tool with no guard behind it, run precisely when a device is suspected + wedged, and it polls (`wait_device` re-scans every 0.5 s). The bounded read gives up and + remembers + the path so a poll loop cannot leak a thread and an fd per pass. That memo is keyed by + **kernfs inode, not by path**: a busport does not change when a board returns to the same + physical port, so a path-only blacklist would outlive the wedge and make the tool's own + recovery flow (reset/reflash → `wait_device` polls for the new inode) never see the board + again. A changed inode is the all-clear; `os.stat` is safe on a wedged device because it + does not invoke `->show()`. A give-up reads as None + — the same as unreadable — and `sysfs_stranded()` lets the footer warn that a "missing" + row may be the tool losing sight of healthy hardware. One local bound with a warning + line, not the five-file three-valued contract that was removed. + +- **The recovery budget arithmetic.** `recovery_steps()`, `_time_left()` and its three + per-step gates. The reserve was an independent 250s — one number for the whole fleet — + that could not contain the ladder it + reserved for (reset 30 + reflash 90 + Rescue-DP POR 90 + retry 90 + settles), which is + why the child re-decided before every step — with a bare `- 35` for downstream costs + that nobody could re-derive. Between them they produced a recovery that skipped its own + steps for most real hangs. The reserve now counts `hil_util.REAP_GRACE` **per bounded + step** — `run_cmd` spends that reaping a child it had to SIGKILL, on top of the step's own + timeout — which is what the `- 35` was standing in for. Undersizing it is worse than not + recovering at all: the outer killpg lands mid-reflash and orphans the flasher on the + probe. A unit test asserts the reserve covers the ladder. `USBTEST_RECOVERY_BUDGET` is now derived from + `usbtest.RECOVER_*` **per flasher and per target**: the Rescue-DP legs are openocd-only + (`rescue_openocd` refuses anything else) and a stub reset is screened out, so an esptool + board no longer reserves 200s it can never spend. The child runs the ladder straight + through, and `--outer-timeout` — parsed but unused once the gates went — is deleted. + +### Deliberately kept + +- The pool guard, `drain_pool`, the re-run spec, `_abandon_exit`, the CI ceilings. +- `hil_health`'s sweep **including** `_kill_and_confirm`. SIGKILL is queued, not delivered, + for a task in uninterruptible sleep, and a healthy in-flight testusb sits in exactly that + state — so `os.kill` returning success proves nothing, and the recheck is the only honest + answer to "is the rig dirty for the next job?". +- usbtest's reset→check→reflash ladder and the `convoy_safe` gate. This is the only thing + that unpoisons the rig mid-run, and PR #3832 extends it from 11 to 18 of 27 boards. +- `mtp_test.py` as a separate process — one job, a clean boundary, and runnable by hand + against a board while debugging. + +### Structural changes with no behaviour change + +- Blocking device IO now runs in a child process everywhere, not just where it was noticed + first. The printer WRITE half joined the read half (`usblp_open` ignores `O_NONBLOCK` and + stalls in `usb_autopm_get_interface()` holding the driver-global `usblp_mutex`), and the + HID echo followed (`hid.enumerate()` reads `manufacturer`/`product` for every HID device + it lists, both under the device lock). `test_device_midi_test` is NOT in that set: ALSA + rawmidi honours `O_NONBLOCK` on open (v6.12.96 rawmidi.c:489), unlike usblp. +- `main()`'s two abort paths were near-identical 40-line blocks; `_abort_report` holds that + shape once. The controller-hint cache and pool construction moved to their own helpers. +- The unit suite stopped sleeping 54 of its 78 seconds — mostly one named-and-zeroable + post-flash settle paid by ten tests against a fake rig. -- cgit v1.3.1