Add support for using terminals with Containers and Executables by karolz-ms · Pull Request #173 · microsoft/dcp

Karol Zadora-Przylecki (karolz-ms) · 2026-05-28T18:31:07Z

Sorry for the size... 😬

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

David Negstad (danegsta) · 2026-05-28T21:32:05Z

+	// Socket file lifecycle is owned by the caller, not by ConnManager. Without
+	// this opt-out, net.UnixListener.Close() would unlink the path because Go
+	// created it via ListenUnix.
+	listener.SetUnlinkOnClose(false)


The contract here puts UDS socket cleanup on the caller (SetUnlinkOnClose(false) + the comment block above), but no production caller actually unlinks the file. I grepped — os.Remove on the UDS path only happens in test helpers. Concretely, neither controllers/running_container_data.go closeTerminalResources nor internal/exerunners/process_executable_runner.go closeProcessRunResources/waitForConnManagerShutdown calls os.Remove(terminalSpec.UDSPath).
The consequences are user-visible: delete a terminal Executable/Container and recreate it with the same UDSPath and NewConnManager will fail with bind: address already in use → the resource goes to FailedToStart. The file also persists after DCP exits cleanly.
The natural place to fix this is in the cleanup paths after connMgr.Done() resolves — that's also why the executable runner's waitForConnManagerShutdown exists. (The container path doesn't even wait on Done() today; see the field comment on runningContainerData.connMgr which already claims it does.)

I am going to update that comment on the runningContainerData.connMgr member. But other than that, I am not sure what is the problem here. The contract is, DCP does not create the socket file, the client does, and that means DCP does not delete the socket file either.
And no, If a Container is deleted, and a new Container is created afterwards that uses the same socket path, the socket will be reused, there is no "address in use" problem becasue part of the old Container cleanup is closing the server connection to the socket.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

David Negstad (danegsta) · 2026-05-28T23:16:08Z

Conceptually, should we consider making the new pty controller be invoked via a child command rather than in the controller process directly? That'd be necessary to set us up for future support of persistent resources.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

… startup failure

Copilot

Pull request overview

Copilot reviewed 74 out of 77 changed files in this pull request and generated 5 comments.

Files not reviewed (2)

api/v1/zz_generated.deepcopy.go: Language not supported
api/v1/zz_generated.model_name.go: Language not supported

Copilot

Pull request overview

Copilot reviewed 74 out of 77 changed files in this pull request and generated 5 comments.

Files not reviewed (2)

api/v1/zz_generated.deepcopy.go: Language not supported
api/v1/zz_generated.model_name.go: Language not supported

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Mitch Denny (mitchdenny) · 2026-06-07T00:38:27Z

So the surprise change I springed on you around having DCP connect to the Terminal Host. I feel like I need to explain why that is necessary (and why we might not want a HMP1 listener in DCP at all).
One of the issues that we'll run into with terminal emulation is timing. Programs can send queries to the terminal emulator to determine settings like mouse support and other terminal capabilities. Sometimes these queries are time sensitive (if you don't get a response in N microseconds fallback to some undesirable compat mode). So to help mitigate that it makes more sense for DCP to first open the connection to the terminal host and make sure that the pathway is cleared before launching the process under DCP.

Roger. Currently the order of execution is, first start a process connected to a pseudoterminal, then make HMP1 connection to terminal host. I think it should be possible to do a focused change and introduce a new mode where this order is inverted. This should minimize the delay of the terminal host responding to program inquiries. Investigating...

On related note, I am going to add another mode that has DCP own the terminal socket. David and I don't think the Aspire app host owning the socket will work for persistent Executables and Containers.

Thinking about it some more, I think I will just change the existing "listen" mode to have DCP own the socket file. So "listen" owns the socket file, and "connect" does not. But that will be a separate PR.

Co-authored-by: David Negstad <50252651+danegsta@users.noreply.github.com>

Karol Zadora-Przylecki (karolz-ms) · 2026-06-09T17:34:08Z

/backport to release/0.25

github-actions · 2026-06-09T17:35:09Z

Started backporting to release/0.25: https://github.com/microsoft/dcp/actions/runs/27224217847

github-actions · 2026-06-09T17:35:14Z

Karol Zadora-Przylecki (@karolz-ms) backport PR couldn't be created automatically, please create the backport PR manually!

Open backport PR into release/0.25.

Add support for using terminals with Containers and Executables
b019382

CopilotAI review requested due to automatic review settings May 28, 2026 18:31

Copilotstarted reviewing on behalf of Karol Zadora-Przylecki (karolz-ms)May 28, 2026 18:31 View session

Karol Zadora-Przylecki (karolz-ms) requested a review from David Negstad (danegsta)May 28, 2026 18:32

CopilotAI reviewed May 28, 2026

Karol Zadora-Przylecki (karolz-ms) requested review from Jose Perez Rodriguez (joperezr) and Mitch Denny (mitchdenny)May 28, 2026 21:05

David Negstad (danegsta) reviewed May 28, 2026
View reviewed changes

Comment threadtest/integration/executable_controller_test.go

David Negstad (danegsta) reviewed May 28, 2026
View reviewed changes

Comment threadinternal/termpty/pty_linux.go

David Negstad (danegsta) reviewed May 28, 2026
View reviewed changes

Comment threadinternal/docker/cli_orchestrator.go Outdated

David Negstad (danegsta) reviewed May 28, 2026
View reviewed changes

Comment threadinternal/podman/cli_orchestrator.go Outdated

David Negstad (danegsta) reviewed May 28, 2026
View reviewed changes

Comment threadinternal/docker/cli_orchestrator.go

Karol Zadora-Przylecki (karolz-ms) added 2 commits May 28, 2026 15:15

Fix test
fa037d3

Fix another test
ae810da

CopilotAI review requested due to automatic review settings May 28, 2026 22:22

Copilotstarted reviewing on behalf of Karol Zadora-Przylecki (karolz-ms)May 28, 2026 22:23 View session

Karol Zadora-Przylecki (karolz-ms) mentioned this pull request May 28, 2026
Add Windows PTY support for executables and containers (HMP v1 over UDS) #133
Closed

Update codegen
099a55c

CopilotAI reviewed May 28, 2026

David Negstad (danegsta) reviewed May 28, 2026
View reviewed changes

Comment threadinternal/termpty/pty_darwin.go

Karol Zadora-Przylecki (karolz-ms) added 4 commits May 29, 2026 11:44

Make test more reliable
c48f240

Add instructions about commenting code
74c8bf5

Re-add tests deleted by mistake
61b86ce

HMP1 server differentiates between client disconnecting and process exit
2c79f26

CopilotAI review requested due to automatic review settings May 29, 2026 23:12

Copilotstarted reviewing on behalf of Karol Zadora-Przylecki (karolz-ms)May 29, 2026 23:12 View session

CopilotAI reviewed May 29, 2026

Ensure container attach (PTY) process is terminated upon conn manager…
088e9e0
… startup failure

CopilotAI reviewed Jun 3, 2026
View reviewed changes

Comment threadcontrollers/container_controller.go Outdated
Comment threadcontrollers/container_controller.go Outdated
Comment threadinternal/exerunners/process_executable_runner.go
Comment threadinternal/exerunners/process_executable_runner.go Outdated
Comment threadpkg/process/os_executor.go

Karol Zadora-Przylecki (karolz-ms) added 3 commits June 3, 2026 12:58

Handle PTY closure in HMP1 server
4405e00

Better terminal spec validation
a706ccc

Fix ambiguous comment
27ab8e0

CopilotAI review requested due to automatic review settings June 3, 2026 20:01

Copilotstarted reviewing on behalf of Karol Zadora-Przylecki (karolz-ms)June 3, 2026 20:02 View session

Got dimensions backward
ffdb104

CopilotAI reviewed Jun 3, 2026
View reviewed changes

Comment threadtest/termchild/termchild_windows.go Outdated
Comment threadcontrollers/container_controller.go
Comment threadinternal/testutil/ctrlutil/test_container_orchestrator.go Outdated
Comment threadinternal/termpty/conn_manager.go Outdated
Comment threadAGENTS.md Outdated

Karol Zadora-Przylecki (karolz-ms)and others added 2 commits June 3, 2026 13:09

More dimension confusion
afbee50

Right-size Windows INPUT_RECORD struct
a2d0ed2
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI review requested due to automatic review settings June 3, 2026 20:15

Copilotstarted reviewing on behalf of Karol Zadora-Przylecki (karolz-ms)June 3, 2026 20:15 View session

CopilotAI reviewed Jun 3, 2026

Karol Zadora-Przylecki (karolz-ms)and others added 3 commits June 3, 2026 13:49

Improve test container orchestrator (process exit reporting)
bc64c4b

Fix typo
afff294
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Update comment
d8e0892
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Karol Zadora-Przylecki (karolz-ms)force-pushed the dev/karolz/with-terminal branch from a3e5472 to d8e0892Compare June 3, 2026 20:50

Mitch Denny (mitchdenny) reviewed Jun 7, 2026
View reviewed changes

Establish HMP1 connection before starting terminal-using process
9020a5d

David Negstad (danegsta) reviewed Jun 8, 2026
View reviewed changes

Comment threadAGENTS.md Outdated

David Negstad (danegsta) approved these changes Jun 8, 2026
View reviewed changes

Karol Zadora-Przylecki (karolz-ms)and others added 2 commits June 8, 2026 16:26

Update agent instructions
f2bb540
Co-authored-by: David Negstad <50252651+danegsta@users.noreply.github.com>

Fix wording in agent instructions
212bd8b

Karol Zadora-Przylecki (karolz-ms) enabled auto-merge (squash) June 8, 2026 23:30

Karol Zadora-Przylecki (karolz-ms) merged commit c650b2a into mainJun 8, 2026
11 checks passed

Karol Zadora-Przylecki (karolz-ms) deleted the dev/karolz/with-terminal branch June 8, 2026 23:46

Karol Zadora-Przylecki (karolz-ms) mentioned this pull request Jun 9, 2026
[release/0.25] Add support for using terminals with Containers and Executables #183
Merged

Uh oh!

Conversation

Karol Zadora-Przylecki (karolz-ms) commented May 28, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

David Negstad (danegsta)May 28, 2026

Choose a reason for hiding this comment

Uh oh!

Karol Zadora-Przylecki (karolz-ms)May 29, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

David Negstad (danegsta) commented May 28, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Uh oh!

Mitch Denny (mitchdenny)Jun 7, 2026

Choose a reason for hiding this comment

Uh oh!

Karol Zadora-Przylecki (karolz-ms)Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Karol Zadora-Przylecki (karolz-ms)Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Karol Zadora-Przylecki (karolz-ms)Jun 8, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Karol Zadora-Przylecki (karolz-ms) commented Jun 9, 2026

Uh oh!

github-actionsBot commented Jun 9, 2026

Uh oh!

github-actionsBot commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Karol Zadora-Przylecki (karolz-ms)May 29, 2026•
edited
Loading

Karol Zadora-Przylecki (karolz-ms)Jun 8, 2026•
edited
Loading