Repository navigation
fix: fix workspace actions for explicitly bound downstream terminals - #1837
Conversation
eitsupi
changed the title
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed the latest revision, 4fca874. The explicit pseudoterminal binding path and its close/reconnect coverage look good, but two ownership edge cases below should be fixed before merging.
Validation: TypeScript compilation, extension build, and oxlint passed. The 105 tests in Session Terminal Binding, Session Communication, Workspace Viewer, and R Terminal passed in VS Code 1.140.0 on macOS. Additional targeted tests reproduced stale native fallback after an explicit owner's disconnect and an older terminal-selection callback overwriting a newer public API activation.
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Follow-up review of 4371af1: both findings from my previous review are addressed. I reran the original stale-native-disconnect and pending-selection reproductions; both now pass. The revision-based retirement also correctly requires a fresh native attach before an unbound terminal can resume PID routing.
One new readiness regression is described inline: a live explicit binding is ignored when deciding whether a managed terminal can accept its first source command.
Validation: TypeScript compilation, extension build, and oxlint passed. All 112 tests in Session Terminal Binding, Session Communication, Workspace Viewer, and R Terminal passed in VS Code 1.140.0 on macOS; all five GitHub CI checks are green. A targeted source-command comparison passes without explicit binding and fails after binding the same connected session to the same terminal.
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed the latest revision, 33664fe, including the shared terminal registry, exclusive associations, and reconnect/discovery guards. All three findings from my earlier reviews are fixed: stale native fallback stays removed, older terminal selection no longer overrides newer activation, and a managed terminal accepts its first source command after explicit binding. I reran those reproductions successfully.
One reconnect selection regression remains, described inline: selecting the same session while its replacement handshake is resolving leaves the Workspace attached to the destroyed previous Session object.
Validation: TypeScript compilation, extension build, and oxlint passed. All 130 relevant extension tests and all 17 terminal-registry Node tests passed in the local macOS setup with VS Code 1.140.0; all five GitHub CI checks are green. An additional reconnect comparison passes without an intervening selection and fails when the same terminal is reselected during discovery. A recovery check confirms that Workspace execution rejects until the terminal is selected again after reconnect.
eitsupi
changed the title
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed the latest revision, 0a224be. All findings from my earlier reviews are addressed, and I found no new actionable issues.
The reconnect change now carries the selected logical session to its replacement connection even after same-session reselection, while preserving a newer selection of a different session. Old Workspace nodes still reject execution rather than redirecting to the replacement. The final naming refactor preserves that behavior.
Validation: TypeScript compilation, extension build, oxlint, all 137 relevant extension tests, and all 17 terminal-registry Node tests passed locally with VS Code 1.140.0 on macOS. The seven independent reproductions covering the earlier ownership, readiness, and reconnect findings passed on 4fde676; the final commit only names the existing preservation condition, and the full relevant suite was rerun on 0a224be. CI for the latest commit is still running.
2 participants
Fixes #1836.
#1764 introduced the public session API with pseudoterminal downstream clients in mind. #1805 made Workspace actions execute in their owning session, but terminal dispatch only resolves native PID associations, leaving extension-owned pseudoterminals without an execution target.
Add backward-compatible
session.activate(sessionId, { terminal })to explicitly bind a terminal. Interactive execution retains priority; terminal execution uses the session's current explicit or native association. Unassociated background sessions still reject terminal execution.Execution, readiness and terminal selection share a registry with at most one terminal per session and one session per terminal. Explicit binding replaces either endpoint's previous association, so a superseded native terminal cannot return as a fallback when the explicit terminal closes. Live explicit bindings take priority over native attach. Close and connection replacement invalidate associations; queued sends validate the exact association, and delayed native discovery cannot overwrite intervening ownership changes or a newer connection. Reconnect refreshes the selected session's connection even after same-session reselection, while preserving a newer selection of a different session; old Workspace nodes still reject execution.
Downstream follow-up: vscode-R-console needs to pass its VS Code Terminal when activating a sess session, and repeat registration after reconnecting. Older vscode-R implementations ignore the extra argument, so downstream can retain compatibility with their existing activation API. No vscode-R-console sources or sess wire protocol are changed here.