Skip to content

fix: resolve hardware verification findings B2, D1, D2, D3 - #113

Merged
V3RON merged 2 commits into
mainfrom
fix/hardware-verification-b2-d1-d2-d3
Sep 5, 2026
Merged

V3RON merged 2 commits into
mainfrom
fix/hardware-verification-b2-d1-d2-d3

Conversation

@V3RON

@V3RON V3RON commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Restack of #91 onto main, which #91 could not reach: its base was
feat/owned-device-roots-5-doctor, a stack branch that the squash-merges never
made an ancestor of anything on main. Merging it as it stood would have
updated a dead branch and carried a duplicate of #84's already-landed commit.

B1 is deliberately not here. It landed independently as #90, with stronger
coverage than #91's version, so this carries only the four findings still
missing after #111. #91 can be closed in favour of this.

Finding What was wrong
B2 A rejected root took the whole daemon down at startup. Convergence called into a driver for every device it found, including devices of a platform refused at discovery; the NoDriverError aborted convergence and stopped the process — so Android's bad root took iOS with it, the exact inverse of ADR 0001's "a refused root costs that platform and nothing else". Such devices are now left as the registry found them, and doctor reports the rejection.
D1 The kernel caps a Unix socket path at 104 bytes on macOS (108 on Linux), and daemon.sock sits directly under SIMLOCK_HOME. A deeper home failed on connect with a bare EINVAL. resolveDaemonSocketPath refuses it up front in every frontend, with a USAGE error naming the limit.
D2 The Android cold-boot estimate was 31s against emulators that consistently took longer, so lease reported an ETA it could not meet. Raised to 70s from the measured runs.
D3 A device kept its old address through shutdown and quarantined. Android hands the console port to the next boot, so a stale address let list --devices show two records claiming one port.

Conflict resolution

Resolved against current main rather than mechanically — defaultCliEnvironment
has since been refactored into buildCliEnvironment, cliErrorCode now routes
through isSimlockError rather than DaemonClientError, and the new
SIMLOCK_HOME paragraph belonged under its own heading rather than where the
three-way merge landed it. #91's duplicate B1 test was dropped.

Verification

pnpm run check green: typecheck, typecheck:e2e, lint, format, 1329 unit tests,
47 e2e in the fake-driver lane. fallow audit clean on the changed files.

The slow real-hardware lanes have not been run, which is worth weighing here
specifically: these findings came out of manual hardware testing, and D2's new
70s estimate is a measured value this branch cannot re-measure in CI.

Refs #73, #70

Restacked onto main from #91, which was stranded against the now-merged stack
branch `feat/owned-device-roots-5-doctor`. Its B1 fix (a detached lease's
expiry timer keeping a stopped daemon alive) landed independently as #90, with
stronger coverage than the version here, so this carries only the four
findings still missing after #111.

B2 -- a rejected root took the whole daemon down at startup. Startup
convergence called into a driver for every device it found, including devices
of a platform whose driver was refused at discovery; the resulting
`NoDriverError` aborted convergence and stopped the process. That is the exact
inverse of what per-platform fail-closed discovery promises, and of ADR 0001's
"a refused root costs that platform and nothing else": Android's bad root took
iOS down with it. Devices of a platform with no driver are now left exactly as
the registry found them, and `doctor` reports the rejection.

D1 -- the kernel caps a Unix socket path at 104 bytes on macOS, 108 on Linux,
and `daemon.sock` sits directly under `SIMLOCK_HOME`. A deeper home failed on
connect with a bare `EINVAL`. `resolveDaemonSocketPath` now refuses it up
front, in every frontend, with a `USAGE` error naming the limit.

D2 -- the Android cold-boot estimate was 31s against emulators that
consistently took longer, so `lease` reported an ETA it could not meet. Raised
to 70s from the measured runs.

D3 -- a device kept its old `address` through `shutdown` and `quarantined`.
Android hands the console port to the next boot, so a stale address let
`list --devices` show two records claiming one port. The address is dropped on
those transitions; the next `makeReady` supplies a fresh one.

Conflicts resolved against main rather than mechanically: `defaultCliEnvironment`
has since been refactored into `buildCliEnvironment`, `cliErrorCode` now routes
through `isSimlockError` instead of `DaemonClientError`, and the `SIMLOCK_HOME`
paragraph belonged under its own heading rather than where the merge landed it.

Refs #73, #70
Both were introduced by this branch, and neither was caught by the suite.

D1 delivered nothing it promised. `buildCliEnvironment` resolved the socket path
eagerly, and `runCli` takes its environment as a *default parameter* -- whose
initializer runs before the function body, and therefore before the try/catch.
A `SIMLOCK_HOME` over the platform limit escaped `runCli` entirely as an
uncaught rejection with a raw stack trace, so: no structured error line, exit 1
rather than the documented 2, and `simlock --help` -- which needs no socket at
all -- died with it. The `SocketPathTooLongError` branch added to `cliErrorCode`
was unreachable, and `errorExitCode` never got a matching branch. MCP had the
same shape, printing a stack trace onto the stdio channel a client is trying to
speak protocol over.

The path is now resolved where it is used, inside the lazily-invoked `connect`
closure, in both frontends; `errorExitCode` maps it to 2. `--help` works under a
too-deep home, and a daemon command reports one `USAGE` line naming SIMLOCK_HOME.

D3 stranded a device it was supposed to tidy up. Dropping `address` on the way
into `quarantined` is permanent: `quarantined -> ready` is a `reclaim`, and
`ReclaimResult` carries no address for `recoverFromQuarantine` to restore. The
device then came out of quarantine grantable with no address at all --
`grantedDevice` makes the field optional, so nothing rejected it, and the holder
got a grant with no adb serial it could recover, since `driverData` (which holds
the port) is not part of a grant.

The premise was wrong too. The console port lives in `driverData.port`, not in
`address`, and this driver reuses the port already recorded there rather than
taking a new one -- `ManagedDeviceLifecycle.recoverLeased` says so outright. The
drop now stops at `shutdown`, where `makeReady` re-supplies the address on the
way back.

Also from the review, without behaviour changes:

- The emulator `stdio: "ignore"` + `unref()` in this branch is the *other* half
  of B1, not one of the four findings the previous commit message claimed were
  all that was here. `ScriptedProcessRunner` now records the handle each spawn
  returned, index-aligned with `calls`, and a test asserts the launches are
  detached and nothing else is -- argv cannot show this, and `stdio: "ignore"`
  alone does not release the event loop.
- `paths.test.ts` gains the 103/104 and 107/108 boundaries and a multi-byte
  case; every previous case passed against an off-by-one limit or a
  code-unit-based check.
- `StartupConverger`'s class comment claimed devices of a refused platform are
  "left exactly as the registry found them". They are not:
  `#releaseOrphanedHeldLeases` runs first and unguarded, and a dark platform's
  devices still count toward capacity even though excess selection excludes them
  from the candidates. Both limits are now written down where the claim was.

Refs #73, #70
@V3RON

V3RON commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Adversarial review pass — two regressions found and fixed

A review pass against this branch found two defects it had introduced, neither caught by the suite. Both are fixed in dc65a64.

D1 delivered nothing it promised. buildCliEnvironment resolved the socket path eagerly, and runCli takes its environment as a default parameter — whose initializer runs before the function body, and so before the try/catch. An over-long SIMLOCK_HOME escaped runCli entirely as an uncaught rejection with a raw stack trace: no structured error line, exit 1 instead of the documented 2, and simlock --help — which needs no socket at all — died with it. The SocketPathTooLongError branch this PR added to cliErrorCode was unreachable dead code, and errorExitCode never got a matching branch. MCP had the same shape, printing a stack trace onto the stdio channel a client speaks protocol over. Reproduced before the fix, and again after:

$ SIMLOCK_HOME=<127-byte path> simlock --help    # exit 0, prints help
$ SIMLOCK_HOME=<127-byte path> simlock status    # exit 2
{"error":{"code":"USAGE","message":"Daemon socket path is 127 bytes, but this platform allows at most 103: ... Point SIMLOCK_HOME at a shorter directory."}}

D3 stranded a device it was meant to tidy up. Dropping address on the way into quarantined is permanent — quarantined -> ready is a reclaim, and ReclaimResult carries no address for recoverFromQuarantine to restore. The device came back out grantable with no address; grantedDevice makes the field optional so nothing rejected it, and driverData (which holds the port) is not part of a grant, so the holder could not recover the serial. The premise was wrong too: the console port lives in driverData.port, and this driver reuses the one already recorded there rather than taking a new one — ManagedDeviceLifecycle.recoverLeased says so outright. The drop now stops at shutdown, where makeReady re-supplies the address on the way back.

Both fixes are mutation-tested: reverting either fails the new tests.

Also from the review

  • The unref() hunk is B1's other half, not one of the four findings. My earlier commit message claimed this branch carried "only the four findings still missing" — that was wrong, and unref was described nowhere. ScriptedProcessRunner now records the handle each spawn returned, index-aligned with calls, and a test asserts the launches are detached and nothing else is. stdio: "ignore" alone does not release the event loop; nothing asserted this before, and deleting handle.unref() left the whole suite green.
  • paths.test.ts gains the real boundaries (103/104, 107/108) and a multi-byte case. Every previous case passed against an off-by-one limit or a code-unit-based check.
  • StartupConverger's comment was corrected. It claimed devices of a refused platform are "left exactly as the registry found them". They are not — see the known limits below.

Known limits, now written down rather than implied

Neither is introduced by this PR, and both are documented in the code where the overclaim used to be:

  1. #releaseOrphanedHeldLeases runs first and unguarded, so a dark platform's device that held a lease still moves to reclaiming and sits there until its driver returns.
  2. A dark platform's devices still count toward capacity. runningCapacity has no driver filter, so a refused platform's stale inventory can push global.running over budget while no individual platform is over — and excess selection then evicts the healthy platform's warm device, since dark ones are excluded from the candidates but not from the count. This PR makes it reachable (previously convergence crashed first). Fixing it means deciding whether an undrivable device should consume capacity at all, which affects lease admission and the warm pool — a design call that wants its own change rather than being folded in here.

pnpm run check green: 1337 unit, 47 e2e.

@V3RON
V3RON merged commit 9a58822 into main Sep 5, 2026
5 checks passed
@V3RON
V3RON deleted the fix/hardware-verification-b2-d1-d2-d3 branch September 5, 2026 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant