Skip to content

perf(compression): avoid cloning memo misses twice - #11727

Merged
diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
RaviTharuma:fix/compression-memo-post-store-clone
Aug 28, 2026
Merged

diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
RaviTharuma:fix/compression-memo-post-store-clone

Conversation

@RaviTharuma

@RaviTharuma RaviTharuma commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • return freshly computed compression results after storing their defensive cache copy
  • preserve defensive clones on both cache storage and cache hits
  • cover mutation isolation and exact lookup counts for sync and async misses/hits
  • prune the now-stale @typescript-eslint/no-unused-vars suppression for the updated regression test

Validation

  • node --import tsx/esm --test tests/unit/compression/result-memo.test.ts (34 passed)
  • old post-store lookup restored temporarily: focused suite fails exact sync/async lookup assertions
  • npm run lint
  • npm run typecheck:core
  • npm run check:complexity
  • npm run check:file-size -- --base-ref origin/release/v3.8.51
  • npm run check:tracked-artifacts
  • git diff --check

⚠️ base-red inherited: #11449

@RaviTharuma

Copy link
Copy Markdown
Contributor Author

CI note: the focused compression memo regression, core typecheck, focused ESLint, complexity and file-size gates pass locally. Current required failures reproduce base/repository drift unrelated to this two-file PR: generated skills sync (combo-routing legacy key), docs count baseline (providers 42 vs 46), stale ESLint suppressions, broad quality baseline failures, and unit shard 1 terminating from worker heap exhaustion. Build, Vitest and unit shards 2-4 passed.

@RaviTharuma

Copy link
Copy Markdown
Contributor Author

Review-fix cleanup plan (before edits):

  • remove the already-unused callCount local in the touched test file
  • add the smallest existing-style __ForTests lookup counter seam in resultMemo.ts
  • assert exact miss/hit lookup counts for both sync and async entry points while retaining mutation-isolation coverage
  • prove the regression fails with the old post-store lookup restored, then run focused tests, core typecheck, focused lint and static gates

@RaviTharuma

Copy link
Copy Markdown
Contributor Author

Review fixes pushed as fcb28e874:

  • exact test-only memo lookup counter now proves sync and async misses perform only the initial lookup; restoring either old post-store memoLookup makes the focused suite fail (expected 3/1, actual 4/2)
  • mutation isolation remains covered across miss and repeated hits
  • removed pre-existing unused callCount in the touched test

Corrected lint evidence: the earlier focused command used the repository suppressions file and exited 0 before this cleanup, but that command can fail globally when suppressions are stale. Current changed-file lint was therefore rerun against an empty suppressions file and exits 0: eslint open-sse/services/compression/resultMemo.ts open-sse/services/compression/strategySelector.ts tests/unit/compression/result-memo.test.ts --suppressions-location /private/tmp/omniroute-empty-suppressions.json --max-warnings 0. Also green: 31 focused tests, typecheck:core, complexity, file-size, and diff check.

@RaviTharuma
RaviTharuma force-pushed the fix/compression-memo-post-store-clone branch from 908c367 to 6f143cf Compare August 27, 2026 03:21
@RaviTharuma

Copy link
Copy Markdown
Contributor Author

Correction: amended commit is 6f143cf03 (the earlier review comment named an intermediate local hash). PR head and validation described there are otherwise current.

@RaviTharuma
RaviTharuma force-pushed the fix/compression-memo-post-store-clone branch from a1fc9c1 to faec168 Compare August 27, 2026 13:52
@diegosouzapw
diegosouzapw merged commit a14a91d into diegosouzapw:release/v3.8.51 Aug 28, 2026
16 checks passed
AndrianBalanescu added a commit to AndrianBalanescu/OmniRoute that referenced this pull request Aug 31, 2026
…— return caller object, reset lookup counter in clearMemoStore
diegosouzapw added a commit that referenced this pull request Sep 2, 2026
…tion, and token estimation (#7847) (#11844)

* perf(compression): memory and OOM mitigations for large payload hashing and token estimation

* fix(compression): implement getMemoStats observability for result memo (#7847)

Adds the missing memo observability layer referenced by
tests/unit/compression/oom-memo-memory.test.ts and the monitoring API:

- resultMemo.ts: lifetime hit/miss counters + bounded time-ordered ring
  buffer (10k entries, ~90KB) powering 1m/5m/15m/1h hit-rate windows;
  getMemoStats() reports size/capacity/hits/misses/hitRate + windows.
- memoLookup() tags served results with stats.memoHit = true.
- clearMemoStore() also resets counters and the ring.
- compression/index.ts re-exports getMemoStats for the monitoring route.
- types.ts: optional memoHit field on CompressionStats.
- New GET /api/monitoring/compression route exposing the stats snapshot
  (lightweight, no DB) for operators to track cache-hit efficiency.

* fix(compression): align memo contract with upstream #11727 — return caller object, reset lookup counter in clearMemoStore

* fix(compression): restore unwrapEventEnvelope in stream payload collector summaries

The OOM-mitigation commit accidentally replaced unwrapEventEnvelope(evt.data)
with asRecord(evt.data) in the summary builders and live push, breaking
translate-mode {event, data} envelope unwrapping (clientPayload type detection)
and failing 2 stream-payload-collector tests. Restored upstream semantics;
kept the jsonLength OOM optimization as the only delta in this file.

* refactor(compression): break down writeValue and writeEncodedString to pass complexity ratchets

Refactors jsonSha256 internal helpers (writeValue, writeEncodedString)
into small, single-responsibility sub-functions under the complexity
threshold (max cyclomatic 15, max cognitive 15). Preserves exact
JSON.stringify parity, circular reference guards on both arrays and
plain objects, and escape behavior (all 530 relevant tests pass).

* test(compression): make oom-memo heap assertion robust without expose-gc

The CI unit-test shard runner does not pass --expose-gc, so global.gc is
undefined and heapUsed can still momentarily hold GC-pending transients
(observed 53.4 MiB after a 3MiB body). Gate the retained-heap assertion
on forced collection being available (3 forced cycles for array buffers)
instead of skipping it silently, and keep it fully active when
--expose-gc is present.

* fix(compression): restore worker-pool offload path in runCompressionAsync

The OOM-mitigation refactor dropped the isCompressionWorkerEligible /
runCompressionInWorker dispatch at the top of runCompressionAsync, silently
removing the base's worker-thread offload for eligible large payloads.
Restore the block exactly as on release/v3.8.51, ahead of the result-memo
path, keeping the memoization and hashing improvements intact.

* docs(api): document GET /api/monitoring/compression and log route errors via pino

Add the new monitoring endpoint to docs/openapi.yaml following the
neighboring System entries, and replace the route's console.error with
the repo-standard pino logger.

* fix(skills): regenerate omni-resilience and add changelog fragment

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

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Co-authored-by: Andrian Balanescu <AndrianBalanescu@users.noreply.github.com>
Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com>
Bl0ck154 pushed a commit to Bl0ck154/OmniRoute that referenced this pull request Sep 19, 2026
…tion, and token estimation (diegosouzapw#7847) (diegosouzapw#11844)

* perf(compression): memory and OOM mitigations for large payload hashing and token estimation

* fix(compression): implement getMemoStats observability for result memo (diegosouzapw#7847)

Adds the missing memo observability layer referenced by
tests/unit/compression/oom-memo-memory.test.ts and the monitoring API:

- resultMemo.ts: lifetime hit/miss counters + bounded time-ordered ring
  buffer (10k entries, ~90KB) powering 1m/5m/15m/1h hit-rate windows;
  getMemoStats() reports size/capacity/hits/misses/hitRate + windows.
- memoLookup() tags served results with stats.memoHit = true.
- clearMemoStore() also resets counters and the ring.
- compression/index.ts re-exports getMemoStats for the monitoring route.
- types.ts: optional memoHit field on CompressionStats.
- New GET /api/monitoring/compression route exposing the stats snapshot
  (lightweight, no DB) for operators to track cache-hit efficiency.

* fix(compression): align memo contract with upstream diegosouzapw#11727 — return caller object, reset lookup counter in clearMemoStore

* fix(compression): restore unwrapEventEnvelope in stream payload collector summaries

The OOM-mitigation commit accidentally replaced unwrapEventEnvelope(evt.data)
with asRecord(evt.data) in the summary builders and live push, breaking
translate-mode {event, data} envelope unwrapping (clientPayload type detection)
and failing 2 stream-payload-collector tests. Restored upstream semantics;
kept the jsonLength OOM optimization as the only delta in this file.

* refactor(compression): break down writeValue and writeEncodedString to pass complexity ratchets

Refactors jsonSha256 internal helpers (writeValue, writeEncodedString)
into small, single-responsibility sub-functions under the complexity
threshold (max cyclomatic 15, max cognitive 15). Preserves exact
JSON.stringify parity, circular reference guards on both arrays and
plain objects, and escape behavior (all 530 relevant tests pass).

* test(compression): make oom-memo heap assertion robust without expose-gc

The CI unit-test shard runner does not pass --expose-gc, so global.gc is
undefined and heapUsed can still momentarily hold GC-pending transients
(observed 53.4 MiB after a 3MiB body). Gate the retained-heap assertion
on forced collection being available (3 forced cycles for array buffers)
instead of skipping it silently, and keep it fully active when
--expose-gc is present.

* fix(compression): restore worker-pool offload path in runCompressionAsync

The OOM-mitigation refactor dropped the isCompressionWorkerEligible /
runCompressionInWorker dispatch at the top of runCompressionAsync, silently
removing the base's worker-thread offload for eligible large payloads.
Restore the block exactly as on release/v3.8.51, ahead of the result-memo
path, keeping the memoization and hashing improvements intact.

* docs(api): document GET /api/monitoring/compression and log route errors via pino

Add the new monitoring endpoint to docs/openapi.yaml following the
neighboring System entries, and replace the route's console.error with
the repo-standard pino logger.

* fix(skills): regenerate omni-resilience and add changelog fragment

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

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Co-authored-by: Andrian Balanescu <AndrianBalanescu@users.noreply.github.com>
Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com>
@RaviTharuma
RaviTharuma deleted the fix/compression-memo-post-store-clone branch September 23, 2026 19:37
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
Obrigado! Validado em lote combinado (8 PRs, release/v3.8.51):

- TDD claro: sem o fix, o teste focado falha nas asserções de contagem exata de lookup sync/async; com o fix, `tests/unit/compression/result-memo.test.ts` passa (34/34).
- Remove supressão eslint agora obsoleta (`no-unused-vars` no arquivo de teste).
- `⚠️ base-red inherited: diegosouzapw#11449` reconhecido e verificado — não é responsabilidade desta PR (confirmado via probe-worktree do tip puro).
- Gates estáticos do lote OK.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…tion, and token estimation (diegosouzapw#7847) (diegosouzapw#11844)

* perf(compression): memory and OOM mitigations for large payload hashing and token estimation

* fix(compression): implement getMemoStats observability for result memo (diegosouzapw#7847)

Adds the missing memo observability layer referenced by
tests/unit/compression/oom-memo-memory.test.ts and the monitoring API:

- resultMemo.ts: lifetime hit/miss counters + bounded time-ordered ring
  buffer (10k entries, ~90KB) powering 1m/5m/15m/1h hit-rate windows;
  getMemoStats() reports size/capacity/hits/misses/hitRate + windows.
- memoLookup() tags served results with stats.memoHit = true.
- clearMemoStore() also resets counters and the ring.
- compression/index.ts re-exports getMemoStats for the monitoring route.
- types.ts: optional memoHit field on CompressionStats.
- New GET /api/monitoring/compression route exposing the stats snapshot
  (lightweight, no DB) for operators to track cache-hit efficiency.

* fix(compression): align memo contract with upstream diegosouzapw#11727 — return caller object, reset lookup counter in clearMemoStore

* fix(compression): restore unwrapEventEnvelope in stream payload collector summaries

The OOM-mitigation commit accidentally replaced unwrapEventEnvelope(evt.data)
with asRecord(evt.data) in the summary builders and live push, breaking
translate-mode {event, data} envelope unwrapping (clientPayload type detection)
and failing 2 stream-payload-collector tests. Restored upstream semantics;
kept the jsonLength OOM optimization as the only delta in this file.

* refactor(compression): break down writeValue and writeEncodedString to pass complexity ratchets

Refactors jsonSha256 internal helpers (writeValue, writeEncodedString)
into small, single-responsibility sub-functions under the complexity
threshold (max cyclomatic 15, max cognitive 15). Preserves exact
JSON.stringify parity, circular reference guards on both arrays and
plain objects, and escape behavior (all 530 relevant tests pass).

* test(compression): make oom-memo heap assertion robust without expose-gc

The CI unit-test shard runner does not pass --expose-gc, so global.gc is
undefined and heapUsed can still momentarily hold GC-pending transients
(observed 53.4 MiB after a 3MiB body). Gate the retained-heap assertion
on forced collection being available (3 forced cycles for array buffers)
instead of skipping it silently, and keep it fully active when
--expose-gc is present.

* fix(compression): restore worker-pool offload path in runCompressionAsync

The OOM-mitigation refactor dropped the isCompressionWorkerEligible /
runCompressionInWorker dispatch at the top of runCompressionAsync, silently
removing the base's worker-thread offload for eligible large payloads.
Restore the block exactly as on release/v3.8.51, ahead of the result-memo
path, keeping the memoization and hashing improvements intact.

* docs(api): document GET /api/monitoring/compression and log route errors via pino

Add the new monitoring endpoint to docs/openapi.yaml following the
neighboring System entries, and replace the route's console.error with
the repo-standard pino logger.

* fix(skills): regenerate omni-resilience and add changelog fragment

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

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Co-authored-by: Andrian Balanescu <AndrianBalanescu@users.noreply.github.com>
Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.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.

2 participants