Skip to content

fix(websocket): Conn.Param, IP and RemoteAddr answer from the upgrade request on every engine; every test helper that starts a server fails at once with Start's error (celeris#721, celeris#706) - #918

Merged
FumingPower3925 merged 10 commits into
mainfrom
fix/celeris-721-706-ws-conn-params-start-errors
Oct 3, 2026

Conversation

@FumingPower3925

@FumingPower3925 FumingPower3925 commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #721. Fixes #706.

What was wrong

#721, middleware/websocket. Lines are at the base 609a015. middleware/websocket/ is byte-identical there to 2ca5d72, where the lane first built this PR.

  • Conn.Param read c.params (conn.go:950), which nothing ever set: setupConn (websocket.go:318-326) set the headers and the query only. So Param returned "" on every engine, ever since the middleware landed (v1.3.4).
  • Conn.IP and Conn.RemoteAddr asked c.conn (conn.go:880-925), the hijacked net.Conn. The engine path (epoll, io_uring, and Adaptive, which runs them) has none, so IP() returned "" and RemoteAddr() returned nil, although the Context knows the peer (Context.RemoteAddr).
  • On the way: IP() cached its result on its first call with an unsynchronized write (conn.go:909-926). When the read and the write goroutine called it at once, that was a data race: -race reports it on the std path today, and this PR would have extended it to the engine path.
  • Found in review round 1: on a dual-stack listener ("[::]:port"; no engine sets IPV6_V6ONLY), the engines report an IPv4 peer as "[127.0.0.1]:port". netip.ParseAddrPort rejects that form, so RemoteAddr() returned a raw string address, not the *net.TCPAddr its doc promises. The test compared only Network() and String(), which the raw form reproduces, so it did not catch this. The engine-side formatting is a separate defect, filed as engines: an IPv4 client of a dual-stack listener is reported as "[127.0.0.1]:port", and Server.Addr of an IPv6 listener as "[::%0]:port" (epoll, io_uring, adaptive) #925 (it reaches every handler's Context.RemoteAddr).

#706, the rest of it. #730 fixed the WebSocket and static waitForReady. The issue's last step covers every other helper that starts a server in a goroutine and then waits for it. Those helpers were audited in #706's comment at 2776d4c. The lane re-checked them at 2ca5d72 (scripts/c706-audit.sh, logs/c706-audit.txt): all 18 sites (17 files) are still there, and the 23 other test files that start a server in a goroutine and changed since then all read Start's result while they wait (soak_test.go through #730's waitForReady). Twelve sites never read Start's error while waiting. A failed start then burns the whole wait (2-30 s) and reports a bare "not ready", or the test goes on against no server, or it dereferences a nil Addr(). Six sites read the error once, 500 ms in, so a start that fails later is missed and the test drives a server that is not there. Note: since #737, a Start that fails before any engine runs closes the listener it was handed. The helpers that dial that listener therefore no longer report a dead server ready; they time out instead, without the cause.

Re-checked again for round 1 at the new base 609a015 (fix-r1/scripts/c706-reaudit.sh, fix-r1/logs/c706-reaudit-609a015.txt, with a positive control). Of the 21 test files added or changed since 2ca5d72, four start a server in a goroutine: auto_head_options_engines_421_linux_test.go, middleware/cache/slow_leader_913_linux_test.go, middleware/sse/sse_head_421_linux_test.go and middleware/sse/sse_onconnect_reject_835_linux_test.go. All four pass Start's result to a wait that selects on it (waitReady761, waitReady913, c714WaitReady, waitReady835).

The fix

#721. At upgrade, next to captureQuery:

  • captureParams reads the matched route pattern (c.FullPath(), whose names the router owns) and records one pair per named parameter (:room) and catch-all (*path), with c.Param's value. On the engine path it runs after Detach, which copies the params, so the values are copies. On the hijack path it clones them. It parses the names by the rule the router splits a pattern with (router_tree.go, splitPath and findSegmentEnd), and its comment now says so (round 2), because Context has no accessor for its params' names.
  • The engine path keeps the address the engine reported for the connection as a *net.TCPAddr, so RemoteAddr() returns the same type as the hijack path's net.Conn. peerAddr parses the address with netip.ParseAddrPort; when that fails, it parses the host and the port on their own, which accepts the dual-stack form "[a.b.c.d]:port" (round 1). An address that still does not parse is kept as a string net.Addr.
  • IP() is computed once, in setupConn, and only read afterwards.
  • LocalAddr() stays nil on the engine path (the request does not carry it); its doc now says so. Exposing it is an item on v1.6.0 polish: middleware #888.

Cost: per upgrade, not per message. Measured in the package at this head (fix-r1/scripts/wsallocs.sh, fix-r1/logs/wsallocs.log); allocation counts are exact.

path what allocations bytes
engine peerAddr + IP()'s value, IPv4 peer ("127.0.0.1:54321") 5 (2 + 3) 88
engine the same, dual-stack form ("[127.0.0.1]:54321") 7 (4 + 3) 200
engine the same, IPv6 peer ("[2001:db8::1]:443") 5 104
hijack (std) IP()'s value from the net.Conn's *net.TCPAddr, now computed at upgrade. The old lazy version paid the same on the first IP() call and nothing if IP() was never called 3 32
both params: none for a route without params; one slice on the engine path (/ws/:room/*rest); on the hijack path the slice plus one clone per param 0 / 1 / 1 + n

The first version of this body said "one [][2]string and one *net.TCPAddr". That was not measured, and it was low.

#706. Each site now reads Start's error while it waits:

  • Where several sites in a test package share the shape, the package gets a helper. waitServerStarted (root, root external tests, middleware) waits for s.Addr() to answer and fails at once with Start's error, putting the error back in done for the caller's shutdown. waitUpOrStopped (middleware/sse, middleware/websocket external tests, engine/epoll) waits, after the existing 500 ms settle sleep, until the server publishes its address or Start returns, so the existing skip-or-fail on Start's error then runs with the real error.
  • The rest are fixed inline. sse_disconnect_linux_test.go checks done on every poll instead of only after its 30 s deadline. The celeris_closeprobe drain test and engine/std/sse_shutdown_test.go select on their serverDone. test/drivercmp/redis/integrated_async_test.go stops with Start's error instead of a nil Addr() dereference. server_async_test.go's GETs get a timeout.
  • Every helper has its own test: Start fails at once (an engine type no factory knows), and the helper must report Start's error within 5 s and leave it in done. The round-1 test's own helper, start721WSServer, waits the same way, and for the server's address before it dials: the native engines close the listener they are handed and bind the port again, and until then that listener's backlog accepts a dial that the engine then resets.

Hunks in engine/epoll (2 lines in each of 2 tests, plus a new test file) and engine/std (one wait loop) are small, for the #443 move.

Tests and controls (#721)

Round 1 was rebased onto 4bb2042, and then onto 609a015 when #923 merged (it changes only middleware/cache). Both rebases had no conflict; git range-diff shows every patch identical, and every file this PR touches is the same at 0aaa6e3 and at the pushed head c1f8e05 (fix-r1/logs/rebase-609a015-C.txt). The head set ran again at c1f8e05: 9 of 9 tests, 14 of 14 arms, 0 SKIP (fix-r1/logs/c-head3.log). The other runs are at 0aaa6e3 on 4bb2042. The runs use Docker golang:1.27 linux/arm64 with --cpus 4, seccomp=unconfined and memlock 8 MiB, so io_uring runs one worker, as in CI. The kernel is 7.0.12-linuxkit, and the command is go test -race -v -count=1 with CELERIS_REQUIRE_IOURING_WORKERS=1 and CELERIS_REQUIRE_DUALSTACK=1. Every container goes through the laptop lock, and tallies come from --- PASS/FAIL/SKIP: lines only.

TestConnParamsAndPeerAddress runs std, and epoll, io_uring and Adaptive with sync and async handlers, first on a loopback listener and then on a dual-stack "[::]:0" listener reached over IPv4: 14 arms. The route is GET /ws/:room/*rest, with 20 upgrades per arm. The handler reports Param("room"), Param("rest"), IP(), RemoteAddr()'s type, network and string, and the *net.TCPAddr's IP and Port fields. It reports them before the client sends a frame and again after a frame longer than the whole upgrade request, which the engine receives into the same buffer. On a host without dual-stack IPv6 the dual-stack arms skip, unless CELERIS_REQUIRE_DUALSTACK=1 (CI sets it); this is the adaptive package's pattern. TestConnIPConcurrentCalls: two goroutines call IP() on a hijack-path Conn under -race. TestPeerAddrParsesEngineForms (unit, round 1): peerAddr on IPv4, IPv6, the dual-stack form, a v4-mapped IPv6 address, a zone, and two forms that must stay raw.

control tree result log
failing-first (round 1) main's code (4bb2042) plus this head's test files, without peer_addr_721_test.go, which calls peerAddr (fix-r1/scripts/ff-c.sh) both #721 tests fail. TestConnParamsAndPeerAddress: all 14 arms fail. Param and the catch-all are wrong in 20 of 20 upgrades on every arm, std included, before and after a frame; IP, RemoteAddr and the TCPAddr fields are wrong in 20 of 20 on the 12 native arms and right on std. TestConnIPConcurrentCalls: -race reports the data race. The six #706 helper tests pass (they test the helpers, which are test code) fix-r1/logs/c-ff2.log
failing-first (round 1 tests on the round-0 fix) test commit 1a78590: the round-1 tests on the earlier fix the 6 native dual-stack arms fail: RemoteAddr is not a *net.TCPAddr in 20 of 20 upgrades (a websocket.rawAddr printing "[127.0.0.1]:port"). The loopback arms, the std arms and every Param/IP check pass. TestPeerAddrParsesEngineForms fails on "[127.0.0.1]:35224" fix-r1/logs/c-t2.log
head 0aaa6e3 9 of 9 tests pass, 14 of 14 arms with 0 of 20 wrong everywhere, no race report, 0 SKIP fix-r1/logs/c-head2.log
negative control (round 1) head with websocket.go cp'd back to the round-0 fix (1a78590), sha256 proved, then restored by cp and proved reverted: the round-1 failures again (6 native dual-stack arms, the unit test); restored: 9 of 9, 14 of 14 fix-r1/logs/c-neg2-{reverted,restored}.{cp.txt,log}
mutants the 7 from before, now judged on 14 arms: params not captured; peer address not kept; params captured before Detach without a copy; IP() caching lazily; the same with no eager compute (the pre-fix shape, two files); the root waitServerStarted and the sse waitUpOrStopped without their select on Start's result. Plus 3 for round 1: peerAddr always raw (the reviewer's surviving mutant C-peeraddr-always-raw), and the dual-stack fallback removed, judged by the engine test and by the unit test. Each is injected and restored by cp with sha256 proofs and compiled first (fix-r1/logs/c-mut2-compile.txt) 9 of 9 killed where they are faults: params not captured (all 14 arms fail, std included); peer address not kept, params as views, and peerAddr always raw (12 native arms fail, std passes); the dual-stack fallback removed (the 6 native dual-stack arms fail, std passes; and the unit test fails); the pre-fix IP() shape (-race); both #706 helper mutants (their tests fail). As in round 0, a lazy write in IP() alone survives: with the IP computed in setupConn, that branch is unreachable, so it is not a fault fix-r1/logs/c-mutants2.log

The round-0 runs, on the branch as built on 2ca5d72 and rebased onto c88d843, stay in logs/ (c-ff2, c-head2, c-neg2-*, c-mutants2, c-mutant-ip2). An earlier round-1 run used a test helper that dialed the handed listener before the engine had bound the port again. It failed every native arm at the head itself, so it is void and kept apart in fix-r1/logs/void-c1/.

Tests and controls (#706): a Start that fails, at every site

For each site, the site's own test is run with Start made to fail: an engine type no factory knows, and at the six "read once at 500 ms" sites a 700 ms delay before that failing Start. The same injection (asserted to match once in each tree, applied and restored by cp with sha256 proofs) runs on main and on this head (scripts/inject-706.py, scripts/inject-inner.sh).

On main, 16 of the 17 sites never show Start's error (s02 does, but only after 120 s). Two tests pass against a server that never started (s09 TestStartAfterShutdown, s18 TestWSRSTMidUpgradeTorture), one hangs until go test's timeout (s03, 180 s), and the rest fail later with a symptom such as connection reset by peer or handler did not start in time. On this head every site fails at once (0.3-4.6 s) with Start's own error. The head runs used d0405d6, whose #706 files differ from this head's only in comment lines (logs/c706-head-vs-final.diff, with a positive control); round 1 changes none of them. Logs: logs/c706-base.log, logs/c706-head.log; table: scripts/c706-table.py.

site main (2ca5d72) this head
s01-liveEngineServer FAIL, 5.9 s, Start's error shown: no; server not ready within 5s FAIL, 0.6 s, Start's error shown: yes; server stopped before it was ready: Start returned create engine: unknown engine type: unknown
s02-startSSEServer FAIL, 120.6 s, Start's error shown: yes; server start: create engine: unknown engine type: unknown FAIL, 0.5 s, Start's error shown: yes; server start: create engine: unknown engine type: unknown
s03-closeprobe hung: go test -timeout (180 s) panic, 181.9 s, Start's error shown: no; server not ready within 30s FAIL, 1.1 s, Start's error shown: yes; server stopped before it was ready: Start returned create engine: unknown engine type: unknown
s04-asyncPromote FAIL, 6.5 s, Start's error shown: no; server did not become ready FAIL, 1.5 s, Start's error shown: yes; server stopped before it was ready: Start returned create engine: unknown engine type: unknown
s06-liveServer FAIL, 0.4 s, Start's error shown: no; request: Post "http://127.0.0.1:38275/echo": dial tcp 127.0.0.1:38275: connect: connection refused FAIL, 0.4 s, Start's error shown: yes; server stopped before it was ready: Start returned create engine: unknown engine type: unknown
s07-startTestServer FAIL, 0.4 s, Start's error shown: no; request: Post "http://127.0.0.1:44269/v1/users": read tcp 127.0.0.1:59364->127.0.0.1:44269: read: connection r FAIL, 0.5 s, Start's error shown: yes; server stopped before it was ready: Start returned create engine: unknown engine type: unknown
s08-startMixedAsyncServer FAIL, 6.4 s, Start's error shown: no; GET http://127.0.0.1:36487/cpu: Get "http://127.0.0.1:36487/cpu": dial tcp 127.0.0.1:36487: connect: connectio FAIL, 1.5 s, Start's error shown: yes; server stopped before it was ready: Start returned create engine: unknown engine type: unknown
s09-StartAfterShutdown PASS (the failed start went unnoticed), 1.7 s, Start's error shown: no; `` FAIL, 1.5 s, Start's error shown: yes; server stopped before it was ready: Start returned create engine: unknown engine type: unknown
s10-DrainsInFlight FAIL, 2.7 s, Start's error shown: no; handler did not start in time FAIL, 0.7 s, Start's error shown: yes; server stopped before it was ready: Start returned create engine: unknown engine type: unknown
s11-reopenerDiscriminator FAIL, 11.4 s, Start's error shown: no; discriminator: a server that started must have the re-opener running (the leak assertions above would pass vac FAIL, 1.6 s, Start's error shown: yes; discriminator: the server's Start returned create engine: unknown engine type: unknown before the re-opener st
s12-stdSSEShutdown FAIL, 0.7 s, Start's error shown: no; write request: write tcp 127.0.0.1:42230->127.0.0.1:38675: write: connection reset by peer FAIL, 0.3 s, Start's error shown: yes; server stopped before it was ready: Start returned create engine: unknown engine type: unknown
s13-epollDetachInline FAIL, 3.5 s, Start's error shown: no; read 101 line: read tcp 127.0.0.1:41358->127.0.0.1:40809: read: connection reset by peer FAIL, 2.6 s, Start's error shown: yes; server start: create engine: unknown engine type: unknown
s14-epollRDHUP FAIL, 3.9 s, Start's error shown: no; SSE handler never ran — the request itself was not delivered FAIL, 1.1 s, Start's error shown: yes; server start: create engine: unknown engine type: unknown
s15-sseNative FAIL, 4.6 s, Start's error shown: no; read after 0 events: read tcp 127.0.0.1:60346->127.0.0.1:44503: read: connection reset by peer FAIL, 4.6 s, Start's error shown: yes; server start: create engine: unknown engine type: unknown
s16-wsNative FAIL, 5.1 s, Start's error shown: no; read first line: read tcp 127.0.0.1:49414->127.0.0.1:46581: read: connection reset by peer FAIL, 4.6 s, Start's error shown: yes; server start: create engine: unknown engine type: unknown
s17-detachedCount FAIL, 1.1 s, Start's error shown: no; anchor 0: upgrade: status "" err read tcp 127.0.0.1:48000->127.0.0.1:43929: read: connection reset by peer FAIL, 1.1 s, Start's error shown: yes; server start: create engine: unknown engine type: unknown
s18-wsRSTTorture PASS (the failed start went unnoticed), 14.4 s, Start's error shown: no; TestWSRSTMidUpgradeTorture/epoll: completed 4097 RST-mid-upgrade cycles across 32 workers; no panic FAIL, 1.8 s, Start's error shown: yes; server start: create engine: unknown engine type: unknown

sites: main 17, head 17; every injected file restored and sha256-proved: True

Also in test/drivercmp/redis/integrated_async_test.go: the file gets the //go:build linux that its companion integrated_test.go has (it uses that file's celerisRedisEnv), so the module vets off Linux again. This is the #890 item from #894's review, in a file this PR changes anyway (go vet on darwin: main rc=1 undefined: celerisRedisEnv, head rc=0; logs/drivercmp-darwin-vet.txt).

test/drivercmp/redis/integrated_async_test.go is not in the table: its benchmark needs a Redis server (CELERIS_REDIS_ADDR), so the change there is checked by go vet only.

Whole-package suites (-race -v at 0aaa6e3, the CI shape above): websocket 144, middleware 7, sse 88, engine/std 17, root 423, engine/epoll 142 top-level PASS, 0 FAIL, no data race; the 14 arms of the #721 test ran there too, with no SKIP (fix-r1/logs/c-suites2.log, tallied by fix-r1/scripts/suites-tally.py into fix-r1/logs/suites-tally.txt). SKIPs, all pre-existing and env-gated: websocket TestMeasureWedgeRate (WS667_WEDGE=1) and TestHubBroadcastFormatsOnce; sse's 3 alloc-count tests (unstable under -race); root TestRouteAdaptive_SettleReopenCost (CELERIS_592_COST=1), the 3 TestAdaptiveSettledRouteRetime592/iouring/* subtests (2 io_uring workers at 8 MiB memlock) and 2 .../io_uring/async-route subtests (celeris#751); epoll TestWriteBufBackpressureClosesSlowConsumer (GOTEST_BACKPRESSURE=1) and the two tcp_synack_retries=0 tests.

golangci-lint v2.13.2 (GOOS=linux) at this head on the packages it touches that CI lints (root, middleware, middleware/websocket, middleware/sse, engine/std, engine/epoll): 0 issues (fix-r1/logs/lint2.log). actionlint v1.7.12 with shellcheck: 0 (fix-r1/logs/actionlint-r1.txt). The test/drivercmp/redis module and the celeris_closeprobe build are not in CI's lint job; their pre-existing findings are filed on #890. GOOS=linux amd64 and arm64 build + vet of every touched package, including test/drivercmp/redis, at c1f8e05: 0 failures (fix-r1/logs/xcompile-C3.txt).

CI. A new Unit step runs the #721 tests, the peer-address unit test and the six #706 helper tests by name (the root race step excludes ./middleware/websocket). It sets CELERIS_REQUIRE_IOURING_WORKERS=1 and CELERIS_REQUIRE_DUALSTACK=1 and keeps an exact tally: 9 top-level PASS, the 14 arms of the #721 test PASS (4 io_uring, 4 Adaptive), no SKIP line, and every go test exits 0. The step's own script was run on the head log (c-head3, passes) and on the failing-first and test-commit logs (fail, exit 1) (fix-r1/logs/ci-tally-proof-c.txt). CI at c1f8e05 is green (43 pass), and the step printed top-level PASS 9 (want 9), #721 arm PASS 14 (want 14), io_uring 4 (want 4), adaptive 4 (want 4), SKIP lines 0, go test runs failed 0 (run 37113758780; fix-r1/scripts/ci-unit-log.sh, fix-r1/logs/ci-918r1-unit.log).

Review round 2

Both lenses approved. The one NIT is fixed in 4804782, a comment only: captureParams names the router rule its parsing of the param names mirrors. The branch was rebased onto 56a6c1e (#906 and #910 merged since 609a015; neither touches a file of this PR) with no conflict, and git range-diff shows every earlier patch identical (fix-r2/logs/rebase-56a6c1e-C.txt). Every control ran again at the pushed head 4804782, in the CI shape above (fix-r2/scripts/chain-c1.sh):

control result log
failing-first: main 56a6c1e plus this head's test files, without peer_addr_721_test.go (fix-r2/scripts/ff-c.sh) TestConnParamsAndPeerAddress fails on all 14 arms (12 native; the 2 std arms too, because Param is wrong on every engine), and TestConnIPConcurrentCalls reports the data race. The six #706 helper tests pass fix-r2/logs/c-ff1.log
head 9 of 9 tests, 14 of 14 arms, 0 SKIP, no race report fix-r2/logs/c-head1.log
negative control: websocket.go and conn.go cp'd back to main (sha256 proved), peer_addr_721_test.go set aside, then restored by cp and proved reverted: the failing-first result again (14 arms, the race); restored: 9 of 9, 14 of 14 fix-r2/logs/c-neg1-{reverted,restored}.{cp.txt,log}
mutants: the 10 from round 1 9 of 9 faults killed, each with the expected arms; the lazy IP() write, unreachable, survives as before fix-r2/logs/c-mutants1.log
#706: Start made to fail at the 17 sites, at this head (fix-r2/scripts/inject-snap.sh) every site fails at once (1.1-7.2 s) with Start's own error (unknown engine type in the output), none reaches go test's timeout, every file restored and proved. The main column of the table above still describes main: the 17 files are the same blobs at 56a6c1e as at 2ca5d72 (fix-r2/logs/c706-main-files-2ca5d72-vs-56a6c1e.txt) fix-r2/logs/c706-head1.log
#706 re-audit at 56a6c1e 30 test files added or changed since 2ca5d72; five start a server in a goroutine, and all five select on Start's result: the four of round 1, and large_response_linux_test.go, whose startServer761 became startServerConfig761 in #906 and serves #906's two new engine tests (they call it and start no server of their own) fix-r2/logs/c706-reaudit-56a6c1e.txt

Whole-package suites at 4804782 (fix-r2/logs/c-suites1.log; fix-r2/scripts/suites-tally.py, fix-r2/logs/suites-tally.txt): middleware 7, sse 88, engine/std 17, root 427, engine/epoll 142 top-level PASS, 0 FAIL, no data race; websocket 143 PASS and 1 FAIL. The failure is TestBackpressureInboundSequenceIntegrity/io_uring/multishot_recv, which stopped on environment: 10 dial/handshake failures (its client's 3 s dial and 5 s handshake deadlines, on a host under load). Main fails the same way: run alone 3 times on main 56a6c1e and 3 times on this head, alternating, in the CI shape, it failed 2 of 3 times on main and 1 of 3 on the head, each time with environment: N dial/handshake failures and nothing else (fix-r2/scripts/flake-ws.sh, fix-r2/logs/c-flake-{head,main}{1,2,3}.log). The test counts those errors and discards them; that is now an item on #890 (comment). The whole websocket suite, run again at this head, passed: 144 top-level PASS, 0 FAIL, 2 SKIP (the same two env-gated tests), no data race (fix-r2/logs/c-suites2.log). SKIPs, all pre-existing and env-gated, are the ones listed for round 1 above. GOOS=linux amd64 and arm64 build + vet of every touched package, including test/drivercmp/redis: 0 failures (fix-r2/logs/xcompile-C1.txt). golangci-lint v2.13 on the packages CI lints: 0 issues (fix-r2/logs/lint1.log). actionlint v1.7.12 with shellcheck: 0 (fix-r2/logs/actionlint-r2.txt). CI at 4804782 is green (44 pass), and the step printed top-level PASS 9 (want 9), #721 arm PASS 14 (want 14), io_uring 4 (want 4), adaptive 4 (want 4), SKIP lines 0, go test runs failed 0 (run 37121308818; fix-r2/scripts/ci-unit-log.sh, fix-r2/logs/ci-918r2-unit.log). The step's own script passes on the head log and fails on the failing-first and negative-control logs (fix-r2/scripts/ci-tally-proof-c.sh, fix-r2/logs/ci-tally-proof-c.txt).

Overlap

git merge-tree of this head with every open PR that shares a file is clean (fix-r2/scripts/overlap.sh, fix-r2/logs/overlap-r2.txt): only ci.yml is shared, with #914, #917 and #920. The three lane MW PRs merge with each other and with main cleanly; each adds its CI step at a different place. On the merged tree (main 56a6c1e + #920's, #917's and this head), the three PRs' test sets pass: #920's set 17 of 17 tests and 105 of 105 arms, #917's 5 of 5 and 28 of 28, this PR's 9 of 9 and 14 of 14, no SKIP, no data race (fix-r2/logs/m-{a2,b2,c2}.log).

Evidence root: evidence/lanes-20261003/MW/. The first build's scripts and logs are in scripts/ and logs/; round 1's are in fix-r1/scripts/ and fix-r1/logs/; round 2's in fix-r2/scripts/ and fix-r2/logs/.

@FumingPower3925 FumingPower3925 added this to the v1.6.0 milestone Oct 3, 2026
@FumingPower3925 FumingPower3925 added bug Something isn't working testing Testing infrastructure and helpers middleware Middleware implementation labels Oct 3, 2026
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: goceleris/celeris/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b013e092-af65-4e5c-81c2-607feac77c68
📥 Commits

Reviewing files that changed from the base of the PR and between 4804782 and 1ec4802.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The WebSocket middleware captures route parameters and engine peer addresses for connection accessors. Server test readiness checks detect early startup completion and report its result. Regression tests cover both changes.

Changes

WebSocket connection metadata

Layer / File(s) Summary
Capture route parameters and peer addresses
middleware/websocket/conn.go, middleware/websocket/websocket.go
The middleware stores route parameters and engine peer addresses on the connection. Conn.RemoteAddr and Conn.IP use the stored address when no net.Conn exists.
Test WebSocket metadata behavior
middleware/websocket/conn_ip_race_721_test.go, middleware/websocket/conn_params_addr_721_linux_test.go, middleware/websocket/peer_addr_721_test.go, middleware/websocket/*_linux_test.go, .github/workflows/ci.yml
Tests cover concurrent Conn.IP calls, peer-address parsing, and route parameters and peer addresses across engines. The CI step checks the selected test pass and skip counts.

Server startup readiness

Layer / File(s) Summary
Add startup wait helpers and failure tests
server_start_wait_706*_test.go, middleware/start_wait_706_test.go, engine/epoll/start_wait_706_linux_test.go, middleware/sse/start_wait_706_linux_test.go, middleware/websocket/start_wait_706_ext_linux_test.go
The helpers observe server readiness and early Start completion. Tests verify that startup errors are reported and remain available to shutdown logic.
Update readiness checks
adaptive_settle_reopen_test.go, async_promote_integration_linux_test.go, engine/epoll/*_test.go, engine/std/sse_shutdown_test.go, middleware/integration_chain*_test.go, middleware/pre_routing_order_test.go, middleware/sse/*_linux_test.go, middleware/websocket/*_linux_test.go, server_async_test.go, server_shutdown_test.go, test/drivercmp/redis/integrated_async_test.go
Server tests check startup results while waiting for readiness. The Redis async test is restricted to Linux, and getBody uses a 10-second HTTP timeout.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 1ec48

The supplied evidence shows no remaining actionable merge risk; the route-parameter lifetime concern is resolved by the existing context-detach behavior.

Architecture Summary

Architecture risk: 🔵 Low · up to 1ec48

The change affects 9 systems.

Changed systems: middleware, engine, adaptive_settle_reopen_test.go, async_promote_integration_linux_test.go, server_async_test.go, server_shutdown_test.go, server_start_wait_706_ext_test.go, server_start_wait_706_test.go, test

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — middleware (service) was modified; 17 changed files map to changed impact.
  • observed — engine (service) was modified; 4 changed files map to changed impact.
  • observed — adaptive_settle_reopen_test.go (service) was modified; 1 changed file maps to changed impact.
  • observed — async_promote_integration_linux_test.go (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in adaptive_settle_reopen_test.go: While waiting for the re-opener, the test now checks whether the server’s Start result is available and fails with that result if startup returns first.
  • observed — Modified behavior in async_promote_integration_linux_test.go: The test now sends StartWithListener’s result to a channel and passes that channel to waitServerStarted, replacing readiness polling via /ping and ignoring the start result. It constructs the base URL from the returned address and gives the HTTP client a 10-second timeout.
  • observed — Modified behavior in engine/epoll/detach_inline_regression_linux_test.go: After the settle delay, the test now calls waitUpOrStopped with a 30-second timeout before checking the recorded startup error.
  • observed — Modified behavior in engine/epoll/rdhup_detached_linux_test.go: After the settle sleep, startRDHUPServer now waits for the server to become available or stop, using a 30-second timeout; previously it proceeded directly to checking startErr.
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning test/drivercmp/redis/integrated_async_test.go adds //go:build linux to fix the separate Darwin vet failure (undefined: celerisRedisEnv). The readiness-loop change in that file supports #706, but… Remove the Linux build constraint or move the Darwin vet fix to a separate PR.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title uses fix(websocket): summary form, describes the WebSocket metadata and server-start helper changes, and ends with references to both issues.
Description check ✅ Passed The description explains the WebSocket fixes, server-start helper changes, tests, and reported results.
Linked Issues check ✅ Passed #721: middleware/websocket captures named and catch-all route params and the peer address at upgrade. It computes IP() during setup and parses dual-stack IPv4 addresses as *net.TCPAddr. The regr…
Full details: Out of Scope Changes check

Explanation

test/drivercmp/redis/integrated_async_test.go adds //go:build linux to fix the separate Darwin vet failure (undefined: celerisRedisEnv). The readiness-loop change in that file supports #706, but the build constraint does not implement either linked issue.

  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…from the upgrade request on every engine (celeris#721)
…ade, so Conn.Param, IP and RemoteAddr answer on every engine (celeris#721)
…with Start's error, and probes the server's own address (celeris#706)
…'s IP and port, on a dual-stack listener too (celeris#721)

The test compared RemoteAddr's Network() and String() only, which any
net.Addr printing "tcp ip:port" passes, while the doc promises a
*net.TCPAddr and callers type-assert it. It now checks the type and the IP
and Port fields, and runs every arm again on a "[::]:port" listener, where
the native engines report an IPv4 peer as "[a.b.c.d]:port". A host
without dual-stack IPv6 skips those arms unless CELERIS_REQUIRE_DUALSTACK=1,
the adaptive package's pattern. A unit test pins the forms peerAddr parses.
…listener as its *net.TCPAddr (celeris#721)

The engines report that peer as "[a.b.c.d]:port", which
netip.ParseAddrPort rejects, so RemoteAddr returned the raw string, not
the *net.TCPAddr its doc promises. When the whole address does not parse,
peerAddr now parses the host and the port on their own.
@FumingPower3925
FumingPower3925 force-pushed the fix/celeris-721-706-ws-conn-params-start-errors branch from c1f8e05 to 4804782 Compare October 3, 2026 11:57
@FumingPower3925
FumingPower3925 marked this pull request as ready for review October 3, 2026 12:22
@FumingPower3925
FumingPower3925 merged commit b780e73 into main Oct 3, 2026
43 checks passed
@FumingPower3925
FumingPower3925 deleted the fix/celeris-721-706-ws-conn-params-start-errors branch October 3, 2026 13:39
FumingPower3925 added a commit that referenced this pull request Oct 5, 2026
…nst its measured runs, not an absolute 2 ms bar (#953)

TestAdaptiveImmediatePromote_Epoll again exercises its fast route (since #918 it never sent /ping, so the check could neither flake nor fail). It runs 64 /ping requests timed inside the handler. A promotion is explained when a run measured over 2 ms (or 8 runs over 300 us) precedes it, and the route is then re-armed; more than 2 unexplained promotions fail. The bars are written out in the test so a change to handler.go cannot silently move them. Controls (blocking bar 0, streak bar 0, every route promoted) each fail 10 of 10.

Fixes #752
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working middleware Middleware implementation testing Testing infrastructure and helpers

Projects

None yet

1 participant