fix(server): graceful-shutdown connection drain + guard-first parked teardown (#438 F1/F2) - #445
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds live connection-task tracking, parked-read cancellation, and a five-second graceful-shutdown drain. Parked sessions now deregister clients before closing streams. Unix integration tests cover blocked, idle, and subscribed connections. ChangesConnection shutdown lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ShardEventLoop
participant MonoioIdlePark
participant ConnectionHandlers
participant PersistenceTeardown
ShardEventLoop->>MonoioIdlePark: cancel_all_parked()
MonoioIdlePark->>ConnectionHandlers: cancel parked reads
ConnectionHandlers-->>ShardEventLoop: live connection tasks finish
ShardEventLoop->>PersistenceTeardown: begin teardown after drain or deadline
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix graceful shutdown by draining connection tasks and enforcing guard-first teardown
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shard/conn_accept.rs`:
- Around line 1168-1179: Remove the unwrap calls and their clippy suppressions
from ParkedSession::into_parts and ParkedSession::park_readable. Encode or
validate the populated-session state through the API so into_parts returns an
explicit state result and park_readable handles an absent stream without
panicking, while preserving normal behavior for initialized sessions.
In `@src/shard/event_loop.rs`:
- Around line 2103-2132: Seal Monoio accept and migration intake as soon as
shutdown cancellation begins: update the local_accept_rx and conn_rx try_recv
loops to stop receiving new work, and add the missing cancellation handling to
the local accept task. In the MigrateConnection path of the Monoio source
handler, drain queued migration payloads through guarded handlers before
checking conn_accept::live_conn_tasks(), and reject or stop new migration
handoffs after cancellation. Add a multishard SIGTERM regression test combining
migration with connection intake.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7728647e-733b-49fb-81df-c41e3c3cde1c
📒 Files selected for processing (7)
CHANGELOG.mdsrc/client_registry.rssrc/server/conn/handler_monoio/idle_park.rssrc/server/conn/handler_monoio/mod.rssrc/shard/conn_accept.rssrc/shard/event_loop.rstests/shutdown_drain.rs
Comment on lines
+2103
to
+2132
| // F1 (#438): bounded connection drain BEFORE persistence | ||
| // teardown — the `break` below returns from `run`, and | ||
| // dropping the monoio runtime kills every connection task | ||
| // still pending, truncating in-flight replies and | ||
| // skipping the blocking/subscriber shutdown arms (found | ||
| // live: 19/50 BLPOP clients lost their shutdown reply on | ||
| // Linux io_uring). Stage-1/2 idle-park reads are plain | ||
| // awaits the token cannot wake, so fire their cancellers; | ||
| // the woken handlers see the cancelled token and exit | ||
| // through the flush+FIN epilogue. Re-fired every tick to | ||
| // close the mid-batch re-park race. Deadline-bounded: a | ||
| // wedged peer must not hold up shutdown — its task is | ||
| // then dropped, the pre-F1 behaviour. | ||
| { | ||
| let drain_deadline = std::time::Instant::now() + SHUTDOWN_DRAIN_MAX; | ||
| loop { | ||
| crate::server::conn::handler_monoio::idle_park::cancel_all_parked(); | ||
| let live = conn_accept::live_conn_tasks(); | ||
| if live == 0 { | ||
| break; | ||
| } | ||
| if std::time::Instant::now() >= drain_deadline { | ||
| tracing::warn!( | ||
| "Shard {shard_id}: shutdown drain timed out with {live} connection task(s) still live; dropping them" | ||
| ); | ||
| break; | ||
| } | ||
| monoio::time::sleep(std::time::Duration::from_millis(2)).await; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Seal Monoio input before the shutdown drain.
The local_accept_rx.try_recv() and conn_rx.try_recv() loops run before this shutdown check. The local accept task has no cancellation arm. Under sustained accepts, those loops can keep receiving work and prevent the five-second drain from starting.
A source handler can also return MigrateConnection before its later shutdown check in src/server/conn/handler_monoio/mod.rs Lines 3028-3065. This branch does not drain queued migration payloads before checking live_conn_tasks(). A target shard can tear down while a transferred socket is still queued.
Stop accept intake when cancellation starts. Drain queued migrations into guarded handlers before testing the live count. Prevent new migration handoffs after cancellation. Add a multishard SIGTERM regression test that combines migration and connection intake.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shard/event_loop.rs` around lines 2103 - 2132, Seal Monoio accept and
migration intake as soon as shutdown cancellation begins: update the
local_accept_rx and conn_rx try_recv loops to stop receiving new work, and add
the missing cancellation handling to the local accept task. In the
MigrateConnection path of the Monoio source handler, drain queued migration
payloads through guarded handlers before checking
conn_accept::live_conn_tasks(), and reject or stop new migration handoffs after
cancellation. Add a multishard SIGTERM regression test combining migration with
connection intake.
Collaborator
Author
There was a problem hiding this comment.
Part-skip, part-tracked: (1) "prevents the drain from starting" doesn't hold — the accept-drain loops are non-blocking try_recv with bounded channel capacity, and the shutdown check runs every event-loop iteration, so the drain starts within one iteration regardless of accept pressure. Connections accepted during the drain window queue unspawned and are dropped at teardown — same client-visible outcome as pre-F1 (and as a plain listener close). (2) Sealing intake (cancellation arm on the dedicated accept task + closing listeners at drain start) and the queued MigrateConnection fd payload are real follow-ups — the fd-leak half is exactly issue #438 F4 ("migration fd lifecycle: raw i32 payload with no Drop leaks the fd on shutdown/undelivered"), which is next in the campaign queue and will cover the drain interaction.
There was a problem hiding this comment.
@TinDang97, agreed. I withdraw the claim that the try_recv loops can prevent the shutdown drain from starting. The loops are non-blocking, and the bounded channels ensure that the shutdown check runs on the next event-loop iteration.
The remaining concerns are follow-up work, not blockers for this PR. #438 F4 will address migration FD ownership and the shutdown interaction. Closing accept intake during drain is also appropriate for that follow-up.
🐇
✏️ Learnings added
Learnt from: TinDang97
Repo: pilotspace/moon PR: 445
File: src/shard/event_loop.rs:2103-2132
Timestamp: 2026-08-07T12:02:37.736Z
Learning: In `src/shard/event_loop.rs`, the Monoio `local_accept_rx.try_recv()` and `conn_rx.try_recv()` loops are non-blocking and consume bounded channels. They do not prevent the shutdown check from starting the connection drain on the next event-loop iteration. The shutdown behavior for connections accepted during the drain is to leave them unspawned and drop them during teardown.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
TinDang97
force-pushed
the
fix/438-f1-f2-shutdown-drain
branch
from
August 7, 2026 11:48
49bc762 to
e938c9f
Compare
…teardown (#438 F1/F2) F1 (conn#8) — graceful shutdown never drained connection tasks. On SIGTERM each shard's event loop observed the cancelled token, ran its persistence teardown and returned from `run`; dropping the runtime then killed every pending connection task mid-poll. In-flight replies were truncated and the blocking/subscriber shutdown arms ("-ERR server shutting down") never executed. Measured pre-fix on Linux io_uring: 19/50 BLPOP-blocked clients lost their shutdown reply and saw a bare EOF; macOS/kqueue passed only by scheduler luck (small replies complete inline, conn tasks happened to run before the event-loop task). The shutdown arm now runs a bounded drain BEFORE persistence teardown: - monoio: stage-1/2 idle-park reads are plain awaits the token cannot wake, so the drain fires their cancellers (`cancel_all_parked`, re-fired every 2 ms tick to close the mid-batch re-park race). The woken handler sees the sweep-cancel errno, checks the token, and exits through the normal flush+FIN epilogue instead of re-parking or task-parking. Tracking-connection selects gain a token arm (they park in a select, not a cancel-registered read). - tokio: the main/subscriber/blocking selects already had token arms — they just never got polled. The drain keeps the LocalSet polling until they exit. - Both: a per-shard-thread `ConnTaskGuard` counter (incremented at spawn, RAII-decremented at task exit) is the drain's stop condition; ceiling 5 s (`SHUTDOWN_DRAIN_MAX`) so a wedged peer cannot hold up shutdown — on expiry remaining tasks are dropped, the pre-fix behaviour. F2 (conn#9 / sec L1) — kill_clients' fd-liveness invariant (registry deregister strictly before fd close) held in handler tasks via local-before-parameter drop order, but the task-exit park watcher co-owned {registry_guard, stream} as future upvars, whose drop order is merely capture order — and F1's drain makes dropping that future a routine path, not a teardown-only one. Both are now wrapped in `ParkedSession`, whose hand-written Drop deregisters before closing the fd; the invariant and its owners are restated at the kill site. The wrapper holds plain fields and relies on language-guaranteed declaration-order field drop (RFC 1857) — no Options/unwraps, and the wake path destructures the parts back out — with the order stated as a contract in the struct docs and pinned by a unit test. Generic over the guard type so those tests run under the tokio feature set CI actually executes. Review round: the drain's canceller rescan fast-ticks (2 ms) only for its first 5 iterations — enough to close the one legal re-park interleaving — then backs off to 50 ms, capping the shutdown-only O(registry) scan cost at high connection counts. Bonus find (tokio-only, same class): connections accepted by the CENTRAL listener were io-bound to the MAIN runtime's driver (tokio io resources bind at creation), so their reads/writes died with main's runtime regardless of shard-side draining — with SO_REUSEPORT splitting accepts roughly evenly between the central and per-shard listeners, ~half of all tokio connections failed their final writes with "A Tokio 1.x context was found, but it is being shutdown" (caught by the new drain test in CI; diagnosed via write-path tracing). Forwarded streams are now re-registered with the owning shard's runtime driver at spawn (`into_std` + `from_std` in shard context) — the monoio path already did this by forwarding std streams and converting on the shard. Red/green: tests/shutdown_drain.rs (4 tests: 50-conn BLPOP drain at shards 1 and 2, parked-idle FIN, subscriber FIN — each also asserts bounded process exit). Pre-fix Linux io_uring: 19/50 replies lost; post-fix: 50/50 delivered, all suites green (Linux monoio release, Linux tokio CI-parity, macOS monoio release). Unit tests: cancel_all_parked, ParkedSession drop order/into_parts, ConnTaskGuard. Bench gates waived — no hot-path cost: the drain loop and cancellers run only inside the shutdown arm; ConnTaskGuard is one thread-local Cell increment/decrement per connection LIFETIME (not per operation); the stage-1/2 cancel arms add one `is_cancelled()` atomic load on the already-cold cancel path; the tracking-select token arm is a pending-until-cancelled future identical in cost to the existing subscriber arm. Known flake (pre-existing, not this change): parked_idle_parity timing asserts fail under heavy suite load on the shared VM (fires with 2 vCPUs, passes solo) — deadline-poll fix deferred to the test-hygiene sweep. Refs #438 (F1 = conn#8, F2 = conn#9 = sec L1) author: Tin Dang
TinDang97
force-pushed
the
fix/438-f1-f2-shutdown-drain
branch
from
August 7, 2026 12:02
e938c9f to
f867d7b
Compare
TinDang97
added a commit
that referenced
this pull request
Aug 7, 2026
… gates (#438 F3–F6 + conn-secondary) (#446) * fix(server): accept-loop HOL block, migration fd ownership, park auth gates (#438 F3-F6 + conn-secondary) F3 (routing F10) — the central accept loop delivered every connection with a blocking send on the routed shard's bounded (4096) conn channel: one wedged shard froze accepts for ALL shards. The monoio legs were worse than reviewed — `flume::Sender::send` there is synchronous, so a full channel stalled the entire listener THREAD (plain + TLS legs alike). All five delivery sites (tokio plain/TLS, monoio plain/TLS x2 select shapes) now `try_send` and rotate to the next shard with room (`try_route_conn`); the affinity hint is best-effort and any live shard serves the client. Only when EVERY channel refuses does the loop fall back to the pre-F3 blocking send — server-wide saturation, where back-pressure is the correct behaviour — with a warn. Unit-tested (rotation, disconnected-skip, all-full payload return, target-first). F4 (routing F12) — MigrateConnectionPayload carried the migrated socket as a raw i32 with no Drop: a migration message still queued in an SPSC ring at shutdown, or drained into pending_migrations but never spawned, leaked the fd and stranded the client on a connection no task would ever serve (silent, until the fd table filled). The payload now owns the socket as an OwnedFd end to end — producer (tokio into_std; monoio dup wrapped at birth) → ring → pending_migrations → spawn_migrated_* — so every undelivered path closes the socket and the client gets a FIN. This also discharges the F1-drain interaction flagged on PR #445: queued migrations dropped by a shutting-down shard now close deterministically. Three `unsafe from_raw_fd` blocks became safe ownership conversions (net -3 unsafe). Unit test pins the drop semantics; a revert to a raw fd no longer compiles. Resumed-parked conns stay deliberately can_migrate:false — the resume wrapper has no MigrateConnection arm, and migration sampling exists for HOT conns while this path is only reachable by idling past --conn-park-secs; rationale documented at the spawn site. F5 (sec L3) — both migrated-connection spawn sites built the target shard's ConnectionContext with requirepass: None. Session auth was unaffected (it travels in MigratedConnectionState), but AUTH issued on a migrated conn wrongly answered "no password is set", and any future auth-re-derivation from the context would have failed open. The real requirepass is now read from runtime config at spawn. F6 (sec L2) — unauthenticated connections could task-park: on an auth-enabled server an attacker could open maxclients silent sockets, never AUTH, and hold each at the parked ~3.3 KB watcher footprint indefinitely and invisibly. The stage-2 parkable predicate now requires conn.authenticated — pre-AUTH conns keep their full handler task (costly enough to surface in monitoring, still reaped by `timeout N`); no-auth servers see no change (authenticated=true from accept). Red/green on macOS monoio: parked_clients=2 pre-fix, =1 post-fix (authed idle sibling doubles as the positive control that the park machinery stayed live). conn-secondary — * is_sweep_cancel matched any bare errno 125, so an ECANCELED from a non-sweep source read as "sweep cancel -> re-park": for a dead fd that is a permanent park->wake->park spin at 100% CPU (the exact E11 failure class the errno check was built to prevent). The idle-park slot now records provenance (`swept`, set by sweep/drain before firing, re-armed false at every park) and the park arms require it via consume-once `was_swept_cancel`; blocking.rs keeps the plain errno check where a stray 125 only re-arms a level-triggered poll (no spin possible). Unit-tested (provenance required, consume-once, stale-flag cleared on re-park, drain marks too). * client_registry::register double-counted TOTAL_CLIENTS and the shard gauges when inserting over a still-present id (the kept_registration miss-arm fail-safe racing an entry back into existence): permanent +1 skew feeding shard_overloaded routing. register() now balances against the replaced entry, and the miss arm (an invariant violation, not a code path) warns loudly. Unit-tested. * `timeout` read-once: already fixed by the D1 chore-sweep rework — both shard chores re-read runtime_config.timeout every second and CONFIG SET timeout is live (verified, comment at the monoio chore says exactly this). No change. Gates: fmt + clippy -D warnings (default and tokio,jemalloc), new unit tests green under both feature sets, parked_idle_parity 7/7 + shutdown_drain 4/4 + tls_park_keyupdate on macOS monoio AND tokio (F6 leg degenerates to parked=0 under tokio as documented), full suites on macOS monoio (28 suites; one cross_shard_consistency_red load flake, solo-green — same documented family as cdg6e), Linux VM monoio (lib 4519/4519; client_tracking_invalidation push-delivery flake fires on UNMODIFIED main too — 3x-vs-3x merge-base A/B, 1/3 main vs 2/3 branch, same missing-second-key signature; pre-existing, queued for the test-hygiene sweep) and Linux VM tokio CI-parity (exit 0, 190 suites). Bench gates waived — no hot-path cost: try_route_conn replaces a blocking send with try_send attempts on the accept path (not the command path); the OwnedFd is layout-identical to i32 with a Drop that only runs on the cold migration/teardown path; the park-arm provenance check is two Cell ops on the already-cold cancelled-read path; the parkable predicate gains one bool test evaluated once per park decision. Refs #438 (F3 = routing F10, F4 = routing F12, F5 = sec L3, F6 = sec L2, conn-secondary) author: Tin Dang * fix(server): review round for #446 — live-shard fallback, deterministic tests, F6 identity check Review findings (Qodo + CodeRabbit, converging on one real defect): 1. Fallback could drop a conn a live shard would take (both bots, CONFIRMED): when try_route_conn exhausted the rotation, the blocking fallback targeted the ORIGINALLY routed shard — if that receiver was disconnected while another shard was merely full, the fallback errored instantly and the accepted conn was dropped. The helper now reports the first LIVE-but-full shard from the rotation (Err((Option<usize>, payload))); callers back-pressure on THAT shard, and only drop (with an error log) when every receiver is gone — i.e. shutdown, where dropping is correct. New unit tests: disconnected-target-with-live-full-shard, all-disconnected. 2. Flaky global-state unit tests (both bots): the TOTAL_CLIENTS bracket could false-fail if an unrelated test registered between its two loads, and a failed assert skipped cleanup; it now retries with a fresh id (bug reads +1 on EVERY attempt; fix needs one undisturbed attempt) and deregisters before the verdict. The F4 fd-drop test probed the raw fd number, which the parallel runner could reuse between close and probe; it now observes the close through the PEER's read() -> EOF — no fd-number reasoning at all. 3. F6 test identified WHICH conn parked (CodeRabbit): parked_clients=1 also holds if the silent conn parked and the authed one didn't. The test now closes the authed conn and requires the gauge to drain to 0 while the silent conn stays open. Skipped with rationale (replied on-thread): - "Split oversized files" — pre-existing sizes, out of scope for a hardening PR; noted for a future refactor wave. - New-unsafe flag on the monoio dup wrap — consolidates two prior unsafe sites at the same location; net unsafe count for the PR is -3, SAFETY comment present. Gates: fmt + clippy -D warnings (both feature sets), listener/registry/ migrate-fd unit tests green both feature sets, parked_idle_parity monoio 3/4 full-green runs (the one bad run failed on connect/read setup races in OTHER tests — the documented harness flake class queued for the test-hygiene sweep; the new assertions never tripped) and tokio 7/7. Refs #438, #446 author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 7, 2026
…iene) (#449) Pid-only temp-dir names (`temp_dir().join(format!("...-{}", pid))`) at eight spawn sites across six suites resurrected STALE data dirs: a crashed run leaves its dir behind, the pid is eventually reused, and the next run's server silently reloads the leftover persistence state (the documented CWD/stale-reload trap) — failures then point at whatever assertion tripped over the ghost data, not at the cause. - info_memory_allocator_pagecache, memory_doctor_response, memory_prometheus_kinds: the shared Moon harness now owns a tempfile::TempDir (random unique name; removed on drop AFTER the child is killed — hand-written Drop body runs before field drops). - vector_exact_rerank (3 sites): RAII tempdir; also removes the manual pre/post remove_dir_all pairs, so cleanup now happens even when an assert panics mid-test. - vector_db_isolation (2 sites): unique random dirs via tempfile::Builder + keep(). Deliberately NOT RAII: the restart test shares one dir between two Moon values (kill_keep_dir → same-port respawn), so the existing PathBuf ownership + manual cleanup are kept and only the collision-prone NAME is fixed. - tls_park_keyupdate: RAII tempdir; early-return and end-of-test manual removes deleted. parked_idle_parity deadline-poll fixes (the flakes deferred from the #444/#445 rounds, all environmental classes observed in CI or today's gate runs): - CLIENT KILL registry-removal assert now POLLS with a 10s deadline — the client-side close arrives instantly via shutdown(2) but the registry entry is released only when the killed handler task gets scheduled; the read-once assert fired 3/3 on the starved 2-vCPU runner and passed solo (documented assert-too-soon race). - All 13 TcpStream::connect sites use a connect_retry helper (10s deadline, 50ms backoff): under full-suite load a freshly listening server can still refuse the first attempts, which failed tests at the connect line (seen live today at the F6 test's connect). - Read deadlines 10s → 30s in the two reply helpers; deadline-bound, so green runs spend no extra time. The client_tracking_invalidation multikey second-key push flake is PRODUCT-side (a real RESP3 client would miss the same invalidation) — filed as #448 with the merge-base A/B evidence instead of being papered over here. Gates: fmt + clippy -D warnings (default and tokio,jemalloc); all six converted suites + parked_idle_parity green on macOS monoio (31 tests); tokio leg green on the four suites that run there. Bench gates waived — tests-only change, no src/ code touched. Refs #444 #445 #446, closes nothing (#448 filed for the product flake) author: Tin Dang
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
1 participant
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.
Summary
Lands F1 (conn#8) and F2 (conn#9 / sec L1) from the #438 c10k hardening tail — together, as the review prescribed: F1's drain refactor is exactly what makes F2's latent drop-order hazard a routine path. Writing F1's red test then caught a third bug in the same class (tokio-only, pre-existing).
F1 — graceful shutdown never drained connection tasks
On SIGTERM each shard's event loop ran its persistence teardown and returned from
run; dropping the runtime killed every pending connection task mid-poll. In-flight replies were truncated and the blocking/subscriber shutdown arms (-ERR server shutting down) never executed.Measured pre-fix (Linux io_uring): 19/50 BLPOP-blocked clients lost their shutdown reply. macOS/kqueue passed only by scheduler luck.
Fix — a bounded drain in the shutdown arm, before persistence teardown:
cancel_all_parked, re-fired per 2 ms tick to close the mid-batch re-park race); the woken handler checks the token and exits through the normal flush+FIN epilogue. Tracking-conn selects gain a token arm.ConnTaskGuardlive-task counter (incremented pre-spawn, RAII-decremented at exit). CeilingSHUTDOWN_DRAIN_MAX = 5 sso a wedged peer cannot hold up shutdown (on expiry: pre-fix behaviour).Bonus fix — tokio central-listener connections died with the MAIN runtime's io driver
The first CI run of the new drain test failed under tokio with ~half the clients losing replies despite the drain running correctly (live 50→0 in 16 ms). Write-path tracing showed their final writes failing with "A Tokio 1.x context was found, but it is being shutdown": connections accepted by the central listener are io-bound at creation to the main runtime's driver, so once main's runtime drops, their io is dead no matter what the shard drains — and SO_REUSEPORT splits accepts roughly evenly between the central and per-shard listeners on Linux. Forwarded streams are now re-registered with the owning shard's runtime driver at spawn (
into_std+from_stdin shard context). The monoio path was already immune (it forwards std streams and converts on the shard). Post-fix: 50/50 replies under tokio.F2 — guard-first {registry_guard, stream} teardown
kill_clients' fd-liveness invariant (deregister strictly before fd close) held in handler tasks via local-before-parameter drop order, but the task-exit park watcher co-owned both as future upvars (drop order = capture order, no guarantee). Now wrapped inParkedSessionwith a hand-written guard-firstDrop; invariant + owners restated at the kill site. Generic over the guard type so the drop-order unit tests run under the tokio feature set CI executes.Red/green
tests/shutdown_drain.rs: 50-conn BLPOP drain (shards 1 + 2), parked-idle FIN, subscriber FIN — each also asserts bounded process exit. Pre-fix: Linux io_uring 19/50 lost (monoio), Linux tokio ~25/50 lost → post-fix 50/50 on both runtimes.cancel_all_parkedage/idempotency,ParkedSessiondrop order +into_parts,ConnTaskGuardcounting.Gates
-D warnings(default + tokio,jemalloc) ✅ConnTaskGuardis one thread-local Cell op per connection lifetime; the tokio stream re-registration is oneinto_std/from_stdpair per connection accept (epoll deregister+register, off the per-op path).Refs #438