fix(auth): stop per-socket AsyncLocalStorage heap leak in addTransactionCapability - #2722
Conversation
addTransactionCapability created a new AsyncLocalStorage per socket. On Node's legacy async-context propagation (Node 22, pre-AsyncContextFrame), every live AsyncLocalStorage stamps a slot on every pending async resource, so cost grows O(live ALS instances x live async resources). Under reconnection churn each new socket adds another never-disabled instance, and a burst of pending promises/timeouts pushes the heap to OOM even with --max-old-space-size=4096. Hoist the storage to a module-level singleton. The transaction context lives in the run(ctx, work) scope rather than on the instance, so distinct sockets do not collide.
Review follow-up: a process-wide AsyncLocalStorage holding a bare TransactionContext broke per-store isolation. If a wrapped store B was used inside store A's transaction callback, B saw A's ambient context and its writes were committed into A's backing store. Store the context in a Map keyed by a per-store token. Each store resolves only its own context, new transactions inherit outer stores' contexts so nesting across stores stays isolated (both the legacy transaction() and the record-scoped transactWith() APIs), and the process still keeps a single AsyncLocalStorage instance (the memory fix). Adds regression tests for both cross-store leakage and cross-store nesting.
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change replaces single-context async-local transaction storage with a shared map keyed by per-store tokens. Transaction entry points inherit and update that map, while new tests verify isolation across independent stores and nested transactions. ChangesCross-store transaction isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StoreA
participant transaction
participant txStorage
participant StoreB
StoreA->>transaction: start transaction
transaction->>txStorage: read shared context map
transaction->>txStorage: run inherited map with StoreA token
StoreB->>transaction: start transaction
transaction->>txStorage: read shared context map
transaction->>txStorage: run inherited map with StoreB token
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Utils/auth-utils.ts (1)
234-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace implementation-restating comments with rationale.
These comments describe what
storeToken,getContext, andisInTransactiondo. Keep only the isolation rationale—preventing an ambient context from another wrapped store being used—or remove them.As per coding guidelines, “Write comments explaining the why … Do not write comments that restate the code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Utils/auth-utils.ts` around lines 234 - 247, Replace the comments above storeToken, getContext, and isInTransaction with a concise rationale explaining that the store-specific token prevents an ambient transaction context from another wrapped store being used, or remove the comments if no rationale is needed; do not describe the symbols’ implementations.Source: Coding guidelines
src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts (1)
78-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd equivalent
transactWith()isolation coverage.This suite only exercises
transaction(), while the independently changedtransactWith()map-inheritance path needs the same cross-store and nested-store regression protection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts` around lines 78 - 95, The test suite currently covers nested cross-store isolation only through transaction(); add equivalent coverage for transactWith(). In the existing “keeps nested transactions across stores isolated by token” test area, exercise nested transactWith() calls across a and b, assert both stores report active transactions, write distinct values, and verify each backing store retains only its own value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts`:
- Around line 35-46: Remove the new any usages in the test store’s get and set
methods: type get<T> using SignalDataTypeMap[T], and iterate set’s input with
Object.keys(d) cast to Array<keyof SignalDataTypeMap> so indexed access remains
type-safe. Preserve the existing fixture behavior while enforcing the
SignalDataTypeMap contract.
In `@src/Utils/auth-utils.ts`:
- Line 446: Update the transaction-entry trace call in the surrounding
transaction flow to use structured logging, passing the lock key in an object
before the message while preserving the existing “entering transaction” text.
---
Nitpick comments:
In `@src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts`:
- Around line 78-95: The test suite currently covers nested cross-store
isolation only through transaction(); add equivalent coverage for
transactWith(). In the existing “keeps nested transactions across stores
isolated by token” test area, exercise nested transactWith() calls across a and
b, assert both stores report active transactions, write distinct values, and
verify each backing store retains only its own value.
In `@src/Utils/auth-utils.ts`:
- Around line 234-247: Replace the comments above storeToken, getContext, and
isInTransaction with a concise rationale explaining that the store-specific
token prevents an ambient transaction context from another wrapped store being
used, or remove the comments if no rationale is needed; do not describe the
symbols’ implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 845ff5b3-93e3-4d49-8739-b9feab9e31d3
📒 Files selected for processing (2)
src/Utils/auth-utils.tssrc/__tests__/Utils/auth-utils.cross-store-isolation.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Utils/auth-utils.ts (1)
234-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace implementation-restating comments with rationale.
These comments describe what
storeToken,getContext, andisInTransactiondo. Keep only the isolation rationale—preventing an ambient context from another wrapped store being used—or remove them.As per coding guidelines, “Write comments explaining the why … Do not write comments that restate the code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Utils/auth-utils.ts` around lines 234 - 247, Replace the comments above storeToken, getContext, and isInTransaction with a concise rationale explaining that the store-specific token prevents an ambient transaction context from another wrapped store being used, or remove the comments if no rationale is needed; do not describe the symbols’ implementations.Source: Coding guidelines
src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts (1)
78-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd equivalent
transactWith()isolation coverage.This suite only exercises
transaction(), while the independently changedtransactWith()map-inheritance path needs the same cross-store and nested-store regression protection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts` around lines 78 - 95, The test suite currently covers nested cross-store isolation only through transaction(); add equivalent coverage for transactWith(). In the existing “keeps nested transactions across stores isolated by token” test area, exercise nested transactWith() calls across a and b, assert both stores report active transactions, write distinct values, and verify each backing store retains only its own value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts`:
- Around line 35-46: Remove the new any usages in the test store’s get and set
methods: type get<T> using SignalDataTypeMap[T], and iterate set’s input with
Object.keys(d) cast to Array<keyof SignalDataTypeMap> so indexed access remains
type-safe. Preserve the existing fixture behavior while enforcing the
SignalDataTypeMap contract.
In `@src/Utils/auth-utils.ts`:
- Line 446: Update the transaction-entry trace call in the surrounding
transaction flow to use structured logging, passing the lock key in an object
before the message while preserving the existing “entering transaction” text.
---
Nitpick comments:
In `@src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts`:
- Around line 78-95: The test suite currently covers nested cross-store
isolation only through transaction(); add equivalent coverage for
transactWith(). In the existing “keeps nested transactions across stores
isolated by token” test area, exercise nested transactWith() calls across a and
b, assert both stores report active transactions, write distinct values, and
verify each backing store retains only its own value.
In `@src/Utils/auth-utils.ts`:
- Around line 234-247: Replace the comments above storeToken, getContext, and
isInTransaction with a concise rationale explaining that the store-specific
token prevents an ambient transaction context from another wrapped store being
used, or remove the comments if no rationale is needed; do not describe the
symbols’ implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 845ff5b3-93e3-4d49-8739-b9feab9e31d3
📒 Files selected for processing (2)
src/Utils/auth-utils.tssrc/__tests__/Utils/auth-utils.cross-store-isolation.test.ts
🛑 Comments failed to post (2)
src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts (1)
35-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove new
anyusage from the test store.
Record<string, any>and(d as any)[type]bypass the strict fixture contract. Typeget<T>againstSignalDataTypeMap[T]and iterateObject.keys(d) as Array<keyof SignalDataTypeMap>instead.As per coding guidelines, “Do not use
anyin new code. Existinganys in tests are tolerated as warnings, not invitations.”🧰 Tools
🪛 ast-grep (0.44.1)
[error] 35-37: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g.
if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const id of ids) {
if (id in bucket) out[id] = bucket[id]
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').(prototype-pollution-recursive-merge-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/Utils/auth-utils.cross-store-isolation.test.ts` around lines 35 - 46, Remove the new any usages in the test store’s get and set methods: type get<T> using SignalDataTypeMap[T], and iterate set’s input with Object.keys(d) cast to Array<keyof SignalDataTypeMap> so indexed access remains type-safe. Preserve the existing fixture behavior while enforcing the SignalDataTypeMap contract.Source: Coding guidelines
src/Utils/auth-utils.ts (1)
446-446: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use structured logging for transaction entry.
Include the lock key:
logger.trace({ key }, 'entering transaction').As per coding guidelines, “Use structured logging with objects first and message last.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Utils/auth-utils.ts` at line 446, Update the transaction-entry trace call in the surrounding transaction flow to use structured logging, passing the lock key in an object before the message while preserving the existing “entering transaction” text.Source: Coding guidelines
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
@codex Write a simple test to reproduce this bug |
3 participants
Problem
addTransactionCapabilitycreated a newAsyncLocalStorageper socket. On Node's legacy async-context propagation (Node 22, pre-AsyncContextFrame), every liveAsyncLocalStoragestamps a slot on every pending async resource, so cost growsO(live ALS instances x live async resources). Under reconnection churn each new socket adds another never-disabled instance, and a burst of pending promises/timeouts pushes the heap to OOM even with--max-old-space-size=4096.Fix
AsyncLocalStorage(one instance per process). The transaction context lives in therun(ctx, work)scope, not on the instance.Symboltoken (Map<symbol, TransactionContext>) so a wrapped store only ever resolves its own context, even when another wrapped store runs inside its transaction callback. New transactions inherit outer stores' contexts, so cross-store nesting stays isolated.transaction()and the record-scopedtransactWith()APIs.Tests
Adds
auth-utils.cross-store-isolation.test.tscovering cross-store context leakage and cross-store nesting. All existingauth-utils*suites pass (8 suites, 25 passed / 1 todo).Ported from a
master-based fix; adapted to thedeveloptransaction refactor (heldLocks,sealed,transactWith,LockManager).Summary by cubic
Stops a per-socket AsyncLocalStorage heap leak in addTransactionCapability by switching to a single process-wide storage. Preserves cross-store transaction isolation and stability under reconnection churn.
Written for commit 45177dc. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests