Skip to content

Don't cancel run context on stdin EOF#296

Open
llogen wants to merge 1 commit intomainfrom
fix/dutctlRPC
Open

Don't cancel run context on stdin EOF#296
llogen wants to merge 1 commit intomainfrom
fix/dutctlRPC

Conversation

@llogen
Copy link
Contributor

@llogen llogen commented Mar 12, 2026

The send goroutine's defer cancel() caused the receive goroutine to be killed immediately when stdin was /dev/null (e.g. when run non-interactively), resulting in no output being printed. Only the receive goroutine should drive termination.

The send goroutine's defer cancel() caused the receive goroutine to be
killed immediately when stdin was /dev/null (e.g. when run non-interactively),
resulting in no output being printed. Only the receive goroutine should
drive termination.

Signed-off-by: llogen <christoph.lange@blindspot.software>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes dutctl run streaming RPC lifecycle so that stdin reaching EOF (e.g., non-interactive /dev/null) does not prematurely terminate the RPC receive path, ensuring server output is still printed.

Changes:

  • Introduce a recvDone signal to let the receive goroutine drive overall termination.
  • Remove cancel() from the send goroutine teardown to prevent stdin EOF from cancelling the run context.
  • Update the final wait/termination select to wait on receive completion instead of runCtx.Done().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +231 to 236
// Send routine — stdin EOF must not cancel the context; only the receive
// routine drives termination so that all server responses are processed.
go func() {
defer cancel()

reader := bufio.NewReader(app.stdin)

for {
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that stdin EOF no longer cancels the context (good), the send routine still exits on io.EOF without half-closing the client->server request stream. Server-side code treats client stream closure (io.EOF on Receive) as the signal that no more requests are coming; consider explicitly closing the request side (e.g., stream.CloseRequest()/equivalent for connect.BidiStreamForClient) when stdin reaches EOF so the server can stop waiting for further client messages without relying on full context cancellation.

Copilot uses AI. Check for mistakes.
// any worker to report an error.
select {
case <-runCtx.Done():
case <-recvDone:
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recvDone is closed unconditionally in the receive goroutine, including when that goroutine hits a non-EOF error and sends it on errChan. In that situation both recvDone and errChan can become ready, and the final select may pick recvDone and return nil, silently dropping the receive error (same for a send error racing with receive completion). Consider making the completion signal carry an error (e.g., recvDone chan error) or, at minimum, ensure the recvDone path checks/drains errChan before returning nil so real worker errors can’t be lost.

Suggested change
case <-recvDone:
case <-recvDone:
// Prefer any pending worker error over a silent success.
select {
case err := <-errChan:
if err != nil {
return err
}
default:
}

Copilot uses AI. Check for mistakes.
@jenstopp
Copy link
Member

The context is:
Triggering the dutctl client binary inside another framework, right? Although this should also work, of course, it is not the intended use: the client is for humans, and the RPC API (of the agent) is for systems.

Thus, I need to take a closer look, as this change touches a critical part of goroutine synchronization and cannot pull it in quickly.

@jenstopp jenstopp changed the title fix(dutctl): don't cancel run context on stdin EOF Don't cancel run context on stdin EOF Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants