fix(router): re-time settled adaptive routes so a store that turns slow no longer pins the worker forever - #603
Merged
Conversation
…ow cannot pin a worker forever (celeris#592)
Mechanism: settling (celeris#361) was TERMINAL. handler.go times an adaptive
route only while router.adaptiveLearning() is true, adaptiveLearning
short-circuits on the `settled` set, and the only statement that ever removed a
route from `settled` was an explicit .Async()/.Sync() at registration
(router.go setAsync). A route that settled while its backend was fast — 256
consecutive sub-300µs inline runs — and whose backend LATER turned slow
therefore kept running inline on the engine worker for every request, for the
life of the process, queueing every other connection on that worker behind the
blocking call.
Fix: adaptiveSettleTTL (5 s, mirroring adaptivePromoteTTL). One per-server
background goroutine — started in doPrepare only when the server has adaptive
routes, stopped in Shutdown — clears the settled set every tick, so the route
is timed again and a run over adaptiveBlockingThreshold promotes it
immediately.
Cost: nothing is added to the request path. The settled fast path is
byte-for-byte what it was (one sync.Map load in adaptiveLearning); per-request
sampling by counter or by clock read, the two alternatives, would put back
exactly what celeris#361 removed for the same detection bound. The fast streak
is deliberately NOT reset by the re-open, so a route that is still fast
re-settles on its very next run: the amortized price is ONE timed inline run
(two time.Now() calls) per adaptive route per 5 s. Worst-case detection latency
is adaptiveSettleTTL plus one request.
Measured (docker golang:1.27, arm64, --cpus 4, seccomp=unconfined, memlock
128 MB, -race, 20 runs per engine, rig adaptive_settled_retime_linux_test.go
cherry-picked from measure/589-settled-route-stall with the `settled` subtest
INVERTED):
settled subtest, this branch
iouring 20/20 FIXED: settled at flip 20/20, promoted 20/20,
AsyncPromotedConns 8-9, promote latency after the store turned slow
min 5122 ms / med 5416 ms / max 5437 ms (bound 8 s)
epoll 20/20 FIXED: same state, promote latency min 4816 / med 5416 /
max 5443 ms; /ping stalled fraction after promotion
max 0.007 (11 of 19217 samples > 5 ms), bar 0.05
NEGATIVE CONTROL, the same inverted rig on origin/main (bef00fc), 1 run per
engine: both FAIL — never promoted within the 8 s bound,
AsyncPromotedConns=0, settled_after=true, stalled_frac 1.000
(iouring /ping median 893 ms, epoll 2096 ms).
io_uring's post-promotion stalled fraction is PRINTED, not asserted
(med 0.333 on the settled subtest vs 0.330 on the fully async control): that
worker pin is the separate celeris#593 sweep defect, not dispatch.
The two rig controls are unchanged and still invert: explicit .Async()
(AsyncPromotedConns=9, epoll 2 stalled of 19312) and still-learning
(promoted 20/20, epoll 2 stalled of 19309).
…s#592 settle re-opener (1) Start the re-opener AFTER the engine is created and published. Started before createEngine, a failed engine creation returned from doPrepare with startErr set and no engine ever stored: the caller gets an error instead of a Server, never calls Shutdown, and nothing would ever stop the ticker — it ran until the process exited. Nothing can settle before the engine serves a request, so the later start costs no coverage. TestRouteAdaptive_NoReopenerWhenEngineCreationFails forces the failure with an EngineType createEngine does not know (it passes Config.Validate and fails in createEngine itself) and asserts both reopenStop == nil and zero startSettleReopener goroutines in a full runtime.Stack scan; the second half is the discriminator — the same two assertions must FIND the re-opener on a server that starts, so a tree that simply never started it cannot pass. (2) CLAMP the fast streak at adaptiveSettleStreak instead of letting it Add without bound. Settling used to be terminal, so the counter stopped growing the moment it first reached the threshold. With the re-opener every tick returns the route to the timed path and adds at least one more increment for the life of the process; on int32 wrap the counter goes NEGATIVE, `Add(1) >= streak` stops holding and the route can never settle again — the fix would have permanently re-introduced the per-request timing celeris#361 removed. TestRouteAdaptive_FastStreakClampedAcrossReopens asserts the invariant that makes the wrap unreachable (0 <= fastStreak <= adaptiveSettleStreak) over 10,000 settle/re-open cycles and over 1,000 runs inside a single re-open window, and that a slow run still zeroes the streak. (3) Correct the adaptiveSettleTTL cost claim, which was wrong. It is not one timed run per route per TTL: clearing the settled set opens a gate that stays open until the FIRST re-timed run stores `settled` again, so every inline run that passes the gate in that window is timed. The bound is one timed run per CONCURRENTLY-EXECUTING inline handler per tick — an inline run holds its engine worker for the whole run, so that worker's next request cannot start until the route has re-settled. The "1 request in 5,000,000" figure is replaced with a measurement: TestRouteAdaptive_SettleReopenCost replicates handler.go's dispatch gate verbatim, counts timed runs at the gate over 200 re-opens, and measures steady-state throughput and the per-timed-run overhead separately so the amortized figure is not inferred from the test's own re-open cadence. Measured (golang:1.27 container, 4 CPUs): 1.00 / 1.99 / 3.39 / 3.02 timed runs per re-open at 1 / 2 / 4 / 8 concurrent runners (1.00 / 1.96 / 3.96 / 7.75 on darwin/arm64, 10 cores, where all K are truly concurrent), maximum exactly K in any single re-open on both, 124 ns per timed run (116 ns darwin/arm64) — under 1 µs of extra CPU per adaptive route per 5 s, about 1 request in 1.5 million at 1M req/s on four workers. The same correction is applied to reopenSettled and Config.AsyncHandlers. Also fixes a readiness race in the rig itself, found by the 20-run campaign: runStall589 read EngineInfo().Metrics.Workers as soon as one /ping was served, but the native engines rebind per-worker SO_REUSEPORT sockets, so the first worker answers while the worker count is still 0. It aborted one io_uring run in 40 on a precondition. Readiness now waits for the full worker set; a genuine memlock cap still runs the deadline out and fails.
Three defects in the measurement rig added in the previous commit, all found by running it rather than reading it: - The leak scan counted bare occurrences of "startSettleReopener" in the all-goroutine dump, which is TWO per live goroutine (its frame and its created-by line): a single leaked re-opener was reported as 2. It now counts "created by ...startSettleReopener" lines, one per live goroutine. - The jitter guard on the concurrency bound watched only the 300µs branch and missed the 2 ms one. A run over adaptiveBlockingThreshold goes through promoteRouteImmediate, which ALSO zeroes the fast streak, so the route then needs adaptiveSettleStreak fast runs to re-settle and that one re-open window stays open for ~256 runs: max_timed_in_one_reopen=258 with slow_classified=0, twice in five -race runs at 8 runners on 4 CPUs. Both classifications are now counted and reported (blocking_classified), and the bound is not asserted on a case where either fired. - The amortized-fraction bar sat at 1e-6 with every observed value at ~2.4e-7. Its denominator is the rig's own synthetic loop rate, which collapses several-fold under -race or on a loaded box, so the bar was inside the noise. It is now 1e-4 — four orders above the observed values, still tight enough to catch a re-opener that re-times per request (which would read ~1). Stability after the fix: 8/8 -race and 8/8 plain runs of TestRouteAdaptive_* in the container, no failures. Negative control on 385700d unchanged: the clamp test still fails at "cycle 0: fast streak = 257, want 256" and the leak test still reports 1 leaked re-opener goroutine after a failed Start.
CI's race job runs the root package with no -short, so the testing.Short() guard never fired there and every PR paid 35 s under -race for a throughput measurement that a shared runner cannot measure anyway.
…sion rig Rebasing celeris#592 onto main put PR #604 (celeris#593, the io_uring timeout-sweep TryLock) underneath this branch. Both add adaptive_settled_retime_linux_test.go, so the file was an add/add conflict; the #592 version is the one kept, because its `settled` subtest asserts the FIXED behaviour (route promoted within the bound, AsyncPromotedConns >= 1, epoll stalled fraction under 5 %) and requires the old #589 defect signature to fail. Everything #604 changed in engine/iouring/worker.go is unaffected. What is carried over from main's copy of the rig, verbatim: * skipIfMemlockCaps589 — the io_uring pre-flight, gated on the engine's own exported iouring.MaxWorkersForMemlock so the skip predicate and capWorkersToMemlock cannot drift apart. * memlockCeiling589 — so the workers-shortfall Fatal states the ceiling the limit actually allowed instead of speculating about it. * the skipIfMemlockCaps589 call site at the top of runStall589, before the engine is constructed. This is not cosmetic on this branch. The #592 rig tightened the readiness wait to hold out for the WHOLE worker set (a native engine answers /ping from its first SO_REUSEPORT worker while EngineInfo().Metrics.Workers is still 1), so a runner whose RLIMIT_MEMLOCK caps io_uring to one worker no longer under-fills quietly — it runs the readiness deadline out. That is exactly how CI failed PR #603: `server did not become ready (... Workers:1 ...), want 2 workers`. With the pre-flight the io_uring half skips with the memlock reason and the package is ok; the epoll half locks no pages and always runs. When the limit DOES allow the workers, the Metrics.Workers check still Fatals, because then the shortfall is the engine's fault and not the environment's.
…action is now 0 The rig's own prose said the io_uring settled fraction is printed rather than asserted "until that fix lands", and the IOURING592 log line said so on every run. That fix HAS landed: PR #604 (celeris#593) is the commit this branch was just rebased onto. Leaving the text as it was would ship a log line that asserts something false on every io_uring run. Re-measured on this rebased tree, 10 runs per engine, all subtests (golang:1.27, --cpus 4, --security-opt seccomp=unconfined, --ulimit memlock=134217728, workers=2 in all 30 io_uring runs / loops=2 in all 30 epoll runs): 60/60 subtests PASS, 10/10 top-level PASS, package ok 741.99 s. settled, both engines: verdict=FIXED in 20/20, promoted_in_bound=true, promote_ms 5415-5422 of the 8 s bound, async_promoted_conns=8. stalled_frac 0.000 in ALL 60 runs — io_uring settled included (0 of 874-960 /ping samples over the 5 ms bar, ping_max 0.30-2.49 ms). Before #604 the io_uring worker was pinned ~30 % of the window even with every conn async-dispatched, which is why the bar was epoll-only. claim589_assert=fail in 60/60: the #589 defect signature is absent everywhere, so the rig still discriminates in the direction it claims to. 0 ANOMALY589 lines, 0 NOT_FIXED, 0 data races, 0 panics. The stall bar is deliberately left epoll-only. Tightening it to cover io_uring is now supported by the measurement, but it is a second change and belongs in its own commit; the IOURING592 line carries the number for whoever makes it.
FumingPower3925
force-pushed
the
fix/592-settled-route-retime
branch
from
September 14, 2026 05:39
c62bb6a to
82661a5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #592 (measured under #589). The rig that found the defect judges the fix, the negative control was run, and an adversarial reviewer re-read the pushed diff rather than the report.
Reviewer verdict
Finding closed: True. Mergeable: True.
All three binding findings are closed in the pushed tree, verified by reading the diff and re-running tests myself, not from the report. (1) START ORDER: server.go:675 calls startSettleReopener after createEngine's error return and after engineRef.Store(&eng); Shutdown (server.go:370) stops it before the loadEngine()==nil early return. TestRouteAdaptive_NoReopenerWhenEngineCreationFails gates it with a genuine discriminator (a Std server that starts must show reopenStop != nil and a live "created by ...startSettleReopener" frame, and zero after cancel), so a tree that simply never starts the ticker cannot pass; PASS under -race -count=2 locally. (2) CLAMP: router.go:400-403 is
if c := fv.(*atomic.Int32); c.Add(1) >= adaptiveSettleStreak { c.Store(adaptiveSettleStreak); r.settled.Store(...) }; the wrap argument is correct (unclamped, the re-opener adds >=1 per tick forever and a negative int32 makesAdd(1) >= streakunsatisfiable, permanently re-introducing per-request timing). TestRouteAdaptive_FastStreakClampedAcrossReopens asserts 0 <= streak <= 256 over 10,000 cycles and 1,000 in-window runs plus the slow-run zeroing; PASS under -race. (3) DOC: the corrected cost model appears in all three sites (handler.go adaptiveSettleTTL, router.go reopenSettled, config.go Config.AsyncHandlers) and I independently reproduced its shape: GOMAXPROCS=2 -race gave timed_per_reopen 1.00/1.07/1.28/1.24 and max_timed_in_one_reopen 1/2/4/4 at K=1/2/4/8, i.e. max <= K, matching the claim of one timed run per concurrently-executing inline handler per tick.git diff 385700d..HEADon non-test files is EXACTLY these three changes and nothing else (two code lines + doc text), so no scope creep. Hot path is untouched: handler.go:141-155 byte-for-byte identical, adaptiveLearning unchanged, the added Store is on the learning branch only. settled.Clear() requires Go>=1.23 and the module is go 1.27.0. gofmt clean on all six files; go vet clean for darwin and GOOS=linux. Only 6 files in the diff, no scratch files, no debug prints (RESULT592/MEASURE592 are t.Logf, matching house rig style); scratch worktrees are outside the repo and the main worktree is clean. The rig's only change versus the reviewed commit is the SO_REUSEPORT worker-count readiness wait plus a richer fatal message, a real apparatus fix that still fails on a genuine memlock cap.Residuals the reviewer recorded
go test -race -count=1 -timeout=300sover the root package and never passes -short, so its testing.Short() skip does nothing in CI. I measured 35.4 s at GOMAXPROCS=2 under -race, saturating every core; that step takes 154 s today (run 34790598323). It is exactly the class of test whose exclusions that job's own comments enumerate (./adaptive/..., ./middleware/websocket). It also self-disables its load-bearing concurrency bound whenever a jitter run fires (~40% of -race runs at K=8 by the agent's own data), and a >2 ms jitter run calls promoteRouteImmediate, closing the gate for adaptivePromoteTTL = 5 s while waitSettled can only spin against its 30 s fatal deadline. Suggest an opt-in env gate like the other rigs, or capping runners at 4.