[Inference] 01 - Standalone PII detection runtime (RE2 + native RegExp fallback) - #288758
[Inference] 01 - Standalone PII detection runtime (RE2 + native RegExp fallback)#288758jonwalstedt wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Pull request overview
Adds a new standalone workflow anonymization PII detection runtime (RE2-based with a dedicated Piscina worker pool) that can be wired into the inference pipeline later.
Changes:
- Introduces core PII regex rule/match types and a RE2 rule executor with Jest coverage.
- Adds a dedicated Piscina worker service + wrapper to run regex detection off-thread with timeouts/pool rebuild.
- Adds utilities for RE2 compilation validation and deterministic entity token generation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| x-pack/platform/plugins/shared/inference/server/workflow_anonymization/detection/types.ts | Defines rule/match/payload types and failure-mode contract. |
| x-pack/platform/plugins/shared/inference/server/workflow_anonymization/detection/regex_worker_wrapper.js | Worker entry wrapper to set up Node env before loading the task. |
| x-pack/platform/plugins/shared/inference/server/workflow_anonymization/detection/regex_worker_task.ts | Piscina task entrypoint wiring to the executor. |
| x-pack/platform/plugins/shared/inference/server/workflow_anonymization/detection/regex_worker_service.ts | Dedicated Piscina pool management, timeouts, and failure-mode handling. |
| x-pack/platform/plugins/shared/inference/server/workflow_anonymization/detection/index.ts | Public exports for the new runtime surface. |
| x-pack/platform/plugins/shared/inference/server/workflow_anonymization/detection/execute_regex_rules.ts | RE2-based regex execution over records, including zero-length match handling. |
| x-pack/platform/plugins/shared/inference/server/workflow_anonymization/detection/execute_regex_rules.test.ts | Unit tests for the RE2 executor behavior. |
| x-pack/platform/plugins/shared/inference/server/workflow_anonymization/detection/entity_mask.ts | Deterministic token generation via HMAC-SHA256. |
| x-pack/platform/plugins/shared/inference/server/workflow_anonymization/detection/assert_re2_compilable.ts | Helper to validate RE2-only patterns at definition-save time. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4a19dd3 to
2f845ef
Compare
Covers the four contracts identified in review: - Worker pool enabled: tasks run in Piscina and return matches - Worker pool disabled: falls back to synchronous execution - Timeout: AbortError path destroys the pool, recreates it, and throws - failureMode 'allow_unsafe': returns [] and logs on error - failureMode 'block' (default): rethrows without logging Follows the same real-Piscina approach as the o11y RegexWorkerService tests — no mocking, exercises the actual worker boundary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The test claimed to verify the advance-one-char guard for zero-length matches, but a+ can never produce a zero-length match so the guard was never exercised. Switching to a* causes RE2 to emit zero-length hits between non-'a' characters; the guard now fires and both 'aaa' runs are still found, as asserted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rkerService Adds RE2JS.compile() guard in runSync so patterns requiring native RegExp (lookahead/lookbehind/backreferences) throw before execution when the worker pool is disabled. The worker pool provides ReDoS containment via isolation and task timeout; the sync path has neither, so catastrophic backtracking on the event loop must be prevented at the gate. Adds test asserting the sync path rejects a positive-lookahead pattern. Renames AnonymizationWorkerConfig to WorkflowAnonymizationWorkerConfig to reflect the dedicated pool; the placeholder alias in config.ts is superseded when merged with 06-inference-anon-config. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… rebuild Address review feedback: on task timeout, destroying and recreating the whole Piscina pool rejects all other in-flight detection tasks sharing the pool (up to maxThreads concurrently), not just the timed-out one. Piscina already terminates the specific worker thread when the task's AbortSignal fires, so relying on that signal alone contains the runaway task without collateral impact to sibling requests. Reviewed-at: elastic#288758 (comment) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- entity_mask.test.ts: compare hash portions only in delimiter-collision test; full-token comparison always passes when entityClass prefixes differ - types.ts: correct allow_unsafe doc to reflect whole-payload fail-open behavior (returns [] for entire payload, not per-rule skipping) Finding 1 (executeRegexRules exported) was already resolved before review. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove executeRegexRules from index.ts — it is a worker-internal function, not a public API; exporting it invited callers to bypass the pool, failureMode, and the RE2 sync gate - Consolidate sync-path RE2 enforcement: add re2Only flag to compileRule and executeRegexRules so runSync no longer pre-compiles each pattern twice; a single pass now both validates and compiles - Drop typeof value !== 'string' guard in executeRegexRules — Record<string,string> guarantees string values by type; the check was contradicting the contract - Add distinct error message for Piscina queue-at-capacity errors so operators can tell queue saturation apart from bad-pattern failures - Validate non-empty entityClass in generateEntityToken - Use WorkflowAnonymizationWorkerConfig in test (was AnonymizationWorkerConfig, will diverge when elastic#288762 lands) - Use service.stop() in afterEach instead of casting to any - Add assert_re2_compilable.test.ts covering valid patterns, RE2-unsupported constructs, invalid syntax, and error message content - Add maxQueue overflow tests using a spy on the internal Piscina run method - Update config.ts placeholder comment with TODO(elastic#288762) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ce.run() Address review feedback: the JSDoc said "logs and skips the offending rule" but the implementation catches any payload failure and returns [] for the entire payload, not per-rule. Updated to "logs and returns no matches for the entire payload". Reviewed-at: elastic#288758 (comment)
…RegexWorkerService Address review feedback: the service matched 'Task queue is at capacity' but Piscina 5.3.1 throws 'Task queue is at limit', so the saturation branch never fired. Fixed the string in the service and the mock in the test. Piscina does not expose a stable error code, so message matching is the only option; added a comment pinning the verified version. Reviewed-at: elastic#288758 (comment)
…es.test.ts Address review feedback: the describe block was titled "skips empty and non-string field values" but the payload type is Record<string, string> so non-string values are impossible, and the test only covers empty strings. Renamed to "skips empty string fields". Reviewed-at: elastic#288758 (comment)
…RegexWorkerService Previously allow_unsafe returned [] for the entire payload when any rule failed to compile — so one broken rule silenced all other rules' matches, letting all PII through unmasked. Now invalid rules are filtered and logged individually before dispatch; surviving rules still execute and return their matches. Infrastructure failures (timeout, queue saturation, worker crash) still produce a whole-payload [] because there are no partial results to save. compileRule is exported from execute_regex_rules.ts so the pre-filter reuses the same RE2-first → native-fallback logic without duplication. On the sync path re2Only=true is forwarded, so non-RE2 patterns are also skipped (they would block the event loop). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d allow_unsafe semantics Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
33dc52c to
3cd88bf
Compare
There was a problem hiding this comment.
Reviewed the standalone PII detection runtime. The core logic (RE2-first with native fallback, zero-length match handling, fail-closed default, sync-path RE2 gate) is sound and well-tested. One inline finding: the run() doc comment misstates the block-mode behavior for infrastructure failures in a way that contradicts the tested code and could mislead downstream consumers into a fail-open PII leak.
Generated by Claude Reviewer for #288758 · claude · opus · 190.2 AIC · ⌖ 22.3 AIC · ⊞ 5.5K
… rebuild Address review feedback: on task timeout, destroying and recreating the whole Piscina pool rejects all other in-flight detection tasks sharing the pool (up to maxThreads concurrently), not just the timed-out one. Piscina already terminates the specific worker thread when the task's AbortSignal fires, so relying on that signal alone contains the runaway task without collateral impact to sibling requests. Reviewed-at: elastic#288758 (comment) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ce.run() Address review feedback: the JSDoc said "logs and skips the offending rule" but the implementation catches any payload failure and returns [] for the entire payload, not per-rule. Updated to "logs and returns no matches for the entire payload". Reviewed-at: elastic#288758 (comment)
…RegexWorkerService Address review feedback: the service matched 'Task queue is at capacity' but Piscina 5.3.1 throws 'Task queue is at limit', so the saturation branch never fired. Fixed the string in the service and the mock in the test. Piscina does not expose a stable error code, so message matching is the only option; added a comment pinning the verified version. Reviewed-at: elastic#288758 (comment)
…es.test.ts Address review feedback: the describe block was titled "skips empty and non-string field values" but the payload type is Record<string, string> so non-string values are impossible, and the test only covers empty strings. Renamed to "skips empty string fields". Reviewed-at: elastic#288758 (comment)
… rebuild Address review feedback: on task timeout, destroying and recreating the whole Piscina pool rejects all other in-flight detection tasks sharing the pool (up to maxThreads concurrently), not just the timed-out one. Piscina already terminates the specific worker thread when the task's AbortSignal fires, so relying on that signal alone contains the runaway task without collateral impact to sibling requests. Reviewed-at: elastic#288758 (comment) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ce.run() Address review feedback: the JSDoc said "logs and skips the offending rule" but the implementation catches any payload failure and returns [] for the entire payload, not per-rule. Updated to "logs and returns no matches for the entire payload". Reviewed-at: elastic#288758 (comment)
…RegexWorkerService Address review feedback: the service matched 'Task queue is at capacity' but Piscina 5.3.1 throws 'Task queue is at limit', so the saturation branch never fired. Fixed the string in the service and the mock in the test. Piscina does not expose a stable error code, so message matching is the only option; added a comment pinning the verified version. Reviewed-at: elastic#288758 (comment)
…es.test.ts Address review feedback: the describe block was titled "skips empty and non-string field values" but the payload type is Record<string, string> so non-string values are impossible, and the test only covers empty strings. Renamed to "skips empty string fields". Reviewed-at: elastic#288758 (comment)
… rebuild Address review feedback: on task timeout, destroying and recreating the whole Piscina pool rejects all other in-flight detection tasks sharing the pool (up to maxThreads concurrently), not just the timed-out one. Piscina already terminates the specific worker thread when the task's AbortSignal fires, so relying on that signal alone contains the runaway task without collateral impact to sibling requests. Reviewed-at: elastic#288758 (comment) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ce.run() Address review feedback: the JSDoc said "logs and skips the offending rule" but the implementation catches any payload failure and returns [] for the entire payload, not per-rule. Updated to "logs and returns no matches for the entire payload". Reviewed-at: elastic#288758 (comment)
…RegexWorkerService Address review feedback: the service matched 'Task queue is at capacity' but Piscina 5.3.1 throws 'Task queue is at limit', so the saturation branch never fired. Fixed the string in the service and the mock in the test. Piscina does not expose a stable error code, so message matching is the only option; added a comment pinning the verified version. Reviewed-at: elastic#288758 (comment)
…es.test.ts Address review feedback: the describe block was titled "skips empty and non-string field values" but the payload type is Record<string, string> so non-string values are impossible, and the test only covers empty strings. Renamed to "skips empty string fields". Reviewed-at: elastic#288758 (comment)
…hrows on infra failure
Corrects both regex_worker_service.ts and types.ts: infrastructure failures
(timeout, queue saturation) throw in 'block' mode; only 'allow_unsafe' degrades
to an empty result. The previous wording ("regardless of failureMode") contradicted
the implemented and tested behavior and could have led callers to omit a try/catch.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@elasticmachine merge upstream |
|
PR run: bk-01a09eb7-0c70-417d-b9c8-374805773096::smoke-tests::anthropic-claude-4.5-haiku | Baseline (main): bk-01a086a9-0774-4273-8efa-e2b095b5e434::smoke-tests::anthropic-claude-4.5-haiku
Summary View full comparison in UI | Refresh baseline against latest main (click Unblock in the eval build) No significant changes (5 rows)
|
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
Test Failures
History
cc @jonwalstedt |
Summary
What
Adds a standalone PII detection runtime in a new directory —
inference/server/workflow_anonymization/detection/— that is entirely separate from the existing o11y anonymization path (chat_complete/anonymization/). One existing file is modified:config.tsadds aWorkflowAnonymizationWorkerConfigplaceholder type (superseded when06-inference-anon-configlands and introduces the dedicatedworkers.workflowAnonymizationconfig block).Four things are exported from
index.ts:assertRe2CompilablegenerateEntityToken<ENTITY_CLASS>_<hex>PiiRegexWorkerServicePiiRegexRule,PiiRegexMatch,PiiRegexWorkerTaskPayload,PiiDetectionFailureModeWhy
Workflow-driven anonymization needs its own detection runtime for three reasons:
Fail-closed by design. The existing o11y executor silently swallows uncompilable patterns (
catch { return [] }), so a misconfigured rule detects nothing and PII reaches the LLM unmasked. Our runtime's default (failureMode: 'block') throws when a pattern is invalid in both RE2 and native RegExp syntax, so a broken rule propagates an error rather than silently matching nothing. WithfailureMode: 'allow_unsafe', invalid rules are logged and skipped individually — the remaining rules still run and return their matches, so one bad rule does not silence the whole detection suite.RE2-first, native RegExp fallback. RE2JS is tried first; patterns that contain constructs RE2 does not support (lookahead, lookbehind, backreferences) fall back to native RegExp. This matters because users can clone a managed workflow and define their own rules — they are not limited to RE2-compatible syntax. For native RegExp patterns the Piscina worker timeout and per-task AbortSignal provide ReDoS containment (a timed-out task's worker thread is killed by the signal without affecting sibling tasks). When the worker pool is disabled the sync path enforces RE2-only patterns, so catastrophic backtracking cannot block the event loop.
assertRe2Compilableis still exported for callers (e.g. PR #288780) that want to enforce RE2-only at definition-save time for specific rule sets.PiiRegexRulealso exposesmaxMatchLength— a ceiling on accepted match size in characters. This is the escape hatch for length upper-bounds that RE2 cannot encode as lookahead (e.g. "match this pattern but only if it's under 50 chars").Own Piscina pool. Sharing a pool with o11y would couple our capacity and failure modes to theirs.
PiiRegexWorkerService(named to distinguish from o11y'sRegexWorkerService) manages its own worker pool.How it fits in the stack
Wave 0 — no dependencies. Can be reviewed and merged independently of the rest of the stack. This PR adds no plugin registration, no Kibana routes, and no lifecycle hooks — it is a pure server-side library. Merging it does not change any running behaviour. This is inert until PR #288786 wires it into the around-completion hook. PR #288780 imports
assertRe2Compilablefrom this runtime to validate step definitions at workflow save time.Testing
Key assertions:
(?<=@)\w+againstuser@exampleyieldsexample).a*while still finding bothaaaruns in'aaa hello aaa world'; a permanently-zero-length pattern (x*) terminates without hanging.executeRegexRulesand propagates throughPiiRegexWorkerService.run()whenfailureModeis'block'(default);'allow_unsafe'logs and skips the invalid rule while still running all valid rules — one bad rule does not silence the rest. Infrastructure failures (timeout, queue saturation) still return[]for the whole payload regardless offailureMode.(?=a)(a+)+$on 10 000achars) causes the in-flight task to abort via AbortSignal; the pool remains intact for sibling tasks.maxQueue) so operators can distinguish saturation from bad-pattern failures.assertRe2Compilable— valid RE2 patterns pass; lookahead, lookbehind, backreferences, and invalid syntax all throw with a message that includes the offending pattern.generateEntityToken— same scope + class + value always produce the same token; different inputs diverge; length-prefixed HMAC input prevents delimiter collisions; emptyentityClassthrows.Tip
Testing the full flow: A non-merge umbrella branch combining all changes in this stack is available at #289950. Check out
anon/umbrellato test the complete end-to-end anonymization pipeline without checking out individual PRs.Stack status
Generated with Claude Code / Sonnet 4.6