Skip to content

fix(terminals): negotiate explicit close intent for paired runtimes - #10129

Merged
OrcaWin merged 7 commits into
mainfrom
pr-10013-mass-terminal-teardown
Jul 27, 2026
Merged

OrcaWin merged 7 commits into
mainfrom
pr-10013-mass-terminal-teardown

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Jul 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Negotiate session-tabs.close-intent.v1 inside encrypted authentication for paired runtime clients.
  • Refuse a reasonless session.tabs.close only for in-process callers and paired runtime clients that advertised the capability.
  • Preserve current-main behavior for old clients, old servers, mobile clients, and explicit user closes.
  • Add a deterministic baseline/candidate/revert oracle spanning real v25/v26 daemon PTYs, five worktrees, capable and legacy viewer identities, exact process incarnations, kill events, and post-close I/O.
  • Supersede fix(terminals): attribute mass terminal kills and confirm bulk orphan sweep (#9949) #10013 without retaining its orphan-sweep attribution diagnostics.

Cause

A pre-contract paired desktop viewer can turn stale PTY-exit observations into sequential reasonless session.tabs.close requests. The host treats those requests as explicit user intent and routes them through closeMobileSessionTab, RuntimeNotifier.closeTerminalTab, and immediate daemon shutdown. That RPC path bypasses #10013's orphan-sweep confirmation guard.

An old viewer's intentional close and stale cleanup close are byte-identical. A global reasonless-close refusal therefore prevents the incident but makes mixed-version behavior worse than current main by breaking intentional closes from old clients.

Compatibility

Client Server Result
old new Legacy reasonless close remains allowed, matching current main
new old The old server ignores the additive encrypted auth field; existing explicit user close and legacy cleanup behavior are unchanged
new new The authenticated socket carries the negotiated capability; reasonless close is refused and explicit reason: "user" remains destructive
mobile new Existing mobile compatibility is unchanged; E2EE v2 still rejects additive auth metadata

The plaintext hello now carries only the ephemeral ECDH public key. The capability is parsed from encrypted authentication after the device token succeeds, then bound to that authenticated socket. A destructive request cannot assert the capability itself, and an intermediary cannot strip it without invalidating encryption. Capability parsing is bounded to 64 non-empty strings of at most 128 characters.

Reproduction oracle

GitHub CI command:

pnpm exec playwright test tests/e2e/daemon-generation-legacy-close-safety.spec.ts \
  --config tests/playwright.config.ts \
  --project electron-headless \
  --workers=1

The harness starts live v25 and v26 daemon generations. Each generation owns one capable-viewer PTY and one legacy-viewer PTY in separate worktrees, plus a fifth unrelated control PTY. An observer lists every target before, between, and after the close sequences while issuing zero closes.

  • Current main / capability disabled: the capable sequence also kills its two exact process trees, so the oracle is red.
  • Prior global-refusal candidate: the legacy sequence survives, exposing the mixed-version regression, so the oracle is red.
  • This candidate: capable PTYs survive, snapshots are republished, post-close I/O succeeds, and no kill occurs; byte-identical legacy closes retain current-main shutdown behavior; the unrelated control survives.

A real-WebSocket integration contract separately proves encrypted auth metadata reaches authenticated socket state, RPC context, and the close-refusal handler.

Screenshots

No visual change.

Testing

  • Reliability-gate Vitest command: 1,211 passed
  • Focused encrypted-auth/close contracts: 138 passed
  • pnpm typecheck
  • pnpm build:desktop and pnpm build:web
  • Reliability-gate and max-lines checks
  • Full Vitest execution: 37,578 passed, 60 skipped; one unchanged GitHub-project test hit its 30-second timeout under full-suite load and passed 2/2 in 10.9 seconds when isolated
  • Environment-sensitive relay test passed 10/10 when inherited Git config injection was removed
  • Prior-head GitHub Playwright E2E: compatibility oracle passed in 5.3 seconds (run 30296400442, shard 10); that shard failed only in three unrelated plugin/performance tests
  • Required GitHub checks on review head c528b6bb5

pnpm lint passes Oxlint, switch exhaustiveness, quadratic-buffer, reliability-gate, max-lines, skill-manifest, and localization-catalog checks. Its final localization-coverage step fails on six unchanged Ghostty search keywords from current main.

No Playwright tests were run locally for this revision.

Review and security

Three internal review/fix rounds completed. The production path adds one bounded capability parse during authentication and one membership check per close. It adds no polling, subprocess, provider scan, retry, timer, filesystem mutation, path construction, dependency, or broad lifecycle change.

The capability only enables stricter self-denial. Local/in-process cleanup, typed lifecycle close, daemon routing, SSH/relay, WSL, reconnect, sleep/wake, shutdown, and explicit single-terminal close paths are otherwise unchanged.

Remaining gaps

  • Any pre-contract paired desktop viewer remains vulnerable to its own stale reasonless close storm. Preserving that viewer's intentional-close behavior makes this unavoidable until it upgrades.
  • The primary live E2E gap is a headed Orca desktop acting as the paired server with a separate client.
  • Secondary parity coverage is still needed for a real headless orca serve.
  • Docker SSH proves only the SSH provider/relay path and is not paired-server evidence.
  • A physical headed host is only needed if later evidence implicates OS window, ConPTY, updater, or sleep/wake semantics.

Supersedes #10013.
Fixes #9949.
Linear: STA-2354.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The RPC close handler now refuses reasonless closes from non-mobile clients while preserving explicit user and mobile close behavior. Encrypted authentication propagates runtime close-intent capabilities across runtime clients and connections. New fixtures bundle a legacy close client and support canaries across worktrees. A Playwright scenario exercises legacy closes across two daemon generations, records liveness and kill events, writes reconstruction data, and validates refusal and PTY survival. Reliability-gate metadata, assertions, evidence, budgets, and promotion criteria were updated accordingly.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address #9949 by preventing reasonless close from capable paired runtimes while preserving legacy, mobile, and explicit user closes.
Out of Scope Changes check ✅ Passed The diff stays focused on capability negotiation, close-refusal flow, and supporting tests/fixtures; no unrelated product areas appear.
Title check ✅ Passed The title is concise and accurately captures the main change: negotiating explicit close intent for paired runtimes.
Description check ✅ Passed The description covers the required summary, screenshots, testing, review, security, and notes content, with only minor template mismatches.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

fabolivark added a commit to ab2webco/orca-oss that referenced this pull request Jul 24, 2026
Adopt upstream stablyai/orca#10129: a reasonless session.tabs.close from an
old paired runtime viewer is now refused + republished instead of destructively
closing a live authoritative PTY across worktrees/daemon generations. Legacy
mobile user-closes preserved; explicit-reason closes still close.

Prod change is fail-closed (clientKind !== 'mobile'); +689 lines are the
v25/v26 daemon-generation E2E safety harness + reliability gate.
OrcaWin added 2 commits July 27, 2026 01:25
…l-teardown

# Conflicts:
#	src/main/runtime/rpc/e2ee-channel.ts
#	src/main/runtime/rpc/mobile-socket-wiring.ts
@OrcaWin OrcaWin changed the title fix(terminals): refuse ambiguous legacy runtime closes fix(terminals): negotiate explicit close intent for paired runtimes Jul 27, 2026

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
config/reliability-gates.jsonc (1)

1165-1173: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

result: "passed" records a run that did not exercise the current oracle.

The summary states this run captured baseline/current-main destructive behavior and that the capability-gated combined oracle "supersedes that candidate result and is pending GitHub CI". Since this is the only passed evidence run for tests/e2e/daemon-generation-legacy-close-safety.spec.ts, the gate's partial protection is satisfied by evidence that explicitly disclaims the declared invariant/oracle. Either attach a run of the spec at its current head, or record this entry in a way that doesn't read as passing evidence for the new contract.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: aaa353b1-d2a2-42b2-a2a7-06a18cb99ad0

📥 Commits

Reviewing files that changed from the base of the PR and between 2560a32 and 9049def.

📒 Files selected for processing (2)
  • config/reliability-gates.jsonc
  • src/cli/runtime/websocket-transport.test.ts

@Jinwoo-H

Copy link
Copy Markdown
Contributor

Addressed the evidence-integrity review in 480e8b0: the earlier baseline-only entry was replaced with the current-head GitHub CI result from run 30250731941 / shard 10 (the compatibility oracle passed in 5.1s), and deterministic red/green evidence is now marked complete. Overall gate protection remains partial for the explicit headed paired-server and headless serve live-topology gaps.

@Jinwoo-H

Copy link
Copy Markdown
Contributor

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai 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.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: be007343-938e-4ba8-964c-a043341fc59a

📥 Commits

Reviewing files that changed from the base of the PR and between 480e8b0 and a1b97a1.

📒 Files selected for processing (17)
  • config/reliability-gates.jsonc
  • src/cli/runtime/websocket-transport.test.ts
  • src/main/runtime/remote-runtime-request-connection.integration.test.ts
  • src/main/runtime/rpc/e2ee-channel-v2.test.ts
  • src/main/runtime/rpc/e2ee-channel.test.ts
  • src/main/runtime/rpc/e2ee-channel.ts
  • src/main/runtime/rpc/mobile-e2ee-auth-validation.ts
  • src/main/runtime/rpc/mobile-socket-wiring.test.ts
  • src/main/runtime/rpc/mobile-socket-wiring.ts
  • src/renderer/src/web/web-runtime-client.test.ts
  • src/renderer/src/web/web-runtime-client.ts
  • src/shared/remote-runtime-client.test.ts
  • src/shared/remote-runtime-client.ts
  • src/shared/remote-runtime-request-connection.test.ts
  • src/shared/remote-runtime-request-connection.ts
  • src/shared/remote-runtime-shared-control-connection.test.ts
  • src/shared/remote-runtime-shared-control-frame-handler.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • config/reliability-gates.jsonc

OrcaWin added 2 commits July 27, 2026 12:36
…l-teardown

# Conflicts:
#	src/main/runtime/rpc/core.ts
#	src/main/runtime/rpc/dispatcher.ts
@OrcaWin
OrcaWin merged commit 24706cc into main Jul 27, 2026
9 checks passed
@OrcaWin
OrcaWin deleted the pr-10013-mass-terminal-teardown branch July 27, 2026 22:22
zpyoung pushed a commit to zpyoung/orca that referenced this pull request Jul 29, 2026
…r workers (stablyai#9925)"

This reverts commit cd05f2f for the
v1.4.160-rc.4 cut.

Three files needed manual resolution because later commits touched the same
regions. Their post-9925 work is preserved:

- src/main/runtime/rpc/core.ts and dispatcher.ts keep clientCapabilities and
  the RuntimeCapability import from stablyai#10129 (close-intent negotiation for
  paired runtimes).
- src/cli/runtime/client.ts keeps the deferred websocket-transport import
  from stablyai#10919.
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.

Intermittent cross-worktree PTY teardown when opening an embedded browser

2 participants