Skip to content

feat: delegate manual attachment to external terminals - #1871

Merged
eitsupi merged 6 commits into
REditorSupport:mainfrom
eitsupi:feat/external-terminal-attachment
Oct 12, 2026
Merged

eitsupi merged 6 commits into
REditorSupport:mainfrom
eitsupi:feat/external-terminal-attachment

Conversation

@eitsupi

@eitsupi eitsupi commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

#1868 and #1870 provide managed sess preparation, connection references, and explicit terminal binding, but manual attachment still has a gap. When an external terminal has no live session binding, the status-bar attach action can send the native command to vscode-R's global endpoint. In vscode-R-console, that bypasses SessProxy and replaces the proxy-backed transport. PID discovery can also wait indefinitely on an extension-owned pseudoterminal before its frontend gets a chance to recover.

This adds session.registerTerminalIntegration(terminal, { attach(token) }) so the frontend can restore its own transport and bind the accepted connection using the existing APIs:

  • While a registered handler is pending, repeated attach requests remain busy even if a live binding already exists. Otherwise a live terminal owner is focused first, or the registered handler runs before PID discovery. Registered terminals are excluded from native PID association and use explicit binding.
  • Registration survives disconnects and connection replacement. Disposal, terminal close, and watcher shutdown revoke it and request cancellation; moving focus away also cancels a pending attempt.
  • Registering an integration releases native PID waits that already started for that terminal: scans continue to other terminals, and the old manual attach stops without fallback. Discovery updates use the same eligibility rules, and invalidated operations remain invalid after registration disposal. Terminal and watcher lifetimes, and the lifetime of the requesting handshake or manual operation, also bound native PID waits.
  • Concurrent attach requests do not submit duplicate work. Success requires a current live terminal binding, and delayed completion preserves a newer choice of another session. A decline, exception, or cancellation never falls back to the native command.

Consumers can prepare through their proxy, observe the matching connection, and call activate(sessionId, { terminal, connectionId, select: false }) before returning true. vscode-R then verifies the binding and selects/focuses the terminal. Consumers retain responsibility for prompt readiness, submission queues, failure explanations, and R code already submitted when cancellation occurs.

This completes the third part of the external session API for the intended 3.2.0 release alongside #1868 and #1870, so consumers can adopt managed sess with a supported manual recovery path. This change is on the TypeScript side and does not change the bundled sess revision or introduce another reinstall/restart transition. External frontends must adopt the hook; pasting a copied native attach command remains outside this API and can still bypass a proxy.

@eitsupi eitsupi added this to the 3.2.0 milestone Oct 12, 2026
@eitsupi
eitsupi requested review from Fred-Wu and renkun-ken October 12, 2026 03:25
@eitsupi
eitsupi marked this pull request as draft October 12, 2026 03:25
@eitsupi
eitsupi marked this pull request as ready for review October 12, 2026 03:56

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 5a99394. The delegated attach path and lifecycle coverage look solid, but there is one remaining late-registration race, detailed inline.

Validation: TypeScript and oxlint passed; all 85 tests in the terminal integration, binding, lifecycle, and workspace viewer suites passed; all 9 httpgd client tests passed. Two additional regression probes show that registering an integration does not release an already pending PID wait (native discovery and manual activation). With this PR's sess installed in a temporary library, the R workspace communication test passed; the standard plot viewer test timed out with both the PR and base session implementations, so I have not attributed that failure to this change.

Comment thread src/session.ts Resolved
@eitsupi
eitsupi marked this pull request as draft October 12, 2026 11:25
@eitsupi
eitsupi requested a review from renkun-ken October 12, 2026 12:09
@eitsupi
eitsupi marked this pull request as ready for review October 12, 2026 12:09

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the updates through b846038. The earlier P2 finding is fixed: registering an integration now releases an existing native discovery or manual-attach PID wait without needing processId to resolve. The new regression tests leave the PID unresolved, and both independent reproductions from my previous review now pass.

I also checked terminal/lifecycle invalidation, stale PID results, late rejection handling, discovery publication guards, and replacement handshakes. I found no new actionable issues in these changes.

Validation on macOS with VS Code 1.141.0: TypeScript compilation and oxlint passed; 123 extension-host tests passed across the session, terminal integration, binding, lifecycle, and workspace viewer suites; all 18 PID-wait/httpgd Node tests passed. Five additional focused probes passed, covering the original two failures, repeated handshakes on the same socket, closing a terminal during native discovery, and focus cancellation during a manual PID wait. The standard plot viewer test was excluded because its timeout previously reproduced with the base session implementation; the R workspace communication test passed using the bundled sess in a temporary library.

@eitsupi
eitsupi merged commit 18bdedf into REditorSupport:main Oct 12, 2026
13 of 15 checks passed
@Fred-Wu

Fred-Wu commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

@eitsupi Thanks so much for creating those APIs to support downstream applications, especially for my R console. I have updated and tested using the new APIs, they work well with the latest vscode-R pre-release version from github. Much appreciated.

@eitsupi
eitsupi deleted the feat/external-terminal-attachment branch October 13, 2026 02:05
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.

3 participants