Skip to content

fix: preserve daemon cleanup and capture lifetime proof - #3138

Closed
thymikee wants to merge 2 commits into
fix/session-capture-finishfrom
fix/daemon-capture-review-controls
Closed

thymikee wants to merge 2 commits into
fix/session-capture-finishfrom
fix/daemon-capture-review-controls

Conversation

@thymikee

@thymikee thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Follow-up to #3133–#3137 for #3116. Daemon test cleanup stops independently observed children even with corrupt metadata, joins registered successors, and locks registration before deleting a directory. Timeout diagnostics distinguish retained metadata from a process that survived.

Capture bindings retain their own adopted handle after retirement. Cleanup refreshes only the same lifetime before a lazy import; native starts refuse closed admission. The shared fixtures now model lifetime and fence identity. Three capture field adapters live with their binding, fixing the eager-import CI regression.

flowchart LR
  R[Captured lifetime] --> B[Capture binding]
  B --> H[Owned handle]
  B --> C{Current lifetime and fence match?}
  C -->|Yes| U[Update matching slot]
  C -->|No| K[Preserve successor]
Loading

Validation

Head: 0780e5e4c6. 33 changed files; 126 focused tests pass without skips. Ten planted regressions failed, then restored green. All 757 global import-budget tests pass. Lint, typecheck and changed-code Fallow pass. Independent read-only audit found no actionable issues.

All locally runnable affected checks pass; platform/device CI remains GitHub-authoritative.

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 +426 B
Package (unpacked) 4.96 MB 4.96 MB +426 B
Package (download) 1.49 MB 1.49 MB +124 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.2 ms 27.1 ms -0.1 ms
CLI --help 81.6 ms 83.4 ms +1.7 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 33 files

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

Re-trigger cubic

Comment thread packages/capture-kit/src/durable-capture/transitions.test.ts
Comment thread scripts/layering/session-resource-ownership.ts
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

This PR is ready at 0780e5e. All 14 checks pass, and bundle size grows by +1.1 kB unpacked, under the 3 kB threshold. There are no conflicts. I did not rerun the focused tests or the planted mutations, and I judged mutation sensitivity by reading the base code paths. I also did not check whether the fence state that createNextFence mints leaves anything behind on the now-refused start paths. I read only enough to see that it is in-memory. The base is fix/session-capture-finish (#3137), so this review covers only the delta from 1376a9e. Please merge #3137 first. After that, only the notes below remain.

Not blocking, and you can take or leave these: the same admission check (binding.assertAdoptable() after the last await, before the native start) now sits at four sites (record-runtime.ts:156, session-audio.ts:133, session-observability.ts:379 and session-perf-runtime.ts:221), but only perf has a handler-route test, so deleting the record, audio or logs line leaves the suite green. The rule is that no native capture start runs unless the admission check passed after the last await before it. You could parametrize the perf route test over record, audio and logs, or move the assert, start, adopt sequence into one durable-capture-resource helper and test it once. Also, the fixture binding in session-binding.fixtures.ts:59 copies the ownership rules of bindSessionCapture (the retained-handle read fallback and the clear predicate) from session-capture-binding.ts:47-53. Exporting that predicate from capture-kit and using it in both places would keep the transitions tests tied to the shipped rule.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 3, 2026
@thymikee
thymikee added this pull request to stack #3146 October 3, 2026 08:46
@thymikee
thymikee force-pushed the fix/session-capture-finish branch from 1376a9e to c4bd3ea Compare October 3, 2026 14:41
@thymikee
thymikee force-pushed the fix/daemon-capture-review-controls branch 2 times, most recently from aa51d9b to d9019e1 Compare October 3, 2026 15:16
@thymikee
thymikee force-pushed the fix/session-capture-finish branch from c4bd3ea to 75c680a Compare October 3, 2026 15:16
@thymikee
thymikee force-pushed the fix/daemon-capture-review-controls branch from d9019e1 to ee66f4b Compare October 3, 2026 16:41
@thymikee
thymikee force-pushed the fix/session-capture-finish branch from 17ba57f to 480429a Compare October 3, 2026 16:56
@thymikee
thymikee force-pushed the fix/daemon-capture-review-controls branch from ee66f4b to 2184bed Compare October 3, 2026 16:56
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

The code still looks right at 2184bed. The rebase fixed the earlier gap: the owner table now sits in the base, so this PR no longer touches it. Handle identity in clear is covered by 'clearing an older handle or fence leaves a replacement capture intact' in src/daemon/tests/session-capture-binding.test.ts.

Not blocking: the last commit, 'chore(gates): colocate durable capture field owners', now only adds a comment to the unit-include list at https://github.com/callstack/agent-device/blob/2184bed/vitest.config.ts#L181, so its title no longer matches the change. You could retitle it (for example 'test: note why daemon-test-cleanup runs in the unit lane') or squash it into the first commit, or leave it.

On the open threads: I did not run the focused tests or the R68 gate. I also did not check that #3142 declares the record-only screenRecording writer, since that change is outside this PR. The two threads below no longer apply, so please resolve them. #3138 (comment) was already resolved before the earlier review, and the test above pins handle identity at the binding level. #3138 (comment) is about scripts/layering/session-resource-ownership.ts, which this PR no longer touches, and you said #3142 declares the owner.

Checks for 2184bed are still queued or running. The cancelled runs are older runs replaced by the push, not failures. No conflicts are known. Before merge, the checks must finish green and the base PR (fix/session-capture-finish) must land.

@thymikee
thymikee force-pushed the fix/session-capture-finish branch from 480429a to fd483da Compare October 3, 2026 17:49
@thymikee
thymikee force-pushed the fix/daemon-capture-review-controls branch from 2184bed to a0b22de Compare October 3, 2026 17:49
@thymikee
thymikee removed this pull request from stack #3146 October 3, 2026 19:38
@thymikee
thymikee force-pushed the fix/session-capture-finish branch from fd483da to e0a818c Compare October 3, 2026 19:39
@thymikee
thymikee force-pushed the fix/daemon-capture-review-controls branch from a0b22de to 054dd00 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. Capture/lifetime changes are in #3135, registration corrections in #3127, and the exact home-path assertion in #3186. 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:16 UTC

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

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant