Skip to content

fix(server): archiving or deleting a thread releases its devices - #34175

Open
binjamshid wants to merge 1 commit into
pingdotgg:mainfrom
binjamshid:fix/release-devices-on-thread-archive
Open

binjamshid wants to merge 1 commit into
pingdotgg:mainfrom
binjamshid:fix/release-devices-on-thread-archive

Conversation

@binjamshid

Copy link
Copy Markdown
Contributor

Problem

Archiving a thread, deleting it, or deleting its project left the thread's device sessions claimed. Another thread that opened the same simulator or emulator got DEVICE_IN_USE, and after a delete no thread was left that could call device_close. #32524 covers settling; this covers archive and delete, which were asked to be tracked separately.

Change

Archive and delete already queue the per-thread terminal.cleanup effect after the command commits (Orchestrator.ts archive, ThreadDeletion.ts; project delete goes through thread delete). That effect now also releases each of the thread's device sessions through DeviceService.close, the same release device_close does. Devices stay booted.

  • ResourceCleanupService.cleanupDevices closes sessions one at a time and tries every one, so an unreachable host does not keep other hosts' claims. Any failure is returned so the effect worker retries.
  • Terminal and device cleanup run independently; one failing does not skip the other.
  • No contract, schema or outbox change.

Scope and approval

Bug fix for #34165, the archive/delete part of #17470 that #32524 leaves unchanged.

Verification

  • The reporter's three cases (archive, thread delete, project delete) in runtimeLayer.test.ts: all three failed on main with the session still held, and pass with the fix.
  • vp run check:changed origin/main: format, lint, knip, graphql, typecheck (3 packages) and 195 tests in 5 files pass.
  • Not checked: a real simulator or emulator.

Fixes #34165

Archiving or deleting a thread, or deleting its project, left the thread's device sessions claimed, so another thread opening the same simulator or emulator got DEVICE_IN_USE. The per-thread teardown effect that archive and delete already queue now also releases each of the thread's device sessions the way device_close does, leaving the devices booted. Fixes pingdotgg#34165.
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Nov 11, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at e46a8a0

Macroscope's review found this PR approvable — Archiving or deleting a thread now releases its device claims alongside terminal cleanup. Release is scoped to that thread and leaves devices running for other threads.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Thread archive and deletion cleanup now releases the thread’s device sessions while leaving devices booted. The terminal cleanup effect runs device cleanup alongside terminal cleanup and propagates terminal cleanup failures.

Changes: Thread Device Cleanup

Layer / File(s) Summary
Device cleanup service
apps/server/src/orchestration-v2/ResourceCleanupService.ts
ResourceCleanupService adds cleanupDevices, which closes the thread’s device sessions and reports the first close failure as a device cleanup error. The default implementation is a no-op.
Lifecycle cleanup integration
apps/server/src/orchestration-v2/EffectWorker.ts, apps/server/src/orchestration-v2/runtimeLayer.test.ts, apps/server/src/guest/GuestShareService.test.ts, apps/server/src/provider/userInputAttachments.test.ts
The terminal cleanup effect runs terminal and device cleanup in parallel. A device cleanup failure alone does not fail the effect; a terminal cleanup failure is propagated. Lifecycle tests cover archiving a thread, deleting a thread, and deleting its project, and verify that other threads’ sessions remain and the simulator stays booted. Test cleanup layers provide the new service dependency.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant EffectWorker
  participant ResourceCleanupService
  participant DeviceService
  EffectWorker->>ResourceCleanupService: cleanupDevices(threadId)
  ResourceCleanupService->>DeviceService: Get sessions for threadId
  loop Each session
    ResourceCleanupService->>DeviceService: Close session host/device pair
  end
  DeviceService-->>ResourceCleanupService: Close results
  ResourceCleanupService-->>EffectWorker: Cleanup result
Loading

Possibly related issues

  • pingdotgg/t3code#32930 — It reports missing device-session cleanup after archive and deletion, which this change adds.
  • pingdotgg/t3code#34173 — It identifies the same archive and deletion cleanup cases covered by this change.

Possibly related PRs

  • pingdotgg/t3code#32935 — Both changes address orchestration-v2 device-session cleanup; that PR also covers settle and session-identity targeting.
  • pingdotgg/t3code#25832 — Both changes integrate device-session release with thread lifecycle cleanup.

Suggested reviewers: yash-singh1, juliusmarminge

Merge Risk: Minimal · up to e46a8

Archive and deletion cleanup is wired to release device sessions, and cleanup failures remain retryable. No issue identified here needs to block merging after normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title is concise, specific, and accurately describes the primary change: releasing device sessions when a thread is archived or deleted.
Description check Passed The description includes Problem, Change, Scope and approval, and Verification sections. It explains the affected behavior, implementation, scope, focused tests, reported check results, and the untest…
Linked Issues check Passed No active directly linked issue remains. Linked issue #34165 is closed and supplies historical context only. The pull request adds device cleanup to the existing terminal cleanup effect and adds cover…
Out of Scope Changes check Passed The changes stay within the stated cleanup fix. ResourceCleanupService closes each thread device session, EffectWorker runs device and terminal cleanup independently, and runtime-layer tests cover the…
Full details: Description check

Explanation

The description includes Problem, Change, Scope and approval, and Verification sections. It explains the affected behavior, implementation, scope, focused tests, reported check results, and the untested real-device scenario.

Full details: Linked Issues check

Explanation

No active directly linked issue remains. Linked issue #34165 is closed and supplies historical context only. The pull request adds device cleanup to the existing terminal cleanup effect and adds coverage for archive, thread delete, and project delete, but these are not additional required objectives.

Full details: Out of Scope Changes check

Explanation

The changes stay within the stated cleanup fix. ResourceCleanupService closes each thread device session, EffectWorker runs device and terminal cleanup independently, and runtime-layer tests cover the three lifecycle paths. The test-layer updates provide the required DeviceService implementation. No unrelated feature, contract, schema, or outbox change is shown.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 631ea1b and e46a8a0.

Files selected for processing: 5

  • apps/server/src/guest/GuestShareService.test.ts
  • apps/server/src/orchestration-v2/EffectWorker.ts
  • apps/server/src/orchestration-v2/ResourceCleanupService.ts
  • apps/server/src/orchestration-v2/runtimeLayer.test.ts
  • apps/server/src/provider/userInputAttachments.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

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

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Archived or deleted threads keep holding their devices

1 participant