Review fixes: recorder Flush without panic; unreadable config file is an error
Implemented-By: OpenCode session (model recorded in docs/implementer-log.md)
This commit is contained in:
@@ -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/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/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/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
|
## Reviews
|
||||||
|
|
||||||
|
|||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -147,7 +147,9 @@ func (r *statusRecorder) WriteHeader(code int) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func (r *statusRecorder) Flush() {
|
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) {
|
func (p *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
|
||||||
|
|||||||
@@ -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))
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user