Skip to content

fix(ENG-12540): prevent unhandled rejection cascade on ONNX load failure - #37

Merged
hiskudin merged 3 commits into
mainfrom
fix/unhandled-rejection-cascade
Apr 1, 2026
Merged

hiskudin merged 3 commits into
mainfrom
fix/unhandled-rejection-cascade

Conversation

@hiskudin

@hiskudin hiskudin commented Apr 1, 2026 •

Copy link
Copy Markdown
Collaborator

Context

When the ONNX model fails to load (e.g. on Alpine where onnxruntime-node native binary can't load), void inFlight.finally(...) in _loadModel() creates an unhandled promise rejection. The NestJS global handler wraps and re-emits it, creating a recursive cascade ~130 levels deep on every request that triggers defender.

The growing error strings ("Unhandled rejection: [object Promise] reason: Error: Unhandled rejection: ..." × 130) consume memory continuously under production load, contributing to pod restarts.

Fix

Replace void inFlight.finally(...) with inFlight.finally(...).catch(() => {}).

The .catch(() => {}) silences the duplicate rejection from the .finally() chain. The actual error still propagates correctly through await inFlight → loadModel() catch → caller's try/catch.

One-line diff

- void inFlight.finally(() => _loadingPromises.delete(this.modelPath));
+ inFlight.finally(() => _loadingPromises.delete(this.modelPath)).catch(() => {});

🤖 Generated with Claude Code


Summary by cubic

Stop the unhandled rejection cascade when ONNX model loading fails, eliminating error storms and memory growth under load. Addresses ENG-12540 by catching the .finally() rejection while keeping the original error flow intact.

  • Bug Fixes
    • Add .catch(() => {}) to the inFlight.finally(...) chain; the original error still propagates via await inFlight.
    • Add a test that asserts no unhandledRejection on model load failure and uses try/finally to reliably clean up the listener; document the catch inline.

Written for commit f11b785. Summary will update on new commits.

`void inFlight.finally(...)` discarded the promise returned by
`.finally()`. When `inFlight` rejects (e.g. onnxruntime-node native
binary fails to load), the `.finally()` promise also rejects with no
handler — causing an unhandled rejection. The NestJS global handler
wraps and re-emits it, creating a recursive cascade (~130 deep) on
every request that triggers defender. The growing error strings
consume memory, contributing to pod restarts.

The actual error still propagates correctly through `await inFlight`
→ `loadModel()` catch → caller's try/catch. The `.catch(() => {})`
only silences the duplicate rejection from the `.finally()` chain.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@hiskudin
hiskudin requested a review from a team as a code owner April 1, 2026 09:23
Copilot AI review requested due to automatic review settings April 1, 2026 09:23
cubic-dev-ai[bot]
cubic-dev-ai Bot previously approved these changes Apr 1, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 1 file

Auto-approved: Isolated fix to prevent unhandled promise rejections and associated memory growth during ONNX load failures.

Copilot AI 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.

Pull request overview

This PR prevents an unhandled promise rejection cascade when the ONNX model load fails (e.g., native onnxruntime-node binary can’t load), by ensuring the promise created by the .finally() cleanup path is explicitly handled.

Changes:

  • Replace void inFlight.finally(...) with inFlight.finally(...).catch(() => {}) to prevent an unhandled rejection from the .finally() chain on load failures.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/classifiers/onnx-classifier.ts
Comment thread src/classifiers/onnx-classifier.ts
Comment thread src/classifiers/onnx-classifier.ts
@hiskudin hiskudin changed the title fix: prevent unhandled rejection cascade on ONNX load failure fix(ENG-12540): prevent unhandled rejection cascade on ONNX load failure Apr 1, 2026
- Add comment explaining why .catch(() => {}) is needed
- Add test that verifies no unhandledRejection is emitted when the
  ONNX model fails to load

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="specs/onnx-classifier.spec.ts">

<violation number="1" location="specs/onnx-classifier.spec.ts:87">
P3: Ensure the global unhandledRejection listener is always removed even when the test fails. Wrap the test body in a try/finally so the listener is cleaned up on all exits.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread specs/onnx-classifier.spec.ts Outdated
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

0 issues found across 1 file (changes from recent commits).

Auto-approved: Fixes a critical production issue involving recursive error cascades and memory growth. The change is isolated, follows standard promise handling practices, and includes a regression test.

@hiskudin
hiskudin merged commit 52c99e3 into main Apr 1, 2026
4 checks passed
@hiskudin
hiskudin deleted the fix/unhandled-rejection-cascade branch April 1, 2026 11:20
hiskudin added a commit that referenced this pull request May 5, 2026
…ty redaction

Two issues from Codex review:

P1: PatternDetector classified obfuscated payloads (1gn0r3 pr3v10us,
S Y S T E M:, Zalgo) by running Tier 1 against normalized text, but
Sanitizer.applyRiskBasedMethods ran containsRoleMarkers/removePatterns
against the un-normalized original. Result: detection escalated risk
to medium/high but the actual obfuscated content survived in the
sanitized output and could flow into downstream prompts.

Fix: added Step 1.5 in the sanitizer that runs at HIGH risk only —
NFD decompose + stripCombiningMarks + normalizeWhitespace +
normalizeLeetSpeak — before role stripping and pattern removal. At
high risk Tier 1 already has high confidence of an attack, so the
trade-off of stripping benign accents from the redacted output is
acceptable. Medium-risk benign content is unaffected.

P2: detectHtmlEntities pushed every 3+ entity run into detections,
causing redactAllEncoding to wipe benign escaped content like
&#49;&#48;&#37; ("10%") whenever the field was escalated to high risk
for an unrelated reason. The decoder doc claimed "Only emits suspicious"
but the code emitted all.

Fix: kept the detector pushing all detections (so decodeAllLevels can
chain through HTML→base64→plaintext correctly), but processEncodedContent
in REDACT mode now filters out non-suspicious HTML entities. Decode
mode is unaffected.

Verified end-to-end:
- Leet, whitespace-spaced, and Zalgo payloads are now redacted in output
- Benign HTML entities (10% encoded) survive sanitization
- Accented text (café, niño) preserved at low/medium risk

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hiskudin added a commit that referenced this pull request May 13, 2026
…#39)

* feat(tier1): add obfuscation normalisation chain to Tier 1 detection

Adds a pre-processing normalisation chain to PatternDetector.analyze so
existing injection patterns catch obfuscated variants without requiring
new regex entries for each substitution style.

Changes:
- leet-normalizer.ts (new): normalizeLeetSpeak() reverses digit/symbol
  substitutions (4→a, 3→e, 1→i, 0→o, 5→s, 7→t); protects hex escapes,
  base64 blobs and $( from corruption
- normalizer.ts: adds normalizeWhitespace() — collapses letter-by-letter
  spacing (S Y S T E M → SYSTEM) and embedded newlines inside words
- pattern-detector.ts: runs normalisation chain (whitespace → unicode →
  leet) before Tier 1 matching; two-pass pattern run on raw+normalised
  text with dedup so obfuscation patterns still fire on raw text
- encoding-detector.ts: adds decodeAllLevels() for chained encoding
  (base64 of hex etc.) and containsSuspiciousEncodingDeep()
- sanitizer.ts: uses containsSuspiciousEncodingDeep in high-risk path
- patterns.ts: removes stale leet entries from FAST_FILTER_KEYWORDS

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(lint): merge duplicate normalizer imports and sort import order

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(review): address PR review comments on obfuscation normalisation

- encoding-detector: containsSuspiciousEncodingDeep now also runs
  containsSuspiciousEncoding on the decoded result, catching payloads
  that remain partially encoded when maxIterations is reached (P1)
- types: add normalised?: boolean to PatternMatch to signal when
  position/matched values reference the normalised form, not the
  original input string (P2)
- pattern-detector: tag normalised-pass matches with normalised:true
  so consumers can distinguish them from raw-text matches (P2)
- normalizer: remove \s* from newline regex to avoid silently consuming
  word-separator spaces (e.g. "ignore\n previous" → "ignoreprevious"
  would break multi-word pattern matching) (P2)
- test: assert specifically on ignore_previous rather than using ||
  with leetspeak_injection, so regressions in leet normalisation are
  caught rather than masked by the raw-text pass (P2)

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(tier1): correct normalisation order and skip redundant pass for plain text

Two bugs found during PR review:

1. Normalisation order was wrong: running normalizeWhitespace before
   normalizeUnicode means Cyrillic/fullwidth homoglyph spacing attacks
   (e.g. "s у s t e m" with Cyrillic у) are never collapsed — whitespace
   normalisation uses [a-zA-Z] and exits before unicode normalisation
   resolves the homoglyphs to ASCII. Fix: run normalizeUnicode first so
   all characters are ASCII before whitespace collapse runs.
   Order is now: normalizeUnicode → normalizeWhitespace → normalizeLeetSpeak

2. Performance: when normalisation produces no change (plain text with
   keywords, the common case), detectPatterns was called twice on identical
   text. Added a rawText === analysisText guard to short-circuit to a
   single pass, restoring the original single-pass performance for
   unobfuscated input.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(tier1): run raw pass when normHasKeywords; add missing unit tests

Raw pass was skipped for pure leet-speak inputs (e.g. "1gn0r3 pr3v10us")
because leet keywords were removed from FAST_FILTER_KEYWORDS, making
rawHasKeywords false. The comment claimed the raw pass "catches obfuscation
patterns like leetspeak_injection" but it was never actually running for
those inputs. Fix: run the raw pass whenever rawHasKeywords OR normHasKeywords
is true, so raw obfuscation patterns fire even when only the normalised text
triggered the fast filter.

Adds unit tests for:
- normalizeWhitespace: letter spacing, embedded newlines, edge cases
- normalizeLeetSpeak: substitution map, all protected sequence types ($(/
  hex/unicode/base64), ! boundary rule, plain text passthrough
- decodeAllLevels: single-layer, double-layer (chained), maxIterations cap,
  amplification guard
- containsSuspiciousEncodingDeep: single/double encoded payloads, benign cases

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* feat(detection): add HTML entity, ROT13, ROT47, binary, Morse, and Zalgo detection

New encoding detectors (all integrated into detectEncoding/decodeAllLevels):
- HTML entities: decodes &#NNN;, &#xHH;, and 30 named entities; gate: 3+
  grouped entity tokens; suspicious when decoded content has injection keyword
- ROT13: gate 70%+ letter density; only emits when decoded text contains an
  injection keyword — prevents false positives on arbitrary high-letter text
- ROT47: printable ASCII rotation; conservative — only emits on suspicious
  decoded content
- Binary strings: gate 3+ space-separated 8-bit groups of [01]; decodes via
  parseInt(group, 2)
- Morse code: gate 5+ dot/dash groups; 36-entry table (A–Z, 0–9); rejects if
  >20% unknown symbols

Zalgo / combining marks (normalizer.ts):
- stripCombiningMarks() strips U+0300–U+036F and 4 other combining ranges
- normalizeUnicode() now runs NFD → stripCombiningMarks → NFKC so marks are
  separated before being stripped (NFKC alone would compose them into
  precomposed chars that the regex cannot see)
- containsSuspiciousUnicode() flags 3+ combining marks

Leet-speak improvements (leet-normalizer.ts):
- Token-aware normalization: only substitutes within alphanumeric tokens that
  contain at least one letter — "price: 100" stays "price: 100"
- Added @→a and 8→b to LEET_MAP
- Includes !, @, $ in token regex so "adm!n", "@dm1n", "$y$tem" normalize correctly
- PROTECTED_SEQUENCE continues to guard $( before token processing runs

Tier 1 patterns (patterns.ts):
- Added binary_string_encoding (medium) and morse_code_encoding (low) patterns
- Upgraded rot13_mention severity from low → medium

All new behaviour covered by 28 new test cases (240 total, up from 212).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(encoding): prevent full-text detection overlap in processEncodedContent

ROT13 and ROT47 detections span position=0, length=text.length. When both
fired on the same text alongside positional detections (hex/base64), the
reverse-position splice loop would apply positional replacements first, then
the full-text detection would overwrite them using the original text.length —
corrupting previous replacements. In decodeAllLevels (action:"decode"), a
text triggering both ROT13 and ROT47 would oscillate across all 5 iterations
without converging.

Fix: processEncodedContent now separates positional from full-text detections.
Positional detections are applied first. Full-text is only applied when there
are no positional detections; only the first full-text detection is used when
multiple exist. decodeAllLevels naturally converges because after positional
content is decoded, the next iteration re-evaluates the full-text transforms.

Also adds three tests flagged during review:
- normalizeWhitespace: letter-adjacent newline (no surrounding spaces)
- normalizeUnicode: precomposed accent (café → cafe) via NFD decomposition
- normalizeLeetSpeak: mixed alphanumeric tokens (v3rs10n → version)

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* test(encoding): add ROT13+ROT47 simultaneous detection coverage

Adds two tests specifically for the full-text detection overlap fix:
- processedText is a coherent string (not a corrupted splice) when both
  ROT13 and ROT47 fire on the same input
- decodeAllLevels converges (levels <= 2, not oscillating to maxIterations=5)

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

* fix(detection): add encoding-based risk escalation in sanitizeStringField

Encoded payloads (ROT13, binary, Morse, etc.) don't trigger Tier 1
patterns because their content has no fast-filter keywords — so risk
stays at the default "medium" and encoding detection in the sanitizer
(Step 4, gated behind riskLevel === "high") never runs. Injections
encoded in these formats pass through undetected.

Fix: after Tier 1 classification in sanitizeStringField, run
containsSuspiciousEncoding (shallow, single pass, ~0.05ms per field)
as an additional risk escalation check. If suspicious encoding is found,
risk escalates to "high" and the existing deep multi-level decoder in
the sanitizer's Step 4 handles decoding and redaction.

Before: ROT13/binary/Morse payloads → risk stays medium → encoding
detection skipped → allowed: true (injection passes through)

After: ROT13/binary/Morse payloads → encoding escalation → risk=high →
deep decode + redaction → blocked

Quality test results (Tier 1 + sanitizer, no ML):
- Precision: 100% (0 false positives on 10 benign inputs)
- Recall: 88% (7/8 encoding types caught; ROT47 miss due to short input)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: restore main's v4 ONNX model files (accidentally reverted during merge)

* fix(review): address 3 Codex review findings on obfuscation PR

P1: Accent stripping leaked into returned output (data loss bug)
- normalizeUnicode previously did NFD→stripCombiningMarks→NFKC, which
  rewrote 'café' → 'cafe' for benign user content. Sanitizer.applyRiskBasedMethods
  returns this output to callers, breaking the analysis-only contract.
- Fix: removed stripCombiningMarks from normalizeUnicode (kept NFKC only).
  Combining-mark stripping now lives in the analysis-only path inside
  PatternDetector.analyze: stripCombiningMarks(rawText.normalize('NFD'))
  runs before normalizeUnicode in the chain.

P1: Long leet payloads bypassed the fast filter
- The leet normaliser skips 20+ char alphanumeric tokens (treated as
  base64-like blobs), so '1gn0r3pr3v10us1nstruct10ns' was left unchanged
  by both the leet normaliser and unicode normaliser. With the leet
  keywords removed from FAST_FILTER_KEYWORDS, the fast filter
  short-circuited and leetspeak_injection was never evaluated.
- Fix: restore '1gn0r3', 'f0rg3t', 'byp4ss' to FAST_FILTER_KEYWORDS so
  long leet payloads still pass the filter and reach the regex.

P2: Chained encoding payloads (e.g. btoa(btoa(...))) slipped through
- sanitizeStringField escalated using shallow containsSuspiciousEncoding,
  which only catches single-layer encodings. Doubly-encoded payloads —
  where the outer layer decodes to another encoded blob with no visible
  keywords — stayed at medium risk and never reached the deep check in
  the sanitizer (gated at high).
- Fix: sanitizeStringField now uses containsSuspiciousEncodingDeep which
  loops through layers (max 5, with amplification guard). Also tracks
  whether escalation came from encoding so the early-block path records
  'encoding_detection' in methodsByField — without this, blockHighRisk
  would set sanitized to '[CONTENT BLOCKED]' but allowed would stay true
  because hasThreats only counts active sanitization methods.

Test updates:
- normalizeUnicode tests now assert accent preservation (the new contract)
- new stripCombiningMarks tests cover the analysis-only Zalgo path

268/269 tests passing (1 pre-existing flaky test on main, not introduced
by this PR — onnx-classifier batch test scoring borderline injection
sample at 0.467 vs >0.5 threshold).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(review): remove dead try/catch in detectBinaryStrings (aikido)

* fix(review): close detection-vs-sanitization gap and benign HTML entity redaction

Two issues from Codex review:

P1: PatternDetector classified obfuscated payloads (1gn0r3 pr3v10us,
S Y S T E M:, Zalgo) by running Tier 1 against normalized text, but
Sanitizer.applyRiskBasedMethods ran containsRoleMarkers/removePatterns
against the un-normalized original. Result: detection escalated risk
to medium/high but the actual obfuscated content survived in the
sanitized output and could flow into downstream prompts.

Fix: added Step 1.5 in the sanitizer that runs at HIGH risk only —
NFD decompose + stripCombiningMarks + normalizeWhitespace +
normalizeLeetSpeak — before role stripping and pattern removal. At
high risk Tier 1 already has high confidence of an attack, so the
trade-off of stripping benign accents from the redacted output is
acceptable. Medium-risk benign content is unaffected.

P2: detectHtmlEntities pushed every 3+ entity run into detections,
causing redactAllEncoding to wipe benign escaped content like
&#49;&#48;&#37; ("10%") whenever the field was escalated to high risk
for an unrelated reason. The decoder doc claimed "Only emits suspicious"
but the code emitted all.

Fix: kept the detector pushing all detections (so decodeAllLevels can
chain through HTML→base64→plaintext correctly), but processEncodedContent
in REDACT mode now filters out non-suspicious HTML entities. Decode
mode is unaffected.

Verified end-to-end:
- Leet, whitespace-spaced, and Zalgo payloads are now redacted in output
- Benign HTML entities (10% encoded) survive sanitization
- Accented text (café, niño) preserved at low/medium risk

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
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.

3 participants