Skip to content

Commit bcad4e3

Browse files
fix(pii): mask offloaded large payloads chunk-by-chunk instead of aborting at 16MB (#5810)
* fix(pii): mask offloaded large payloads chunk-by-chunk and retry transient mask failures A block output past the 16MB inline materialization ceiling aborted the run before masking even started: the redaction path hydrated the whole offloaded value at once, and the pre-flight size assert fired on the manifest's total byteSize. Large-array manifests now page one stored chunk at a time (materialize -> mask -> re-store, rebuilt via the manifest writer with preview derived from masked items), so peak heap stays ~one chunk regardless of payload size. Single refs up to the 64MB durable cap hydrate with a raised budget and run serially outside the concurrency pool. Mask-batch chunk requests now retry transient failures (network errors, 408/429/5xx, honoring Retry-After) with jittered backoff, so a single ALB blip or Presidio pod restart no longer fails a whole payload's redaction. Nested-ref masking now runs the string pass before ref substitution, fixing a latent double-mask when a masked nested value shrinks back inline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A1JYstmLHk9qMGyBDqYRcJ * fix(pii): retry runtime timeouts and socket closes in mask-batch chunks Verified end-to-end against a 26MB / 40k-record offloaded output: the chunk-wise path masks it in ~54s on a single local Presidio worker where the old path aborted at the 16MB ceiling. The exercise surfaced two more transient error shapes the retry classifier missed — runtime-level request timeouts (undici's default 300s headers timeout, Bun's TimeoutError) and mid-flight socket closes — both of which previously failed the whole payload's redaction on the first occurrence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A1JYstmLHk9qMGyBDqYRcJ * fix(pii): gate the spaCy fast path on entities the loaded models can produce The registry's SpacyRecognizers claim every entity in Presidio's default NER-model mapping — including PHONE_NUMBER/AGE/ID/EMAIL, which exist for transformer de-identification backends and which no spaCy model can emit. The NER_ENTITIES derivation trusted that claim, so any request naming PHONE_NUMBER (present in nearly every redaction rule) silently forced the full spaCy pass and the regex-only fast path never fired. Intersect the claimed set with the entities the loaded models' actual NER labels map onto; the hard floor of core NER entities is unchanged, and a future backend that genuinely emits phone labels would re-gate automatically. Verified live: PHONE_NUMBER-only requests take nlp=skip with span parity against the full path, PERSON still forces NER, and a 26MB/40k-record block-output redaction with the realistic entity set runs entirely on the fast path (~3.2min vs ~15min projected full-NER on one worker). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A1JYstmLHk9qMGyBDqYRcJ * fix(pii): hydrate oversized-chunk manifests serially, not just single refs Review finding: a manifest whose packer emitted a chunk past the inline ceiling (one item larger than the chunk target) hydrates that chunk with the raised 64MB budget inside the REF_CONCURRENCY pool, so several such manifests could hydrate oversized blobs concurrently — the exact heap scenario the serial path exists to prevent. The serial gate now covers any ref whose hydration can exceed the inline ceiling: oversized single refs and manifests containing an oversized chunk. Normally-chunked manifests stay pooled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A1JYstmLHk9qMGyBDqYRcJ * fix(pii): serialize oversized hydrations globally across nested redaction passes Review finding: the serial gate was per-resolveReplacements invocation, so nested oversized refs discovered inside different pooled parents each got their own pool and could hydrate oversized blobs concurrently. A shared promise-chain gate now threads through the options from the entry points, and a reentrancy flag lets a gated ref's own nested oversized work run directly instead of deadlocking on the hold. Covered by a cross-parent max-in-flight assertion and a nested-oversized deadlock regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A1JYstmLHk9qMGyBDqYRcJ * fix(pii): fail fast on a null mask-batch body; use sleep() in gate test A 200 response with a null JSON body threw TypeError on the data.masked read, which the retry classifier treats as transient — burning the full retry budget on a deterministic shape failure. Null-guard the body so it throws the non-retryable shape error immediately. Also swap the gate test's inline setTimeout promise for sleep() to satisfy check:utils, which failed CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A1JYstmLHk9qMGyBDqYRcJ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 7ee8f46 commit bcad4e3

6 files changed

Lines changed: 667 additions & 75 deletions

File tree

apps/pii/server.py

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -176,23 +176,50 @@ def build_analyzer() -> AnalyzerEngine:
176176
batch_analyzer = BatchAnalyzerEngine(analyzer_engine=analyzer)
177177
anonymizer = AnonymizerEngine()
178178

179-
# Every entity the spaCy NER recognizers can produce. A request touching any of
180-
# these must run spaCy; a request naming only non-NER (regex/checksum) entities can
181-
# skip it. Derived from the live registry so it stays authoritative if Presidio's
182-
# default entity set changes (e.g. ORGANIZATION), unioned with a known floor so an
183-
# unexpectedly empty derivation can never let an NER request skip the NLP pass.
179+
# Every entity the spaCy NER recognizers can actually produce. A request touching
180+
# any of these must run spaCy; a request naming only non-NER (regex/checksum)
181+
# entities can skip it. The registry's SpacyRecognizers CLAIM every entity in
182+
# Presidio's default NER-model mapping — including PHONE_NUMBER/AGE/ID/EMAIL,
183+
# which exist for transformer models and which no spaCy model can emit — so the
184+
# claimed set is intersected with the entities the loaded models' NER labels map
185+
# onto. Without that filter, selecting PHONE_NUMBER (present in nearly every
186+
# redaction rule) silently forces the full spaCy pass and the regex-only fast
187+
# path never fires. The floor guarantees the core NER entities always take the
188+
# full pass even if label introspection ever derives empty.
184189
_SPACY_NER_FLOOR = frozenset({"PERSON", "LOCATION", "NRP", "DATE_TIME", "ORGANIZATION"})
185-
NER_ENTITIES = _SPACY_NER_FLOOR | frozenset(
186-
entity
187-
for recognizer in analyzer.registry.recognizers
188-
if isinstance(recognizer, SpacyRecognizer)
189-
for entity in recognizer.supported_entities
190+
191+
192+
def _producible_ner_entities() -> frozenset:
193+
"""Presidio entities some loaded spaCy model's NER labels map onto."""
194+
configuration = getattr(analyzer.nlp_engine, "ner_model_configuration", None)
195+
mapping = configuration.model_to_presidio_entity_mapping if configuration else {}
196+
producible = set()
197+
for nlp in getattr(analyzer.nlp_engine, "nlp", {}).values():
198+
if "ner" not in nlp.pipe_names:
199+
continue
200+
for label in nlp.get_pipe("ner").labels:
201+
entity = mapping.get(label)
202+
if entity:
203+
producible.add(entity)
204+
return frozenset(producible)
205+
206+
207+
NER_ENTITIES = _SPACY_NER_FLOOR | (
208+
frozenset(
209+
entity
210+
for recognizer in analyzer.registry.recognizers
211+
if isinstance(recognizer, SpacyRecognizer)
212+
for entity in recognizer.supported_entities
213+
)
214+
& _producible_ner_entities()
190215
)
191216

192217
# One blank NlpArtifacts per language, built once at startup. Passing these to
193218
# analyze() skips nlp_engine.process_text (the spaCy tok2vec+ner pass) entirely:
194-
# the pattern recognizers still match on the raw text, SpacyRecognizer is excluded
195-
# by the entity filter, and score_threshold is unset so detection is identical.
219+
# the pattern recognizers still match on the raw text, SpacyRecognizer finds
220+
# nothing in blank artifacts (and can only be consulted at all for claimed-but-
221+
# unproducible entities like PHONE_NUMBER), and score_threshold is unset so
222+
# detection is identical.
196223
# Only context-based score boosting (which needs real tokens) is unavailable — an
197224
# accepted trade for skipping NER on the hot block-output path. Read-only, so it
198225
# is safe to share across requests and workers.

apps/sim/lib/guardrails/mask-client.test.ts

Lines changed: 76 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,15 @@
33
*/
44
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
55

6-
const { mockToken, mockBaseUrl } = vi.hoisted(() => ({
6+
const { mockToken, mockBaseUrl, mockSleep } = vi.hoisted(() => ({
77
mockToken: vi.fn(),
88
mockBaseUrl: vi.fn(),
9+
mockSleep: vi.fn(),
910
}))
1011

1112
vi.mock('@/lib/auth/internal', () => ({ generateInternalToken: mockToken }))
1213
vi.mock('@/lib/core/utils/urls', () => ({ getInternalApiBaseUrl: mockBaseUrl }))
14+
vi.mock('@sim/utils/helpers', () => ({ sleep: mockSleep }))
1315

1416
import { maskPIIBatchViaHttp } from '@/lib/guardrails/mask-client'
1517

@@ -20,6 +22,7 @@ describe('maskPIIBatchViaHttp', () => {
2022
vi.clearAllMocks()
2123
mockToken.mockResolvedValue('tok')
2224
mockBaseUrl.mockReturnValue('http://app.internal:3000')
25+
mockSleep.mockResolvedValue(undefined)
2326
fetchMock = vi.fn(async (_url: string, init: { body: string }) => {
2427
const { texts } = JSON.parse(init.body) as { texts: string[] }
2528
return new Response(JSON.stringify({ masked: texts.map((t) => `M(${t})`) }), {
@@ -52,10 +55,80 @@ describe('maskPIIBatchViaHttp', () => {
5255
expect(fetchMock).toHaveBeenCalledTimes(3) // 2000-per-request cap
5356
})
5457

55-
it('throws on a non-2xx response so the caller can scrub', async () => {
56-
fetchMock.mockResolvedValueOnce(new Response('boom', { status: 500 }))
58+
it('throws immediately on a deterministic 4xx without retrying', async () => {
59+
fetchMock.mockResolvedValueOnce(new Response('bad request', { status: 400 }))
5760

5861
await expect(maskPIIBatchViaHttp(['a'], [])).rejects.toThrow(/mask-batch request failed/)
62+
expect(fetchMock).toHaveBeenCalledTimes(1)
63+
expect(mockSleep).not.toHaveBeenCalled()
64+
})
65+
66+
it('retries a transient 5xx with backoff and then succeeds', async () => {
67+
fetchMock.mockResolvedValueOnce(new Response('deploying', { status: 503 }))
68+
69+
const out = await maskPIIBatchViaHttp(['a'], [])
70+
71+
expect(out).toEqual(['M(a)'])
72+
expect(fetchMock).toHaveBeenCalledTimes(2)
73+
expect(mockSleep).toHaveBeenCalledTimes(1)
74+
})
75+
76+
it('retries a rejected fetch (network error) and then succeeds', async () => {
77+
fetchMock.mockRejectedValueOnce(new TypeError('fetch failed'))
78+
79+
const out = await maskPIIBatchViaHttp(['a'], [])
80+
81+
expect(out).toEqual(['M(a)'])
82+
expect(fetchMock).toHaveBeenCalledTimes(2)
83+
})
84+
85+
it('retries a runtime-level request timeout and then succeeds', async () => {
86+
const timeout = new Error('The operation timed out.')
87+
timeout.name = 'TimeoutError'
88+
fetchMock.mockRejectedValueOnce(timeout)
89+
90+
const out = await maskPIIBatchViaHttp(['a'], [])
91+
92+
expect(out).toEqual(['M(a)'])
93+
expect(fetchMock).toHaveBeenCalledTimes(2)
94+
})
95+
96+
it('gives up after the retry budget is exhausted on a persistent 5xx', async () => {
97+
fetchMock.mockImplementation(async () => new Response('down', { status: 503 }))
98+
99+
await expect(maskPIIBatchViaHttp(['a'], [])).rejects.toThrow(/mask-batch request failed/)
100+
expect(fetchMock).toHaveBeenCalledTimes(8)
101+
expect(mockSleep).toHaveBeenCalledTimes(7)
102+
})
103+
104+
it('mints a fresh internal token per attempt', async () => {
105+
fetchMock.mockResolvedValueOnce(new Response('deploying', { status: 503 }))
106+
107+
await maskPIIBatchViaHttp(['a'], [])
108+
109+
expect(mockToken).toHaveBeenCalledTimes(2)
110+
})
111+
112+
it('does not retry a null 200 body (deterministic, not a transient TypeError)', async () => {
113+
fetchMock.mockResolvedValueOnce(
114+
new Response('null', { status: 200, headers: { 'content-type': 'application/json' } })
115+
)
116+
117+
await expect(maskPIIBatchViaHttp(['a'], [])).rejects.toThrow(/unexpected result/)
118+
expect(fetchMock).toHaveBeenCalledTimes(1)
119+
expect(mockSleep).not.toHaveBeenCalled()
120+
})
121+
122+
it('does not retry a shape mismatch (deterministic server bug)', async () => {
123+
fetchMock.mockResolvedValueOnce(
124+
new Response(JSON.stringify({ nope: true }), {
125+
status: 200,
126+
headers: { 'content-type': 'application/json' },
127+
})
128+
)
129+
130+
await expect(maskPIIBatchViaHttp(['a'], [])).rejects.toThrow(/unexpected result/)
131+
expect(fetchMock).toHaveBeenCalledTimes(1)
59132
})
60133

61134
it('returns [] without any request for empty input', async () => {

apps/sim/lib/guardrails/mask-client.ts

Lines changed: 90 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { sleep } from '@sim/utils/helpers'
2+
import { backoffWithJitter, parseRetryAfter } from '@sim/utils/retry'
13
import type { GuardrailsMaskBatchResult } from '@/lib/api/contracts'
24
import { generateInternalToken } from '@/lib/auth/internal'
35
import { env } from '@/lib/core/config/env'
@@ -18,6 +20,60 @@ import type { CustomPiiPattern } from '@/lib/guardrails/pii-entities'
1820
*/
1921
const CHUNK_CONCURRENCY = env.PII_MASK_CHUNK_CONCURRENCY ?? 64
2022

23+
/**
24+
* Per-chunk retry budget for transient failures (network errors, 408/429/5xx).
25+
* A large payload fans out into many chunk requests, so a single blip — an ALB
26+
* 502 during a deploy, a Presidio pod restart — must not fail the whole
27+
* redaction (and, on the execution-altering stages, abort the run). With the
28+
* default 500ms→30s jittered backoff this rides out ~2 minutes of outage per
29+
* chunk before giving up. Deterministic failures (4xx, shape mismatches) throw
30+
* immediately.
31+
*/
32+
const MAX_CHUNK_ATTEMPTS = 8
33+
34+
const RETRYABLE_STATUSES = new Set([408, 429, 500, 502, 503, 504])
35+
36+
class MaskChunkHttpError extends Error {
37+
constructor(
38+
message: string,
39+
readonly status: number,
40+
readonly retryAfterMs: number | null
41+
) {
42+
super(message)
43+
this.name = 'MaskChunkHttpError'
44+
}
45+
}
46+
47+
function isRetryableChunkError(error: unknown): boolean {
48+
if (error instanceof MaskChunkHttpError) {
49+
return RETRYABLE_STATUSES.has(error.status)
50+
}
51+
// A rejected fetch (connection refused/reset, DNS, socket drop) is transient —
52+
// Node wraps these in TypeError('fetch failed'). Runtime-level request
53+
// timeouts (undici's default 300s headers/body timeout, Bun's TimeoutError)
54+
// and mid-flight socket closes surface with their own names/codes per runtime;
55+
// all are congestion or connection churn, not a deterministic failure: a chunk
56+
// queued behind a saturated Presidio fleet must retry, not fail the payload.
57+
if (error instanceof TypeError) {
58+
return true
59+
}
60+
const { name, code } = (error ?? {}) as { name?: unknown; code?: unknown }
61+
if (name === 'TimeoutError' || name === 'HeadersTimeoutError' || name === 'BodyTimeoutError') {
62+
return true
63+
}
64+
return (
65+
typeof code === 'string' &&
66+
[
67+
'ECONNRESET',
68+
'ECONNREFUSED',
69+
'EPIPE',
70+
'ETIMEDOUT',
71+
'ConnectionClosed',
72+
'ConnectionRefused',
73+
].includes(code)
74+
)
75+
}
76+
2177
/**
2278
* Mask PII across many strings via the internal app-container endpoint.
2379
*
@@ -29,8 +85,10 @@ const CHUNK_CONCURRENCY = env.PII_MASK_CHUNK_CONCURRENCY ?? 64
2985
* concurrency, so a large payload fans out rather than serializing; order is
3086
* preserved, so the returned array matches `texts` length.
3187
*
32-
* Rejects on any non-2xx, timeout, or shape mismatch so the caller can apply
33-
* its own fail-safe (scrubbing rather than leaking).
88+
* Transient chunk failures (network errors, 408/429/5xx) retry with jittered
89+
* backoff (see {@link MAX_CHUNK_ATTEMPTS}); only a deterministic failure or an
90+
* exhausted retry budget rejects, so the caller can apply its own fail-safe
91+
* (scrubbing rather than leaking).
3492
*/
3593
export async function maskPIIBatchViaHttp(
3694
texts: string[],
@@ -64,8 +122,29 @@ async function postChunk(
64122
language: string | undefined,
65123
customPatterns: CustomPiiPattern[] | undefined
66124
): Promise<string[]> {
67-
// Mint per request: a single token (5min TTL) can expire mid-batch when a
68-
// large execution fans out into many sequential chunk requests.
125+
for (let attempt = 1; ; attempt++) {
126+
try {
127+
return await postChunkOnce(url, texts, entityTypes, language, customPatterns)
128+
} catch (error) {
129+
if (attempt >= MAX_CHUNK_ATTEMPTS || !isRetryableChunkError(error)) {
130+
throw error
131+
}
132+
const retryAfterMs = error instanceof MaskChunkHttpError ? error.retryAfterMs : null
133+
await sleep(backoffWithJitter(attempt, retryAfterMs))
134+
}
135+
}
136+
}
137+
138+
async function postChunkOnce(
139+
url: string,
140+
texts: string[],
141+
entityTypes: string[],
142+
language: string | undefined,
143+
customPatterns: CustomPiiPattern[] | undefined
144+
): Promise<string[]> {
145+
// Mint per attempt: a single token (5min TTL) can expire mid-batch when a
146+
// large execution fans out into many sequential chunk requests or a chunk
147+
// spends its retry budget waiting out an outage.
69148
const token = await generateInternalToken()
70149

71150
// boundary-raw-fetch: internal server-to-server call to the app container (internal JWT auth, configurable base URL)
@@ -80,11 +159,15 @@ async function postChunk(
80159

81160
if (!response.ok) {
82161
const detail = await response.text().catch(() => '')
83-
throw new Error(`PII mask-batch request failed (${response.status}): ${detail.slice(0, 200)}`)
162+
throw new MaskChunkHttpError(
163+
`PII mask-batch request failed (${response.status}): ${detail.slice(0, 200)}`,
164+
response.status,
165+
parseRetryAfter(response.headers.get('retry-after'))
166+
)
84167
}
85168

86-
const data = (await response.json()) as GuardrailsMaskBatchResult
87-
if (!Array.isArray(data.masked)) {
169+
const data = (await response.json()) as GuardrailsMaskBatchResult | null
170+
if (!data || !Array.isArray(data.masked)) {
88171
throw new Error('PII mask-batch returned an unexpected result')
89172
}
90173
return data.masked

0 commit comments

Comments
 (0)