docs(ingest): correct the segfault's blast radius, measured end to end - #650
Open
josephismikhail wants to merge 28 commits into
Open
docs(ingest): correct the segfault's blast radius, measured end to end#650josephismikhail wants to merge 28 commits into
josephismikhail wants to merge 28 commits into
Conversation
#598 fixed a real crash, and the fix stands. What it claimed about WHO the crash reached does not, and that claim is in three load-bearing comments. They say the pool "terminates every worker at the end of every run, so roughly one `ix map` in twelve exited 139 after a completely successful ingest". That number is the 20-teardowns-per-process rate from the reproduction script, applied to a CLI run that does ONE teardown. I never ran the end-to-end case. Run now, against the pre-fix build (084f472) and a live backend, pool of 21 workers over a 300-file repo: one pool per process, then exit 1-3 of 15 segfaulted twenty pools in one process 7 of 10 a real `ix ingest` of 300 files 0 of 60 0 of 60. At the single-teardown rate that has a probability of about 0.2%, so the CLI is genuinely not exposed, not merely lucky. Confirmed the pool really does run there rather than being skipped -- an instrumented dist prints `init concurrency=21` then `destroy workers=21` on every run -- so the teardown path executes and still does not crash. The exposure scales with teardowns per PROCESS, which makes the affected consumers the ones that build many pools in one: the MCP server's in-process runner (the default unless IX_MCP_SUBPROCESS=1), and the vitest suite, where this first surfaced as an intermittent "Worker exited unexpectedly". That is what the comments say now. Nothing else changes -- `unref()` is still the right teardown and the tests are untouched in substance. This only stops the next reader inheriting a user-impact figure I did not measure. 13 pool tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
Review of #650. The correction was itself wrong in three ways, and the reviewer's arithmetic is right. I wrote that the old "one `ix map` in twelve" was "the 20-teardown rate applied to a 1-teardown case". It was not. 5 of 6 runs crashing over 20 teardowns implies 1-(1-p)^20 = 5/6, so p = 8.6% per teardown, which IS 1 in 12. The division was correct; what nobody had done was run the CLI. Checked: 8.6%, 1 in 11.7. Worse, I asserted a cause my own table refutes. `ix ingest` builds exactly one pool and tears it down once in the outermost `finally` -- so "one pool per process" and "a real ix ingest" are the SAME configuration, and "exposure scales with teardowns per process" predicts several crashes in 60 ingests where I measured none. Pooling the harness single-teardown runs gives 5 of 40, 12.5%, at which zero in 60 has probability 0.03%: the two really do differ, but not for the reason I gave, because both are one teardown. The comments now report both numbers, say the ingest does not follow from the harness, and state plainly that WHY is unestablished rather than inventing a model. They also say not to rely on the escape. And I named the wrong test file. Of the 13 pools in `parse-pool.test.ts` only one loads the addon, and an un-addon'd worker is not the crashing case -- so that file is not exposed, and the comment sat in it claiming otherwise. `ingest-files.test.ts` is where this surfaced: 14 real ingests per vitest process. The MCP in-process runner shares that shape and is now marked as inferred rather than measured, which is what it is. Also corrected "`ix map` ... 60 runs" to `ix ingest`, which is the command that was actually run, and labelled the two tables as separate experiments with their pool size. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…do not pin one Round-2 review of #650, and the reviewer is right on every count. My "correction" was as unsupported as the thing it corrected, in the opposite direction. The claim it rested on -- "the harness numbers agree at ~12.5% per teardown" -- is false, and the table three lines above says so. The 20-teardown runs imply 8.6% (5 of 6), 5.8% (7 of 10) and 4.3% (7 of 12); the single-teardown runs imply 12.5% (5 of 40). Those do not agree. And at 4.3%, seeing zero crashes in 60 ingests has a probability of 7.2% -- which is unremarkable, so the "a real ingest genuinely escapes, and WHY is unestablished" framing was built on picking the highest of four inconsistent estimates. The reviewer also caught that the escape claim contradicted the very next paragraph: `ingest-files.test.ts` drives 14 runs of the SAME `ingestFiles` path that I said escapes at 0 of 60. Under a per-ingest model both cannot be true, and I had explicitly rejected the only model that reconciles them. So the comments now claim NO per-command impact. They report the range, say it is too wide to turn into a figure, note that 0 of 60 does not establish exemption either, and state what is actually solid: the crash is real, reproducible, and reached the test suite. Two earlier versions made a blast-radius claim in each direction and both were wrong; the third makes none. Also fixed: - the instrumentation I cited proved nothing about the addon. `[POOL] init` / `destroy` print whether or not a worker ever parsed, and a worker whose bindings failed to load resolves null and is absorbed as `filesSkipped`. Checked properly: those runs report entitiesParsed 1200, parseError 0, unparsed 0, and the graph holds 300 classes and 300 functions. The addon was loaded. Now cited that way. - "this file is NOT the exposed one" was wrong in `parse-pool.test.ts`: its real-worker test does exactly one addon-loaded teardown, so the file is exposed once, not zero times. It says that now. - 0.03% was a point estimate dressed as certainty; the range is what is quoted instead. 13 pool tests pass. No behaviour change, in this commit or the PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
Round-3 review of #650. The previous revision over-swung: in dropping a claim the data did not support, it also dropped the one thing the data DOES say. Comparing like with like -- the harness and a real ingest at the SAME single teardown -- 5 of 40 against 0 of 60 is a real difference. Fisher exact p = 0.009, computed rather than asserted. The last revision waved that away by reaching for 4.3%, a rate derived from a TWENTY-teardown run under the independence model that the four disagreeing estimates undermine. Importing a rate across configurations to dismiss an anomaly is the same move as importing one to manufacture the original "1 in 12"; third direction, same error. The comments now state the difference, and state that WHY is unestablished and not teardown count, since both are one. The counts are back in `parse-pool.ts` rather than a bare range. That file implements the teardown and is the first one opened; "4.3-12.5%" cannot be checked at the site, and the next person asking "is terminate() safe if we tear down only once?" should not have to reconstruct 5-of-40 from PR archaeology. `parseError 0` was the wrong evidence for the addon having loaded -- it is blind to exactly the failure being excluded, because a worker whose bindings fail throws in `parseFile`, posts `{ok:false}`, and lands in `filesSkippedUnparsed`, never in `parseErrors`. `unparsed: 0` is the field that catches it, and that is what is cited now, with the 300 classes and 300 functions in the graph. Also: restored why the MCP runner is singled out (it has the many-pools-in-one-process shape), since removing the causal model left that sentence with no reason in it; and removed a sentence in `parse-pool.test.ts` that restated its own table's intro six lines above. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…R retracts Round-4 review of #650. Three findings, and the middle one means I wrote a wrong mechanism into the comment last round by accepting a reviewer's without checking it. I had said a worker whose tree-sitter bindings fail to load "resolves null and lands in `filesSkippedUnparsed`, never in `parseErrors`", and used that to argue `parseError: 0` was blind to the case. It is not. The `tree-sitter` import in `core-ingestion/src/index.ts` is STATIC, and `parse-worker.ts` statically imports that module -- so a bindings failure throws during module evaluation, never reaches `parseFile`, and kills the worker. `ParsePool` counts that as a crashed parse and `ingestFiles` folds it into the reported number (`parseErrors + crashedParses()`, `ingest.ts` 3490 and 3570). A bindings failure would therefore show up in `parseError`, which read 0. So both counters rule it out, for different reasons, and the comment now says which does what: `parseError: 0` catches a dead worker, `unparsed: 0` catches the quieter null-returning case where an optional grammar is simply absent. That second one is the mechanism I had misattributed to the first. Verified rather than taken on the reviewer's word this time: `import Parser from 'tree-sitter'` at index.ts:5, `import { parseFile } from './index.js'` at parse-worker.ts:8, and the two `+ crashedParses()` call sites. Also: `parse-pool.test.ts` still quoted "~8% per-teardown" 110 lines below the new text saying no rate can be quoted, and 8.6% is precisely one of the four disagreeing estimates the PR forbids using. Removed. And the claim about which pools load the addon said "the pools below" when four of the thirteen are above it -- the claim was right, but unverifiable as written. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…e counters disjoint Round-5 review of #650. Six findings; three change what the comments say. THE ESTIMATES DO NOT DISAGREE. That sentence carried the whole PR -- "no rate can be quoted, because the estimates disagree (4.3-12.5%)" -- and it was the same number-picking it was written to retract. The three twenty-teardown figures are not three estimates, they are one experiment split three ways. Pooled: 19 of 28 runs crashed over 20 teardowns each, which is ~5.5% per teardown, and the single-teardown arm (5 of 40) is consistent with it -- P(>=5 of 40 | p=0.055) = 0.07. So there is one harness rate, roughly 3-8%, and the comments now say it. Pooling also strengthens the other conclusion rather than weakening it: against 5.5%, 0 of 60 has probability 0.033, so the real ingest is low on the pooled comparison as well as on the like-for-like one (Fisher p=0.009). THE COUNTERS ARE NOT DISJOINT. I wrote that `parseError` and `unparsed` "catch different things" and that an earlier revision "had the counter backwards". Wrong both ways: a null parse raises `filesSkippedUnparsed` (`ingest.ts:2963` and `:3131`) AND, when the worker died, the reported `parseError` via `+ crashedParses()`. `ingest.ts:3483` says so explicitly -- my comment contradicted a comment in the file it was citing. A bindings failure moves both, so either reading zero rules it out; what `unparsed` catches alone is a healthy worker returning null for a missing optional grammar. THE ADDON LOADS AT SPAWN. The static-import fact this PR introduced falsifies two comments the PR did not touch: `parse-pool.test.ts` says an "untouched worker has not loaded the addon", but `index.ts` imports tree-sitter and twelve grammars statically, so every spawned worker dlopens them. That is not academic -- left as-is it invites a `terminate()` fast path for never-dispatched workers, which is the segfault. Corrected, while keeping what was actually measured: having PARSED is what armed the crash, since spawn-then-destroy with no parse did not reproduce it. Also restored the user-visible signature, which the retraction had deleted along with the rate although it was never in dispute: exit 139 after a successful ingest, patches committed and summary printed. An operator hitting that had nothing left in the repo to match it against. Plus a broken "top of this file" pointer and a 113-character comment line. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…nline fixtures load the addon Round-6 review of #650. Seven findings; three are wrong claims I added last round while fixing the round before. I applied a global fact to a local context where it is false. The "asks a worker to end itself" test runs the inline `polite` fixture -- a .mjs that imports only `node:worker_threads` and `node:fs` -- so it never loads the addon at all, yet I wrote that it "has it, since `index.ts` imports tree-sitter statically". That also contradicted a comment 25 lines above in the same commit. The static-import fact is true of the REAL worker; it says nothing about inline fixtures. Both sites now say which worker they are talking about, and the parse in the real-worker test is justified by what was measured (having parsed is what armed the crash) rather than by the addon being loaded, which it already is at spawn. The quoted interval was the wrong one. "Roughly 3-8%" is the confidence interval of the twenty-teardown arm ALONE, quoted immediately after a sentence saying to combine both arms. Fitting both -- 19 of 28 runs at 20 teardowns, plus 5 of 40 at one -- gives 6.3% per teardown, 95% CI 4.1-9.3%, and the single-teardown arm's own point estimate of 12.5% sits outside the band I had quoted. Anyone sizing exposure from it would have taken 8% as the ceiling of a range that actually reaches 9.3%. The `ingest.ts:3482` citation was backwards. That comment says the RAW `parseErrors` and `filesSkippedUnparsed` ARE disjoint -- which is precisely why the summary adds `+ crashedParses()`. Non-disjointness holds only for the REPORTED field. As written it read as contradicting the file it cited, inviting someone to "fix" whichever comment they found second. Also: the negative result had no denominator ("0 of 6" -- of what?). It was 6 runs of TWENTY teardowns each, where the fitted rate predicts a crash in all but ~0.1% of runs, so it is strong rather than meaningless; in a comment whose thesis is that sample sizes matter, that was the one number missing its own. Plus a "the rate is quoted once" claim falsified by the same commit, and an exposure claim that conflated pool teardowns with worker teardowns (the real-worker pool runs at concurrency 2, so the file disposes two addon-loaded threads, not one, and not the 21 the rate is quoted for). 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…er, not the CLI Round-7 review of #650. Eight findings, and the substantive one dissolves the puzzle every previous revision has been talking around. I had been treating `ix ingest` 0-of-60 as an unexplained anomaly while reporting, eight lines above, that `ingest-files.test.ts` -- which drives REAL ingests, ~10 per vitest process -- crashed intermittently. Those are the same population, and I never put them together. Doing so: 2 of 75 vitest processes at ~10 real ingests each -> 0.27% per teardown at 0.27%, P(zero crashes in 60 CLI ingests) -> 0.85 So the CLI result is exactly what the vitest observation predicts. It was never an anomaly, there is no "WHY is not established" to answer, and the outlier is the MINIMAL HARNESS at 6.3%, which overstates real exposure by roughly twenty times. Real ingests crash rarely rather than never -- the one model consistent with all four datasets, and the one none of my revisions offered. The comments now present the two populations separately and say not to size anything from the harness rate. Three arithmetic leftovers, all the same class: prose updated to the pooled 6.3% fit, derived numbers left at the old 5.5%. "P(0 of 60) = 0.033" is the 5.5% value (6.3% gives 0.020), and "all but ~0.1% of runs" is likewise 5.5% (6.3% gives 0.04%). Corrected. Also: the "spawn-then-destroy did not crash" observation appeared in the test file without the hedge its twin carries in `parse-worker.ts`, and the test file is exactly where someone would add a `terminate()` fast path for never-dispatched workers -- the wording that motivated this whole PR lived there. It now says explicitly that this is a statement about what ARMS the crash, not a licence to terminate undispatched workers, which hold the addon too. Plus: a hardcoded `ingest.ts:3482` line reference in a 3,800-line file swapped for the symbol it means, a note that the rate is per teardown of a 21-worker pool and does not carry to other sizes, and a "that row" with three rows above it. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…n what `unparsed` covers Round-8 review of #650. Three findings, all in the numbers this PR exists to get right. The vitest divisor was unstated and the PR contradicted itself about it -- `parse-pool.test.ts` says 14 ingests per process while `parse-worker.ts` said ~10. Both are true of different moments: the file drove 9-10 when the 2-of-75 crashes were observed and drives 14 now. A rate is meaningless without saying which, so the comments now give the raw observation, the divisor used, and an instruction to redo the division rather than reuse the result. The derived figures move slightly (0.3% per teardown, P(0 of 60) = 0.84) and the harness/real ratio is "more than twenty times" rather than a false precision. "A crash in all but 0.04% of runs" conflated a joint probability with a per-run one. 0.04% is the chance of all six runs coming back clean (0.937^120); a SINGLE clean run of twenty teardowns is a 27% event, which the adjacent "19 of 28 runs crashed" line already implies. As written the sentence was refuted by its own table, which is the fastest way to get a reader to discount the block -- including the "not a licence to terminate never-dispatched workers" conclusion it exists to support. And `unparsed` does not only catch a missing grammar. `ingest.ts` says so directly, a few lines from the counter: a parse that THREW is caught inside `parseFile`, returns null, and is indistinguishable from an absent grammar to the worker and to the counter alike -- the transient OOM sub-case included. Someone diagnosing a nonzero `unparsed` on a repo where every grammar is present would have been sent looking for a phantom. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…pointed the wrong way Round-9 review of #650. Four findings. The instruction I added last round was inverted. "Redo the division, do not reuse the 0.3%" gets it exactly backwards: the PER-TEARDOWN rate is the portable quantity, and the per-process count is what was tied to the ~9.5 teardowns in force when it was measured. A reader following it literally re-divides 2-of-75 by 14 and gets 0.19%, an underestimate of a datum never observed under 14 teardowns. What needs redoing with 14 is the multiplication back up to today's per-process exposure -- about 3.9% -- and the comments now say that, with the number. The "(1.9% when the machine was loaded)" parenthetical was a second, unshown measurement presented as if it came from the 2-of-75 line. It cannot: 1.9% per teardown over 9.5 teardowns implies ~12 of 75 processes crashing, not 2. It is the loaded dataset, 9 of 50, which works out to 2.1%. Both are shown now, because the difference matters to the conclusion they sit beside: P(zero in 60) is 0.84 at the idle rate but 0.29 at the loaded one. The CLI result is unremarkable under either -- which is the honest strength of that claim, rather than the strongest of the two. `parse-pool.ts` had the pool-size caveat positioned where it read as the explanation for the 20x gap, and it cannot be one: both populations tore down 21-worker pools, the harness by construction and real ingests via `os.cpus().length - 1` on the same machine. It also lacked the "not established" that its twin in `parse-worker.ts` carries, so that file alone invited a maintainer to take pool size as the cause and stop looking. Both now say what the gap is not, and that what it IS remains unknown. And `parse-pool.test.ts` paired the 2-of-75 observation with today's 14 ingests rather than the ~9.5 in force when it was taken -- the same cross-comment mismatch this PR exists to remove, reintroduced by me two rounds ago. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…t the confound Round-10 review of #650. Six findings; the first two are the ones that would have misled someone doing real work. Every headline number was the IDLE rate presented as the rate. The same comment measured a loaded machine at 2.1% per teardown against 0.28% idle -- 7.3x apart -- and then quoted "about 0.3%" and "3.9% of processes" as though load did not exist. CI is the loaded case. An engineer triaging `ingest-files.test.ts` flakiness follows that guidance, gets 3.9%, and concludes the segfault is too rare to be what they are seeing; the real figure for that file under load is 25% of processes. The comments now lead with the loaded rate and give both. That also shrinks the mystery this PR has been circling. The harness is 22x the idle rate but only 3.1x the loaded one, so machine load accounts for most of the gap I had been calling unexplained. What is actually unexplained is 3.1x. And one variable was never controlled: parses per worker. The vitest fixture is 30 files over 21 workers (~1.4 each); the `ix ingest` measurement was 300 over the same pool (~14 each). If exposure grows with parses per worker -- which this comment's own "having parsed arms it" observation makes plausible -- then 0 of 60 is more surprising than the quoted probabilities suggest. Said, rather than left for the next reviewer. The grammar count was wrong by more than half. `index.ts` loads twelve grammars by static import, ELEVEN more optional ones eagerly at module scope, and four through top-level `await` -- about 27 native addons, not twelve, all held from spawn. Load-bearing, because the same comment reasons that exposure depends on how many addon-loaded isolates are disposed. Those four top-level awaits also confound the spawn-then-destroy result I had been citing as clean. Module evaluation is still suspended for a moment after spawn, so the worker's `message` handler is not yet registered: a worker destroyed in that window cannot answer `__shutdown` and may not have finished loading. "No parse" and "not fully loaded" are not separated, so "parsing arms it" is now marked unproven rather than measured. The safety conclusion is unchanged. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…files disagreeing Round-11 review of #650. Five findings, all in the prose, and three of them are the same failure: a fact stated correctly in one file and wrongly in another. The parses-per-worker caveat was attached to the wrong comparison in `parse-pool.ts`. It sat in the paragraph about the unexplained harness-versus-real gap, but ~1.4 and ~14 are the two REAL-ingest datasets, not harness against real -- and the harness's own figure is ~1.4, the same as the vitest fixture, so it cannot explain that gap at all. What it does threaten is the pair the 0-of-60 argument rests on. Both files now say which comparison it bears on, and name the harness's value so the reader can see why it is not the explanation. `parse-pool.test.ts` told a reader to plan against "~25% of processes" for "this suite", using `ingest-files.test.ts`'s roll-up while the paragraph directly below established that THIS file has one addon-loaded pool doing a single teardown of two threads. Someone triaging a red `parse-pool.test.ts` would have taken 25% as its flake budget and written off a real failure. The two files also disagreed by one on the addon count -- "about 27 native addons" against "tree-sitter and ~27 grammars" -- for the same enumeration. It is 27 grammars plus the core, 28 addons, and both say that now. And the test file still asserted "having parsed arms the crash" as settled, which the worker file retracts in this same diff as confounded by top-level-await module evaluation. Left alone it would have been the surviving justification a later reader cited for the exact `terminate()` fast path this PR exists to prevent. It carries the retraction now. Also: "understates a CI leg sevenfold" was the per-TEARDOWN ratio leaking into a per-PROCESS sentence. 3.9% to 25% is 6.5x. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…the loaded rate Round-12 review of #650. Six findings; the first turns a caveat into an argument. Parses per worker was undercounted by 2x, and it was the one variable I had claimed WAS controlled. `ingestFiles` parses a `.ts` file twice -- once in the index prescan and once in the streaming loop, both on the same pool; `parse-pool.ts` says so in its own `countLoss` note, which I had read and not connected. So a 30-file ingest dispatches ~60 tasks, and over 21 workers the vitest fixture is ~2.9 per worker and the 300-file `ix ingest` ~28.6, against ~1.4 for the harness, which does no prescan. Counted properly it stops being a threat to the conclusion and becomes support for it: the harness parses the LEAST per worker and crashes the most, by 22x. If exposure grew with parses per worker the ordering would be reversed. The same holds between the two real-ingest datasets -- the CLI parses ten times more per worker than the vitest fixture and crashed zero times in 60. The addon count was stated as a constant when it is an upper bound. 14 of the 27 grammars are optional dependencies loaded through helpers that return null when absent, and the Windows machine these numbers came from has no `tree-sitter-sas` prebuild; an `--omit=optional` install holds 13. "Up to 28" now, with the floor spelled out, because the floor is what the safety conclusion needs: a spawned worker always holds the core and the required grammars, never zero. And the loaded rate was quoted bare -- 2.1%, 25% -- in a comment that carries an explicit CI for the harness rate two paragraphs up. It rests on 9 of 50 processes, so the interval is 12%-43% rolled up. Wide, and still an order above the idle case: Fisher exact on 2/75 against 9/50 is p = 0.006. The finding survives the interval; the false precision did not. Also reflowed two 105-character lines my own edits had created. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…on both arms Round-13 review of #650. Six findings; two are reasoning errors, not wording. "Load accounts for most of the harness/real gap" was an uncontrolled comparison, and I made it while the same comment scolds the reader about uncontrolled variables. The harness's load state was never varied -- those runs are the quiet arm -- so pitting the harness against the LOADED real rate mixes conditions. The only load-matched comparison available is idle against idle, which is 22x. And since load plainly raises the rate, a loaded harness would sit above 6.3%, so 3.1x is a lower bound on what is unexplained rather than the residue after removing load. Both files say that now. The dose-response argument was extended past its power. Between the harness (~1.4 parses per worker) and the idle vitest arm (~2.9) it holds: fewer parses, more crashes, wrong ordering for parses-per-worker to be the cause. Between the two real-ingest datasets (~2.9 against ~28.6) it does not: 0 of 60 has almost no power there, because even if the rate scaled fully with parses per worker, P(zero in 60) would still be about 0.18. That pair stays uncontrolled and is simply not evidence either way. The confound I described last round was anachronistic. That experiment ran on the PRE-FIX build, where `destroy()` was an unconditional `terminate()` and `__shutdown` did not exist -- so "could not answer `__shutdown`" and message-handler timing play no part. What does confound it is the top-level-`await` grammar loads: a worker spawned and destroyed in the same breath may have been mid-evaluation, holding fewer addons or none. Same conclusion, correct mechanism. Also: 27 - 14 leaves 13 required grammars, not "the required dozen" -- `tree-sitter-powershell` is required but loads through the optional-tolerant helper, which is what makes it easy to miscount, and `parse-worker.ts` already said 13 while the test file said twelve. Both fixtures behind the parses-per-worker figures were entirely `.ts`, which is what makes the 2x apply to every file in them; said explicitly, since a mixed repo would fall toward the single-parse count. And the consistency check that licenses pooling the two harness arms now lives in the comment rather than only in the PR description, which does not survive as a durable artifact. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
… do it Round-14 review of #650. Six findings; the first is the one that mattered. The whole point of this PR is that "an untouched worker has not loaded the addon" invited a `terminate()` fast path for never-dispatched workers. I corrected that in `parse-worker.ts` and in the tests -- and left `shutdown()`'s own doc in `parse-pool.ts` still saying only that "an idle worker that has parsed a single file is enough". That is the file where such a fast path would actually be written. Someone editing `shutdown` reads it, sees the parsing framing, and adds the guard the PR exists to prevent, never having opened the two files carrying the correction. The warning is there now, stated as a prohibition and pointing at where the retraction is argued. The "13 required grammars, never absent" claim is wrong, and it appeared twice. `tree-sitter-powershell` IS a required dependency, but it loads through the same optional-tolerant helper as the optionals, so it is null wherever it has no prebuild -- and it is absent from this very checkout, which is how the reviewer caught it. The guaranteed floor is the core plus the twelve STATIC grammars. That is the number the safety conclusion needs, and it is still never zero. The Fisher figure did not survive recomputation: 2/75 against 9/50 is 0.00683 two-sided and 0.00419 one-sided, neither of which rounds to the 0.006 I quoted. It says 0.007 two-sided now, with the tail named -- in a comment whose entire premise is that its numbers check out, that one did not. Also: the test file quoted the rates with no "pre-fix" qualifier and then described this file's exposure in the present tense, so a reader could go hunting a CI flake that cannot occur -- on the shipped build nothing terminates a worker and every one of those rates is zero. And two comment lines ran to 109 and 111 columns against the repo's 100-column prettier width, which prettier will not reflow and `format:check` will not catch. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…tency check Round-15 review of #650. Seven findings; three are places where the same comment says two different things, and one of those could cause the exact harm the block exists to prevent. "holding fewer addons -- or none" sat twelve lines below "what a spawned worker is ALWAYS holding is the core plus the twelve STATIC grammars, never zero". Both cannot be true, and the mechanism cannot produce "none" anyway: ESM evaluates the static graph and all eleven synchronous `tryLoadGrammar` calls before control reaches the first top-level `await`, so a worker suspended there still holds about two dozen addons. A reader who took "or none" at face value could conclude a just-spawned worker is addon-free and write the `terminate()` fast path this whole block forbids. The directive in `parse-pool.ts` -- the one I moved there last round precisely because that is where such a fast path gets written -- justified itself with "`index.ts` resolves EVERY grammar at module scope". That is false for 15 of the 27, which go through null-returning helpers, and `parse-worker.ts` spends a paragraph saying so. Someone checking the premise would find several null grammars and dismiss the directive. It now states the floor that is actually guaranteed, which is all the crash needs. The test file still said never-dispatched workers "hold the core and the required grammars", thirteen lines after its own text explains that a required grammar can be absent. The consistency check was unsourced: `P(>=5 of 40 | p=0.055) = 0.07` uses 0.055, a number appearing nowhere in the file, while the rate quoted two lines above is 6.3% -- at which the same quantity is 0.105. 0.055 is the twenty-teardown arm's own rate, and the check asks whether the other arm contradicts IT. Said explicitly now, since the paragraph argues this check belongs in the code rather than only in the PR. Also: the `ix ingest` row sat under a header scoping everything to "~9.5 ingests per process" when that row is one teardown per process, and the P(zero in 60) figures below it are only right at one. The test file presented a projection as a measurement ("that was ~25%") when what was measured is 9 of 50 and 2 of 75 at ~9.5 each. And the powershell absence was attributed to a missing prebuild when the lockfile pins it with no os/cpu restriction -- it is a partial install here; the structural point (a tolerant helper means "required" does not imply "present") is what carries the argument. Separately, and outside this diff: the reviewer notes `index.ts` imports `tree-sitter-powershell` by the subpath `bindings/node/index.js`, which the file's own header warns throws ERR_PACKAGE_PATH_NOT_EXPORTED for any package declaring `"exports"`. If 0.26.4 does, PowerShell parsing has been silently null since #595. Not investigated here; worth its own look. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
The round-15 review raised that `index.ts` imports tree-sitter-powershell by a subpath, which the file's own header warns throws ERR_PACKAGE_PATH_NOT_EXPORTED for any package declaring `"exports"` -- so PowerShell parsing might have been silently null since #595, hidden by the tolerant helper that returns null on any failure. It is not. The `core-ingestion tests` step runs that package's 358-test suite, including its PowerShell query tests, and passes on all five matrix legs (both ubuntu, both macos, and windows-2022) on main. The subpath resolves. Worth having checked rather than left open: a helper that swallows every failure is exactly the shape that would hide this, and the local absence of the package -- a partial install here -- looks identical to a load failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…tory Round-16 review of #650, and the last four rounds have made the shape of this obvious. Every round the reviewer found real defects, and by this one they were almost entirely about keeping an elaborate statistical write-up consistent across three files: a rate quoted with the wrong divisor here, a conclusion that contradicts a sentence twenty lines up there, a ratio that belongs to a different pair of populations. Sixteen rounds, and the report was generating the defects faster than the reviews retired them. So the report comes out of the source. What stays is what a maintainer needs and what must not drift: - do not terminate these workers; it segfaults the process - never-dispatched workers are NOT exempt, because the addon is held from spawn and not from the first parse -- the claim that started all this - the one comparison that settles the verb (terminate 5 of 6 vs asking 0 of 6), kept once, beside the code it justifies - a pointer to PR #650 and this history for everything else That removes 203 lines and adds 25. The rates, populations, intervals, Fisher tests and load effects are not lost -- they are in the PR and in these commits, which are singular and permanent and do not have to be kept mutually consistent forever. This round's findings, all real, are retired by that rather than patched: the CI-leg advice quoted a 25% figure derived on a 21-worker pool when CI runners give pools of 2-3; the `ix ingest` row sat under a header scoping it to 9.5 teardowns when it is one; and "the harness parses the least per worker" was false for five of the fourteen teardowns behind the data -- `ingest-files.test.ts` calls `fixture(4)` twice and `fixture(12)` once, not `fixture(30)` throughout, which puts those runs at 0.38 and 1.14 parses per worker, below the harness's 1.4. I had checked the fixture size once and assumed it everywhere. Two errors also survive in the PR body, which becomes the squash message, and are corrected there: "13 required grammars" as the floor (it is the core plus the twelve static ones) and Fisher "p = 0.006" (0.007 two-sided, 0.004 one-sided). 13 pool tests pass; full suite 1791 passed with the 3 read-containment symlink failures that need Windows Developer Mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…etraction correctly Round-17 review of #650. Four of the five findings are seams my own simplification left: cutting the statistics out of the source left pointers aimed at text that no longer exists. `parse-pool.ts` and the test file both said "`parse-worker.ts` retracts the inference". It does not -- the retraction was in the paragraph I deleted last round, and `parse-worker.ts` now contains no confound discussion at all. Someone weighing a `terminate()` fast path would have followed the pointer to the strongest argument against it and found nothing. Both sites now make the argument themselves, which is what "self-contained" has to mean once the shared write-up is gone. The retraction was also misattributed. "Comments HERE once said the opposite" is in `parse-worker.ts`, and no revision of that file ever said it: `git log --all -S"untouched worker"` on that path returns nothing, and the exempting comments both lived in `parse-pool.test.ts`. A maintainer auditing the correction of record would have found it unfounded in exactly the sentence that is the correction. And the confound mechanism I had been citing does not hold. It argued a spawned-then-destroyed worker "may still have been mid-evaluation, holding fewer addons" -- but the first top-level `await` in `index.ts` comes after the core, all twelve static grammars and all eleven synchronous optionals, so such a worker holds 24 of 28 addons, well past the floor the same comment calls "all the crash needs". The observation stands (parsed workers crashed, spawn-then-destroy did not); what it cannot do is establish that an undispatched worker is safe, and that is now what it says. Also: `core-ingestion/index.ts` is not a path -- the file is `core-ingestion/src/index.ts`, and the pointer sits in a package that does not depend on core-ingestion, so a reader has to navigate by it. And the deleted table left "Twenty teardowns per process, six runs each:" promising results that no longer follow. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…etracted numbers Round-18 review of #650. Six findings; two undercut the premise of last round's simplification. I moved the statistics out of the source on the grounds that "the evidence lives in the history". It does not, usefully. `main` squash-merges, so `git log -p` on these paths shows this work as one commit and #598 as the one before it -- and #598 is where the retracted figures live ("~8% per teardown", "one `ix map` in twelve"). Worse, the squashed body of this PR concatenates seventeen rounds of superseded values beside their corrections (0.006 against 0.007, 0.27% against 0.28%, 5.5% against 6.3%) with nothing marking which is current. Following my own pointer lands a reader on the retracted rate first. Both comments now name the PR DESCRIPTION as the single current record and say plainly that the file history is not it. And I reintroduced, in softer words, the contradiction commit 2081465 already retired. The test file said the parsed-versus-spawned observation "was never isolated from how far module evaluation had got" -- which can only matter if a spawned worker might hold less than the floor, three lines after the same block says the floor is the core plus twelve static grammars and never zero. My own last commit message says that mechanism does not hold. It now uses the wording `parse-pool.ts` already had: the mechanism was never isolated and the addon is held either way. `parse-worker.ts` was also the one file asserting the inference flatly -- "terminating it can crash exactly as terminating a busy one can" -- while the other two hedge, and the only data point runs the other way. It states the precondition now: the addon is held, so an undispatched worker is not KNOWN to be safe. That is what licenses the rule; the rate was never measured. Also: "only ONE pool below loads the addon ... the other twelve fixtures" mixed scopes -- nine pools sit below that line, twelve is the file-wide count -- and two pointers named `ParsePool.shutdown` with no path, from a package that does not depend on the one holding it. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…hreads-pool exposure Round-19 review of #650. Six findings; one exposed a hole in last round's own fix, and one pointed at a place none of these comments had looked. I had made all three files name "the DESCRIPTION of PR #650" as the single current record. That does not survive: this repo squash-merges COMMIT MESSAGES, not the PR body, so after merge the measurements exist nowhere in the repository -- and a PR description is editable and unversioned, so the cited authority can change or vanish while the code still points at it. Anyone on a clone, a fork or the packaged `core-ingestion` had a dangling pointer. The numbers now live in `docs/parse-pool-teardown.md`, which is versioned and travels with the checkout, and the three comments point there. The reviewer also asked whether this PR's own finding implicates `core-ingestion`'s suite: it runs `vitest run --pool threads`, 36 of its 38 test files import `./index.js`, and the addon is held from spawn -- so every vitest worker thread holds it and tinypool tears those threads down with `terminate()`. That is exactly the exposed shape. Measured rather than argued: 0 segfault signatures in 10 local runs, and the `core-ingestion tests` step passes on all five CI legs. A real theoretical exposure that does not manifest, now recorded in the doc so the next reader does not have to rediscover the question. Three wording defects, all of the same family this PR keeps producing: - "which is all the crash needs" claimed holding the addon is SUFFICIENT, two lines above the observation that spawn-then-destroy did not crash. It is the precondition -- necessary, not sufficient -- and says so now. - `parse-worker.ts` said the undispatched rate "was never measured" while the other two files reported an experiment where spawn-then-destroy did not crash. All three now say the same thing: observed once not to crash, mechanism never isolated, rate never measured. - the test file carried a hand-maintained census ("thirteen pools ... the other twelve fixtures") that a single added test would falsify with the suite green -- the same class of comment this PR deleted 203 lines of. It states the property without the count, and the exact tally lives in the doc. 13 pool tests pass; full suite 1791. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…ting figures Round-20 review of #650. Six findings and a nit. The doc led with "terminate() 5 of 6" and then reported "19 of 28" for the same configuration two sections later, never saying how they relate. They are not two experiments: 7 of 12, 7 of 10 and those 5 of 6 pool to exactly 19 of 28. Read alone the six imply 8.6% per teardown -- which is essentially the "one in twelve" this document exists to retract, so leading with that arm was carrying the retracted number as the headline evidence. The doc now says the six are one arm of the fit, and why quoting a single arm is the hazard. `parse-pool.ts` had reintroduced the over-read, in the file the warning is meant to harden: "Necessary, not sufficient: spawn-then-destroy was observed not to crash, so holding the addon is not on its own enough." A null result with an unstated N cannot establish insufficiency if the same result cannot establish safety -- the inference is symmetric, and I had already said so about the other direction three rounds ago. It now says sufficiency is unknown and reading it either way is the over-claim. The vitest exposure I recorded last round had the wrong mechanism. Vitest's worker entry does not import `core-ingestion`, so a thread picks the addon up when it evaluates a test file, not at spawn -- "36 of 38 files import ./index.js" does not mean every spawned thread ran one. The measurement is unaffected; the mechanism now matches the runtime rather than being borrowed from `parse-worker.ts`, where it IS spawn-time. And the PR's own consolidation principle was not honoured: "5 of 6" appeared four times and the terminate-vs-native timing twice, disagreeing with itself (3981ms against 4457ms) inside one file. One copy of each now, with the others referring to it; the two timings are labelled as two runs of the same experiment, which is what they are. Also: `docs/README.md` indexes every file in `docs/` and did not list the new one, which undercuts the point of putting it there. Added. 13 pool tests pass; full suite 1791. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…"from spawn" Round-21 review of #650. Five findings; two change what the document concludes rather than how it words it. I filed the vitest measurement as "a theoretical exposure that does not manifest" -- reassurance. It is the opposite. That suite terminates addon-loaded threads, and 0 segfaults in 10 runs is only consistent with the 6.3% harness rate at a probability of 1.5e-3 (at ~10 addon-loaded teardowns per run) or ~1e-11 (at one per test file). It is the strongest evidence in the record that the harness rate does not transfer, and it strains the premise the "do not add a fast path" rule leans on: either this is not the same shape, or holding the addon is nowhere near sufficient. The rule survives, and the document now says on what: not on a rate anyone can quote for undispatched workers, but on the precondition being real and nobody having isolated what else is needed. Recording a result that cuts against the model as though it supported it is the same failure as quoting the arm that flatters an argument -- which this document warns about two sections earlier. "The addon is held from SPAWN" was also categorical and is not quite true. `index.ts` has top-level `await`, so there is a real window after `new Worker()` in which the thread holds nothing -- and it is reachable, since `init()` spawns eagerly and a throw during discovery can reach `destroy()` with no dispatch at all. It is held from module EVALUATION, which a worker reaches on its own; that is what makes undispatched workers unsafe, and it is what the three files say now. The doc's own vitest paragraph had already drawn this distinction correctly while the other sites did not. Also: the `unref()` arm (one teardown per run, ten runs) was being compared against the twenty-teardown `terminate()` row, which reads as a ~50x effect where the licensed comparison is much weaker; the spawn-then-destroy arm was load-bearing in three comments with its n recorded nowhere, and is now stated (6 runs of twenty teardowns, 0 crashes, a 0.04% outcome under the fitted rate); and "some required ones also load through null-returning helpers" is exactly one, `tree-sitter-powershell`. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
Round-22 review of #650. Six findings, and the first undoes what I did last round. I claimed the vitest result falsified the addon model: 0 segfaults in 10 runs of a suite that terminates addon-loaded threads, against a 6.3% rate, "1.5e-3 ... at one per test file ~1e-11". That mixed units. The 6.3% is per POOL teardown and a pool disposes 21 isolates, so the per-isolate hazard is 0.31%. Tinypool terminates threads one at a time: ~10 addon-loaded threads is ~3% per run, P(0 in 10 runs) = 0.73, and even at one per test file 0.33. There is nothing to explain, and the paragraph I wrote to say the model was in trouble was itself the error -- applying a per-pool rate to individual isolates, in a document whose whole discipline is saying which unit a rate is in. The section now shows the conversion and says why it is the easiest mistake here to make. The same error had me under-reading the other arm. Spawn-then-destroy is 0 of 120 teardowns, not "too small a sample": under the parsed rate that is a 0.04% outcome, so it rejects "holding the addon is on its own enough" at p = 4e-4. Parsing, or something travelling with it, does matter. And `parse-worker.ts` said the undispatched rate "was never measured" when 0 of 120 measures it -- the 95% upper bound is 2.5%, lower than parsed and a long way from zero. That bound, not a symmetry argument, is what the prohibition rests on, and all three files now say so together. Two more, both of which pointed the wrong way for a reader: - I attributed the addon-free window to top-level `await`. ESM evaluates a module's dependency graph BEFORE its body, so by the time `index.ts` reaches its first await the core and twelve static grammars are already in the isolate. The window ends before the awaits begin -- so a worker suspended at one is NOT safe to terminate, which is the opposite of what my wording invited. - "a throw during discovery can reach `destroy()` with no dispatch" is not reachable: the pool is lazy (`ensureParsePool` runs only at the three parse sites) and `destroy()` is guarded by `if (pool)`. The genuinely reachable case is stronger and simpler -- `init()` spawns `concurrency` workers, so any batch smaller than the pool leaves some never dispatched. Also: the `unref()` arm is four isolates in one teardown, where terminating would fail about 1% of runs -- it carries little on its own, and saying it would "fail well under half the time" implied evidence it does not have. 13 pool tests pass. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…kers before they loaded Round-23 review of #650. Seven findings, and the second invalidates an experiment I have been citing since round 21. The spawn-then-destroy arm -- 0 of 120 teardowns, which I used to reject "holding the addon is on its own enough" at p = 4e-4 and to bound the undispatched rate at 2.5% -- did not measure undispatched workers. I read the harness back: it calls `pool.init()` and then `await pool.destroy()` with nothing between, no ack and no 'online' wait. So it tore the threads down inside the addon-free window the same document describes, and measured threads that had not finished loading. Both numbers are withdrawn, and with them the claim that parsing matters. What is left is simpler and does not depend on an experiment: nothing has measured whether an evaluated-but-undispatched worker crashes. The rule rests on the mechanism -- such a worker holds the addon, which is the precondition -- and on nobody having shown it is safe. That the reviewer had to ask how the arm synchronised, and that the answer was "it did not", is the same lesson as the rest of this PR: a number survives review far longer than the setup that produced it. The retracted headline from two rounds ago also survived in the bolded lede: the vitest section still opened "The strongest evidence against the harness rate transferring" while its own body spent four paragraphs disowning that. Someone skimming the ledes would have taken away exactly the claim the section retracts. It reads as what it is now -- a consistency check. Also: the addon-free window closes near the START of dependency-graph evaluation, not at its end -- `tree-sitter` and the twelve grammars are `index.ts`'s own static imports -- so the previous wording left room to believe a worker suspended at a top-level `await` might be addon-free. It is not. Plus a sentence stated twice in consecutive lines, a paragraph restating the one above it, and a stray comma. 13 pool tests pass; full suite 1791. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…ure is zero everywhere Round-24 review of #650, and the first with no medium or high findings -- seven precision defects, all fixed. The doc warned that mixing per-pool and per-isolate rates is "the easiest error in this document to make, and the reason every rate here says which it is" -- while its own headline said only "6.3% per teardown" and did not disclose the per-POOL unit until eighty lines later. A reader taking it as per-worker computes 75% for a pool. The unit is at the headline now, with the per-isolate figure beside it. `parse-pool.test.ts` said "on the shipped build the exposure is zero everywhere, since nothing terminates a worker any more". Not everywhere: this PR's own doc had to go and check `core-ingestion`'s suite, which runs on vitest's threads pool and terminates threads that loaded the addon. The claim is scoped to `ParsePool` now and points at the other consumer. "~10 addon-loaded threads in a run" was a number I did not measure and did not source. Vitest sizes its pool to available parallelism -- 21 here -- and 36 of 38 files import the index, so the honest form is the bracket: P(0 in 10 runs) runs 0.73 / 0.52 / 0.33 across 10 / 21 / 36 threads. Unremarkable at every one of them, which is the point, but the single quoted 0.73 was the most flattering end of it. Also: `parse-pool.ts` had re-imported the per-isolate hazard to derive an "about 1%" -- a doc-only constant copied into source, which is exactly the duplication this PR removed 203 lines to end, and which would go stale the next time the fit moves (it has moved twice). It defers to the doc now. The same file collapsed "from spawn" and "once module evaluation completes" into one clause, when the narrow window between them is the entire reason the 0-of-120 arm was rejected. `docs/README.md` had lost its trailing newline. And six comment lines were left over 80 columns by earlier edits. 13 pool tests pass; full suite 1791. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
… one it opens Round-25 review of #650. One medium, and it was mine: round 24 rewrote the DO-NOT rule to say a worker holds the grammars "from spawn once its module evaluation completes", then added "there is a narrow addon-free window before that evaluation". `index.ts` does not finish evaluating until four top-level `await tryImportGrammar(...)` calls resolve -- so that wording places a worker suspended mid-evaluation INSIDE the safe window. It is wrong in the unsafe direction, on the one comment that calls itself "the file where a fast path would be written", and `parse-worker.ts` and the doc both said the opposite and were right. The window is now pinned at the end that matters: it CLOSES when `index.ts`'s static imports evaluate -- near the top of its dependency graph, moments after `new Worker()` -- not when evaluation finishes. A worker suspended at one of those awaits already holds all twelve grammars. `parse-pool.test.ts` carried the same "once their module evaluation completes" framing and is corrected with it. This is the second round to catch drift between three hand-written copies of one mechanism, which is the argument for the doc, not against it. The copies that remain now state where the window ends rather than paraphrasing when it runs, because that is the direction a maintainer's mistake would go. Five smaller corrections: Both source files asserted the rejected spawn-then-destroy arm "tore its workers down before they had loaded anything" -- a mechanism claim nothing measured, since the harness awaited no ack and no 'online' event. They now say what the harness shows, matching the doc's "had not finished loading". `parse-pool.test.ts` still said exposure through `ParsePool` is zero. It is not: an unresponsive worker is left alive and `unref()`'d, and its isolate is disposed at process exit. That path is measured only as four addon-loaded threads, 0 failures in 10 runs -- which at h = 0.31% is the expected result whether or not it is safe. Same over-claim shape this PR retracts elsewhere. The "Real ingests" table was the one rate in the doc not labelled per POOL, two paragraphs after the section insisting every rate says which it is -- and its 0.28% sits beside a 0.31% per-ISOLATE hazard, which is exactly the confusion the doc calls its easiest error. The CI matrix was leaned on as if equivalent to the local runs. It runs Node 22 and 24 against measurements taken on Node 26, on 3-4 vCPU runners well below the thread bracket the table works through. It is a weaker corroboration and now says so. And seven lines left over 80 columns by earlier reflows, including one consisting of the single word "which". Prettier does not touch comments, so nothing else would catch them. 13 pool tests pass; full suite 1791. No behaviour change; the TypeScript diff against main is still comments-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…onfig decides Round-26 review of #650. The doc twice attributed the vitest-side `terminate()` to tinypool. Vitest 4 dropped it: it is not in `vitest@4.1.11`'s dependencies, not in either lockfile, and not in `node_modules` at all. The conclusion was right -- `ThreadWorker.stop()` is `await this.thread.terminate()`, per worker -- but a reader who went to check the attribution would have found nothing, which is the one failure this file exists to prevent. It now cites what is actually there, and says plainly that the earlier attribution was wrong. The second correction is bigger than a citation. The doc presented "how many addon-loaded threads a run terminates" as an unknown and worked a 10 / 21 / 36 bracket over it. The config already determines it: `isolate` defaults to true, `core-ingestion` ships no vitest config, and vitest reuses a runner only when the finished task AND the next queued one are both non-isolated -- otherwise it stops the worker after each file. So it is about one worker per test file, 36 addon-loaded of 38, and the answer is the 0.33 row, not a range containing it. That also removes the "21 (this machine's parallelism)" row, which was not a possible value for the quantity: parallelism bounds concurrent threads, not terminated ones, and only terminated ones carry the hazard. The same confusion had leaked into the CI paragraph, which downweighted the matrix partly because 3-4 vCPU runners give fewer concurrent threads -- true and irrelevant, since they still terminate ~36. The Node 22/24-vs-26 half of that objection is sound and now carries it alone. Three smaller ones: Two rates appeared before the per-pool/per-isolate distinction was introduced -- 8.6% at the top and 5.5% in the pooling paragraph -- in a document whose closing rule is that every rate says which it is. They say it now, at first use. `parse-pool.test.ts` said nothing terminates a worker "which is the exposure this PR removes". #650 removes no exposure; it is comments-only, and #598 removed the `terminate()`. After the squash "this PR" resolves to nothing, which is the same unreliable-history problem the file is built around. It names #598. The same file claimed the threads hold the twelve static grammars from spawn "and the rest by the time it does" -- then two lines later gave the guaranteed floor as the core plus the twelve, because the other 15 are absent wherever a platform has no prebuild. The first clause was simply wrong; it is gone rather than walked back. And an orphaned wrap from round 25 left `event` alone on a line. 13 pool tests pass; full suite 1791; lint and typecheck clean. Still comments-only against main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
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
1 participant
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.
#598 fixed a real crash and the fix stands. What the code said about how often it hit anyone did not survive being measured. This PR replaces that with the measurements — and, after sixteen review rounds, puts them where they can stay correct.
What the code says now
Three short claims, in the files that need them:
terminate()on a thread holding the tree-sitter addon segfaults the process.index.tsresolves grammars at module scope. Two comments previously said the opposite, which is what made aterminate()fast path for undispatched workers look safe. That is the claim this PR exists to retract.terminate()5 of 6 runs SIGSEGV; asking 0 of 6.Everything else lives here and in the commit history. Keeping a statistical write-up consistent across three source files generated more defects across successive reviews than it prevented — this PR shed 203 lines of it on that basis.
The measurements
Pre-fix commit
084f472, live ArangoDB + memory-layer, pool of 21 workers (os.cpus().length - 1on the measuring machine).Minimal harness — nothing else in the process:
Both arms fit one rate: 6.3% per teardown, 95% CI 4.1–9.3%. Pooling is licensed by asking whether the one-teardown arm contradicts the twenty-teardown arm's own rate (5.5%):
P(≥5 of 40 | p=0.055) = 0.067. It does not — borderline, and a failure to reject rather than a demonstration of agreement.Real ingests —
ingest-files.test.tsunder vitest, ~9.5 ingests per process at the time:Fisher exact on 2/75 vs 9/50 is p = 0.007 two-sided (0.004 one-sided), so load matters.
A real
ix ingestof 300 files: 0 of 60 — this is one teardown per process, not 9.5. At the idle rateP(zero in 60) = 0.84, at the loaded rate 0.29. Unremarkable under either.What that does and does not establish
The minimal harness overstates real exposure by 22× on the only load-matched comparison available (idle vs idle). Against the loaded real rate it is 3.1×, but that mixes conditions — the harness was never run loaded, and load raises the rate — so treat 3.1× as a lower bound on what is unexplained, not the residue after subtracting load.
Do not carry these rates to another pool size. They are per teardown of a 21-worker pool, and exposure plainly depends on how many addon-loaded isolates are disposed. CI runners are 3–4 vCPU, so a CI pool is 2–3 workers, not 21; the roll-ups below are not a CI flake budget.
Rolled up over that file's 14 ingests today: ~3.9% of processes idle, ~25% loaded — a projection, not a measurement.
Parses per worker is not controlled, and was miscounted twice.
ingestFilesparses a.tsfile twice (index prescan, then streaming loop, both on the same pool), so file counts double. Andingest-files.test.tsis not uniformly 30 files: it callsfixture(30)eight times,fixture(12)once andfixture(4)twice — 0.38 to 2.9 parses per worker across its runs, straddling the harness's ~1.4. So the earlier claim that "the harness parses the least per worker and crashes the most" is false for several of the teardowns behind the data, and parses-per-worker remains an open confound rather than a resolved one.The addon really was loaded
A bindings failure would move both counters, and both read zero: the workers die at module evaluation, raising the reported
parseError(the+ crashedParses()fold), and their tasks resolve null, raisingunparsed. The graph held 300 classes and 300 functions.The guaranteed floor for a spawned worker is the core plus the twelve statically imported grammars — not "13 required", because
tree-sitter-powershellis a required dependency that nonetheless loads through a null-returning helper and is absent from this checkout (a partial install; the lockfile pins 0.26.4 with no os/cpu restriction).That helper would also hide a genuine load failure, so it was checked: the
core-ingestion testsstep runs that package's suite on all five matrix legs includingwindows-2022, and passes. PowerShell parsing is not silently null.Scope
Comments only.
unref()teardown, the__shutdowncontract and every assertion are untouched; 13 pool tests pass, full suite 1791.Generated with Claude Code
https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt