Repository navigation
fix(agent): recover the NOW channel host - #2156
Marc-André Moreau (mamoreau-devolutions) merged 19 commits into
Conversation
When the NOW virtual channel failed after negotiation, for example because the peer tore the DVC down while the RDP connection stayed up, DevolutionsSession.exe exited. The agent only starts it again on the next logon or unlock, so the session had no NOW host until then. A channel that fails after negotiation is now reopened after a delay that starts at 1 s and doubles up to 10 s, with a fresh negotiation deadline per channel. Reopening gives up once five consecutive reopened channels fail to negotiate, and shutdown interrupts every wait. A first channel that never negotiates still ends the process, so a client without a NOW listener does not keep it alive. The IO thread of a failed channel must stop within 10 s, so that it releases its channel handle before the next open. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CreateProcessAsUserW hands lpCommandLine to the new process as its whole command line, and the C runtime reads the first token as argv[0]. The command line held only "--session <id>", so DevolutionsSession.exe took "--session" as its module name and never saw the flag, and its logs carried no session id. The command line now starts with the module path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A failure to enumerate processes while handling a logon or unlock returned an error from the session manager task. The service only logs a failed task, so the agent stopped starting DevolutionsSession.exe in any session until the service restarted. The failure is now logged and the session stays waiting, so the next logon or unlock retries. The process is not started without a lookup result, to avoid running two instances in one session. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The agent needs to tell a session process that failed from one that ended on purpose, so that it relaunches only the former. The process exits with a failure code when a task fails or panics, and with success otherwise. A client that never negotiates the first channel no longer counts as a failure, because it likely has no NOW listener and relaunching would not help. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The NOW channel handle is opened for overlapped IO, but messages were written with a synchronous `WriteFile` whose byte count was never checked, so a client slow to read stalled the loop, including its reads, and a short write went unnoticed. Writes are now overlapped, one at a time, and a short write ends the channel, because the rest cannot be sent as a separate DVC message. When the processing queue was full, the loop closed the channel. It now keeps the decoded messages and stops reading until the queue has room, which holds the client back instead. A read shorter than the channel PDU header ended the loop as if the channel had closed; it is now an error that reports the byte count. Pending reads and writes are cancelled, and waited for, before their buffers are freed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A process that writes output before reading its input blocks once its output pipe is full. Writing stdin on the thread that drains that output deadlocked both sides as soon as the input exceeded the stdin pipe buffer, and waiting for that write in the channel task also stopped it from forwarding process output and exit notifications. Stdin is now written by a thread of its own, fed through a bounded queue. The channel task holds back further client messages while a stdin send waits on that queue, which keeps input ordered and pushes back on the client, but keeps forwarding process events. A failed stdin write discards later input instead of failing the session; the process exit is still reported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The session process is started only on logon or unlock, so a crash or a failure to serve the NOW channel left the session without a channel host until the user logged on again. The agent now watches each process it starts. A nonzero exit while the remote session is still ready relaunches it after a delay that starts at 2 seconds and doubles up to 60 seconds per consecutive quick failure. After 5 quick failures it waits for the next logon or unlock. Exit code 0 means the process ended on purpose and is not relaunched. A launch id keeps the exit of a replaced, disconnected or logged-off process from starting another one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Discover active RDP sessions when the service starts and adopt an existing NOW host or launch a watched host without another logon. Recheck current session state when replaying remote-connect events, while retaining queued notifications during discovery. Wait for the previous host to exit before reconciling a reconnect. Keep incomplete termination retryable on later readiness events, and retry transient lookup or process-creation errors during recovery with bounded backoff instead of stranding a ready session. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A full child stdin queue must not stop transport-loss detection. Race message processing against an independent IO-loop notification so a blocked stdin send cannot prevent channel recovery. Publish that notification before waiting for pending IO cancellation. This starts the teardown deadline even if cancellation stalls, while keeping the buffers owned by the IO thread until completion. Preserve an already-completed normal protocol close as a successful host exit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Let maintainers know that an action is required on their side
|
|
Draft while controlled Windows RDP acceptance and test-oracle cleanup remain outstanding. Change weight against
Validation: 31 session-manager tests pass, including 15 consecutive runs of the complete targeted suite; all 41 session tests pass; targeted all-target Clippy passes with warnings denied. Windows incremental-cache permission warnings remain environmental. Both x64 executables build with the The startup regression was observed failing before discovery was added, and the queued-reconnect regression failed without the WTS recheck. Isolated mutations disabling the reconnect recheck, discarding queued SCM events, and disconnecting watcher notifications were caught by the respective regressions. The large-stdin regression also catches a mutant that stops draining output until the stdin writer finishes. UNTESTED properties requiring live checks:
Remaining limits: adopted hosts are not watched; abort/cancel messages behind backpressured stdin still wait; a reconnect termination timeout retains pending termination until another readiness event retries it; repeated failures eventually wait for logon/unlock rather than looping indefinitely. No candidate has been deployed by this change. The existing Production candidate: Agent SHA256 Note LLM-assisted content (no human feedback). |
While the processing queue is full, the IO loop issues no read, so a client that disconnects during stdin backpressure went unnoticed until the next heartbeat write, or forever when heartbeats are disabled, and the channel was never reopened. While messages are held back, write a heartbeat whenever no write has started for a second. A closed channel fails that write, which ends the IO loop and reopens the channel. No inbound message is discarded. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Wait for client-visible bytes with a deadline before reading the probe heartbeat, so a missing probe fails the test rather than blocking the test run on a synchronous pipe read. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The backlog probe sent heartbeats to every client, including one that negotiated a zero heartbeat interval. The NOW specification says zero disables periodic heartbeats and grants no permission for others. The processing task now publishes whether the negotiated capabilities enable heartbeats, and the IO thread probes a held-back channel only when they do. Clients that omit the interval keep the 60-second default and are still probed. A client that disables heartbeats and disconnects while its messages are held back is still not noticed until the processing task drains the backlog. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Return after every detached launch, including Process requests without temporary files. Previously those requests started the child and then panicked because attached IO setup requires a notification sender. Keep the background wait for temporary-file cleanup when needed, and cover both detached builder completion and subsequent execution on the same NOW channel. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The Run handler passed lpDirectory from a WideString consumed inside Option::map, so ShellExecuteW received a pointer into a freed buffer. Borrow the string instead so the buffer outlives the call. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Ending a NOW channel or renegotiating capabilities stops only the IO of attached execution sessions. Their processes keep running and no later channel can reach them. The NOW specification does not define the fate of execution sessions when the channel ends, so this behavior is kept and documented where the sessions are owned. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Transport-loss detection, pre-launch retries, and I/O teardown still have unresolved failure paths that can prevent recovery or retain resources.
4 open issues
What changed in this PR
This PR improves NOW channel availability by adding Agent-side host recovery and making session transport and child-process I/O more resilient.
Changes:
- Discover active RDP sessions at startup and recover failed hosts with bounded retries.
- Reopen lost channels, use overlapped I/O, and preserve inbound messages through backpressure.
- Separate child stdin writes from output draining and propagate task failures through process exit codes.
| File | Description |
|---|---|
devolutions-session/src/main.rs |
Return failure exit codes when session tasks fail. |
devolutions-session/src/dvc/task.rs |
Coordinate channel recovery, teardown, and pending stdin sends. |
devolutions-session/src/dvc/process.rs |
Move stdin writes to dedicated threads and test output/input progress. |
devolutions-session/src/dvc/io.rs |
Add overlapped writes, inbound backpressure, cancellation, and transport tests. |
devolutions-session/Cargo.toml |
Enable required Windows API features. |
devolutions-agent/src/session_manager.rs |
Discover sessions, watch hosts, and schedule recovery. |
crates/win-api-wrappers/src/wts.rs |
Identify sessions using RDP. |
crates/win-api-wrappers/src/process.rs |
Support waiting for process termination before replacement. |
Review effort: Balanced
An attached execution started through NOW kept running after its channel ended, whether the client closed it, the transport was lost, the client renegotiated, or the host exited. No later channel can reach such a process, so every disconnect during a running execution left it, and anything it started, in the user's session. Attached executions now start suspended, join a job object owned by the channel's message processor, and only then run, so every process they start joins the job too. The job kills its processes when its last handle closes: when the processor is dropped at channel end or renegotiation, and when the host exits or crashes. A process that cannot join the job is killed and the execution fails. Detached executions stay outside the job and keep running. Processes left running by an attached execution that already exited are now killed when the channel ends as well. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the scaffold and bare mutant markers on the NOW host and session manager tests with the fault each test catches, and cite the CreatePipe remarks for the blocking anonymous-pipe write. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Lines changed against
Tests are counted from each Rust file's Note LLM-assisted content (no human feedback). |
|
Properties no test checks, each marked
Killing an attached execution tree when the host itself crashes relies on the OS closing the job handle; the tests cover only channel loss inside a running host. Note LLM-assisted content (no human feedback). |
Retry pre-launch recovery after transient lookup, launch, and termination failures. Bound disabled-heartbeat DVC backpressure and cancel blocked synchronous stdin writers during teardown. Treat an already-signaled process handle as completed termination. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Change weight in
Note Human-tuned, LLM-assisted content. |
c9943d9
into
master


Keep NOW available across Agent restarts, RDP reconnects and transport failures. Discover active remote sessions at service startup and launch missing hosts under the user's token, or adopt hosts that already run. Wait for the previous host to terminate before replacing it, and retry failed recovery attempts with bounded backoff.
Reopen lost dynamic virtual channels, use overlapped reads and writes, and apply backpressure instead of dropping inbound messages. Write child stdin independently of output draining so a child that writes output before reading input does not deadlock the host.
Preserve normal channel shutdown as a successful host exit and relaunch watched hosts only after failures. Report malformed or short channel reads as errors, observe transport loss even while stdin is blocked, and bound I/O teardown before opening another channel.
While inbound messages are held back, no read is pending, so a lost client would go unnoticed. Probe such a channel with heartbeats only when the client enables them; otherwise no probe is sent. If the inbound backlog makes no progress, fail the channel after 30 seconds.
End attached executions with their channel. Each starts suspended and joins a kill-on-close job owned by the channel, so it and every process it starts are killed when the channel ends, is renegotiated, or the host exits. A process that cannot join the job is killed and the execution fails. Detached executions stay outside the job and keep running. Processes left running by an attached execution that already exited are killed with the channel too.
Return right after detached execution launches instead of setting up attached IO, which panicked the host for detached Process requests without temporary files. Keep the Run directory string alive while ShellExecuteW reads it.
Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com