test: stabilize codspeed memory benchmarks - #7988
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds isolated child-process execution for memory benchmarks. It adds shared client and server registration helpers, deterministic warmup flows, scenario migrations, IPC and lifecycle tests, configuration updates, and execution-model documentation. ChangesMemory benchmark isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Vitest
participant BenchmarkRegistrar
participant IsolatedMemoryProcess
participant isolated-process-child
participant Workload
Vitest->>BenchmarkRegistrar: register benchmark
BenchmarkRegistrar->>IsolatedMemoryProcess: start isolated process
IsolatedMemoryProcess->>isolated-process-child: send run command
isolated-process-child->>Workload: execute workload
Workload-->>isolated-process-child: complete or throw
isolated-process-child-->>IsolatedMemoryProcess: return result or error
IsolatedMemoryProcess-->>BenchmarkRegistrar: resolve or reject benchmark
BenchmarkRegistrar-->>Vitest: complete benchmark lifecycle
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 3
🧹 Nitpick comments (2)
benchmarks/memory/shared/isolated-process.ts (2)
192-196: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAwait child exit in the failure path.
child.kill()only requests termination.start()returns before the child exits, so a failed start can leave a live process that competes for memory with the next run.#waitForExitalready handles the exit transition safely.♻️ Proposed change
} catch (error) { + const exit = this.#waitForExit(child) child.kill() this.#child = undefined + await exit.catch(() => {}) throw error }🤖 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 `@benchmarks/memory/shared/isolated-process.ts` around lines 192 - 196, Update the catch block in start() to await `#waitForExit` after requesting child termination, ensuring the failed child has fully exited before rethrowing the original error. Preserve the existing child reference cleanup and error propagation.
295-347: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a timeout for IPC waits.
#waitForMessagesettles only on a message, an error, or child exit. If a workload stalls,run()andstop()never settle, and the failure surfaces later as an opaque runner timeout. A bounded wait that kills the child and rejects with the workload name would make the failure diagnosable.🤖 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 `@benchmarks/memory/shared/isolated-process.ts` around lines 295 - 347, Add a bounded timeout to `#waitForMessage` so stalled IPC waits terminate deterministically. On timeout, clean up listeners, kill the child process, and reject with an error that includes the workload name; also clear the timer whenever the wait settles through a message, error, or exit.
🤖 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 `@benchmarks/memory/client/isolated-benchmark.ts`:
- Around line 31-38: Update the memory benchmark suite around isolatedProcess to
remove or neutralize the describe-level beforeEach and afterEach hooks, leaving
lifecycle management exclusively to the setup and teardown callbacks in bench
options. Preserve the existing isolatedProcess.start() and
isolatedProcess.stop() calls in those Tinybench callbacks.
In `@benchmarks/memory/server/isolated-process.test.ts`:
- Around line 24-28: Update the afterEach cleanup around runner.stop() so
environment-variable deletion and temporary-directory removal always execute in
a finally block, even when stop() rejects. Also reset the runner reference
during cleanup, using the existing runner and tempDirectory symbols.
In `@benchmarks/memory/shared/isolated-process-child.ts`:
- Around line 222-250: Update the commandQueue chain around the message handler
so failures from either the main operation or the catch-block send are contained
at every link. Reuse the handler for both fulfillment and rejection, e.g. attach
it as both callbacks to commandQueue.then, and ensure the error-reporting send
cannot leave the chain rejected so later run and stop messages continue
processing.
---
Nitpick comments:
In `@benchmarks/memory/shared/isolated-process.ts`:
- Around line 192-196: Update the catch block in start() to await `#waitForExit`
after requesting child termination, ensuring the failed child has fully exited
before rethrowing the original error. Preserve the existing child reference
cleanup and error propagation.
- Around line 295-347: Add a bounded timeout to `#waitForMessage` so stalled IPC
waits terminate deterministically. On timeout, clean up listeners, kill the
child process, and reject with an error that includes the workload name; also
clear the timer whenever the wait settles through a message, error, or exit.
🪄 Autofix
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 Plus
Run ID: fc9d0b9f-7700-4377-a465-1174a1582619
📒 Files selected for processing (64)
benchmarks/memory/README.mdbenchmarks/memory/client/isolated-benchmark.tsbenchmarks/memory/client/package.jsonbenchmarks/memory/client/scenarios/interrupted-navigations/react/memory.bench.tsbenchmarks/memory/client/scenarios/interrupted-navigations/react/vite.config.tsbenchmarks/memory/client/scenarios/interrupted-navigations/solid/memory.bench.tsbenchmarks/memory/client/scenarios/interrupted-navigations/solid/vite.config.tsbenchmarks/memory/client/scenarios/interrupted-navigations/vue/memory.bench.tsbenchmarks/memory/client/scenarios/interrupted-navigations/vue/vite.config.tsbenchmarks/memory/client/scenarios/loader-data-retention/react/memory.bench.tsbenchmarks/memory/client/scenarios/loader-data-retention/react/vite.config.tsbenchmarks/memory/client/scenarios/loader-data-retention/solid/memory.bench.tsbenchmarks/memory/client/scenarios/loader-data-retention/solid/vite.config.tsbenchmarks/memory/client/scenarios/loader-data-retention/vue/memory.bench.tsbenchmarks/memory/client/scenarios/loader-data-retention/vue/vite.config.tsbenchmarks/memory/client/scenarios/mount-unmount/react/memory.bench.tsbenchmarks/memory/client/scenarios/mount-unmount/react/vite.config.tsbenchmarks/memory/client/scenarios/mount-unmount/solid/memory.bench.tsbenchmarks/memory/client/scenarios/mount-unmount/solid/vite.config.tsbenchmarks/memory/client/scenarios/mount-unmount/vue/memory.bench.tsbenchmarks/memory/client/scenarios/mount-unmount/vue/vite.config.tsbenchmarks/memory/client/scenarios/navigation-churn/react/memory.bench.tsbenchmarks/memory/client/scenarios/navigation-churn/react/vite.config.tsbenchmarks/memory/client/scenarios/navigation-churn/solid/memory.bench.tsbenchmarks/memory/client/scenarios/navigation-churn/solid/vite.config.tsbenchmarks/memory/client/scenarios/navigation-churn/vue/memory.bench.tsbenchmarks/memory/client/scenarios/navigation-churn/vue/vite.config.tsbenchmarks/memory/client/scenarios/preload-churn/react/memory.bench.tsbenchmarks/memory/client/scenarios/preload-churn/react/vite.config.tsbenchmarks/memory/client/scenarios/preload-churn/shared.tsbenchmarks/memory/client/scenarios/preload-churn/solid/memory.bench.tsbenchmarks/memory/client/scenarios/preload-churn/solid/vite.config.tsbenchmarks/memory/client/scenarios/preload-churn/vue/memory.bench.tsbenchmarks/memory/client/scenarios/preload-churn/vue/vite.config.tsbenchmarks/memory/client/scenarios/unique-location-churn/react/memory.bench.tsbenchmarks/memory/client/scenarios/unique-location-churn/react/vite.config.tsbenchmarks/memory/client/scenarios/unique-location-churn/shared.tsbenchmarks/memory/client/scenarios/unique-location-churn/solid/memory.bench.tsbenchmarks/memory/client/scenarios/unique-location-churn/solid/vite.config.tsbenchmarks/memory/client/scenarios/unique-location-churn/vue/memory.bench.tsbenchmarks/memory/client/scenarios/unique-location-churn/vue/vite.config.tsbenchmarks/memory/client/tsconfig.jsonbenchmarks/memory/server/isolated-benchmark.tsbenchmarks/memory/server/isolated-process.test.tsbenchmarks/memory/server/package.jsonbenchmarks/memory/server/scenarios/aborted-requests/react/memory.bench.tsbenchmarks/memory/server/scenarios/aborted-requests/solid/memory.bench.tsbenchmarks/memory/server/scenarios/aborted-requests/vue/memory.bench.tsbenchmarks/memory/server/scenarios/error-paths/react/memory.bench.tsbenchmarks/memory/server/scenarios/error-paths/shared.tsbenchmarks/memory/server/scenarios/error-paths/solid/memory.bench.tsbenchmarks/memory/server/scenarios/error-paths/vue/memory.bench.tsbenchmarks/memory/server/scenarios/request-churn/react/memory.bench.tsbenchmarks/memory/server/scenarios/request-churn/shared.tsbenchmarks/memory/server/scenarios/request-churn/solid/memory.bench.tsbenchmarks/memory/server/scenarios/request-churn/vue/memory.bench.tsbenchmarks/memory/server/scenarios/server-fn-churn/react/memory.bench.tsbenchmarks/memory/server/scenarios/server-fn-churn/solid/memory.bench.tsbenchmarks/memory/server/scenarios/server-fn-churn/vue/memory.bench.tsbenchmarks/memory/server/test-fixtures/isolated-process-setup.tsbenchmarks/memory/server/tsconfig.jsonbenchmarks/memory/shared/isolated-process-child.tsbenchmarks/memory/shared/isolated-process.tsbenchmarks/memory/shared/tsconfig.json
| describe('memory', () => { | ||
| beforeEach(() => isolatedProcess.start()) | ||
| afterEach(() => isolatedProcess.stop()) | ||
|
|
||
| bench(options.name, run, { | ||
| ...memoryBenchOptions, | ||
| setup: () => isolatedProcess.start(), | ||
| teardown: () => isolatedProcess.stop(), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For Vitest 4.1.4 benchmark files, verify whether suite-level beforeEach/afterEach hooks run around bench() tasks and whether Tinybench setup/teardown run for each benchmark cycle.
💡 Result:
In Vitest 4.1.4, the behavior of suite-level hooks and Tinybench setup/teardown in benchmark files is as follows: Suite-Level Hooks (beforeEach/afterEach) Historically, Vitest's suite-level beforeEach and afterEach hooks did not run for bench tasks [1]. While there have been community efforts and pull requests (such as PR #7541) aimed at enabling this functionality, these hooks are generally not standardly supported or guaranteed to execute around benchmark tasks in the same way they do for test blocks [1][2][3]. Benchmark execution in Vitest has historically followed a distinct lifecycle from standard tests [1][3]. Tinybench Setup/Teardown Tinybench, which powers Vitest's benchmarking, explicitly provides setup and teardown options [4][5][6]. These are configured as part of the third argument (options object) passed to the bench function [4][5]. According to Tinybench documentation, these setup and teardown functions are designed to run for each benchmark cycle (task execution) [6][7][8]. They are intended for operations that need to occur immediately before and after the specific code being measured, allowing users to handle stateful setups that should not be included in the timed performance result [3][6]. In summary, you should rely on the explicit setup and teardown options provided in the bench function's configuration object to manage cycle-specific logic, rather than relying on suite-level beforeEach/afterEach hooks [3][4][6]. Top Results: [1][2][3][4][6]
Citations:
- 1: Support hooks in
benchAPI vitest-dev/vitest#5075 - 2: feat: add hooks in bench mode vitest-dev/vitest#7541
- 3: Expose
beforeEachandafterEachhooks from tinybenchTaskvitest-dev/vitest#7599 - 4: https://vitest.dev/api/test
- 5: https://main.vitest.dev/api/test
- 6: https://tinylibs.github.io/tinybench/interfaces/BenchOptions.html
- 7: https://tinylibs.github.io/tinybench/classes/Bench.html
- 8: https://tinylibs.github.io/tinybench/interfaces/ResolvedBenchOptions.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files:"
fd -a 'isolated-benchmark\.ts$|isolated.*memory.*\.ts$|memory.*\.ts$' . | sed 's#^\./##' | head -100
echo
echo "Target file outline:"
ast-grep outline benchmarks/memory/client/isolated-benchmark.ts --view compact || true
echo
echo "Target file lines:"
cat -n benchmarks/memory/client/isolated-benchmark.ts | sed -n '1,120p'
echo
echo "Search isolated process definitions/usages:"
rg -n "class IsolatedMemoryProcess|IsolatedMemoryProcess|start\(|stop\(|isolatedProcess|bench\\(" -S benchmarks . --glob '*.ts' --glob '!node_modules' | head -200Repository: TanStack/router
Length of output: 25556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Vitest version declarations:"
for f in package.json pnpm-lock.yaml yarn.lock package-lock.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
if [ "$f" = "package.json" ]; then
node -e "const p=require('./package.json'); console.log({devDependencies:p.devDependencies?.vitest, dependencies:p.dependencies?.vitest, scripts:p.scripts});" 2>/dev/null || sed -n '/vitest/p' "$f"
else
rg -n "vitest(@|:)|version:" "$f" | head -80
fi
fi
done
echo
echo "Shared process start/stop implementation:"
cat -n benchmarks/memory/shared/isolated-process.ts | sed -n '140,235p'
echo
echo "Isolated process tests around duplicate start:"
cat -n benchmarks/memory/server/isolated-process.test.ts | sed -n '1,130p'
echo
echo "Memory bench options:"
cat -n benchmarks/memory/client/bench-utils.ts | sed -n '1,120p'
echo
echo "Server isolated benchmark counterpart:"
cat -n benchmarks/memory/server/isolated-benchmark.ts | sed -n '1,70p'Repository: TanStack/router
Length of output: 10199
Use Tinybench setup/teardown for the isolated lifecycle.
In Vitest 4 benchmark mode, keep the setup and teardown in the bench() options, but remove the suite beforeEach/afterEach hooks or make them no-ops. The suite hooks can start the process before bench() setup/teardown runs, while IsolatedMemoryProcess.start() rejects a second start because a child process already exists.
🤖 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 `@benchmarks/memory/client/isolated-benchmark.ts` around lines 31 - 38, Update
the memory benchmark suite around isolatedProcess to remove or neutralize the
describe-level beforeEach and afterEach hooks, leaving lifecycle management
exclusively to the setup and teardown callbacks in bench options. Preserve the
existing isolatedProcess.start() and isolatedProcess.stop() calls in those
Tinybench callbacks.
| afterEach(async () => { | ||
| await runner?.stop() | ||
| delete process.env.TSR_MEMORY_ISOLATION_TEST_LOG | ||
| await rm(tempDirectory, { recursive: true }) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make cleanup independent of stop() success.
If runner.stop() rejects, afterEach skips the env-var deletion and the temp-directory removal. The stale TSR_MEMORY_ISOLATION_TEST_LOG value then leaks into later tests, and temp directories accumulate. Run the cleanup in a finally block, and reset runner.
🧹 Proposed fix
afterEach(async () => {
- await runner?.stop()
- delete process.env.TSR_MEMORY_ISOLATION_TEST_LOG
- await rm(tempDirectory, { recursive: true })
+ try {
+ await runner?.stop()
+ } finally {
+ runner = undefined
+ delete process.env.TSR_MEMORY_ISOLATION_TEST_LOG
+ await rm(tempDirectory, { recursive: true, force: true })
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterEach(async () => { | |
| await runner?.stop() | |
| delete process.env.TSR_MEMORY_ISOLATION_TEST_LOG | |
| await rm(tempDirectory, { recursive: true }) | |
| }) | |
| afterEach(async () => { | |
| try { | |
| await runner?.stop() | |
| } finally { | |
| runner = undefined | |
| delete process.env.TSR_MEMORY_ISOLATION_TEST_LOG | |
| await rm(tempDirectory, { recursive: true, force: true }) | |
| } | |
| }) |
🤖 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 `@benchmarks/memory/server/isolated-process.test.ts` around lines 24 - 28,
Update the afterEach cleanup around runner.stop() so environment-variable
deletion and temporary-directory removal always execute in a finally block, even
when stop() rejects. Also reset the runner reference during cleanup, using the
existing runner and tempDirectory symbols.
Merging this PR will degrade performance by 52.65%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem client loader-data-retention (solid) |
153.7 KB | 1,495.8 KB | -89.72% |
| ❌ | Memory | mem client loader-data-retention (react) |
155.7 KB | 1,463.6 KB | -89.36% |
| ❌ | Memory | mem client interrupted-navigations (react) |
248.9 KB | 2,084.2 KB | -88.06% |
| ❌ | Memory | mem client interrupted-navigations (solid) |
341.4 KB | 2,104.5 KB | -83.78% |
| ❌ | Memory | mem client navigation-churn (react) |
443.6 KB | 2,484.5 KB | -82.14% |
| ❌ | Memory | mem client interrupted-navigations (vue) |
500.3 KB | 2,708.6 KB | -81.53% |
| ❌ | Memory | mem client unique-location-churn (solid) |
516.8 KB | 2,796 KB | -81.52% |
| ❌ | Memory | mem client navigation-churn (solid) |
540.4 KB | 2,572.4 KB | -78.99% |
| ❌ | Memory | mem client preload-churn (solid) |
312.1 KB | 1,092.1 KB | -71.42% |
| ❌ | Memory | mem client mount-unmount (solid) |
477.2 KB | 1,501.2 KB | -68.21% |
| ❌ | Memory | mem server error-paths redirect (react) |
208.5 KB | 646.4 KB | -67.74% |
| ❌ | Memory | mem client unique-location-churn (vue) |
986.6 KB | 2,863.2 KB | -65.54% |
| ❌ | Memory | mem client unique-location-churn (react) |
962.5 KB | 2,790.2 KB | -65.5% |
| ❌ | Memory | mem server error-paths not-found (react) |
255.9 KB | 663.3 KB | -61.41% |
| ❌ | Memory | mem server aborted-requests (react) |
564.1 KB | 1,425.5 KB | -60.43% |
| ❌ | Memory | mem server error-paths unmatched (react) |
262.3 KB | 661.7 KB | -60.36% |
| ❌ | Memory | mem client preload-churn (vue) |
738.3 KB | 1,764.8 KB | -58.17% |
| ❌ | Memory | mem server error-paths redirect (solid) |
278.2 KB | 646.7 KB | -56.98% |
| ❌ | Memory | mem server request-churn (solid) |
414.6 KB | 952.5 KB | -56.47% |
| ❌ | Memory | mem client navigation-churn (vue) |
1.2 MB | 2.5 MB | -50.63% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing test-codspeed-memory-stabilization (ea7c024) with main (2265129)
|
View your CI Pipeline Execution ↗ for commit ea7c024
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@benchmarks/memory/client/benchmark.ts`:
- Around line 10-19: Move the before/after pairing validation in
warmClientMemoryWorkload before the !workload.warmup early return, so workloads
without warmup still require both lifecycle hooks or neither. Preserve the
existing error and return behavior after validation.
🪄 Autofix
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 Plus
Run ID: a0de7bae-2d8f-4949-a3a1-f217d2619dc5
📒 Files selected for processing (19)
benchmarks/memory/README.mdbenchmarks/memory/client/benchmark.tsbenchmarks/memory/client/flame-runner.tsbenchmarks/memory/client/scenarios/interrupted-navigations/shared.tsbenchmarks/memory/client/scenarios/loader-data-retention/shared.tsbenchmarks/memory/client/scenarios/mount-unmount/shared.tsbenchmarks/memory/client/scenarios/navigation-churn/shared.tsbenchmarks/memory/client/scenarios/preload-churn/shared.tsbenchmarks/memory/client/scenarios/unique-location-churn/shared.tsbenchmarks/memory/server/benchmark.tsbenchmarks/memory/server/flame-runner.tsbenchmarks/memory/server/isolated-process.test.tsbenchmarks/memory/server/scenarios/aborted-requests/shared.tsbenchmarks/memory/server/scenarios/error-paths/shared.tsbenchmarks/memory/server/scenarios/request-churn/shared.tsbenchmarks/memory/server/scenarios/server-fn-churn/shared.tsbenchmarks/memory/server/test-fixtures/isolated-process-setup.tsbenchmarks/memory/shared/isolated-process-child.tsbenchmarks/memory/shared/isolated-process.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- benchmarks/memory/shared/isolated-process-child.ts
| export async function warmClientMemoryWorkload(workload: ClientMemoryWorkload) { | ||
| if (!workload.warmup) { | ||
| return | ||
| } | ||
|
|
||
| if (Boolean(workload.before) !== Boolean(workload.after)) { | ||
| throw new Error( | ||
| `Client memory workload ${workload.name} must define both before and after when it defines either hook`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate paired lifecycle hooks before the early return.
Line 11 bypasses the before and after pairing check when warmup is absent. A workload with only before can then create measured state without cleanup. Validate the pair before returning.
Proposed fix
export async function warmClientMemoryWorkload(workload: ClientMemoryWorkload) {
- if (!workload.warmup) {
- return
- }
-
if (Boolean(workload.before) !== Boolean(workload.after)) {
throw new Error(
`Client memory workload ${workload.name} must define both before and after when it defines either hook`,
)
}
+ if (!workload.warmup) {
+ return
+ }
+
await workload.before?.()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function warmClientMemoryWorkload(workload: ClientMemoryWorkload) { | |
| if (!workload.warmup) { | |
| return | |
| } | |
| if (Boolean(workload.before) !== Boolean(workload.after)) { | |
| throw new Error( | |
| `Client memory workload ${workload.name} must define both before and after when it defines either hook`, | |
| ) | |
| } | |
| export async function warmClientMemoryWorkload(workload: ClientMemoryWorkload) { | |
| if (Boolean(workload.before) !== Boolean(workload.after)) { | |
| throw new Error( | |
| `Client memory workload ${workload.name} must define both before and after when it defines either hook`, | |
| ) | |
| } | |
| if (!workload.warmup) { | |
| return | |
| } |
🤖 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 `@benchmarks/memory/client/benchmark.ts` around lines 10 - 19, Move the
before/after pairing validation in warmClientMemoryWorkload before the
!workload.warmup early return, so workloads without warmup still require both
lifecycle hooks or neither. Preserve the existing error and return behavior
after validation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/memory/shared/isolated-process.ts (1)
224-227: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for child exit when startup fails.
child.kill()only requests termination. The method clears#childand rejects before the child exits. A laterstart()can overlap with the failed child process and affect benchmark memory results.Register
#waitForExit(child)before killing the child. Await its settlement before rethrowing the startup error.Proposed fix
} catch (error) { - child.kill() this.#child = undefined + const exit = this.#waitForExit(child) + if (child.exitCode === null && child.signalCode === null) { + child.kill() + } + await exit.catch(() => {}) throw error }🤖 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 `@benchmarks/memory/shared/isolated-process.ts` around lines 224 - 227, Update the startup failure catch block around `#waitForExit` and child.kill() to register the child-exit wait before requesting termination, then await its settlement before clearing `#child` and rethrowing the original startup error. Preserve the existing cleanup and rejection behavior after the child has exited.
🤖 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.
Outside diff comments:
In `@benchmarks/memory/shared/isolated-process.ts`:
- Around line 224-227: Update the startup failure catch block around
`#waitForExit` and child.kill() to register the child-exit wait before requesting
termination, then await its settlement before clearing `#child` and rethrowing the
original startup error. Preserve the existing cleanup and rejection behavior
after the child has exited.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b0364019-16a7-4e43-a733-c1dab9e99532
📒 Files selected for processing (5)
benchmarks/memory/README.mdbenchmarks/memory/server/isolated-process.test.tsbenchmarks/memory/server/test-fixtures/isolated-process-setup.tsbenchmarks/memory/shared/isolated-process-child.tsbenchmarks/memory/shared/isolated-process.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- benchmarks/memory/server/test-fixtures/isolated-process-setup.ts
- benchmarks/memory/server/isolated-process.test.ts
1 participant
Summary by CodeRabbit
Benchmark Improvements
Documentation
Tests