Skip to content

refactor: bind session references to stable lifetime entries - #3135

Open
thymikee wants to merge 1 commit into
refactor/session-artifact-pathsfrom
refactor/session-lifetime-entries
Open

thymikee wants to merge 1 commit into
refactor/session-artifact-pathsfrom
refactor/session-lifetime-entries

Conversation

@thymikee

@thymikee thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Session refs now identify a stable, private lifetime entry as well as their stored address and captured record. Lookups remain fresh wrappers; rebuilding a record does not retarget .session. Updates resolve the latest matching record. Retirement checks the lifetime before deleting the record and runtime hints.

Shutdown closes publication admission before its first await. Close carries the ref captured before runtime admission rather than constructing one later. Tests construct real stored refs.

flowchart LR
  Ref[Captured ref: lifetime A, record 1] --> Match{Does entry A still occupy the address?}
  Match -->|Yes| Current[Read or patch current record 2]
  Match -->|No| Refuse[Refuse update; ignore stale retirement]
Loading

Depends on #3134. Ref #3116. Twelve files, 300 gross changed lines. Existing upsert, address-only deletion and reverse-lookup callers migrate in dependent layers; this is the lifetime foundation.

Validation

Tested e180d9f938:

  • 95 focused tests pass; zero skips.
  • Bypassing lifetime comparison: 2 failures/5 passes. Rebuilding from the captured record, reopening admission or allowing occupied publication each causes 1 failure/6 passes. Restored code passes 7/7.
  • Quick checks, Fallow and exact-head affected checks pass: 308 files, 1,990 related tests; zero skips.
  • Independent read-only review has no findings. CI-owned coverage and provider evidence remains pending.

Review in cubic

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.95 MB 4.95 MB +1.1 kB
Package (unpacked) 4.95 MB 4.95 MB +1.1 kB
Package (download) 1.48 MB 1.49 MB +370 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.8 ms 27.8 ms -0.0 ms
CLI --help 88.3 ms 84.0 ms -4.3 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 12 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/daemon/session-lifecycle/internal/session-close.ts
Comment thread src/daemon/session-store.ts
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

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.

@thymikee
thymikee added this pull request to stack #3146 October 3, 2026 08:46
@thymikee
thymikee force-pushed the refactor/session-lifetime-entries branch from e180d9f to a760a6a Compare October 3, 2026 14:41

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant