refactor: bind session resources to stable lifetimes

[thymikee]

Summary

Capture session lifetimes with SessionRef and carry them through resource adoption/disposal. Join admitted dispatches after socket closure, then close admission atomically with the teardown snapshot. Fence log clearing and recording completion against successor sessions. Share the capture-binding policy between production and transition controls.

62 files; part of #3116. Base: main.

Validation

Commit 9252d8251fe1c2d95250f7b84e463db9a87e3e1c. The exact-head pnpm check:affected --base 294dc7d024 --run gate passed. Test Files 1633 passed (1633); Tests 13343 passed | 1 skipped (13344); Test Files 1 passed (1); Tests 12 passed (12).

Fresh GitHub CI and review remain separate. Required native confirmation remains pending; physical recording-health proof is blocked by Xcode signing. The user handles merges. Dependencies, regression evidence and remaining checks.

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	4.97 MB	4.97 MB	+3.9 kB
Package (unpacked)	4.97 MB	4.97 MB	+3.9 kB
Package (download)	1.49 MB	1.49 MB	+1.7 kB

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	29.4 ms	29.5 ms	+0.1 ms
CLI --help	86.2 ms	83.6 ms	-2.6 ms

[cubic-dev-ai]

All reported issues were addressed across 12 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[thymikee]

At e180d9f, one change is needed before merge. closeAdmission() runs too early in shutdown, so an open that finishes during the drain now fails and leaves an abandoned device claim.

closeAdmission() at daemon-runtime.ts:751 runs before closeDaemonServers(). server.close() waits up to 5 s for live connections, and in-flight requests are meant to publish their sessions before teardownDaemonSessions() snapshots them with sessionStore.toArray(). Take an open of a new session that is in flight when SIGTERM, a stop or a takeover arrives. It reaches sessionStore.set(sessionName, nextSession) at session-open-execution.ts:329 (or the provisional set at :356). With no entry yet, set() calls publish(), and publish() now throws daemon_shutting_down. The open's catch then calls rollbackNewSessionClaim with mayHaveStarted=true, which abandons the claim and emits device_claim_open_effects_unconfirmed. The app or runner it launched never goes through teardownDaemonSession, so there is no runner handoff, no lease finalization and no claim ledger release. Before this PR the same open published during the drain and shutdown tore it down cleanly. A record-only record session or any other first publish in the drain window fails the same way. The rule the code must satisfy is that admission closes at the moment shutdown takes its teardown snapshot, so every publication either lands in that snapshot or is refused. Could you move closeAdmission() into teardownDaemonSessions right before sessionStore.toArray()? Please add a daemon-runtime-level test where a publish during the closeDaemonServers drain is torn down with its claim released, and a publish after the snapshot is refused with daemon_shutting_down. That test should fail if the call stays at its current line.

Not blocking, take or leave: the shutdown test in session-store-lifetime.test.ts:95 calls store.closeAdmission() directly, so deleting the daemon-runtime wiring or the requireCurrent throw in session-close-lifecycle-teardown.ts:69 keeps every test green, and covering both through shutdown() with a pending publish and handleSessionCloseCommands with a retired lifetime (asserting session_lifetime_ended) would close that gap; also, update() in session-store.ts:114 swaps entry.current for a new object while production code mutates session objects in place (noteSessionActivity, recordAction, request-execution-scope.ts:490-493, internal-observation), so before #3140 and later adopt update() it should become the only writer or mutate in place.

Could this land together with #3140? publish(ref), update, retire and resolveCurrent have no production caller at this head, so the only live effects are the half-migrated close path (two record sources, address-based delete) and the early shutdown admission. Landing the store primitives with the close migration would give one record and retire(ref) in one reviewable unit. I looked for an existing owner of lifetime identity (the request lock, ref-frame expiry, the lease registry) and found none that binds an address to a record lifetime, so the entry-identity token itself looks justified. Merge order matters: #3135 should not reach main without #3140.

On checks, Smoke Tests failed in pnpm clean:daemon and prepare with daemon_startup_failed ("did not establish a reachable owner"). That looks unrelated: the only daemon-runtime change here is a synchronous flag set inside the shutdown closure, no session exists during startup, and the diff does not touch registration, the lock or startup readiness. I judged this from the code route and did not read the daemon log, so a rerun should confirm it. I also did not run the tests or the mutation checks, and I did not check whether a provider lease taken by an open refused during shutdown is released by expiredProviderLeaseReleaser.

The close teardown route changed, since teardown now re-resolves the record and can throw session_lifetime_ended, and Smoke never reached open or close. Please get a green Smoke run, or run a live iOS simulator open <app> then close at the new head. Close should return "Closed: " and session list should no longer show the address.

Before merge, the teardown snapshot fix and its shutdown-route test need to land, and Smoke needs a green run that reaches open and close.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-04 14:49 UTC

[thymikee]

Thanks for the update. The 5aa8134 head still has one defect from the earlier review (#3135 (comment)). The daemon_startup_failed Smoke failure no longer stands: all 19 checks pass, including four Smoke lanes, and no conflicts are known.

In shutdown() (https://github.com/callstack/agent-device/blob/5aa8134/src/daemon/server/daemon-runtime.ts#L758), sessionStore.closeAdmission() runs before closeDaemonServers(), and server.close() then drains in-flight requests for up to 5 s. Take an open of a new address that is in flight during that drain. It reaches sessionStore.set at session-open-execution.ts:329 (or the provisional set at :356). With no entry yet, set() falls through to publish() (session-store.ts:157), and publish() throws daemon_shutting_down after the app or runner has launched. The open then rolls back with mayHaveStarted=true and abandons the device claim. The same early flag also refuses capture adoption (assertAdmissionOpen in session-capture-binding.ts:19) for a record start or logs start on an existing session during the drain, before teardown takes its snapshot. So a SIGTERM, stop or takeover during an open leaves the claim abandoned and the launched app or runner running, with no runner handoff, lease finalization or claim release. Before this PR that open was published and torn down cleanly.

The rule: admission closes atomically with the teardown snapshot, so every publish or adopt either lands in the set that teardownDaemonSessions tears down or is refused. Please move sessionStore.closeAdmission() into teardownDaemonSessions, just before sessionStore.listRefs() (about :490). Then add a test through shutdown() at the daemon-runtime level. It should show that (a) a publish released during the closeDaemonServers drain is torn down and its claim is released, and (b) a publish after the snapshot is refused with daemon_shutting_down. The test must fail with the call at its current line. Once that lands and checks stay green, this is ready from my side.

Not blocking: makeCaptureSessionBinding in packages/capture-kit/src/durable-capture/session-binding.fixtures.ts:38 is a near-copy of bindSessionCapture (src/daemon/session-capture-binding.ts:9-60), so the capture-kit transition tests run against a second copy of the rule. You could host the generic binding in capture-kit behind a small store port (resolveCurrent, update, assertAdmissionOpen, resolveSessionDir), wrap it in the daemon, and let the fixture supply only a fake store. You can take or leave this.

Is there a smaller owner for the new bindSessionCapture seam? I found none. It replaces sessionSlot.replace and DurableCaptureSessionStore rather than sitting beside them, so it looks justified, and the fixture move above is the only shrink I see.

I did not run the tests, check:affected or mutation checks. I judged the capture-binding tests by reading them. I also did not confirm which Smoke lanes reach open then close on a live device, so I took the green status as meeting the earlier evidence ask. I did not trace whether a provider lease from an open refused during shutdown is released by expiredProviderLeaseReleaser.

On the open inline threads: the thread on session-close.ts:341 still applies as a note (#3135 (comment)). It is not reachable today, and the fix lives in the stacked #3140. The ref.session re-read thread does not apply: the divergence predates this PR, and the fields read are not written by capture updates, so please resolve it (#3135 (comment)).

[thymikee]

The iOS failure at 2392e8e is run 37170774472: testSynthesizedReplacementPacesAnAppOwnedFieldAtItsAcknowledgeWindow observed 11 edits over 366 ms against the 400 ms bound (closest pair 6 ms); 77 of 78 selected XCTest controls passed. The pacing implementation and assertion are unchanged from main for this PR. This does not establish a session-lifetime regression, and the bound has not been weakened. The new recording-binding ownership correction has separate exact-head local validation and fresh CI; this historical native failure remains attributed to 2392e8e.

[cubic-dev-ai]

All reported issues were addressed across 58 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 15 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[thymikee]

Updated review fixes are included at 9252d8251fe1c2d95250f7b84e463db9a87e3e1c. The exact-head affected gate passed: Test Files 1633 passed (1633); Tests 13343 passed | 1 skipped (13344); Test Files 1 passed (1); Tests 12 passed (12).

All eight actionable inline findings are fixed and resolved: join admitted dispatches before retirement, close admission with the teardown snapshot, fence log clearing and capture completion, retain the latest observed slot when clearing mismatches, protect fixture cleanup and assert refusal outside the swallowed-error boundary. Shared capture binding lives behind a dedicated package facet; daemon and fixture adapters exercise the same policy. Four fresh controls were red before their fixes; all 26 focused binding/transition/shutdown controls pass. All 759 eager-closure controls and twelve binding controls pass after the dedicated-facet correction, without relaxed budgets.

Current dependencies, exact-head gates and evidence still required. Fresh GitHub/native evidence and approval remain separate; pending or canceled runs are not passing evidence. The user handles all merges.

[thymikee]

This PR is ready. At 9252d82 the earlier blocking finding from #3135 (comment) is fixed, and all 19 checks pass, so there is nothing to attribute. There are no conflicts. Nothing else must happen before merge.

Not blocking: await Promise.allSettled(inFlightRequests) at https://github.com/callstack/agent-device/blob/9252d82/src/daemon/server/daemon-runtime.ts#L789 has no deadline, so a dispatch that ignores request cancellation could keep shutdown from reaching detach and teardown. A bounded join would keep shutdown live, for example Promise.race with the existing DAEMON_SHUTDOWN_TIMEOUT_MS budget plus a diagnostic naming the unsettled request ids. Admission already closes at the snapshot, so a late publish would be refused. You can take this or leave it. I did not check which handlers ignore cancellation, so I do not know if it can happen in practice.

Of the review threads, the P2 and P3 threads on clearStoppedSessionAppLog, perf recording, the record stop action, the layering kill criterion, the shutdown test setup, the finalize refusal assertion and the session-binding fence are fixed at this commit: #3135 (comment), #3135 (comment), #3135 (comment), #3135 (comment), #3135 (comment), #3135 (comment) and #3135 (comment). The P1 thread on the in-flight join is also fixed, except for the missing time bound noted above: #3135 (comment). Two threads are benign and can be resolved: the ref.session divergence predates this PR and capture updates do not write those fields (#3135 (comment)), and session-close not calling retire(ref) is unreachable today and owned by the stacked #3140 (#3135 (comment)). No P1 or P2 thread still applies, and no lower-priority threads remain.

I did not run tests, check:affected or mutation checks. I judged each new test by reading the old code against it. I reviewed only the logical change after the rebase, and treated the upstream files as out of scope.

[thymikee]

I kept the dispatch join before runner handoff and session disposal. Racing the join against a timer and then continuing teardown would let unfinished dispatches overlap resource cleanup; closing session admission does not stop native effects already dispatched. The five-second constant in server-shutdown.ts bounds connection closure, not completion of device work.

Explicit stop/retirement already has a bounded TERM/KILL policy and process-birth verification. I have not added a separate timed teardown path or claimed every handler honors cancellation. A bounded join needs an explicit terminal policy for unfinished operations first; this remains a non-blocking liveness concern rather than a reason to weaken the join invariant. That distinction also matches the timeout-ownership gap in the architecture audit.

[thymikee]

Thanks, that reasoning holds. A timed join without a terminal policy for unfinished operations would let device work overlap cleanup, which is worse than a slow shutdown. Keeping the join as is makes sense; the liveness gap belongs with the timeout-ownership follow-up. The verdict at 9252d82 is unchanged.