v3.0.2: run shards rediscovery in separate traces - #4
Conversation
Pull Request: #4 (main)
fedc5d5 to
dffefaf
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. 📝 WalkthroughWalkthroughThis PR introduces async trace isolation for ChangesAsync Trace Isolation for Cached Refresh Operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal/CachedRefreshedValue.ts (1)
169-183:⚠️ Potential issue | 🟠 MajorDetach
deps.handler()insidedepsTimeoutBodyto prevent caller trace leakage
depsTimeoutBody()(lines ~169-183) schedules asetTimeoutcallback that runsthis.options.deps.handler()viarunInVoid(...)only, without usingmaybeRunInSeparateTrace(...). As a result, timer-driven dependency rechecks can keep inheriting the AsyncLocalStorage trace context of whoever startedrefreshLoop, even though the subsequent refresh’sdepsPrev+resolverFn()are detached whenkeepCallerTraceis false. Wrap thedeps.handler()call in the same detached runner used for automatic refreshes.🤖 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/internal/CachedRefreshedValue.ts` around lines 169 - 183, depsTimeoutBody currently calls this.options.deps.handler() inside runInVoid, causing timer-driven dependency checks to inherit the caller AsyncLocalStorage trace; modify depsTimeoutBody (the function assigned to depsTimeoutBody) to invoke the deps handler using the same detached runner used elsewhere (i.e., call maybeRunInSeparateTrace or the existing detached wrapper used for resolverFn/automatic refreshes) so that when keepCallerTrace is false the deps.handler() runs detached; preserve existing try/catch/finally behavior and still call this.onError(...) and setTimeout(...).unref() as before.
🤖 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/internal/CachedRefreshedValue.ts`:
- Around line 114-122: The refresh logic incorrectly uses the loop's current
AsyncLocalStorage context for the "next pass" when refreshAndWait() sets
skipDelay/keepCallerTrace; fix by capturing the caller's async context when
refreshAndWait() is invoked and reuse that context for the next resolver pass
(modify refreshAndWait() to store the caller context instead of just flipping
keepCallerTrace, and adjust refreshLoop()/maybeRunInSeparateTrace() to accept
and run the next pass under that captured context via AsyncLocalStorage.run or
equivalent). Also ensure depsTimeoutBody calls this.options.deps.handler()
through maybeRunInSeparateTrace configured to run in a fresh/detached trace (so
timer-driven deps checks never inherit the loop starter's trace).
In `@src/internal/misc.ts`:
- Around line 403-420: The current code captures AsyncLocalStorage.snapshot() at
module load into runInRootAsyncContext, which can freeze whatever async context
is active during import; change it so snapshot() is invoked at use-time instead
of at module init: replace the frozen const runInRootAsyncContext value with a
function that, when called, checks typeof AsyncLocalStorage?.snapshot ===
"function" and calls AsyncLocalStorage.snapshot() to obtain a fresh detached
runner (falling back to running func() inline if snapshot is unavailable), and
update maybeRunInSeparateTrace to call this function each time when
keepCallerTrace is false so background work never replays the import-time
context.
---
Outside diff comments:
In `@src/internal/CachedRefreshedValue.ts`:
- Around line 169-183: depsTimeoutBody currently calls
this.options.deps.handler() inside runInVoid, causing timer-driven dependency
checks to inherit the caller AsyncLocalStorage trace; modify depsTimeoutBody
(the function assigned to depsTimeoutBody) to invoke the deps handler using the
same detached runner used elsewhere (i.e., call maybeRunInSeparateTrace or the
existing detached wrapper used for resolverFn/automatic refreshes) so that when
keepCallerTrace is false the deps.handler() runs detached; preserve existing
try/catch/finally behavior and still call this.onError(...) and
setTimeout(...).unref() as before.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19bec214-97ce-4611-b970-69ab1aeb6de5
📒 Files selected for processing (6)
.coderabbit.yaml.vscode/settings.jsonpackage.jsonsrc/internal/CachedRefreshedValue.tssrc/internal/__tests__/CachedRefreshedValue.test.tssrc/internal/misc.ts
| // Whether the upcoming resolverFn() call must run in the caller's async | ||
| // context (and thus inherit its APM/tracing trace id), or in a fresh, | ||
| // detached trace. The very 1st refresh is triggered synchronously by the | ||
| // very 1st cached() caller (e.g. the 1st query), so it intentionally stays | ||
| // in that caller's trace. Same for refreshes triggered manually via | ||
| // refreshAndWait() (e.g. Cluster#rediscover()). But refreshes triggered on | ||
| // the timer (or by a deps change) must NOT pollute the trace of whoever | ||
| // happened to start the loop - each such refresh gets its own fresh trace. | ||
| let keepCallerTrace = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant regions of the file with line numbers
FILE="src/internal/CachedRefreshedValue.ts"
echo "=== File: $FILE (around keepCallerTrace) ==="
nl -ba "$FILE" | sed -n '80,220p'
echo "=== Search for keepCallerTrace / skipDelay / refreshAndWait / maybeRunInSeparateTrace usage ==="
rg -n "keepCallerTrace|skipDelay|refreshAndWait|refreshLoop|depsTimeoutBody|maybeRunInSeparateTrace" "$FILE"
echo "=== Print surrounding definitions for skipDelay and refreshAndWait if found ==="
# Print a wider window around skipDelay and refreshAndWait occurrences
for pat in "skipDelay" "refreshAndWait" "refreshLoop" "depsTimeoutBody" "maybeRunInSeparateTrace"; do
echo "--- Pattern: $pat ---"
line=$(rg -n "$pat" "$FILE" | head -n 1 | cut -d: -f1 || true)
if [[ -n "${line:-}" ]]; then
start=$((line-40))
end=$((line+80))
nl -ba "$FILE" | sed -n "${start},${end}p"
fi
doneRepository: dimikot/ent-framework
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/internal/CachedRefreshedValue.ts"
echo "=== Search for key identifiers ==="
rg -n "keepCallerTrace|skipDelay|refreshAndWait|refreshLoop|depsTimeoutBody|maybeRunInSeparateTrace" "$FILE"
echo "=== Numbered excerpt around the keepCallerTrace definition (fallback: 1-260) ==="
# Limit output to avoid huge dumps; adjust if file is smaller
wc -l "$FILE"
START=1
END=260
awk -v s="$START" -v e="$END" '{ if(NR>=s && NR<=e) printf "%6d\t%s\n", NR, $0 }' "$FILE"Repository: dimikot/ent-framework
Length of output: 11768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "function maybeRunInSeparateTrace|maybeRunInSeparateTrace" src/internal -S
ls -la src/internalRepository: dimikot/ent-framework
Length of output: 1102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "maybeRunInSeparateTrace" src/internal -SRepository: dimikot/ent-framework
Length of output: 424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/internal/misc.ts"
# Show around maybeRunInSeparateTrace
start=400
end=520
awk -v s="$start" -v e="$end" '{ if(NR>=s && NR<=e) printf "%6d\t%s\n", NR, $0 }' "$FILE"Repository: dimikot/ent-framework
Length of output: 3932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/internal/misc.ts"
rg -n "runInVoid" "$FILE"Repository: dimikot/ent-framework
Length of output: 96
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/internal/misc.ts"
awk 'NR>=360 && NR<=460 { printf "%6d\t%s\n", NR, $0 }' "$FILE"Repository: dimikot/ent-framework
Length of output: 4620
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'NODE'
const { AsyncLocalStorage } = require('async_hooks');
const als = new AsyncLocalStorage();
function defer() {
let resolve;
const promise = new Promise(r => { resolve = r; });
return { promise, resolve };
}
async function test({label}) {
const d = defer();
let createdIn;
let resolvedIn;
await als.run({id: 'A'}, async () => {
createdIn = als.getStore().id;
// resolve promise later, from another ALS context
});
await new Promise(res => setImmediate(res)); // ensure A context unwound
await als.run({id: 'B'}, async () => {
resolvedIn = als.getStore().id;
d.resolve();
});
const resumedId = await d.promise.then(() => als.getStore()?.id ?? null);
return {label, createdIn, resolvedIn, resumedId};
}
(async () => {
const out = [];
out.push(await test({label: 'promise-resolve-in-B'}));
console.log(JSON.stringify(out, null, 2));
})();
NODERepository: dimikot/ent-framework
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'NODE'
const { AsyncLocalStorage } = require('async_hooks');
const als = new AsyncLocalStorage();
function defer() {
let resolve;
const promise = new Promise(r => { resolve = r; });
return { promise, resolve };
}
const d = defer();
async function awaitingInA() {
let before, after;
await als.run({id:'A'}, async () => {
before = als.getStore().id;
await d.promise; // continuation should run with A if ALS is propagated via promise creation
after = als.getStore().id;
});
return { before, after };
}
(async () => {
const waiter = awaitingInA();
// let waiter start and reach await
await new Promise(r => setImmediate(r));
await als.run({id:'B'}, async () => {
d.resolve();
});
const res = await waiter;
console.log(JSON.stringify(res, null, 2));
})();
NODERepository: dimikot/ent-framework
Length of output: 101
Fix: refreshAndWait() manual refresh should run the next pass in the requesting caller’s trace, not the loop starter’s trace.
skipDelayonly flips thekeepCallerTraceboolean (lines 164-167). WhenkeepCallerTracebecomestrue,maybeRunInSeparateTrace(true)just runs inline (func()), so the next resolver pass still executes inrefreshLoop()’s current AsyncLocalStorage context (the loop starter), not the caller that invokedrefreshAndWait(). A regression should show two distinct trace IDs when two callers interact with the same in-flight loop (starting in caller A, triggeringrefreshAndWait()from caller B).- Similarly, deps-change polling (
depsTimeoutBodyat lines 169-183) callsthis.options.deps.handler()withoutmaybeRunInSeparateTrace, so timer-driven deps checks can also attach to the loop starter’s trace despite the comment’s “timer/deps change must NOT pollute” intent.
🤖 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/internal/CachedRefreshedValue.ts` around lines 114 - 122, The refresh
logic incorrectly uses the loop's current AsyncLocalStorage context for the
"next pass" when refreshAndWait() sets skipDelay/keepCallerTrace; fix by
capturing the caller's async context when refreshAndWait() is invoked and reuse
that context for the next resolver pass (modify refreshAndWait() to store the
caller context instead of just flipping keepCallerTrace, and adjust
refreshLoop()/maybeRunInSeparateTrace() to accept and run the next pass under
that captured context via AsyncLocalStorage.run or equivalent). Also ensure
depsTimeoutBody calls this.options.deps.handler() through
maybeRunInSeparateTrace configured to run in a fresh/detached trace (so
timer-driven deps checks never inherit the loop starter's trace).
| /** | ||
| * A snapshot of the "clean" async context, captured at module load time, i.e. | ||
| * BEFORE any request-scoped async context (such as an incoming HTTP request, and | ||
| * thus the APM/tracing "trace id" that tracers propagate through | ||
| * AsyncLocalStorage) could possibly have been established. Calling the returned | ||
| * runner executes a callback within that captured clean context. | ||
| * | ||
| * We deliberately use AsyncLocalStorage.snapshot() and NOT AsyncResource: both | ||
| * can detach an async context, but AsyncLocalStorage (including .snapshot()) is | ||
| * supported across runtimes (Node, Deno, Bun), whereas AsyncResource is a | ||
| * non-functional stub in some of them. If .snapshot() is unavailable (very old | ||
| * runtimes), we degrade gracefully to running the callback inline (no | ||
| * detachment). | ||
| */ | ||
| const runInRootAsyncContext: <T>(func: () => T) => T = | ||
| typeof AsyncLocalStorage?.snapshot === "function" | ||
| ? AsyncLocalStorage.snapshot() | ||
| : (func) => func(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant region around the snippet and call sites
ls -la
sed -n '360,520p' src/internal/misc.ts | nl -ba | sed -n '1,220p'
# Find all references to runInRootAsyncContext / maybeRunInSeparateTrace
rg -n "runInRootAsyncContext|maybeRunInSeparateTrace|AsyncLocalStorage\.snapshot" -S src/internal/misc.ts src/internal -g'*.ts'Repository: dimikot/ent-framework
Length of output: 2381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant region with line numbers (cat -n)
sed -n '360,520p' src/internal/misc.ts | cat -n
# Search for the key symbols/usages across the repo (scoped to TS)
rg -n "runInRootAsyncContext|maybeRunInSeparateTrace|AsyncLocalStorage\.snapshot" -S src -g'*.ts'Repository: dimikot/ent-framework
Length of output: 6187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/internal/misc.ts | cat -n
sed -n '380,470p' src/internal/misc.ts | cat -n
sed -n '120,210p' src/internal/CachedRefreshedValue.ts | cat -nRepository: dimikot/ent-framework
Length of output: 13354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all usages of maybeRunInSeparateTrace and where misc.ts is imported
rg -n "maybeRunInSeparateTrace\\(" -S src -g'*.ts'
rg -n "import\\s*\\{\\s*maybeRunInSeparateTrace\\b" -S src -g'*.ts'
# Search for AsyncLocalStorage instantiation / usage across the repo
rg -n "new\\s+AsyncLocalStorage\\b|AsyncLocalStorage\\." -S src -g'*.ts'Repository: dimikot/ent-framework
Length of output: 175
🌐 Web query:
Node.js AsyncLocalStorage snapshot is it static or instance method
💡 Result:
The AsyncLocalStorage.snapshot method is a static method in Node.js [1][2][3]. It captures the current execution context and returns a function that, when called, executes the provided function within that captured context [1][4]. It is designed to be called directly on the AsyncLocalStorage class, rather than on an instance of the class [1][5].
Citations:
- 1: https://nodejs.org/api/async_context.html
- 2: https://nodejs.org/api/async_context.md
- 3: https://github.com/nodejs/node/blob/main/doc/api/async_context.md
- 4: https://beta.docs.nodejs.org/async_context.html
- 5: https://nodejs.org/dist/latest/docs/api/async_context.html
Don’t freeze the “clean” detached AsyncLocalStorage runner at module init
runInRootAsyncContext is initialized at module evaluation time by calling AsyncLocalStorage.snapshot() and then reused for every detached call (maybeRunInSeparateTrace(..., ...) uses it when keepCallerTrace is false). If src/internal/misc.ts is first imported while a request-scoped trace store is active, the “clean” runner will replay that active request context (e.g., trace id "A"), defeating the detachment goal and pinning background work to the wrong trace.
Regression idea: import src/internal/misc.ts inside als.run({ traceId: "A" }, () => /* dynamic import */) and assert that maybeRunInSeparateTrace(false, async () => ...) does not observe "A".
🤖 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/internal/misc.ts` around lines 403 - 420, The current code captures
AsyncLocalStorage.snapshot() at module load into runInRootAsyncContext, which
can freeze whatever async context is active during import; change it so
snapshot() is invoked at use-time instead of at module init: replace the frozen
const runInRootAsyncContext value with a function that, when called, checks
typeof AsyncLocalStorage?.snapshot === "function" and calls
AsyncLocalStorage.snapshot() to obtain a fresh detached runner (falling back to
running func() inline if snapshot is unavailable), and update
maybeRunInSeparateTrace to call this function each time when keepCallerTrace is
false so background work never replays the import-time context.
f671c9f
into
grok/dimikot/move-mit-source-code-and-api-docs-into-this-repo-to-main-014e
1 participant
PRs in the Stack
(The stack is managed by git-grok.)
Summary by CodeRabbit
Chores
Internal Improvements