fix: fail benchmark reindex when indexer exits non-zero#117
Conversation
Check child exit code and surface stderr instead of recording timings for failed indexer runs.
🦋 Changeset detectedLatest commit: d378b76 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extracts reindex benchmarking logic into a standalone, tested module with fail-fast error handling. ChangesBenchmark reindex extraction and integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
🧹 Nitpick comments (1)
src/benchmark-reindex.test.ts (1)
24-36: ⚡ Quick winAdd boundary tests for invalid
runsvalues.Current tests miss
runsvalidation behavior (0, negative, non-integer), so this regression path is not locked.🧪 Suggested tests
describe("runBenchmarkReindex", () => { + it("rejects invalid runs values", async () => { + const spawnIndexer = async () => ({ exitCode: 0, stderr: "", stdout: "" }); + await expect( + runBenchmarkReindex("bad-runs-zero", [], { runs: 0, spawnIndexer }), + ).rejects.toThrow(/runs >= 1/); + await expect( + runBenchmarkReindex("bad-runs-float", [], { runs: 1.5, spawnIndexer }), + ).rejects.toThrow(/runs >= 1/); + });🤖 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/benchmark-reindex.test.ts` around lines 24 - 36, Add boundary tests for runBenchmarkReindex to assert validation for invalid `runs` inputs (0, negative, and non-integer). Update or add test cases in src/benchmark-reindex.test.ts that call runBenchmarkReindex("label", [], { runs: X, spawnIndexer: ... }) with runs set to 0, -1, and a non-integer (e.g., 1.5) and assert the function either throws or returns a validation error; reference the runBenchmarkReindex helper to implement these assertions and mirror existing test patterns (use async/await and expect(...).toThrow or expect(...).toMatchObject depending on current error behavior).
🤖 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/benchmark-reindex.ts`:
- Around line 7-25: Add JSDoc for the exported API: document IndexerSpawnResult
(fields exitCode, stderr, stdout and what null exitCode means), IndexerSpawn
(contract: accepts args array, returns a Promise resolving to IndexerSpawnResult
and may reject on spawn failure), and runBenchmarkReindex (describe label, args,
opts.spawnIndexer, opts.runs default behavior, that runs is number of iterations
used to compute avg/min/max, and how failures from spawnIndexer are propagated
or handled). Place the comments immediately above the exported declarations
(IndexerSpawnResult, IndexerSpawn, and runBenchmarkReindex) and keep them
concise, specifying return shape and failure semantics.
- Around line 26-42: The code uses opts.runs directly which may be 0, negative,
or non-integer and leads to invalid stats; normalize and validate it first
(e.g., compute let runsNormalized = Number.isFinite(Number(opts.runs)) ?
Math.floor(Number(opts.runs)) : /* default */ 3, then if runsNormalized < 1 set
to 1 or throw). Replace uses of runs with runsNormalized in the loop and in the
avg calculation, ensuring timeMsAsync/spawnIndexer logic and the times array
remain unchanged; reference the variables/functions runs/opts.runs,
runsNormalized, timeMsAsync, spawnIndexer, times, avg, min, max.
---
Nitpick comments:
In `@src/benchmark-reindex.test.ts`:
- Around line 24-36: Add boundary tests for runBenchmarkReindex to assert
validation for invalid `runs` inputs (0, negative, and non-integer). Update or
add test cases in src/benchmark-reindex.test.ts that call
runBenchmarkReindex("label", [], { runs: X, spawnIndexer: ... }) with runs set
to 0, -1, and a non-integer (e.g., 1.5) and assert the function either throws or
returns a validation error; reference the runBenchmarkReindex helper to
implement these assertions and mirror existing test patterns (use async/await
and expect(...).toThrow or expect(...).toMatchObject depending on current error
behavior).
🪄 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: afa8f2cb-9ef2-49b0-b11b-9761a8b4e9ad
📒 Files selected for processing (4)
.changeset/fix-benchmark-reindex-exit-code.mdsrc/benchmark-reindex.test.tssrc/benchmark-reindex.tssrc/benchmark.ts
| export interface IndexerSpawnResult { | ||
| exitCode: number | null; | ||
| stderr: string; | ||
| stdout: string; | ||
| } | ||
|
|
||
| export type IndexerSpawn = (args: string[]) => Promise<IndexerSpawnResult>; | ||
|
|
||
| export async function runBenchmarkReindex( | ||
| label: string, | ||
| args: string[], | ||
| opts: { spawnIndexer: IndexerSpawn; runs?: number }, | ||
| ): Promise<{ | ||
| label: string; | ||
| avg: number; | ||
| min: number; | ||
| max: number; | ||
| runs: number; | ||
| }> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the exported benchmark API surface.
The exported interface/type/function are public API but currently have no accompanying docs. Add concise JSDoc that captures intent/contract (especially failure behavior and runs semantics).
📝 Proposed doc additions
export interface IndexerSpawnResult {
exitCode: number | null;
stderr: string;
stdout: string;
}
+/** Spawns an indexer process for the provided CLI arguments. */
export type IndexerSpawn = (args: string[]) => Promise<IndexerSpawnResult>;
+/**
+ * Benchmarks repeated reindex runs and returns aggregate timing stats.
+ * Throws when any run exits non-zero to avoid reporting failed runs as timings.
+ */
export async function runBenchmarkReindex(As per coding guidelines, "**/*.{ts,tsx,js,jsx}: All public APIs must have accompanying documentation".
🤖 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/benchmark-reindex.ts` around lines 7 - 25, Add JSDoc for the exported
API: document IndexerSpawnResult (fields exitCode, stderr, stdout and what null
exitCode means), IndexerSpawn (contract: accepts args array, returns a Promise
resolving to IndexerSpawnResult and may reject on spawn failure), and
runBenchmarkReindex (describe label, args, opts.spawnIndexer, opts.runs default
behavior, that runs is number of iterations used to compute avg/min/max, and how
failures from spawnIndexer are propagated or handled). Place the comments
immediately above the exported declarations (IndexerSpawnResult, IndexerSpawn,
and runBenchmarkReindex) and keep them concise, specifying return shape and
failure semantics.
There was a problem hiding this comment.
Skipping JSDoc here — this is an internal benchmark helper (benchmark.ts + tests only), not package public API. Names and types are self-explanatory; we avoid signature-restating comments on small src/ utilities per project comment discipline.
There was a problem hiding this comment.
@SutuSebastian, understood — the export keyword here is for testability, not public package API surface, and the names+types speak for themselves. Happy to defer to the project's comment discipline. I'll note this for future reviews.
( ˘︹˘ ) 🐇
✏️ Learnings added
Learnt from: SutuSebastian
Repo: stainless-code/codemap PR: 117
File: src/benchmark-reindex.ts:7-25
Timestamp: 2026-05-20T15:16:31.042Z
Learning: In stainless-code/codemap, exported symbols in internal benchmark helpers (e.g., `IndexerSpawnResult`, `IndexerSpawn`, `runBenchmarkReindex` in `src/benchmark-reindex.ts`) do not require JSDoc. The module is consumed only by `src/benchmark.ts` and tests — `export` is used for testability, not to expose a public package API. Per project comment discipline, JSDoc is omitted when names and types are already self-explanatory, consistent with the `.agents/rules/concise-comments.md` rule.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SutuSebastian
Repo: stainless-code/codemap PR: 75
File: src/parsed-types.ts:37-37
Timestamp: 2026-05-05T20:23:01.334Z
Learning: In the stainless-code/codemap repository, follow the codebase JSDoc conciseness rule (see `.agents/rules/concise-comments.md`): only add JSDoc to fields/functions when the behavior/meaning is not obvious from the identifier name and the TypeScript type alone. Do not require JSDoc for “self-explanatory” members where name + type are sufficient (e.g., fields like `symbols`, `imports`, `exports`, etc. in `ParsedFile`). Also, remove comments that simply restate the function/variable name; those should be deleted rather than treated as documentation.
1 participant
Summary
benchmarkReindexrecorded timings even whenbun src/index.tsexited non-zero, hiding broken index runs behind plausible benchmark numbers.Fix: check
exitCodeafter each spawn; throw with stderr/stdout on failure. Logic extracted tobenchmark-reindex.tswith injectablespawnIndexerfor tests.Test plan
bun test src/benchmark-reindex.test.ts— non-zero exit throws, does not add totimesbun run typecheckSummary by CodeRabbit
Bug Fixes
Tests