Close the logs-done channels only once - #4275
Open
un-def wants to merge 1 commit into
Open
Conversation
Previously, `pullGetHandler` closed `pullDoneCh` on every request it
served in the WaitLogsFinished state, and `streamJobLogs` closed
`wsDoneCh` on every stream that drained after shutdown. Both assumed
they run exactly once per server lifetime.
Neither assumption holds. `Server.stop()` sets WaitLogsFinished as soon
as the job is asked to stop, while the server keeps serving until the
executor returns -- which does not happen while a process left behind
by the job holds the pty slave open, since `io.Copy` from the master
then never sees EOF. The dstack server goes on polling, and the second
pull panics:
http: panic serving 127.0.0.1:50670: close of closed channel
...
api.(*Server).pullGetHandler.func1()
runner/internal/runner/api/http.go:191
`/logs_ws` has the same defect without needing a stuck executor:
nothing limits the number of connections, each is drained by its own
goroutine, and two draining together close the channel twice. Both
handlers hold only a read lock, so concurrent requests can observe the
state together as well.
net/http recovers the panic per connection, so the runner survives it
but serves a broken pull endpoint from then on: the job state stops
being reported and the run hangs.
Now both channels are closed through `sync.Once` helpers. The api
package had no tests; the ones added here cover the repeated and the
concurrent case.
Why the executor fails to return is left alone here.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Previously,
pullGetHandlerclosedpullDoneChon every request it served in the WaitLogsFinished state, andstreamJobLogsclosedwsDoneChon every stream that drained after shutdown. Both assumed they run exactly once per server lifetime.Neither assumption holds.
Server.stop()sets WaitLogsFinished as soon as the job is asked to stop, while the server keeps serving until the executor returns -- which does not happen while a process left behind by the job holds the pty slave open, sinceio.Copyfrom the master then never sees EOF. The dstack server goes on polling, and the second pull panics:/logs_wshas the same defect without needing a stuck executor: nothing limits the number of connections, each is drained by its own goroutine, and two draining together close the channel twice. Both handlers hold only a read lock, so concurrent requests can observe the state together as well.net/http recovers the panic per connection, so the runner survives it but serves a broken pull endpoint from then on: the job state stops being reported and the run hangs.
Now both channels are closed through
sync.Oncehelpers. The api package had no tests; the ones added here cover the repeated and the concurrent case.Why the executor fails to return is left alone here.