fix: bound embedding page-read concurrency to avoid EMFILE on large vaults - #30
Open
MarceloTrenkenchu wants to merge 1 commit into
Open
Conversation
…aults loadPageContents fired one fs.readFile per vault page via an unbounded Promise.all, so a compile on a vault with ~18k pages opened that many files at once and blew past the OS open-file-handle limit (EMFILE on Windows). The failures were also caught and discarded without their error code/message, so a resource-exhaustion failure was indistinguishable from a genuinely missing or corrupt page in the CLI output. - Move the existing worker-pool helper out of orchestration.ts into utils.ts as runWithConcurrency (now shared instead of duplicated) and route loadPageContents's page reads through it with a bounded concurrency of 64. - Surface the caught error's code/message in the warning text instead of swallowing it. - Add page-read-concurrency.test.ts covering the concurrency cap, result-ordering, and edge cases (maxParallel below 1 / above task count). Fixes swarmclawai#29
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
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.
Fixes #29.
Problem
loadPageContentsinpackages/engine/src/embeddings.tsfired onefs.readFileper vault page inside an unboundedPromise.all. On a vault with ~18k pages,swarmvault compileopens that many files at once and blows past the OS open-file-handle limit —EMFILE: too many open fileson Windows, and the equivalent underulimit -nelsewhere for large enough vaults. This reproduced on everycompilerun against our vault (18,423 pages), losing embedding coverage for a large, effectively random subset of pages each time.The failure was also invisible from the CLI output: the
.catch(() => { ...; return ""; })discarded the caught error entirely, soEMFILEprinted as an identical-looking "could not read page" warning to a genuinely missing or corrupt file. We only found the real cause by patching a local build to logerr.code/err.message.Full repro and root-cause writeup in #29.
Fix
runWithConcurrencytopackages/engine/src/utils.ts— this is the existing worker-pool helper that was already defined (unexported) insideorchestration.ts; moved it out and exported it instead of adding a second implementation, and updatedorchestration.tsto import the shared one.loadPageContentsnow routes its page reads throughrunWithConcurrencywith a concurrency cap of 64 instead of an unboundedPromise.all..catchhandler now includes the caught error'scode/messagein the warning text, so a resource-exhaustion failure reads differently from a missing-file failure.Testing
packages/engine/test/page-read-concurrency.test.ts: coversrunWithConcurrency's concurrency cap, result ordering under mixed completion timing, and edge cases (maxParallelbelow 1,maxParallelabove task count).tsc --noEmitonpackages/engine: clean.vitest run test/page-read-concurrency.test.ts test/retrieval.test.ts: all passing.packages/enginesuite both before and after this change on the same machine to separate signal from environment noise: 127/411 and 134/415 tests failed respectively — the +7 delta is exactly the 4 new tests (now passing) plus 3 extra file/test count from adding a file; the same pre-existing failures (missing builtdist/hooks/*.jsartifacts requiring a fullpnpm build, and a few timeouts under slow disk I/O) appear in both runs. Nothing invault.test.ts's failures touchesembeddings.ts,orchestration.ts, orutils.ts.biome checkon the changed files: no findings beyond CRLF line-ending noise from this being a fresh Windows checkout withcore.autocrlf=true(confirmed the same noise appears on an untouched file, e.g.config.ts) —git diffitself is clean of line-ending churn.Scope
Left the concurrency constant at 64 rather than making it configurable — no existing config surface distinguishes this from the embedding-batch size, and a fixed sane default is simplest. Happy to make it configurable via vault config if maintainers would prefer that.