caddyhttp: don't flush a hijacked response

[jotikrishna]

Problem

Since #7945, Server.ServeHTTP flushes the response from a deferred func when an HTTP/1.x request that is being access-logged took at least write_timeout. The deferred func already skips http.ErrHijacked, on the assumption that Flush reports a hijacked response that way.

It does not. When the handler hijacked the connection - as reverse_proxy does when it passes through a 101 Switching Protocols upgrade - and the upgraded stream outlives write_timeout, that deferred flush panics:

{"level":"error","logger":"http.stdlib","msg":"http: panic serving 127.0.0.1:56866: runtime error: invalid memory address or nil pointer dereference
...
bufio.(*Writer).Flush(0x1078443d8?)
	net/http/server.go:1728
net/http.(*response).FlushError(0x637fdd514000)
	net/http/responsecontroller.go:52
github.com/caddyserver/caddy/v2/modules/caddyhttp.(*responseRecorder).FlushError(...)
	modules/caddyhttp/responsewriter.go:244
net/http.(*ResponseController).Flush(0x1421bcc2e?)
	net/http/responsecontroller.go:52
github.com/caddyserver/caddy/v2/modules/caddyhttp.(*Server).ServeHTTP.func1()
	modules/caddyhttp/server.go:692

Root Cause

(*http.response).Hijack releases the bufio.Writer and sets w.w = nil. (*http.response).FlushError calls w.w.Flush() without checking hijacked(); only Write and WriteHeader consult it and return http.ErrHijacked.

So Flush cannot report a hijack the way the deferred func expects, and the flush dereferences a nil writer. net/http recovers the panic, and because it fires only after the upgraded stream has ended, clients never notice - but every upgraded connection that outlives write_timeout ends with an error-level stack trace in the logs. Reported in production against headscale behind reverse_proxy (/ts2021 and /derp, both HTTP/1.1 upgrades) with write 30s, so practically every client session produced one.

Solution

responseRecorder already tracks the hijack: #7649 added the hijacked field and uses it in WriteResponse and DetachAfterHijack. FlushError simply did not consult it.

Return http.ErrHijacked from FlushError when the response was hijacked. The deferred flush in ServeHTTP already handles that error, so the loop closes with no change to callers:

func (rr *responseRecorder) FlushError() error {
	// Hijack releases the underlying bufio.Writer, and net/http's FlushError
	// dereferences it without checking for the hijack, so flushing after an
	// upgrade panics with a nil pointer dereference. Write and WriteHeader
	// already report the hijack; FlushError has to do the same. See #8151.
	if rr.hijacked {
		return http.ErrHijacked
	}
	if rr.stream {
		//nolint:bodyclose
		return http.NewResponseController(rr.ResponseWriterWrapper).Flush()
	}
	return nil
}

Seven insertions in one file. The guard belongs on the recorder because the recorder is the layer that performed the hijack, so it is the layer that can answer whether the response was hijacked.

Worth noting for reviewers: #7649 also sets rr.stream = true inside Hijack, so the if rr.stream branch is now always taken for a hijacked streaming response. That commit made this path deterministic rather than incidental.

Testing

Environment: Go 1.27.0 windows/amd64, base c1acb2f (current master).

Baseline before any edit: go test ./modules/caddyhttp/ -> ok 7.939s, with no fix and no test file present. Clean, so the failures below are attributable to the new tests rather than pre-existing breakage.

Regression test fails before the fix (guard removed, everything else identical):

--- FAIL: TestResponseRecorderFlushAfterHijack (0.00s)
    flushing a hijacked response panicked: runtime error: invalid memory address or nil pointer dereference
--- FAIL: TestServerServeHTTPDeferredFlushAfterHijack (0.01s)
    ServeHTTP panicked after an upgraded connection outlived write_timeout: ...

The panic string matches the one in the issue exactly.

All four tests pass after the fix:

--- PASS: TestResponseRecorderFlushAfterHijack (0.00s)
--- PASS: TestResponseRecorderFlushWithoutHijack (0.00s)
--- PASS: TestResponseRecorderBufferedFlushUnaffected (0.00s)
--- PASS: TestServerServeHTTPDeferredFlushAfterHijack (0.01s)
ok  	github.com/caddyserver/caddy/v2/modules/caddyhttp	0.391s

TestServerServeHTTPDeferredFlushAfterHijack drives the real Server.ServeHTTP path with a handler that writes 101, hijacks, and then outlives write_timeout, which is the scenario requested below. The two other tests are controls: a non-hijacked streamed response must still flush exactly once, and a buffered response must still suppress the flush (#6144).

Check	Command	Result
Package suite	go test ./modules/caddyhttp/	ok 7.887s
Subsystem suite	go test ./modules/caddyhttp/...	ok, 14/14 packages
Build	go build ./...	exit 0
Vet	go vet ./modules/caddyhttp/...	exit 0
Formatting	gofmt -l on both files	clean

The test uses an httptest-style fake that replicates net/http's hijack semantics (a Flush after Hijack faults, exactly as stdlib does) rather than a real socket, so it is deterministic and needs no network. No nc or external process is involved.

Not run, for transparency: golangci-lint is not installed in my environment, so the repository's own lint config has not been executed against this change.

Impact

Upgraded connections that outlive write_timeout stop logging an error-level panic trace. The access-log entry itself is unchanged (status: 101, same duration) - only the spurious stack trace disappears.

Compatibility

No API change. No behaviour change for non-hijacked responses. No dependency change.

References

Fixes #8151

---

Maintainer direction this responds to

steadytao (MEMBER, 2026-10-05): "The recorder-level hijack state looks like the right fix. Please open a PR with the patch and a regression covering an upgraded connection which outlives write_timeout."

Assistance disclosure

AI was used to write this change and its tests; I reviewed the diff and ran every check listed above myself. The issue's own Assistance Disclosure notes the reporter reached the same root cause with AI assistance - this is an independent implementation from a different contributor, disclosed here so the overlap is visible to reviewers rather than something they have to discover.

[CLAassistant]

All committers have signed the CLA.

[WeidiDeng]

WriteHeader doesn't return anything.

[WeidiDeng]

The same as above.

[WeidiDeng]

Implement FlushError instead of Flush.

[WeidiDeng]

WriteHeader has an error log for it.

[WeidiDeng]

The same as above.

[steadytao]

Standard Caddy header to the test file.