fix(ingestion): pipe worker stdout and make ready timeout configurable#2542
fix(ingestion): pipe worker stdout and make ready timeout configurable#2542GenKoKo wants to merge 1 commit into
Conversation
Two worker-pool startup fixes:
1. Spawn parse workers with {stdout: true} and forward the piped stream
to the parent's stdout. Workers with inherited stdout crash silently
during top-of-script init (exit 1, nothing on stderr, roughly half of
a concurrently spawned pool) on macOS 26.5 under Node 22 and 26;
piping eliminates the crash and matches the existing stderr handling.
2. Allow GITNEXUS_WORKER_READY_TIMEOUT_MS to override the hardcoded 5s
ready budget. A full pool cold-starting concurrently on a slow host
can exceed 5s, which reproduces identical timeout signatures across
respawns and gets misclassified as a deterministic startup crash-loop,
aborting the whole analyze.
|
@GenKoKo is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
magyargergo
left a comment
There was a problem hiding this comment.
Review: PR #2542 — pipe worker stdout + configurable ready timeout
Graph-backed review (GitNexus index built at the PR head in a detached worktree: 216,630 nodes / 460,102 edges, --pdg). Verified locally: both new test files pass (4/4), the full worker-pool-*.test.ts set passes (11 files, 68/68), tsc --noEmit clean.
The change itself is sound and well-scoped: detect_changes (compare vs merge-base ed8ab1c2) maps the behavioral diff to createWorkerPool / spawnAndCapture / forwardWorkerStdout / WORKER_READY_TIMEOUT_MS with 0 affected execution flows; the only production d=1 dependent of createWorkerPool is getOrCreateWorkerPool (parse-impl.ts:600), which passes no workerFactory override — so stdout: true genuinely reaches the production pool. Taint pass (explain on the PDG layer): no source→sink findings introduced.
Findings
- [MEDIUM]
gitnexus/src/core/ingestion/workers/worker-pool.ts:1615— theTimeoutDecisionunion reformat failsnpx prettier --check ., which CI enforces on PRs (ci.yml→ci-quality.yml:23). This PR will fail the format gate. Details inline. - [MEDIUM] discoverability — the failure path this PR fixes never mentions the new knob: the timeout message (
worker-pool.ts:813) andhandleWorkerStartupFailure's fix hint (parse-impl.ts:342, "often a missing/broken native binding") still steer a slow-host user toward reinstall/Node-version advice. Details inline on the README row. - [LOW]
worker-pool.ts:709— per-chunktoString('utf8')inforwardWorkerStdoutcan tear multibyte UTF-8 at chunk boundaries; passing the chunk straight toprocess.stdout.writeis both simpler and lossless (probe evidence inline). - [LOW]
worker-pool.ts:427— module-load-time env read diverges from this file's own lazy-read pattern; the "mirrorsGITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS" comment overstates the symmetry. Details inline. - [LOW]
worker-pool-ready-timeout-env.test.ts:99— the invalid-value test doesn't actually assert the 5 s default. Details inline.
Change and blast-radius summary
- Target: fork PR GenKoKo/GitNexus
fix/worker-stdout-and-ready-timeout→main; head10db5a54, merge-base/review baseed8ab1c2(PR is slightly behind main tip4d7a0a69; no overlap with the interim main commits). - 4 files, +249/−4: README row, worker-pool.ts (factory
stdout: true,forwardWorkerStdouttee, env-overridable ready budget), two unit test files mirroringworker-pool-startup-stderr.test.tsconventions (same worker-double +as unknown as Workershape). - Both
WORKER_READY_TIMEOUT_MSconsumers (initial ready gate,waitForWorkerReadyrespawn path) pick up the override; the analyze respawn child inherits env, so the module-load-time read works in all production paths.
Coverage and residual risk
- The core macOS 26.5 claim (inherited stdout → silent worker exit 1) is not reproducible from this Linux review environment; I'm treating it as author-verified empirical evidence. The piped+forwarded path is behaviorally equivalent for consumers (same fd destination, now serialized through the parent), and the visible contract is unit-tested.
- Ordering note (already in the PR body): worker stdout now flows through the parent's event loop instead of raw fd interleaving — strictly more deterministic, no consumer depends on the old interleaving.
- Not covered by tests: the stdout-forward test asserts delivery, not backpressure; worker log volume makes that acceptable.
Verdict
REQUEST CHANGES — solely for the prettier/CI gate failure at worker-pool.ts:1615, which is a one-hunk revert. Everything else is advisory (2× LOW code suggestions, 1× MEDIUM discoverability, 1× LOW test-assertion tightening). With the format fix this is a clean, well-evidenced, appropriately minimal PR — the motivation writeup and test doubles are exemplary.
Reviewed at head 10db5a54f80969a45fdf503874481c0da3c54aa8 against merge-base ed8ab1c24679c45a6463f5b325fee44b3742ecdc.
| type TimeoutDecision = | ||
| | { kind: 'retry' } | ||
| | { kind: 'give-up'; reason: string; excludePaths: readonly string[] }; | ||
| { kind: 'retry' } | { kind: 'give-up'; reason: string; excludePaths: readonly string[] }; |
There was a problem hiding this comment.
[MEDIUM] Fails the CI format gate. This union reformat is not prettier-clean under the repo's pinned prettier (.prettierrc, printWidth 100):
$ npx prettier --check gitnexus/src/core/ingestion/workers/worker-pool.ts
[warn] gitnexus/src/core/ingestion/workers/worker-pool.ts
Prettier wants the original leading-pipe form back:
type TimeoutDecision =
| { kind: 'retry' }
| { kind: 'give-up'; reason: string; excludePaths: readonly string[] };CI runs npx prettier --check . on every PR (ci.yml → ci-quality.yml, format job), so the PR fails as-is. Since this hunk is unrelated to the fix anyway, reverting it resolves both the gate and the churn. (The PR checklist's "Prettier clean on touched files" was likely run with a different prettier version/config.)
| */ | ||
| const WORKER_READY_TIMEOUT_MS = 5_000; | ||
| const WORKER_READY_TIMEOUT_MS = | ||
| positiveInteger(process.env.GITNEXUS_WORKER_READY_TIMEOUT_MS) ?? 5_000; |
There was a problem hiding this comment.
[LOW] Module-load-time env read diverges from this file's own pattern. Every other pool knob resolves its env var lazily — GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS and friends inside resolveWorkerPoolOptions() (lines 560–595), GITNEXUS_WORKER_POOL_SIZE via envWorkerPoolSize() — so the doc comment's "mirroring GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS" overstates the symmetry: this one is frozen at first import.
It works in all production paths (CLI and the analyze respawn child inherit env at process start), but analyze.ts explicitly supports "programmatic callers (tests, long-running hosts)" with env snapshot/restore, and for those a post-import env change silently does nothing — your own test needed vi.resetModules() + dynamic import to cope. Reading it where the other knobs are read (or at createWorkerPool time) removes the special case; alternatively keep it as-is but soften the "mirrors" comment to note the module-load semantics.
| const stream = worker.stdout; | ||
| if (!stream) return; | ||
| stream.on('data', (chunk: Buffer | string) => { | ||
| process.stdout.write(typeof chunk === 'string' ? chunk : chunk.toString('utf8')); |
There was a problem hiding this comment.
[LOW] Per-chunk decode can tear multibyte UTF-8 — and it's avoidable with less code. process.stdout.write accepts a Buffer, so the chunk can be passed through untouched:
stream.on('data', (chunk: Buffer | string) => {
process.stdout.write(chunk);
});Decoding each chunk independently corrupts any multibyte sequence that straddles a chunk boundary. Probe:
const line = Buffer.from('解析ワーカー log line\n');
const chunks = [line.subarray(0, 4), line.subarray(4)]; // cut inside a 3-byte codepoint
chunks.map((c) => c.toString('utf8')).join('') // "解���ワーカー log line" (U+FFFD)
Buffer.concat(chunks).toString('utf8') // "解析ワーカー log line" (lossless)In practice worker-side writes usually arrive as whole chunks, so this is an edge — but given the repo is currently fielding invalid-UTF-8 corruption reports from CJK/Cyrillic-heavy repos (#2544/#2546, different locus, same corruption class), the byte-lossless one-liner is the safer shape. (captureWorkerStderr genuinely needs the string for its bounded tail, so the asymmetry with stderr is fine.)
| | `PROF_LBUG_LOAD` | unset | When `1`, emits one `[lbug-load prof]` summary line per `loadGraphToLbug` call breaking the graph-DB persistence wall into stages (`csv-emit` / `copy-nodes` / `copy-rels` / `fallback` / `total`) plus node & edge counts. Zero-cost when unset. | Attributing large-repo analyze wall time across CSV generation vs. LadybugDB `COPY` (issue #2203) — the analyze "emit" timing is the scope-resolution bucket, not this DB-write path. | | ||
| | `GITNEXUS_MAX_FILE_SIZE` | `512` (KB) | Walker skip threshold in KB. Hard cap is `32768` (tree-sitter buffer ceiling). Equivalent to `--max-file-size <kb>`. | Indexing repos with intentionally-large source files (generated parsers, vendored bundles) that should still be parsed. | | ||
| | `GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS` | `30000` | Worker idle timeout in milliseconds before retry/fallback. Equivalent to `--worker-timeout <seconds>` × 1000. | Slow-parsing files (large minified JS, deeply-nested TS types) that legitimately need more than 30s. | | ||
| | `GITNEXUS_WORKER_READY_TIMEOUT_MS` | `5000` | Startup budget in milliseconds for a parse worker to load its grammar bindings and report `{type:'ready'}`. Slots that miss it are treated as startup crashes. | Slow or heavily loaded hosts where a full pool cold-starting concurrently needs more than 5s, and analyze aborts with "did not report ready within 5000ms". | |
There was a problem hiding this comment.
[MEDIUM] The remedy is undiscoverable at the exact failure it fixes. When the slow-host scenario hits, the user sees did not report ready within 5000ms — likely crashed during top-of-script init (worker-pool.ts:813) and handleWorkerStartupFailure's fix hint — "often a missing/broken native binding or a top-of-script import error" (parse-impl.ts:342) — and, on the respawn path, the native-worker-abort hint suggesting reinstall / Node 22 LTS (analyze.ts:549). None of them mention GITNEXUS_WORKER_READY_TIMEOUT_MS; the only pointer is this README row. The next affected user gets actively wrong advice and likely files another issue (as #2061 / your report did).
One line closes the loop — append the knob to the timeout rejection message:
`… did not report ready within ${WORKER_READY_TIMEOUT_MS}ms — likely crashed during top-of-script init (slow host? raise GITNEXUS_WORKER_READY_TIMEOUT_MS)`That string flows verbatim into readinessFailures → the user-facing failureDetail, so it surfaces in both the initial-gate and respawn paths. Related doc nit: gitnexus/README.md has its own "Worker pool resilience tuning" env table (§ around line 538) listing the other GITNEXUS_WORKER_* knobs — the new var belongs there too.
| workerFactory: () => new ReadyWorker() as unknown as Worker, | ||
| }); | ||
| await pool.dispatch([]); | ||
| expect(pool.getStats().activeSlots).toBe(1); |
There was a problem hiding this comment.
[LOW] The fallback test doesn't assert the fallback. A ReadyWorker that passes the handshake proves module load didn't throw and the pool works — but it would pass identically if an invalid value produced NaN, 0, or any other timeout. The 5 s default is never observed.
The first test already has the stronger recipe: reuse NeverReadyWorker with the invalid env value and assert the failure message contains within 5000ms. That pins the actual default (test wall-time stays bounded by the 50 ms-style deadline only if you keep it as a slow test — if 5 s per run is too slow for the unit suite, asserting on the message constant is still the honest middle ground).
Summary
Two worker-pool startup fixes: spawn parse workers with piped (not inherited) stdout — inherited stdout causes silent startup crashes on some hosts — and make the 5s worker ready budget overridable via
GITNEXUS_WORKER_READY_TIMEOUT_MS.Motivation / context
On macOS 26.5 (arm64),
gitnexus analyzeaborted 100% of the time with thenative-worker-abortrecovery hint. Root-causing it surfaced two independent layers:{ stderr: true }(stdout inherited), roughly half of a concurrently spawned pool exits with code 1 during top-of-script init — nothing on stderr, so the pool's stderr capture (GitNexus analyze gets stuck at 49% during “Resolving calls”. #1741) has nothing to attach. Reproduced deterministically on an idle host under both Node 22.22 and 26.4, on both 1.6.8 and 1.6.9 (so not a recent regression — an environment-triggered latent issue). Spawning with{ stdout: true }eliminates the crash entirely (7/7 workers ready across repeated runs vs 4/7 with inherited stdout). The piped stream is forwarded to the parent's stdout (forwardWorkerStdout, same tee shape ascaptureWorkerStderr) so worker logs stay visible.GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS; the default is unchanged.With both fixes applied, a previously 100%-failing full rebuild (87.6k nodes) completes, and incremental re-analyze passes.
Related: #2432 / #2436 fixed a different crash under the same
native-worker-abortbanner; #2061 reported the same silent worker exit-1 symptom on Node 22.22.x.Areas touched
gitnexus/(CLI / core / MCP server)gitnexus-web/(Vite / React UI).github/(workflows, actions)eval/or other toolingAGENTS.md,CLAUDE.md,.cursor/,llms.txt, etc.)Scope & constraints
In scope
worker-pool.ts: production factory spawns withstdout: true; newforwardWorkerStdouttee;WORKER_READY_TIMEOUT_MSreadsGITNEXUS_WORKER_READY_TIMEOUT_MSvia the existingpositiveIntegerguard.worker-pool-startup-stderr.test.ts).Explicitly out of scope / not done here
Testing & verification
cd gitnexus && npx vitest run test/unit/worker-pool-stdout-forward.test.ts test/unit/worker-pool-ready-timeout-env.test.ts— 4/4 passcd gitnexus && npx vitest run test/unit/worker-pool-*.test.ts(startup-stderr, options, resilience regression files included) — 47/47 passcd gitnexus && npx tsc --noEmit— cleangitnexus analyzefull rebuild goes from 100% fail → complete (87,642 nodes / 116,156 edges), incremental re-analyze passesNote: the full unit suite on the affected host flakes on heavy real-analyze tests (30s per-test timeouts) — identically on unmodified
main— because the host is slow/loaded; failing subsets differ run to run. Relying on CI for the full-suite verdict.Risk & rollout