diff --git a/internal/server/server.go b/internal/server/server.go index b77c6ab..69af47c 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -20,7 +20,7 @@ import ( // implementedPaths backs the health/coverage endpoint. var implementedPaths = []string{ "POST /v1/sprites", - "GET /v1/sprites/{id}/exec (control WebSocket)", + "GET /v1/sprites/{id}/exec (control WebSocket; session list when not upgraded)", "POST /v1/sprites/{id}/checkpoint", "GET /v1/sprites/{id}/checkpoints", "GET /v1/sprites/{id}/checkpoints/{cid}", diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 6733cbc..a8098bf 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -178,9 +178,14 @@ func TestHealth(t *testing.T) { // The exec entry names the control WebSocket, and checkpoint create is the // singular path. joined := strings.Join(payload.Implemented, "\n") - if !strings.Contains(joined, "exec (control WebSocket)") { + if !strings.Contains(joined, "exec (control WebSocket") { t.Fatalf("coverage list missing WS exec entry: %v", payload.Implemented) } + // The same path answers a plain GET with the session list, and a client + // reading this list should be able to see that without trying it. + if !strings.Contains(joined, "session list when not upgraded") { + t.Fatalf("coverage list does not advertise the exec session list: %v", payload.Implemented) + } if !strings.Contains(joined, "POST /v1/sprites/{id}/checkpoint\n") && !strings.HasSuffix(joined, "POST /v1/sprites/{id}/checkpoint") { t.Fatalf("coverage list missing singular checkpoint create: %v", payload.Implemented) } @@ -516,3 +521,65 @@ func TestEmptyFSMarshalsAsObject(t *testing.T) { t.Fatalf("empty sprite GET body = %s, want fs:{} and checkpoints:[]", s) } } + +// A plain GET on the exec path is the session list, not a failed upgrade. +// +// sprites-ex asks for it before deciding whether to reattach, so answering 426 +// here stopped every fountain turn before anything ran (#18). +func TestExecPlainGetReturnsSessionList(t *testing.T) { + h := newHarness(t) + if code, body := h.do(http.MethodPost, "/v1/sprites", map[string]any{"name": "demo"}); code != http.StatusCreated { + t.Fatalf("create sprite: %d %s", code, body) + } + + code, body := h.do(http.MethodGet, "/v1/sprites/demo/exec", nil) + if code != http.StatusOK { + t.Fatalf("plain GET on exec: got %d, want 200 (body %q)", code, body) + } + + var payload struct { + Sessions []any `json:"sessions"` + } + h.mustJSON(body, &payload) + if payload.Sessions == nil { + t.Fatal(`sessions must be present and an array, not null: a client reads body["sessions"] directly`) + } + if len(payload.Sessions) != 0 { + t.Fatalf("a sprite that is not mid-exec has no sessions, got %d", len(payload.Sessions)) + } +} + +// "no sessions" and "no sprite" are different answers. +func TestExecPlainGetOnMissingSpriteIs404(t *testing.T) { + h := newHarness(t) + if code, body := h.do(http.MethodGet, "/v1/sprites/nope/exec", nil); code != http.StatusNotFound { + t.Fatalf("exec session list on a missing sprite: got %d, want 404 (body %q)", code, body) + } +} + +// A Connection header of "keep-alive, Upgrade" is still an upgrade — proxies +// routinely send the list form, and an equality check would route it to the +// session list and break exec behind any proxy. +func TestConnectionHeaderListIsAnUpgrade(t *testing.T) { + for _, tc := range []struct { + conn, upgrade string + want bool + }{ + {"Upgrade", "websocket", true}, + {"keep-alive, Upgrade", "websocket", true}, + {"upgrade", "WebSocket", true}, + {"keep-alive", "websocket", false}, + {"", "", false}, + } { + r := httptest.NewRequest(http.MethodGet, "/v1/sprites/demo/exec", nil) + if tc.conn != "" { + r.Header.Set("Connection", tc.conn) + } + if tc.upgrade != "" { + r.Header.Set("Upgrade", tc.upgrade) + } + if got := isWebSocketUpgrade(r); got != tc.want { + t.Errorf("Connection=%q Upgrade=%q: got %v, want %v", tc.conn, tc.upgrade, got, tc.want) + } + } +} diff --git a/internal/server/ws.go b/internal/server/ws.go index 8a9a85e..eb60e3e 100644 --- a/internal/server/ws.go +++ b/internal/server/ws.go @@ -31,6 +31,21 @@ const ( // [streamExit] and closes. func (s *Server) execSpriteWS(w http.ResponseWriter, r *http.Request) { id := r.PathValue("id") + + // The same path serves two protocols, which is the real API's shape rather + // than a convenience: upgraded, it is the control WebSocket below; plain, it + // is the session list. sprites-ex's Session.list_by_name/2 does exactly + // `Req.get("/v1/sprites/#{name}/exec")` and reads body["sessions"]. + // + // Without this, a plain GET fell through to websocket.Accept and came back + // 426, which is what orphaned every fountain turn — the client asks for the + // session list before it decides whether to reattach or start fresh, so the + // turn died before anything ran. See INTENTIUS/spritzer#18. + if !isWebSocketUpgrade(r) { + s.listExecSessions(w, r) + return + } + cmd := reconstructCmd(r) // Advertise the control-WebSocket capability on the 101 response so a client @@ -124,3 +139,39 @@ func writeFrame(ctx context.Context, c *websocket.Conn, streamID byte, payload [ frame = append(frame, payload...) return c.Write(ctx, websocket.MessageBinary, frame) } + +// isWebSocketUpgrade reports whether the request is asking to upgrade. +// +// Both headers are lists and both are case-insensitive, so this cannot be an +// equality check: Connection is commonly "keep-alive, Upgrade" from a proxy. +func isWebSocketUpgrade(r *http.Request) bool { + if !strings.EqualFold(r.Header.Get("Upgrade"), "websocket") { + return false + } + for _, part := range strings.Split(r.Header.Get("Connection"), ",") { + if strings.EqualFold(strings.TrimSpace(part), "upgrade") { + return true + } + } + return false +} + +// listExecSessions answers a plain GET on the exec path with the sprite's +// current exec sessions. +// +// spritzer runs each exec for the life of one WebSocket and keeps nothing +// afterwards, so a sprite that is not mid-exec has no sessions and the honest +// answer is an empty list. That is also the answer that lets a client get on +// with it: no active session means nothing to reattach to, so it starts a fresh +// turn instead of waiting for one that will never appear. +// +// A missing sprite is still a 404 here, the same as every other operation on +// one, rather than an empty list — "no sessions" and "no sprite" are different +// answers and a client reattaching should be able to tell them apart. +func (s *Server) listExecSessions(w http.ResponseWriter, r *http.Request) { + id := r.PathValue("id") + if _, err := s.store.Get(id); s.handleLookupError(w, id, err) { + return + } + writeJSON(w, http.StatusOK, map[string]any{"sessions": []any{}}) +}