Skip to content

fix: bind capture adoption to session lifetimes - #3136

Open
thymikee wants to merge 2 commits into
refactor/session-lifetime-entriesfrom
fix/session-capture-bindings
Open

thymikee wants to merge 2 commits into
refactor/session-lifetime-entriesfrom
fix/session-capture-bindings

Conversation

@thymikee

@thymikee thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Capture adoption now receives a binding to one session lifetime. The binding owns admission, slot identity, artifact addressing and explicit field updates. Audio, perf, app-log and screen-recording callers capture their ref before asynchronous startup.

An unpublished record-only draft publishes at the existing adoption point. Shutdown can refuse publication while retaining truthful recovery evidence when cleanup is unconfirmed. Failed adoption rechecks authority after cleanup yields, so it cannot terminalize successor evidence.

flowchart LR
  Start[Start native capture] --> Adopt[Lifetime-bound adoption]
  Adopt --> Check{Admission and slot still valid?}
  Check -->|Yes| Publish[Publish owned resource]
  Check -->|No| Cleanup[Dispose captured handle]
  Cleanup --> Evidence[Retain unconfirmed recovery evidence if still authorized]
Loading

Depends on #3135. Ref #3116. Thirty-two files, 797 gross changed lines. Capture finishing migrates in the next dependency layer.

Validation

Tested d0f7cf8121: quick checks, Fallow and exact-head affected checks pass, including 355 related files / 2,357 tests. Focused draft/resource/log tests pass.

Removing admission or lifetime checks, reusing pre-await persistence authority, and coupling draft persistence to admission each fail the regression controls; restored code passes. 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 +2.0 kB
Package (unpacked) 4.94 MB 4.94 MB +2.0 kB
Package (download) 1.48 MB 1.48 MB +799 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.6 ms 27.7 ms +0.1 ms
CLI --help 82.4 ms 84.5 ms +2.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 32 files

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

Re-trigger cubic

Comment thread src/daemon/__tests__/session-capture-binding.test.ts
Comment thread src/daemon/session-capture-binding.ts
Comment thread src/daemon/__tests__/perf-capture-session-resource.test.ts
Comment thread src/daemon/session-observability/internal/session-perf-runtime.ts
Comment thread src/daemon/__tests__/session-capture-binding.test.ts
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

The code in d0f7cf8 looks sound, but the PR is not ready: live device evidence is still missing, and the Coverage check fails because of this change.

Coverage fails in scripts/__tests__/eager-closure-budgets.test.ts. src/daemon/session-teardown.ts went from 46 to 47 evaluated modules. The new module is src/daemon/session-capture-binding.ts. It comes in through the static import at app-log-session-resource.ts:19, and teardown already imports that file for forceCleanupSessionAppLog. I did not run this test locally. I traced the cause from the CI import route and the diff. No conflicts.

The live run should reach three routes and show their output. (1) On a fresh session name with no prior open, whole-screen record start should publish the record-only session, so session list shows it, and record stop should return a playable artifact path. This is the draft-publication route. (2) On an opened session, record start/record stop and logs start/logs stop should succeed and leave no cleanup-pending resource record. (3) Where the device supports them, perf capture start/stop and audio capture start/status should work. This PR changes all four device-facing start paths, and the PR body says live evidence is still pending.

Could one binding constructor own the rule instead? The rule would be that a capture binding derives its slot only from the durable definition's sessionSlot. A method on SessionStore, for example sessionStore.bindCapture(ref, definition.sessionSlot), would sit beside the new assertAdmissionOpen/assertPublishable and write through sessionStore.update(ref, s => slot.replace(s, r)). The four per-kind adapters (audio, perf, screen recording, app log) already restate the sessionSlot.read and sessionSlot.replace that each durable definition declares. That would delete session-capture-binding.ts, the three per-kind binding files, bindSessionAppLog and the three new ownership-table entries. It would also fix the eager-closure budget without a dynamic import, because teardown imports session-store.ts only as a type and already evaluates app-log-session-resource.ts, so the call adds no module. The record-only draft can stay as a small variant of the same method. Nothing has to change first, since the definitions already declare sessionSlot.

Not blocking, and fine to take or leave: (a) Finishing is still keyed by address. clearLiveSlot and forceCleanupLiveDurableCapture in transitions.ts:193 call sessionStore.set(sessionName, ...) after awaiting the native finish, so a successor published at that address during the await would be overwritten. The rule for #3138 is that every finish caller (teardown, close, logs stop, record stop, audio restart, perf stop) holds a ref-bound binding and clears through binding.clear(expected). (b) No test reaches the draft binding's canPersist branch in screen-recording-session-binding.ts:26, where another request publishes the address while a record-only start is in flight. A test could publish the address after the draft is created, run adoptStartedScreenRecording, and assert session_address_occupied, a disposed handle and an unchanged winner record. (c) DurableCaptureSessionBinding.clear has no production caller here, so it could move to #3138, and draft! in record-runtime.ts:144 could go if both session shapes return one {binding, requireRef}.

I judged the tests by reading them and did not run the mutations the PR describes. I also could not confirm whether per-session request serialization makes the read() fallback window reachable in production. #3138 is not in this head, so I did not check what it fixes.

Before merge, please remove the new static edge from the teardown closure so Coverage passes, then attach the live record-only record start/record stop and logs start/logs stop output.

@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