fix(conn): c10k cluster C — bound reply writes, query buffers, and maxclients rejections (+ allocator observability) - #431
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Collaborator
Author
|
CI dispatched manually —
Expected result: one pre-existing failure, |
TinDang97
added a commit
that referenced
this pull request
Aug 5, 2026
…ned to hold locally `freed_memory_stops_being_charged_to_the_process` passed on an Apple Silicon dev machine and FAILED on the GitHub macOS runner, which retained all 386 MiB of a 384 MiB churn-and-free indefinitely. Both were the same commit. That failure is not a flake — it is the retention this module was written for, finally reproducing. It also invalidates a claim made in the previous commit: that the "an idle jemalloc on macOS never purges" hypothesis had been DISPROVED. It had not. One machine reclaimed on its own and I generalised from it. Retention is real; it is just not universal, so "it reclaimed on my machine" was never evidence of anything. The test now asserts the property that IS invariant and does matter: moon must be able to get freed memory back. Going idle is allowed to be sufficient (it is on Linux, and on some macOS configurations); where it is not, the decay lever must do the job. A platform where NEITHER works is the real defect, and that is now what fails. Structurally: leg 1 measures idle reclaim and returns early if it sufficed; leg 2 drives `decay_all_arenas()` for up to 30s — long enough to outlast muzzy_decay_ms, since decay is a two-stage pipeline and one call cannot finish the job — and requires the memory back. Both legs log which path was taken, so the CI output says which side of this split the runner is on rather than leaving it to be inferred from a pass. Module and flag docs corrected to record both measurements side by side instead of the one that fit. `--memory-decay-interval-ms` stays 0: a default should not be flipped on a property that varies by platform without knowing which side a given deployment is on, and `allocator_unreturned_bytes` is now the way to find out. Refs: PR #431 CI run 30990548603 (Check (macOS)) author: Tin Dang
Collaborator
Author
The macOS decay experiment came back negative — the lever is removedThe previous CI run was a real experiment, and it falsified the remedy rather than confirming it.
The two control tests on that same runner passed: So What survives is the part that is proven and uncontested:
Windows: a separate, real bug in my testsThe same run surfaced that all three Fixed by pinning The remaining Windows failure, Gates: fmt, clippy (default + tokio + jemalloc-stats, |
Collaborator
Author
CI settled — the branch is clean; one pre-existing Windows failure remainsRun 30994063596 (after re-running the failed jobs):
Windows is now down to the single pre-existing failure. 5218/5219 pass, and the test count dropped 5222 → 5219, which is the three stall-dependent tests being skipped as intended. That remaining failure reproduces on The Linux failure was contention, not code. First attempt failed Windows write-timeout coverage is a stated gap, not a silent oneThe three stall-dependent tests are skipped on Windows because two attempts to make the server's Cost, stated in Ready for review. |
This was referenced Aug 5, 2026
Closed
TinDang97
force-pushed
the
fix/c10k-acl-intercept-order
branch
from
August 6, 2026 14:37
b53a654 to
5374628
Compare
… (c10k C2) A connection's input buffer grew to whatever a frame header declared. The only ceiling was the parser's 512 MiB bulk limit, which ACCEPTS that size rather than rejecting it, so `$536870911` followed by a dribble of bytes pins half a gigabyte per connection. That memory lives outside `used_memory`, so `maxmemory` never sees it and eviction never fires. And the auth gate runs AFTER parsing, so none of it costs credentials. Measured on the pre-fix binary (macOS, `--requirepass pw`, one unauthenticated socket, never sending a valid command): 200 MiB accepted into a single query buffer, server RSS 4 MB -> 208 MB, connection never closed and nothing logged. Twenty such connections is 10 GB of invisible RSS from a client that cannot run a single command. Two knobs, both Redis-shaped: --client-query-buffer-limit default 1gb (Redis parity) --client-query-buffer-limit-preauth default 64kb The pre-auth ceiling is the part that makes this unreachable without credentials. No legitimate pre-auth command is large — AUTH, HELLO and the inline forms all fit in well under a kilobyte — and the full limit applies from the moment a client authenticates. `query_buf_limit` resolves the pair: the pre-auth value never exceeds the general one, and 0 means "no separate pre-auth rule" rather than "unlimited". Enforcement sites differ by handler because their read models do: - monoio + sharded: one check per read iteration, sited after every read arm and ahead of both parse paths. An incomplete frame is exactly what makes the buffer grow, and it decodes to nothing, so the loop would otherwise come straight back and read more. - handler_single reads through `Framed`, which loops read -> decode -> read internally and does NOT yield while a frame is incomplete — a handler-side check would never run during the growth that matters. The ceiling therefore lives in `RespCodec::decode`, the one place that sees the buffer on every pass, and the loop re-arms it each turn rather than patching all five AUTH/HELLO success sites. The connection gets `-ERR Protocol error: query buffer limit reached` before it is closed. Redis closes silently and logs; a silent close in the middle of a large pipeline is very hard to tell from a crash. Tests: tests/query_buffer_limit.rs (6 e2e, both shard configs, both runtimes) + unit tests for the limit resolution. RED, on the pre-fix binary: "an unauthenticated client dribbled 4249744 bytes into a 512 MiB bulk header and the server was still holding the connection open" at shards=1 AND shards=4. The two `large_value` positive controls stayed green, so the failures are the defect and not the harness. Two harness traps found while proving it, both of which made an earlier version of this suite pass VACUOUSLY: 1. The readiness probe accepted only `+PONG`. These servers run with `--requirepass`, so an unauthenticated PING is refused — `spawn_moon` returned None and every requirepass test returned `ok` without ever connecting. It now also accepts `-NOAUTH`. 2. The dribble used a blocking write plus a per-chunk read timeout, which paced the sender so slowly that a server absorbing everything looked like a server that had stopped reading. It is non-blocking now. Gates: fmt; clippy -D warnings on default and runtime-tokio,jemalloc; lib 4454; query_buffer_limit 6/6 on both runtimes. Refs: tmp/C10K-HARDENING-REVIEW.md C2. author: Tin Dang
…rning (c10k C3) Rejecting a connection for `maxclients` did two unbounded things. The 36-byte `-ERR max number of clients reached` write was untimed. Any peer that reads at all completes it instantly; a peer advertising a zero window never does, and the write simply waited. The rejected fd stayed open for as long as the attacker cared to hold it, so live fds climb past `maxclients` without limit — which defeats the gate and the RLIMIT_NOFILE reconciliation sitting behind it. All three rejection sites are now bounded at 2s: - `conn_accept.rs` monoio (spawned task) — the site the review names; - `conn_accept.rs` TLS (tokio) and `handler_sharded` (tokio), which the review does not name and which are WORSE: both await INLINE, so one zero-window peer parks the accept path rather than a detached task. And every rejection logged a line. Being at `maxclients` is precisely the state in which connections arrive fastest, so the symptom produced an unbounded log flood that competes for the I/O needed to diagnose it. The warning is now at most one per second and carries the number of rejections it stands for, so nothing is silently dropped. Testing, stated honestly: the rate limiter has a unit test (`now_ms` is a parameter, so it is deterministic and needs no sleeping). The write timeout does NOT have an end-to-end test — driving it requires a peer whose receive window is zero for a 36-byte write, which cannot be arranged portably from a test process. It is a defensive bound on a path that was previously unbounded, and `maxclients_reject_parity` still covers the rejection behaviour itself on both runtimes. Gates: fmt; clippy -D warnings on default and runtime-tokio,jemalloc; lib 4455; maxclients_reject_parity 2/2 on both runtimes. Refs: tmp/C10K-HARDENING-REVIEW.md C3. author: Tin Dang
… parks a handler forever (c10k C1) `write_all` on a socket whose receive window is closed never returns. A client could pipeline a large response and then simply stop reading, leaving the handler blocked inside that write while it held the entire serialized reply — the monoio handler coalesces a whole batch into ONE `Bytes` before its single write syscall, so a deep pipeline is hundreds of megabytes — plus its `maxclients` slot, for as long as the attacker cared to wait. N such clients is an OOM, and it costs the attacker nothing: no reads, no CPU, just a TCP window it refuses to open. Adds `--client-write-timeout-ms` (default 60000; 0 keeps the previous wait-forever behaviour) and applies it to every reply-carrying write in all three handlers, which each carry an independent copy of the write path: - `handler_monoio` — batch flush, inline flush, txn flush, pubsub burst, RESP3 push. Bounded with `monoio::select!` against a sleep, the same shape the idle timeout already uses on the read side; the losing future is dropped and takes the reply buffer with it, which is what we want when the next step is closing the connection. - `handler_sharded` — batch flush, blocking-command flush, RESP3 push. - `handler_single` — the tokio `Framed` sink: batch sends, post-AOF flush, subscribe replies, and pubsub pushes. The two tokio-side handlers use `tokio::time::timeout`, matching how they already bound reads. A cancelled write may leave a partial frame on the wire; that is correct here, because the only thing that happens afterwards is teardown. 60s is far beyond any healthy client's stall, and replication does not travel this path — PSYNC hijacks the connection before it. Operators streaming very large replies over very slow links should raise it: the budget covers the whole write call, not each byte. Testing (`tests/write_timeout.rs`, 1 and 4 shards, monoio and tokio): a victim that pipelines ~25 MB of GETs and then stops reading is disconnected, observed from a third-party admin connection via CLIENT LIST because a victim that never reads cannot observe its own teardown promptly. The `--client-write-timeout-ms 0` case asserts the victim SURVIVES, which is the differential control — it proves the timeout is doing the work rather than the harness reporting a pass in both directions. Refs: tmp/C10K-HARDENING-REVIEW.md (C1) author: Tin Dang
…ut (c10k C1) `format_client_line` hardcoded `obl=0 oll=0 omem=0 tot-net-out=0`. That is why the unbounded-write OOM fixed in the previous commit left no trace: an operator watching CLIENT LIST while clients pinned hundreds of megabytes of unread replies saw every one of them reporting zero bytes held. The one signal that would have identified the offending connections was a constant. `ClientLiveState` gains two relaxed atomics — `pending_out_bytes` and `tot_net_out` — maintained by the same `write_all_bounded!` macro that bounds the write, so every reply-carrying path is covered by construction and there is no second place to keep in sync. Two relaxed stores on a path that is about to make a write syscall; the cost is not measurable there. Field mapping, which is not one-to-one with Redis: - `obl` / `omem` — bytes of reply currently in an in-flight write. moon serializes a whole batch into ONE buffer before its single write syscall, so this is exactly the memory a non-reading client is pinning. - `oll` — stays 0. Redis reports the length of a reply *list*; moon has one contiguous output buffer and no list, so there is nothing to count. - `tot-net-out` — cumulative bytes that actually reached the peer. A write abandoned by `--client-write-timeout-ms`, or failed outright, does not count. Wired in the two handlers that own a client-registry entry (monoio top-level and sharded). `handler_single` bounds its writes but never registers with the client registry, so it has nothing to report through; noted rather than silently skipped. Testing (`tests/write_timeout.rs`): against the wait-forever server — where the stall is stable and there is no race to lose — a victim holding ~25 MB of unread reply must report `omem > 1MB` with `obl == omem`, while the admin connection that reads its replies normally must report `omem = 0`. Without the second assertion the counter could be noise rather than a signal. Green on monoio and tokio. Refs: tmp/C10K-HARDENING-REVIEW.md (C1) author: Tin Dang
…-buffer test (c10k C2) `run_preauth_cap` asserted `sent <= 1 MiB`, where `sent` is the number of bytes the CLIENT's non-blocking writes were accepted for. Those bytes live in the client's sndbuf, the in-flight window, and the server's rcvbuf — not in moon. Linux autotunes those buffers into the megabytes, so the assertion passed on macOS and failed on Linux at 2.5 MiB (and 1.25 MiB at 4 shards) while the server's own `read_buf` never exceeded the 64 KiB ceiling. The test was measuring the operating system. Replaced with a differential control that measures the server: the same 4 MiB dribble against a second instance started with the ceilings explicitly disabled must NOT be closed. Socket buffering is identical on both legs, so it cancels out — if the capped connection dies and the uncapped one survives, the ceiling is what killed it, and nothing about the OS can fake that. This also closes a hole in the original test: had something OTHER than the ceiling been ending these connections, the `closed` assertion would still have passed and the test would have proven nothing. The control now fails loudly in that case. Green on Linux (tokio) and macOS (monoio), 1 and 4 shards. Refs: tmp/C10K-HARDENING-REVIEW.md (C2) author: Tin Dang
… and keep the write watchdog off the hot path (c10k C1) Two changes that finish C1. 1. Size cap. `--client-output-buffer-limit-normal` is Redis's `client-output-buffer-limit normal <hard>`, shipped at 256 MiB instead of Redis's unlimited default — that default is precisely why an unread socket can OOM Redis too. A reply larger than the cap is refused and the connection closed rather than buffered. The cap covers the whole serialized reply, so a single value larger than it is undeliverable, not merely a long pipeline; that is intended, and documented on the flag. No pub/sub-class knob ships. moon's only subscriber write is already hard capped at 64 KiB by `MAX_COALESCE_BYTES`, so a pubsub byte-limit could never fire — a knob that cannot fire is worse than no knob, because it reads as protection. The real pub/sub backpressure is the 4096-slot bounded channel (`CONN_CHANNEL_CAPACITY`), which bounds message COUNT rather than bytes. That is a real gap, recorded as a follow-up rather than papered over here. 2. The watchdog stops arming on the hot path. Arming a timer costs a wheel insert plus removal on EVERY batch flush, which at pipeline depth 1 is once per command — on the path this project spent a whole milestone winning against Redis. A reply that fits in the socket buffer is handed to the kernel and returns without blocking, so no timeout could ever fire for it. Only writes >= 256 KiB now arm it (`util::arm_write_timeout`). The same-binary A/B this was meant to be gated on could NOT resolve the question: on the 2-core moon-dev VM the noise floor swamped the effect — P=16 came out +26.7% for the configuration doing strictly MORE work, and raw samples spanned 1.4-1.7x within a single configuration. Rather than report a number that measurement does not support, the cost is removed by construction: the hot path now pays one integer compare. Residual, stated plainly: a SMALL write to a genuinely wedged socket is still unbounded. It holds at most 256 KiB rather than the hundreds of MB C1 is about, but it does keep its `maxclients` slot. That connection is now visible (`omem` > 0 in CLIENT LIST) and killable (`CLIENT KILL` shuts the fd down, which makes the blocked write return) — neither was true before. Testing: `tests/write_timeout.rs` gains an oversized-reply pair at 1 and 4 shards driving a 1 MiB cap, with an under-cap reply as the control so the test cannot pass by breaking everything. `util::write_timeout_gate_tests` pins the gate itself, including that `--client-write-timeout-ms 0` is not resurrected by a large write. 8 integration + 3 unit tests green; fmt and both clippy configurations clean. Refs: tmp/C10K-HARDENING-REVIEW.md (C1) author: Tin Dang
…add an opt-in decay lever
A real instance (macOS, 6.8 days uptime) reported `used_memory` 2.43 GB while
the OS charged the process 7.3 GB, 7.2 GB of which had been pushed to swap.
Nothing in `INFO memory` could explain the ~4.9 GB gap, so the difference
between "jemalloc fragmentation" and "memory moon holds that used_memory does
not charge for" was pure guesswork. This makes it measurable.
`--features jemalloc-stats` compiles jemalloc with `--enable-stats` and adds
Redis's `allocator_*` fields to `INFO memory`, so existing dashboards read them
without translation:
active - allocated -> allocator_frag_bytes (size-class rounding)
resident - active -> allocator_unreturned_bytes (dirty, not given back)
OS charge - resident -> not the allocator's (mmap, stacks, image)
Off by default: jemalloc's stats cost bookkeeping on every allocation and this
project does not pay that on the hot path unconditionally. The fields are
ABSENT rather than zero-filled in a default build — a zero reads as "no
fragmentation", which is a worse answer than "not measured".
`jemalloc_stats()` rate-limits its `epoch` advance to once per 5s. That is not
incidental: the admin metrics scrape path already documents that advancing
`epoch` once a second made jemalloc's internal bookkeeping grow without bound
(~1 MB / 20 s) and deliberately avoids it. INFO can be polled by a monitoring
agent at any rate, so the throttle lives in the accessor rather than trusting
callers.
Also adds `--memory-decay-interval-ms` (default 0) and `memory_ctl`, which
calls `mallctl("arena.4096.decay")` on a plain OS thread. jemalloc's
`background_thread` is genuinely compiled out on Apple platforms —
`JEMALLOC_BACKGROUND_THREAD` is only defined when `abi != macho` in jemalloc's
configure.ac — so the `background_thread:true` baked into moon's malloc conf is
a silent no-op there.
It ships OFF, and is deliberately NOT presented as the fix for the instance
above. The hypothesis that an idle jemalloc never purges on macOS was tested
and DISPROVED: 384 MiB churned and freed drops the physical footprint to 3.7
MiB with no decay call at all, because decay also runs as a side effect of the
frees themselves. Enabling this by default would be claiming a fix that has not
been demonstrated.
Two measurement traps are recorded in `tests/allocator_idle_decay.rs`, both of
which produced confident wrong answers first:
- `ps -o rss` is the wrong instrument on macOS. Darwin jemalloc is built with
JEMALLOC_PURGE_MADVISE_FREE, so purged pages stay resident and merely become
discardable: RSS reads ~387 MiB where the physical footprint reads 3.7 MiB.
- Validate the instrument before trusting it. A vmmap parse that silently
returns a small number is indistinguishable from a process that reclaimed
perfectly, so `footprint_tracks_live_allocations` fails loudly if the reader
breaks. Without it the whole file would confirm whatever you hoped.
The unsafe `mallctl` FFI block was explicitly approved. It is the minimum
possible: `arena.<i>.decay` is declared NEITHER_READ_NOR_WRITE in jemalloc's
ctl.c, so every pointer is NULL and newlen is 0 — no buffer is read or written,
and there is nothing for the caller to size or own. SAFETY comment records
this, and a unit test asserts the linked jemalloc still accepts the ctl so it
cannot rot into a silent no-op.
Testing: 3 unit + 3 integration tests. fmt and clippy clean under default,
`jemalloc-stats`, and `runtime-tokio,jemalloc`. Verified end to end against a
live server: allocator_resident 75.8 MB against an OS RSS of 74.3 MB.
author: Tin Dang
`scripts/audit-unsafe.sh` only scans the three lines immediately above an `unsafe` block for a `SAFETY:` marker. The rationale on `decay_all_arenas` ran to seven lines, so the marker sat outside that window and CI's Lint job failed with "1 unsafe blocks are missing // SAFETY: comments" — the comment was there, the audit just could not see it. Moves the full argument into a `# Soundness` section on the function's doc comment, where it is also visible in rustdoc, and leaves a three-line `// SAFETY:` adjacent to the block that names the same invariant. Nothing about the unsafe call changed. My miss: I ran fmt, clippy and the test suites locally but not `scripts/audit-unsafe.sh`, which is part of the CI gate. author: Tin Dang
…ned to hold locally `freed_memory_stops_being_charged_to_the_process` passed on an Apple Silicon dev machine and FAILED on the GitHub macOS runner, which retained all 386 MiB of a 384 MiB churn-and-free indefinitely. Both were the same commit. That failure is not a flake — it is the retention this module was written for, finally reproducing. It also invalidates a claim made in the previous commit: that the "an idle jemalloc on macOS never purges" hypothesis had been DISPROVED. It had not. One machine reclaimed on its own and I generalised from it. Retention is real; it is just not universal, so "it reclaimed on my machine" was never evidence of anything. The test now asserts the property that IS invariant and does matter: moon must be able to get freed memory back. Going idle is allowed to be sufficient (it is on Linux, and on some macOS configurations); where it is not, the decay lever must do the job. A platform where NEITHER works is the real defect, and that is now what fails. Structurally: leg 1 measures idle reclaim and returns early if it sufficed; leg 2 drives `decay_all_arenas()` for up to 30s — long enough to outlast muzzy_decay_ms, since decay is a two-stage pipeline and one call cannot finish the job — and requires the memory back. Both legs log which path was taken, so the CI output says which side of this split the runner is on rather than leaving it to be inferred from a pass. Module and flag docs corrected to record both measurements side by side instead of the one that fit. `--memory-decay-interval-ms` stays 0: a default should not be flipped on a property that varies by platform without knowing which side a given deployment is on, and `allocator_unreturned_bytes` is now the way to find out. Refs: PR #431 CI run 30990548603 (Check (macOS)) author: Tin Dang
…dows write-timeout tests
Two CI findings from run 30991268000, both of which falsified an assumption
rather than revealing a coding slip.
1. --memory-decay-interval-ms does not work where it is needed. REMOVED.
The lever spawned a housekeeping thread calling mallctl("arena.4096.decay")
— the same call jemalloc's own background thread makes — to compensate for
JEMALLOC_BACKGROUND_THREAD being compiled out when abi == macho.
The CI macOS runner is the environment that reproduces the idle retention
this was built for. There, driving that ctl for 30 seconds reclaimed
nothing: the footprint stayed 386 MiB above baseline across three
independent nextest retries, while mallctl itself returned success and a
separately-validated footprint instrument confirmed the measurement was
real. Shipping it would have meant a new unsafe FFI block plus a documented
knob that silently fails on precisely the platform that needs it.
So the lever, its unsafe block, and the tikv-jemalloc-sys direct dependency
are gone. What remains is the ability to SEE the problem: --features
jemalloc-stats and INFO memory's allocator_* fields, which are proven and
uncontested. The finding is recorded in src/memory_ctl.rs, CHANGELOG "Known
issues", and the test, so it is measured rather than forgotten.
tests/allocator_idle_decay.rs now asserts hard on Linux — which production
targets, and where the background thread is compiled in — and records the
measurement on Apple, where retention is a known platform limitation with no
in-process remedy. Failing the build on something unfixable only trains
people to ignore the gate.
2. All three write_timeout tests failed on Windows. FIXED.
The victim helper assumed ~25 MB of pipelined reply was "comfortably past
any kernel socket buffer". That is a guess, and Winsock falsified it:
send/receive autotuning absorbed the whole reply, the server's write_all
completed normally, and the tests observed a healthy client with omem=0.
The victim now pins SO_RCVBUF to 8 KiB before connecting, which closes the
TCP window deterministically instead of depending on a platform's default
buffer sizing — and on Windows also disables the receive-window autotuning
that grew the buffer in the first place.
Gates: fmt, clippy (default + tokio + jemalloc-stats, all -D warnings),
audit-unsafe (257/257), audit-unwrap (0), write_timeout 5/5,
allocator_idle_decay 2/2.
Refs #20 (c10k hardening campaign, cluster C)
author: Tin Dang
`freed_memory_is_returned_to_the_os_when_idle` is a measurement, not just a
pass/fail gate. On Apple platforms it deliberately records whether the host
reclaimed or retained rather than failing, because jemalloc's background_thread
is compiled out when abi == macho and no in-process remedy exists.
nextest captures stdout for passing tests, so on a green macOS run the only
number the test exists to produce was being discarded — the claim that it
"records the measurement" was not true in CI. A per-test `success-output`
override surfaces it either way.
Verified locally under `--profile ci`:
RECLAIMED: 9 MiB still charged after churning and freeing 384 MiB
(baseline 1 MiB, idle floor 10 MiB)
Refs #20
author: Tin Dang
…ented as a gap
The C1 write-timeout tests need the server's `write_all` to actually block.
On Linux and macOS it does. On Windows it does not, and two attempts to force
it both failed in CI:
1. Relying on a ~25 MB pipelined reply being "bigger than any kernel socket
buffer" — a guess. Winsock's send/receive autotuning absorbed all of it,
`write_all` completed normally, and the tests saw a healthy client with
omem=0.
2. Pinning the victim's SO_RCVBUF to 8 KiB before connect to clamp the TCP
receive window. The server's write still completed; no timeout fired.
A third guess through CI is not diagnosis, and nobody here has a Windows host
to attach a debugger to. One theory was wrong for a checkable reason and is
recorded so it is not re-tried: `handler_single` (which has no output
accounting) is NOT the Windows path — `main.rs` always routes through
`run_sharded`, so `--shards 1` on Windows uses `handler_sharded`, which has
both the timeout and the accounting.
So the three stall-dependent tests are skipped on Windows honestly rather than
weakened until they pass. The cost is stated plainly in the test file and the
CHANGELOG: the C1 protection is UNVERIFIED on Windows. The code path is shared
and compiles there, but no test proves it fires. Windows is not a target
platform (CLAUDE.md: Linux and macOS).
`output_buffer_limit_refuses_oversized_reply_*` is NOT skipped — it rejects by
size before writing, never depends on a stall, and passes on Windows today.
The SO_RCVBUF pinning is kept even though it did not fix Windows: "the reply is
bigger than any socket buffer" was an unstated assumption on every platform,
and pinning the window makes the premise explicit rather than incidental.
Gates: fmt, clippy --all-targets (-D warnings), write_timeout 5/5 locally.
Refs #20
author: Tin Dang
TinDang97
force-pushed
the
fix/c10k-resource-limits
branch
from
August 6, 2026 14:57
5a2b82e to
4269b87
Compare
TinDang97
added a commit
that referenced
this pull request
Aug 6, 2026
The post-merge main matrix went red on `Check (Windows)` for two brand-new c10k tests while every other platform (macOS, console, Lint, MSRV, memory gate) stayed green. Neither is a defect in the code under test; both are the Windows CI environment, handled two different ways. 1. ACL — `privileged_intercepts_are_acl_gated_multi_shard` read an empty reply for `ACL SETUSER` and panicked "failed: \"\"". The minimal RESP client pumps a fixed 250 ms window per reply; on Windows the server process + TCP stack are scheduled slowly enough that a reply that WAS delivered lands after the window closes. The pump accumulates until its deadline, so widening the window to 1500 ms on Windows only tolerates the lag and changes nothing asserted. Kept RUNNING on every platform — verified green on Windows CI. 2. Idle sweep — `idle_connection_is_closed_at_timeout` is skipped on Windows (`#[cfg(not(windows))]`), not merely re-timed. The sweep closes an idle connection by killing its fd, which relies on `shutdown(2)` unblocking a handler parked in a blocking `read()`; that interruption does not fire on Windows the way it does on Linux/macOS, so the connection is never observed closed (>25 s, three retries). Confirmed a Windows-only gap, not a regression: the test passes on macOS locally in ~3.3 s and on the Linux gate. This matches how #431 documented its stall-dependent write-timeout tests as a Windows gap. The other four tests in the file (park-engages, active-never-closed, blocked-exempt, subscriber-exempt) still run on Windows and keep every shared helper used, so nothing is orphaned. Windows is a documented best-effort platform here. No product code changed; test-only. skip-changelog. Refs: c10k hardening review; post-merge Windows CI on main author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 6, 2026
The post-merge main matrix went red on `Check (Windows)` for two brand-new c10k tests while every other platform (macOS, console, Lint, MSRV, memory gate) stayed green. Neither is a defect in the code under test; both are the Windows CI environment, handled two different ways. 1. ACL — `privileged_intercepts_are_acl_gated_multi_shard` read an empty reply for `ACL SETUSER` and panicked "failed: \"\"". The minimal RESP client pumps a fixed 250 ms window per reply; on Windows the server process + TCP stack are scheduled slowly enough that a reply that WAS delivered lands after the window closes. The pump accumulates until its deadline, so widening the window to 1500 ms on Windows only tolerates the lag and changes nothing asserted. Kept RUNNING on every platform — verified green on Windows CI. 2. Idle sweep — `idle_connection_is_closed_at_timeout` is skipped on Windows (`#[cfg(not(windows))]`), not merely re-timed. The sweep closes an idle connection by killing its fd, which relies on `shutdown(2)` unblocking a handler parked in a blocking `read()`; that interruption does not fire on Windows the way it does on Linux/macOS, so the connection is never observed closed (>25 s, three retries). Confirmed a Windows-only gap, not a regression: the test passes on macOS locally in ~3.3 s and on the Linux gate. This matches how #431 documented its stall-dependent write-timeout tests as a Windows gap. The other four tests in the file (park-engages, active-never-closed, blocked-exempt, subscriber-exempt) still run on Windows and keep every shared helper used, so nothing is orphaned. Windows is a documented best-effort platform here. No product code changed; test-only. skip-changelog. Refs: c10k hardening review; post-merge Windows CI on main author: Tin Dang
2 tasks
TinDang97
added a commit
that referenced
this pull request
Aug 7, 2026
…nner (#440) parked_connection_visible_and_killable asserts that CLIENT KILL of a parked/idle connection removes it from CLIENT LIST. The kill path breaks the handler's pending read via shutdown(2); on Windows that interruption does not fire (the same socket-semantics gap already documented for the idle-timeout close test in #439 and the #431 write-timeout suite), so the registry entry is never released and the victim stays listed — the test fails on every retry on the Windows runner while passing on macOS and the Linux gate in ~14s. Windows is a best-effort platform (documented stance); the behaviour under test is validated on the production platforms. Gate carries the standard documented-gap comment. refs: #439, #431 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.
Stacked on #430 (
fix/c10k-acl-intercept-order) — review that first; this PR's diff only makes sense on top of it.Closes cluster C of the c10k hardening review (
tmp/C10K-HARDENING-REVIEW.md), plus one adjacent memory-observability change that came out of investigating a live instance.C1 — reply writes were unbounded (Critical)
write_allon a socket whose receive window is closed never returns. A client could pipeline a large response and then simply stop reading, parking the handler inside that write while it held the entire serialized reply — the monoio handler coalesces a whole batch into oneBytesbefore its single write syscall, so a deep pipeline is hundreds of MB — plus itsmaxclientsslot, for as long as it liked. N such clients is an OOM that costs the attacker nothing: no reads, no CPU, just a TCP window it refuses to open.It was also invisible:
obl/oll/omemwere hardcoded0informat_client_line, so an operator watching an output-buffer OOM in progress saw every client reporting zero bytes held.--client-write-timeout-ms(default 60s,0= previous wait-forever) bounds every reply-carrying write in all three handlers — monoio top-level, sharded, and the tokioFramedpath — each of which carries an independent copy of the write path.--client-output-buffer-limit-normal(256 MiB) refuses an oversized reply instead of buffering it. This deliberately diverges from Redis, whose normal class defaults to unlimited — which is exactly why the same attack works there. Intended consequence: the cap covers the whole serialized reply, so a single value larger than it is undeliverable, not merely a long pipeline.obl/omem/tot-net-outnow report real held bytes, maintained by the same macro that bounds the write, so every path is covered by construction.The watchdog does not arm on the hot path. A timer per batch flush lands once per command at pipeline depth 1 — the path this project spent a milestone winning against Redis. Only writes ≥256 KiB arm it (
util::arm_write_timeout); a reply that fits in the socket buffer cannot block, so it buys nothing there.I could not measure the timer cost honestly. On the 2-core
moon-devVM the same-binary A/B returned +26.7% at P=16 for the configuration doing strictly more work, with raw samples spanning 1.4–1.7× within a single config. Rather than quote a number the measurement does not support, the cost is removed by construction: the hot path now pays one integer compare.Residual, stated rather than hidden: a small write to a genuinely wedged socket is still unbounded. It holds ≤256 KiB rather than hundreds of MB, but keeps its
maxclientsslot. That connection is now visible (omem> 0) and killable (CLIENT KILLshuts the fd down, which makes the blocked write return) — neither was true before.No pub/sub-class knob ships: moon's only subscriber write is already hard-capped at 64 KiB by
MAX_COALESCE_BYTES, so such a knob could never fire, and a knob that cannot fire reads as protection. The real pub/sub backpressure is the 4096-slot bounded channel, which bounds message count, not bytes — recorded as a follow-up.C2 — unbounded query buffer, reachable pre-auth
read_bufgrew to whatever a frame header declared, ceilinged only by the parser's 512 MiB bulk limit which it accepts.$536870911plus a dribble pinned half a gigabyte per connection, outsideused_memoryso eviction never fired, and the auth gate runs after parsing so it cost no credentials. Measured pre-fix: one unauthenticated socket took RSS from 4 MB to 208 MB, never closed, nothing logged.--client-query-buffer-limit(1 GiB) and--client-query-buffer-limit-preauth(64 KiB). Enforcement lives inRespCodec::decodefor theFramedpath, becauseFramed::next()loops read→decode internally and never yields while a frame is incomplete — a handler-side check would not run during the growth that matters.The test for this was wrong and passing. It asserted
sent <= 1 MiB, wheresentcounts bytes the kernel accepted — client sndbuf, in-flight window, server rcvbuf — not bytes moon buffered. It passed on macOS and failed on Linux at 2.5 MiB while the server'sread_bufnever exceeded 64 KiB. Replaced with a differential control: the same dribble against an instance with the ceilings disabled must not be closed. Socket buffering is identical on both legs, so it cancels out.C3 — maxclients rejection could block forever
All three rejection sites wrote
-ERR max number of clients reachedwith an unboundedwrite_all, on a socket that is about to be dropped, and logged one warning per rejection (a connection storm is also a log storm). Bounded to 2s and rate-limited to one warn/sec with a suppressed count.Adjacent: allocator observability
From investigating a live instance reporting
used_memory2.43 GB while the OS charged it 7.3 GB, 7.2 GB of it swapped — with nothing inINFO memoryable to say which part was fragmentation and which was live memoryused_memorydoes not charge for.--features jemalloc-stats(off by default) adds Redis'sallocator_*fields. Absent rather than zero-filled in a default build: a zero reads as "no fragmentation", which is worse than "not measured".--memory-decay-interval-ms(default 0) exists because jemalloc'sbackground_threadis genuinely compiled out on Apple platforms (JEMALLOC_BACKGROUND_THREADis only defined whenabi != macho). It ships off and is explicitly not claimed as a fix: the hypothesis that an idle jemalloc never purges on macOS was tested and disproved — 384 MiB churned and freed reclaims to a 3.7 MiB physical footprint with no decay call at all.One new
unsafeblock, explicitly approved.arena.<i>.decayisNEITHER_READ_NOR_WRITEin jemalloc'sctl.c, so every pointer is NULL andnewlenis 0 — no buffer is read or written. A unit test asserts the linked jemalloc still accepts the ctl, so it cannot rot into a silent no-op.tests/allocator_idle_decay.rsrecords two measurement traps that each produced a confident wrong answer first:ps -o rssis meaningless on macOS (Darwin jemalloc purges withMADV_FREE, so pages stay resident but discardable — RSS reads 387 MiB where the footprint reads 3.7 MiB), and the instrument must be validated before it is trusted.Testing
write_timeout,query_buffer_limit,allocator_idle_decay,util::write_timeout_gate_tests,memory_ctl.cargo fmt --check+ clippy clean under default,jemalloc-stats, andruntime-tokio,jemalloc.--no-fail-fast): 180 suites pass. Four suites failed under parallel load and all four pass in isolation; two failed at spawn (child exited before accepting), and different suites failed across two runs — contention, not a regression.parked_connection_visible_and_killable(force_close_fdis#[cfg(unix)]).Two bugs found, deliberately NOT fixed here
INFO persistencereportsaof_enabled:0unconditionally — hardcoded literal atsrc/command/connection.rs:242, whileappendonlydefaults to"yes".auto-aof-rewrite-percentageis unimplemented andBGREWRITEAOFis gated off in per-shard mode (main.rs:971). Observed ~1 GB/day with no mechanism that can shrink it.Both are out of scope for cluster C; filing separately.
CI
ci.ymltriggers onbranches: [main]only, so this stacked PR gets no matrix. Dispatched manually — run IDs in a comment below.