Skip to content

refactor(memory-safety): back the secure-path cache with the shared bounded map - #10826

Draft
nwparker wants to merge 2 commits into
oom-slice-0-foundationfrom
oom-slice-1-shared
Draft

nwparker wants to merge 2 commits into
oom-slice-0-foundationfrom
oom-slice-1-shared

Conversation

@nwparker

@nwparker nwparker commented Jul 27, 2026 •

Copy link
Copy Markdown
Contributor

Second slice of the re-landed memory-safety work (replaces the oversized #10548). Stacked on #10824.

Why

SecurePathHardeningCache hand-rolled its own key-byte-weighted LRU — eviction loop, byte ledger, and recency reordering — which is exactly what BoundedMap already does. This delegates to it, cutting the implementation roughly in half (49 lines removed, 24 added).

What's preserved

Two behaviors the generic map doesn't model stay in the wrapper:

  1. A path over either the per-key or the aggregate ceiling is rejected outright
  2. A non-positive maxEntries disables the cache rather than caching one entry (BoundedMap has no zero-capacity mode)

Proof it's behavior-preserving

The existing suite had 4 tests, which felt thin for a swap that changes eviction internals. So I differential-tested the old and new implementations directly against each other:

  • 5 bound configurations × 40 seeds × 60 randomized set/get/delete operations
  • After every step, asserted identical: return values, entry count, retained byte total, and LRU path ordering
  • Deterministic PRNG so any failure reproduces

They agree exactly. Also covered explicitly: maxEntries: 0, negative maxEntries, maxKeyBytes > maxTotalKeyBytes, maxTotalKeyBytes: 0, single-entry caches, empty-string keys, re-setting an existing key (refresh without double-counting), and multibyte keys (日本語, emoji) to confirm byte-not-char accounting.

The differential harness was scaffolding and isn't part of the diff — but the two wrapper-specific behaviors it exercised had no coverage before, so those are now pinned as real tests.

Verification

  • pnpm run typecheck — clean
  • oxlint — clean
  • 3,124 tests passing across src/shared
  • secure-file.test.ts (the only consumer) — 17/17

One unrelated pre-existing flake (setup-agent-sequencing.test.ts, spawns real child processes against a 5s timeout) fails under full-suite load; confirmed it fails identically on clean main.

Made with Orca 🐋

…ounded map

SecurePathHardeningCache hand-rolled its own key-byte-weighted LRU — eviction
loop, byte ledger, and recency reordering — duplicating BoundedMap. Delegates to
it instead, cutting the implementation roughly in half.

Two behaviors the generic map does not model are kept in the wrapper: a path
over either the per-key or the aggregate ceiling is rejected outright, and a
non-positive maxEntries disables the cache rather than caching one entry.

Verified as a behavior-preserving swap by differential-testing the old and new
implementations against each other: 5 bound configurations x 40 seeds x 60
randomized set/get/delete operations, asserting identical return values, entry
counts, byte totals, and LRU ordering after every step. Both agree exactly,
including negative bounds, zero ceilings, and multibyte keys.

Adds coverage for the two wrapper-specific behaviors, which had none.
@OrcaWin
OrcaWin marked this pull request as draft October 27, 2026 09:44
@OrcaWin

OrcaWin commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator

Moving this PR to draft because it has been untouched for more than 80 days (latest recorded PR activity or commit timestamp: 2026-07-28T03:18:01Z; checked on 2026-10-27). This keeps the review queue focused on active work while preserving the proposal. Please mark it ready for review when work resumes.

This branch has not been deployed

No deployments
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