fix(web): recover a panic in the event watch goroutine
net/http only recovers the handler goroutine; a panic in the watch now ends the stream with a 500 and a log line instead of the process.
- commit
- 588fe2c3caa3bed3a8983e3ab70b8b625fef316f
- author
- brice · · 2026-10-12 06:08 UTC
- parent
- 2bedec2
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +31 -0
M internal/adapters/web/events.go +12 -0
| @@ -5,6 +5,7 @@ package web | |||
| 5 | 5 | import ( | |
| 6 | 6 | "context" | |
| 7 | 7 | "errors" | |
| 8 | + | "fmt" | |
| 8 | 9 | "io" | |
| 9 | 10 | "net/http" | |
| 10 | 11 | "net/url" | |
| @@ -128,6 +129,17 @@ func (s *Server) events(w http.ResponseWriter, r *http.Request) { | |||
| 128 | 129 | go func() { | |
| 129 | 130 | defer close(finished) | |
| 130 | 131 | defer close(msgs) | |
| 132 | + | // net/http only recovers the handler goroutine: a panic in the watch | |
| 133 | + | // would end the process. It ends this stream as an error instead. | |
| 134 | + | defer func() { | |
| 135 | + | if v := recover(); v != nil { | |
| 136 | + | log.Error().Interface("panic", v).Msg("event watch panicked") | |
| 137 | + | select { | |
| 138 | + | case msgs <- recvd{err: fmt.Errorf("watch panicked: %v", v)}: | |
| 139 | + | case <-ctx.Done(): | |
| 140 | + | } | |
| 141 | + | } | |
| 142 | + | }() | |
| 131 | 143 | err := s.cfg.Events.Watch(ctx, principal(ctx), params, func(we ports.WatchEvent) error { | |
| 132 | 144 | start() | |
| 133 | 145 | select { | |
| @@ -5,6 +5,7 @@ package web | |||||
| 5 | import ( | 5 | import ( | ||
| 6 | "context" | 6 | "context" | ||
| 7 | "errors" | 7 | "errors" | ||
| 8 | + | "fmt" | |||
| 8 | "io" | 9 | "io" | ||
| 9 | "net/http" | 10 | "net/http" | ||
| 10 | "net/url" | 11 | "net/url" | ||
| @@ -128,6 +129,17 @@ func (s *Server) events(w http.ResponseWriter, r *http.Request) { | |||||
| 128 | go func() { | 129 | go func() { | ||
| 129 | defer close(finished) | 130 | defer close(finished) | ||
| 130 | defer close(msgs) | 131 | defer close(msgs) | ||
| 132 | + | // net/http only recovers the handler goroutine: a panic in the watch | |||
| 133 | + | // would end the process. It ends this stream as an error instead. | |||
| 134 | + | defer func() { | |||
| 135 | + | if v := recover(); v != nil { | |||
| 136 | + | log.Error().Interface("panic", v).Msg("event watch panicked") | |||
| 137 | + | select { | |||
| 138 | + | case msgs <- recvd{err: fmt.Errorf("watch panicked: %v", v)}: | |||
| 139 | + | case <-ctx.Done(): | |||
| 140 | + | } | |||
| 141 | + | } | |||
| 142 | + | }() | |||
| 131 | err := s.cfg.Events.Watch(ctx, principal(ctx), params, func(we ports.WatchEvent) error { | 143 | err := s.cfg.Events.Watch(ctx, principal(ctx), params, func(we ports.WatchEvent) error { | ||
| 132 | start() | 144 | start() | ||
| 133 | select { | 145 | select { | ||
M internal/adapters/web/events_test.go +19 -0
| @@ -233,6 +233,25 @@ func TestEventsRefusals(t *testing.T) { | |||
| 233 | 233 | } | |
| 234 | 234 | } | |
| 235 | 235 | ||
| 236 | + | // A panic in the watch runs outside the handler goroutine, where net/http | |
| 237 | + | // would not recover it: it ends the stream with a 500 instead of the process. | |
| 238 | + | func TestEventsWatchPanicIsAnError(t *testing.T) { | |
| 239 | + | t.Parallel() | |
| 240 | + | m := mocks.NewEventService(t) | |
| 241 | + | m.EXPECT().Watch(mock.Anything, mock.Anything, mock.Anything, mock.Anything). | |
| 242 | + | RunAndReturn(func(context.Context, domain.Principal, ports.WatchParams, func(ports.WatchEvent) error) error { | |
| 243 | + | panic("boom") | |
| 244 | + | }) | |
| 245 | + | ts := httptest.NewServer(newTestServer(t, Config{ | |
| 246 | + | Repos: reposServing(t, publicRepo(t, "x")), | |
| 247 | + | Browse: mocks.NewBrowseService(t), | |
| 248 | + | Events: m, | |
| 249 | + | })) | |
| 250 | + | t.Cleanup(ts.Close) | |
| 251 | + | resp, _ := open(t, ts, "topic=home", nil) | |
| 252 | + | assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) | |
| 253 | + | } | |
| 254 | + | ||
| 236 | 255 | // The topic of a repository the visitor cannot see is refused before any | |
| 237 | 256 | // watch is opened: a private repository is a missing one. | |
| 238 | 257 | func TestEventsPrivateRepositoryTopic(t *testing.T) { | |
| @@ -233,6 +233,25 @@ func TestEventsRefusals(t *testing.T) { | |||||
| 233 | } | 233 | } | ||
| 234 | } | 234 | } | ||
| 235 | 235 | ||||
| 236 | + | // A panic in the watch runs outside the handler goroutine, where net/http | |||
| 237 | + | // would not recover it: it ends the stream with a 500 instead of the process. | |||
| 238 | + | func TestEventsWatchPanicIsAnError(t *testing.T) { | |||
| 239 | + | t.Parallel() | |||
| 240 | + | m := mocks.NewEventService(t) | |||
| 241 | + | m.EXPECT().Watch(mock.Anything, mock.Anything, mock.Anything, mock.Anything). | |||
| 242 | + | RunAndReturn(func(context.Context, domain.Principal, ports.WatchParams, func(ports.WatchEvent) error) error { | |||
| 243 | + | panic("boom") | |||
| 244 | + | }) | |||
| 245 | + | ts := httptest.NewServer(newTestServer(t, Config{ | |||
| 246 | + | Repos: reposServing(t, publicRepo(t, "x")), | |||
| 247 | + | Browse: mocks.NewBrowseService(t), | |||
| 248 | + | Events: m, | |||
| 249 | + | })) | |||
| 250 | + | t.Cleanup(ts.Close) | |||
| 251 | + | resp, _ := open(t, ts, "topic=home", nil) | |||
| 252 | + | assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) | |||
| 253 | + | } | |||
| 254 | + | ||||
| 236 | // The topic of a repository the visitor cannot see is refused before any | 255 | // The topic of a repository the visitor cannot see is refused before any | ||
| 237 | // watch is opened: a private repository is a missing one. | 256 | // watch is opened: a private repository is a missing one. | ||
| 238 | func TestEventsPrivateRepositoryTopic(t *testing.T) { | 257 | func TestEventsPrivateRepositoryTopic(t *testing.T) { | ||