Repository navigation
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
Conversation
|
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
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe 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. ChangesWebSocket connection metadata
Server startup readiness
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The supplied evidence shows no remaining actionable merge risk; the route-parameter lifetime concern is resolved by the existing context-detach behavior. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 9 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
28e9c9f to
c1f8e05
Compare
…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)
…er tests by name, io_uring arms required
…he integrated_test.go it builds on
…'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.
… and counts the dual-stack arms
…arsing mirrors (celeris#721)
c1f8e05 to
4804782
Compare
…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
Fixes #721. Fixes #706.
What was wrong
#721,
middleware/websocket. Lines are at the base609a015.middleware/websocket/is byte-identical there to2ca5d72, where the lane first built this PR.Conn.Paramreadc.params(conn.go:950), which nothing ever set:setupConn(websocket.go:318-326) set the headers and the query only. SoParamreturned""on every engine, ever since the middleware landed (v1.3.4).Conn.IPandConn.RemoteAddraskedc.conn(conn.go:880-925), the hijackednet.Conn. The engine path (epoll, io_uring, and Adaptive, which runs them) has none, soIP()returned""andRemoteAddr()returnednil, although the Context knows the peer (Context.RemoteAddr).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:-racereports it on the std path today, and this PR would have extended it to the engine path."[::]:port"; no engine setsIPV6_V6ONLY), the engines report an IPv4 peer as"[127.0.0.1]:port".netip.ParseAddrPortrejects that form, soRemoteAddr()returned a raw string address, not the*net.TCPAddrits doc promises. The test compared onlyNetwork()andString(), 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'sContext.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 at2776d4c. The lane re-checked them at2ca5d72(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.gothrough #730'swaitForReady). 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 nilAddr(). 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 since2ca5d72, 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.goandmiddleware/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:captureParamsreads the matched route pattern (c.FullPath(), whose names the router owns) and records one pair per named parameter (:room) and catch-all (*path), withc.Param's value. On the engine path it runs afterDetach, 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,splitPathandfindSegmentEnd), and its comment now says so (round 2), becauseContexthas no accessor for its params' names.*net.TCPAddr, soRemoteAddr()returns the same type as the hijack path'snet.Conn.peerAddrparses the address withnetip.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 stringnet.Addr.IP()is computed once, insetupConn, and only read afterwards.LocalAddr()staysnilon 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.peerAddr+IP()'s value, IPv4 peer ("127.0.0.1:54321")"[127.0.0.1]:54321")"[2001:db8::1]:443")IP()'s value from thenet.Conn's*net.TCPAddr, now computed at upgrade. The old lazy version paid the same on the firstIP()call and nothing ifIP()was never called/ws/:room/*rest); on the hijack path the slice plus one clone per paramThe first version of this body said "one
[][2]stringand one*net.TCPAddr". That was not measured, and it was low.#706. Each site now reads Start's error while it waits:
waitServerStarted(root, root external tests,middleware) waits fors.Addr()to answer and fails at once with Start's error, putting the error back indonefor the caller's shutdown.waitUpOrStopped(middleware/sse,middleware/websocketexternal 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.sse_disconnect_linux_test.gochecksdoneon every poll instead of only after its 30 s deadline. Theceleris_closeprobedrain test andengine/std/sse_shutdown_test.goselect on theirserverDone.test/drivercmp/redis/integrated_async_test.gostops with Start's error instead of a nilAddr()dereference.server_async_test.go's GETs get a timeout.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) andengine/std(one wait loop) are small, for the #443 move.Tests and controls (#721)
Round 1 was rebased onto
4bb2042, and then onto609a015when #923 merged (it changes onlymiddleware/cache). Both rebases had no conflict;git range-diffshows every patch identical, and every file this PR touches is the same at0aaa6e3and at the pushed headc1f8e05(fix-r1/logs/rebase-609a015-C.txt). The head set ran again atc1f8e05: 9 of 9 tests, 14 of 14 arms, 0 SKIP (fix-r1/logs/c-head3.log). The other runs are at0aaa6e3on4bb2042. The runs use Dockergolang:1.27linux/arm64 with--cpus 4,seccomp=unconfinedand memlock 8 MiB, so io_uring runs one worker, as in CI. The kernel is 7.0.12-linuxkit, and the command isgo test -race -v -count=1withCELERIS_REQUIRE_IOURING_WORKERS=1andCELERIS_REQUIRE_DUALSTACK=1. Every container goes through the laptop lock, and tallies come from--- PASS/FAIL/SKIP:lines only.TestConnParamsAndPeerAddressruns 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 isGET /ws/:room/*rest, with 20 upgrades per arm. The handler reportsParam("room"),Param("rest"),IP(),RemoteAddr()'s type, network and string, and the*net.TCPAddr'sIPandPortfields. 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, unlessCELERIS_REQUIRE_DUALSTACK=1(CI sets it); this is the adaptive package's pattern.TestConnIPConcurrentCalls: two goroutines callIP()on a hijack-path Conn under-race.TestPeerAddrParsesEngineForms(unit, round 1):peerAddron IPv4, IPv6, the dual-stack form, a v4-mapped IPv6 address, a zone, and two forms that must stay raw.4bb2042) plus this head's test files, withoutpeer_addr_721_test.go, which callspeerAddr(fix-r1/scripts/ff-c.sh)TestConnParamsAndPeerAddress: all 14 arms fail.Paramand the catch-all are wrong in 20 of 20 upgrades on every arm, std included, before and after a frame;IP,RemoteAddrand theTCPAddrfields are wrong in 20 of 20 on the 12 native arms and right on std.TestConnIPConcurrentCalls:-racereports the data race. The six #706 helper tests pass (they test the helpers, which are test code)fix-r1/logs/c-ff2.log1a78590: the round-1 tests on the earlier fixRemoteAddris not a*net.TCPAddrin 20 of 20 upgrades (awebsocket.rawAddrprinting"[127.0.0.1]:port"). The loopback arms, the std arms and everyParam/IPcheck pass.TestPeerAddrParsesEngineFormsfails on"[127.0.0.1]:35224"fix-r1/logs/c-t2.log0aaa6e3fix-r1/logs/c-head2.logwebsocket.gocp'd back to the round-0 fix (1a78590), sha256 proved, then restored bycpand provedfix-r1/logs/c-neg2-{reverted,restored}.{cp.txt,log}Detachwithout a copy;IP()caching lazily; the same with no eager compute (the pre-fix shape, two files); the rootwaitServerStartedand the ssewaitUpOrStoppedwithout their select on Start's result. Plus 3 for round 1:peerAddralways raw (the reviewer's surviving mutantC-peeraddr-always-raw), and the dual-stack fallback removed, judged by the engine test and by the unit test. Each is injected and restored bycpwith sha256 proofs and compiled first (fix-r1/logs/c-mut2-compile.txt)peerAddralways 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-fixIP()shape (-race); both #706 helper mutants (their tests fail). As in round 0, a lazy write inIP()alone survives: with the IP computed insetupConn, that branch is unreachable, so it is not a faultfix-r1/logs/c-mutants2.logThe round-0 runs, on the branch as built on
2ca5d72and rebased ontoc88d843, stay inlogs/(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 infix-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
cpwith 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, s18TestWSRSTMidUpgradeTorture), one hangs untilgo test's timeout (s03, 180 s), and the rest fail later with a symptom such asconnection reset by peerorhandler 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 usedd0405d6, 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.2ca5d72)server not ready within 5sserver stopped before it was ready: Start returned create engine: unknown engine type: unknownserver start: create engine: unknown engine type: unknownserver start: create engine: unknown engine type: unknownserver not ready within 30sserver stopped before it was ready: Start returned create engine: unknown engine type: unknownserver did not become readyserver stopped before it was ready: Start returned create engine: unknown engine type: unknownrequest: Post "http://127.0.0.1:38275/echo": dial tcp 127.0.0.1:38275: connect: connection refusedserver stopped before it was ready: Start returned create engine: unknown engine type: unknownrequest: Post "http://127.0.0.1:44269/v1/users": read tcp 127.0.0.1:59364->127.0.0.1:44269: read: connection rserver stopped before it was ready: Start returned create engine: unknown engine type: unknownGET http://127.0.0.1:36487/cpu: Get "http://127.0.0.1:36487/cpu": dial tcp 127.0.0.1:36487: connect: connectioserver stopped before it was ready: Start returned create engine: unknown engine type: unknownserver stopped before it was ready: Start returned create engine: unknown engine type: unknownhandler did not start in timeserver stopped before it was ready: Start returned create engine: unknown engine type: unknowndiscriminator: a server that started must have the re-opener running (the leak assertions above would pass vacdiscriminator: the server's Start returned create engine: unknown engine type: unknown before the re-opener stwrite request: write tcp 127.0.0.1:42230->127.0.0.1:38675: write: connection reset by peerserver stopped before it was ready: Start returned create engine: unknown engine type: unknownread 101 line: read tcp 127.0.0.1:41358->127.0.0.1:40809: read: connection reset by peerserver start: create engine: unknown engine type: unknownSSE handler never ran — the request itself was not deliveredserver start: create engine: unknown engine type: unknownread after 0 events: read tcp 127.0.0.1:60346->127.0.0.1:44503: read: connection reset by peerserver start: create engine: unknown engine type: unknownread first line: read tcp 127.0.0.1:49414->127.0.0.1:46581: read: connection reset by peerserver start: create engine: unknown engine type: unknownanchor 0: upgrade: status "" err read tcp 127.0.0.1:48000->127.0.0.1:43929: read: connection reset by peerserver start: create engine: unknown engine type: unknownTestWSRSTMidUpgradeTorture/epoll: completed 4097 RST-mid-upgrade cycles across 32 workers; no panicserver start: create engine: unknown engine type: unknownsites: 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 linuxthat its companionintegrated_test.gohas (it uses that file'scelerisRedisEnv), so the module vets off Linux again. This is the #890 item from #894's review, in a file this PR changes anyway (go veton darwin: main rc=1undefined: celerisRedisEnv, head rc=0;logs/drivercmp-darwin-vet.txt).test/drivercmp/redis/integrated_async_test.gois not in the table: its benchmark needs a Redis server (CELERIS_REDIS_ADDR), so the change there is checked bygo vetonly.Whole-package suites (
-race -vat0aaa6e3, 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 byfix-r1/scripts/suites-tally.pyintofix-r1/logs/suites-tally.txt). SKIPs, all pre-existing and env-gated: websocketTestMeasureWedgeRate(WS667_WEDGE=1) andTestHubBroadcastFormatsOnce; sse's 3 alloc-count tests (unstable under-race); rootTestRouteAdaptive_SettleReopenCost(CELERIS_592_COST=1), the 3TestAdaptiveSettledRouteRetime592/iouring/*subtests (2 io_uring workers at 8 MiB memlock) and 2.../io_uring/async-routesubtests (celeris#751); epollTestWriteBufBackpressureClosesSlowConsumer(GOTEST_BACKPRESSURE=1) and the twotcp_synack_retries=0tests.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). Thetest/drivercmp/redismodule and theceleris_closeprobebuild are not in CI's lint job; their pre-existing findings are filed on #890.GOOS=linuxamd64 and arm64 build + vet of every touched package, includingtest/drivercmp/redis, atc1f8e05: 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 setsCELERIS_REQUIRE_IOURING_WORKERS=1andCELERIS_REQUIRE_DUALSTACK=1and 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 everygo testexits 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 atc1f8e05is green (43 pass), and the step printedtop-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:captureParamsnames the router rule its parsing of the param names mirrors. The branch was rebased onto56a6c1e(#906 and #910 merged since609a015; neither touches a file of this PR) with no conflict, andgit range-diffshows every earlier patch identical (fix-r2/logs/rebase-56a6c1e-C.txt). Every control ran again at the pushed head4804782, in the CI shape above (fix-r2/scripts/chain-c1.sh):56a6c1eplus this head's test files, withoutpeer_addr_721_test.go(fix-r2/scripts/ff-c.sh)TestConnParamsAndPeerAddressfails on all 14 arms (12 native; the 2 std arms too, becauseParamis wrong on every engine), andTestConnIPConcurrentCallsreports the data race. The six #706 helper tests passfix-r2/logs/c-ff1.logfix-r2/logs/c-head1.logwebsocket.goandconn.gocp'd back to main (sha256 proved),peer_addr_721_test.goset aside, then restored bycpand provedfix-r2/logs/c-neg1-{reverted,restored}.{cp.txt,log}IP()write, unreachable, survives as beforefix-r2/logs/c-mutants1.logfix-r2/scripts/inject-snap.sh)unknown engine typein the output), none reachesgo 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 at56a6c1eas at2ca5d72(fix-r2/logs/c706-main-files-2ca5d72-vs-56a6c1e.txt)fix-r2/logs/c706-head1.log56a6c1e2ca5d72; five start a server in a goroutine, and all five select on Start's result: the four of round 1, andlarge_response_linux_test.go, whosestartServer761becamestartServerConfig761in #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.txtWhole-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 isTestBackpressureInboundSequenceIntegrity/io_uring/multishot_recv, which stopped onenvironment: 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 main56a6c1eand 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 withenvironment: N dial/handshake failuresand 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=linuxamd64 and arm64 build + vet of every touched package, includingtest/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 at4804782is green (44 pass), and the step printedtop-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-treeof this head with every open PR that shares a file is clean (fix-r2/scripts/overlap.sh,fix-r2/logs/overlap-r2.txt): onlyci.ymlis shared, with #914, #917 and #920. The three lane MW PRs merge with each other and withmaincleanly; each adds its CI step at a different place. On the merged tree (main56a6c1e+ #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 inscripts/andlogs/; round 1's are infix-r1/scripts/andfix-r1/logs/; round 2's infix-r2/scripts/andfix-r2/logs/.