fix(edit_block): run fuzzy search in a worker thread to keep event lo…#500
Merged
Conversation
…op responsive When edit_block found no exact match, the fuzzy-search fallback ran recursiveFuzzyIndexOf() synchronously on the main thread. On large files this pinned the event loop for seconds, and several parallel edit_block calls serialized their scans back-to-back, freezing pings and all other tool calls — the server appeared hung. The scan now runs in a worker thread via runFuzzySearchInWorker(): - inline eval worker that imports this module, so no separate worker file ships with the build - 30s timeout terminates runaway scans (errors surface via the existing handleEditBlock catch) - worker and timer are unref'd so a running scan never delays shutdown Measured: 4 parallel 2MB scans drop from 16.0s (serialized) to 4.8s (concurrent), with max ping latency 1-14ms during scans vs ~3.5s before. Adds a regression scenario to the edit-block performance integration test that pings every 200ms during a deliberately slow scan and fails if max latency exceeds 500ms (the existing 5s responsiveness probe was too loose to catch this). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthroughFuzzy-search execution is moved from the main thread to an isolated worker thread with timeout enforcement; edit flow now calls the worker-based search, and an integration test measures event-loop responsiveness during large fuzzy scans. ChangesFuzzy Search Worker Implementation
Sequence Diagram(s)sequenceDiagram
participant performSearchReplace
participant runFuzzySearchInWorker as runFuzzySearchInWorker()
participant Worker
participant runFuzzySearch as runFuzzySearch()
participant recursiveFuzzyIndexOf as recursiveFuzzyIndexOf()
performSearchReplace->>runFuzzySearchInWorker: call(text, query)
runFuzzySearchInWorker->>Worker: spawn with WORKER_CODE (moduleUrl, text, query)
Worker->>runFuzzySearch: import module and call runFuzzySearch(text, query)
runFuzzySearch->>recursiveFuzzyIndexOf: recursiveFuzzyIndexOf(text, query)
recursiveFuzzyIndexOf-->>runFuzzySearch: final match + metrics
runFuzzySearch-->>Worker: postMessage({ ok: true, result, metrics })
Worker->>runFuzzySearchInWorker: postMessage(payload)
runFuzzySearchInWorker-->>performSearchReplace: resolve(result) or reject on timeout/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tools/fuzzySearch.ts (1)
164-170: ⚡ Quick winAdd a small-input fast path before worker spawn.
Line 164 currently pays worker startup + clone overhead for every fuzzy fallback. For small payloads, this is a noticeable regression; keep worker offload for large scans, but run
recursiveFuzzyIndexOf(...)inline under a conservative size threshold.💡 Suggested refactor
export function runFuzzySearchInWorker( text: string, query: string, timeoutMs: number = FUZZY_SEARCH_TIMEOUT_MS ): Promise<{ start: number; end: number; value: string; distance: number }> { + // Avoid worker startup overhead for tiny scans. + const SMALL_TEXT_THRESHOLD = 16 * 1024; // 16KB + const SMALL_QUERY_THRESHOLD = 512; // 512B + if (text.length <= SMALL_TEXT_THRESHOLD && query.length <= SMALL_QUERY_THRESHOLD) { + return Promise.resolve(recursiveFuzzyIndexOf(text, query)); + } + return new Promise((resolve, reject) => { const worker = new Worker(WORKER_CODE, { eval: true, workerData: { moduleUrl: import.meta.url, text, query } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/fuzzySearch.ts` around lines 164 - 170, The current runFuzzySearchInWorker always spawns a Worker (using WORKER_CODE and workerData) which is costly for small inputs; add a fast-path at the top of runFuzzySearchInWorker that checks the payload size (e.g. text.length and/or query.length against a conservative threshold constant) and, if under the threshold, calls recursiveFuzzyIndexOf(text, query) inline and returns a resolved Promise with the same shape as the worker result (respecting the timeoutMs parameter by returning immediately); otherwise keep the existing Worker creation logic unchanged. Ensure you reference runFuzzySearchInWorker, recursiveFuzzyIndexOf, WORKER_CODE and honor the timeoutMs argument.test/integration/edit-block-performance.js (1)
693-699: ⚡ Quick winMake the minimum-ping assertion duration-aware.
Lines 693-699 use a fixed
>= 5ping count, which can fail on faster machines if the scan completes quickly even when latency stays healthy. Derive the minimum from measured scan duration (with a floor of 1) to reduce flakiness.💡 Suggested refactor
- assert.ok( - pingLatencies.length >= FUZZY_SCAN_MIN_PING_COUNT, - `event loop blocked during fuzzy scan: only ${pingLatencies.length} ping(s) completed, expected >= ${FUZZY_SCAN_MIN_PING_COUNT}` - ); + const durationBasedMinPingCount = Math.max( + 1, + Math.floor(durationMs / FUZZY_SCAN_PING_INTERVAL_MS) + ); + const minPingCount = Math.min(FUZZY_SCAN_MIN_PING_COUNT, durationBasedMinPingCount); + assert.ok( + pingLatencies.length >= minPingCount, + `event loop blocked during fuzzy scan: only ${pingLatencies.length} ping(s) completed, expected >= ${minPingCount}` + );🤖 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 `@test/integration/edit-block-performance.js` around lines 693 - 699, Replace the fixed FUZZY_SCAN_MIN_PING_COUNT check with a duration-aware minimum: compute the fuzzy scan duration (e.g., scanEnd - scanStart) and derive minPings = Math.max(1, Math.floor(scanDuration / pingIntervalMs)) or another sensible division that yields expected ping count; then assert pingLatencies.length >= minPings and update the failure message to include minPings and the scanDuration. Update the assertion that currently references FUZZY_SCAN_MIN_PING_COUNT to use this computed minPings and ensure you have timestamps (scan start/end) captured around the code that fills pingLatencies so scanDuration and minPings are available for the assertion involving pingLatencies, maxPingLatencyMs and FUZZY_SCAN_MAX_PING_LATENCY_MS.
🤖 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 `@test/integration/edit-block-performance.js`:
- Line 673: The call editPromise.finally(() => { editDone = true; }) creates a
detached promise that can produce an unhandled rejection; update the code in
runFuzzyEventLoopResponsivenessWorkflow so the finally chain attaches a
rejection handler (e.g., append a .catch(...) to the finally chain) or otherwise
handle errors from editPromise so no detached promise can reject, while keeping
the editDone assignment inside the finally handler.
---
Nitpick comments:
In `@src/tools/fuzzySearch.ts`:
- Around line 164-170: The current runFuzzySearchInWorker always spawns a Worker
(using WORKER_CODE and workerData) which is costly for small inputs; add a
fast-path at the top of runFuzzySearchInWorker that checks the payload size
(e.g. text.length and/or query.length against a conservative threshold constant)
and, if under the threshold, calls recursiveFuzzyIndexOf(text, query) inline and
returns a resolved Promise with the same shape as the worker result (respecting
the timeoutMs parameter by returning immediately); otherwise keep the existing
Worker creation logic unchanged. Ensure you reference runFuzzySearchInWorker,
recursiveFuzzyIndexOf, WORKER_CODE and honor the timeoutMs argument.
In `@test/integration/edit-block-performance.js`:
- Around line 693-699: Replace the fixed FUZZY_SCAN_MIN_PING_COUNT check with a
duration-aware minimum: compute the fuzzy scan duration (e.g., scanEnd -
scanStart) and derive minPings = Math.max(1, Math.floor(scanDuration /
pingIntervalMs)) or another sensible division that yields expected ping count;
then assert pingLatencies.length >= minPings and update the failure message to
include minPings and the scanDuration. Update the assertion that currently
references FUZZY_SCAN_MIN_PING_COUNT to use this computed minPings and ensure
you have timestamps (scan start/end) captured around the code that fills
pingLatencies so scanDuration and minPings are available for the assertion
involving pingLatencies, maxPingLatencyMs and FUZZY_SCAN_MAX_PING_LATENCY_MS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecc4ad91-123b-4ee9-8e6f-d1a42de8ddda
📒 Files selected for processing (3)
src/tools/edit.tssrc/tools/fuzzySearch.tstest/integration/edit-block-performance.js
Without an explicit terminate the worker lingers until its module graph drains (the in-worker telemetry HTTPS call alone holds it ~3s), keeping a structured-clone copy of the full file content in memory per call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The worker previously imported fuzzySearch.ts, which pulls in capture.js and through it server.ts — booting the entire app module graph per fuzzy search. That cost ~275ms per call on small files and fired the search telemetry from inside the worker, where the client identity is never initialized. fuzzySearchCore.ts now holds the pure search functions with fastest-levenshtein as its only import; the worker loads just that and returns timing metrics as data, which the main thread captures with the real client id (same event names and payloads as before). Worker round-trip drops to ~18ms. The worker snippet also gained a .catch so import/search failures surface as a structured error instead of an opaque worker crash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tools/fuzzySearchCore.ts`:
- Around line 69-150: The recursiveFuzzyIndexOf flow seeds iterativeReduction
with an uninformed parentDistance (often Infinity) so iterativeReduction can
shrink a slice without that slice's actual measured distance; change the
recursion to propagate a full measured candidate or at minimum compute the
distance for the current slice (call distance(...) on text.substring(start,end)
or the current left/right slice) and pass that real distance into
iterativeReduction instead of parentDistance; update recursiveFuzzyIndexOf to
use that measured FuzzyMatch (start,end,value,distance) as the baseline when
recursing or calling iterativeReduction, and ensure iterativeReduction
accepts/uses the seeded distance (and ideally a seeded start/end/value) so its
returned value always reflects a distance actually computed for that slice
(refer to recursiveFuzzyIndexOf, iterativeReduction, parentDistance,
bestDistance, and distance).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e7145b6-80e2-4611-bd54-869efee3d93e
📒 Files selected for processing (2)
src/tools/fuzzySearch.tssrc/tools/fuzzySearchCore.ts
The .finally() call created a derived promise with no rejection handler; if edit_block failed it raised unhandledRejection even though editPromise itself is awaited later. Swallow rejections on the detached chain only — errors still surface via the awaited editPromise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
edgarsskore
force-pushed
the
fix/fuzzy-search-worker-thread
branch
from
091cb7f to
357b9d8
Compare
In all recursive paths parentDistance is exact — the parent passes the distance it measured on precisely the child's slice, so the original branch-and-bound seeding was sound there. The one broken path is a top-level call where the whole text fits the small-segment branch (text <= 2x query length): parentDistance defaults to Infinity, the first shrink check always passes, and a match at position 0 can never be returned at position 0 — e.g. matching 'the quick brown fox' in 'the quick brwn fox jumps' returned start 1 / distance 2 instead of start 0 / distance 1. Measuring the slice distance on entry fixes that case and is a no-op for recursive calls (the computed seed equals the value the parent passed; scan timings are unchanged). Costs one extra distance() call per search, the same size as a single shrink-loop iteration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
edgarsskore
force-pushed
the
fix/fuzzy-search-worker-thread
branch
from
357b9d8 to
4110710
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When edit_block found no exact match, the fuzzy-search fallback ran recursiveFuzzyIndexOf() synchronously on the main thread. On large files this pinned the event loop for seconds, and several parallel edit_block calls serialized their scans back-to-back, freezing pings and all other tool calls — the server appeared hung.
The scan now runs in a worker thread via runFuzzySearchInWorker():
Measured: 4 parallel 2MB scans drop from 16.0s (serialized) to 4.8s (concurrent), with max ping latency 1-14ms during scans vs ~3.5s before.
Adds a regression scenario to the edit-block performance integration test that pings every 200ms during a deliberately slow scan and fails if max latency exceeds 500ms (the existing 5s responsiveness probe was too loose to catch this).
Measured impact
Summary by CodeRabbit
Performance Improvements
Tests