Skip to content

fix(daemon): keep a recording downloadable when its lease ends - #3955

Merged
thymikee merged 2 commits into
callstack:mainfrom
alcpereira:fix/keep-recording-on-lease-end
Oct 26, 2026
Merged

thymikee merged 2 commits into
callstack:mainfrom
alcpereira:fix/keep-recording-on-lease-end

Conversation

@alcpereira

@alcpereira alcpereira commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

When a lease ends while a remote recording runs, teardown finishes the recording but drops the result. The export stays in the daemon's temp directory with no artifact id, so the client's later record stop fails (UNAUTHORIZED) and the files leak. This is the lease-end half of #2281 (ADR 0024 rule 1).

  • A leased session's teardown (lease expiry, release, or close) registers a committed remote-temp export and its telemetry as downloadable artifacts of the lease's tenant: delete after download, 15-minute expiry. If registration fails, it deletes the files. A failed finish keeps its forced cleanup.
  • GET /artifacts entries gain an optional sessionName (additive).
  • trackDownloadableArtifact returns the existing entry for a file the same tenant already has, so a record stop replay serves the same ids.
  • Local mode is unchanged.

14 files. Possible follow-up: an artifacts download <id> CLI command (HTTP clients can already GET /artifacts/<id>).

Validation

  • Head 3f2fddcf8: pnpm check:affected --run --head 3f2fddcf8 passed (exactCommit: true): format, lint, typecheck, layering, fallow, build, vitest-related, daemon-wire-compat, command-docs, docs-links, docs-prose.
  • New tests fail without the fix: 4 fail with the retention call disabled, and the replay test fails without the duplicate-id fix.
  • Coverage failed at c1a44778c on eager-closure-budgets (teardown statically imported the retention module). 3f2fddcf8 loads it on demand; the budget test passes locally.
  • CI still owes: Integration Tests, Coverage, Apple/Linux smoke at 3f2fddcf8.
  • No device run; the change is daemon artifact registration, not device behaviour.

Lease-end teardown finished an active recording but dropped the
completion, so a remote caller's export stayed in the daemon temp
directory with no artifact id: the client's later record stop failed
with the lease gone and the files leaked.

- A leased session's teardown (lease expiry, release, or close) registers a committed remote-temp export
  and its telemetry as downloadable artifacts of the lease's tenant,
  with the standard delete-after-download and 15-minute expiry. If
  registration fails, it deletes the files. A failed finish keeps its
  forced cleanup and registers nothing.
- The /artifacts inventory carries an optional public sessionName so
  clients sharing a tenant can find their own recording.
- trackDownloadableArtifact returns the entry that already tracks a file
  for the same tenant unchanged, so a record stop replay serves the same
  ids instead of two ids racing to delete one file.
- record stop artifacts are built in one place for the reply and the
  teardown.

Lease-end half of callstack#2281.

@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 14 files

Reply to a comment to ask cubic a question or push back. It learns from your replies.

View guided diff | Re-trigger cubic

Comment thread docs/adr/0024-screen-recording-provable-signal.md Outdated
Comment thread packages/daemon/src/sessions/leased-recording-retention.ts Outdated
Comment thread packages/daemon/src/sessions/leased-recording-retention.ts
Comment thread website/docs/docs/remote-proxy.md Outdated
Comment thread .changeset/leased-recording-retention.md
Comment thread packages/daemon/src/core/session-tenant-scope.ts
Comment thread docs/adr/0007-remote-device-leases.md Outdated
- session-teardown.ts imports the retention module only when the
  session holds a lease, so the daemon entry keeps its merge-base eager
  closure (eager-closure-budgets, which failed the Coverage job).
- The registration-failure rollback removes each file best-effort, so
  one failed removal does not leak the rest.
- The unleased-teardown test also checks the tenant inventory.
- Docs: ADR 0007 names close as a teardown that keeps the recording,
  ADR 0024 scopes forced cleanup to the teardown finish, and the
  remote-proxy sessionName wording matches the contract.
@thymikee

Copy link
Copy Markdown
Member

At c1a4477 one code fix is needed before merge, and the Coverage failure needs a look. The Coverage job (pnpm gate unit-ci) failed, but its log ends mid-run at "building macOS helper" with no failing test or threshold line, so I cannot tell whether it is a timeout or a test in this PR. Before merge, please re-run that job or share the full log, and fix the best-effort deletion in the rollback at https://github.com/callstack/agent-device/blob/c1a4477/packages/daemon/src/sessions/leased-recording-retention.ts#L44 (or drop the rollback, since trackDownloadableArtifact has no throwing path). A failed rmSync there aborts the loop and escapes into the teardown aggregate.

Not blocking, and fine to take or leave: sessionName on the wire (https://github.com/callstack/agent-device/blob/c1a4477/packages/contracts/src/cloud-artifacts.ts#L56), tenantPublicSessionName and their doc sentences are not needed, because the inventory is already tenant-filtered and the retained video keeps the client's file name; a dedupe hit at artifact-tracking.ts:102 returns the first entry unchanged, so a record stop replay near minute 14 gets an id that expires about a minute later (the rule could be one live entry per path and tenant, expiring 15 minutes after its latest hand-out, or the replay could document that it inherits the original expiry); and the unleased test at leased-recording-retention.test.ts:301 asserts that a remote-temp export stays on disk, which pins the old leak as expected, so it could assert only that nothing is registered.

On the open inline threads: the one on rmSync still stands (#3955 (comment)), and three lower-priority threads still apply (the sessionName wording in remote-proxy.md, the "inverse" comment overclaim, and the ADR 0007 retention wording). These do not apply, so please resolve them: #3955 (comment) (an unleased teardown has no tenant to register under), #3955 (comment) (the new ADR paragraph is scoped to teardown disposal), #3955 (comment) (teardown holds a live handle, so no earlier entry can exist), and #3955 (comment) (retention runs on every close of a leased session).

I read the code only and did not run the new tests or check:affected. No test drives the lease sweeper or the close command end to end, and I did not verify whether a record stop replay of a torn-down session is reachable after the lease ends.

@thymikee

Copy link
Copy Markdown
Member

The PR is ready at 3f2fddc. The rollback concern from the earlier review (#3955 (comment)) and the Coverage failure are both fixed: the delta removes the static import of leased-recording-retention.ts that tripped the daemon eager-closure budget, and all 14 checks pass at this commit. There are no conflicts.

Not blocking: the new private removeBestEffort in packages/daemon/src/sessions/leased-recording-retention.ts:61 repeats the one in artifact-tracking.ts:357, and its rollback cannot run because trackDownloadableArtifact has no throwing path (artifact-tracking.ts:99-122), so you could drop the try/catch or export the shared helper. The caller in session-teardown.ts:208 also checks ref.session.lease before the dynamic import, and retainLeasedSessionRecording checks it again at leased-recording-retention.ts:26, so one of the two could go. Take or leave both.

The cubic-dev-ai threads no longer apply and can be resolved. Fixed at this commit: the tenant inventory assertion (#3955 (comment)), the ADR 0024 cleanup scope (#3955 (comment)), the per-rmSync try/catch (#3955 (comment)), the remote-proxy.md wording (#3955 (comment)), the session-tenant-scope.ts comment (#3955 (comment)) and the ADR 0007 wording (#3955 (comment)). Benign: teardown holds the live recording handle and the rollback is unreachable (#3955 (comment)), and retention runs on every teardown of a leased session, so "close" in the changeset is accurate (#3955 (comment)).

I read the code only and did not run the tests locally. No test drives the lease sweeper or the close command end to end.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 25, 2026
@thymikee
thymikee merged commit fa96af6 into callstack:main Oct 26, 2026
14 checks passed
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.

2 participants