perf: parallelize benchmark candidate search - #169
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:
📝 WalkthroughWalkthroughBenchmark retrieval now separates corpus collection from search orchestration. It adds persisted SQLite caches, prepared fusion evaluation, selectable router strategies, native worker execution, schema-24 timing diagnostics, and expanded tests and documentation. ChangesBenchmark retrieval pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
benchmarks/tests/worker-pool.test.ts (1)
89-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the bound instead of restating the formula.
Line 90 duplicates the exact expression from
getDefaultWorkerCount. The test passes for any implementation that repeats the formula and fails for any change to the reservation policy, even a valid one. Assert the observable contract: the value is at least 1 and does not exceedavailableParallelism().♻️ Proposed assertion
- expect(getDefaultWorkerCount()).toBe(Math.max(1, availableParallelism() - 1)) + expect(getDefaultWorkerCount()).toBeGreaterThanOrEqual(1) + expect(getDefaultWorkerCount()).toBeLessThanOrEqual(availableParallelism())As per path instructions, "Test behavior not implementation, through public interfaces only. Tests must survive internal refactoring."
🤖 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/tests/worker-pool.test.ts` around lines 89 - 93, Update the getDefaultWorkerCount assertion in the “derives a bounded default and honors explicit sizing” test to verify only the observable bounds: the result is at least 1 and no greater than availableParallelism(). Keep the existing resolveWorkerCount(0) and resolveWorkerCount(3) assertions unchanged.Source: Path instructions
benchmarks/retrieval/runner.ts (3)
519-554: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
splitSampleshere as well.Lines 528-529 and 546-547 repeat the inline filter pair that
splitSamplesnow encapsulates. The fusion and router loops already use the helper. Using it in all five places keeps one definition of the development and validation split.♻️ Proposed change for the grouped-fold loop
for (let fold = 0; fold < config.groupedFolds; fold++) { + const split = splitSamples(group.samples, (sample) => sample.groupedFold === fold) weightSearch.push( yield* runParallelSearch(() => optimizeWeightsParallel( group.model, group.queryKind, groupedStrategy, String(fold + 1), - group.samples.filter((sample) => sample.groupedFold !== fold), - group.samples.filter((sample) => sample.groupedFold === fold), + split.development, + split.validation, optimizationProfile, searchOptions, ), ), ) }🤖 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/retrieval/runner.ts` around lines 519 - 554, Replace the duplicated inline sample filters in the grouped-fold and repository-holdout calls to optimizeWeightsParallel with splitSamples, passing the appropriate holdout predicate or split criteria. Preserve the existing groupedFold and repository development/validation membership while using the same helper already used by the fusion and router loops.
291-292: 🚀 Performance & Scalability | 🔵 TrivialConsider the cost of one pool per search call.
The configuration itself is correct.
{}resolves the worker count fromPIX_BENCH_WORKERSor the default, and{ workerCount: 0 }forces the serial pool.Each of the six search entry points creates a pool and closes it. For the
fullprofile the runner therefore starts and terminatesavailableParallelism() - 1worker threads once per fold, per model, per fusion method, and re-clones a snapshot each time. Worker startup and snapshot cloning can consume a noticeable share of the search time.If timing instrumentation shows this cost, keep the worker threads alive across searches and send a new snapshot per search instead of recreating the pool. The PR already lists timing instrumentation as follow-up work, so this is a good measurement target.
🤖 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/retrieval/runner.ts` around lines 291 - 292, Measure pool startup, shutdown, and snapshot-cloning time around the six search entry points before changing the pool lifecycle; if overhead is material, reuse a persistent search pool across calls and provide a fresh snapshot for each search while preserving serial mode and the existing worker-count resolution.
118-122: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate cancellation through the benchmark search.
Effect.tryPromiseprovides anAbortSignal, butwithCandidatePoolcloses workers only in thefinallyblock of the operation promise. If an interruption abandons that promise, the worker threads can keep running. Thread cancellation/cleanup fromrunParallelSearchthrough every parallel search path.🤖 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/retrieval/runner.ts` around lines 118 - 122, Update runParallelSearch and each parallel-search call path to accept and propagate the AbortSignal supplied by Effect.tryPromise into withCandidatePool, ensuring worker shutdown runs when the effect is interrupted rather than relying only on the operation promise’s finally block.benchmarks/retrieval/weight-search.ts (2)
619-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
samplesandfusionparameters from the pooled ranking helpers.
rankWeightCandidatesnever readssamplesorfusion. The pool holds the prepared snapshot and the fusion method.selectBestWeightsforwards both values only to keep the signature.selectBestWeightsPerSubsetalready takes onlypool, which shows the intended shape.The current signature implies a guarantee that the code does not enforce. A caller can pass a pool built from different samples or a different fusion method. The search then returns quality values for the wrong sample set, with no length mismatch and no error.
♻️ Proposed signature cleanup
const rankWeightCandidates = async ( - samples: readonly WeightSearchSample[], candidates: readonly ChannelWeights[], limit: number, qualityCache: Map<string, QualitySummary>, - fusion: FusionMethod, pool: CandidateEvaluationPool, objective: RouterObjective = "reranker-top20", baseline?: QualitySummary, profile: OptimizationProfile = SEARCH_PRIORITY_PROFILE, ): Promise<readonly WeightCandidate[]> => {const selectBestWeights = async ( - samples: readonly WeightSearchSample[], - fusion: FusionMethod = "rrf", pool: CandidateEvaluationPool, objective: RouterObjective = "reranker-top20", baseline?: QualitySummary, profile: OptimizationProfile = SEARCH_PRIORITY_PROFILE, ): Promise<{ readonly weights: ChannelWeights; readonly quality: QualitySummary }> => {Update the seven call sites of
selectBestWeightsaccordingly.Also applies to: 659-678
🤖 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/retrieval/weight-search.ts` around lines 619 - 639, Remove the unused samples and fusion parameters from rankWeightCandidates and selectBestWeights, leaving pool as the source of the prepared snapshot and fusion method. Update selectBestWeights to stop forwarding those values, and adjust all seven selectBestWeights call sites to use the reduced signature; keep selectBestWeightsPerSubset’s existing pool-only shape unchanged.
1609-1628: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winShare one preparation object across the pools and the router search.
withParallelEvidencePoolsderivesevidenceSamplesandproxySamples, builds evaluation snapshots from them, and thenselectBestEvidenceRoutercallsprepareRouterSearchto derive them again.routerEvaluationCandidatemapssamplespositionally into weight vectors, and the pool evaluates them positionally. Replacing the prepared derivation fromselectBestEvidenceRouterinstead of passing the rawsampleskeeps one derivation source for both the pool snapshots and candidate weights.🤖 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/retrieval/weight-search.ts` around lines 1609 - 1628, Reuse the prepared evidenceSamples and proxySamples from withParallelEvidencePools when invoking selectBestEvidenceRouter instead of passing raw samples. Update selectBestEvidenceRouter and prepareRouterSearch to accept and use this shared preparation object, ensuring routerEvaluationCandidate and pool evaluation consume the same positional derivation.benchmarks/retrieval/fusion-core.d.mts (1)
4-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the declared parameters instead of
unknown.
evaluatePreparedContributionsis the hot boundary betweenfusion.tsand the.mjsimplementation.unknowndisables checking of both arguments, so a wrong snapshot or weight object is only detected at runtime.PreparedFusionSnapshotis already exported from./fusion.js, andChannelWeightsfrom../../src/domain/retrieval.js.♻️ Proposed typing
+import type { ChannelWeights } from "../../src/domain/retrieval.js" +import type { PreparedFusionSnapshot } from "./fusion.js" import type { QualitySummary } from "./types.js" import type { EvaluationCandidate, EvaluationSnapshot } from "./worker-pool.js" export function evaluatePreparedContributions( - matrix: unknown, - weights: unknown, + matrix: PreparedFusionSnapshot, + weights: ChannelWeights, ): { readonly chunkIndex: number readonly score: number }[]Note:
fusion.tsimports the type fromworker-pool.jsindirectly; confirm no type-only cycle warning appears after the change.🤖 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/retrieval/fusion-core.d.mts` around lines 4 - 10, Update the declared parameters of evaluatePreparedContributions to use PreparedFusionSnapshot from ./fusion.js for matrix and ChannelWeights from ../../src/domain/retrieval.js for weights instead of unknown, preserving the existing return type. Use type-only imports as needed and verify the declarations do not introduce a type-only cycle warning.benchmarks/tests/channels.test.ts (1)
588-607: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test can pass without running any worker.
createCandidateEvaluationPoolfalls back to a serial pool when worker startup fails, becausefallbackToSerialis notfalse(worker-pool.ts lines 427-430). This test cannot observe the pool, so a failed worker startup turns the assertion into serial-versus-serial equality and the test still passes.
benchmarks/tests/worker-pool.test.tslines 95-114 assertsmode: "parallel"for a directly created pool, so the worker path is not completely uncovered. To make this specific regression test meaningful, expose the pool mode through the search result or assert the worker count fromgetDefaultWorkerCountbefore running the comparison.🤖 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/tests/channels.test.ts` around lines 588 - 607, The parallel-versus-serial test around optimizeEvidenceRouterParallel must verify that workers actually ran instead of allowing serial fallback to satisfy equality. Update the test or search result plumbing around optimizeEvidenceRouterParallel and createCandidateEvaluationPool to expose and assert parallel mode or the expected worker count, while preserving the existing result comparison.benchmarks/retrieval/fusion-core.mjs (1)
33-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a parity test for the worker-pool metrics.
benchmarks/retrieval/worker-pool.tscallsfusion-core.mjs, whoserecallAt,reciprocalRank, andcontextRecallAtBudgetduplicate the implementations inbenchmarks/retrieval/metrics.ts. Current tests compare pool/serial to each other or compare development vs static validation, but not both metric sources against the same candidate. Add one test that asserts these results matchsummarizeunder mixed sample weights, including a chunk index missing fromcontextTokens.🤖 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/retrieval/fusion-core.mjs` around lines 33 - 60, Add a parity test covering worker-pool metrics against the canonical summarize implementation in metrics.ts. Use mixed sample weights and include a candidate containing a chunk index absent from contextTokens, then assert recallAt, reciprocalRank, and contextRecallAtBudget results from fusion-core.mjs match summarize for the same candidate; retain existing tests unchanged.
🤖 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/retrieval/worker-pool.ts`:
- Around line 214-217: Update the worker exit handler in the worker pool to call
handleWorkerError for every exit while this.closed is false, including exit code
0; retain the existing closed guard so expected shutdown exits remain
suppressed.
- Around line 143-159: Memoize the contextTokens result by chunks array
reference within createEvaluationSnapshot so samples sharing the same
input.chunks reuse one computed number[] instead of recalculating it; preserve
separate results for distinct chunks arrays and continue assigning each sample
its corresponding cached context-token array.
In `@CONTEXT.md`:
- Around line 248-253: Update the benchmark documentation around the prepared
evaluator to remove the “Serial” qualifier, since both serial and worker pools
use the same prepared snapshot. Also document that workerCount: 1 selects the
serial pool through createCandidateEvaluationPool, alongside the existing
PIX_BENCH_WORKERS=0 case.
---
Nitpick comments:
In `@benchmarks/retrieval/fusion-core.d.mts`:
- Around line 4-10: Update the declared parameters of
evaluatePreparedContributions to use PreparedFusionSnapshot from ./fusion.js for
matrix and ChannelWeights from ../../src/domain/retrieval.js for weights instead
of unknown, preserving the existing return type. Use type-only imports as needed
and verify the declarations do not introduce a type-only cycle warning.
In `@benchmarks/retrieval/fusion-core.mjs`:
- Around line 33-60: Add a parity test covering worker-pool metrics against the
canonical summarize implementation in metrics.ts. Use mixed sample weights and
include a candidate containing a chunk index absent from contextTokens, then
assert recallAt, reciprocalRank, and contextRecallAtBudget results from
fusion-core.mjs match summarize for the same candidate; retain existing tests
unchanged.
In `@benchmarks/retrieval/runner.ts`:
- Around line 519-554: Replace the duplicated inline sample filters in the
grouped-fold and repository-holdout calls to optimizeWeightsParallel with
splitSamples, passing the appropriate holdout predicate or split criteria.
Preserve the existing groupedFold and repository development/validation
membership while using the same helper already used by the fusion and router
loops.
- Around line 291-292: Measure pool startup, shutdown, and snapshot-cloning time
around the six search entry points before changing the pool lifecycle; if
overhead is material, reuse a persistent search pool across calls and provide a
fresh snapshot for each search while preserving serial mode and the existing
worker-count resolution.
- Around line 118-122: Update runParallelSearch and each parallel-search call
path to accept and propagate the AbortSignal supplied by Effect.tryPromise into
withCandidatePool, ensuring worker shutdown runs when the effect is interrupted
rather than relying only on the operation promise’s finally block.
In `@benchmarks/retrieval/weight-search.ts`:
- Around line 619-639: Remove the unused samples and fusion parameters from
rankWeightCandidates and selectBestWeights, leaving pool as the source of the
prepared snapshot and fusion method. Update selectBestWeights to stop forwarding
those values, and adjust all seven selectBestWeights call sites to use the
reduced signature; keep selectBestWeightsPerSubset’s existing pool-only shape
unchanged.
- Around line 1609-1628: Reuse the prepared evidenceSamples and proxySamples
from withParallelEvidencePools when invoking selectBestEvidenceRouter instead of
passing raw samples. Update selectBestEvidenceRouter and prepareRouterSearch to
accept and use this shared preparation object, ensuring
routerEvaluationCandidate and pool evaluation consume the same positional
derivation.
In `@benchmarks/tests/channels.test.ts`:
- Around line 588-607: The parallel-versus-serial test around
optimizeEvidenceRouterParallel must verify that workers actually ran instead of
allowing serial fallback to satisfy equality. Update the test or search result
plumbing around optimizeEvidenceRouterParallel and createCandidateEvaluationPool
to expose and assert parallel mode or the expected worker count, while
preserving the existing result comparison.
In `@benchmarks/tests/worker-pool.test.ts`:
- Around line 89-93: Update the getDefaultWorkerCount assertion in the “derives
a bounded default and honors explicit sizing” test to verify only the observable
bounds: the result is at least 1 and no greater than availableParallelism().
Keep the existing resolveWorkerCount(0) and resolveWorkerCount(3) assertions
unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8654068-24a4-4363-b156-e5c1f5afc3a1
📒 Files selected for processing (10)
CONTEXT.mdbenchmarks/retrieval/fusion-core.d.mtsbenchmarks/retrieval/fusion-core.mjsbenchmarks/retrieval/fusion-worker.mjsbenchmarks/retrieval/fusion.tsbenchmarks/retrieval/runner.tsbenchmarks/retrieval/weight-search.tsbenchmarks/retrieval/worker-pool.tsbenchmarks/tests/channels.test.tsbenchmarks/tests/worker-pool.test.ts
|
Review follow-up pushed in 420f408.
Validation:
CodeRabbit is currently rate-limited for the follow-up review. Pool reuse/startup timing remains tracked in #170. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
benchmarks/retrieval/weight-search.ts (3)
1451-1453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA required
poolparameter follows the defaultedprofileparameter in four pool helpers. The default value ofprofileis unreachable because every caller must pass it to reachpool. Movepoolbeforeprofilein each helper, or group the trailing arguments into one options object.
benchmarks/retrieval/weight-search.ts#L1451-L1453: reorderoptimizeWeightsWithPoolsopool: CandidateEvaluationPoolprecedesprofile: OptimizationProfile = SEARCH_PRIORITY_PROFILE.benchmarks/retrieval/weight-search.ts#L1534-L1536: apply the same reorder inoptimizeFusionWeightsWithPool.benchmarks/retrieval/weight-search.ts#L1837-L1839: apply the same reorder infitRecommendedWeightsWithPool.benchmarks/retrieval/weight-search.ts#L1883-L1885: apply the same reorder infitRecommendedFusionWeightsWithPool.🤖 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/retrieval/weight-search.ts` around lines 1451 - 1453, The defaulted profile parameter prevents callers from omitting it when supplying pool. In benchmarks/retrieval/weight-search.ts at lines 1451-1453, 1534-1536, 1837-1839, and 1883-1885, reorder the parameters in optimizeWeightsWithPool, optimizeFusionWeightsWithPool, fitRecommendedWeightsWithPool, and fitRecommendedFusionWeightsWithPool so pool precedes the defaulted profile, and update all call sites accordingly.
1340-1389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the phase timings and the candidate timings overlap.
randomSearchMsandbeamSearchMsare wall-clock spans.selectRandomRouterandrankRouterCandidatesadd tocandidatePreparationMs,candidateEvaluationMs, andcandidateSelectionMsinside those same spans. A consumer that sums all fields inRouterSearchTimingstherefore double counts the same work. Add a short comment onMutableRouterSearchTimingsthat states the phase fields and the candidate fields are two overlapping views of the same wall clock.🤖 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/retrieval/weight-search.ts` around lines 1340 - 1389, Add a documentation comment to the MutableRouterSearchTimings type definition explaining that the phase-level timing fields (randomSearchMs, beamSearchMs) and the candidate-level timing fields (candidatePreparationMs, candidateEvaluationMs, candidateSelectionMs) represent overlapping views of the same wall-clock time. State that summing all fields in RouterSearchTimings will result in double-counting because selectRandomRouter and rankRouterCandidates increment candidate timings within the same spans measured by the phase timings.
91-96: 📐 Maintainability & Code Quality | 🔵 TrivialRemove the duplicate
ParallelSearchOptionstype.
runner.tscurrently defines its ownParallelSearchOptions, whileweight-search.tsexports the shared one. The duplicate declaration can drift from the original; remove therunner.tsdefinition and import the exportedParallelSearchOptionsfromweight-search.ts.
[low_effort和高reward]🤖 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/retrieval/weight-search.ts` around lines 91 - 96, Remove the locally declared ParallelSearchOptions type from runner.ts and import the exported ParallelSearchOptions from weight-search.ts instead. Update runner.ts references to use this shared definition without changing the existing search behavior.benchmarks/retrieval/runner.ts (2)
810-816: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAccumulate results with
pushinstead of repeated spreads.Each iteration rebuilds both arrays, which is quadratic in the number of jobs. Use mutable arrays and
push.♻️ Proposed change
- let evidenceRouterSearch: readonly EvidenceRouterSearchResult[] = [] - let recommendedEvidenceRouters: readonly RecommendedEvidenceRouter[] = [] + const evidenceRouterSearch: EvidenceRouterSearchResult[] = [] + const recommendedEvidenceRouters: RecommendedEvidenceRouter[] = [] for (const result of routerResults) { - if (result.kind === "holdout") - evidenceRouterSearch = [...evidenceRouterSearch, ...result.results] - else recommendedEvidenceRouters = [...recommendedEvidenceRouters, ...result.results] + if (result.kind === "holdout") evidenceRouterSearch.push(...result.results) + else recommendedEvidenceRouters.push(...result.results) }🤖 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/retrieval/runner.ts` around lines 810 - 816, Update the result accumulation around evidenceRouterSearch and recommendedEvidenceRouters to use mutable arrays with push for each result.results collection instead of repeatedly creating arrays with spread syntax. Preserve the existing holdout versus non-holdout routing behavior while avoiding quadratic copying across routerResults.
796-807: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe non-parallel branch discards available candidate workers.
canParallelizeRouterJobsis false whenrouterWorkerBudgetis 1 or 2, even ifserialSearchis false. This branch then overridesworkerCount: 0, so router jobs run fully serial and the available workers stay idle. The other searches in this file keepsearchOptions.workerCount. Consider running each job with the remaining budget instead of forcing zero.♻️ Proposed change
(job) => runParallelSearch((signal) => runRouterSearchJob(job, optimizationProfile, { ...searchOptions, - workerCount: 0, signal, }), ),🤖 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/retrieval/runner.ts` around lines 796 - 807, In the non-parallel branch of the Effect.forEach block where runParallelSearch calls runRouterSearchJob, replace the hardcoded `workerCount: 0` override with the remaining worker budget from searchOptions so available workers are utilized when serialSearch is false and budget remains, keeping the behavior consistent with other searches in the file that preserve searchOptions.workerCount.benchmarks/retrieval/ts-loader.mjs (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the swallowed error to module-not-found.
The
catchblock discards every failure from the.tsresolution attempt. If a real.tsfile exists but resolution fails for another reason, the loader silently resolves the.jsspecifier instead, and the resulting failure appears far from its cause. Check the error code before falling back.♻️ Proposed change
if (specifier.startsWith(".") && specifier.endsWith(".js")) { try { return await defaultResolve(`${specifier.slice(0, -3)}.ts`, context, defaultResolve) - } catch { - // Keep normal JavaScript resolution for actual .js modules. + } catch (cause) { + // Keep normal JavaScript resolution for actual .js modules. + if (cause?.code !== "ERR_MODULE_NOT_FOUND") throw cause } }🤖 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/retrieval/ts-loader.mjs` around lines 3 - 7, Update the catch block around the .ts defaultResolve attempt to inspect the caught error and fall back to normal JavaScript resolution only for the module-not-found error code; rethrow all other resolution failures so they remain visible at the source.benchmarks/retrieval/worker-pool.ts (3)
574-582: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
message.typecheck.Line 575 already returns
falsewhenmessage.typeis not a string. Line 576 repeats the same condition and can never be reached with a non-stringtype.♻️ Proposed cleanup
const isQueueWorkerMessage = (message: unknown): message is QueueWorkerMessage => { if (!isQueueRecord(message) || typeof message.type !== "string") return false - if (typeof message.type !== "string") return false if (message.type === "ready") return 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/retrieval/worker-pool.ts` around lines 574 - 582, Remove the redundant second `typeof message.type !== "string"` guard from `isQueueWorkerMessage`, preserving the initial validation and all subsequent message-type handling unchanged.
329-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate the
resultpayload inisWorkerMessage, asisQueueWorkerMessagedoes.
isWorkerMessagechecks only thetypefield, then assertsmessage is WorkerMessage. Line 422 readsmessage.results.lengthon that basis. If aresultmessage ever arrives withoutresults, the handler throws aTypeErrorinside theworker.on("message")listener, which becomes an uncaught main-thread exception instead of a pool failure.
isQueueWorkerMessageat lines 574-582 already validatestaskIdandresultsfor the queue protocol. Align the two guards so both fail throughhandleWorkerError.The worker is first-party code, so this is defence in depth rather than a live defect.
♻️ Proposed payload validation
const isWorkerMessage = (message: unknown): message is WorkerMessage => { - return hasWorkerMessageType(message, ["ready", "result", "error"]) + if (!hasWorkerMessageType(message, ["ready", "result", "error"])) return false + if (message.type !== "result") return true + const record = message as { readonly taskId?: unknown; readonly results?: unknown } + return Number.isInteger(record.taskId) && Array.isArray(record.results) }🤖 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/retrieval/worker-pool.ts` around lines 329 - 331, Update isWorkerMessage to validate result-message payloads, including the presence and expected shape of results, matching the checks performed by isQueueWorkerMessage. Preserve ready and error handling, and ensure malformed result messages are rejected so the existing worker message flow routes them through handleWorkerError instead of accessing message.results unsafely.
651-658: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the
snapshotIdreuse contract.
snapshotIdFortrusts an explicitrequestedIdand skips thesnapshotIdsWeakMap.dispatchthen omits the snapshot payload whenever the target slot already recorded that id at line 717. If a caller reuses onesnapshotIdfor two differentEvaluationSnapshotvalues, the worker keeps evaluating the first snapshot and returns silently wrong metrics instead of an error.The one in-repo caller,
benchmarks/retrieval/router-job-pool.tsline 258, namespaces the id per worker slot and keeps the mapping stable, so the current behavior is correct. State the requirement on the publicevaluatesignature at lines 97-102 so future callers keep it.♻️ Proposed documentation
- /** Enqueue candidates for one prepared snapshot and preserve candidate order in the result. */ + /** + * Enqueue candidates for one prepared snapshot and preserve candidate order in the result. + * + * When `snapshotId` is supplied, it must identify the snapshot uniquely. Workers cache a + * snapshot by id, so reusing one id for a different snapshot evaluates the stale snapshot. + */ readonly evaluate: (🤖 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/retrieval/worker-pool.ts` around lines 651 - 658, Document the snapshot ID reuse contract on the public evaluate signature around evaluate: callers must keep each explicit snapshotId stable for the same EvaluationSnapshot and must not reuse one ID across different snapshots, since dispatch may omit the snapshot payload for an already recorded ID. Preserve the existing snapshotIdFor behavior and implementation.benchmarks/tests/worker-pool.test.ts (2)
242-275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the test name with the asserted path.
The name says "keeps worker metrics aligned with canonical summarization". The body calls
evaluateCandidatesSerialand never starts a worker, so it compares the serial evaluator againstsummarize. Other tests in this file already assert worker output equalsevaluateCandidatesSerial, so the worker chain is covered transitively.Rename the test to state the serial-versus-canonical comparison, or run the assertion through a queue-backed pool to match the current name.
The coverage itself is valuable. The out-of-range
chunkIndex: 99on line 245 and the two distinctqueryKindvalues both discriminate real regressions in snapshot construction.🤖 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/tests/worker-pool.test.ts` around lines 242 - 275, The test name does not match its serial-only assertion. Rename the test around evaluateCandidatesSerial and summarize to describe the serial-versus-canonical comparison, while preserving the existing parityRankings, out-of-range chunkIndex, and distinct queryKind coverage.
229-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test covers the pre-registration abort branch only.
evaluatesuspends atawait this.readybefore it registers the abort listener.controller.abort()on line 234 therefore runs while the signal check at worker-pool.ts lines 810-813 is still ahead. The rejection comes from that re-check, not from the"abort"listener registered on line 809.The behavior asserted is correct and the test is deterministic. The mid-flight path stays uncovered: an abort that arrives while batches are already dispatched must reject the request, let the in-flight task clear
slot.busythrough thetask.request.settledguard at line 760, and leave the queue usable.Add a second case that awaits one successful evaluation first, then aborts a following request after dispatch begins.
As per path instructions: "Test behavior not implementation, through public interfaces only."
🤖 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/tests/worker-pool.test.ts` around lines 229 - 240, Add a separate public-interface test alongside “rejects an aborted queue request without closing the shared queue” that first awaits a successful evaluation to ensure readiness, then starts a second evaluation, waits until dispatch has begun, aborts its controller, and asserts rejection with “interrupted.” Finally, verify a subsequent evaluation still resolves successfully and close the queue in cleanup; synchronize only through observable queue behavior rather than internal implementation details.Source: Path instructions
🤖 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 @.fallowrc.json:
- Around line 7-8: Add benchmarks/retrieval/router-job-worker.mjs to the
reachability entries in .fallowrc.json so the worker started by
runEvidenceRouterJobs() through router-job-pool.ts’s URL entry is included in
the audit graph. Do not add fusion-core.mjs or fusion-worker.mjs, since their TS
imports already provide coverage.
In `@benchmarks/retrieval/ts-loader.mjs`:
- Around line 1-10: Update the documented Node runtime requirements in
package.json by declaring an engines.node range that supports the worker
execArgv flags used by router-job-pool.ts, including --experimental-strip-types
and --experimental-loader. Keep the existing worker configuration unchanged
unless the project’s supported Node range cannot provide those flags, in which
case remove the deprecated or unavailable flags from workerExecArgv.
In `@benchmarks/retrieval/weight-search.ts`:
- Around line 1714-1733: The selectStaticWeightsForSelections function repeats
pool.evaluate() calls for each objective in the selections loop, even though the
evaluation result depends only on the constant parameters fullPool,
productionQuality, and profile (which are objective-independent). Extract the
weight grid evaluation outside the loop by computing it once with fullPool,
productionQuality, and profile before iterating over selections, then pass the
cached quality result to each selectStaticWeights call to avoid redundant
evaluations per objective.
- Around line 1429-1440: Update the abort cleanup around the pool operation at
benchmarks/retrieval/weight-search.ts lines 1429-1440 and 1668-1691 so each
pool.close() promise uses the same rejection handler as the corresponding
finally cleanup. Ensure abort-triggered close failures are handled immediately
rather than discarded or left for the final close path, while preserving
listener removal and normal teardown behavior.
In `@benchmarks/retrieval/worker-pool.ts`:
- Around line 862-879: Update QueuedCandidateEvaluationPool so its reported mode
is derived from the wrapped CandidateEvaluationQueue rather than hardcoded to
"parallel"; ensure serial queues report "serial" while parallel queues retain
"parallel", and propagate that value through stats().
- Around line 821-832: Update NativeCandidateEvaluationQueue.close() to reject
any pending readiness promise before terminating workers, ensuring callers
awaiting this.ready settle when closing during startup. Preserve the existing
close idempotency and request-settlement behavior, and attach a rejection
handler in the constructor or rely on the existing create() catch so the
intentional this.ready rejection is handled.
In `@benchmarks/tests/worker-pool.test.ts`:
- Around line 160-162: Update the test case containing the parallelHoldout
candidateEvaluationMs assertion to set an explicit timeout covering startup of
the holdout worker, both router workers, and two router evaluations. Choose a
budget that accommodates slow CI while preserving the existing timing assertion.
---
Nitpick comments:
In `@benchmarks/retrieval/runner.ts`:
- Around line 810-816: Update the result accumulation around
evidenceRouterSearch and recommendedEvidenceRouters to use mutable arrays with
push for each result.results collection instead of repeatedly creating arrays
with spread syntax. Preserve the existing holdout versus non-holdout routing
behavior while avoiding quadratic copying across routerResults.
- Around line 796-807: In the non-parallel branch of the Effect.forEach block
where runParallelSearch calls runRouterSearchJob, replace the hardcoded
`workerCount: 0` override with the remaining worker budget from searchOptions so
available workers are utilized when serialSearch is false and budget remains,
keeping the behavior consistent with other searches in the file that preserve
searchOptions.workerCount.
In `@benchmarks/retrieval/ts-loader.mjs`:
- Around line 3-7: Update the catch block around the .ts defaultResolve attempt
to inspect the caught error and fall back to normal JavaScript resolution only
for the module-not-found error code; rethrow all other resolution failures so
they remain visible at the source.
In `@benchmarks/retrieval/weight-search.ts`:
- Around line 1451-1453: The defaulted profile parameter prevents callers from
omitting it when supplying pool. In benchmarks/retrieval/weight-search.ts at
lines 1451-1453, 1534-1536, 1837-1839, and 1883-1885, reorder the parameters in
optimizeWeightsWithPool, optimizeFusionWeightsWithPool,
fitRecommendedWeightsWithPool, and fitRecommendedFusionWeightsWithPool so pool
precedes the defaulted profile, and update all call sites accordingly.
- Around line 1340-1389: Add a documentation comment to the
MutableRouterSearchTimings type definition explaining that the phase-level
timing fields (randomSearchMs, beamSearchMs) and the candidate-level timing
fields (candidatePreparationMs, candidateEvaluationMs, candidateSelectionMs)
represent overlapping views of the same wall-clock time. State that summing all
fields in RouterSearchTimings will result in double-counting because
selectRandomRouter and rankRouterCandidates increment candidate timings within
the same spans measured by the phase timings.
- Around line 91-96: Remove the locally declared ParallelSearchOptions type from
runner.ts and import the exported ParallelSearchOptions from weight-search.ts
instead. Update runner.ts references to use this shared definition without
changing the existing search behavior.
In `@benchmarks/retrieval/worker-pool.ts`:
- Around line 574-582: Remove the redundant second `typeof message.type !==
"string"` guard from `isQueueWorkerMessage`, preserving the initial validation
and all subsequent message-type handling unchanged.
- Around line 329-331: Update isWorkerMessage to validate result-message
payloads, including the presence and expected shape of results, matching the
checks performed by isQueueWorkerMessage. Preserve ready and error handling, and
ensure malformed result messages are rejected so the existing worker message
flow routes them through handleWorkerError instead of accessing message.results
unsafely.
- Around line 651-658: Document the snapshot ID reuse contract on the public
evaluate signature around evaluate: callers must keep each explicit snapshotId
stable for the same EvaluationSnapshot and must not reuse one ID across
different snapshots, since dispatch may omit the snapshot payload for an already
recorded ID. Preserve the existing snapshotIdFor behavior and implementation.
In `@benchmarks/tests/worker-pool.test.ts`:
- Around line 242-275: The test name does not match its serial-only assertion.
Rename the test around evaluateCandidatesSerial and summarize to describe the
serial-versus-canonical comparison, while preserving the existing
parityRankings, out-of-range chunkIndex, and distinct queryKind coverage.
- Around line 229-240: Add a separate public-interface test alongside “rejects
an aborted queue request without closing the shared queue” that first awaits a
successful evaluation to ensure readiness, then starts a second evaluation,
waits until dispatch has begun, aborts its controller, and asserts rejection
with “interrupted.” Finally, verify a subsequent evaluation still resolves
successfully and close the queue in cleanup; synchronize only through observable
queue behavior rather than internal implementation details.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fd0bc3e4-c34d-409e-abb3-68b56eee38c5
📒 Files selected for processing (14)
.fallowrc.jsonCONTEXT.mdbenchmarks/retrieval/fusion-core.d.mtsbenchmarks/retrieval/fusion-worker.mjsbenchmarks/retrieval/router-job-pool.tsbenchmarks/retrieval/router-job-worker.mjsbenchmarks/retrieval/runner.tsbenchmarks/retrieval/ts-loader.mjsbenchmarks/retrieval/types.tsbenchmarks/retrieval/weight-search.tsbenchmarks/retrieval/worker-message.tsbenchmarks/retrieval/worker-pool.tsbenchmarks/tests/channels.test.tsbenchmarks/tests/worker-pool.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- benchmarks/retrieval/fusion-worker.mjs
- CONTEXT.md
- benchmarks/tests/channels.test.ts
- benchmarks/retrieval/fusion-core.d.mts
| "tests/scripts/check-dtype-output.mjs", | ||
| "benchmarks/retrieval/ts-loader.mjs" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the full audit configuration and locate worker entry modules referenced only by URL.
cat -n .fallowrc.json
fd -t f -e mjs . benchmarks/retrievalRepository: Lucas-Bur/pix
Length of output: 4157
🏁 Script executed:
#!/bin/bash
# Inspect how the benchmark worker modules are referenced and whether they start other worker modules.
set -euo pipefail
printf '\nRelevant imports and URLs in benchmarks/retrieval:\n'
rg -n --glob '*.ts' --glob '*.mjs' 'fuzz-worker|fusion-worker|router-job-worker|ts-loader|new Worker|URL\.create|new Worker|from\b' .
printf '\nWorker module headers/large static references:\n'
for f in benchmarks/retrieval/fusion-core.mjs benchmarks/retrieval/fusion-worker.mjs benchmarks/retrieval/router-job-worker.mjs benchmarks/retrieval/ts-loader.mjs; do
echo "--- $f"
sed -n '1,220p' "$f"
done
printf '\nWorker module count and entry files matching imports/URLs only:\n'
python3 - <<'PY'
from pathlib import Path
import re
workdir=Path('benchmarks/retrieval')
worker_files={p.relative_to(workdir).as_posix() for p in workdir.glob('*.mjs')}
entry_files={'src/index.ts','tests/scripts/bench-startup.mjs','tests/scripts/check-dtype-output.mjs','benchmarks/retrieval/ts-loader.mjs'}
imports=set()
urls=set()
for p in Path('.').rglob('*'):
if p.is_file() and p.suffix in {'.ts','.mjs'}:
try:
s=p.read_text(errors='ignore')
except Exception:
continue
for path in re.findall(r'["\']([^"\']+?\.mjs)["\']', s):
abs=Path(path)
if abs.is_absolute():
imports.add(str(abs))
else:
imports.add(str((Path(p.parent)/abs).resolve()))
for m in re.finditer(r'(?:new Worker|import\s*\(|fetch\s*|\.worker\b)\s*[^(]*=\s*(?:["\']([^"\']+)["\']|URL\.create\([^)]+\)|\d+)', s, re.S):
text=s[m.start():m.end()]
# capture first URL string arg if present
for s2 in re.findall(r'(?:new Worker|fetch\s*\(|import\s*\()\s*["\']([^"\']+?\.mjs)["\']', s[:m.end()], re.S):
urls.add(str(Path(m.group(1) or s2)))
for f in sorted(worker_files):
covered_by_import=any(str(abs_f).endswith('/'+f) or str(abs_f).endswith('/benchmarks/retrieval/'+f) for abs_f in imports)
covered_by_url=f in urls
print(f"{f}: entry={f in entry_files}: imports={covered_by_import}: urls={covered_by_url}: imported={covered_by_import or covered_by_url}")
PYRepository: Lucas-Bur/pix
Length of output: 50370
🏁 Script executed:
#!/bin/bash
# Check worker entry files and their static reachability, avoiding large unrelated output.
set -euo pipefail
printf '\nRelevant worker files:\n'
for f in \
benchmarks/retrieval/fusion-core.mjs \
benchmarks/retrieval/fusion-worker.mjs \
benchmarks/retrieval/router-job-worker.mjs \
benchmarks/retrieval/ts-loader.mjs \
benchmarks/retrieval/router-job-pool.ts \
benchmarks/retrieval/worker-pool.ts
do
echo "--- $f: lines=$(wc -l < "$f")"
sed -n '1,240p' "$f"
done
printf '\nStatic references to retrieval worker modules:\n'
python3 - <<'PY'
from pathlib import Path
import re
targets = {
"benchmarks/retrieval/fusion-core.mjs",
"benchmarks/retrieval/fusion-worker.mjs",
"benchmarks/retrieval/router-job-worker.mjs",
"benchmarks/retrieval/ts-loader.mjs",
}
entry_sources = {"benchmarks/retrieval/ts-loader.mjs"}
for name in sorted(targets):
print(f"TARGET {name}")
in_entry = name in entry_sources
found_imports=[]
found_urls=[]
imported_by=[]
self_imports=[]
self_urls=[]
for p in Path(".").rglob("*"):
if p.is_file() and p.suffix in {".ts", ".mjs"}:
try:
s = p.read_text(errors="ignore")
except Exception:
continue
for m in re.finditer(r'["\']([^"\']+)\.mjs["\']', s):
url = m.group(0)
resolved = (Path(p.parent) / m.group(1)).resolve()
if name in ("benchmarks/retrieval/" + str(Path(resolved).relative_to(Path(".").resolve()))):
found_imports.append((str(p), url))
if name == (f"benchmarks/retrieval/{str(Path(resolved).relative_to(Path('.')))}" if str(Path(resolved).relative_to(Path('.'))).startswith('benchmarks/retrieval/') else str(Path(resolved))):
self_imports.append((str(p), url))
for m in re.finditer(r'\bURL\.create\(["\']([^"\']+)["\']\s*(?:,\s*\{[^}]+\})?\)', s):
if targets & {name}:
pass
for m in re.finditer(r'\bURL\.create\(["\']([^"\']+)\.mjs["\']\s*(?:,\s*\{[^}]+\})?\)', s):
resolved = (Path(p.parent) / m.group(1)).resolve()
if str(resolved) == name or ("benchmarks/retrieval/" + str(Path(resolved).relative_to(Path(".").resolve())) == name):
found_urls.append((str(p), f"URL.create(\"{m.group(1)}.mjs\")"))
for m in re.finditer(r'\b(?:new Worker|fetch|import\s*\()([^;\n\'"]+)["\']([^"\']+\.mjs)["\']', s, re.S):
pass
print(" direct_static: import", found_imports)
print(" direct_static: URL.create", found_urls)
print(" entry_source_or_url", in_entry)
PYRepository: Lucas-Bur/pix
Length of output: 27968
Add the worker modules needed by URL entry to the audit graph.
runEvidenceRouterJobs() starts workers via URL from benchmarks/retrieval/router-job-pool.ts, so static analysis will not reach benchmarks/retrieval/router-job-worker.mjs. Add that worker to .fallowrc.json entries or another reachability configuration. benchmarks/retrieval/fusion-core.mjs and benchmarks/retrieval/fusion-worker.mjs are referenced by TS import, so they do not need URL-based entry coverage.
🤖 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 @.fallowrc.json around lines 7 - 8, Add
benchmarks/retrieval/router-job-worker.mjs to the reachability entries in
.fallowrc.json so the worker started by runEvidenceRouterJobs() through
router-job-pool.ts’s URL entry is included in the audit graph. Do not add
fusion-core.mjs or fusion-worker.mjs, since their TS imports already provide
coverage.
| const pool = await createEvaluationPoolForSamples(samples, fusion, profile, options) | ||
| const closeOnAbort = () => { | ||
| void pool.close() | ||
| } | ||
| options.signal?.addEventListener("abort", closeOnAbort, { once: true }) | ||
| if (options.signal?.aborted) closeOnAbort() | ||
| try { | ||
| return await operation(pool) | ||
| } finally { | ||
| options.signal?.removeEventListener("abort", closeOnAbort) | ||
| await pool.close() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outlines/snippets =="
wc -l benchmarks/retrieval/weight-search.ts
sed -n '1380,1470p' benchmarks/retrieval/weight-search.ts
sed -n '1620,1715p' benchmarks/retrieval/weight-search.ts
echo "== CandidateEvaluationPool definitions/usages =="
rg -n "createEvaluationPoolForSamples|interface CandidateEvaluationPool|class .*EvaluationPool|type CandidateEvaluationPool|async close\(|close\(" benchmarks/retrieval/weight-search.ts
echo "== broader pool implementations/files =="
rg -n "class |interface |type .*EvaluationPool|close\(" benchmarks/retrieval -g '*.ts' | head -200
echo "== Node abort listener event behavior probe =="
node - <<'JS'
process.on('unhandledRejection', (reason) => {
console.log('unhandledRejection', reason && reason.message)
process.exit(1)
})
const signal = AbortSignal.timeout(0)
signal.addEventListener('abort', () => {
void Promise.reject(new Error('pool close failed'))
})
if (signal.aborted) void Promise.reject(new Error('aborted during setup')).catch(() => {})
setTimeout(() => process.exit(0), 10)
JSRepository: Lucas-Bur/pix
Length of output: 14841
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== createEvaluationPoolForSamples =="
sed -n '120,185p' benchmarks/retrieval/weight-search.ts
echo "== worker-pool relevant sections =="
sed -n '55,100p' benchmarks/retrieval/worker-pool.ts
sed -n '220,260p' benchmarks/retrieval/worker-pool.ts
sed -n '480,535p' benchmarks/retrieval/worker-pool.ts
sed -n '850,965p' benchmarks/retrieval/worker-pool.ts
sed -n '320,425p' benchmarks/retrieval/worker-pool.ts
sed -n '810,845p' benchmarks/retrieval/worker-pool.ts
echo "== CandidateEvaluationPool interface =="
python3 - <<'PY'
from pathlib import Path
text = Path('benchmarks/retrieval/worker-pool.ts').read_text()
start = text.index('export interface CandidateEvaluationPool')
end = text.find('\n}', start)
print(text[start:end+2])
PY
echo "== deterministic abort listener unhandledRejection behavior =="
node - <<'JS'
let called = 0;
process.on('unhandledRejection', (reason) => {
called++;
console.log('unhandledRejection count', called, 'message', reason && reason.message);
});
const signal = AbortSignal.timeout(0);
signal.addEventListener('abort', () => {
void Promise.reject(new Error('pool close failed'));
});
setTimeout(() => {
console.log('called at flush', called);
if (called === 0) process.exit(1);
}, 10);
JSRepository: Lucas-Bur/pix
Length of output: 15524
Handle close() rejections in the abort handlers. The abort listener discards the close() promise with void, so worker-teardown rejections become unhandled. Attach the same rejection handler used in the finally path, and avoid relying on the final close path to clean up a failed abort cleanup.
📍 Affects 1 file
benchmarks/retrieval/weight-search.ts#L1429-L1440(this comment)benchmarks/retrieval/weight-search.ts#L1668-L1691
🤖 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/retrieval/weight-search.ts` around lines 1429 - 1440, Update the
abort cleanup around the pool operation at benchmarks/retrieval/weight-search.ts
lines 1429-1440 and 1668-1691 so each pool.close() promise uses the same
rejection handler as the corresponding finally cleanup. Ensure abort-triggered
close failures are handled immediately rather than discarded or left for the
final close path, while preserving listener removal and normal teardown
behavior.
| async close(): Promise<void> { | ||
| if (this.closePromise !== undefined) return this.closePromise | ||
| this.closed = true | ||
| for (const request of this.requests) | ||
| this.settleRequest(request, new Error("Candidate evaluation queue closed")) | ||
| this.readyRequests.length = 0 | ||
| for (const slot of this.slots) slot.busy = false | ||
| this.closePromise = Promise.all( | ||
| this.slots.map((slot) => slot.worker.terminate().catch(() => -1)), | ||
| ).then(() => undefined) | ||
| return this.closePromise | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject pending readiness in close() to remove a hang path.
close() sets this.closed = true, then terminates the workers. The resulting "exit" events reach attachCandidateWorkerLifecycle at lines 184-186, which suppresses onError because isClosed() is now true. If any worker had not yet sent "ready", its rejectReady never runs, so this.ready stays pending. An evaluate call parked on await this.ready at line 783 then never settles, and there is no task timeout.
createCandidateEvaluationQueue awaits NativeCandidateEvaluationQueue.create, which awaits ready first, so callers of the public factory always hold a started queue. The hang needs close() to race a still-starting queue. Reject the readiness promises in close() so the state is unreachable by construction.
🛡️ Proposed fix
async close(): Promise<void> {
if (this.closePromise !== undefined) return this.closePromise
this.closed = true
+ const closeCause = new Error("Candidate evaluation queue closed")
+ for (const slot of this.slots) slot.rejectReady(closeCause)
for (const request of this.requests)
- this.settleRequest(request, new Error("Candidate evaluation queue closed"))
+ this.settleRequest(request, closeCause)
this.readyRequests.length = 0Note that this.ready then rejects. Add a void this.ready.catch(() => undefined) in the constructor, or keep the rejection handled by the existing create() catch, so no unhandled rejection is reported.
📝 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.
| async close(): Promise<void> { | |
| if (this.closePromise !== undefined) return this.closePromise | |
| this.closed = true | |
| for (const request of this.requests) | |
| this.settleRequest(request, new Error("Candidate evaluation queue closed")) | |
| this.readyRequests.length = 0 | |
| for (const slot of this.slots) slot.busy = false | |
| this.closePromise = Promise.all( | |
| this.slots.map((slot) => slot.worker.terminate().catch(() => -1)), | |
| ).then(() => undefined) | |
| return this.closePromise | |
| } | |
| async close(): Promise<void> { | |
| if (this.closePromise !== undefined) return this.closePromise | |
| this.closed = true | |
| const closeCause = new Error("Candidate evaluation queue closed") | |
| for (const slot of this.slots) slot.rejectReady(closeCause) | |
| for (const request of this.requests) | |
| this.settleRequest(request, closeCause) | |
| this.readyRequests.length = 0 | |
| for (const slot of this.slots) slot.busy = false | |
| this.closePromise = Promise.all( | |
| this.slots.map((slot) => slot.worker.terminate().catch(() => -1)), | |
| ).then(() => undefined) | |
| return this.closePromise | |
| } |
🤖 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/retrieval/worker-pool.ts` around lines 821 - 832, Update
NativeCandidateEvaluationQueue.close() to reject any pending readiness promise
before terminating workers, ensuring callers awaiting this.ready settle when
closing during startup. Preserve the existing close idempotency and
request-settlement behavior, and attach a rejection handler in the constructor
or rely on the existing create() catch so the intentional this.ready rejection
is handled.
| expect( | ||
| parallelHoldout.results[0]?.searchDiagnostics.timings.candidateEvaluationMs, | ||
| ).toBeGreaterThan(0) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how candidateEvaluationMs is accumulated and what test timeout applies.
set -uo pipefail
echo "=== candidateEvaluationMs accumulation ==="
rg -nP -C6 '\bcandidateEvaluationMs\b' --type=ts benchmarks
echo "=== timing helpers used near candidate evaluation ==="
rg -nP -C3 'performance\.now\(\)|Math\.round|Math\.floor|toFixed' --type=ts benchmarks/retrieval
echo "=== vitest timeout configuration ==="
fd -i -e ts -e mts -e js -e mjs -e json 'vitest.config|vitest.workspace|package.json' --max-depth 3 \
--exec rg -nP -C3 'testTimeout|hookTimeout|"test"' {}
echo "=== explicit timeouts in this test file ==="
fd -g 'worker-pool.test.ts' --exec rg -nP -C2 'it\(|describe\(|timeout' {}Repository: Lucas-Bur/pix
Length of output: 41068
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== test file relevant sections ==="
sed -n '80,170p' benchmarks/tests/worker-pool.test.ts
sed -n '270,325p' benchmarks/tests/worker-pool.test.ts
echo "=== vitest config files ==="
for f in $(git ls-files | grep -Ei '(^|/)vitest\.config(\.(ts|js|mjs|mts))?$|(^|/)vitest\.workspace(\.(ts|js|mjs|mts))?$|(^|/)vitest\.env\.' ); do
echo "--- $f"
sed -n '1,220p' "$f" | grep -n -C 3 -E 'testTimeout|hookTimeout|testTimeout|pool|benchmark|worker|retrieval|evidence|expectInterval|expect|it\(|timeout' || true
done
echo "=== package test scripts/config refs ==="
sed -n '1,120p' package.jsonRepository: Lucas-Bur/pix
Length of output: 8505
Confirm the worker-startup timeout for the router job queue.
candidateEvaluationMs is accumulated with fractional performance.now() deltas, so this assertion should not round to 0. This test still starts one holdout worker plus two router workers and runs two router evaluations; set an explicit test timeout around this it or document the available setup budget for slow CI.
🤖 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/tests/worker-pool.test.ts` around lines 160 - 162, Update the test
case containing the parallelHoldout candidateEvaluationMs assertion to set an
explicit timeout covering startup of the holdout worker, both router workers,
and two router evaluations. Choose a budget that accommodates slow CI while
preserving the existing timing assertion.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/retrieval/evaluation/weight-search.ts (1)
1682-1688: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuarantee that
fullPool.close()runs whenproxyPool.close()rejects.The
finallyblock awaitsproxyPool.close()beforefullPool.close(). If the proxy close rejects, the full pool is never closed and its native workers stay alive for the rest of the benchmark run. Close both pools independently and then propagate any failure.🛡️ Proposed fix
} finally { options.signal?.removeEventListener("abort", closeFullPoolOnAbort) if (closeProxyPoolOnAbort !== undefined) options.signal?.removeEventListener("abort", closeProxyPoolOnAbort) - if (proxyPool !== undefined && proxyPool !== fullPool) await proxyPool.close() - await fullPool.close() + const closures = + proxyPool !== undefined && proxyPool !== fullPool + ? [proxyPool.close(), fullPool.close()] + : [fullPool.close()] + const outcomes = await Promise.allSettled(closures) + const failure = outcomes.find((outcome) => outcome.status === "rejected") + if (failure !== undefined && failure.status === "rejected") throw failure.reason }🤖 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/retrieval/evaluation/weight-search.ts` around lines 1682 - 1688, Update the cleanup logic in the finally block around proxyPool and fullPool so both close operations are attempted independently, even when proxyPool.close() rejects. Ensure fullPool.close() always runs, while preserving propagation of close failures after cleanup completes.
♻️ Duplicate comments (2)
benchmarks/retrieval/evaluation/weight-search.ts (1)
1440-1444: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbort handlers discard
close()rejections. Each abort handler starts pool teardown withvoid ...close()and drops the returned promise. If teardown rejects, the rejection is unhandled and can terminate the benchmark process. The shared root cause is the discarded promise in all three handlers.
benchmarks/retrieval/evaluation/weight-search.ts#L1440-L1444: attach a rejection handler topool.close()incloseOnAbortinsidewithCandidatePool.benchmarks/retrieval/evaluation/weight-search.ts#L1647-L1649: attach the same rejection handler tofullPool.close()incloseFullPoolOnAbort.benchmarks/retrieval/evaluation/weight-search.ts#L1668-L1670: attach the same rejection handler toproxyPool.close()incloseProxyPoolOnAbort.🤖 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/retrieval/evaluation/weight-search.ts` around lines 1440 - 1444, Prevent unhandled teardown rejections in the abort handlers by attaching a rejection handler to each pool-close promise. Update closeOnAbort within withCandidatePool to handle pool.close(), closeFullPoolOnAbort to handle fullPool.close(), and closeProxyPoolOnAbort to handle proxyPool.close(); apply the same established rejection handling at all three sites.benchmarks/retrieval/execution/candidate-evaluation-pool.ts (1)
575-581: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject pending readiness in
close()to remove a hang path.
close()setsthis.closed = trueand terminates the workers.attachFusionWorkerLifecyclethen suppressesonErrorfor the resulting exit events, becauseisClosed()returns true. A slot that has not yet sentreadynever rejects, sothis.readystays pending. Anevaluatecall parked onawait this.readyat Line 538 never settles, and no task timeout exists. Reject the slot readiness promises inclose().🛡️ Proposed fix
async close(): Promise<void> { if (this.closePromise !== undefined) return this.closePromise this.closed = true + const closeCause = new Error("Candidate evaluation queue closed") + for (const slot of this.slots) slot.rejectReady(closeCause) for (const request of this.requests) - this.settleRequest(request, new Error("Candidate evaluation queue closed")) + this.settleRequest(request, closeCause) this.readyRequests.length = 0
this.readythen rejects. Keep that rejection handled by the existingcreate()catch, or addvoid this.ready.catch(() => undefined)in the constructor, so no unhandled rejection is reported.🤖 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/retrieval/execution/candidate-evaluation-pool.ts` around lines 575 - 581, Update close() to reject every pending slot readiness promise when closing, so evaluate calls awaiting this.ready settle instead of hanging; use the slot readiness state exposed by the existing pool implementation. Ensure the resulting rejection remains handled through create()’s existing catch, or add constructor-level rejection handling for this.ready to prevent unhandled-rejection reports.
🧹 Nitpick comments (8)
docs/adr/0019-evidence-based-fusion-and-optimization-profiles.md (1)
132-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord the known limit of the linear model at small corpora.
The line
T_develop(N) ~= 32.15 + 0.14 * Npredicts about 45 s forN = 91, but the measured point in the table above is8.98 s. The fixed term dominates at smallN, so the model overpredicts the smallest corpus by roughly five times. The text explains that the fit is empirical, but it does not state this residual.Add one sentence that the fixed term is calibrated by the two larger points and that the model must not be used below a few hundred chunks.
🤖 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 `@docs/adr/0019-evidence-based-fusion-and-optimization-profiles.md` around lines 132 - 144, Add a sentence immediately after the linear-model equation noting that its fixed term is calibrated from the two larger measured points, that it substantially overpredicts the smallest corpus, and that the model must not be used for corpora below a few hundred chunks.Source: Path instructions
benchmarks/retrieval/runner.ts (1)
145-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
serialSearchduplicatessearchOptions.workerCount === 0.The runner passes both
serialSearchandsearchOptions. The two values can disagree if a future caller setsworkerCount: 0without settingserialSearch.runBenchmarkSearchcan derive the serial mode fromsearchOptions.workerCountand drop the extra parameter.This is a benchmark-internal API, so the change stays local to
search.tsand this call site.🤖 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/retrieval/runner.ts` around lines 145 - 176, The runBenchmarkSearch API redundantly receives serialSearch alongside searchOptions.workerCount. Remove the serialSearch parameter from runBenchmarkSearch and its callers, derive serial behavior from searchOptions.workerCount inside runBenchmarkSearch, and update this invocation while preserving the existing worker-count configuration.benchmarks/BASELINE.md (1)
203-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the Schema-19 comparison out of the Schema 15 section.
This subsection sits under
## Schema 15: Successive Halving Router Search, but its evidence is a Schema-19 comparison between two later artifacts. The current placement suggests the artifacts belong to Schema 15. Put the subsection under its own schema heading, or state at the start of the subsection why it lives in the Schema 15 section.🤖 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/BASELINE.md` around lines 203 - 216, Move the “Strategy Equivalence Check” subsection out from under “Schema 15: Successive Halving Router Search” and place it under a distinct Schema-19 heading, preserving its comparison details and conclusions unchanged.Source: Path instructions
benchmarks/retrieval/evaluation/search.ts (2)
416-428: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffBound the number of concurrently active router jobs.
Promise.allstarts every planned router job at once. Each job keeps its own beam state, archive, and prepared candidate snapshots on the main thread, so peak memory grows withallRouterJobs.length. For thefullprofile the ADR documentsJ = 27jobs over roughly 6,888 chunks. The shared queue bounds worker execution, but it does not bound main-thread job state.Consider running jobs with a concurrency limit that is derived from the worker budget, so the queue stays saturated without holding all job state at the same time.
♻️ Proposed bounded-concurrency variant
canParallelize - ? runParallelSearch((signal) => - Promise.all( - allRouterJobs.map((job) => - runRouterSearchJob(job, optimizationProfile, { - ...searchOptions, - workerCount: 0, - evaluationQueue: candidateQueue, - signal, - }), - ), - ), - ) + ? Effect.forEach( + allRouterJobs, + (job) => + runParallelSearch((signal) => + runRouterSearchJob(job, optimizationProfile, { + ...searchOptions, + workerCount: 0, + evaluationQueue: candidateQueue, + signal, + }), + ), + { concurrency: Math.max(2, candidateQueue?.workerCount ?? 2) }, + ) : Effect.forEach(🤖 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/retrieval/evaluation/search.ts` around lines 416 - 428, Replace the unbounded Promise.all over allRouterJobs in the canParallelize branch with a bounded-concurrency scheduler. Derive the maximum active router jobs from the configured worker budget, keep the shared evaluationQueue and signal behavior unchanged, and ensure every job completes while limiting main-thread job state to the concurrency cap.
460-519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord the queue shutdown duration even when
close()fails.
Effect.orDieconverts a failedclose()into a defect. In that case the assignment on Line 502 never runs, andcandidateQueueShutdownDurationMsstays0. The artifact then reports a shutdown duration of zero for a run that actually spent time in shutdown. Set the elapsed time before the failure can propagate.♻️ Proposed timing fix
: Effect.orDie( Effect.gen(function* () { const queueStartedAt = performance.now() - yield* runParallelSearch(() => candidateQueue!.close()) - candidateQueueShutdownDurationMs = performance.now() - queueStartedAt + yield* Effect.ensuring( + runParallelSearch(() => candidateQueue!.close()), + Effect.sync(() => { + candidateQueueShutdownDurationMs = performance.now() - queueStartedAt + }), + ) }), ),🤖 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/retrieval/evaluation/search.ts` around lines 460 - 519, Update the candidate queue shutdown block inside the Effect.ensuring finalizer so candidateQueueShutdownDurationMs is assigned immediately after runParallelSearch(() => candidateQueue!.close()) completes or fails, before Effect.orDie propagates the failure. Preserve the existing queue cleanup and failure behavior while ensuring failed close attempts report their elapsed shutdown time.benchmarks/retrieval/evaluation/collect.ts (1)
226-229: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAppend to the grouped array instead of copying it.
Each iteration rebuilds the whole array for the query kind. That makes grouping quadratic in the number of samples per kind. Push into a mutable array instead.
♻️ Proposed refactor
- samplesByQueryKind.set(entry.queryKind, [ - ...(samplesByQueryKind.get(entry.queryKind) ?? []), - sample, - ]) + const kindSamples = samplesByQueryKind.get(entry.queryKind) + if (kindSamples === undefined) samplesByQueryKind.set(entry.queryKind, [sample]) + else kindSamples.push(sample)🤖 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/retrieval/evaluation/collect.ts` around lines 226 - 229, Update the grouping logic around samplesByQueryKind to retrieve or initialize one mutable array per entry.queryKind, then append sample with push instead of reconstructing and resetting the array on every iteration. Preserve the existing grouping behavior while avoiding repeated array copies.benchmarks/retrieval/execution/sqlite-index.ts (1)
45-57: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winShare one benchmark store scope per model instead of two.
withSqliteBenchmarkStorebuildssqliteBenchmarkLayeron every call, so each call constructs a newSqliteIndexStoreand a newSparseEmbedder.benchmarks/retrieval/evaluation/collect.tscalls this helper twice per model: once ininspectBenchmarkCacheat Line 441 and once incollectModelMeasurementsat Line 358. That loads the sparse embedder twice per model and opens the same database file twice. Consider exposing the scoped layer so the caller can inspect the cache and collect measurements inside one scope.🤖 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/retrieval/execution/sqlite-index.ts` around lines 45 - 57, Refactor withSqliteBenchmarkStore and its callers so sqliteBenchmarkLayer is constructed and scoped once per model, then reuse that scope for both inspectBenchmarkCache and collectModelMeasurements. Expose the scoped store/layer to the evaluation flow while preserving ensureBenchmarkCacheTable and both operations’ existing behavior.benchmarks/retrieval/execution/candidate-evaluation-worker.mjs (1)
15-19: 🩺 Stability & Availability | 🔵 TrivialConsider bounding the worker snapshot cache.
snapshotsgrows for each newsnapshotIdand never releases an entry. A shared queue serves many router jobs in one benchmark run, so every worker retains the contribution matrices of every prepared snapshot until the worker exits. For large corpora this raises peak memory per worker. Consider adding a release message that the queue sends when a pool that owns a snapshot closes, and clear the matching entry inslot.knownSnapshots.🤖 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/retrieval/execution/candidate-evaluation-worker.mjs` around lines 15 - 19, Bound the worker snapshot cache by adding a release message for closed snapshot-owning pools and handling it in the candidate evaluation worker. When release is received, remove the matching snapshot from the worker’s snapshots cache and clear the corresponding entry in slot.knownSnapshots, while preserving normal snapshot reuse for active jobs.
🤖 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/BASELINE.md`:
- Line 4: Update the schema-23 cache statement in BASELINE.md to distinguish
absent benchmark-owned embedding vector caches from the existing benchmark-owned
SQLite index and ranking cache, matching the persisted retrieval cache behavior
described in the ADR.
In `@docs/adr/0019-evidence-based-fusion-and-optimization-profiles.md`:
- Line 83: Reorder the ADR sections so the required top-level sequence remains
Status → Context → Decision → Rationale → Consequences. Move the content under
“Runtime Estimation” after “## Consequences,” or convert it into a subsection
within “## Rationale,” without leaving it as a top-level section between
Rationale and Consequences.
- Around line 112-115: The ADR text should not describe the shared candidate
queue as inherently eleven-worker. Update the passage to state that jobs share a
candidate queue whose size is bounded by the host CPU budget and configured
worker count, and identify 11 only as the value measured on the calibration
machine; preserve the existing explanation of job overlap and K/H holdout
factors.
---
Outside diff comments:
In `@benchmarks/retrieval/evaluation/weight-search.ts`:
- Around line 1682-1688: Update the cleanup logic in the finally block around
proxyPool and fullPool so both close operations are attempted independently,
even when proxyPool.close() rejects. Ensure fullPool.close() always runs, while
preserving propagation of close failures after cleanup completes.
---
Duplicate comments:
In `@benchmarks/retrieval/evaluation/weight-search.ts`:
- Around line 1440-1444: Prevent unhandled teardown rejections in the abort
handlers by attaching a rejection handler to each pool-close promise. Update
closeOnAbort within withCandidatePool to handle pool.close(),
closeFullPoolOnAbort to handle fullPool.close(), and closeProxyPoolOnAbort to
handle proxyPool.close(); apply the same established rejection handling at all
three sites.
In `@benchmarks/retrieval/execution/candidate-evaluation-pool.ts`:
- Around line 575-581: Update close() to reject every pending slot readiness
promise when closing, so evaluate calls awaiting this.ready settle instead of
hanging; use the slot readiness state exposed by the existing pool
implementation. Ensure the resulting rejection remains handled through
create()’s existing catch, or add constructor-level rejection handling for
this.ready to prevent unhandled-rejection reports.
---
Nitpick comments:
In `@benchmarks/BASELINE.md`:
- Around line 203-216: Move the “Strategy Equivalence Check” subsection out from
under “Schema 15: Successive Halving Router Search” and place it under a
distinct Schema-19 heading, preserving its comparison details and conclusions
unchanged.
In `@benchmarks/retrieval/evaluation/collect.ts`:
- Around line 226-229: Update the grouping logic around samplesByQueryKind to
retrieve or initialize one mutable array per entry.queryKind, then append sample
with push instead of reconstructing and resetting the array on every iteration.
Preserve the existing grouping behavior while avoiding repeated array copies.
In `@benchmarks/retrieval/evaluation/search.ts`:
- Around line 416-428: Replace the unbounded Promise.all over allRouterJobs in
the canParallelize branch with a bounded-concurrency scheduler. Derive the
maximum active router jobs from the configured worker budget, keep the shared
evaluationQueue and signal behavior unchanged, and ensure every job completes
while limiting main-thread job state to the concurrency cap.
- Around line 460-519: Update the candidate queue shutdown block inside the
Effect.ensuring finalizer so candidateQueueShutdownDurationMs is assigned
immediately after runParallelSearch(() => candidateQueue!.close()) completes or
fails, before Effect.orDie propagates the failure. Preserve the existing queue
cleanup and failure behavior while ensuring failed close attempts report their
elapsed shutdown time.
In `@benchmarks/retrieval/execution/candidate-evaluation-worker.mjs`:
- Around line 15-19: Bound the worker snapshot cache by adding a release message
for closed snapshot-owning pools and handling it in the candidate evaluation
worker. When release is received, remove the matching snapshot from the worker’s
snapshots cache and clear the corresponding entry in slot.knownSnapshots, while
preserving normal snapshot reuse for active jobs.
In `@benchmarks/retrieval/execution/sqlite-index.ts`:
- Around line 45-57: Refactor withSqliteBenchmarkStore and its callers so
sqliteBenchmarkLayer is constructed and scoped once per model, then reuse that
scope for both inspectBenchmarkCache and collectModelMeasurements. Expose the
scoped store/layer to the evaluation flow while preserving
ensureBenchmarkCacheTable and both operations’ existing behavior.
In `@benchmarks/retrieval/runner.ts`:
- Around line 145-176: The runBenchmarkSearch API redundantly receives
serialSearch alongside searchOptions.workerCount. Remove the serialSearch
parameter from runBenchmarkSearch and its callers, derive serial behavior from
searchOptions.workerCount inside runBenchmarkSearch, and update this invocation
while preserving the existing worker-count configuration.
In `@docs/adr/0019-evidence-based-fusion-and-optimization-profiles.md`:
- Around line 132-144: Add a sentence immediately after the linear-model
equation noting that its fixed term is calibrated from the two larger measured
points, that it substantially overpredicts the smallest corpus, and that the
model must not be used for corpora below a few hundred chunks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d76a7c91-7a56-47bc-b4c5-77879fa1fd24
📒 Files selected for processing (31)
CONTEXT.mdbenchmarks/BASELINE.mdbenchmarks/README.mdbenchmarks/retrieval/corpus/prepare.tsbenchmarks/retrieval/corpus/repository.tsbenchmarks/retrieval/evaluation/baseline.tsbenchmarks/retrieval/evaluation/collect.tsbenchmarks/retrieval/evaluation/folds.tsbenchmarks/retrieval/evaluation/metrics.tsbenchmarks/retrieval/evaluation/optimization-profiles.tsbenchmarks/retrieval/evaluation/prepared-fusion-core.d.mtsbenchmarks/retrieval/evaluation/prepared-fusion-core.mjsbenchmarks/retrieval/evaluation/prepared-fusion.tsbenchmarks/retrieval/evaluation/ranking.tsbenchmarks/retrieval/evaluation/report.tsbenchmarks/retrieval/evaluation/search.tsbenchmarks/retrieval/evaluation/types.tsbenchmarks/retrieval/evaluation/weight-search.tsbenchmarks/retrieval/execution/benchmark-cache.tsbenchmarks/retrieval/execution/candidate-evaluation-pool.tsbenchmarks/retrieval/execution/candidate-evaluation-worker.mjsbenchmarks/retrieval/execution/sqlite-index.tsbenchmarks/retrieval/runner.tsbenchmarks/retrieval/sqlite-index.tsbenchmarks/tests/benchmark-cache.test.tsbenchmarks/tests/channels.test.tsbenchmarks/tests/corpus.test.tsbenchmarks/tests/optimization-profiles.test.tsbenchmarks/tests/retrieval.test.tsbenchmarks/tests/worker-pool.test.tsdocs/adr/0019-evidence-based-fusion-and-optimization-profiles.md
💤 Files with no reviewable changes (1)
- benchmarks/retrieval/sqlite-index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- benchmarks/tests/worker-pool.test.ts
- benchmarks/tests/channels.test.ts
| identifier coverage, query length, score geometry, and channel agreement. Explicit user-selected profiles | ||
| may be added later. | ||
|
|
||
| ## Runtime Estimation |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the required ADR section order.
## Runtime Estimation is inserted between ## Rationale and ## Consequences. The required ADR structure is Status → Context → Decision → Rationale → Consequences. Move this content after ## Consequences, or fold it into ## Rationale as a subsection, so the five required sections stay in order.
As per path instructions: "ADRs in docs/adr/ follow the format: Status → Context → Decision → Rationale → Consequences."
🤖 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 `@docs/adr/0019-evidence-based-fusion-and-optimization-profiles.md` at line 83,
Reorder the ADR sections so the required top-level sequence remains Status →
Context → Decision → Rationale → Consequences. Move the content under “Runtime
Estimation” after “## Consequences,” or convert it into a subsection within “##
Rationale,” without leaving it as a top-level section between Rationale and
Consequences.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/BASELINE.md (1)
39-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse schema 24 in the historical-format note.
Lines 39-42 state that schema 23 is the current artifact format. This file and
benchmarks/README.mddocument schema 24 as current. Change the reference to schema 24, or identify the specific intermediate format if schema 23 is intentional.🤖 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/BASELINE.md` around lines 39 - 42, Update the historical-format note in BASELINE.md to reference schema 24 as the current artifact format, consistent with benchmarks/README.md; only retain schema 23 if the note explicitly identifies it as an intentional intermediate format.
🧹 Nitpick comments (2)
benchmarks/retrieval/evaluation/report.ts (1)
74-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the factor label and the factor value from one check.
Line 74 selects the label from a substring match on
algorithm. Lines 77-80 select the value from a property check onhalvingKeepFactor. Two independent discriminators describe one decision. If a future strategy setshalvingKeepFactorwithout the matching algorithm name, the report printspromotion factorbeside a keep factor.Use the property check for both.
♻️ Proposed refactor
- const strategyFactorLabel = artifact.searchStrategy.algorithm.includes("successive-halving") - ? "keep" - : "promotion" - const strategyFactor = - "halvingKeepFactor" in artifact.searchStrategy - ? artifact.searchStrategy.halvingKeepFactor - : artifact.searchStrategy.proxyPromotionFactor + const usesHalvingKeepFactor = "halvingKeepFactor" in artifact.searchStrategy + const strategyFactorLabel = usesHalvingKeepFactor ? "keep" : "promotion" + const strategyFactor = usesHalvingKeepFactor + ? artifact.searchStrategy.halvingKeepFactor + : artifact.searchStrategy.proxyPromotionFactor🤖 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/retrieval/evaluation/report.ts` around lines 74 - 80, Update the strategy-factor derivation around strategyFactorLabel and strategyFactor to use the same "halvingKeepFactor" property check for both decisions. Assign the keep label when that property exists and the promotion label otherwise, while preserving the corresponding factor values.benchmarks/retrieval/evaluation/types.ts (1)
82-92: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRecord the shared provenance fields for
successive-halvingtoo.The
successive-halvingentry omitsseed,normalization,tieBreaking,guardrailTolerance, andobjectives.BenchmarkArtifact.searchStrategystores this object verbatim, so successive-halving artifacts no longer describe the seed, the weight normalization, or the tie-breaking rule. That reduces reproducibility of the comparison the strategy exists for.Add the applicable fields with values that match the implemented behavior, for example
tieBreaking: "objective>complexity"and the sameseedandnormalizationused by the shared search 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 `@benchmarks/retrieval/evaluation/types.ts` around lines 82 - 92, Update the "successive-halving" strategy entry in the strategy configuration to include the shared provenance fields seed, normalization, tieBreaking, guardrailTolerance, and objectives, using the same values as the implemented shared search behavior. Preserve its existing algorithm and tuning parameters, and set tieBreaking to the applicable "objective>complexity" 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 `@benchmarks/BASELINE.md`:
- Around line 33-35: Update the benchmark comparison note in BASELINE.md to
remove the incorrect claim that embeddings were reused from a persistent cache.
Describe the actual reused retrieval or index artifacts if applicable, using the
benchmark documentation as the source of truth; otherwise state that no
embedding cache was used.
In `@benchmarks/README.md`:
- Around line 158-163: Update both benchmarks/README.md sections at lines
158-163 and 320-326 to document both candidate-evaluation modes: qualify the
strategy comparison and architecture description as applying in worker mode, and
mention that workerCount <= 1 uses SerialCandidateEvaluationQueue as the serial
fallback instead of the native worker queue.
In `@benchmarks/tests/worker-pool.test.ts`:
- Around line 165-195: Add an explicit extended timeout to the heavy “runs the
historical halving stage through the worker queue” test, and apply the same
timeout treatment to the earlier queue test in this file. Keep the test
assertions and queue cleanup unchanged.
---
Outside diff comments:
In `@benchmarks/BASELINE.md`:
- Around line 39-42: Update the historical-format note in BASELINE.md to
reference schema 24 as the current artifact format, consistent with
benchmarks/README.md; only retain schema 23 if the note explicitly identifies it
as an intentional intermediate format.
---
Nitpick comments:
In `@benchmarks/retrieval/evaluation/report.ts`:
- Around line 74-80: Update the strategy-factor derivation around
strategyFactorLabel and strategyFactor to use the same "halvingKeepFactor"
property check for both decisions. Assign the keep label when that property
exists and the promotion label otherwise, while preserving the corresponding
factor values.
In `@benchmarks/retrieval/evaluation/types.ts`:
- Around line 82-92: Update the "successive-halving" strategy entry in the
strategy configuration to include the shared provenance fields seed,
normalization, tieBreaking, guardrailTolerance, and objectives, using the same
values as the implemented shared search behavior. Preserve its existing
algorithm and tuning parameters, and set tieBreaking to the applicable
"objective>complexity" value.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 06e62ae9-5de9-47b3-8161-46bf73833f4d
📒 Files selected for processing (9)
CONTEXT.mdbenchmarks/BASELINE.mdbenchmarks/README.mdbenchmarks/retrieval/evaluation/report.tsbenchmarks/retrieval/evaluation/types.tsbenchmarks/retrieval/evaluation/weight-search.tsbenchmarks/retrieval/runner.tsbenchmarks/tests/retrieval.test.tsbenchmarks/tests/worker-pool.test.ts
| Successive Halving was 96.8% faster on the larger Effect v4 control. The worker queue completed both | ||
| strategies successfully; the large runtime gap comes from candidate selection/evaluation volume, not | ||
| embedding, which was reused from the persistent cache. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the cache explanation for the runtime comparison.
Line 35 says that embedding was reused from a persistent cache. benchmarks/README.md Lines 105-106 state that each model run computes Dense and Sparse vectors once and has no benchmark embedding cache. If the run reused retrieval or index artifacts, name those artifacts instead of saying that the embedding was reused.
🤖 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/BASELINE.md` around lines 33 - 35, Update the benchmark comparison
note in BASELINE.md to remove the incorrect claim that embeddings were reused
from a persistent cache. Describe the actual reused retrieval or index artifacts
if applicable, using the benchmark documentation as the source of truth;
otherwise state that no embedding cache was used.
Source: Path instructions
| The router search defaults to `proxy-promotion`. Set `PIX_BENCH_ROUTER_STRATEGY` to | ||
| `successive-halving` to select the historical Successive-Halving variant. It uses the original | ||
| lexicographic `R@20`, `R@10`, `Context@4k`, and MRR comparator plus its `halvingKeepFactor`. | ||
| Both strategies use the same candidate evaluator and native worker queue, so their artifacts can be | ||
| compared directly. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document both candidate-evaluation modes.
The README describes native worker execution as unconditional. The queue factory selects SerialCandidateEvaluationQueue when workerCount <= 1, while worker mode uses the native queue.
benchmarks/README.md#L158-L163: qualify the strategy comparison within worker modeand mention serial fallback.benchmarks/README.md#L320-L326: apply the same qualification to the architecture description.
📍 Affects 1 file
benchmarks/README.md#L158-L163(this comment)benchmarks/README.md#L320-L326
🤖 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/README.md` around lines 158 - 163, Update both
benchmarks/README.md sections at lines 158-163 and 320-326 to document both
candidate-evaluation modes: qualify the strategy comparison and architecture
description as applying in worker mode, and mention that workerCount <= 1 uses
SerialCandidateEvaluationQueue as the serial fallback instead of the native
worker queue.
| it("runs the historical halving stage through the worker queue", async () => { | ||
| const candidateQueue = await createCandidateEvaluationQueue({ workerCount: 2 }) | ||
| try { | ||
| const parallel = await fitRecommendedEvidenceRouter( | ||
| "fixture", | ||
| "dbsf", | ||
| halvingSamples, | ||
| SEARCH_PRIORITY_PROFILE, | ||
| { | ||
| workerCount: 0, | ||
| evaluationQueue: candidateQueue, | ||
| routerSearchStrategy: "successive-halving", | ||
| }, | ||
| ) | ||
| const serial = await fitRecommendedEvidenceRouter( | ||
| "fixture", | ||
| "dbsf", | ||
| halvingSamples, | ||
| SEARCH_PRIORITY_PROFILE, | ||
| { workerCount: 0, routerSearchStrategy: "successive-halving" }, | ||
| ) | ||
| expect(parallel.map(withoutSearchTimings)).toEqual(serial.map(withoutSearchTimings)) | ||
| const result = parallel[0] | ||
| if (result === undefined) throw new Error("Missing halving router result") | ||
| expect(result.searchDiagnostics.proxyEvaluations).toBeGreaterThan(0) | ||
| expect(result.searchDiagnostics.proxyPromotions).toBeGreaterThan(0) | ||
| expect(result.searchDiagnostics.timings.randomSearchMs).toBe(0) | ||
| } finally { | ||
| await candidateQueue.close() | ||
| } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Set an explicit timeout for this test.
This test runs the full beam search twice over 40 samples, once through a two-worker queue and once serially. It is the heaviest case in the file. Add an explicit timeout to the it call so slow CI machines do not fail on the default budget. The same concern was raised for the earlier queue test in this file.
🤖 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/tests/worker-pool.test.ts` around lines 165 - 195, Add an explicit
extended timeout to the heavy “runs the historical halving stage through the
worker queue” test, and apply the same timeout treatment to the earlier queue
test in this file. Keep the test assertions and queue cleanup unchanged.
d688154 to
6be00ba
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
benchmarks/retrieval/evaluation/folds.ts (1)
55-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a locale-independent tie breaker.
localeCompare()without an explicit locale uses host-default collation. If twostableHash()values collide, different host locales can order their keys differently and change fold assignment. Use binary string comparison for this tie breaker.Proposed change
- .sort((left, right) => left.order - right.order || left.key.localeCompare(right.key)) + .sort( + (left, right) => + left.order - right.order || + (left.key < right.key ? -1 : left.key > right.key ? 1 : 0), + )Verify with colliding keys under different configured host locales.
🤖 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/retrieval/evaluation/folds.ts` at line 55, Replace the locale-dependent tie breaker in the fold sorting chain with binary string comparison of left.key and right.key, while preserving the primary left.order comparison. Ensure colliding stableHash() keys receive deterministic ordering regardless of the host locale, and verify this behavior with colliding keys under different configured locales.
🤖 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.
Nitpick comments:
In `@benchmarks/retrieval/evaluation/folds.ts`:
- Line 55: Replace the locale-dependent tie breaker in the fold sorting chain
with binary string comparison of left.key and right.key, while preserving the
primary left.order comparison. Ensure colliding stableHash() keys receive
deterministic ordering regardless of the host locale, and verify this behavior
with colliding keys under different configured locales.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e29eba6-09e3-4e32-9a8d-6c9ef439820b
📒 Files selected for processing (31)
CONTEXT.mdbenchmarks/BASELINE.mdbenchmarks/README.mdbenchmarks/retrieval/corpus/prepare.tsbenchmarks/retrieval/corpus/repository.tsbenchmarks/retrieval/evaluation/baseline.tsbenchmarks/retrieval/evaluation/collect.tsbenchmarks/retrieval/evaluation/folds.tsbenchmarks/retrieval/evaluation/metrics.tsbenchmarks/retrieval/evaluation/optimization-profiles.tsbenchmarks/retrieval/evaluation/prepared-fusion-core.d.mtsbenchmarks/retrieval/evaluation/prepared-fusion-core.mjsbenchmarks/retrieval/evaluation/prepared-fusion.tsbenchmarks/retrieval/evaluation/ranking.tsbenchmarks/retrieval/evaluation/report.tsbenchmarks/retrieval/evaluation/search.tsbenchmarks/retrieval/evaluation/types.tsbenchmarks/retrieval/evaluation/weight-search.tsbenchmarks/retrieval/execution/benchmark-cache.tsbenchmarks/retrieval/execution/candidate-evaluation-pool.tsbenchmarks/retrieval/execution/candidate-evaluation-worker.mjsbenchmarks/retrieval/execution/sqlite-index.tsbenchmarks/retrieval/runner.tsbenchmarks/retrieval/sqlite-index.tsbenchmarks/tests/benchmark-cache.test.tsbenchmarks/tests/channels.test.tsbenchmarks/tests/corpus.test.tsbenchmarks/tests/optimization-profiles.test.tsbenchmarks/tests/retrieval.test.tsbenchmarks/tests/worker-pool.test.tsdocs/adr/0019-evidence-based-fusion-and-optimization-profiles.md
💤 Files with no reviewable changes (1)
- benchmarks/retrieval/sqlite-index.ts
🚧 Files skipped from review as they are similar to previous changes (28)
- benchmarks/tests/corpus.test.ts
- benchmarks/retrieval/evaluation/prepared-fusion-core.d.mts
- benchmarks/retrieval/execution/candidate-evaluation-worker.mjs
- benchmarks/retrieval/corpus/prepare.ts
- benchmarks/retrieval/evaluation/prepared-fusion-core.mjs
- benchmarks/retrieval/evaluation/optimization-profiles.ts
- benchmarks/README.md
- benchmarks/retrieval/evaluation/metrics.ts
- benchmarks/retrieval/evaluation/prepared-fusion.ts
- benchmarks/tests/optimization-profiles.test.ts
- benchmarks/retrieval/evaluation/report.ts
- benchmarks/retrieval/evaluation/baseline.ts
- benchmarks/retrieval/runner.ts
- benchmarks/retrieval/evaluation/search.ts
- benchmarks/tests/channels.test.ts
- benchmarks/retrieval/evaluation/collect.ts
- benchmarks/retrieval/execution/sqlite-index.ts
- benchmarks/tests/benchmark-cache.test.ts
- benchmarks/retrieval/execution/candidate-evaluation-pool.ts
- benchmarks/retrieval/evaluation/ranking.ts
- docs/adr/0019-evidence-based-fusion-and-optimization-profiles.md
- CONTEXT.md
- benchmarks/tests/retrieval.test.ts
- benchmarks/BASELINE.md
- benchmarks/retrieval/execution/benchmark-cache.ts
- benchmarks/retrieval/evaluation/weight-search.ts
- benchmarks/retrieval/evaluation/types.ts
- benchmarks/tests/worker-pool.test.ts
1 participant
Summary
Refs #166.
ode:worker_threads pool with bounded batching, deterministic result ordering, serial fallback, and lifecycle/error cleanup.
Validation
The complete validation matrix, timing breakdown instrumentation, robust promotion protocol, and NDCG objective remain follow-up work under #166.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation