From f7e68479e6864277d4208b662489d140e604ebbc Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 13:46:41 -0400 Subject: [PATCH 1/3] rest: pass informational 1xx WriteHeaders through all three writer wrappers (#165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit net/http deliberately leaves its wroteHeader latch false for 1xx — the chain's three writer wrappers all disagreed, latching on the FIRST WriteHeader regardless of status class: - statusWriter metered a 103 as the final status - commitWriter sent a 103-then-panic down the re-panic path (connection abort) instead of writing the contract 500 - cacheControlWriter burned the stamp decision on the 103, so the real 200 committed with no Cache-Control All three now treat 100..199 as pass-through via a shared informational() predicate: forward to the delegate, latch nothing — the subsequent final WriteHeader behaves exactly as a first call. One shared table test (informational_internal_test.go) drives the 103 through each wrapper's owning middleware on a real server (wire-faithful 1xx machinery, the forwarded 103 observed via httptrace) — a fourth wrapper joins the table. Latch-era docs (#164 rollback phrasing, the deferred metrics relabel) made precise for the forwarded-1xx case. Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/cachecontrol.go | 11 +- rest/cachecontrol_internal_test.go | 14 +++ rest/informational.go | 19 ++++ rest/informational_internal_test.go | 156 ++++++++++++++++++++++++++++ rest/metrics.go | 16 ++- rest/sentry.go | 23 +++- 6 files changed, 230 insertions(+), 9 deletions(-) create mode 100644 rest/informational.go create mode 100644 rest/informational_internal_test.go diff --git a/rest/cachecontrol.go b/rest/cachecontrol.go index b5468e4..6201994 100644 --- a/rest/cachecontrol.go +++ b/rest/cachecontrol.go @@ -59,7 +59,10 @@ func cacheControl(maxAgeSeconds int, next http.Handler) http.Handler { } // cacheControlWriter injects the Cache-Control header at commit time — the -// first WriteHeader, Write, or flush — and only when the response is a 200. +// first FINAL (non-1xx) WriteHeader, Write, or flush — and only when the +// response is a 200. A forwarded informational (1xx) WriteHeader commits +// nothing (#165): the stamp decision belongs to the final status that +// follows, exactly as net/http's own writer leaves 1xx uncommitted. // Commit time is the only safe moment: setting the header eagerly would leak // it onto error responses written later (writeError never clears headers), // and onto the contract 500 the sentry layer writes after a handler panic. @@ -75,6 +78,12 @@ type cacheControlWriter struct { } func (w *cacheControlWriter) WriteHeader(code int) { + if informational(code) { + // 1xx never commits — forward and keep the stamp decision for the + // final status (#165; rationale on the informational predicate). + w.ResponseWriter.WriteHeader(code) + return + } if !w.committed { w.committed = true if code == http.StatusOK { diff --git a/rest/cachecontrol_internal_test.go b/rest/cachecontrol_internal_test.go index 8300d66..fbb67d3 100644 --- a/rest/cachecontrol_internal_test.go +++ b/rest/cachecontrol_internal_test.go @@ -43,6 +43,20 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) { assert.Empty(t, rr.Header().Get("Cache-Control")) }) + // An informational WriteHeader latches nothing (#165) — and in particular + // must not stamp: a stamp on the 1xx would sit in the live map and leak + // onto whatever final status follows, here a 500 (the leak class the type + // doc forbids). The stamp-on-the-real-200 direction needs real 1xx wire + // machinery the recorder lacks — informational_internal_test.go's shared + // table covers it on a real server. + t.Run("WriteHeader 103 leaves the decision to the final status", func(t *testing.T) { + w, rr := wrap() + w.WriteHeader(http.StatusEarlyHints) + w.WriteHeader(http.StatusInternalServerError) + assert.Empty(t, rr.Header().Get("Cache-Control"), + "a 500 after a forwarded 103 must not carry the freshness signal") + }) + t.Run("second WriteHeader cannot change the first decision", func(t *testing.T) { w, rr := wrap() w.WriteHeader(http.StatusInternalServerError) diff --git a/rest/informational.go b/rest/informational.go new file mode 100644 index 0000000..0eda1f1 --- /dev/null +++ b/rest/informational.go @@ -0,0 +1,19 @@ +package rest + +// informational reports whether code is an informational (1xx) status. +// +// Informational responses precede the final response on the wire and never +// commit it — net/http's own response writer special-cases 1xx and leaves its +// wroteHeader latch false. Every writer wrapper in the chain with commit-time +// state (statusWriter's status capture, commitWriter's commit latch, +// cacheControlWriter's header stamp) must mirror that: forward a 1xx +// WriteHeader to the delegate and latch nothing, so the subsequent final +// WriteHeader behaves exactly as a first call (#165). A new wrapper with +// WriteHeader state starts here — and joins the shared table in +// informational_internal_test.go. +// +// The API itself emits no 1xx today; the guard exists so the wrappers stay +// faithful to net/http's commit semantics rather than to current traffic. +func informational(code int) bool { + return code >= 100 && code <= 199 +} diff --git a/rest/informational_internal_test.go b/rest/informational_internal_test.go new file mode 100644 index 0000000..fc0d3bb --- /dev/null +++ b/rest/informational_internal_test.go @@ -0,0 +1,156 @@ +package rest + +// Shared latch coverage for the chain's writer wrappers (#165): an +// informational (1xx) WriteHeader must pass through to the delegate and latch +// NOTHING — net/http itself never treats 1xx as a commit (the stdlib response +// writer leaves its wroteHeader latch false), so a wrapper that latches there +// diverges from the wire: metrics would meter a 103 as the final status, the +// panic recovery would abort a connection whose response is still rewritable, +// and Cache-Control would miss the real 200. +// +// The observations are wire-faithful: a real httptest server (the stdlib's +// actual 1xx machinery, which a ResponseRecorder does not have) with the +// forwarded 103 captured via httptrace on the client side. + +import ( + "io" + "net/http" + "net/http/httptest" + "net/http/httptrace" + "net/textproto" + "testing" + + "github.com/getsentry/sentry-go" + "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The predicate's boundaries pin the wire definition exactly: 1xx is +// [100, 199], nothing more — a guard that drifts to e.g. (100, 199] would +// silently latch on a forwarded 100 Continue. +func TestInformational_Boundaries(t *testing.T) { + assert.False(t, informational(99), "99 is not a status class at all") + assert.True(t, informational(http.StatusContinue), "100 opens the informational class") + assert.True(t, informational(http.StatusEarlyHints), "103 is the one seen in the wild") + assert.True(t, informational(199), "199 closes the informational class") + assert.False(t, informational(http.StatusOK), "200 is a final status — it must latch") +} + +// TestWriterWrappers_Informational1xxDoesNotLatch drives WriteHeader(103) +// through each writer wrapper's owning middleware and asserts (a) the 103 +// reaches the wire and (b) the wrapper's latch decision still belongs to the +// FINAL response — each row finishes the request the way that makes its own +// latch observable. A new writer wrapper joins this table. +func TestWriterWrappers_Informational1xxDoesNotLatch(t *testing.T) { + // Counter readings the metrics row shares between build (before) and + // assert (after) — rows run sequentially, never in parallel. + var meter200Before, meter103Before float64 + // Event accessor the sentry row shares between build and assert. + var sentryEvents func() []*sentry.Event + meter := func(status string) float64 { + // Route is "" (no routeLabel in this chain), method clamps to GET, + // no key validated — the deferred recording's exact label set. + return testutil.ToFloat64(requestsTotal.WithLabelValues("", http.MethodGet, status, "")) + } + + cases := []struct { + name string + // build mounts the middleware that owns the wrapper under test. + build func(t *testing.T, next http.Handler) http.Handler + // finish completes the request after the shared WriteHeader(103). + finish func(w http.ResponseWriter) + // assert checks the final response for the wrapper's latch decision. + assert func(t *testing.T, res *http.Response, body string) + }{ + { + name: "statusWriter meters the real 200", + build: func(t *testing.T, next http.Handler) http.Handler { + meter200Before = meter("200") + meter103Before = meter("103") + return metricsMiddleware(next) + }, + finish: func(w http.ResponseWriter) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + }, + assert: func(t *testing.T, res *http.Response, body string) { + require.Equal(t, http.StatusOK, res.StatusCode) + assert.Equal(t, meter200Before+1, meter("200"), + `the request must meter as status="200" — the final status, not the informational one`) + assert.Equal(t, meter103Before, meter("103"), + `a 103 is never a final status — no status="103" child may grow`) + }, + }, + { + // The 103 must not burn commitWriter's latch: the recovery can + // still honestly write the contract 500 — an informational + // response is not a final one, so the wire is still rewritable. + // (Burned, it would re-panic into a connection abort instead.) + name: "commitWriter keeps the contract 500 after a panic", + build: func(t *testing.T, next http.Handler) http.Handler { + sentryEvents = enableSentry(t).Events + captureErrorLog(t) + return sentryMiddleware(next) + }, + finish: func(w http.ResponseWriter) { + panic("exploded after the 103") + }, + assert: func(t *testing.T, res *http.Response, body string) { + require.Equal(t, http.StatusInternalServerError, res.StatusCode, + "the contract 500 must land — the 103 committed nothing") + assert.Equal(t, "application/json", res.Header.Get("Content-Type")) + assert.JSONEq(t, `{"code":13,"message":"Internal Server Error","details":[]}`, body, + "the panic 500 must keep the contract error shape") + require.Len(t, sentryEvents(), 1, "one panic = one event") + }, + }, + { + name: "cacheControlWriter stamps the real 200", + build: func(t *testing.T, next http.Handler) http.Handler { + return cacheControl(600, next) + }, + finish: func(w http.ResponseWriter) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + }, + assert: func(t *testing.T, res *http.Response, body string) { + require.Equal(t, http.StatusOK, res.StatusCode) + assert.Equal(t, "max-age=600", res.Header.Get("Cache-Control"), + "the 103 must not burn the commit — the real 200 carries the freshness signal") + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := tc.build(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusEarlyHints) // the shared informational write + tc.finish(w) + })) + srv := httptest.NewServer(h) + defer srv.Close() + + var got1xx []int + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req = req.WithContext(httptrace.WithClientTrace(req.Context(), &httptrace.ClientTrace{ + Got1xxResponse: func(code int, _ textproto.MIMEHeader) error { + got1xx = append(got1xx, code) + return nil + }, + })) + + res, err := srv.Client().Do(req) + require.NoError(t, err, "the final response must arrive — a burned latch must not abort the request") + body, err := io.ReadAll(res.Body) + require.NoError(t, err) + require.NoError(t, res.Body.Close()) + srv.Close() // wait out the handler so deferred recording has run + + require.Equal(t, []int{http.StatusEarlyHints}, got1xx, + "the 103 must be forwarded to the wire, not swallowed") + tc.assert(t, res, string(body)) + }) + } +} diff --git a/rest/metrics.go b/rest/metrics.go index 49b8c2f..baf7d46 100644 --- a/rest/metrics.go +++ b/rest/metrics.go @@ -149,8 +149,10 @@ func metricsMiddleware(next http.Handler) http.Handler { // Recording is DEFERRED so a panicking handler still meters — // otherwise panic-per-request reads as a flat error rate while the - // service burns (#92). A panicked request that never wrote a response - // has sw.code == 0, which status() reports as the implied 200; label + // service burns (#92). A panicked request that never wrote a final + // response has sw.code == 0 — a forwarded 1xx captures nothing + // (#165), so a 103-then-panic relabels 500 like any other unwritten + // case — which status() reports as the implied 200; label // it 500 instead — the conventional label for an aborted request // (net/http recovers the panic itself, logs it, and closes the // connection without writing anything; HTTP/2 resets the stream). A @@ -198,13 +200,21 @@ func methodLabel(m string) string { // statusWriter captures the response status for the counter's status label. // A handler that writes a body without an explicit WriteHeader gets the -// net/http implied 200. +// net/http implied 200. Informational (1xx) WriteHeaders capture nothing +// (#165): a 1xx is never the final status, so the label belongs to whatever +// final write follows. type statusWriter struct { http.ResponseWriter code int } func (w *statusWriter) WriteHeader(code int) { + if informational(code) { + // 1xx never commits — forward and keep the capture for the final + // status (#165; rationale on the informational predicate). + w.ResponseWriter.WriteHeader(code) + return + } if w.code == 0 { w.code = code } diff --git a/rest/sentry.go b/rest/sentry.go index bd3874e..e41aa6f 100644 --- a/rest/sentry.go +++ b/rest/sentry.go @@ -284,8 +284,11 @@ func reportServerError(r *http.Request, status int) { }) } -// commitWriter tracks whether anything reached the wire, so the panic -// recovery knows whether the contract 500 can still be written. Created by +// commitWriter tracks whether the FINAL response started on the wire, so the +// panic recovery knows whether the contract 500 can still be written. A +// forwarded informational (1xx) WriteHeader latches nothing (#165): a 1xx +// precedes the final response and leaves it rewritable, exactly as net/http's +// own writer treats it. Created by // the OUTERMOST middleware but the INNERMOST wrapper in write delegation — // writes run gzipWriter → statusWriter → commitWriter → the server's writer // (metrics builds its statusWriter around this one). Unwrap keeps @@ -300,6 +303,14 @@ type commitWriter struct { } func (w *commitWriter) WriteHeader(code int) { + if informational(code) { + // 1xx never commits — forward and keep the latch for the final + // status (#165; rationale on the informational predicate): an + // informational response leaves the wire rewritable, so the recovery + // can still honestly write the contract 500. + w.ResponseWriter.WriteHeader(code) + return + } w.committed = true w.ResponseWriter.WriteHeader(code) } @@ -320,9 +331,11 @@ func (w *commitWriter) Write(b []byte) (int, error) { // committed=true on an untouched wire, sending a later handler panic down the // re-panic path (connection abort) instead of the contract 500 the recovery // can still honestly write. The latch only rolls back when this call was the -// first to set it — after a prior Write/WriteHeader bytes are genuinely on -// the wire and the state keeps. A genuine I/O error also keeps it: by then -// the delegate really flushed, so the commit happened. +// first to set it — after a prior Write or a prior FINAL (non-1xx) +// WriteHeader, bytes of the final response are genuinely on the wire and the +// state keeps (a forwarded 1xx sets no latch at all (#165), so it never +// stands between a first flush and this rollback). A genuine I/O error also +// keeps it: by then the delegate really flushed, so the commit happened. func (w *commitWriter) FlushError() error { latched := !w.committed w.committed = true From 4d8be2ef37d81a2da88975d432b96d87053ebda8 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 14:10:38 -0400 Subject: [PATCH 2/3] =?UTF-8?q?rest:=20exclude=20101=20from=20informationa?= =?UTF-8?q?l()=20=E2=80=94=20stdlib=20commits=20on=20101=20(#165)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-review fixup. net/http routes 1xx through its non-latching informational path EXCEPT 101 Switching Protocols, where it sets wroteHeader and commits; the predicate now mirrors that carve-out so a post-101 panic re-panics instead of "writing" a contract 500 the stdlib silently drops. 101 row added to the boundary pin (red on the old predicate; dropping the carve-out goes red again). The shared wire table now writes both 100 and 103 so inlined-range drift in any one wrapper fails at wire level, not only in the unit pin. Doc sweep: corrected every "stdlib never latches on 1xx" claim (informational.go, informational_internal_test.go, cachecontrol.go, sentry.go, metrics.go), scoped the latch≡wire phrasing leftovers to the FINAL response (sentry FlushError + tests, cachecontrol rollback premise, commitSnapshot, metrics status()/Help/never-wrote family, gzip Content-Length note), tightened the informational.go preface (Expect:100-continue bypasses the wrappers; legacy statusRecorder latches on 1xx, #176), and noted two pre-existing adjacent holes: statusWriter's flush-blind implied 200 (#174) and gzip's Content-Encoding on interim responses (#175). Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/cachecontrol.go | 18 ++++++++----- rest/cachecontrol_test.go | 6 +++-- rest/gzip.go | 8 ++++-- rest/informational.go | 42 ++++++++++++++++++++--------- rest/informational_internal_test.go | 34 ++++++++++++++--------- rest/metrics.go | 26 +++++++++++------- rest/sentry.go | 24 ++++++++++------- rest/sentry_test.go | 18 +++++++------ 8 files changed, 112 insertions(+), 64 deletions(-) diff --git a/rest/cachecontrol.go b/rest/cachecontrol.go index 6201994..b7d5648 100644 --- a/rest/cachecontrol.go +++ b/rest/cachecontrol.go @@ -60,9 +60,11 @@ func cacheControl(maxAgeSeconds int, next http.Handler) http.Handler { // cacheControlWriter injects the Cache-Control header at commit time — the // first FINAL (non-1xx) WriteHeader, Write, or flush — and only when the -// response is a 200. A forwarded informational (1xx) WriteHeader commits -// nothing (#165): the stamp decision belongs to the final status that -// follows, exactly as net/http's own writer leaves 1xx uncommitted. +// response is a 200. A forwarded informational WriteHeader (1xx minus 101 — +// rationale on the informational predicate) commits nothing (#165): the +// stamp decision belongs to the final status that follows, exactly as +// net/http's own writer leaves that set uncommitted (101 latches, there and +// here). // Commit time is the only safe moment: setting the header eagerly would leak // it onto error responses written later (writeError never clears headers), // and onto the contract 500 the sentry layer writes after a handler panic. @@ -79,8 +81,9 @@ type cacheControlWriter struct { func (w *cacheControlWriter) WriteHeader(code int) { if informational(code) { - // 1xx never commits — forward and keep the stamp decision for the - // final status (#165; rationale on the informational predicate). + // A non-latching 1xx (the predicate excludes 101) never commits — + // forward and keep the stamp decision for the final status (#165; + // rationale on the informational predicate). w.ResponseWriter.WriteHeader(code) return } @@ -113,8 +116,9 @@ func (w *cacheControlWriter) Write(b []byte) (int, error) { // made is rolled back. Leaving it would poison the live header map and lie // committed=true: a handler reacting to the failed flush by writing an // error would commit a non-200 carrying max-age, the exact leak class the -// type doc forbids. The rollback's premise — nothing reached the wire, so -// nothing committed — holds for wrappers that fail before writing anything: +// type doc forbids. The rollback's premise — nothing of the final response +// reached the wire, so nothing committed — holds for wrappers that fail +// before writing anything of it: // an unflushable wrapper ABOVE gzip, or a chain with no flushable bottom and // no gzip in between. It is NOT universal: gzip's FlushError (rest/gzip.go) // pushes its header and sync block downstream BEFORE the delegated flush can diff --git a/rest/cachecontrol_test.go b/rest/cachecontrol_test.go index 8b3b118..b74f5e0 100644 --- a/rest/cachecontrol_test.go +++ b/rest/cachecontrol_test.go @@ -58,8 +58,10 @@ func captureHeader(h http.Handler, last *http.Header) http.Handler { // commitSnapshot clones the header map at commit time — the first // WriteHeader, Write, or flush (FlushError, for symmetry with the production // writers) — which is exactly when a real server snapshots headers onto the -// wire. Anything set afterwards is invisible to clients and must stay -// invisible to the battery. +// FINAL response (a first WriteHeader(1xx) is exactly when it does NOT — no +// route in this battery emits one, so the helper keeps the simple +// first-call rule). Anything set afterwards is invisible to clients and +// must stay invisible to the battery. type commitSnapshot struct { http.ResponseWriter last *http.Header diff --git a/rest/gzip.go b/rest/gzip.go index 7e94d70..bddbf2e 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -14,6 +14,10 @@ import ( // // Exported because the legacy gateway chain reuses it until cutover deletes // that stack. +// +// A handler-sent 1xx on a gzip-negotiated request carries Content-Encoding: +// gzip in the interim response — the stdlib sends the live header map per +// RFC 8297 — pre-existing, adjacent to the holes tracked in #175. func GzipMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { @@ -64,8 +68,8 @@ func (w *gzipResponseWriter) FlushError() error { // uncompressed Content-Length must go here too — same staleness Write // handles above; left in place it truncates the compressed stream. An // explicit WriteHeader still commits it (net/http latches the length at - // WriteHeader) — pre-existing hole, reachable on develop without any - // flush, tracked separately. + // a final WriteHeader) — pre-existing hole, reachable on develop without + // any flush, tracked separately. w.Header().Del("Content-Length") if err := w.Writer.Flush(); err != nil { return err diff --git a/rest/informational.go b/rest/informational.go index 0eda1f1..40274d9 100644 --- a/rest/informational.go +++ b/rest/informational.go @@ -1,19 +1,35 @@ package rest -// informational reports whether code is an informational (1xx) status. +import "net/http" + +// informational reports whether code is an informational status that +// net/http forwards WITHOUT committing the response. +// +// That set is 1xx minus 101: net/http's own response writer routes +// `code >= 100 && code <= 199 && code != StatusSwitchingProtocols` through +// its informational path, leaving its wroteHeader latch false — but on a 101 +// it SETS wroteHeader and commits ("We shouldn't send any further headers +// after 101", per the stdlib comment), silently dropping everything after. +// This predicate mirrors that exactly, 101 carve-out included: excluding 101 +// here means the wrappers latch on it just as the stdlib does, so e.g. a +// post-101 panic re-panics instead of "writing" a contract 500 the stdlib +// would swallow. Every writer wrapper in the new-stack rest chain with +// commit-time state (statusWriter's status capture, commitWriter's commit +// latch, cacheControlWriter's header stamp) must mirror that: forward a +// non-latching 1xx WriteHeader to the delegate and latch nothing, so the +// subsequent final WriteHeader behaves exactly as a first call (#165). The +// legacy gateway's statusRecorder still latches on all 1xx — known, tracked +// in #176, dies at cutover. A new rest-chain wrapper with WriteHeader state +// starts here — and joins the shared table in informational_internal_test.go. // -// Informational responses precede the final response on the wire and never -// commit it — net/http's own response writer special-cases 1xx and leaves its -// wroteHeader latch false. Every writer wrapper in the chain with commit-time -// state (statusWriter's status capture, commitWriter's commit latch, -// cacheControlWriter's header stamp) must mirror that: forward a 1xx -// WriteHeader to the delegate and latch nothing, so the subsequent final -// WriteHeader behaves exactly as a first call (#165). A new wrapper with -// WriteHeader state starts here — and joins the shared table in -// informational_internal_test.go. +// (HTTP/2 would treat all 1xx informationally, but RFC 9113 removes 101 from +// HTTP/2 entirely, and this server is plain HTTP/1.1 — latching on 101 is +// safe on both.) // -// The API itself emits no 1xx today; the guard exists so the wrappers stay -// faithful to net/http's commit semantics rather than to current traffic. +// No handler calls WriteHeader with a 1xx today (net/http's automatic +// Expect:100-continue reply bypasses the wrapper chain entirely); the guard +// exists so the wrappers stay faithful to net/http's commit semantics rather +// than to current traffic. func informational(code int) bool { - return code >= 100 && code <= 199 + return code >= 100 && code <= 199 && code != http.StatusSwitchingProtocols } diff --git a/rest/informational_internal_test.go b/rest/informational_internal_test.go index fc0d3bb..525cac5 100644 --- a/rest/informational_internal_test.go +++ b/rest/informational_internal_test.go @@ -1,9 +1,10 @@ package rest -// Shared latch coverage for the chain's writer wrappers (#165): an -// informational (1xx) WriteHeader must pass through to the delegate and latch -// NOTHING — net/http itself never treats 1xx as a commit (the stdlib response -// writer leaves its wroteHeader latch false), so a wrapper that latches there +// Shared latch coverage for the chain's writer wrappers (#165): a +// non-latching informational WriteHeader (1xx minus 101 — net/http commits +// on 101; rationale on the informational predicate) must pass through to the +// delegate and latch NOTHING — the stdlib response writer leaves its +// wroteHeader latch false for that set, so a wrapper that latches there // diverges from the wire: metrics would meter a 103 as the final status, the // panic recovery would abort a connection whose response is still rewritable, // and Cache-Control would miss the real 200. @@ -32,16 +33,18 @@ import ( func TestInformational_Boundaries(t *testing.T) { assert.False(t, informational(99), "99 is not a status class at all") assert.True(t, informational(http.StatusContinue), "100 opens the informational class") + assert.False(t, informational(http.StatusSwitchingProtocols), + "101 is the stdlib's one latching 1xx — net/http commits on it (no headers may follow a 101), so the wrappers must latch too") assert.True(t, informational(http.StatusEarlyHints), "103 is the one seen in the wild") assert.True(t, informational(199), "199 closes the informational class") assert.False(t, informational(http.StatusOK), "200 is a final status — it must latch") } -// TestWriterWrappers_Informational1xxDoesNotLatch drives WriteHeader(103) -// through each writer wrapper's owning middleware and asserts (a) the 103 -// reaches the wire and (b) the wrapper's latch decision still belongs to the -// FINAL response — each row finishes the request the way that makes its own -// latch observable. A new writer wrapper joins this table. +// TestWriterWrappers_Informational1xxDoesNotLatch drives WriteHeader(100) +// then WriteHeader(103) through each writer wrapper's owning middleware and +// asserts (a) both reach the wire and (b) the wrapper's latch decision still +// belongs to the FINAL response — each row finishes the request the way that +// makes its own latch observable. A new writer wrapper joins this table. func TestWriterWrappers_Informational1xxDoesNotLatch(t *testing.T) { // Counter readings the metrics row shares between build (before) and // assert (after) — rows run sequentially, never in parallel. @@ -125,7 +128,14 @@ func TestWriterWrappers_Informational1xxDoesNotLatch(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { h := tc.build(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusEarlyHints) // the shared informational write + // The shared informational writes: both class boundaries the + // stdlib forwards without latching (100 opens it, 103 is the + // one seen in the wild) — so an inlined-range drift in any + // one wrapper fails here at wire level, not only in the + // predicate unit pin. + for _, code := range []int{http.StatusContinue, http.StatusEarlyHints} { + w.WriteHeader(code) + } tc.finish(w) })) srv := httptest.NewServer(h) @@ -148,8 +158,8 @@ func TestWriterWrappers_Informational1xxDoesNotLatch(t *testing.T) { require.NoError(t, res.Body.Close()) srv.Close() // wait out the handler so deferred recording has run - require.Equal(t, []int{http.StatusEarlyHints}, got1xx, - "the 103 must be forwarded to the wire, not swallowed") + require.Equal(t, []int{http.StatusContinue, http.StatusEarlyHints}, got1xx, + "both informational writes must be forwarded to the wire, not swallowed") tc.assert(t, res, string(body)) }) } diff --git a/rest/metrics.go b/rest/metrics.go index baf7d46..cb9b9c6 100644 --- a/rest/metrics.go +++ b/rest/metrics.go @@ -33,7 +33,7 @@ var ( requestsTotal = promauto.With(metricsRegistry).NewCounterVec( prometheus.CounterOpts{ Name: "api_http_requests_total", - Help: "API requests by mux route pattern, method, HTTP status, and validated key id. " + + Help: "API requests by mux route pattern, method, final HTTP status, and validated key id. " + "route is empty when the request never reached routing (the auth 401/503 tiers, or a pre-routing panic); " + "\"/\" is the catch-all (unknown path / wrong method / clean-path 307). " + "key_id is empty when no key validated.", @@ -155,9 +155,13 @@ func metricsMiddleware(next http.Handler) http.Handler { // case — which status() reports as the implied 200; label // it 500 instead — the conventional label for an aborted request // (net/http recovers the panic itself, logs it, and closes the - // connection without writing anything; HTTP/2 resets the stream). A + // connection without writing a response; HTTP/2 resets the stream). A // handler that already committed a status before panicking keeps that - // status — it is on the wire. The panic is re-raised AFTER recording + // status — it is on the wire. One gap: statusWriter has no FlushError, + // so a flush-committed implied 200 is invisible to this capture — a + // 103-then-flush-then-panic meters 500 against a 200 already committed + // on the wire. Pre-existing blind spot, tracked in #174. + // The panic is re-raised AFTER recording // (the inner defer fires as this deferred func returns) so the sentry // recovery layer outside this one (#132) — and net/http when sentry // is disabled — sees semantics unchanged. @@ -200,9 +204,10 @@ func methodLabel(m string) string { // statusWriter captures the response status for the counter's status label. // A handler that writes a body without an explicit WriteHeader gets the -// net/http implied 200. Informational (1xx) WriteHeaders capture nothing -// (#165): a 1xx is never the final status, so the label belongs to whatever -// final write follows. +// net/http implied 200. Informational WriteHeaders (1xx minus 101 — +// rationale on the informational predicate) capture nothing (#165): those +// are never the final status, so the label belongs to whatever final write +// follows. type statusWriter struct { http.ResponseWriter code int @@ -210,8 +215,9 @@ type statusWriter struct { func (w *statusWriter) WriteHeader(code int) { if informational(code) { - // 1xx never commits — forward and keep the capture for the final - // status (#165; rationale on the informational predicate). + // A non-latching 1xx (the predicate excludes 101) never commits — + // forward and keep the capture for the final status (#165; + // rationale on the informational predicate). w.ResponseWriter.WriteHeader(code) return } @@ -235,8 +241,8 @@ func (w *statusWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter } -// status returns the captured status; a handler that never wrote anything is -// the implied 200, same as net/http reports it. +// status returns the captured status; a handler that never wrote a final +// response is the implied 200, same as net/http reports it. func (w *statusWriter) status() int { if w.code == 0 { return http.StatusOK diff --git a/rest/sentry.go b/rest/sentry.go index e41aa6f..41fe594 100644 --- a/rest/sentry.go +++ b/rest/sentry.go @@ -286,9 +286,11 @@ func reportServerError(r *http.Request, status int) { // commitWriter tracks whether the FINAL response started on the wire, so the // panic recovery knows whether the contract 500 can still be written. A -// forwarded informational (1xx) WriteHeader latches nothing (#165): a 1xx -// precedes the final response and leaves it rewritable, exactly as net/http's -// own writer treats it. Created by +// forwarded informational WriteHeader (1xx minus 101 — rationale on the +// informational predicate) latches nothing (#165): those precede the final +// response and leave it rewritable, exactly as net/http's own writer treats +// them (101 is the exception: the stdlib commits on it, and so does this +// latch). Created by // the OUTERMOST middleware but the INNERMOST wrapper in write delegation — // writes run gzipWriter → statusWriter → commitWriter → the server's writer // (metrics builds its statusWriter around this one). Unwrap keeps @@ -304,10 +306,11 @@ type commitWriter struct { func (w *commitWriter) WriteHeader(code int) { if informational(code) { - // 1xx never commits — forward and keep the latch for the final - // status (#165; rationale on the informational predicate): an - // informational response leaves the wire rewritable, so the recovery - // can still honestly write the contract 500. + // A non-latching 1xx (the predicate excludes 101) never commits — + // forward and keep the latch for the final status (#165; rationale + // on the informational predicate): an informational response leaves + // the wire rewritable, so the recovery can still honestly write the + // contract 500. w.ResponseWriter.WriteHeader(code) return } @@ -326,9 +329,10 @@ func (w *commitWriter) Write(b []byte) (int, error) { // (http.ErrNotSupported surfaces naturally when nothing below can flush). // // If the delegated flush reports http.ErrNotSupported, no layer below could -// flush — nothing reached the wire — and the latch this call set is rolled -// back (#164; mirror of cacheControlWriter's rollback): leaving it would lie -// committed=true on an untouched wire, sending a later handler panic down the +// flush — nothing of the final response reached the wire — and the latch this +// call set is rolled back (#164; mirror of cacheControlWriter's rollback): +// leaving it would lie committed=true on a wire the final response never +// touched, sending a later handler panic down the // re-panic path (connection abort) instead of the contract 500 the recovery // can still honestly write. The latch only rolls back when this call was the // first to set it — after a prior Write or a prior FINAL (non-1xx) diff --git a/rest/sentry_test.go b/rest/sentry_test.go index 6eafd88..111dc69 100644 --- a/rest/sentry_test.go +++ b/rest/sentry_test.go @@ -626,12 +626,13 @@ func TestSentry_PanicAfterFlushRepanicsInsteadOfRewriting(t *testing.T) { require.Len(t, tr.Events(), 1, "the panic is still captured even when the response cannot be rewritten") } -// A first flush the delegate fails with http.ErrNotSupported sent NOTHING — -// no layer below could flush, so the wire is untouched (#164). The committed -// latch FlushError itself set must roll back (mirror of cacheControlWriter's -// #163 R1 rollback: latch-was-ours + errors.Is), or a later handler panic -// takes the committed re-panic path and aborts the connection instead of -// writing the contract 500 over a genuinely untouched wire. +// A first flush the delegate fails with http.ErrNotSupported sent NOTHING of +// the final response — no layer below could flush, so the wire the final +// response would land on is untouched (#164). The committed latch FlushError +// itself set must roll back (mirror of cacheControlWriter's #163 R1 rollback: +// latch-was-ours + errors.Is), or a later handler panic takes the committed +// re-panic path and aborts the connection instead of writing the contract +// 500 over a wire the final response genuinely never touched. // // The reachable trigger is a BASE writer below sentryMiddleware with no flush // support: test harnesses today (noFlushWriter here), plausibly a cutover-era @@ -674,8 +675,9 @@ func TestSentry_PanicAfterFailedFirstFlushWritesContract500(t *testing.T) { } // The latch-was-ours guard on the #164 rollback: a failed flush AFTER a prior -// Write or WriteHeader must NOT reset the latch — bytes (or the status line) -// are genuinely on the wire, so the only honest panic semantics left are the +// Write or a prior FINAL (non-1xx) WriteHeader must NOT reset the latch — +// bytes (or the final status line) are genuinely on the wire, so the only +// honest panic semantics left are the // committed path's re-panic and connection abort. A rollback here would write // a contract 500 behind a response already started — the exact corruption // commitWriter exists to prevent. From fa1b632d3d0fc70e2c46e9aeff63800b1f562bc6 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 14:36:39 -0400 Subject: [PATCH 3/3] rest: wire-level 101 latch witness + carve-out doc alignment (#165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 fixups. TestWriterWrappers_101Latches pins the 101 carve-out at wire level per wrapper (own request each — nothing may follow a 101): commitWriter re-panics a 101-then-panic into an honest connection abort (one sentry event) instead of a clean 101; statusWriter meters the committed status="101", not the unwritten-case 500 relabel; cacheControlWriter's burned commit refuses a post-101 stamp (live-map peek — the stdlib's own Header() snapshot masks that stamp at the wire). Each row kills its wrapper's inlined carve-out-less-range mutant; the commitWriter mutant survived the entire prior suite. Doc sweep: the latching set is "non-1xx, plus 101" — replaced the stale "FINAL (non-1xx)" equation and the two now-false 1xx generalisations (sentry.go rollback note, metrics.go relabel note) with the non-latching-1xx vocabulary and predicate pointers; precision fixes in the satellites. Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/cachecontrol.go | 3 +- rest/cachecontrol_internal_test.go | 3 +- rest/cachecontrol_test.go | 6 +- rest/informational.go | 4 +- rest/informational_internal_test.go | 146 ++++++++++++++++++++++++++-- rest/metrics.go | 25 +++-- rest/sentry.go | 11 ++- rest/sentry_test.go | 6 +- 8 files changed, 175 insertions(+), 29 deletions(-) diff --git a/rest/cachecontrol.go b/rest/cachecontrol.go index b7d5648..5c46404 100644 --- a/rest/cachecontrol.go +++ b/rest/cachecontrol.go @@ -59,7 +59,8 @@ func cacheControl(maxAgeSeconds int, next http.Handler) http.Handler { } // cacheControlWriter injects the Cache-Control header at commit time — the -// first FINAL (non-1xx) WriteHeader, Write, or flush — and only when the +// first latching WriteHeader (non-1xx, or the 101 carve-out — see the +// informational predicate), Write, or flush — and only when the // response is a 200. A forwarded informational WriteHeader (1xx minus 101 — // rationale on the informational predicate) commits nothing (#165): the // stamp decision belongs to the final status that follows, exactly as diff --git a/rest/cachecontrol_internal_test.go b/rest/cachecontrol_internal_test.go index fbb67d3..b98e761 100644 --- a/rest/cachecontrol_internal_test.go +++ b/rest/cachecontrol_internal_test.go @@ -43,7 +43,8 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) { assert.Empty(t, rr.Header().Get("Cache-Control")) }) - // An informational WriteHeader latches nothing (#165) — and in particular + // A non-latching 1xx WriteHeader (the predicate carves out 101) latches + // nothing (#165) — and in particular // must not stamp: a stamp on the 1xx would sit in the live map and leak // onto whatever final status follows, here a 500 (the leak class the type // doc forbids). The stamp-on-the-real-200 direction needs real 1xx wire diff --git a/rest/cachecontrol_test.go b/rest/cachecontrol_test.go index b74f5e0..1671d40 100644 --- a/rest/cachecontrol_test.go +++ b/rest/cachecontrol_test.go @@ -58,9 +58,9 @@ func captureHeader(h http.Handler, last *http.Header) http.Handler { // commitSnapshot clones the header map at commit time — the first // WriteHeader, Write, or flush (FlushError, for symmetry with the production // writers) — which is exactly when a real server snapshots headers onto the -// FINAL response (a first WriteHeader(1xx) is exactly when it does NOT — no -// route in this battery emits one, so the helper keeps the simple -// first-call rule). Anything set afterwards is invisible to clients and +// FINAL response (a first non-latching 1xx WriteHeader is exactly when it +// does NOT — no route in this battery emits any 1xx, so the helper keeps the +// simple first-call rule). Anything set afterwards is invisible to clients and // must stay invisible to the battery. type commitSnapshot struct { http.ResponseWriter diff --git a/rest/informational.go b/rest/informational.go index 40274d9..6ba69c1 100644 --- a/rest/informational.go +++ b/rest/informational.go @@ -9,7 +9,9 @@ import "net/http" // `code >= 100 && code <= 199 && code != StatusSwitchingProtocols` through // its informational path, leaving its wroteHeader latch false — but on a 101 // it SETS wroteHeader and commits ("We shouldn't send any further headers -// after 101", per the stdlib comment), silently dropping everything after. +// after 101", per the stdlib comment), dropping everything after from the +// wire — wire-silent only: the server logs each superfluous WriteHeader, and +// Write returns http.ErrBodyNotAllowed. // This predicate mirrors that exactly, 101 carve-out included: excluding 101 // here means the wrappers latch on it just as the stdlib does, so e.g. a // post-101 panic re-panics instead of "writing" a contract 500 the stdlib diff --git a/rest/informational_internal_test.go b/rest/informational_internal_test.go index 525cac5..b8e2d4f 100644 --- a/rest/informational_internal_test.go +++ b/rest/informational_internal_test.go @@ -11,10 +11,11 @@ package rest // // The observations are wire-faithful: a real httptest server (the stdlib's // actual 1xx machinery, which a ResponseRecorder does not have) with the -// forwarded 103 captured via httptrace on the client side. +// forwarded 100 and 103 captured via httptrace on the client side. import ( "io" + "log" "net/http" "net/http/httptest" "net/http/httptrace" @@ -27,9 +28,10 @@ import ( "github.com/stretchr/testify/require" ) -// The predicate's boundaries pin the wire definition exactly: 1xx is -// [100, 199], nothing more — a guard that drifts to e.g. (100, 199] would -// silently latch on a forwarded 100 Continue. +// The predicate's boundaries pin the wire definition exactly: the +// non-latching set is [100, 199] minus the 101 carve-out (net/http commits +// on 101) — a guard that drifts to e.g. (100, 199] would silently latch on a +// forwarded 100 Continue. func TestInformational_Boundaries(t *testing.T) { assert.False(t, informational(99), "99 is not a status class at all") assert.True(t, informational(http.StatusContinue), "100 opens the informational class") @@ -128,9 +130,10 @@ func TestWriterWrappers_Informational1xxDoesNotLatch(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { h := tc.build(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // The shared informational writes: both class boundaries the - // stdlib forwards without latching (100 opens it, 103 is the - // one seen in the wild) — so an inlined-range drift in any + // The shared informational writes: the opening boundary plus + // the code seen in the wild, both forwarded by the stdlib + // without latching (100 opens the class, 103 is the 1xx real + // traffic carries) — so an inlined-range drift in any // one wrapper fails here at wire level, not only in the // predicate unit pin. for _, code := range []int{http.StatusContinue, http.StatusEarlyHints} { @@ -164,3 +167,132 @@ func TestWriterWrappers_Informational1xxDoesNotLatch(t *testing.T) { }) } } + +// TestWriterWrappers_101Latches is the wire-level witness for the predicate's +// 101 carve-out: each wrapper must LATCH on WriteHeader(101) exactly as +// net/http commits on it. It cannot share the {100, 103} table above — +// nothing may follow a 101, so every row gets its own request. +// +// The dangerous wrapper is commitWriter: with the carve-out dropped (an +// inlined carve-out-less 1xx range, say) a 101 forwards without latching, so +// a 101-then-panic leaves committed == false and the recovery "writes" the +// contract 500 — which the stdlib swallows (superfluous WriteHeader, Write +// returns http.ErrBodyNotAllowed) before flushing the 101 it committed on — +// and the panic dissolves into a CLEAN 101 to the client. The honest +// behaviour, pinned here, is the committed path: re-panic, connection abort, +// hard client error. The sibling rows pin the carve-out's cheap consequences +// in the other two wrappers: statusWriter captures the 101 (a 101-then-panic +// meters the genuinely committed status="101", not the unwritten-case 500 +// relabel), and cacheControlWriter commits on the 101 (no stamp — not a +// 200 — and the burned commit means a post-101 Write must not stamp either). +// That last one needs a live-map peek from inside the handler: the stdlib +// masks a late stamp at the wire all by itself (response.Header() snapshots +// the logically-written state on first access after commit), so the live map +// is the one observation point where a non-latching wrapper's stamp shows. +func TestWriterWrappers_101Latches(t *testing.T) { + // Shared between build (before) and check (after) — rows run + // sequentially, never in parallel. Same conventions as the table above. + var meter101Before, meter500Before float64 + var sentryEvents func() []*sentry.Event + meter := func(status string) float64 { + return testutil.ToFloat64(requestsTotal.WithLabelValues("", http.MethodGet, status, "")) + } + + cases := []struct { + name string + // build mounts the middleware that owns the wrapper under test. + build func(t *testing.T, next http.Handler) http.Handler + // finish completes the request after the shared WriteHeader(101). + // It runs on the server goroutine — assert only, never require. + finish func(t *testing.T, w http.ResponseWriter) + // check sees the raw client outcome: a latched 101 makes some rows + // end in a deliberate connection abort, not a response. + check func(t *testing.T, res *http.Response, err error) + }{ + { + name: "commitWriter re-panics into a connection abort", + build: func(t *testing.T, next http.Handler) http.Handler { + sentryEvents = enableSentry(t).Events + captureErrorLog(t) + return sentryMiddleware(next) + }, + finish: func(_ *testing.T, w http.ResponseWriter) { + panic("exploded after the 101") + }, + check: func(t *testing.T, res *http.Response, err error) { + require.Error(t, err, + "the 101 committed the response — the recovery must re-panic into a connection abort, not dissolve the panic into a clean 101") + require.Len(t, sentryEvents(), 1, "one panic = one event — the re-panic path still captures") + }, + }, + { + name: "statusWriter meters the committed 101", + build: func(t *testing.T, next http.Handler) http.Handler { + meter101Before = meter("101") + meter500Before = meter("500") + return metricsMiddleware(next) + }, + finish: func(_ *testing.T, w http.ResponseWriter) { + panic("exploded after the 101") + }, + check: func(t *testing.T, res *http.Response, err error) { + require.Error(t, err, + "no recovery layer in this chain — net/http aborts the connection") + assert.Equal(t, meter101Before+1, meter("101"), + `a 101 commits, so the capture keeps it: a 101-then-panic meters as status="101"`) + assert.Equal(t, meter500Before, meter("500"), + "the 500 relabel is only for panics with NO committed status — the 101 was committed") + }, + }, + { + name: "cacheControlWriter commits on the 101 and never stamps it", + build: func(t *testing.T, next http.Handler) http.Handler { + return cacheControl(600, next) + }, + finish: func(t *testing.T, w http.ResponseWriter) { + // Wire-dead after a 101 (the stdlib returns + // http.ErrBodyNotAllowed), but a non-latching wrapper would + // stamp the live header map here. assert (never require — + // this runs on the server goroutine, and FailNow is not + // goroutine-safe) on the map directly: the stdlib's own + // Header() snapshot keeps such a stamp off the wire, so this + // peek is what makes the row a mutant witness and not just a + // wire pin — see the test doc. + _, _ = w.Write([]byte("{}")) + assert.Empty(t, w.Header().Get("Cache-Control"), + "the 101 burned the commit — a post-101 Write must not stamp even the live map") + }, + check: func(t *testing.T, res *http.Response, err error) { + require.NoError(t, err, "a 101 without a panic is a clean response") + require.Equal(t, http.StatusSwitchingProtocols, res.StatusCode) + assert.Empty(t, res.Header.Get("Cache-Control"), + "a 101 is not a 200 — the freshness signal must never reach the wire, not even via a post-101 Write") + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewUnstartedServer(tc.build(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusSwitchingProtocols) + tc.finish(t, w) + }))) + // The panic rows re-raise into net/http's per-connection + // recovery, which logs the abort — keep it out of test output. + srv.Config.ErrorLog = log.New(io.Discard, "", 0) + srv.Start() + defer srv.Close() + + res, err := srv.Client().Get(srv.URL + "/") + if res != nil { + // A 101 body is the raw connection (protocol-switch + // semantics, no Content-Length) — close it WITHOUT reading: + // nothing more is coming, and a read could block on a socket + // the server may hold open. + require.NoError(t, res.Body.Close()) + } + srv.Close() // wait out the handler so deferred recording has run + tc.check(t, res, err) + }) + } +} diff --git a/rest/metrics.go b/rest/metrics.go index cb9b9c6..150e99c 100644 --- a/rest/metrics.go +++ b/rest/metrics.go @@ -150,12 +150,15 @@ func metricsMiddleware(next http.Handler) http.Handler { // Recording is DEFERRED so a panicking handler still meters — // otherwise panic-per-request reads as a flat error rate while the // service burns (#92). A panicked request that never wrote a final - // response has sw.code == 0 — a forwarded 1xx captures nothing - // (#165), so a 103-then-panic relabels 500 like any other unwritten - // case — which status() reports as the implied 200; label - // it 500 instead — the conventional label for an aborted request - // (net/http recovers the panic itself, logs it, and closes the - // connection without writing a response; HTTP/2 resets the stream). A + // response has sw.code == 0, which status() reports as the implied + // 200; label it 500 instead — the conventional label for an aborted + // request (net/http recovers the panic itself, logs it, and closes + // the connection without writing a response; HTTP/2 resets the + // stream). A forwarded non-latching 1xx (the predicate excludes 101) + // captures nothing (#165), so a 103-then-panic relabels 500 like any + // other unwritten case — but a 101-then-panic keeps the captured 101: + // the stdlib committed on it, so that status, not the relabel, is the + // honest one. A // handler that already committed a status before panicking keeps that // status — it is on the wire. One gap: statusWriter has no FlushError, // so a flush-committed implied 200 is invisible to this capture — a @@ -204,10 +207,12 @@ func methodLabel(m string) string { // statusWriter captures the response status for the counter's status label. // A handler that writes a body without an explicit WriteHeader gets the -// net/http implied 200. Informational WriteHeaders (1xx minus 101 — -// rationale on the informational predicate) capture nothing (#165): those -// are never the final status, so the label belongs to whatever final write -// follows. +// net/http implied 200. Non-latching informational WriteHeaders (1xx minus +// 101 — rationale on the informational predicate) capture nothing (#165): +// those are never the final status, so the label belongs to whatever final +// write follows. A 101 IS captured, like a final status — the stdlib commits +// on it — so it meters as status="101" (the carve-out's consequence here, +// symmetric with commitWriter's latch and cacheControlWriter's commit). type statusWriter struct { http.ResponseWriter code int diff --git a/rest/sentry.go b/rest/sentry.go index 41fe594..bb69494 100644 --- a/rest/sentry.go +++ b/rest/sentry.go @@ -335,10 +335,13 @@ func (w *commitWriter) Write(b []byte) (int, error) { // touched, sending a later handler panic down the // re-panic path (connection abort) instead of the contract 500 the recovery // can still honestly write. The latch only rolls back when this call was the -// first to set it — after a prior Write or a prior FINAL (non-1xx) -// WriteHeader, bytes of the final response are genuinely on the wire and the -// state keeps (a forwarded 1xx sets no latch at all (#165), so it never -// stands between a first flush and this rollback). A genuine I/O error also +// first to set it — after a prior Write or a prior latching WriteHeader (a +// final status, or the 101 carve-out — see the informational predicate) the +// commit already happened and the state keeps: bytes or the final status +// line are genuinely out, or the stdlib latched on the 101 itself. (A +// forwarded non-latching 1xx sets no latch at all (#165); a 101 latches like +// a final status, so a flush after it correctly finds the latch already set +// and never rolls back.) A genuine I/O error also // keeps it: by then the delegate really flushed, so the commit happened. func (w *commitWriter) FlushError() error { latched := !w.committed diff --git a/rest/sentry_test.go b/rest/sentry_test.go index 111dc69..d5e6c4b 100644 --- a/rest/sentry_test.go +++ b/rest/sentry_test.go @@ -675,8 +675,10 @@ func TestSentry_PanicAfterFailedFirstFlushWritesContract500(t *testing.T) { } // The latch-was-ours guard on the #164 rollback: a failed flush AFTER a prior -// Write or a prior FINAL (non-1xx) WriteHeader must NOT reset the latch — -// bytes (or the final status line) are genuinely on the wire, so the only +// Write or a prior latching WriteHeader (a final status, or the 101 +// carve-out — see the informational predicate) must NOT reset the latch — +// the commit already happened (bytes or the final status line genuinely out, +// or the stdlib's own 101 latch set), so the only // honest panic semantics left are the // committed path's re-panic and connection abort. A rollback here would write // a contract 500 behind a response already started — the exact corruption