fix(#5970): size the daemon's in-process parse gate from the background core budget - #6023
Merged
Merged
Conversation
…nd core budget cmd/grafel/daemon.go sized the process-wide tree-sitter parse semaphore from runtime.GOMAXPROCS(0) — 100% of the daemon's runtime parallelism. #5972 (CLI index path) and #5973 (extract-subprocess fanout) both shipped the 25% policy and neither touched this call site: both fixed paths that FORK, and this is the one that does not. It is also the only one that runs reactively during the user's normal work — the watcher fires on their own save/checkout and the daemon re-parses in-process while they type — so it was the most user-visible instance of the policy and the last unfixed one. WHAT CHANGES daemonParseCap(hostCPU, fileVal) resolves the cap; installDaemonParseCap loads the cpu.json pin from the caps store and installs the result. Precedence, matching resolveDaemonGOMAXPROCSWith for every other daemon CPU knob, with only the trailing default differing: GRAFEL_DAEMON_GOMAXPROCS > cpu.json daemon_gomaxprocs > IndexCoreBudgetFor Both operator surfaces keep top precedence UNCLAMPED in either direction. On a 12-core host the daemon's gate goes 12 -> 3. Installed after the caps store exists and re-resolved on SIGHUP alongside the GOMAXPROCS re-apply, so the live-mutable surface stays live for the gate that actually bounds cgo. The half-cores GOMAXPROCS DEFAULT is deliberately not consulted: it bounds the daemon's Go-runtime parallelism (query handling included), it is not a statement about how much background parsing may draw. Correction to an earlier draft of this message: the pre-change `parseCap := runtime.GOMAXPROCS(0)` did NOT reflect cpu.json either. applyDaemonGOMAXPROCSFromCaps runs only from the SIGHUP handler, never at startup, so at process start the runtime carried the env-or-half-cores value and the pin was ignored. Honouring it here is a fix, not a restoration. FOREGROUND/BACKGROUND DETERMINATION The daemon's in-process parse is NOT always background, so it is not capped unconditionally. Two user-awaited paths parse inside the daemon process: - daemonIndexFunc — the synchronous `grafel index` RPC, with the user's CLI blocked on it (it already asserts this by passing WithInteractive(true)); - daemonRebuildFuncCore — but ONLY under GRAFEL_SUBPROCESS_INDEXER=0. On the shipped default its per-repo index forks (runRebuildSubprocess, whose child sizes its own gate from --interactive) and daemonForegroundLinks does no tree-sitter parsing at all (internal/links has no treesitter dependency), so the hold is gated on !sched.SubprocessIndexEnabled(). Everything else through the gate — the watcher-driven incremental reindex — is background. The existing foreground signals do not reach here: --interactive is resolved inside the index CHILD, and sched's per-group registry is consulted at FORK time. Neither exists on the path that never forks. Since the gate is one process-wide semaphore installed once at startup, the exemption cannot be a second cap; it is a refcounted, idempotent scoped lift (indexstate.BeginForegroundParse). ParseConcurrencyCap still reports the CONFIGURED cap so ensureParseConcurrencyDefault's "already installed?" check is unaffected; EffectiveParseConcurrencyCap reports what acquirers are gated on now. KNOWN AND BOUNDED The lift is PROCESS-WIDE, not per-caller: while a hold is live, concurrent BACKGROUND parsing is uncapped too, and since release does not preempt, an overshoot outlives the release until admitted parses drain (a probe reached 20 concurrent parses against a background cap of 2). Gating the rebuild hold on the in-process path removes the case that mattered — a 10-12 minute window suspending the budget in the one config where the hold bought nothing, while the watcher-driven reindex the barge does not gate kept parsing. What remains is bounded by the scheduler's hard-coded worker pool of 2 and TryIncremental's sequential extraction. The per-caller exemption (acquire that bypasses the semaphore without touching parseGateCap) is the correct end state and is filed as #6022; BeginForegroundParse's doc now states the process-wide semantics plainly, since reading it as caller-scoped is what produced this. Two further gaps, accepted: `--async` on a scheduler-less non-split daemon falls through to the synchronous s.index and takes the lift (there is no watcher in that config to escape); and in split mode the rebuild is applied asynchronously with retries, so a hold can be live with nobody waiting — the same assumption the existing foreground barge already makes. TESTS The failure mode this issue is about is a guard that exists, looks right and binds nothing, so the tests assert the daemon path specifically: the value that actually reaches indexstate when the daemon installs it (including from a real cpu.json on disk), and — via the AST of runDaemonMode, which binds sockets and blocks forever so it cannot be called — that runDaemonMode is what installs it, with the host core count UNSCALED and a non-nil caps store. The argument assertion matters: a call-exists-only check stays green against installDaemonParseCap(runtime.NumCPU() * 4), which reinstates the original bug's exact magnitude. A separate test pins that the rebuild's hold is conditional, not merely present. Mutation-checked, each turns the suite red: reverting the call site to runtime.GOMAXPROCS(0); scaling the hostCPU argument; passing a nil caps store; dropping the cpu.json branch from the policy; making the helper GOMAXPROCS-shaped; dropping either foreground hold; making the rebuild hold unconditional; making the lift inert; and making the release closure a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft
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.
The daemon's in-process tree-sitter parse gate was sized from
runtime.GOMAXPROCS(0)— on a 12-core host, a cap of 12. Background indexing is supposed to get 25% of the machine (process.IndexCoreBudget(),max(1, NumCPU/4)), so the gate that is meant to bound it was running at 4x the policy. This is write-side RSS: each concurrent parse holds a tree-sitter tree, and tree-sitter allocates through cgo, whichGOMEMLIMITcannot see.The gate is now sized
env > cpu.json > 25% budget— the same precedence chainresolveDaemonGOMAXPROCSWithdocuments, differing only in the trailing default. On a 12-core host: 12 → 3.Precedence, and a correction
cpu.jsonis now consulted. An earlier draft of this description called that a restored behaviour, on the theory that the oldruntime.GOMAXPROCS(0)read reflected the pin becauseapplyDaemonGOMAXPROCSFromCapshad already applied it. That was wrong, and worth recording rather than quietly fixing:applyDaemonGOMAXPROCSFromCapshas exactly one non-test call site, inside the SIGHUP handler (cmd/grafel/daemon.go:394). Nothing calls it at startup. So at process start the runtime carried the env-or-half-cores value and thecpu.jsonpin was already ignored by the parse gate.An operator pinning the daemon to 1 core via
cpu.jsonon a 64-core box therefore got a parse gate of 12 before this change, not 1. This closes a pre-existing gap; it does not restore something that previously worked.installCapReloadHandlernow re-resolves the gate on SIGHUP alongside the GOMAXPROCS re-apply, so the live-mutable surface stays live for the gate that actually bounds cgo.Dropping the half-cores GOMAXPROCS default is deliberate:
internal/process/corebudget.go:38-45already documents that GOMAXPROCS cannot bound cgo, so deriving a cgo-allocation cap from it is a category error.The foreground lift was suspending the budget for the whole rebuild
indexstate.BeginForegroundParselifts the cap process-wide, not for the calling unit of work:effectiveParseCapLockedreturns 0 for every acquirer while any hold is live, and there is no caller identity in the gate. A probe measured 20 concurrent background parses against a background cap of 2, and because release does not preempt, the overshoot persists past the release until those parses drain.daemonRebuildFuncCoreheld it across the entire rebuild — self-documented elsewhere as 10-12 minutes — while the barge taken alongside it gates EXCLUSIVE stages only and explicitly does not stop watcher-driven incremental indexing.And on the shipped default that hold bought nothing. With the subprocess indexer on, the rebuild does no in-process tree-sitter parsing at all: the per-repo index forks via
runRebuildSubprocess, the in-processindexFnis theGRAFEL_SUBPROCESS_INDEXER=0branch, andinternal/linkshas no treesitter dependency. So the hold was pure downside — a multi-minute window in which the budget this commit exists to enforce was not enforced, in exchange for nothing.The hold is now conditional on
!sched.SubprocessIndexEnabled(). TheBeginForegroundParsedoc comment previously read as though the exemption were scoped to the caller ("a USER-AWAITED unit of in-process parsing"), which is how this call site came to exist; it now opens with "READ THIS BEFORE ADDING A CALL SITE", states the process-wide semantics and the non-preempting release, and carries the probe numbers.The per-caller exemption (
AcquireParseSlotForeground, bypassing the semaphore without mutating sharedparseGateCap) is the correct end state and is not built here — see #6022. The residual escape is currently bounded to ≤2 by the scheduler worker pool being hard-coded to 2 andTryIncrementalextracting sequentially; note that raising that pool for throughput would silently turn this back into a memory regression.Verification
The cap binds on the path that runs:
installDaemonParseCapsits in the shared prelude ofrunDaemonMode, ahead of the mode branch, so bothgrafel serveandgrafel engineroute through it. Nothing re-installs downstream —ensureParseConcurrencyDefaultearly-returns because the configured cap is always > 0 in the daemon, andextract-internalis a separate process. TheParseConcurrencyCap/EffectiveParseConcurrencyCapsplit is load-bearing: hadensureParseConcurrencyDefaultread the effective cap, agrafel indexRPC inside a daemon runningGRAFEL_DAEMON_GOMAXPROCS=12would clobber 12 → 3.runDaemonModeis not directly testable, so the install is pinned by an AST assertion. Review found the first version of that pin value-blind — it checked only that the call existed, andinstallDaemonParseCap(runtime.NumCPU() * 4, ...)left all 418 tests green, reinstalling the original bug's exact magnitude. The pin now checks the argument expressions. Verified independently: that mutation now fails withrunDaemonMode passes hostCPU=runtime.NumCPU() * 4 ... want runtime.NumCPU().Full mutation battery, all died: call site reverted to
runtime.GOMAXPROCS(0); helper GOMAXPROCS-shaped; scaled hostCPU argument; nil caps store; policy ignoringcpu.json; either foreground hold dropped; the lift made inert (this last one turns 4 tests red across two packages — a permanently-uncapped daemon is caught).Values verified against
indexCoreBudgetDivisor = 4: 1→1, 2→1, 4→1, 8→2, 12→3, 16→4, 32→8, 64→16, with the env override unclamped in both directions.go build ./... && go vet && gofmt -lclean. 1369 tests acrosscmd/grafel,internal/daemon,internal/process,internal/indexstate.internal/daemon/sched -race -count=2(394) andinternal/indexstate+internal/treesitter -race -count=4(540) green.Known and bounded
Two edge cases accepted rather than fixed:
--asyncon a watcher-less non-split daemon (s.scheduler == nil) falls through to the synchronous index path and takes the foreground lift — low severity, since there is no watcher in that configuration to escape; and in split mode the rebuild is applied asynchronously by the engine'srebuildWorkerwith retries, so the hold can be live with no human waiting, which is the same assumption the existing barge and foreground registry already make.Independently adversarially reviewed.
Closes #5970
Refs #5954, #6022