Repository navigation
fix(daemon): scope a transient device claim to the session that holds it - #3714
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
|
I reviewed d3c79e1 and found two problems that need to be fixed before merge. CI is still pending, with five jobs queued or in progress and no failures yet. Coverage now needs a second ownership rule in device-claim-admission.ts. The routed session must match, and the root of the claim's recorded workspace must also equal the root of the caller's cwd. This disagrees with session routing, where an explicit This change decides which install, reinstall, push, boot and shutdown requests reach a device. The only live evidence is a "parent verified" Android log that is not linked or accessible, and the issue was reported on iOS simulators. Could the fix be smaller? Pass only the routed Not blocking, and fine to take or leave: once the workspace rule is gone, the android-lifecycle fixture in android-lifecycle.test.ts can go back to Open threads that still apply: the Before merge, decide on the workspace rule, either by keying coverage on the routed session alone or by moving fencing into session routing. Then post the live transcript at the new head. |
d3c79e1 to
1c8f3e5
Compare
There was a problem hiding this comment.
All reported issues were addressed across 14 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 3 unresolved issues already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
fdacffa to
f885d74
Compare
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
f885d74 to
30e2b73
Compare
|
Thanks for the update. At 30e2b73 the label guard and the kind-aware coverage fix the earlier copyable-label hole for the transient direction, and the docs and record-decoding threads are addressed. Two problems from the earlier review at d3c79e1 (#3714 (comment)) remain, and one of them has a new route. The claim owner match still ignores the new kind. Coverage still requires the claim's workspace root to equal the caller's cwd root, in https://github.com/callstack/agent-device/blob/30e2b73/packages/daemon/src/resources/device/device-claim-admission.ts#L79. An explicit I could not find a live transcript for the changed admission route, which decides which install, reinstall, push and boot requests reach a device. At the final head, on one daemon with an iOS simulator or Android emulator, please run The Coverage check fails, and I think this PR causes it. The new On the open threads: the inline threads on owner-identity null start times and the legacy-record test claim still apply (the first goes away with the kind check above). Six lower-priority threads are fixed at this head and can be resolved. The Before merge, the claim owner match must compare kind, coverage must key on the routed session alone, and the live transcript must be posted at the final head. |
30e2b73 to
cac1247
Compare
|
Validation at
|
|
Live boundary evidence at
iOS/macOS lanes are still pending at this head. The |
|
Thanks for the update. The code at cac1247 closes most of the earlier threads, but one defect from the last round is still open, and the live evidence is still missing. Fixed since 30e2b73: transient coverage now refuses any claim that is not an open-session claim, and legacy records decode as Still open: The only evidence at head for https://github.com/callstack/agent-device/blob/cac1247/packages/daemon/src/resources/device/device-claim-admission.ts#L76 is a direct module exercise with fake devices, and it bypasses the daemon router, request scope and CLI. Please post raw output at the final head, on one daemon with an iOS simulator or Android emulator. Run Could the fix also remove code? If Not blocking: On the open threads, the copied-label thread at #3714 (comment) still applies through the open route above, and the coverage thread at #3714 (comment) still applies for the same reason. You can resolve #3714 (comment) (the no-cwd SDK case stays cooperative on purpose, and admission.test.ts:401 pins it). You can also resolve these, which are fixed at head: #3714 (comment), #3714 (comment), #3714 (comment), #3714 (comment), #3714 (comment), #3714 (comment), #3714 (comment) and #3714 (comment). I traced the open-route takeover by reading the code and did not run it, and I ran no tests at head. Both Smoke Tests jobs are still queued or running, and Smoke exercises open, install and reinstall through the changed admission route, so please check its log before merge. Before merge, make |
|
Coordinating shared CI evidence (read-only; no action implied for your change): your iOS Smoke lane at |
cac1247 to
76766ea
Compare
|
Thanks for the update. The head is 76766ea. It is a rebase, so the main defect from the earlier review (#3714 (comment)) is still there. The transient coverage check, the help text and the docs are now fixed.
The admission route for Two open review threads still apply to the same code: the transient session-string match and the unstated legacy adoption. The fix above should cover both, so please resolve them with it. These threads no longer apply and can be resolved: no-cwd SDK case (cooperative by design, pinned by Smoke Tests is still running and has no failure yet. It drives open, install and reinstall through this admission route, so its result matters here. The earlier iOS Smoke failure was a daemon startup failure in the preflight step, before any claim code runs, and the same failure hit #3707 and #3735. I ran no tests, and I did not wait for Smoke. Before merge, the settlement fix with its regression test and the live daemon transcript are needed, and Smoke must pass. |
#1809 made install/reinstall and their transient-exclusive siblings covered by any claim this daemon's process held, so a second session in the same daemon could install over a device another session had open. Sessions of one daemon are separate device owners: admission now compares the request's routed session against the held claim, and a foreign session's claim refuses the command with DEVICE_IN_USE the way open already does. Session addresses are copyable, so the covered claim must additionally record the caller's own workspace, canonicalized on both sides through the same scope root session routing hashes; a request without a cwd carries no workspace claim. A release whose unlink fails now abandons the claim instead of leaving it live, so the owner is never locked out of its own device. The versioned help and user docs name the second DEVICE_IN_USE flavor's real owner; main's device-verification notes already attribute that claim to another session, so no guidance budget moves there.
…kspace #3345's coverage rule pairs the routed session with the workspace that took the claim. The raw push request carried a temp cwd, so it routed under another workspace, and the open session's claim no longer covered it. Running it from a directory of the checkout reproduces the real CLI owner shape the client calls above already form, and keeps the relative payload resolution the test is for. Probe evidence at the pre-fix commit: the push refused with DEVICE_CLAIM_LIVE_OWNER naming the checkout workspace.
An in-flight command's transient claim and an open session's claim differed only by the shape of the claim's `session` label, so a request whose routed session string happened to equal `transient:<command>` — reachable today with `--session transient:install`, which the store sanitizes only in directory segments — matched the coverage branch and ran while the first request still held the device (review r4295092657). The record now stores the kind: `transientCommand` names the scoped command (schema v3), and coverage admits only open-session claims; a command-scoped claim is a live owner to every request but its holder, proven by the release token at the admission layer. Legacy v1/v2 records translate the label that encoded the kind, once, in the record decode (tag history: v2 is released in v0.21.28, so live records use it). A session literally named this command's label is refused rather than renew-adopted, so its claim keeps only what its principal grants. Fold in the review's documentation findings: the DEVICE_IN_USE help names in-flight commands as owners a caller cannot close (r4295092708 sibling), commands.md and sessions.md state the workspace condition next to session ownership without implying every caller sends a workspace (r4295092708, r4295092739), and sessions.md names the owner only when the claim is readable, pointing at `device status` otherwise (r4295092723). Shrink the android-lifecycle raw-push fixture under the merge-base length: the file is over the 1,000-line ratchet tripwire and may not grow, so the push payloads are written directly into the claiming workspace by a loop (Coverage job test-file-size ratchet, run 38905771494).
… a kind r4295744998: released v1/v2 recorded a command-scoped claim and an open session literally named `transient:<command>` as the same record, so the label is not a kind proof, and stamping a guessed kind made the schema version alone decide whether a colon-named session's claim covered its commands. Legacy records now decode kindless, which is safe by construction rather than by guess: every consumer of a claim's kind first requires `isClaimOwnedByThisDaemon` (same stateDir, pid, and process start time), a live process writes only v3 records whose kind is stored, and the forged-identity window that reaches a kind decision anyway is pinned by a closest-negative test: a kindless legacy record under this process's identity behaves exactly as a colon-named open session's claim — refused to foreign commands by the label guard, covering for the named session — never as a command claim any in-flight request can adopt or leak. The record test pins the kind assertions directly (ordinary legacy and current claims decode `transientCommand` undefined; the v3 kind comes only from its stored field; malformed kinds fail closed) and the android-lifecycle provenance comment states what the no-cwd client calls actually record: the daemon-side fallback workspace at the routed `default` address, joined explicitly by the pushes whose checkout-child cwd exists for relative payload resolution, not as a workspace-authority test (r4295745008, r4295745014).
Drop the duplicated transient-claim refusal case (its label-guard sibling already proves it) and rewrite comments that narrated mutations and review rounds into statements of the constraint each test pins.
A migrated v1/v2 record decoded with no kind at all, and coverage read an absent transientCommand as proof of an open session. The ownership check in front of that decision matches pid plus start time, and two processes agree on both whenever a start time is unreadable, so a leftover v2 command claim can outlive PID reuse and present as this daemon's own session. Decoded claims now carry a persisted claimOwnerKind. A current writer states open-session or transient-command; a record that predates the field states unstated and can hold and fence a device but can never cover a command. The kind is stored rather than derived from schemaVersion so renewal, abandon, and clear keep writing the current shape — a renewed record that kept its legacy version would be stamped with the migrated device shape and read back as inconsistent.
An acquirer now holds an existing claim only when the record states the very kind of claim the acquirer would write itself — the same transientCommand for transient admission — under the same session, workspace, and daemon identity. An unstated record matches no acquirer, so acquisition never adopts a migrated v1/v2 record, and an open session named after a command label never inherits an in-flight command's owner token. claimHeldDevice passes the acquirer's typed kind and command to resolveExistingClaim, the boundary every acquire route goes through, which retires the label guard in acquireTransientDeviceClaim. Ref: #3714 (comment)
76766ea to
00e4952
Compare
|
Settlement fix + live daemon transcript at The rule, as requested. Regression test (fail-before at
Live proof through the real daemon + CLI (one daemon, isolated Daemon-side NDJSON for both refusals carries Gate at this exact commit: Two notes, honestly bounded: the live legs prove the admission route ( Raw transcript (verbatim /tmp/ad-3345-r11-live-final.log, built head, one daemon, emulator-5554) |
|
Author evidence-provenance audit for #3714 (comment) — the original comment and its raw transcript appendix are preserved verbatim; these are corrections to the summary around them, not new claims of re-execution.
|
|
This PR is ready. The fixes since the earlier review (#3714 (comment)) hold at 00e4952. A session string like transient: no longer adopts an in-flight claim, and an unstated legacy record is never held at acquisition and never covers. Both cubic-dev-ai P1 threads are fixed at this head, so the author can resolve #3714 (comment) (the kind check at device-claim-settlement.ts:220 and the coverage refusal at device-claims.ts:164) and #3714 (comment) (unstated records are excluded by DeviceClaimAcquirerKind and never cover). Not blocking, and you can take or leave these: the transientCommand comparison at https://github.com/callstack/agent-device/blob/00e4952/packages/daemon-contracts/src/device-claim-settlement.ts#L221 cannot fire today, because acquireTransientDeviceClaim already refuses every live transient-command record of this daemon (keep it as type-level completeness or drop it and let the kind check carry the rule), and the test comment at https://github.com/callstack/agent-device/blob/00e4952/packages/daemon/src/resources/device/__tests__/device-claim-settlement.test.ts#L134 says the refusal is pinned at the settlement boundary when it actually comes from the early guard at device-claims.ts:164. The code looks good at 00e4952. I did not run the new tests or a mutation pass. I judged regression validity from a read of the earlier commit and your recorded fail-before. The live-proof claims come from your comment at this head, and I did not inspect the raw transcript artifacts. I also did not check the null-start semantics of ownerIdentityMatches, so I cannot say whether the null-start case of the migrated-record test fails before the fix. Smoke Tests failed in "Preflight iOS runner" because the daemon did not start within 15 s (no lock, no daemon.json). The step before it took about 51 s, so the host looks overloaded. This matches #3782 and is likely unrelated, since the diff only affects code that runs after a request reaches a running daemon. I judged this from the log excerpt and did not see daemon.log. Please re-run Smoke Tests, then merge. |
* origin/main: fix(daemon): scope a transient device claim to the session that holds it (#3714) # Conflicts: # packages/daemon/src/resources/device/__tests__/device-claim-admission.test.ts # packages/daemon/src/server/request-execution-scope.ts
…bals * commit 'd8a9bd40c0': fix(daemon): abandon a transient device claim under the same env that cleared it fix(daemon): scope a transient device claim to the session that holds it (#3714) test: keep the scenario harness's state dir and transport out of reach of its env override test: fail the transport test on an unexpected exit or throw and always remove its workspace
Summary
Closes #3345. Another session’s
installorreinstallrefuses while a session owns the device. Acquisition requires the recorded claim kind to match the acquirer, including its transient command. Legacy kindless records remain fences and cannot donate ownership tokens. Owning-session operations remain valid.Example:
open Lab --session a;install app.apk --session breturnsDEVICE_IN_USE, reasonDEVICE_CLAIM_LIVE_OWNER, ownera;reinstall Lab app.apk --session asucceeds. The common acquisition boundary, admission, records, tests, help and docs span 18 files.Validation
Head
00e495278e, basea61556b8: directpnpm check:affected --run --head 00e495278ec3b3d2b1af932902b8edba115078f1passed withexactCommit: true, unchanged clean fingerprints, 10 executed checks, seven documented skips and 2,333 passing related tests. Receipt:.tmp/validation/run-v6Vzc8/receipt.json.Raw daemon/CLI proof covers foreign refusals, owner reinstall, close and no remaining claim. Author audit corrects provenance and the initial wrong test expectation; final settlement tests pass 15/15. Independent review found no blocking issues, with bound-checkout mutation limitations; both checkouts are restored clean.
Maintainer accepts the fix; readiness label is present. Current-head iOS CI failed public-CLI preflight with a 15-second daemon startup timeout and empty daemon log. Cause and branch participation remain unproven. Other checks passed. The requested retry has not run: campaign policy requires proven infrastructure failure. #3782 tracks the startup class.