fix: enforce device claims for sessionless device mutations (#1799) - #1809
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head b807323. Clean code/architecture verdict: claim policy is descriptor-required, transient-exclusive admission sits at the request-runtime binding seam before operations are exposed, legacy classifications are gate-rejected, and the two-daemon regressions plus live Android A/B directly prove #1799 and cleanup. Readiness is currently blocked by exact-head CI: Android Smoke failed in the unrelated live scroll helper ( |
|
Re the Android Smoke failure at b807323: the failing step is |
|
Android Smoke rerun on the same head passed → the earlier |
b807323 to
01fcba5
Compare
|
Tightening pass after review — rebased onto current Before → after (this PR's diff vs
What changed:
|
|
Clean re-review at 01fcba5: the scope split is coherent—daemon-stop claim reporting moved to #1818, while #1809 retains the descriptor-owned claim policy, runtime-binding admission, transient lifecycle, and the exact foreign-claim boot/shutdown regressions for #1799. No shutdown-reporting files remain in this diff. Exact-head CI and all platform smokes pass; merge state is CLEAN. Ready-for-human. |
`boot` and `shutdown` never consulted the host-global device claim store, so a daemon in one state directory could terminate an emulator another daemon held a verified-live claim on and report success (#1799). Rather than adding a claim check to those two handlers, this makes the class unrepresentable: `CommandDescriptor` gains a REQUIRED `deviceClaimPolicy` trait (#1320's vocabulary), and the request-execution scope enforces it where the request runtime bindings create a device binding — the one seam through which any handler can obtain device operations, and already the place per-device deduplication lives. A `transient-exclusive` command acquires a command-scoped claim before operations reach the handler, refuses a foreign live claim with the existing DEVICE_IN_USE/DEVICE_CLAIM_LIVE_OWNER error, and releases in the scope's finally. Every other policy performs no claim-store I/O, so session-bound commands keep #1320's non-goal intact.
01fcba5 to
36083a9
Compare
|
Summary
Closes #1799.
Before:
shutdownonly refused a device that had an active session in its own daemon's session store, then calledshutdownTarget(). It never consulted the host-global device claim store, so a daemon in another state directory terminated an emulator another worktree held a verified-live claim on and reported success.boothad the same gap.After: a foreign live claim refuses the command with the existing
DEVICE_IN_USE/DEVICE_CLAIM_LIVE_OWNERerror (retriable: false, daemonlessdevice statusrecovery hint) before any device operation exists.Design
The fix is not a claim check in the two handlers — it makes the class unrepresentable, per #1320's "Command descriptor policy" table:
CommandDescriptorgains a REQUIREDdeviceClaimPolicywith no default, using feat: add cross-worktree device ownership and safe recovery #1320's vocabulary (none|observe|require-owner|transient-exclusive|acquire-session|release-session). TypeScript forces every descriptor to declare one; a completeness/honesty test (mirroring the timeout-policy gate) pins the bounded, diffable set of everything that deviates fromrequire-owner.createRequestRuntimeBindings— the only way any handler obtains device operations — already caches one binding per device key, so admitting the claim as part of creating that binding is both the choke point and the natural per-device deduplication.bindExactDevicedeliberately bypasses the cache, so it admits its own. There is no per-handler call to forget.transient-exclusiveacquires a command-scoped claim after the gateway bind resolves (binding composes the operation catalog and mutates nothing) and before the narrowed projection reaches the handler; the request scope releases it on dispose, after the bindings it guards.stateDir+ owner identity, so a claim taken earlier in the same request (open's) can never lock the daemon out of its own device.acquireTransientDeviceClaimreuses the existing claim implementation (same lock, inspection, write, orphan reconciliation); it recordssession: "transient:<command>"sodevice statusshows an honest owner for a claim that lives milliseconds. No second claim implementation.shutdownself-guard (active session →close --shutdown) is unchanged.Classification (every command, honestly)
acquire-sessionopenrelease-sessionclosetransient-exclusiveboot,shutdown,install,reinstall,install_source,push,prepareobservedevices,capabilities,device,doctor,apps,appstatenonebatch,install-from-source,react-devtools), and local-CLI commandsrequire-ownerhover)Why the sessionless-capable mutations stop where they do. I first classified
keyboard,clipboardandtrigger-app-eventastransient-exclusivetoo. Live verification caught that as dishonest: those commands still declare ADR 0019legacyplatform execution and reach their device through dispatch, not through the request scope's device binding, so the gate never runs for them — a foreignkeyboard statusstill succeeded against a claimed emulator. They arerequire-owneruntil their platform execution migrates todevice-runtime, and the honesty test now requiresdevice-runtimeexecution fortransient-exclusive, so the same dishonest pairing cannot be declared again.Two more deliberate
require-ownerclassifications with reasons, as follow-ups:record— sessionlessrecord startleaves a recording running past the request, and a command-scoped claim released infinallywould free a device whose attributable resource is still owned (feat: add cross-worktree device ownership and safe recovery #1320: "Prevent device claims from becoming free while attributable resources remain owned").runtime(internal port-reverse) — same shape: it establishes durable per-session runtime state that outlives the request.Validation
Regression tests, proven red first
Reverted the production change (
git checkout HEAD --on the touched production files, removeddevice-claim-admission.ts), kept the tests:That
ok: trueon a foreign-claimed device is #1799 exactly. Restored, green:Coverage:
session-boot-shutdown-device-claims.test.ts— two-daemon-shaped (foreign state dir + claim):boot/shutdownrefused with the typed reason,shutdownTarget/bootTargetnever called, the owner's claim untouched, and the transient claim released after both a successful and a failing shutdown.device-claim-admission.test.ts— one table over all six policies proving claim-store I/O happens only fortransient-exclusive, plus foreign-claim refusal, same-daemon coverage, provider-owned skip, and the construction guard: the realcreateRequestExecutionScopeclaims forshutdownand not forsnapshoton the same binding call.device-claim-policy.test.ts— registry-driven completeness, the reviewed non-require-ownersets, and thetransient-exclusive ⇒ device-runtimehonesty invariant.Live device verification (Android,
Pixel_9_Pro_XL→emulator-5554)Pre-fix repro on the reported 0.20.9 build. Daemon A (state dir A) held a live claim; daemon B (different state dir, different cwd) ran:
With this branch (rebooted the AVD, both daemons on the built branch):
boot --platform android --device Pixel_9_Pro_XLfrom B returned the identical refusal. Afterclose --session fixed-ain A, B'sshutdownsucceeded ("OK: killing emulator, bye bye") and left no claim file behind.Sessionless
installfrom B while A held the claim was refused the same way, while the owner's own in-sessioninstallreached the device (it failed onINSTALL_FAILED_TEST_ONLY, an unrelated property of the helper APK I used) — the same-daemon coverage path with no claim-store collision.Cleanup: both daemons stopped, the AVD I booted shut down, no leftover claim file. iOS was not exercised; the seam is platform-neutral and
isLocalDeviceClaimTargetgates it on the admitted runtime owner exactly asopendoes.Gates
pnpm exec tsc --noEmit,pnpm format,pnpm check:affected --run(432 files / 3684 tests, layering + fallow green — fallow reports no issues in the changed files). Wire compat:pnpm check:daemon-wire-compat(151 declarations, 0 changed — this PR adds no daemon wire surface).Tradeoffs and follow-ups
boot/shutdownby design — feat: add cross-worktree device ownership and safe recovery #1320 requires auditing every sessionless device-mutating command, and the descriptor trait is what makes the audit permanent. A large share of the diff is one declaration line per descriptor, matching the existingtimeoutPolicyconvention; the interaction descriptors that were byte-identical now share aTARGETED_TOUCH_INTERACTION_TRAITSbundle beside the existingGENERIC_MUTATING_LINUX_DEVICE_COMMAND_TRAITS, which also removes the clone group the added line would otherwise have created.keyboard,clipboard,trigger-app-eventbecome enforceable once ADR 0019 migrates their platform execution todevice-runtime;recordandruntimeneed a claim whose lifetime follows the durable resource, not the request, before they can leaverequire-owner.install/reinstall/install_source/push/prepareare newlytransient-exclusive— a behavior change for sessionless use against a foreign-claimed device (now refused).installwas live-verified both ways;push/preparewere not.