fix: bound WinRM command.Wait() and remotefs upload against dead sessions

[james-nesbitt]

What

Bound every point where a WinRM command or a remote-file transfer could block forever on a session that has silently died.

Why

Fixes #472. Observed as a 40+ minute hang in Mirantis/launchpad's smoke-windows CI, right after a Windows host reboot, during a remotefs.Upload call as part of an MCR uninstall. A goroutine dump from the test's own timeout panic pinned both stuck points precisely (see the issue for the full stacks).

How

protocol/winrm

• command.Wait() takes the ctx StartProcess already receives and races it against completion. context.Background() callers see unchanged (unbounded) behaviour; callers with a deadline are now honoured.
• Giving up needs care: the goroutines copying the command's output outlive Wait(). StartProcess now wraps the caller's writers in a detachableWriter that Wait() cuts loose before returning, and closes the command and the shell in the background — in separate goroutines, so a close that never returns cannot strand the other — since each is a synchronous request to a host that has very likely stopped answering.
• Detaching is deliberately non-blocking, so it cannot interrupt a write already inside the caller's writer: an io.Writer offers no way to do that, and waiting for one would make Wait() unbounded again. StartProcess's documentation now states the contract — when Wait() returns a context error the command is still running, and the caller must not recycle or concurrently read the writers it passed in.
• Rig's own writers are made safe for that window rather than relying on it being narrow: cmd.ExecOutputContext drops its stdout buffer instead of pooling it when the context expired; the stderr buffer behind ExecOptions.ErrString — read by waiterWrapper.Wait the moment Wait returns — is mutex guarded; and iostream.ScanWriter and redact.Writer, the per-line trace writers closed at that same moment, guard the state Write and Close share. Closing them therefore waits for a write in flight, so on the abandoned path waiterWrapper.Wait hands that to a goroutine instead of doing it itself — they still get closed, releasing the scanner goroutine each one owns, just not on the caller's time.
• Command input is copied through a context-aware reader, so that goroutine stops at the next read boundary instead of outliving an abandoned command.

remotefs

• winFile had no bound anywhere: not on writing a rigrcp request, not on waiting for its response, and not on the payload transfers in Write, Read and CopyTo that happen between two round trips. All are now bounded by commandTimeout (30s, a var so tests can lower it).
• It is an idle timeout, not a transfer budget: the countdown restarts on every step forward, and Write chunks its payload so one large call reports progress as it goes. A slow but progressing multi-megabyte upload is not killed for taking longer than 30s. What counts as a step differs by direction, and commandTimeout documents it: a round trip completing, each successful read of a payload coming back, but on the way out only a whole chunk handed to the connection — io.PipeWriter.Write does not return before that — so the write side has a floor of one payloadChunkSize per commandTimeout, roughly a kilobyte a second.
• Hitting the timeout means the session is dead, so winFile.abort() tears it down instead of leaving it half-open. Closing the far ends of the pipes releases everything parked on them (the response reader, a payload write, a payload read) with the timeout error rather than leaking those goroutines; cancelling the command context stops the remote helper; the file is marked closed so later operations fail fast.
• New exported remotefs.ErrTimeout so callers can recognise this failure mode. It wraps context.DeadlineExceeded, so errors.Is(err, context.DeadlineExceeded) matches too.
• The watchdog decides whether to abort from the time of the last progress, under its own lock, rather than leaving it to the timer: neither Stop nor Reset has any effect on an AfterFunc callback that has already begun, and such a callback can sit waiting for that lock while the operation it covers finishes or moves on. Without this, a watchdog expiring in a photo finish could tear down a session that had just succeeded, or one that had just reported progress.
• Errors from a rigrcp round trip no longer carry an fs.PathError of their own, leaving the public operation to name itself: a stalled Read reports read rather than open, and callers that already wrapped these errors stop nesting two path errors.
• rigrcp.ps1 accumulates a w N payload up to the declared count. $in is a FileStream over stdin, so Read returns as soon as any data is there, and a payload larger than one transport chunk has always arrived in several — the single-Read short-read check could reject a perfectly good WinFS.WriteFile. Predates this PR (bytes.Reader implements io.WriterTo, so WriteFile hands over the whole payload at once), surfaced while reviewing it.
• Upload skips its temp-file cleanup after a session timeout: RemoveAll would run against the connection that just died and block there, leaving the public call unbounded on the very path this adds.
• For that to work the timeout has to survive the trip out. winFile records why it aborted, because closing the pipes is what releases a parked transfer but io.Pipe keeps the first close error it is given — a transfer that raced the rigrcp command exiting came back with a plain closed pipe, and Upload would not have recognised the timeout it has to skip cleanup for. copyAndVerifyUpload joins a failed close onto a failed copy rather than dropping it, and WinFS.ReadFile/WriteFile/ReadDir report a close that timed out instead of discarding it in a defer, so they cannot return success over a session that has gone.
• Close reports a quit that timed out instead of only logging it, and waits for the helper to actually exit rather than treating the local write as proof it shut down. The file is closed by then either way, but the session may not be, and Upload goes straight from there to checksumming the file it just wrote.
• rigrcp.ps1's quit handler called Close-Dipose; the function is Close-Dispose. Every quit threw, got caught by the loop's handler and emitted a stray error object instead of disposing the streams. Harmless before — nothing read a response to q — but not something to leave in place now that Close waits on that exit.
• winFileDirBase.closed becomes an atomic.Bool — it was already written from the goroutine watching the rigrcp command exit while callers read it, and the watchdog adds a third goroutine.

Testing

• remotefs/winfile_internal_test.go: each previously unbounded point (command write, response wait, Write payload, Read payload) returns ErrTimeout in well under a second instead of hanging, and each asserts the session was actually torn down. Two positive tests pin the idle-vs-budget semantics: a slow but steady Write and CopyTo that both outlast commandTimeout still complete. Two tests pin the watchdog photo finish, one per direction: neither stop() nor progress() loses to a callback that has already begun. Happy path and "session ended" path unaffected.
• protocol/winrm/connection_test.go: command now holds the shell and the command behind the small unexported interfaces its own methods use, rather than *winrm.Shell/*winrm.Command directly — seam enough to stand a fake in, since the concrete types talk SOAP to a real host. Both halves of Wait are covered: finishing inside the deadline, exit codes, waiting for the output copies, and the deadline passing first (prompt context error, writers out of the caller's reach by the time Wait returns, teardown landing in the background). Without the fix that last test does not fail — it hangs, which is WinRM command.Wait() and remotefs upload can hang forever on a dead session #472's own shape. Also covered: detachableWriter (including that detach() returns with a write parked inside the caller's writer) and ctxReader.
• go build, go vet, go test -race ./... and golangci-lint run ./... all clean.
• End-to-end validation is via the launchpad smoke-windows re-run this was found on.

Written by AI: claude-sonnet-5, claude-opus-5

[Copilot]

🟡 Changes recommended

Unresolved timeout coverage, cleanup, and payload-write blocking issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR bounds WinRM command waits and remote-file control operations to prevent indefinite hangs after dead sessions.

Changes:

• Propagates contexts into WinRM command waits.
• Adds a 30-second rigrcp command timeout.
• Adds timeout, success, and ended-session tests.

File summaries

File	Review findings
remotefs/winfile.go	Critical: payload writes can still block indefinitely. Moderate: timed-out operations may leave goroutines and the file daemon active; timeout errors lack a public sentinel.
remotefs/winfile_internal_test.go	Nit: add coverage for a blocked writer and its cleanup.
protocol/winrm/connection.go	Applies caller context deadlines to command waits.

Review details

Suppressed comments (2)

remotefs/winfile.go:279

• On a timeout after the file daemon has been opened, this returns without canceling f.cancel or closing the pipe. The response reader goroutine above (and a writer goroutine when the write itself is the operation that timed out) can therefore remain parked forever, while the WinRM command was started with a cancellable-but-not-deadlined context. Cancel/tear down the daemon on this failure so the per-command goroutines are released and the file is not left half-open.

	case <-ctx.Done():
		return nil, f.pathErr(OpOpen, fmt.Errorf("%w: writing rcp command %q", errRcpTimeout, cmd))

remotefs/winfile_internal_test.go:29

• This test uses io.Discard for stdin, so fmt.Fprintf can never block and the newly added write-timeout path (and its blocked-writer cleanup) is untested; it only covers waiting for a response. Add a separate case with an unconsumed io.Pipe writer or another blocking writer, while retaining this response-timeout case.

		stdin:  nopWriteCloser{io.Discard},
		stdout: bufio.NewReader(stdoutR),

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

WinRM abandonment can still block or leak resources, and timeout handling and test timing need correction.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

remotefs/winfile.go:352

• timedOut hard-codes OpOpen, but command is also used by Read (which returns this error directly). A read that hits the new timeout therefore exposes an fs.PathError whose Op is open, contradicting the filesystem convention that the operation names the public call (remotefs/patherror_test.go:10-19). Return the timeout cause here and let each public operation add its own path error, as Read and the other callers require.

func (f *winFile) timedOut(what string) error {
	err := fmt.Errorf("%w: %s", ErrTimeout, what)
	f.abort(err)
	return f.pathErr(OpOpen, err)

remotefs/winfile_internal_test.go:213

• This test sleeps for exactly half of the 100 ms idle timeout before each chunk, so scheduler/GC jitter can make the gap between progress reports reach the timeout and fire the watchdog before the next read. With three chunks the test is already intended to outlast the timeout; use more chunks (or otherwise leave a margin per interval) while keeping the total transfer longer than timeout.

		timeout = 100 * time.Millisecond
		chunks  = 3

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved writer-lifetime, cleanup, timeout-race, and buffer-handling issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

protocol/winrm/connection.go:365

• This changes Wait() so it can return while the remote command and its copying goroutines are still running, but the exported StartProcess documentation below still says that its Wait() blocks until the command finishes. Update the public contract (and the Waiter documentation if applicable) to explain the early context return and the resulting writer-lifetime requirement.

// Wait blocks until the command finishes or ctx (the context StartProcess was
// called with) is done, whichever happens first.
//
// Without this, a command whose underlying WinRM connection has silently
// died -- for example the host rebooted mid-command and this shell was
// never actually torn down on this end -- blocks forever: neither
// c.wg.Wait() nor the underlying masterzen/winrm Command.Wait() it calls
// take a context or have any internal timeout of their own. If ctx has no
// deadline this preserves the previous unbounded behaviour; callers that
// want a bound must set one on the context passed to StartProcess.

protocol/winrm/connection.go:352

• The two cleanup requests are issued serially even though this path is specifically for an unresponsive host. If c.cmd.Close() blocks on the dead WinRM session, c.sh.Close() is never attempted, leaving the shell cleanup behind indefinitely. Run the cleanup requests independently or give each its own bounded cancellation path.

	go func() {
		_ = c.cmd.Close()
		_ = c.sh.Close()
	}()

remotefs/winfile.go:120

• Timer.Stop does not wait for an AfterFunc callback that has already started, so the callback can still call abort after the transfer returns successfully. If expiry races with the final read/write, this marks the file closed and closes its pipes, causing a subsequent operation to fail with fs.ErrClosed despite the completed operation. Synchronize callback execution with stop (or use an active/generation check) so an operation cannot be aborted after it has ended.

func (w *watchdog) stop() {
	w.timer.Stop()
}

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect timeout coverage and concurrency safety.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

protocol/winrm/connection.go:351

• On the context-error path, abandon lets Wait return while the stdout/stderr copy goroutines can still be inside the writers. waiterWrapper.Wait then immediately closes its traceClosers (for example, redactWriter/iostream.ScanWriter), and detach cannot prevent an already-started write. Those writers are not concurrency-safe, so cancellation with output logging enabled can race their buffers/closed flags or block while the copy is still using them. Owned closers need to be shut down only after the copy goroutines have exited, or made safe for this lifetime.

func (c *command) abandon() error {
	c.stdout.detach()
	c.stderr.detach()
	go func() { _ = c.cmd.Close() }()
	go func() { _ = c.sh.Close() }()

protocol/winrm/connection.go:419

• The context-error path can also leave the goroutine copying stdin running: ctxReader only notices cancellation before the next read, and a read already in progress may continue after Wait returns. The contract warns callers not to reuse the output writers but says nothing about reusing or closing the input reader, so callers can still race that goroutine; document the same lifetime restriction for stdin (or make the input side part of the teardown guarantee).

// that recycles or concurrently reads the writers it passed in must therefore
// not do so when Wait reports a context error.

remotefs/winfile.go:215

• f.stdin is the io.PipeWriter created in open, whose Write does not return until the entire slice has been consumed. Thus dog.progress() is not called while a 32 KiB chunk is transferring: a steadily progressing connection slower than about 1 KiB/s can spend over 30 seconds in one Write and be aborted as stalled, contrary to the documented idle-timeout behavior. The watchdog needs to observe partial consumption rather than only chunk completion.

	dog := f.watch(fmt.Sprintf("writing %d byte payload", len(p)))
	defer dog.stop()
	written := 0
	for written < len(p) {
		n, err := f.stdin.Write(p[written:min(written+payloadChunkSize, len(p))])

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Two unresolved moderate findings require correction before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

remotefs/winfile.go:511

• If the final q round-trip times out, command aborts the session and returns ErrTimeout, but this code logs that error and still returns nil. In the upload path, remote.Close() can therefore report success and the caller proceeds to Sha256 on a dead WinRM session, potentially re-entering an unbounded background command instead of seeing the timeout. Propagate the q error after marking/closing the file.

	_, err = f.command("q")
	log.Trace(context.Background(), "rigrcp quit", log.ErrorAttr(err))
	f.stdin.Close()
	f.closed.Store(true)

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Cleanup can still block on writer closure, and abandoned-command teardown may accumulate goroutines.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

protocol/winrm/connection.go:351

• Each abandoned command starts two teardown goroutines, and the comments explicitly allow either Close call to never return. Repeated context-bounded commands against a dead host therefore permanently accumulate Wait/close goroutines and their captured shell/command state, eventually exhausting process resources even though callers no longer hang. Please add connection-level tracking or a bounded/deduplicated teardown strategy before treating this as a general fix.

func (c *command) abandon() error {
	c.stdout.detach()
	c.stderr.detach()
	go func() { _ = c.cmd.Close() }()
	go func() { _ = c.sh.Close() }()

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Add coverage for command.Wait cancellation paths and correct the misleading writer-lifetime comment.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

protocol/winrm/connection.go:286

• This new internal comment says the caller is entitled to reuse its buffers as soon as Wait returns, but the StartProcess contract below explicitly forbids recycling, closing, or concurrently using those writers when Wait returns a context error. Since detach cannot cover a write that already passed the atomic check, the sentence is misleading and should describe the caller's required lifetime instead.

// those goroutines outlive Wait, while a caller is entitled to reuse the
// buffers it passed in as soon as Wait returns.

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical issues remain with unbounded upload cleanup and incompatible chunked payload handling.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The positive transfer test concurrently reads one pipe through separate readers, making it flaky and potentially incorrect.

Review details

Suppressed comments (1)

remotefs/winfile_internal_test.go:254

• This test starts serve's reader on s.commands, then drains payload bytes from the separate s.requests reader, but both fields wrap the same stdinR pipe. The serveAndDrain helper is likewise concurrently reading s.commands, so payload bytes can be consumed and discarded by one reader while the test waits on the other, making this positive transfer test flaky or falsely exercising a different stream than the real helper. Consume the command and subsequent payload from one reader (the helper's single stdin stream) instead of having concurrent readers.

		<-served // s.requests is serve's until it has read the command
		var total int64
		for range chunks {
			time.Sleep(timeout / 4)
			n, err := io.CopyN(io.Discard, s.requests, payloadChunkSize)
			total += n

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved timeout propagation and error-handling issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

remotefs/upload.go:109

• The timeout guard only sees the error returned by copyAndVerifyUpload, but that helper discards remote.Close() errors when CopyFrom already failed. If the source fails and the subsequent close hits the new session timeout, failure remains the source error, so this defer calls RemoveAll on the dead connection and can hang forever. Propagate or otherwise record an ErrTimeout from that close before relying on this check.

	var failure error
	defer func() {
		if errors.Is(failure, ErrTimeout) {
			return
		}
		_ = fsys.RemoveAll(tmpPath)
	}()

	if failure = copyAndVerifyUpload(fsys, tmpPath, local); failure != nil {

remotefs/winfile.go:519

• Although Close now returns ErrTimeout, the Windows convenience helpers still call it via defer f.Close() and discard the result (winfs.go:103,309,323). Thus ReadFile, WriteFile, and ReadDir can report success after the quit timed out and the WinRM session died; propagate the close error in those helpers (or otherwise ensure a timeout is not swallowed).

	// The file itself is closed by now, so a helper that merely exited ahead
	// of the quit is not a failure. A quit that timed out is: the session
	// died rather than shut down, and the caller would otherwise carry on
	// against a dead host -- Upload goes straight from here to checksumming
	// the file it just wrote.
	if errors.Is(quitErr, ErrTimeout) {
		return f.pathErr(OpClose, quitErr)

• Files reviewed: 12/12 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Transfer progress is reported only after a full chunk completes, which can incorrectly trigger the idle timeout on slow, continuously progressing links.

Review details

Suppressed comments (1)

remotefs/winfile.go:256

• io.PipeWriter.Write does not return until the entire 32 KiB slice has been consumed, so dog.progress() is not called when individual bytes move; it is called only after a whole chunk completes. A continuously progressing link that takes more than commandTimeout to deliver one chunk is therefore aborted as idle, despite the documented idle-timeout contract and the claim that slow transfers may outlast the timeout. Progress needs to be reported from a lower-level write/read boundary (or the protocol needs an explicitly documented per-chunk minimum rate), rather than only after this blocking write returns.

		n, err := f.stdin.Write(p[written:min(written+payloadChunkSize, len(p))])

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved timeout and cleanup paths can still hang or incorrectly skip temporary-file removal.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

remotefs/upload.go:117

• The cleanup decision is based on errors.Is(failure, ErrTimeout) after aggregating every CopyFrom error. A source reader is allowed to return this exported sentinel itself; io.Copy then propagates it through remote.CopyFrom while the remote file can still close successfully, but Upload will incorrectly skip removing the temporary file. Keep the session-timeout signal separate from the copy error (for example, derive it from the remote close/typed session error) instead of classifying the aggregate error by sentinel alone.

	var failure error
	defer func() {
		if errors.Is(failure, ErrTimeout) {
			return
		}
		_ = fsys.RemoveAll(tmpPath)
	}()

	if failure = copyAndVerifyUpload(fsys, tmpPath, local); failure != nil {

remotefs/winfile.go:315

• io.CopyN can block inside dst.Write after src.Read has reported progress. If callers pass a backpressured writer such as an io.PipeWriter with no reader, the watchdog will abort and close the remote pipes after commandTimeout, but that cannot interrupt the caller's writer, so CopyTo still never returns. Run the copy behind a cancellation/return boundary or explicitly define and enforce a non-blocking destination contract; as written this does not bound all CopyTo waits.

		n, err := io.CopyN(dst, src, resp.N-total)
		total += n
		if err != nil {
			return total, f.pathErr(OpCopyTo, fmt.Errorf("copy: %w", f.abortCause(err)))

• Files reviewed: 14/14 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Broad concurrent lifecycle and teardown changes warrant final human review.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

[kke]

Ok that grew quite dramatically..