Skip to content

fix: Serve session manager shutdown can hang forever after the bounded HTTP shutdown returns#896

Open
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-6fb6c021
Open

fix: Serve session manager shutdown can hang forever after the bounded HTTP shutdown returns#896
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-6fb6c021

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • Added context-aware close paths for serveSessionManager and serveRuntime.
  • serveRuntime.CloseContext cancels the active run first, then waits for the runtime mutex only until the supplied context is done.
  • Changed runServeLegacy session-manager cleanup to use a bounded shutdown context instead of the previous unbounded deferred Close.
  • Added a regression test that holds rt.mu to simulate a stuck provider/tool stream and verifies CloseContext still returns while cancelling the active interrupt.

Why this is high-value

term-llm serve is a daily web/API/jobs path. Before this change, server shutdown used a 10s bounded serveServer.Stop, but the deferred sessionMgr.Close() that ran afterwards could block forever on a stateful runtime whose provider/tool ignored cancellation while holding rt.mu. That could wedge restarts/reloads until the process was killed. This keeps normal cleanup behavior for cooperative runtimes while bounding the shutdown wait for stuck ones.

Validation

  • gofmt -w cmd/serve.go cmd/serve_session.go cmd/serve_runtime.go cmd/serve_test.go
  • go build ./...
  • go test ./cmd -count=1
  • go test ./... -skip TestProgramRunnerDoesNotLeakBackgroundChildren
  • git diff --check

Note: go test ./... was run and consistently fails in internal/jobs at the existing TestProgramRunnerDoesNotLeakBackgroundChildren (background child process ... still appears to be alive). The same isolated test fails independently of this serve-session change; the suite passes when only that unrelated jobs test is skipped.

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.

1 participant