Skip to content

fix(compression): track only the recursion path in isStrictlySerializable (#13154) - #13423

Merged
diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
KooshaPari:pr/13154-compression-gate
Sep 17, 2026
Merged

diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
KooshaPari:pr/13154-compression-gate

Conversation

@KooshaPari

@KooshaPari KooshaPari commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

isStrictlySerializable() usa um seen que acumula a árvore inteira e nunca é desempilhado, então dois irmãos apontando para o MESMO objeto não-cíclico eram lidos como ciclo e o body era rejeitado. Esta PR troca o rastreamento para o caminho de recursão (try/finally), que é o critério correto para detectar ciclo.

objeto compartilhado (não-cíclico):  antes false  →  agora true
ciclo real (controle negativo):      antes false  →  agora false

Escopo — leia antes de fechar a #13154

O título anterior desta PR dizia "remove the isStrictlySerializable gate". Isso não é o que o diff faz, e o corpo anterior também listava undefined como causa corrigida. Medido na árvore desta branch:

isStrictlySerializable({body:{}, mode:"standard", options:{provider:undefined}})  →  false
isStrictlySerializable({a: new Date()})                                          →  false

O gate continua rejeitando undefined e Date. Como strategySelector.ts:519-528 monta workerOptions com 9 chaves sempre presentes, na prática quase toda chamada real tem pelo menos uma undefined — ou seja, o caminho comum da #13154 (compressão caindo inline no event loop) permanece.

Por isso: esta PR corrige o caso do sub-objeto compartilhado, que é real e tem teste vermelho-no-tip/verde-com-o-fix verificado por execução. A #13154 fica aberta para o caso undefined/Date.

A mudança em strategySelector.ts é somente comentário, sem alteração de comportamento.

…id bodies (diegosouzapw#13154)

Fixes diegosouzapw#13154

isCompressionWorkerEligible used isStrictlySerializable to pre-validate
bodies before posting them to a worker thread. The gate is stricter than
structuredClone (which postMessage uses natively), rejecting:
- undefined values (common in optional config fields)
- Date, Map, Set, Uint8Array, RegExp (all structuredClone-compatible)
- Shared (non-cyclic) sub-objects (misread as cycles)

This caused compression to fall back to inline execution on the main
event loop, blocking every concurrent request for the duration of
compression passes — the exact failure mode of diegosouzapw#10300.

The serializability walk is a slower, buggier duplicate of the check
postMessage already performs. Removing it:
- Eliminates a recursive walk of the entire body on the main thread
- Fixes false rejections that prevent worker offload
- Allows the catch block at the call site to properly fall through
  to inline compression instead of silently shipping uncompressed
Copilot AI lite review requested due to automatic review settings September 12, 2026 09:45

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for #13154. Holding — this goes a bit further than the bug.

The reported false negatives come from isStrictlySerializable sharing one seen set across siblings, so a repeated non-cyclic sub-object reads as a cycle. Removing the gate entirely means bodies that really can't be structured-cloned reach postMessage and only fail inside the worker.

The catch change also means a worker timeout now re-runs the same heavy compression inline on the event loop. The worker exists precisely to avoid that.

Could you instead fix the cycle detection (track the recursion path — add before descending, delete after), keep the timeout fallback returning the body uncompressed, and add a test with a shared sub-object and one with a real cycle?

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for tracking down the isStrictlySerializable false-positive (shared seen set across
siblings) — that diagnosis is correct. Removing the gate entirely goes further than the bug
report though: it also lets a body with a genuine cycle reach postMessage, where it only fails
inside the worker, and the catch-block change means a worker timeout now re-runs the same
heavy compression synchronously on the event loop — the exact case the worker exists to avoid.
Could you instead fix cycle detection by tracking the recursion path (add before descending,
delete after) rather than dropping the check, keep the timeout fallback returning the body
uncompressed, and add tests for both a shared-non-cyclic-object case and a real-cycle case?

…in isStrictlySerializable (diegosouzapw#13154)

Restores the cycle-detection gate instead of removing it: the original
bug was a single `seen` set shared across the entire recursion tree,
never backtracked, so two sibling branches referencing the SAME
non-cyclic sub-object were misread as a cycle. Adding to `seen` before
descending and removing it after (try/finally) fixes the false positive
while a genuine cycle is still rejected before it ever reaches
postMessage/the worker.

Also reverts the worker-failure catch in runCompressionAsync back to
returning the body uncompressed: a worker timeout means the compression
was already too heavy for the worker's own budget, so falling through
to run that same heavy compression synchronously on the main event loop
defeats the point of offloading it to a worker in the first place.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw diegosouzapw changed the title fix(compression): remove isStrictlySerializable gate that rejects valid bodies (#13154) fix(compression): track only the recursion path in isStrictlySerializable (#13154) Sep 17, 2026
@diegosouzapw
diegosouzapw merged commit 7a0b0c6 into diegosouzapw:release/v3.8.51 Sep 17, 2026
14 of 16 checks passed
insoln added a commit to insoln/OmniRoute that referenced this pull request Sep 22, 2026
Replays diegosouzapw#12755 on top of diegosouzapw#13637 so both fixes survive intact.

A synchronous spawn failure left the queue stranded: nothing else calls dispatch() again, so queued jobs and their full request bodies were retained for the lifetime of the process. Queued jobs now reject as retryable when no worker is left to drain them, while a capacity-spawn failure with a healthy worker keeps the backlog for that worker.

Preserve diegosouzapw#13637's reject-based runtime-fault contract: fast faults retry compression in-process, while a dispatch timeout degrades to the uncompressed body after spending the worker budget. Keep fail-open reporting, strip non-cloneable callbacks from wire jobs, and guard postMessage failures.

Add an end-to-end regression proving the diegosouzapw#13423 shared-subobject shape remains worker-eligible and falls back to in-process compression after synchronous MODULE_NOT_FOUND.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…able (diegosouzapw#13154) (diegosouzapw#13423)

* fix(compression): remove isStrictlySerializable gate that rejects valid bodies (diegosouzapw#13154)

Fixes diegosouzapw#13154

isCompressionWorkerEligible used isStrictlySerializable to pre-validate
bodies before posting them to a worker thread. The gate is stricter than
structuredClone (which postMessage uses natively), rejecting:
- undefined values (common in optional config fields)
- Date, Map, Set, Uint8Array, RegExp (all structuredClone-compatible)
- Shared (non-cyclic) sub-objects (misread as cycles)

This caused compression to fall back to inline execution on the main
event loop, blocking every concurrent request for the duration of
compression passes — the exact failure mode of diegosouzapw#10300.

The serializability walk is a slower, buggier duplicate of the check
postMessage already performs. Removing it:
- Eliminates a recursive walk of the entire body on the main thread
- Fixes false rejections that prevent worker offload
- Allows the catch block at the call site to properly fall through
  to inline compression instead of silently shipping uncompressed

* fix(compression): track only the recursion path, not the whole tree, in isStrictlySerializable (diegosouzapw#13154)

Restores the cycle-detection gate instead of removing it: the original
bug was a single `seen` set shared across the entire recursion tree,
never backtracked, so two sibling branches referencing the SAME
non-cyclic sub-object were misread as a cycle. Adding to `seen` before
descending and removing it after (try/finally) fixes the false positive
while a genuine cycle is still rejected before it ever reaches
postMessage/the worker.

Also reverts the worker-failure catch in runCompressionAsync back to
returning the body uncompressed: a worker timeout means the compression
was already too heavy for the worker's own budget, so falling through
to run that same heavy compression synchronously on the main event loop
defeats the point of offloading it to a worker in the first place.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: Koosha Pari <koosha@phenotype.ai>
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.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