Skip to content

fix(daemon): let the daemon exit while a detached lease is outstanding - #90

Merged
V3RON merged 1 commit into
mainfrom
claude/pr-85-rebase-description-3s22wy
Sep 2, 2026
Merged

V3RON merged 1 commit into
mainfrom
claude/pr-85-rebase-description-3s22wy

Conversation

@V3RON

@V3RON V3RON commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Replaces #85, rebased directly onto main instead of the owned-device-roots stack — this defect predates that work and is unrelated to it.

The defect

simlock daemon stop returns promptly and the process stays alive. The operator is left with a stopped daemon still holding its socket, and whatever restarts it finds the address in use.

LeaseEngine.dispose — the hook DaemonServer#stop invokes — cancelled the quarantine coordinator's timers and not the expiry scheduler's:

/** Cancels the quarantine coordinator's armed retry timers on daemon shutdown. */
dispose(): void {
  this.#quarantine.dispose();
}

A lease's TTL is a real setTimeout, so an outstanding lease kept Node's event loop alive for as long as its deadline had left — fifteen minutes by default for a detached one. LeaseExpiryScheduler.dispose() existed and was never called from production code.

Held leases hid it: shutdown releases them, and releasing cancels the timer. Detached leases are deliberately left alone, since their liveness is the TTL rather than a connection, so they were the only ones left armed.

Why the fix is safe

Cancelling expires nothing early and loses nothing: ttlDeadline is persisted with the lease and re-armed by LeaseExpiryScheduler.restore on the next start. That is the same mechanism that lets a detached lease outlive a restart in the first place, which is the property that had to be preserved — the fix must not become "release everything on shutdown".

Not part of the owned-device-roots stack

It reproduces on main, with none of that stack applied. #85 carried it as the sixth of six stacked PRs and additionally picked up unrelated review fixes from #84 along the way, which drew a request to split this fix out and land it directly on main. This PR is that split: only LeaseEngine.dispose and its tests, cherry-picked from #85's 08abd5de53. The comment-only hunk in e2e/lease-environment-passthrough.test.ts is dropped here since that file doesn't exist on main yet — it's introduced by #83.

Verification

The unit test fails against the old code: removing this.#expiry.dispose() gives expected 1 to be +0, an armed timer surviving disposal. Coverage is in two places:

  • a unit test asserting no timer survives disposal and that the lease itself is untouched, since cancelling a timer must not expire a lease;
  • an e2e case in daemon lifecycle & recovery that leases detached, stops the daemon, and lets teardown's stray-process check be the assertion.

pnpm run check green: typecheck, typecheck:e2e, lint, format, 995 unit tests, e2e 35 passed / 1 expected fail / 9 skipped. fallow audit clean across 3 changed files.

🤖 Generated with Claude Code

https://claude.ai/code/session_015ia6AUpUzuF7aL3Z4CKUCx


Generated by Claude Code

`daemon stop` returned promptly and the process stayed. `LeaseEngine
.dispose` cancelled the quarantine coordinator's timers and not the
expiry scheduler's, and a lease's TTL is a real `setTimeout`, so an
outstanding lease kept Node's event loop alive for as long as its
deadline had left -- fifteen minutes, by default, for a detached one. The
operator sees a stopped daemon holding its socket, and whatever restarts
it finds the address in use.

Held leases hid it: shutdown releases them, and releasing cancels the
timer. Detached leases are deliberately left alone, since their liveness
is the TTL rather than a connection, so they were the only ones left
armed -- which is why nothing in the suite had tripped over it until an
e2e flow left one behind.

Cancelling expires nothing early and loses nothing. `ttlDeadline` is
persisted with the lease and re-armed by `LeaseExpiryScheduler.restore`
on the next start, which is the same mechanism that lets a detached lease
outlive a restart in the first place.

Predates the device-root work: it reproduces on main.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ia6AUpUzuF7aL3Z4CKUCx
@V3RON
V3RON merged commit b12b086 into main Sep 2, 2026
5 checks passed
V3RON added a commit that referenced this pull request Sep 5, 2026
…01) (#111)

Implements ADR 0001. Ownership stops being a guess about a device's name and
becomes a fact about its location.

- A device root is proven, not assumed. `ensureOwnedRoot` creates a root only
  when it creates it empty itself, and refuses one that is unmarked, marked for
  another instance, symlinked, or wrongly owned or permissioned. The root is
  assembled in a staging sibling and published with a single `rename`, so
  creation is atomic with its marker.
- Both drivers answer `listManaged` from root membership. A `simlock-` name
  prefix survives only as a cosmetic label; it is no longer evidence of
  ownership, so an identically-named user device is never adopted.
- A refused root costs that platform and nothing else -- never a fallback to
  the default device location; reported as `driver.root-rejected` at startup
  and as a `doctor` finding on every run after.
- A grant carries a driver-built `environment` the core never reads,
  `--export-env` prints it for `eval`, and the scoped `simlock simctl` /
  `simlock adb` wrappers let a lease holder reach a contained device without
  handing back the capability containment removed.
- `doctor --purge-orphans` is the single opt-in exception to registry-only
  destruction, behind its own flag and a confirmation, with every root
  re-proven before anything is destroyed.

Integrates the five reviewed PRs #80-#84 and resolves them against ADR 0003's
typed-contract daemon. Two ADR corrections were made during implementation:
#82 amends decision 4 (`ADB_EMU=0` is what actually contains adb's emulator
scan; raising `ADB_LOCAL_TRANSPORT_MAX_PORT` alone widens a sweep that always
starts at 5555), and #84 amends `docs/ARCHITECTURE.md`, which claimed Simlock
cannot address anything outside a root -- `destroyLegacy` breaks that by
design, per the ADR's migration paragraph.

Known gap: the hardware-verification fixes in #91 are not included. B1 landed
independently via #90, but B2 (a rejected root plus interrupted-reclaim state
crashes the daemon at startup), D1 (no socket-path length check), D2 (Android
boot estimate too low) and D3 (stale address on quarantined devices) are still
outstanding and land next. The slow real-hardware lanes have not been run.

Refs #73, #70
V3RON added a commit that referenced this pull request Sep 5, 2026
Restack of #91 onto main, which it could not reach: its base was the
squash-merged stack branch `feat/owned-device-roots-5-doctor`. B1 is not here --
it landed independently as #90 with stronger coverage -- so this carries the
four findings still missing after #111.

- **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, and the resulting `NoDriverError` aborted
  convergence and stopped the process -- Android's bad root took iOS with it,
  the inverse of ADR 0001's "a refused root costs that platform and nothing
  else". Such devices are now left alone and reported by `doctor`.
- **D1**: the kernel caps a Unix socket path at 104 bytes on macOS, 108 on
  Linux, and `daemon.sock` sits under `SIMLOCK_HOME`. A deeper home failed on
  connect with a bare `EINVAL`; it now reports one `USAGE` line naming the
  limit, in both the CLI and MCP.
- **D2**: the Android cold-boot estimate was 31s against emulators measured at
  60-71s, so `lease` quoted an ETA it could not meet and `Doctor` flagged
  healthy boots. Raised to 70s.
- **D3**: a device kept a stale `address` through `shutdown`. Dropped there,
  where `makeReady` re-supplies it on the way back.

Also carries the emulator `stdio: "ignore"` + `unref()` that is the other half
of B1's daemon-stop fix, now with a test -- argv cannot show whether a spawned
child released the event loop, and `stdio: "ignore"` alone does not.

An adversarial review of this branch found two regressions it had introduced,
fixed here before merge: D1's error never reached its handler (resolved in
`runCli`'s default parameter, which runs before the try/catch, so it escaped as
an uncaught rejection and took `simlock --help` with it), and D3's drop extended
to `quarantined`, where it was permanent -- `ReclaimResult` carries no address
for `recoverFromQuarantine` to restore, leaving a device grantable with no
serial its holder could reach.

Known limit, documented in `StartupConverger` rather than implied: a refused
platform's devices still count toward capacity, so a large dark inventory can
make the healthy platform look over budget and get its warm device evicted.
Pre-existing, and reachable now that convergence no longer crashes first;
fixing it means deciding whether an undrivable device should consume capacity
at all.

Refs #73, #70
@V3RON
V3RON deleted the claude/pr-85-rebase-description-3s22wy branch September 9, 2026 17:12
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.

2 participants