Skip to content

fix(daemon): end the session and clear its claim when its lease ends - #3480

Merged
thymikee merged 6 commits into
mainfrom
fix/session-ends-with-lease
Oct 14, 2026
Merged

thymikee merged 6 commits into
mainfrom
fix/session-ends-with-lease

Conversation

@V3RON

@V3RON V3RON commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

When a lease was released or expired, the session opened under it stayed alive and kept its device claim. Its owner could not close it, and the next tenant's open failed with DEVICE_IN_USE. Expiry was only acted on by a later request, which often found nothing.

Now, when a lease ends, the daemon tears down the session bound to it under that session's execution locks and clears its claim, even if teardown fails. This covers tenant lease_release, host DELETE /admin/leases/<id>, and expiry (onLeaseExpired, plus a 1 s sweep so no request is needed). In-flight work and human-control holds still keep a lease alive. close and retainOnClose are unchanged. Help text and ADR 0007 updated. 13 files.

Validation

Tested commit 77f8e43. pnpm check:affected --run: all runnable checks passed (incl. layering, fallow, 2211 related tests).

New tests fail without the fix and pass with it: release ends the session; host DELETE ends it; expiry ends it with no request (runtime timer); an unreachable device still loses its claim; release waits for a running command.

Live run on a private-set iOS simulator, two proxy tenants, local daemon:

A open → claims: 1;  B open → DEVICE_IN_USE
lease.release(A) → released:true, claims: 0, leased_session_ended LEASE_RELEASED
B open → Opened, claims: 1;  B close → claims: 0
A open 09:58:00Z; no requests; 10:03:00.9Z leased_session_ended LEASE_EXPIRED, claims: 0
B open → Opened;  B close → claims: 0

Existing daemon and server suites pass (2010 tests). CI pending.

View guided diff Turn on auto-fix

Releasing a lease (tenant lease_release or host DELETE) or letting it
expire left the session opened under it holding its device claim, so the
next tenant's open failed with DEVICE_IN_USE and the owner could no longer
close it. The daemon now tears that session down under its execution locks
and clears the claim even when teardown fails, and sweeps the lease registry
every second so expiry no longer waits for a later request.
@thymikee

Copy link
Copy Markdown
Member

Thanks for this. The fix for ending a session and clearing its claim when the lease ends looks right at 77f8e43, but I found a shutdown race that needs fixing first. The five checks on this commit pass. The PR has merge conflicts with main, because #3431 rewrote daemon-runtime.ts, which holds the new wiring, so CI on the rebased head is the result that counts.

endSessionsForLease checks shuttingDown once, before it queues on the session locks (daemon-runtime.ts#L557). The locked task never checks again. A lease end started by the sweep or the registry is fired with void and is not in inFlightRequests, so shutdown() neither waits for it nor fences it. If SIGTERM, the shutdown command or a fatal error lands while that task runs or waits for the locks, settleIdleExpiredSession can run alongside detachForDaemonShutdown and teardownDaemonSessions on the same session. The runner meant for hand-off can be stopped, the device claim can be cleared twice, and shutdownClaimLedger can record the claim inconsistently. The rule is that no lease-end teardown starts after shutdown begins, and shutdown waits for any that already started. Re-check shuttingDown inside the withSessionLocks task, and track pending lease ends in a set that shutdown awaits before teardownDaemonSessions, as it does for inFlightRequests. Idle expiry already does this with closing() inside the lock pair and sessionIdleExpiry.cancel().

Not blocking, and you can take or leave these: endLeasedSession awaits settleIdleExpiredSession with no time limit, so a teardown that hangs delays the lease_release and host DELETE replies and never reaches clearDeviceClaim, while idle expiry bounds the same settle; the "waits for the command still running" test copies the withSessionLocks lambda from daemon-runtime.ts, so no test covers the shipped lock keys; and expiry now has two teardown routes (cleanupExpiredLeasedSession on the request path, settleIdleExpiredSession on the sweep path), so one function would be easier to reason about.

Simplicity question: could LeaseRegistry fire one onLeaseEnded(lease, reason) event from releaseLease and both consumeExpired* methods, wired once in daemon-runtime? That would drop the endSessionsForLease threading through router deps, the handler chain, the lease handler, the http server and host-lease-http. If endLeaseSessions took executionLocks and a closing() predicate and took the lock pair itself, as settleIdleSessionUnderLock does, it would also fix the race above in one place.

I did not run the tests or reproduce the live run, and I relied on your PR body output for it. I found the shutdown window by reading the code. Please rebase onto main first, then fence lease-end teardown against shutdown and rerun CI on the new head. I did not check how the change rebases onto #3431 or #3430.

* 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)
  ...

# Conflicts:
#	packages/daemon/src/server/http/daemon-runtime-lease-expiry.test.ts
#	packages/daemon/src/server/http/daemon-runtime.ts
#	packages/daemon/src/sessions/__tests__/lease-lifecycle.test.ts
#	packages/daemon/src/sessions/lease-lifecycle.ts
Lease ends now run through one server-side owner that takes the
session's execution lock pair itself, re-checks shutdown inside it, and
tracks every end in flight. Shutdown cancels new ends and drains
running ones before the runner hand-off and session teardown.
@thymikee

Copy link
Copy Markdown
Member

Thanks for the review. New head is 5780eec. I merged main into the branch. The lease wiring now lives in the moved packages/daemon files, and the runtime expiry test moved to src/__tests__/daemon-composition/server/, where the other runtime boot tests live.

  • Shutdown race: fixed. A new server module, daemon-lease-session-end.ts, now owns every lease end. It takes the session's execution lock pair itself and checks shutdown again inside the locks. It also tracks each lease end that is still running. Shutdown cancels new lease ends first. Then it waits for running ones before the runner hand-off and session teardown. New tests: a queued end does not tear down after cancel, and drain waits for a teardown that is still running.
  • Test copied the lock lambda: fixed. The lock-key choice is now in the shipped module, and the tests call that module directly.
  • Smaller design, one place takes the locks with a closing() check: done, as described above. endLeaseSessions moved out of sessions/lease-lifecycle.ts, because the sessions layer may not import the server lock helpers. That file now exports only endLeasedSession.
  • Smaller design, one onLeaseEnded event from LeaseRegistry: declined. The lease_release and host DELETE replies must come after the session has ended and its claim is clear, so that the next tenant's open works immediately. releaseLease runs inside the device mutation, so an event fired there cannot be awaited without a deadlock against a command that holds the session locks.
  • Unbounded teardown: kept as is. The lease end must hold the session locks until teardown finishes, as idle expiry does. If shutdown stopped waiting early, the race above would come back. If the teardown itself had a time limit, the claim could be cleared while a runner is still stopping.
  • Two expiry teardown routes: kept. On the request path, the request already holds the session locks, so it must clean up inline. A queued lease end would wait behind those same locks. Both routes now share endLeasedSession for the claim clear and retire.

Checks run: pnpm typecheck, pnpm lint, pnpm format, pnpm check:layering, pnpm check:fallow, pnpm check:production-exports, and vitest on the lease test files plus src/__tests__/daemon-composition/server/ (66 tests). All pass. I did not do a new live device run on this head.

@thymikee

Copy link
Copy Markdown
Member

The earlier findings from 77f8e43 are fixed, and one new problem remains in 5780eec: a session that a lease ends no longer gets its script written.

endLeasedSession calls the injected teardown and then sessionStore.retire(ref), but it never calls sessionStore.finalizeRepairTeardown(ref) (lease-lifecycle.ts#L105). The runtime injects settleIdleExpiredSession, and that function leaves finalizeRepairTeardown to its caller on purpose. Every other route that ends a session calls it before retire: teardownDaemonSession, idle expiry, and close. This PR adds three new routes that end sessions: tenant lease_release, host DELETE, and the 1 s expiry sweep. Each one now drops a session that recorded a script without writing its .ad file. A repair session gets no healed-script commit and no REPAIR_SESSION_EXPIRED or REPAIR_COMMIT_FAILED tombstone, which breaks ADR 0012 R7. Before this PR, the released or expired session stayed alive, so the later close or shutdown wrote the script. The rule is that every route that ends a session runs finalizeRepairTeardown(ref) once before it retires the session. endLeasedSession is the single finalizer for both lease routes (the sweep/release module and the request-path cleanupExpiredLeasedSession), so please add the call there, after the teardown and claim clear and before retire. Please also add a test that publishes a session with a recorded script, ends its lease, and asserts that the .ad file was written.

Not blocking: the composition test in daemon-runtime-lease-expiry.test.ts mocks createLeaseSessionEnd with no-op cancel and drain functions, so deleting cancel() or await drain() from shutdown leaves every test green, and the mock could record both calls and assert that shutdown runs cancel, then drain, before session teardown; take it or leave it.

I did not run any tests, and I read the missing .ad write from the code without reproducing it with a recorded leased session. The live device run was on 77f8e43. This head moved the wiring into packages/daemon, but settleIdleExpiredSession did not change, so I am not asking for a new device run. I also did not check the layering claim that sessions/ may not import the server lock helpers.

CI is still pending. All 17 lanes were queued after the merge of main and none has failed. Integration, Coverage, and Typecheck & Package exercise the daemon runtime and lease paths this diff changes, so their results decide CI. There are no conflicts. Before merge, endLeasedSession must call finalizeRepairTeardown with the .ad test, and CI must be green on 5780eec or later.

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.21 MB 5.22 MB +1.5 kB
Package (unpacked) 5.21 MB 5.22 MB +1.5 kB
Package (download) 1.57 MB 1.57 MB +390 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 20.5 ms 21.4 ms +0.9 ms
CLI --help 56.6 ms 59.2 ms +2.7 ms

* origin/main:
  feat(remote): name the held lease on upload, artifact download and diagnostics requests (#3594)
  fix(android): record the launched intent owner from the device (#3600)
  fix(selectors): type rect-requiring targets as addressable (#3608)
  feat(snapshot): interpret iOS node facts once at producer seams (#3592)
  refactor(gates): own the client-only replay rule as daemon-process reachability (R82) (#3604)
* origin/main:
  feat(leasing): proxy CLI names each new lease, daemon keeps a caller-chosen lease id (#3479)
…nd into the execution scope

endLeasedSession now runs finalizeRepairTeardown before retire, so lease
release, host DELETE and the expiry sweep write the session's .ad like every
other session-ending route (ADR 0012 R7).

createLeaseSessionEnd moves into request-execution-scope.ts, which already
owns the lease registry's execution locks and evaluates the same modules, so
the daemon's eager closure does not grow and no new package entry is added.

The composition test records cancel and drain and asserts shutdown runs both
before it tears sessions down.
@thymikee

Copy link
Copy Markdown
Member

New head is 85220c8. Main is merged in (562e798, including #3479 and #3594; no conflicts).

  • Lease end now writes the script. endLeasedSession calls sessionStore.finalizeRepairTeardown(ref) after teardown and the claim clear, before retire. Lease release, host DELETE and the expiry sweep now write the session's .ad like the other session-ending routes (ADR 0012 R7). New test end publishes the recorded script of the session it ends publishes a leased session with a recorded step and an explicit script path, ends its lease, and reads the .ad. It fails without the fix (ENOENT).
  • Eager-closure budget. createLeaseSessionEnd is now in packages/daemon/src/server/request-execution-scope.ts. That module already owns getLeaseRegistryExecutionLocks and already imports lease-lifecycle, request-binding and request-execution-locks. daemon-lease-session-end.ts and its ./testing/server/daemon-lease-session-end export are removed, so src/daemon.ts does not grow and there is no new facade entry. No approval row and no baseline change. The unit test moved to server/__tests__/request-execution-scope-lease-session-end.test.ts.
  • Shutdown order test. daemon-runtime-lease-expiry.test.ts now mocks createLeaseSessionEnd through testing/request-execution-scope and records cancel and drain. A new test records session teardown (through SessionStore.closeAdmission) and asserts the order cancel, drain, teardown. It fails if drain moves after teardownDaemonSessions.

Gates run on this head: scripts/__tests__/eager-closure-budgets.test.ts (1036 passed), targeted vitest for the lease, session, server/http and daemon-composition/server tests (163 files, 1010 tests passed), pnpm lint, pnpm typecheck, oxfmt check, pnpm check:layering, pnpm check:production-exports and pnpm check:fallow, all green. I did not run the full unit suite or device lanes locally.

@thymikee

Copy link
Copy Markdown
Member

This PR is ready on the code. The follow-up commit 85220c8 fixes what the earlier review raised: the daemon now writes the script when a lease ends, and the new test fails without that fix because the old endLeasedSession had no write path.

Not blocking: the lease-end fan-out in packages/daemon/src/server/request-execution-scope.ts runs on the daemon lifecycle, not inside a request, and it sits in a module that is now 716 lines, while its sibling createSessionIdleExpiry stays in server/http/daemon-session-idle-expiry.ts, so a later split could give both lock-scoped session-ending sweepers one shared module, and you can take or leave that.

CI is still pending on 85220c8. Smoke Tests, Typecheck & Package and Coverage are queued and none has failed. Typecheck & Package and Coverage exercise the daemon runtime and the lease-lifecycle and request-execution-scope modules this change touches, so their results decide CI.

I ran no tests. I did not check the eager-closure budget or the layering claim. The new .ad test covers an ordinary armed authoring session, not the repair tombstone (REPAIR_SESSION_EXPIRED / REPAIR_COMMIT_FAILED). That path runs through finalizeRepairTeardown, which is shared and already covered on the other routes. The live device run is from 77f8e43. This change only touches how the daemon writes the script and where the fan-out module lives, so I am not asking for a new run.

Before merge, CI must be green on 85220c8 and the PR must leave draft.

@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 15:17
@thymikee
thymikee merged commit 314ab6d into main Oct 14, 2026
17 of 18 checks passed
@thymikee
thymikee deleted the fix/session-ends-with-lease branch October 14, 2026 15:17
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-14 15:19 UTC

thymikee added a commit that referenced this pull request Oct 14, 2026
* origin/main:
  fix(daemon): end the session and clear its claim when its lease ends (#3480)
thymikee added a commit that referenced this pull request Oct 14, 2026
* origin/main:
  fix(daemon): end the session and clear its claim when its lease ends (#3480)
thymikee added a commit that referenced this pull request Oct 14, 2026
* origin/main:
  fix(daemon): end the session and clear its claim when its lease ends (#3480)
thymikee added a commit that referenced this pull request Oct 14, 2026
…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.
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