Repository navigation
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two client lifecycle paths can leave an interactive connection alive after it should have ended:
term.Bridge()documents that stdin EOF ends the bridge, but its stdin goroutine simply returns on EOF or read/input error. The main loop keeps waiting on remote output/context, so the caller never reaches its deferred shell cleanup and keepalives can continue indefinitely.Shell.Close()synchronously sends terminal-close and app-termination frames before closing the transport. A stalled write, or another input write holding the send mutex, can preventClose()from ever reaching transport closure. Cancelling the bridge alone cannot unblock that path.Changes
Remote.Input()failures to the bridge through a buffered result channel. EOF and the existing~.escape return normally; other errors propagate to the caller.Shell.Close()is invoked, before acquiring the send mutex or attempting cleanup writes. It closes the transport if cleanup is still blocked, which releases the pending socket write. Stop the timer after cleanup and close the transport on every path. Healthy connections still send both cleanup frames and close immediately; they do not wait two seconds.sync.Oncebehavior andClose() errorcontract: graceful and forced closure both return nil. The timer is not a lifetime limit for an active session.If the timer wins before the cleanup frames are delivered, server cleanup uses its existing disconnect/recovery timeout. A 2-second client close does not promise remote process termination within two seconds.
Tests included
go test -race -count=1 ./... go vet ./... go build ./...All pass on this PR checkout (Go 1.27.2, macOS); gofmt also passes. Six added regression tests cover:
Close()interrupting a blocked input write that owns the send mutex;Close()interrupting its own stalled cleanup frame;The included tests use fake transports and local pipes; they need no account or remote service.
Additional integration validation performed
These completed experiments use the separate DWAgent cleanup patch in dwservice/agent#75. They are not run by the unit-test command above, and their remote-process result depends on the server fix too:
sleepgoneAll five scenarios passed with the race detector. Each observed the actual shell/job before closing and asserted zero tested processes, managers, pending children and shell activity before fallback cleanup. The harness used the real shell/session method bodies; account/relay bootstrap was substituted, and it made no global network changes.
The rebuilt client was also tested through the real DWService relay against the patched root agent: 16/16 cases comprising ten immediate open/close cycles, command success, remote exit 7, stdin EOF, command timeout, client SIGINT and client SIGKILL. Every case observed actual shell PIDs; tested foreground jobs were observed as well. No tested shells/jobs or PTY masters remained, activity counts returned to zero, and agent heartbeat/process identity stayed healthy. SIGKILL necessarily bypasses client cleanup; remote recovery was verified separately. These relay tests originated on the same Mac that hosted the remote agent, not on a second client host.
Behavior and limits
Interactive stdin EOF now disconnects as documented. Piping input into interactive mode may therefore close before all remote output has arrived; use
-cfor command execution/output capture. This change does not add a whole-operation timeout or change signal handling. A separate server lock/reaping defect was reproduced during diagnosis; this client patch cannot repair a deadlocked DWAgent by itself. Conversely, the server fix should protect cleanup for clients without this patch.