Skip to content

privacy: raise the classify/redact body cap to 1 MB - #1013

Open
lloydmak99 wants to merge 1 commit into
fix/privacy-classify-status-codesfrom
fix/privacy-body-cap
Open

privacy: raise the classify/redact body cap to 1 MB#1013
lloydmak99 wants to merge 1 commit into
fix/privacy-classify-status-codesfrom
fix/privacy-body-cap

Conversation

@lloydmak99

@lloydmak99 lloydmak99 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Stacked on #1012base 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 models table's context_length: 512 — the constant's own comment read "Privacy classify input is text only, 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:

text density tokens admitted
sparse English (7.26 B/token, measured) ~36,000
typical English (~4 B/token) ~65,000
dense / code-like (~3 B/token) ~87,000

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

Corrected after review. The original version of this section made two claims that do not survive tracing through the code. Both are corrected below, with thanks to the reviewers who caught them.

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; on text_inference_routes that is body_hash_middleware (lib.rs:1629), which runs before auth and calls body.collect() with no limit. DefaultBodyLimit only inserts an extension consulted at Bytes extraction 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's body_bytes.clone() is a refcounted Bytes clone and nearly free, but /privacy/redact materializes the input several more times, concurrently: the Bytes arg, parsed.input, texts: Vec<String>, haystack = texts.join(..), re-serialized upstream_bytes, and the redacted vec 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:

  • This is still a tightening. These routes sit on a router whose default is already 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).
  • Timeout. DEFAULT_COMPLETION_TIMEOUT_SECS = 600; measured latency was 1.1–5.2s for inputs up to 34k tokens.

Addressed from review

  • Body limit now enforced before hashing. body_hash_middleware honours the route's configured DefaultBodyLimit rather than collecting unbounded, so the 1 MB cap actually bounds memory on this path.
  • /privacy/redact span application moved off the async runtime. apply_detected_spans is O(U·L + U²) in unique PII values and input length, with no .await in the loop — a ~16x worst-case CPU increase with this cap change, blocking a tokio worker throughout. Now runs under spawn_blocking.
  • Just-under-limit tests assert the expected success status rather than only != 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.

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
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s).

@ironloopai

ironloopai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review · Status

🟩 Completed

IronLoop completed the review and posted it to GitHub.

Result

Open submitted review →

Run details
  • Run: 2b6eeb49-1100-41d8-baa1-ee01f1803418
  • Base: fix/privacy-classify-status-codes at b6491b7
  • Head: fix/privacy-body-cap at 7a1cca9
  • Created: 2026-09-03 16:36 UTC
  • Updated: 2026-09-03 16:42 UTC

Automatic trigger · attempt 1 of 3 · completed in 6m 4s

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review — privacy: raise the classify/redact body cap to 1 MB

The constant change itself is sound, the tests genuinely demonstrate the cap moved (not just renamed), and the deploy-order note re: nearai/cvm-compose-files#223 is the right call. No existing review threads on this PR to build on, so this is a first pass.

Two things in the Risk section don't hold up when traced through the code, and one of them is a real regression surface for /privacy/redact specifically.


⚠️ 1. /privacy/redact does super-linear, CPU-bound span application on the async runtime — this scales ~16x with the cap raise

privacy_redact calls apply_detected_spans directly on the request task (crates/api/src/routes/completions.rs:6766), with no spawn_blocking. The inline comment there says it "only does in-process work (JSON parse + UTF-8 boundary checks)" — that understates it.

The actual cost is in redact_one (crates/services/src/auto_redact/apply.rs:213):

let dummy = map.lookup_or_mint(&span.category, original, |c| haystack.contains(c));

and in lookup_or_mint (crates/services/src/auto_redact/placeholders.rs:106-119), per unique (category, original):

  • would_collide(&candidate)haystack.contains(c), a full O(L) scan of the concatenated input (haystack = texts.join(...), i.e. the whole 1 MB body),
  • self.entries.iter().any(|(d, _)| d == &candidate) → O(U) string compares,
  • self.entries.insert(pos, ...) → O(U) memmove.

Total ≈ O(U·L + U²), where U = unique PII values and L = input length. Both U and L scale with the cap, so 256 KB → 1 MB is roughly a 16x worst-case CPU increase, not 4x. detect::parse_response (detect.rs:123-136) puts no bound on span count, so U is provider-controlled.

Failure scenario — the documented use case ("one-shot sanitizer ahead of calling a third-party LLM"): a client posts a 1 MB contacts/log dump, ~15k rows × 3 PII fields ≈ 45k unique spans.

  • haystack.contains work: 45,000 × 1,048,576 ≈ 4.7×10¹⁰ bytes scanned → seconds of pure CPU even at memmem throughput.
  • entries scan + insert: ≈ 10⁹ additional ops.
  • There is no .await anywhere in that loop, so a tokio worker thread is blocked for the whole duration. With the 64-concurrent-per-org cap, a handful of such requests can starve workers and stall unrelated in-flight SSE streams on the same threads. DEFAULT_COMPLETION_TIMEOUT_SECS doesn't help — this is after the provider call returns.

At 256 KB this was latent; at 1 MB it's reachable. Suggested fixes, in order of effort:

// completions.rs — get it off the runtime
let redacted = match tokio::task::spawn_blocking(move || {
    services::auto_redact::apply_detected_spans(&texts, &response_bytes)
}).await { ... };
// placeholders.rs — kill the O(U²) term
minted: HashSet<String>,   // replaces entries.iter().any(...)

The O(U·L) haystack term is the bigger one: every minted dummy has a fixed prefix (redacted, redacted_secret_, redacted_pii_, +1-555-01, 000-00-, Redacted, … Redacted Way), so a single up-front pass over the haystack collecting which dummy-shaped strings actually occur would replace U scans with 1.

Alternatively: give /privacy/redact its own, lower cap. /privacy/classify is a pure passthrough and carries none of this cost — only redact does, so a shared constant couples them unnecessarily.

⚠️ 2. The "~2 copies per in-flight request" memory figure is ~3x low for /privacy/redact

body_hash_middleware's body_bytes.clone() is a refcounted Bytes clone, so that pair is nearly free. But the redact handler materializes the input several more times, all live simultaneously:

# 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.

⚠️

@ironloopai ironloopai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread crates/api/src/lib.rs
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant