mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 08:03:12 +00:00
Neil
8a28de5255
Round-2 review is right that removing `_findInLine`'s recursion (8d5d1d548b) also removed the only thing that stopped an unbounded scan, and that above the old stack-overflow depth the failure mode flipped from a bounded crash to a blocked renderer main thread. It is worse than the review found: the O(rows^2) walk it measured is one of three unbounded scans, and the other two never terminate at all. All three fixed in the addon this branch already patches. 1. regex and wholeWord kept the O(rows^2) walk, because the skip added in6fddfd04e5excluded them. Measured here, one un-newlined line at 80 cols, no match, real Terminal + SearchAddon under happy-dom, quiet host: rows mode upstream branch HEAD this commit 3000 regex 6251 ms 5740 ms 4 ms 3000 wholeWord 6186 ms 5938 ms 2 ms 5000 regex 17369 ms 17112 ms 4 ms 5000 wholeWord 17416 ms 16996 ms 5 ms 12000 regex 11066 ms RangeError 99271 ms 11 ms 12000 wholeWord 11461 ms RangeError 95453 ms 11 ms The cause was `_findInLine`, not the skip. wholeWord returned at the first `indexOf` hit that was not a word, and forward regex bailed on a zero-length first match, so neither was monotone in the start offset and neither could be skipped. Both now scan on within the line, and forward regex drives `exec` over the whole line from `lastIndex` rather than over `slice(offset)`, since a slice re-anchors `^` and `\b` at whatever column the row happened to wrap at. With the matcher monotone the skip is sound for every option and the exclusion list is gone. The wholeWord bail was also a plain defect, exactly as the review reported: `needle` with `wholeWord` in `aneedlea needle done` found nothing upstream and now selects x=9. 2. The rewind never terminated after a reflow. `terminal.resize()` leaves the buffer's `CircularList` holding entries at negative indices (204 of them in the regression test's buffer), so `getLine(-1)` answers with a stale wrapped line instead of undefined and the walk runs backwards forever. Instrumented: 38 million `getLine` calls and still descending, `min=-37999999`. Bounded at row 0, which is as far back as a line start can be. 3. `translateBufferLineToStringWithWrap` never terminated when one line is longer than the whole scrollback — the field report's own input class. Every buffer row is then a continuation and the ring answers an out-of-range row by cycling back to row 0, so the forward walk for the end of the line runs until `strings.push` throws `RangeError: Invalid array length` after ~31 s. Bounded at `buffer.active.length`. 2 and 3 were unreachable upstream, which overflows the stack first, so both arrived with commit 1. Both wedge branch HEAD: the reflow regression test does not fail against it, it hangs past vitest's 30 s timeout, because the loop blocks the event loop the same way it blocks the renderer. Upstream throws RangeError on the same input in 1 ms. Rebuttals, with evidence: - The review's "the obvious way this patch could be worse than the crash — swapping a bounded recursion for an unbounded loop — does not happen" is wrong. `_getCyclicIndex` does go negative and `get()` does return undefined, but only while the ring is unrotated and untouched by reflow, which is what the 60 000-row measurement had. After `resize()` the backing array carries negative keys (verified: 204 of them, `Object.keys` contains `-1`), and when the line outruns the scrollback the forward walk cycles instead. Defects 2 and 3 above. - The review's reflow fuzz being "inconclusive rather than failing" (both attempts over its 40-min and 15-min caps) was not happy-dom being slow. It was defect 2: 119 of the 300 scenarios resize, and the same corpus that ran past 40 minutes now completes in 34 s. - `node config/scripts/regenerate-xterm-patches.mjs --check`, which the review could not run, passes: all four packages in sync, lockfile hash included. Evidence the skip is still behaviour-neutral: 300 randomized scenarios (seeded; random cols/rows/scrollback, blob and short-line mixes with and without trimmed heads, 119 with a `resize()` reflow, findNext/findPrevious sequences, options {} / caseSensitive / wholeWord / regex / wholeWord+caseSensitive / incremental, comparing full transcripts of every selection range and resultCount) against a build of this same source with the skip forced off: 0 divergences. The corpus has the power to see an unsound skip — the same 300 scenarios report 60 divergences against a build that keeps this skip but restores upstream's matcher. Not behaviour-neutral against upstream, and deliberately so: 60 of those 300 scenarios differ, all in the families this branch is fixing — a line longer than the scrollback (upstream finds nothing, or over-counts matches by walking the ring twice), a trimmed line head, and whole-word hits upstream gives up on. 4 of the 60 are scenarios where upstream throws RangeError. Also fixes the doc drift the review flagged: xterm-patch-regeneration.md still said the generator covers `addon-webgl` and `addon-serialize`, "all three"; xterm-upstream.json has listed four packages since this branch added `addon-search`. Still not covered, and why: the perf test asserts only the no-match shape. The many-matches shape is dominated by `_bufferColsToStringOffset`, which this patch does not touch.