Skip to content

feat(cli): release the proxy lease after close - #3477

Merged
thymikee merged 7 commits into
mainfrom
feat/cli-release-lease-after-close
Oct 14, 2026
Merged

thymikee merged 7 commits into
mainfrom
feat/cli-release-lease-after-close

Conversation

@V3RON

@V3RON V3RON commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

In proxy mode the daemon ends a session's lease on close, but no release goes over the wire, so a gateway in front of the daemon never learns the lease ended. The CLI also kept the old lease id, so the next open heartbeated it, got LEASE_NOT_FOUND, and allocated a new lease.

Now, after a successful proxy close, the CLI sends lease_release for its stored lease and removes the lease from its local connection state. A failed or "not found" release does not fail close. Nothing changes for leases the daemon keeps on close (retainOnClose, now recorded from allocate/heartbeat), host-allocated macos-app leases, non-proxy connections, or local mode. 6 files; the remote-proxy docs gain one sentence.

Validation

Tested commit 6e8d5d128: pnpm check:affected --run passed (format, lint, typecheck, layering, fallow, build, docs; 44 related test files, 441 tests).

New tests in cli-close.test.ts. The two release tests fail without the fix. The retain, macos-app, and non-proxy tests each fail when their guard is removed.

Live run: built CLI → logging relay → agent-device proxy → local daemon, using a private iOS simulator:

open   lease.allocate → e45eefd6…
close  command=close; lease.release e45eefd6… → {"released":false}; local lease: none
open   lease.allocate → fe83d4d1… (no heartbeat)

On origin/main the same flow sends no release after close, and the next open gets lease.heartbeat → 401 LEASE_NOT_FOUND and then allocates again.

With the change, disconnect on an open session still sends close then lease.release. Local-mode open/close works as before.

View guided diff Turn on auto-fix

In proxy mode the daemon ends a closed session's lease itself, so a host
fronting the daemon never saw a release, and the CLI kept the stale lease
id: the next open heartbeated it, got LEASE_NOT_FOUND and reallocated.

After a successful close the CLI now sends lease_release for its stored
proxy lease (any failure is ignored) and drops the lease from its local
connection state. Leases the daemon retains on close, host-allocated
macos-app leases and non-proxy connections are left alone.
@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.22 MB 5.22 MB +2.3 kB
Package (unpacked) 5.22 MB 5.22 MB +2.3 kB
Package (download) 1.57 MB 1.57 MB +840 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.1 ms 26.9 ms +0.8 ms
CLI --help 75.8 ms 79.6 ms +3.8 ms

@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://callstack.github.io/agent-device/pr-preview/pr-3477/

Built to branch gh-pages at 2026-10-14 17:02 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@thymikee

Copy link
Copy Markdown
Member

The code needs a design decision and live evidence before it is ready to merge. At 6e8d5d1 I found no blocking defect, but the daemon should own the lease outcome instead of a cached CLI flag. I did not re-run the live relay run or the tests, and the live claim comes from the PR body. Smoke Tests is still queued, not failing, and there are no conflicts that I know of. The diff touches the top-level CLI close route that smoke flows run, but for local close releaseProxyLeaseAfterClose returns at once because flags.remoteConfig is unset.

Could the daemon's close success response (buildCloseSuccessResponse in src/daemon/session-lifecycle/internal/session-close.ts) report the lease outcome (released, retained, or none, plus the leaseId)? The CLI would then clear its stored leaseId when the response says released, on every route that runs close, batch included. A gateway that must see lease ends would read that outcome from the close reply it already relays, or the daemon would emit the release itself. Either way, leaseRetainOnClose, its validator, the leaseStateMatches comparison and the command-name hook in src/cli.ts would go. This also covers the cases the CLI-side check misses today. Examples are a batch with a close step, a close from MCP, Node or the AI SDK, a same-client allocate with retainOnClose after the last heartbeat, and a close whose platform teardown fails after the lease is released. What would have to change first is a decision on whether the gateway's lease view is a daemon or proxy contract. An ADR 0007 note would define the signal once on the daemon response.

Not blocking, and you can take or leave these: the skip decision at https://github.com/callstack/agent-device/blob/6e8d5d1/src/cli/commands/connection.ts#L356 reads a CLI-side copy of retain state from the last heartbeat, while the daemon decides at close time, so a stale copy can end a lease the daemon just retained. The hook at https://github.com/callstack/agent-device/blob/6e8d5d1/src/cli.ts#L188 keys on the top-level command name, so batch and failed-close paths skip the release and the local clear. The bare catch at https://github.com/callstack/agent-device/blob/6e8d5d1/src/cli/commands/connection.ts#L360 swallows auth and network errors with no diagnostic, so a diagnostic with the normalized error code would help.

#3479 edits connection-runtime.ts and the same remote-proxy.md paragraph, so expect a conflict there. I read that from its description and file list, not from merged code. #3480 makes lease end tear down the session, so the two can land in either order, but it does not remove the need for a daemon-owned outcome. Whether any real host upgrades a CLI-allocated lease to retainOnClose mid-session depends on the gateway outside this repo, which I cannot see.

Before merge, we need a maintainer decision on whether the close response should own the lease outcome, and Smoke Tests needs to finish.

* origin/main: (83 commits)
  feat(ios): interpret React Native accessibility value semantics (#3589)
  fix(android): reinstall and reselect the test IME helper after it disappears (#3606)
  feat(snapshot): carry provider keyboard observations through acquisition (#3593)
  chore(web): bump managed agent-browser backend to 0.39.0 (#3591)
  fix(fold): report a dispatched pose as unverified when devicectl has no hinge-angle stream (#3587)
  feat(android): expose system-bar membership on snapshot nodes (#3585)
  fix(android): align ADB and Appium field values (#3584)
  test(closure): read committed JSON modules on the merge-base side (#3602)
  fix(ci): preserve manual Android suite timing (#3554)
  refactor(daemon): make the layer folders the declaration (PR 10) (#3520)
  refactor(move): extract the daemon server into @agent-device/daemon (11b) (#3546)
  refactor(daemon): package-local runtime fakes for the remaining daemon ports [11a-4] (#3544)
  refactor(daemon): cut daemon test edges into root data fixtures and fakes [11a-3] (#3517)
  refactor(11a-2): package-local fakes at the daemon ports; root-own command-surface composition tests (#3519)
  test(replay): verify Settings queries and bound scrolling (#3560)
  feat(snapshot): measure Android Appium viewport (#3586)
  fix(ios): verify a fill whose unidentified field moved while it kept focus (#3596)
  refactor(coverage): one fallow config with per-function CRAP ceilings (#3588)
  test: bound CLI promotion cases and align provider budget (#3590)
  fix(metro): report what the dev-server stop proved instead of assuming success (#3575)
  ...
Replace the bare catch with a warn diagnostic that carries the normalized
error code and reason, so auth and network failures are visible.
Session close now says what it did with the session's daemon lease:
`lease: { outcome: 'released' | 'retained', leaseId }` or
`{ outcome: 'none' }`. A close that fails after the release or retention
carries the same field in its error details. The shape is declared once
as SessionCloseLeaseOutcome in contracts, and the Node client returns it
from sessions.close() and apps.close(). ADR 0007 records that the close
reply owns the lease outcome.
The CLI no longer caches retainOnClose or sends lease_release after
close. The daemon already ends the lease on close and reports it, so the
CLI transport reads every reply of a remote connection and forgets the
stored proxy lease when a close reply, a failed close, or a batch close
step reports it released. This covers batch and failed-close routes that
the top-level close hook missed.
@thymikee

Copy link
Copy Markdown
Member

New head is a7dc1d6. Main was merged in at 74921c8 with no conflicts.

  • Bare catch: fixed in 80c995f with a warn diagnostic. That code is now gone, because the CLI no longer sends a release after close.
  • The daemon close response now owns the lease outcome. close returns lease: { outcome: 'released' | 'retained', leaseId } or { outcome: 'none' }. The shape is SessionCloseLeaseOutcome in contracts. A close that fails after the release puts the same field in its error details, and a batch close step has it in its step result. The Node client returns it from sessions.close() and apps.close().
  • The daemon ends the lease on close, as before. The CLI makes no lease_release call after close. It reads each reply on a remote connection and forgets the stored proxy lease id when the reply says released. This works for top-level close, a batch with a close step, and a failed close.
  • Removed from the CLI: the cached leaseRetainOnClose flag, its validator, the leaseStateMatches comparison, the close hook in cli.ts, and the after-close release with its diagnostic.
  • ADR 0007 has a short paragraph: the close reply owns the lease outcome, clients never release after close, and a gateway reads the lease end from the close reply it relays.
  • Docs: remote-proxy.md and client-api.md describe the lease field. CLI help did not change.

Checks run: pnpm build, pnpm typecheck, pnpm lint, pnpm check:layering, pnpm check:production-exports, the wire-compat unit test and pnpm check:daemon-wire-compat (no wire declaration changed, so no ack), pnpm format (only my files changed), and vitest on 87 close, lease and remote-connection test files (1063 passed). The new tests fail when the change is reverted. I did not do a live run on the new head.

@thymikee

Copy link
Copy Markdown
Member

The close reply now carries the lease outcome on the direct route, and the lease_release round trip from 6e8d5d1 is gone. One gap remains on replay and test routes, and the evidence is still missing on a7dc1d6.

closeReplayTestSession runs the close with the caller's lease, but it returns void and drops the reply's lease field. A replay script's close step goes through the replay invoke, so its reply data never reaches the outer response. For proxy agent-device test or replay of a script that closes, the daemon releases the lease, yet no reply to the CLI or a relaying gateway says outcome:'released'. Nothing changes from main there: the CLI keeps the dead lease id, the next open gets heartbeat LEASE_NOT_FOUND and allocates again, and ADR 0007 and remote-proxy.md claim the close reply always carries the field. The rule should be that every daemon reply to a request that ended a session lease reports that outcome where the CLI and gateway already read it. Test cleanup, replay close steps and batch steps are all such requests. Could the request scope record each close's outcome and attach it once at response level through the shared response builder, for example as endedLeases? closeLeaseFields could then drop its batch-envelope parsing. The smaller option is to make ADR 0007 and remote-proxy.md list the routes that carry the field and exclude replay and test.

The PR body's live run is from 6e8d5d1, which had the old lease_release design. This head changes the daemon close reply and the CLI reply path in closed-lease.ts, so please run the built CLI through a logging relay into agent-device proxy and a local daemon on this head. Please show: (1) open then lease.allocate; (2) close --json with the reply showing lease {outcome:'released', leaseId} and no lease.release after it; (3) the stored connection state without leaseId; (4) the next open doing lease.allocate with no heartbeat. Then repeat (2) to (4) with a batch whose steps include close.

I reviewed the 18-file logical diff from cfa9653 to a7dc1d6 and did not run tests, check:daemon-wire-compat or a live proxy. The test results I know of come from your comment. I traced closeReplayTestSession passing req.meta, but I did not confirm how replay builds the wire request for each step. Whether a real gateway reads lease from the relayed close reply depends on code outside this repo.

CI is still pending: two Smoke Tests jobs are queued and Compatibility & Provenance is running, and none has failed. The diff touches what both check, since local close now gets lease:{outcome:'none'} in every reply, and the close reply gains a field. The cli.ts wrapper returns early when remoteConfig is unset, so the local CLI path is unchanged. The PR also has merge conflicts with main. Before merge it needs the live proxy run on this head and a fix or documented exclusion for the replay and test close routes.

…e-after-close

Keep both ADR 0007 rules: #3480's lease end ends the session, and the close
reply owns the lease outcome. A close that finds its lease already inactive
reports released.
Every daemon reply to a request that ended a session lease now lists it in
endedLeases at response level (data on success, error.details on failure).
The outermost request owns the ledger; batch steps, replay and test script
closes, and test session cleanup record on it, so proxy replay/test of a
script that closes tells the CLI and a relaying gateway the lease ended.

The per-close lease field and SessionCloseLeaseOutcome are gone, and the CLI
reads only the response-level field.
@thymikee

Copy link
Copy Markdown
Member

New head is e38482f. It fixes the replay and test gap. Each daemon reply to a request that ended a session lease now lists that lease once at response level, as endedLeases: [{ leaseId, outcome: 'released' }]. It is in data on success and in error.details on failure. The outermost request keeps the list. Batch steps, replay and test script closes, and the session cleanup that test runs after each script all add to it, so a step reply never repeats it. The per-close lease field and SessionCloseLeaseOutcome are removed. The CLI reads only the response-level field, so the batch results/partialResults parsing is gone. ADR 0007 and remote-proxy.md now describe endedLeases and list the requests that carry it. client-api.md is the same as on main again, because sessions.close() and apps.close() no longer return lease.

Live run on e38482f. The setup was: built CLI → a small logging relay → agent-device proxy → local daemon. The client and the daemon used separate state dirs. The device was a new iPhone 17 Pro simulator (iOS 26.2) in a private device set, booted before the run and deleted after it. Each open was open com.apple.Preferences --platform ios --ios-simulator-device-set <set>. The relay logged each JSON-RPC method and the endedLeases field of each reply:

open    lease.allocate 021991a0… → command=open ok
close   command=close → endedLeases=[{"leaseId":"021991a0…","outcome":"released"}]
        (no lease.release after it; stored leaseId: none)
open    lease.allocate e28841ba… → command=open ok   (no heartbeat)

batch   [snapshot, close]: lease.heartbeat e28841ba… → command=batch
        → endedLeases=[{"leaseId":"e28841ba…","outcome":"released"}]
        (no lease.release after it; stored leaseId: none)
open    lease.allocate 500c2b84… → command=open ok   (no heartbeat)

replay  open-close.ad (open "com.apple.Preferences", close):
        lease.heartbeat 500c2b84… → command=replay
        → endedLeases=[{"leaseId":"500c2b84…","outcome":"released"}]
        (no lease.release after it; stored leaseId: none)
open    lease.allocate cda2d532… → command=open ok   (no heartbeat)

The close --json and batch --json output that the CLI prints does not show endedLeases. The relay log above shows the raw daemon reply that has it. The replay --json output shows it.

I did not do a live run for test. In proxy mode the CLI needs an open session that holds the lease before it runs test, and that session holds the only device. The test session's open then fails with "Device is already in use". The test path is covered by unit tests: a handler test for session cleanup, and a router test for a replay script that closes.

Checks on e38482f: pnpm check:affected --run passed (488 test files, 3631 tests). It includes format, lint, typecheck, layering, fallow, build and docs. These also passed: vitest on 262 lease, close, replay, batch and connection test files (1994 tests), pnpm vitest run test/wire-compat, and pnpm check:daemon-wire-compat. No wire declaration changed, so there is no ack. The new tests fail when their part of the fix is reverted: the ledger recording in close, the batch hand-down in the router, and the hand-down in test cleanup.

@thymikee

Copy link
Copy Markdown
Member

This is ready for review at e38482f. Every reply to a request that ended a session lease now lists it once as endedLeases, so the replay and test close routes from the earlier review no longer drop the outcome, and the CLI reads only that field. The live proxy run on e38482f (close, batch and replay through a logging relay into agent-device proxy) covers the missing run, and the handler test covers test, which cannot open a second session on the leased device through the proxy.

Not blocking: a test script's own close step gets the ledger only because the replay dispatch copies the whole private request state, and no router-level test pins that. One router test case with a script that closes, next to the replay case, would lock it in.

There are no conflicts. CI is still queued on e38482f. Because the change wraps the outermost request handler, check any Smoke, Integration or Compatibility failure against request-router.ts first.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 14, 2026
@thymikee
thymikee marked this pull request as ready for review October 14, 2026 16:32
@thymikee
thymikee merged commit 3cefb33 into main Oct 14, 2026
17 of 19 checks passed
@thymikee
thymikee deleted the feat/cli-release-lease-after-close branch October 14, 2026 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants