From 9190b2f3c53b00d63fad6f7023f54db267da04a9 Mon Sep 17 00:00:00 2001 From: Team Humaki Date: Fri, 18 Sep 2026 03:33:32 -0700 Subject: [PATCH] session: unblock a cancelled stdin write when the kill fails Closing the output reader only ends the read phase. A command still blocked in writeCommand held the session lock, so Close could not close stdin either. Cut stdin in cancelNow (and in Close, before the lock) so a failed kill still returns. Fixes #112 --- internal/server/session_cancel_unix_test.go | 41 ++++++++++++++++++++ internal/server/shell.go | 43 ++++++++++++++------- 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/internal/server/session_cancel_unix_test.go b/internal/server/session_cancel_unix_test.go index e206b3a..847653b 100644 --- a/internal/server/session_cancel_unix_test.go +++ b/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 diff --git a/internal/server/shell.go b/internal/server/shell.go index 0bbb1cd..ccc9a61 100644 --- a/internal/server/shell.go +++ b/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