From 9e0f906c8bacf9bf04bed76d38c320d4734bfb2b Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Fri, 25 Sep 2026 02:58:17 -0700 Subject: [PATCH] Review fixes: recorder Flush without panic; unreadable config file is an error Implemented-By: OpenCode session (model recorded in docs/implementer-log.md) --- docs/implementer-log.md | 1 + internal/config/unreadable_test.go | 47 +++++++++++++++++++++ internal/proxy/proxy.go | 4 +- internal/proxy/recorder_test.go | 66 ++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 internal/config/unreadable_test.go create mode 100644 internal/proxy/recorder_test.go diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 38b9e46..9abf202 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -10,6 +10,7 @@ owner fills in the Model column. The reviewer adds findings under "Reviews" once | v0/03-proxy | 2026-09-25 | done | 1 | pass | none | `SplitRoute` must reject an empty first segment (`/`, `//x`) as `ok=false`; the model peek restores the body and leaves non-JSON/empty as `""`. | llama.cpp/ornith-1.5-35b-a3b | | v0/04-admin-main | 2026-09-25 | done | 1 | pass | none | `timeout --signal=TERM 3` exits 124 on a timed-out child on this GNU system, so the task's `exit=0` is not observable through it; sent SIGTERM directly and confirmed crossbar's own exit code is 0 with both log lines. | llama.cpp/ornith-1.5-35b-a3b | | v0/05-smoke-readme-deploy | 2026-09-25 | done | 1 | pass | none | `README.md` `## Run` uses `install -m` instead of `cp` and adds `systemctl daemon-reload` before `enable --now`, which is required for systemd to see the new unit; the task said only "copy … then enable --now". | llama.cpp/ornith-1.5-35b-a3b | +| v0/01-review-fixes | 2026-09-25 | done | 1 | pass | none | `Flush` now two-value. Assertion inventory (`grep -n '\.(' internal/*/*.go`): proxy.go:150 fixed to two-value; proxy_test.go:274 net/http guarantees the server writer is a Flusher. No other unchecked outside assertion. Recorder test panicked before the fix, passed after; config tests passed as-is. | llama.cpp/ornith-1.5-35b-a3b | ## Reviews diff --git a/internal/config/unreadable_test.go b/internal/config/unreadable_test.go new file mode 100644 index 0000000..5253df0 --- /dev/null +++ b/internal/config/unreadable_test.go @@ -0,0 +1,47 @@ +package config_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "git.wntrmute.dev/kyle/crossbar/internal/config" +) + +// A file that exists but cannot be read is an error, and not a validation error: nothing about +// the configuration has been judged. Only a missing file is "absent" (and that is an error too). +func TestUnreadableFileIsAnError(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root can read a 000 file") + } + dir := t.TempDir() + path := filepath.Join(dir, "crossbar.toml") + good, err := os.ReadFile(filepath.Join("testdata", "good.toml")) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, good, 0o000); err != nil { + t.Fatal(err) + } + _, err = config.Load(path) + if err == nil { + t.Fatal("Load on an unreadable file must fail") + } + if _, ok := config.IsError(err); ok { + t.Errorf("an unreadable file is not a validation *Error: %v", err) + } + if !strings.HasPrefix(err.Error(), "config: ") { + t.Errorf("Error() = %q, want the config: prefix", err.Error()) + } +} + +func TestDirectoryIsAnError(t *testing.T) { + _, err := config.Load(t.TempDir()) + if err == nil { + t.Fatal("Load on a directory must fail") + } + if _, ok := config.IsError(err); ok { + t.Errorf("a directory is not a validation *Error: %v", err) + } +} diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index bb59b26..65f3eb4 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -147,7 +147,9 @@ func (r *statusRecorder) WriteHeader(code int) { } func (r *statusRecorder) Flush() { - r.ResponseWriter.(http.Flusher).Flush() + if f, ok := r.ResponseWriter.(http.Flusher); ok { + f.Flush() + } } func (p *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { diff --git a/internal/proxy/recorder_test.go b/internal/proxy/recorder_test.go new file mode 100644 index 0000000..b5ffa45 --- /dev/null +++ b/internal/proxy/recorder_test.go @@ -0,0 +1,66 @@ +package proxy_test + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "git.wntrmute.dev/kyle/crossbar/internal/config" + "git.wntrmute.dev/kyle/crossbar/internal/health" + "git.wntrmute.dev/kyle/crossbar/internal/proxy" +) + +// noFlush is a ResponseWriter that does not implement http.Flusher. Middleware and test +// recorders like this exist in the wild; the proxy must degrade to buffering, never panic. +type noFlush struct{ w http.ResponseWriter } + +func (n noFlush) Header() http.Header { return n.w.Header() } +func (n noFlush) Write(b []byte) (int, error) { return n.w.Write(b) } +func (n noFlush) WriteHeader(code int) { n.w.WriteHeader(code) } + +func TestStreamingWriterWithoutFlusherDoesNotPanic(t *testing.T) { + up := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(200) + for i := 0; i < 3; i++ { + fmt.Fprintf(w, "data: chunk %d\n\n", i) + w.(http.Flusher).Flush() + } + })) + t.Cleanup(up.Close) + cfg, err := config.Parse(strings.NewReader(fmt.Sprintf(` +listen = "127.0.0.1:1" +[hosts.alpha] +base_url = %q +models = { "m" = { } } +[routes.r] +hosts = ["alpha"] +`, up.URL))) + if err != nil { + t.Fatal(err) + } + h := &fakeHealth{st: map[string]health.Status{"alpha": {Healthy: true, Loaded: []string{"m"}}}} + p := proxy.New(cfg, h, nil) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/r/v1/chat/completions", strings.NewReader(`{"model":"m","stream":true}`)) + func() { + defer func() { + if r := recover(); r != nil { + t.Fatalf("ServeHTTP panicked on a writer without Flush: %v", r) + } + }() + p.ServeHTTP(noFlush{rec}, req) + }() + if rec.Code != 200 { + t.Fatalf("status %d", rec.Code) + } + if got := rec.Body.String(); !strings.Contains(got, "chunk 0") || !strings.Contains(got, "chunk 2") { + t.Errorf("body = %q, want all three chunks", got) + } + if rec.Header().Get(proxy.HostHeader) != "alpha" { + t.Errorf("host header %q", rec.Header().Get(proxy.HostHeader)) + } +}