0.3.67: gate the diagnostics route (#73), fix our shared mallinfo2 test, and the keepcost correction - #76
Open
emooreatx wants to merge 2 commits into
Open
0.3.67: gate the diagnostics route (#73), fix our shared mallinfo2 test, and the keepcost correction#76emooreatx wants to merge 2 commits into
emooreatx wants to merge 2 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…t in the code In 0.3.63 I wrote, in `diag.rs` and in that commit message, that `malloc_trim` "is not the alternative it appears to be: keepcost measured 104KB, so a trim had roughly nothing at the top of the heap to hand back." That reasoning is wrong. `keepcost` is the releasable space at the top of the MAIN ARENA, and since glibc 2.8 `malloc_trim` does not stop there: `mtrim` walks every arena's free bins and `MADV_DONTNEED`s whole free pages inside them. So it reaches exactly the fragmented free lists `keepcost` says nothing about — which on this node still hold ~591MB WITH the arena cap applied. Eric caught it from the other direction while measuring ciris-server, where `keepcost` is 3.86MB against `fordblks` of ~888MB. Same instrument, opposite conclusion, and the error was mine on both nodes: I ruled a lever out on a number that measures something else. What changes: - The comment on `keepcost` in the report now says what it actually bounds, and names this mistake so the next reader does not repeat it from the same field. - The arena-cap doc no longer claims trim was ruled out. Trim is UNTESTED here, which is a different statement, and it carries the reason it might not be free: the pages it returns fault back in on reuse, and this node's problem is churn. - `status.malloc_trim_secs` (default 0, off) makes it testable. Without gdb on the host there is no way to trigger a trim in a running process, so the correction stays theoretical unless the binary can be asked. Enabling it logs `fordblks` and `RssAnon` either side, so it is an A/B like the cap got — and the two disagreeing is itself the finding, since madvised pages leave RSS while freed-but-untrimmed ones do not. Not enabled anywhere. The cap was adopted on 205 minutes of measurement; this gets the same standard or it stays off. 115 tests (1 new). The new one initially asserted `rc` against itself — a tautology that could not fail — and now checks it is one of glibc's two real return values. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012B5ebRpgmkqskVYLZ7DH67
…r shared test Three things, all in `diag.rs` and its route, all mine. **CIRISStatus#73 — the memory route answered unauthenticated on the published port.** ciris-server gates the identical report on the same host behind `CIRIS_DIAGNOSTICS` and binds it to loopback, so the two nodes disagreed about whether allocator internals are public. They share a host and, deliberately, one instrument; they should share the switch. `ciris_server::diag::enabled()` IS that switch, read from the same process, so one `CIRIS_DIAGNOSTICS=1` turns both on and nothing here invents a second control to drift from it. Off, the route is not mounted at all — a 404 indistinguishable from any absent path, rather than a 403 advertising that there is something to ask for. What this does NOT do is bind to loopback, and the comment says so at the call site. The server pairs its gate with `require_loopback`; an adapter cannot, because that guard is not exported and the read-API listener does not hand us `ConnectInfo`. With diagnostics on, the route is reachable from wherever the port is, and the edge stays load-bearing. "Gated" and "gated and loopback-only" are different promises and only one of them is being made here. **The flaky test, fixed the way the server fixed it in 0.5.200.** Our copy still asserted on `uordblks` alone against a 32MB block. Two glibc facts break that: a block past the mmap threshold is not in `uordblks` at all (it is mmapped, and lands in `hblkhd`), and in a test binary this size other threads free arena memory in the same millisecond, so `uordblks` can FALL while this thread holds its allocation. Now 64MiB — past the 32MiB ceiling of the dynamic mmap threshold, hence always mmapped — measured as `uordblks + hblkhd`, with half the block as slack. Taken from ciris-server rather than re-derived: the two modules are one instrument on purpose, and a test that is flaky in one is flaky in both. **The `keepcost` correction** this branch opened with, unchanged: `keepcost` bounds the top of the main arena, not what `malloc_trim` returns, because since glibc 2.8 `mtrim` madvises free pages inside every arena. Eric's measurement settled it in the meantime — 3.9KB of keepcost predicted, 172MB delivered, with `fordblks` and `arena` not moving at all. The win is residency, exactly as the corrected comment says, and `status.malloc_trim_secs` (default off) is how it gets tested rather than assumed. Rebased onto 0.3.66; the version this branch opened with (0.3.64) was taken by a repin while it sat open. 115 tests, fmt, clippy -D warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
emooreatx
force-pushed
the
fix/trim-claim-was-wrong
branch
from
September 8, 2026 13:12
54d360d to
35caeb6
Compare
emooreatx
changed the title
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.
Rebased onto 0.3.66 and grown two fixes, all in
diag.rsand its route, all mine.CIRISStatus#73 — the memory route answered unauthenticated on the published port
ciris-server gates the identical report on the same host behind
CIRIS_DIAGNOSTICSand binds it to loopback, so the two nodes disagreed about whether allocator internals are public. They share a host and — deliberately — one instrument; they should share the switch.ciris_server::diag::enabled()is that switch, read from the same process, so oneCIRIS_DIAGNOSTICS=1turns both on and nothing here invents a second control to drift from it. Off, the route isn't mounted at all: a 404 indistinguishable from any absent path, rather than a 403 advertising that there's something to ask for.What this does not do is bind to loopback, and the comment says so at the call site. The server pairs its gate with
require_loopback; an adapter can't — that guard isn't exported and the read-API listener doesn't hand usConnectInfo. With diagnostics on, the route is reachable from wherever the port is, and the edge stays load-bearing. "Gated" and "gated and loopback-only" are different promises, and only one of them is being made here.Open question for the server: would you export the loopback guard (or serve adapter routers with
ConnectInfo) so an adapter can match the full contract rather than half of it?The flaky mallinfo2 test, fixed the way the server fixed it in 0.5.200
Our copy still asserted on
uordblksalone against a 32 MB block. Two glibc facts break that: a block past the mmap threshold isn't inuordblksat all (it's mmapped, and lands inhblkhd), and in a test binary this size other threads free arena memory in the same millisecond, souordblkscan fall while this thread holds its allocation.Now 64 MiB — past the 32 MiB ceiling of the dynamic mmap threshold, hence always mmapped — measured as
uordblks + hblkhd, with half the block as slack. Taken from ciris-server rather than re-derived: the two modules are one instrument on purpose, and a test that's flaky in one is flaky in both.The
keepcostcorrection this branch opened withUnchanged:
keepcostbounds the top of the main arena, not whatmalloc_trimreturns, because since glibc 2.8mtrimmadvises free pages inside every arena. Eric's measurement settled it while this sat open — 3.9 KB of keepcost predicted, 172 MB delivered, withfordblksandarenanot moving at all. The win is residency, exactly as the corrected comment says, andstatus.malloc_trim_secs(default off) is how it gets tested rather than assumed.115 tests, fmt, clippy
-D warnings. Version bumped to 0.3.67 — the 0.3.64 this branch opened with was taken by a repin while it sat open.🤖 Generated with Claude Code