Skip to content

fix: finish session captures through lifetime bindings - #3137

Open
thymikee wants to merge 2 commits into
fix/session-capture-bindingsfrom
fix/session-capture-finish
Open

thymikee wants to merge 2 commits into
fix/session-capture-bindingsfrom
fix/session-capture-finish

Conversation

@thymikee

@thymikee thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Capture finishing and forced cleanup now use a lifetime-bound session binding. The coordinator keeps the captured handle/fence across native awaits; clearing checks that the same lifetime and resource still occupy the slot. A rebuilt record keeps its unrelated changes.

Capture-kit no longer accepts a whole session record or sessionSlot.replace projection. Daemon field owners provide explicit slot updates. Close, shutdown, audio, perf, app-log and record callers use the shared finish operations; shutdown and record-only removal retire captured lifetimes.

sequenceDiagram
  participant F as Finish operation
  participant N as Captured native handle
  participant S as Session binding
  F->>S: Read current owned resource
  F->>N: Finish captured handle
  Note over S: Record may rebuild or address may be reused
  N-->>F: Completion
  F->>S: Clear only matching lifetime and handle/fence
Loading

Depends on #3136. Ref #3116. Thirty-six files, 661 gross changed lines. Recording/journal and remaining session writers migrate next.

Validation

Tested 1376a9e26c: quick checks, Fallow and exact-head affected checks pass, including 277 related files / 1,726 tests. All 73 focused capture/teardown tests pass.

Removing clear, bypassing handle comparison or using address-only clearance each fails a held-finish control; restored code passes 19/19. R68 rejects former package writers and allows the daemon owners. Independent read-only review has no findings. CI-owned coverage/provider and live device evidence remain 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.94 MB 4.94 MB -1.1 kB
Package (unpacked) 4.94 MB 4.94 MB -1.1 kB
Package (download) 1.48 MB 1.48 MB -120 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 21.5 ms 21.8 ms +0.3 ms
CLI --help 63.7 ms 62.7 ms -1.0 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 36 files

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

Re-trigger cubic

Comment thread src/daemon/request-execution-scope.ts
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

At 1376a9e, I found two problems that need fixing before this can merge. The Coverage check fails, and I think the failure comes from this diff.

cleanupExpiredLeasedSession passes session.name to teardownSession. That is the public name from resolvePublicSessionName. A cwd-scoped implicit session is stored under formatScopedSessionName(scopeId, ...). So sessionStore.lookup(sessionName) misses the expired record and returns early. teardownSessionResources and finalizeBoundSessionApplicationLifecycle never run. Before this change, teardown got the session object and fell back to it, so the resources were still finished. Now an expired leased cwd-scoped session is deleted, but its recording, app-log, audio and perf captures and its platform lifecycle are never finished. That leaks native recorders and execution hosts, and no retry can reach them because the record is gone. If a bare-named session exists at that name, the lookup binds that unrelated session and finishes its captures instead. The rule: every teardown entry point binds the lifetime it was handed and never re-derives an address from SessionState.name. Please make SessionTeardown take the SessionRef, have lease-lifecycle capture it with sessionStore.lookup(params.sessionName), and retire that ref, as #3139 does. The earlier thread on this line is marked resolved, but that fix is only in #3139, not in this head. If the two PRs land together, this is covered. Please say so in the PR.

The Coverage job fails in scripts/__tests__/eager-closure-budgets.test.ts. The eager closure of session-teardown.ts grows from 46 to 50 modules against the merge-base. Three of the four new modules come from this PR: the new static imports of audio-probe-session-binding.ts, perf-capture-session-binding.ts and screen-recording-session-binding.ts. Each is a roughly 10-line wrapper over bindSessionCapture. The fourth, session-capture-binding.ts, comes from #3136. The rule: session-teardown's closure adds no module beyond those already evaluated. Please colocate the three per-field binders with bindSessionCapture in session-capture-binding.ts, which app-log-session-resource.ts already pulls in. Then update the R68 RESOURCE_OWNERS entries and their test to name that one owner file. Please do not add an APPROVED_OVER_CEILING row.

Is one binder module the smaller shape here? It would export bindSessionAudioProbe, bindSessionPerfCapture, bindSessionScreenRecording and bindSessionAppLog next to bindSessionCapture. That removes three files, stops the closure growth, and gives R68 one owner path per field. The net -146 production lines and the removal of DurableCaptureResourceDefinition and sessionSlot are a good direction.

The PR body says live device evidence is still pending. On one Android emulator or iOS simulator, with a fresh daemon built from this head, please run these and paste the CLI output. First run open <app>, record start, record stop. The stop should return the artifact path, and a second record start should succeed, which shows the slot was cleared through the binding. Second, run record start with no session, then record stop. session list should no longer show the record-only session, which shows the retire(ref) path. Third, run open, record start, logs start, close. The close response should report no cleanup failure, and the recording artifact should be finalized.

Not blocking, take or leave: stopSessionAppLog is now a pure pass-through to forceCleanupSessionAppLog, so one wrapper can go; record-runtime.ts:250 uses ref!, which the plan guarantees but the type does not, so the stop-live plan type could carry the ref; the comment at failed-finish.test.ts:588 is over the format width; and the teardownSessionResources tests changed signatures but add no held-finish case through the teardown route.

I read the diff but did not run the mutation controls from the PR body. I also did not confirm a production flow where a leased session is cwd-scoped. I only confirmed that session.name is the public name and that cwd-scoped addresses differ from it.

Before merge, please move the three binders into session-capture-binding.ts and land the lease-expiry SessionRef fix in or before this PR.

@thymikee
thymikee added this pull request to stack #3146 October 3, 2026 08:46

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