privacy: raise the classify/redact body cap to 1 MB - #1013
Conversation
The 256 KB per-route cap was sized from the same wrong belief as the models table's `context_length: 512` — the constant's own comment said "model context is small (e.g. 512 tokens)". openai/privacy-filter advertises a 128,000-token context window. Measured against the live endpoint, 256 KB admits roughly: ~36,000 tokens sparse English (7.26 bytes/token, measured) ~65,000 tokens typical English (~4 bytes/token) ~87,000 tokens dense/code-like text (~3 bytes/token) So the cap, not the model, was bounding input. 128,000 tokens is ~512 KB of typical English and ~930 KB of sparse text, so 1 MB covers the real context window with headroom for JSON escaping. Risk is bounded. Auth rejects before the size limit is evaluated — verified live: 300 KB with an invalid key returns 401, the same body with a valid key returns 413 — so only authenticated, rate-limited, credit-checked callers reach the size path. These routes also sit on a router whose default limit is already 25 MB for audio transcription; at 1 MB, privacy/classify stays 25x tighter than its siblings. The two oversized-payload tests built a 300 KB body, which is now UNDER the cap and would have asserted nothing. They are raised past 1 MB, and a just-under-the-limit case is added to each so the cap is shown to have moved rather than merely been renamed. Refs #987
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s). |
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 6m 4s |
Review —
|
| # | allocation | site |
|---|---|---|
| 1 | body: Bytes |
handler arg |
| 2 | parsed.input (serde_json::Value) — not dropped, lives to end of fn |
completions.rs:6523 |
| 3 | texts: Vec<String> (s.clone() / s.to_string()) |
completions.rs:6541-6548 |
| 4 | haystack = texts.join(...) |
auto_redact/mod.rs:177 |
| 5 | upstream_bytes (re-serialized, plus JSON escaping) |
completions.rs:6666 |
| 6 | redacted: Vec<String> + final response JSON |
completions.rs:6766 |
That's ~6 concurrent 1 MB allocations per in-flight redact, so ~400 MB/org at 64 concurrent, not 128 MB — and this is a shared multi-tenant TEE, so it's N orgs × that. Still bounded, but worth restating in the PR body with the right number since the 128 MB figure is what the merge decision rests on.
ℹ️ 3. The unauthenticated-surface argument is right in conclusion but wrong in mechanism (pre-existing — not a blocker for this diff)
Auth rejects before the size limit is evaluated. […] Only authenticated, rate-limited, credit-checked callers reach the size path.
Axum applies layers so the last .layer() is outermost. On text_inference_routes that's from_fn(middleware::body_hash_middleware) (crates/api/src/lib.rs:1629) — it runs before auth, and body.collect() (middleware/body_hash.rs:37) has no limit. DefaultBodyLimit only inserts an extension; enforcement happens at Bytes extraction inside the handler, after the body is already fully buffered.
So the 401 you observed came after the 300 KB was buffered. Your conclusion still holds — this PR does not widen anything, because the per-route cap never gated pre-auth buffering in the first place. But the reason is "already unbounded, unchanged here," not "auth gates it." (The repo half-knows this: auto_redact_adversarial.rs:474 notes "middleware ordering can produce 400 instead," and the comment at lib.rs:1633-1634 claiming body_hash sits after auth on the file routes looks incorrect for the same reason.) Worth a separate issue.
Nit — the just-under tests
assert_ne!(status, 413) passes on a 500/502 from the mock too. It does prove the cap moved (1 MB would have been a hard 413 at 256 KB), so it's not wrong — but asserting the expected success status would make it a stronger guard. Envelope math checks out: 1,047,552 + ~45 bytes leaves ~979 bytes of headroom under 1,048,576.
Verdict: #1 is the one I'd want addressed before merge, or at least consciously accepted — spawn_blocking alone is a two-line change that removes the runtime-starvation half of the risk. #2 is a correction to the PR body. #3 is pre-existing and belongs in its own issue.
There was a problem hiding this comment.
Review · Summary
Found one request-size enforcement issue in the expanded privacy endpoints.
Findings: 🔴 High 1
Code-specific findings are attached to the diff.
Validation
- ✅ Privacy request-path inspection — Reviewed the updated route configuration, body-hash middleware, both handlers, and the changed boundary tests.
Review details
- Run:
2b6eeb49-1100-41d8-baa1-ee01f1803418 - Attempts: 1
| const PRIVACY_CLASSIFY_MAX_BODY_SIZE: usize = 256 * 1024; // 256 KB | ||
| // The privacy model's context window is 128k tokens. A 1 MB cap covers it for | ||
| // typical text while keeping this route well below the 25 MB router default. | ||
| const PRIVACY_CLASSIFY_MAX_BODY_SIZE: usize = 1024 * 1024; // 1 MB |
There was a problem hiding this comment.
🔴 High · Enforce the body limit before hashing
DefaultBodyLimit constrains the later Bytes extractor, not direct body reads. Both privacy routes pass through body_hash_middleware, which calls body.collect() before that extractor runs, so an oversized (including chunked) request is fully buffered and hashed before it becomes a 413. The new 1 MiB cap therefore does not bound memory consumed on this path. Apply a streaming request-body limit or bounded collection around the hashing step while preserving the intended authentication order.
Stacked on #1012 — base is
fix/privacy-classify-status-codes, so merge that first. Both branches touch the same test files; stacking avoids a conflict.Part of #987.
Why
The 256 KB per-route cap was sized from the same wrong belief as the
modelstable'scontext_length: 512— the constant's own comment read "Privacy classify input is text only, model context is small (e.g. 512 tokens)."openai/privacy-filteradvertises a 128,000-token context window.Measured against the live endpoint, 256 KB admits roughly:
So the cap, not the model, was bounding input. 128,000 tokens is ~512 KB of typical English and ~930 KB of sparse text, so 1 MB covers the real context window with headroom for JSON escaping.
Risk
Auth rejects before the size limit is evaluated.This was wrong in mechanism, though the conclusion stands. Axum applies layers so the last.layer()is outermost; ontext_inference_routesthat isbody_hash_middleware(lib.rs:1629), which runs before auth and callsbody.collect()with no limit.DefaultBodyLimitonly inserts an extension consulted atBytesextraction inside the handler — after buffering. So the 401 I observed for a 300 KB request with an invalid key came after those 300 KB were buffered.The conclusion that this PR does not widen the unauthenticated surface still holds — but because that path was already unbounded and is unchanged here, not because auth gates it.
Memory is higher than first stated.
body_hash_middleware'sbody_bytes.clone()is a refcountedBytesclone and nearly free, but/privacy/redactmaterializes the input several more times, concurrently: theBytesarg,parsed.input,texts: Vec<String>,haystack = texts.join(..), re-serializedupstream_bytes, and theredactedvec plus response JSON. That is roughly 6 concurrent 1 MB allocations per in-flight redact, so ~400 MB per org at the 64-concurrent cap — not the ~128 MB originally stated. Bounded, but this is a shared multi-tenant TEE, so it is N orgs × that.What does hold unchanged:
AUDIO_TRANSCRIPTION_MAX_BODY_SIZE = 25 MB; the per-route layer overrides it downward. At 1 MB, privacy/classify remains 25x tighter than its siblings (/audio/transcriptions,/embeddings,/rerank,/score).DEFAULT_COMPLETION_TIMEOUT_SECS = 600; measured latency was 1.1–5.2s for inputs up to 34k tokens.Addressed from review
body_hash_middlewarehonours the route's configuredDefaultBodyLimitrather than collecting unbounded, so the 1 MB cap actually bounds memory on this path./privacy/redactspan application moved off the async runtime.apply_detected_spansis O(U·L + U²) in unique PII values and input length, with no.awaitin the loop — a ~16x worst-case CPU increase with this cap change, blocking a tokio worker throughout. Now runs underspawn_blocking.!= 413.Tests
The two oversized-payload tests built a 300 KB body — now under the cap, so they would have asserted nothing. Raised past 1 MB, and a just-under-the-limit case added to each so the cap is shown to have moved rather than merely been renamed.
Deploy order
Do not deploy this ahead of nearai/cvm-compose-files#223. Until that lands, the backend still chunks at
PRIVACY_MAX_LENGTH=4096, so a 1 MB input becomes ~32 sequential chunks down the path that previously hard-exited on CUDA OOM. After #223 it is a single ~15 GB pass on a dedicated 141 GB H200.