Skip to content

fix: finish session captures through lifetime bindings - #3137

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

thymikee wants to merge 3 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.96 MB 4.96 MB -1.1 kB
Package (unpacked) 4.96 MB 4.96 MB -1.1 kB
Package (download) 1.49 MB 1.49 MB -238 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.2 ms 27.6 ms +1.5 ms
CLI --help 83.6 ms 83.9 ms +0.2 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
@thymikee
thymikee force-pushed the fix/session-capture-bindings branch from d0f7cf8 to 097ab95 Compare October 3, 2026 14:41
@thymikee
thymikee force-pushed the fix/session-capture-finish branch 2 times, most recently from c4bd3ea to 75c680a Compare October 3, 2026 15:16
@thymikee
thymikee force-pushed the fix/session-capture-bindings branch from 712e7da to bef2ebb Compare October 3, 2026 16:41
@thymikee
thymikee force-pushed the fix/session-capture-finish branch 2 times, most recently from 17ba57f to 480429a Compare October 3, 2026 16:56
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

The earlier consolidation of the teardown imports into session-capture-binding.ts looks good, but one defect and the missing device evidence from the review at 1376a9e still remain at 480429a.

request-execution-scope.ts:421 is unchanged. teardownExpiredSession re-derives a ref with sessionStore.lookup(expiredSessionName) and returns early on a miss. A cwd-scoped leased session is stored under its scoped address, not its public name, so the lookup misses it. Then teardownSessionResources and finalizeBoundSessionApplicationLifecycle never run. When such a session expires, its recording, app-log, audio and perf captures and its platform lifecycle keep running after the record is deleted. If a session exists under the bare public name, the lookup binds that unrelated session instead, and its captures can be finished by mistake. Can every teardown entry point bind the SessionRef it was handed and never re-derive an address from a name? To do that, either bring the #3139 change into this PR (the lease-lifecycle callback passes the captured SessionRef into teardownExpiredSession, and that ref is retired), or state in the PR body that this PR must land together with #3139.

The live device evidence asked for at 1376a9e is still missing, and the PR body still calls it pending. Please run these on one Android emulator or iOS simulator, with a fresh daemon built from this head, and paste the CLI output. First: open <app>, record start, record stop, then a second record start that succeeds. Second: record start with no session, record stop, then session list shows no record-only session. Third: open, record start, logs start, close, where close reports no cleanup failure and the recording artifact is finalized.

Not blocking: bindRecordOnlyScreenRecording in screen-recording-session-binding.ts:20 still writes draft.screenRecording, while R68 now lists only session-capture-binding.ts as the owner, and the gate stays green only because it scans object properties, not assignments, so moving it into session-capture-binding.ts and deleting this file would close the gap, or you can leave it.

The cubic-dev-ai thread on the expiry lookup still applies (#3137 (comment)). It is marked fixed in #3139, but line 421 at this head still looks up the public name and returns early.

Checks for 480429a are queued and the earlier runs were cancelled, so CI has not reported yet. The Coverage eager-closure failure at 1376a9e came from the session-teardown.ts imports. This commit moves them into session-capture-binding.ts, but I have not run the eager-closure budget test, so please confirm Coverage passes. There are no conflicts. Before merge, the lease-expiry SessionRef fix must be in this PR or the PR body must say the two land together, and the three live runs must be pasted. I could not re-read lease-lifecycle at this head and relied on the 1376a9e finding that expiredSessionName is the public session name, and I did not check whether #3139 is set to land in the same merge.

Session teardown imported three per-capture binding modules plus their shared
binding, which grew its eager closure past the merge-base. The audio, perf and
screen-recording bindings now live beside bindSessionCapture.
@thymikee
thymikee removed this pull request from stack #3146 October 3, 2026 19:38
@thymikee
thymikee force-pushed the fix/session-capture-bindings branch from 2652dd2 to cbf0a6e Compare October 3, 2026 19:39
@thymikee
thymikee force-pushed the fix/session-capture-finish branch from fd483da to e0a818c Compare October 3, 2026 19:39
@thymikee
thymikee added this pull request to stack #3187 October 3, 2026 19:45
@thymikee
thymikee removed this pull request from stack #3187 October 3, 2026 20:58
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Consolidated into #3135 as part of reducing #3116 to seven PRs. The composition preserves the complete pre-consolidation source tree, including tests and later review corrections. This PR is superseded; its review discussion and native evidence remain available. Outstanding findings transfer to the owning keeper in the implementation record.

@thymikee thymikee closed this Oct 3, 2026
@github-actions

github-actions Bot commented Oct 3, 2026

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

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