session: unblock a cancelled stdin write when the kill fails

internal/server/session_cancel_unix_test.go

@@ -310,6 +310,47 @@ func TestFailedKillIsReportedWhileTheCommandStillRuns(t *testing.T) {
 	}
 }
 
+// Issue #112: a failed kill used to cut only the output reader. A command still
+// blocked in writeCommand — the shell is inside sleep, so a megabyte of comment
+// fills the stdin pipe — never returned, even though Closed() was already true.
+func TestFailedKillUnblocksABlockedStdinWrite(t *testing.T) {
+	session := newTestSession(t)
+	session.killContainer = func() error {
+		return errors.New("no permission to signal the group")
+	}
+
+	pidFile := filepath.Join(t.TempDir(), "child.pid")
+	// First line records the pid, second parks the shell so it stops reading
+	// stdin, then enough comment to overflow a typical pipe buffer (~64KiB).
+	command := "echo $$ > " + pidFile + "\n/bin/sleep 600\n" + strings.Repeat("# padding\n", 80_000)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	done := runAsync(session, ctx, command)
+	pid := waitForPIDFile(t, pidFile)
+	t.Cleanup(func() { syscall.Kill(pid, syscall.SIGKILL) })
+
+	// Give writeCommand time to fill the pipe and block. The pid file means
+	// the shell has consumed the first line and is now inside sleep.
+	time.Sleep(200 * time.Millisecond)
+
+	start := time.Now()
+	cancel()
+	r := awaitExec(t, done, 5*time.Second)
+	if took := time.Since(start); took > sessionWaitDelay/2 {
+		t.Fatalf("blocked stdin write took %s to return after a failed kill", took)
+	}
+	if !errors.Is(r.err, ErrSessionCancelled) {
+		t.Fatalf("blocked submit = (%d, %v), want ErrSessionCancelled", r.code, r.err)
+	}
+	if !strings.Contains(r.err.Error(), "no permission to signal the group") {
+		t.Fatalf("blocked submit reported %q without the kill error", r.err)
+	}
+	if !session.Closed() {
+		t.Fatal("Closed() was false after a cancel that could not kill")
+	}
+}
+
 // A watcher can wake after its own request disarmed and after the next request
 // armed. A gate that only asks "is anything armed" reads true in that state and
 // destroys a session nobody cancelled — 50 times in 100 under the scheduling

internal/server/shell.go

@@ -132,6 +132,7 @@ type ShellSession struct {
 	// which processes belong to it. See session_container.go.
 	container    *sessionContainer
 	containerOff sync.Once
+	stdinOff     sync.Once
 	// killContainer is the container kill cancelNow performs. It is a field so
 	// a test can make the kill fail without also replacing what cancelNow does
 	// around it, which is the part worth testing.
@@ -161,16 +162,31 @@ func (s *ShellSession) endOutput(cause error) {
 	}
 }
 
+// cutStdin unblocks a Write still sitting in writeCommand. Closing the reader
+// only ends the output-reading phase; a command large enough to fill the stdin
+// pipe is still stuck in the submit phase, and Close cannot take the session
+// lock to close stdin while that Write holds it.
+func (s *ShellSession) cutStdin() {
+	s.stdinOff.Do(func() {
+		if s.stdin != nil {
+			s.stdin.Close()
+		}
+	})
+}
+
 // cancelNow is what a cancellation does: end the session's processes and stop
-// waiting on their output. The two are separate steps on purpose. Killing the
-// container does not guarantee the output pipe closes, because a descendant
-// that escaped the container still holds it, so the reader is cut here rather
-// than left to a copier that may never see EOF. That is also what makes a kill
-// that *failed* reach the caller immediately instead of when the command it
-// could not stop happens to end.
+// waiting on their output or on a blocked stdin write. Killing the container
+// does not guarantee the output pipe closes, because a descendant that escaped
+// the container still holds it, so the reader is cut here rather than left to a
+// copier that may never see EOF. The stdin write is cut for the same reason: a
+// kill that failed leaves the shell alive, so a Write blocked on a full pipe
+// would never return. Cutting both is also what makes a kill that *failed*
+// reach the caller immediately instead of when the command it could not stop
+// happens to end.
 func (s *ShellSession) cancelNow() error {
 	s.closed.Store(true)
 	err := s.killContainer()
+	s.cutStdin()
 	s.endOutput(ErrSessionCancelled)
 	return err
 }
@@ -425,21 +441,20 @@ func (s *ShellSession) Closed() bool { return s.closed.Load() }
 // its container. Killing only the shell would leave its children orphaned and
 // still holding the device's resources.
 func (s *ShellSession) Close() {
-	// Mark, kill and cut the output before taking the lock. A command still
-	// waiting on the shell holds that lock, and these three things are exactly
-	// what end its wait — locking first would make Close hang on the session it
-	// is trying to tear down.
-	first := !s.closed.Swap(true)
+	// Mark, kill and cut the pipes before taking the lock. A command still
+	// waiting on the shell holds that lock, and these are exactly what end its
+	// wait — locking first would make Close hang on the session it is trying
+	// to tear down. Stdin is cut here rather than under the lock so a Write
+	// blocked in writeCommand can return; that Write is the lock holder.
+	s.closed.Store(true)
 	s.releaseContainer()
 	if s.container == nil && s.cmd != nil && s.cmd.Process != nil {
 		s.cmd.Process.Kill()
 	}
+	s.cutStdin()
 	s.endOutput(fmt.Errorf("session closed"))
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	if first && s.stdin != nil {
-		s.stdin.Close()
-	}
 }
 
 // RunOneShot executes a command in a fresh shell whose process working