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
Open
Conversation
…d HTTP shutdown returns
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
serveSessionManagerandserveRuntime.serveRuntime.CloseContextcancels the active run first, then waits for the runtime mutex only until the supplied context is done.runServeLegacysession-manager cleanup to use a bounded shutdown context instead of the previous unbounded deferredClose.rt.muto simulate a stuck provider/tool stream and verifiesCloseContextstill returns while cancelling the active interrupt.Why this is high-value
term-llm serveis a daily web/API/jobs path. Before this change, server shutdown used a 10s boundedserveServer.Stop, but the deferredsessionMgr.Close()that ran afterwards could block forever on a stateful runtime whose provider/tool ignored cancellation while holdingrt.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.gogo build ./...go test ./cmd -count=1go test ./... -skip TestProgramRunnerDoesNotLeakBackgroundChildrengit diff --checkNote:
go test ./...was run and consistently fails ininternal/jobsat the existingTestProgramRunnerDoesNotLeakBackgroundChildren(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.