diff options
| -rw-r--r-- | docs/superpowers/followup/pr3803-hil-blindness-reporting.md | 186 | ||||
| -rw-r--r-- | docs/superpowers/followup/pr3803-usbtest-recovery-reserve.md | 175 | ||||
| -rw-r--r-- | docs/superpowers/specs/2026-07-30-hil-usbtest-fleet-wedge-design.md | 10 | ||||
| -rw-r--r-- | test/hil/helper/hil_health.py | 6 | ||||
| -rw-r--r-- | test/hil/helper/hil_util.py | 6 | ||||
| -rwxr-xr-x | test/hil/hil_test.py | 30 |
6 files changed, 23 insertions, 390 deletions
diff --git a/docs/superpowers/followup/pr3803-hil-blindness-reporting.md b/docs/superpowers/followup/pr3803-hil-blindness-reporting.md deleted file mode 100644 index 374ee62c7..000000000 --- a/docs/superpowers/followup/pr3803-hil-blindness-reporting.md +++ /dev/null @@ -1,186 +0,0 @@ -# Blindness Reporting Gaps Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Make a HIL worker's sysfs blindness reach the report in the two cases where it -currently does not — an untested producer, and a board that raises. - -**Architecture:** A worker returns `hil_util.sysfs_blind()` as the last field of its result -tuple; `_blind_note()` turns that into a report banner. Two holes: nothing tests the -producer, and a board that raises returns no tuple at all, so its blindness is lost. - -**Tech Stack:** Python 3.13 stdlib, multiprocessing Pool with `maxtasksperchild=1`. - -## Global Constraints - -- A blind worker answers `SYSFS_UNKNOWN` for every attribute, so its "device not found" - means "could not tell". The report must say so or a red cell reads as a broken board. -- `maxtasksperchild=1`: one worker per board, so the flag is per-board and must not be - smeared across boards. -- Tests: `cd test/hil && python3 test/test_hil_bounded.py`. - -## What is already established - -- `hil_test.test_board` returns `(..., hil_util.sysfs_blind(), stray)`; `_blind_note(mret)` - renders the banner; wired into all three report paths. -- **The producer is provably untested**: replacing `hil_util.sysfs_blind()` with `False` in - the return leaves all tests green. Nothing drives `test_board` — it needs a board dict, a - real flock, a flasher and `test_example` per test. -- Blindness fired for real on ci.lan: four workers went blind in one run, and cells failed - *because* of it (`Printer device not found ... (this worker is blind)`). - -**Why this is a separate PR:** closing it means making `test_board` testable, which is a -refactor of the harness's orchestration layer — a different scope from the containment -work, and the reason the gap was accepted rather than papered over. - -## File Structure - -- `test/hil/hil_test.py` — extract the result-tuple assembly from `test_board` so it can be - built and asserted without running a board; carry blindness out of the raise path. -- `test/hil/test/test_hil_bounded.py` — tests for both. - ---- - -### Task 1: Make the result tuple assembly testable - -**Files:** -- Modify: `test/hil/hil_test.py` (`test_board`, the `return (name, err_count, ...)` at the - end of the try block) -- Test: `test/hil/test/test_hil_bounded.py` - -**Interfaces:** -- Produces: `_board_result(name, err_count, failed_tests, rows, t_total, board_wide_fail)` - returning the 7-tuple `(name, err_count, failed, rows, t_total, blind, stray)`, reading - `hil_util.sysfs_blind()` and `hil_health.kill_own_children()` itself. - -- [ ] **Step 1: Write the failing test** - -```python -class BoardResultCarriesBlindness(unittest.TestCase): - def test_a_blind_worker_reports_it(self): - from helper import hil_util, hil_health - self.addCleanup(setattr, hil_util, 'sysfs_blind', hil_util.sysfs_blind) - self.addCleanup(setattr, hil_health, 'kill_own_children', hil_health.kill_own_children) - hil_util.sysfs_blind = lambda: True - hil_health.kill_own_children = lambda: 0 - row = hil_test._board_result('b', 0, [], [], 1.0, False) - self.assertTrue(row[5], 'blindness did not reach the result tuple') - self.assertIn('b', hil_test._blind_note([row])) - - def test_a_sighted_worker_does_not(self): - from helper import hil_util, hil_health - self.addCleanup(setattr, hil_util, 'sysfs_blind', hil_util.sysfs_blind) - self.addCleanup(setattr, hil_health, 'kill_own_children', hil_health.kill_own_children) - hil_util.sysfs_blind = lambda: False - hil_health.kill_own_children = lambda: 0 - row = hil_test._board_result('b', 0, [], [], 1.0, False) - self.assertFalse(row[5]) - self.assertEqual(hil_test._blind_note([row]), '') -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cd test/hil && python3 test/test_hil_bounded.py BoardResultCarriesBlindness -v` -Expected: FAIL — `module 'hil_test' has no attribute '_board_result'` - -- [ ] **Step 3: Write minimal implementation** - -```python -def _board_result(name, err_count, failed_tests, rows, t_total, board_wide_fail): - """Assemble a worker's result tuple. Separate from test_board so the two fields only - the WORKER can answer -- its process-global blindness latch and what it could not kill - -- are testable without running a board.""" - stray = hil_health.kill_own_children() - return (name, err_count, [] if board_wide_fail else sorted(set(failed_tests)), - rows, t_total, hil_util.sysfs_blind(), stray) -``` - -Replace the tail of `test_board` with: - -```python - return _board_result(name, err_count, failed_tests, rows, t_total, board_wide_fail) -``` - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cd test/hil && python3 test/test_hil_bounded.py -v` -Expected: PASS, and the existing `BlindWorkerReachesTheReport` tests still pass. - -- [ ] **Step 5: Verify the mutation is now caught** - -Replace `hil_util.sysfs_blind()` with `False` inside `_board_result` and re-run; the suite -MUST fail. Restore it. - -- [ ] **Step 6: Commit** - -```bash -git add test/hil/hil_test.py test/hil/test/test_hil_bounded.py -git commit -m "test/hil: make the worker result tuple testable, covering blindness" -``` - ---- - -### Task 2: Carry blindness out of the worker-raise path - -**Files:** -- Modify: `test/hil/hil_test.py` (`test_board`'s except/finally, and `main`'s worker-raise - handler that builds synthetic rows) -- Test: `test/hil/test/test_hil_bounded.py` - -**Interfaces:** -- Consumes: `_board_result` from Task 1. -- Produces: a board that raises still contributes a row whose blindness field is accurate. - -- [ ] **Step 1: Write the failing test** - -```python - def test_a_board_that_raises_still_reports_blindness(self): - """The result tuple is returned inside a try whose finally only releases the lock, - so a board that dies by exception contributed nothing -- and its blindness, the - thing that most explains its failure, was lost with it.""" - from helper import hil_util - self.addCleanup(setattr, hil_util, 'sysfs_blind', hil_util.sysfs_blind) - hil_util.sysfs_blind = lambda: True - row = hil_test._board_result_on_error('b', RuntimeError('boom')) - self.assertTrue(row[5]) - self.assertIn('b', hil_test._blind_note([row])) -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cd test/hil && python3 test/test_hil_bounded.py BoardResultCarriesBlindness -v` -Expected: FAIL — no `_board_result_on_error` - -- [ ] **Step 3: Write minimal implementation** - -```python -def _board_result_on_error(name, exc): - """A row for a board that died by exception. err_count 1, no per-test detail, but the - blindness and stray fields are still accurate -- they explain the failure more often - than the exception text does.""" - rows = [(name, {hil_report.BOUNDARY_CELL: - f'{hil_report.REPORT_CELL["fail"]} {type(exc).__name__}'}, None)] - return _board_result(name, 1, [], rows, 0.0, True) -``` - -Wrap the body of `test_board` so the exception path returns it instead of propagating. - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cd test/hil && python3 test/test_hil_bounded.py -v` -Expected: PASS - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/hil_test.py test/hil/test/test_hil_bounded.py -git commit -m "test/hil: keep a raising board's blindness in the report" -``` - ---- - -## Caution - -`test_board`'s `finally` releases the board flock. Any restructuring MUST keep that -release on every path, including the new error path — a leaked flock locks the board until -the host reboots. diff --git a/docs/superpowers/followup/pr3803-usbtest-recovery-reserve.md b/docs/superpowers/followup/pr3803-usbtest-recovery-reserve.md deleted file mode 100644 index eb8959520..000000000 --- a/docs/superpowers/followup/pr3803-usbtest-recovery-reserve.md +++ /dev/null @@ -1,175 +0,0 @@ -# usbtest Recovery Reserve Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Make the post-hang recovery reserve a derived, asserted property instead of an -accident of four independently-set constants. - -**Architecture:** `hil_test` passes `--budget` and `--outer-timeout` to `usbtest.py`, which -decides at runtime whether a recovery still fits. Today the reserve survives only because -the four numbers happen to line up; nothing ties them together or fails when they stop. - -**Tech Stack:** Python 3.13 stdlib. - -## Global Constraints - -- `usbtest.py`: `RECOVER_FLASH_TIMEOUT = 90`, `RECOVER_RESET_TIMEOUT = 30`. -- `hil_test.py`: `USBTEST_BATTERY_BUDGET = 260`, `USBTEST_RECOVERY_BUDGET = 250`, - `USBTEST_OVERSHOOT = 120`; `outer = BATTERY_BUDGET + (RECOVERY_BUDGET if recovery else - OVERSHOOT)`, used for both the child's `--outer-timeout` and the parent's `run_cmd` bound. -- All five are env-overridable via `hil_util.pos_int_env`, so a rig can change them. -- Tests: `cd test/hil && python3 test/test_hil_health.py` and `test_hil_bounded.py`. - -## What is already established - -The reserve holds at the shipped values, checked by hand: - -- The battery checks its budget BEFORE dispatching a case, so it can overshoot by one - case — worst case `260 + 60 + 5 = 325 s`. -- Recovery is gated on `_time_left() >= RECOVER_RESET_TIMEOUT`, where - `_time_left() = outer_timeout - elapsed - 35`; with `outer = 510` that allows recovery - until `elapsed = 445 s`, and the reflash until `385 s`. -- So ~60 s of margin survives, and recovery does fire. - -**The defect is structural, not arithmetic:** lower `--outer-timeout`, raise `--timeout`, or -raise `USBTEST_BATTERY_BUDGET` via the env and the reserve silently disappears. The failure -mode is a skipped reflash that leaves the D-state holder for the next job — the exact thing -the containment exists to prevent — with no error anywhere. - -**Why this is a separate PR:** it changes the timing contract between `hil_test` and -`usbtest.py`, which affects every board's run duration, so it wants its own review and a -full rig run. - -## File Structure - -- `test/hil/usbtest.py` — a `reserve_ok()` predicate plus a startup assertion. -- `test/hil/hil_test.py` — derive the battery budget from the outer bound rather than - setting both independently. -- `test/hil/test/test_hil_health.py` — tests. - ---- - -### Task 1: Assert the reserve at startup - -**Files:** -- Modify: `test/hil/usbtest.py` (constants block, and `main()` after argparse) -- Test: `test/hil/test/test_hil_health.py` - -**Interfaces:** -- Produces: `usbtest.reserve_ok(budget, outer, case_timeout)` returning bool. - -- [ ] **Step 1: Write the failing test** - -```python -class RecoveryReserveIsChecked(unittest.TestCase): - """The battery may overshoot its budget by ONE already-started case, so the outer bound - must leave room for that overshoot AND a bounded recovery afterwards.""" - - def setUp(self): - import usbtest - self.u = usbtest - - def test_the_shipped_numbers_leave_room(self): - self.assertTrue(self.u.reserve_ok(budget=260, outer=510, case_timeout=60)) - - def test_a_tighter_outer_bound_is_rejected(self): - self.assertFalse(self.u.reserve_ok(budget=260, outer=380, case_timeout=60)) - - def test_a_longer_case_timeout_is_rejected(self): - self.assertFalse(self.u.reserve_ok(budget=260, outer=510, case_timeout=200)) -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cd test/hil && python3 test/test_hil_health.py RecoveryReserveIsChecked -v` -Expected: FAIL — `module 'usbtest' has no attribute 'reserve_ok'` - -- [ ] **Step 3: Write minimal implementation** - -```python -def reserve_ok(budget: int, outer: int, case_timeout: int) -> bool: - """Does `outer` leave room for the battery's worst case AND a bounded recovery? - - The budget is checked BEFORE dispatch, so the battery can run to - `budget + case_timeout + 5` (the +5 is run_case's reap). _time_left() subtracts a - further 35 s of fixed tail. A reflash needs RECOVER_FLASH_TIMEOUT beyond that. - """ - worst_case_end = budget + case_timeout + 5 - return outer - worst_case_end - 35 >= RECOVER_FLASH_TIMEOUT -``` - -In `main()`, after parsing args: - -```python - if args.budget and args.outer_timeout and not reserve_ok( - args.budget, args.outer_timeout, args.timeout): - print(f'warning: --outer-timeout {args.outer_timeout} leaves no room for a bounded ' - f'recovery after a --budget {args.budget} battery with --timeout ' - f'{args.timeout} cases; a HUNG board will be left wedged', file=sys.stderr) -``` - -Warn, do not exit: a caller that deliberately runs without recovery is legitimate. - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cd test/hil && python3 test/test_hil_health.py RecoveryReserveIsChecked -v` -Expected: PASS - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/usbtest.py test/hil/test/test_hil_health.py -git commit -m "usbtest: check the recovery reserve instead of assuming it" -``` - ---- - -### Task 2: Derive the outer bound from one place - -**Files:** -- Modify: `test/hil/hil_test.py` (constants block ~line 227, and `test_device_usbtest`) -- Test: `test/hil/test/test_hil_bounded.py` - -**Interfaces:** -- Consumes: `usbtest.reserve_ok` semantics (duplicate the arithmetic, do not import - usbtest — `hil_test` must not import it). -- Produces: an assertion at module import that the shipped constants satisfy the reserve. - -- [ ] **Step 1: Write the failing test** - -```python - def test_the_shipped_constants_satisfy_the_reserve(self): - """Whatever the env overrides, the pair hil_test computes must leave recovery room: - outer - (budget + case_timeout + 5) - 35 >= 90.""" - outer = hil_test.USBTEST_BATTERY_BUDGET + hil_test.USBTEST_RECOVERY_BUDGET - self.assertGreaterEqual(outer - (hil_test.USBTEST_BATTERY_BUDGET + 60 + 5) - 35, 90) -``` - -- [ ] **Step 2: Run test to verify it fails** - -Temporarily set `HIL_USBTEST_RECOVERY_BUDGET=100` and run; expect FAIL. Unset. - -- [ ] **Step 3: Add the guard** - -```python -# The recovery reserve is a PROPERTY of these two, not a coincidence: the battery may -# overshoot its budget by one already-started case (checked before dispatch), and a bounded -# reflash needs 90 s after a 35 s fixed tail. Env overrides make this checkable at import -# rather than discoverable when a wedge is left unrecovered. -if USBTEST_RECOVERY_BUDGET - 60 - 5 - 35 < 90: - print(f'warning: HIL_USBTEST_RECOVERY_BUDGET={USBTEST_RECOVERY_BUDGET} leaves no room ' - f'for a bounded reflash after a one-case overshoot; HUNG boards will stay wedged', - file=sys.stderr) -``` - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `cd test/hil && python3 test/test_hil_bounded.py -v` -Expected: PASS - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/hil_test.py test/hil/test/test_hil_bounded.py -git commit -m "hil: warn when the timeout constants leave no recovery reserve" -``` 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 cc1c95d53..a34848f06 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 @@ -1,9 +1,9 @@ # HIL fleet-wedge containment Date: 2026-07-30 -Status: implemented, then superseded in part — addendum last checked 2026-08-12 -against the shipped code; where they disagree the CODE and the usb-kernel-recover -skill win, never this document. +Status: implemented, then superseded in part, then TRIMMED (2026-08-25 — see the +addendum at the end). Last checked against the shipped code 2026-08-25; where they +disagree the CODE and the usb-kernel-recover skill win, never this document. - **Pool guard.** A single constant, not the flat 4200s below and not a derivation: `POOL_TIMEOUT = pos_int_env('HIL_POOL_TIMEOUT', 3600)`. A per-controller model briefly @@ -18,8 +18,8 @@ skill win, never this document. 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. 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. + constant: `usbtest.recovery_reserve(flasher)` derives it per flasher (RP-target openocd 390s, + other openocd/jlink/stlink 190s, esptool/lm4flash 150s) — 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 diff --git a/test/hil/helper/hil_health.py b/test/hil/helper/hil_health.py index b9c05c236..d78d0f220 100644 --- a/test/hil/helper/hil_health.py +++ b/test/hil/helper/hil_health.py @@ -246,10 +246,8 @@ def _kill_kids(kids: dict, seen: set) -> int: if denied: _p(f'warning: could not kill {sorted(denied)}; they still hold whatever they ' f'had open (probe, usbfs node) into the next job', flush=True) - # SURVIVORS, not the signalled-child count: the caller needs to know the rig is dirty - # for the next job, and a count of what we successfully signalled cannot tell it that. - # (They are different units anyway -- a killpg is counted once per child sharing the - # group -- so the old return was never comparable to anything.) + # SURVIVORS, not the count we signalled: the caller needs to know the rig is dirty for + # the next job, and a killpg is counted once per child sharing the group anyway. return len(denied) diff --git a/test/hil/helper/hil_util.py b/test/hil/helper/hil_util.py index 29eb05a18..c5b09f65f 100644 --- a/test/hil/helper/hil_util.py +++ b/test/hil/helper/hil_util.py @@ -315,9 +315,9 @@ def usb_scan(vid_pid=None, serial=None, vid=None, timeout=SYSFS_READ_GRACE) -> l """ out = [] for d in glob.glob('/sys/bus/usb/devices/*-*'): - # Interfaces are '<busport>:<cfg>.<ifnum>' (e.g. 2-4:1.0) -- they CONTAIN the - # colon, they do not end with it, so the original endswith() never fired and every - # scan opened idVendor/idProduct on all of them (measured: 31 of 44 matches). + # `in`, not endswith: an interface is '<busport>:<cfg>.<ifnum>' (2-4:1.0), which + # CONTAINS the colon rather than ending with it. Screening them out here is worth + # real time -- they were 31 of 44 matches on this rig. if ':' in os.path.basename(d): continue try: diff --git a/test/hil/hil_test.py b/test/hil/hil_test.py index c4ec19f46..6a7ee206a 100755 --- a/test/hil/hil_test.py +++ b/test/hil/hil_test.py @@ -238,9 +238,9 @@ USBTEST_BATTERY_BUDGET = hil_util.pos_int_env('HIL_USBTEST_BATTERY_BUDGET', 260) # as it goes to print its JSON, turning ~29 real per-case verdicts into "usbtest did not # run" and re-paying the whole battery on retry. # Worst case, from usbtest.py: --timeout 60 (the case) + 5s post-SIGKILL reap + -# dmesg_tail(), which is bounded by HELPER_TIMEOUT=30 and runs on BOTH the FAIL and HUNG -# timeout paths = 95s. 120 leaves a margin; 75 (my first estimate, taken before checking -# dmesg_tail) was 20s SHORT and would have killed the battery mid-print. +# dmesg_tail(), bounded by HELPER_TIMEOUT=30 and run on BOTH the FAIL and HUNG timeout +# paths = 95s. 120 leaves a margin. Re-derive it if any of those three moves -- dmesg_tail +# is the one easily missed, and without it the estimate lands 20s short. USBTEST_OVERSHOOT = 120 # Named, not a literal, so the unit tests can zero it: every test that drives # test_device_usbtest against a fake rig otherwise pays a real 3s (ten of them, 30s a run). @@ -1509,9 +1509,9 @@ def test_device_usbtest(board): f'usbfs node, so usbtest hang recovery is disabled for {board["name"]}; a ' f'HUNG case will leave it wedged for the rest of the run', flush=True) if recovery: - # ship the RECOVERY flasher as `flasher`: usbtest.py, recovery_steps and - # convoy_safe all read board['flasher'], so substituting here keeps the entire - # child side unaware that a second roster entry exists + # ship the RECOVERY flasher as `flasher`: usbtest.py and convoy_safe both read + # board['flasher'], so substituting here keeps the entire child side unaware that + # a second roster entry exists rb = json.dumps({'name': board['name'], 'flasher': _rec_flasher}) cmd += f' --recover-board {shlex.quote(rb)} --recover-fw {shlex.quote(_current_fw)}' # The reserve above USBTEST_BATTERY_BUDGET exists because the battery can overrun by @@ -2500,20 +2500,16 @@ def main() -> None: err_count = build_err + sum(e[1] for e in mret) _write_failed_spec(failed_fname, report_dir, mret) finally: - # Not `with Pool(...)`: its __exit__ joins the workers unbounded, hanging on + # Not `with Pool(...)`: its __exit__ joins the workers unbounded and hangs on # any worker in uninterruptible sleep. shutdown_pool bounds the same terminate() - # by a grace period, so the pool is NOT cleanly closed/joined when it returns - # False. Record the outcome but never exit here: the report below is the only - # record of a run that otherwise passed. + # and returns False when the pool is NOT cleanly closed. # - # Same ordering as the timeout path: what the workers spawned must be - # snapshotted and killed while its parent is alive, or terminate() reparents it - # out of reach. + # Sweep BEFORE shutdown: what the workers spawned must be snapshotted and + # killed while its parent is alive, or terminate() reparents it out of reach. # - # Both calls must stay guarded: a raise here skips accumulate_report(), so a run - # whose boards ALL passed publishes an empty report dir -- and both can raise - # for reasons unrelated to the results. pool_abandoned stays fail-CLOSED, so - # _abandon_exit still arms. + # Both calls stay guarded and neither exits: a raise here would skip + # accumulate_report and publish an empty report dir for a run whose boards all + # passed. pool_abandoned is fail-CLOSED, so _abandon_exit still arms. try: # Still worth running for the TIMEOUT path, where the workers are # genuinely stuck mid-task and their children are still reachable through |