fix: bound every MCP request and control RPC so a busy daemon cannot hang the caller - #385
Merged
Conversation
An MCP handler that never returns used to take the whole session with it, and only on the transports without a daemon. mcp-go's stdio server hands every handler the server-lifetime context, so nothing downstream can cut a call off. A blocked handler holds its worker forever; once the pool and its queue are exhausted, processMessage falls back to running tool calls inline on the goroutine that reads stdin. Every other method — resources/read, prompts/get, tools/list — is already handled inline there. So one wedged handler stops the server reading its own input, and the client's notifications/cancelled sits unread in the pipe behind it. That is why the reporter saw index_health produce nothing for 30s and then watched every follow-up probe die the same way. The daemon socket was immune because it wraps each JSON-RPC frame in a 60-second request lifetime. This gives the other transports the same guarantee, at the layer they share: the middleware chokepoint that already carries the panic firewall. Same argument, different failure — a panic in one handler used to drop every session's transport, and so does a hang, just more quietly. An overrunning handler is abandoned, not killed: the caller gets a terminal structured error naming the deadline, and the transport's slot is released immediately so later requests are served. The handler keeps running, because it may be inside a store call nothing can interrupt, so the error says the side effect is unknown rather than pretending the work did not happen. Detached handlers are counted; past a ceiling new requests are refused with that count, which is the honest answer at that point. The bound fires just inside any deadline the transport already imposed, so the client reads the handler's diagnosis instead of an opaque transport timeout. index_health, the tool both reports named, gets the cancellation it could not previously honour. Its cost is proportional to workspace size — one stat per tracked file plus a graph scan — and it checked nothing, so a client that had already given up still paid for the whole sweep. It now also computes graph.Stats() once instead of twice. GORTEX_MCP_TOOL_TIMEOUT tunes or disables the bound for a machine where a tool legitimately runs longer than a minute. Fixes #300
… CLI Nothing in the control path had a deadline. Client.Control read the reply with a bare ReadBytes, the handshake ack was read the same way, and handleControl dispatched with context.Background(). The controller's mutex is coarse — Track holds it for the whole indexing run, as do Reload and every enricher — so any of those turned every other control request into an indefinite, silent block. internal/hooks had already learned this and set a socket deadline at each of its call sites; none of the CLI's own verbs did. Two of those unbounded calls sat where they did the most damage. `gortex daemon stop` opens with a Status lookup that exists only to print an uptime on the summary card, so the stop request had not even been sent when the command hung. And every `gortex call` opens with a Status probe to decide whether the daemon owns the repo. That is the reported shape: stop hangs past five minutes, and so does every CLI query, with nothing printed either way and no answer but kill -9. Kinds now carry a budget, and the two ends agree on it. Status, search_symbols and proxy answer promptly or return a terminal timeout naming the likely cause. Track, untrack, reload and the enrichers stay unbounded — they are long by design and a user who starts one is waiting on purpose. Shutdown is unbounded too, for the opposite reason: the daemon flushes and closes its store before acking, and abandoning that half-done is worse than a slow stop, so `daemon stop` carries the bound itself and falls through to watching the process exit rather than erroring out. The daemon now schedules its teardown even when the ack cannot be delivered, so a client that gave up waiting no longer leaves a flushed daemon still holding the store lock. The stop path also no longer queues behind the work it is ending. Reading the attached watcher — the first thing the teardown hook does — took the coarse mutex for a single pointer read; it is an atomic now. The routing probe fails open. Treating an indeterminate answer as "the daemon does not own this repo" produced a message that was both false and actively harmful: it told the user to run `gortex track`, which is the unbounded operation holding the lock. Proceeding to the MCP path costs nothing to be wrong about — that path does not need the controller mutex, and a genuinely untracked repo still comes back as repo_not_tracked. Fixes #370
Both hang reports ended with the same complaint: no error told the user which layer was responsible. Write down what each layer bounds, what "abandoned" means for a call's side effects, which kinds are deliberately unbounded and why, and where GORTEX_MCP_TOOL_TIMEOUT applies — it is read in the server's process, and on the daemon socket it can only tighten the bound, never raise it past the transport's own.
zzet
force-pushed
the
fix/mcp-daemon-request-liveness
branch
from
July 28, 2026 06:00
5923aa9 to
2e4f7bd
Compare
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.
Two reports, one shape: a request that never comes back, with nothing printed and no way out but
kill -9.gortex trackfinishes,gortex daemon stophangs past five minutes and everygortex callhangs with it.index_healthproduces nothing for 30s,smart_contextfor 60s, and every follow-up probe dies the same way.Neither is a slow-code problem. In both, something took longer than the caller was willing to wait and there was no layer that could say so.
#370 — the control plane had no bound anywhere
Client.Controlread the reply with a bareReadBytesand no socket deadline. The handshake ack was read the same way.handleControldispatched withcontext.Background(). The controller's mutex is coarse —Trackholds it across the whole indexing run, as doReloadand every enricher — so any of those turned every other control request into an indefinite, silent block.internal/hookshad already learned this: every one of its control call sites sets a socket deadline. None of the CLI's own verbs did.Two of those unbounded calls sat where they did the most damage:
gortex daemon stopopens with a Status lookup that exists only to print an uptime on the summary card (cmd/gortex/daemon.go,daemonUptimeBeforeStop). The stop request had not been sent yet when the command hung.gortex callopens with a Status probe to decide whether the daemon owns the repo (cmd/gortex/cli_daemon.go,daemonOwnsRepo).That is the reported shape exactly: stop hangs, every query hangs, nothing is printed either way.
#300 — the embedded transport had no request lifetime at all
mcp-go's stdio server hands every handler the server-lifetime context, so nothing downstream can cut a call off. A blocked handler holds its worker forever; once the pool and its queue are exhausted,
processMessagefalls back to running tool calls inline on the goroutine that reads stdin. Every other method —resources/read,prompts/get,tools/list— is already handled inline there.So one wedged handler stops the server reading its own input, and the client's
notifications/cancelledsits unread in the pipe behind it. That is why the follow-up probes died too.The daemon socket was immune: it already wraps each JSON-RPC frame in a 60s request lifetime. The embedded path never got the same guarantee.
index_healthis a fair thing to have hung on. It is a "minimal health probe" from outside, but the work is proportional to workspace size — oneos.Statper tracked file plus a whole-graph scan, run twice — and it checked no context at all.What changes
MCP (
internal/mcp/tool_deadline.go). Every tool call, resource read and prompt fetch is bounded, at the middleware chokepoint that already carries the panic firewall. Same argument, different failure: a panic in one handler used to drop every session's transport, and so does a hang, just more quietly.An overrunning handler is abandoned, not killed. The caller gets a terminal structured error naming the deadline; the transport's slot is released immediately so later requests are served. The handler keeps running — it may be inside a store call nothing can interrupt — so the error says the side effect is unknown rather than pretending the work did not happen. Detached handlers are counted, and past a ceiling new requests are refused with that count. The bound fires just inside any deadline the transport already imposed, so the client reads the handler's diagnosis instead of an opaque transport timeout.
GORTEX_MCP_TOOL_TIMEOUTtunes or disables it.index_healthnow honours cancellation and computesgraph.Stats()once instead of twice.Control plane (
internal/daemon,cmd/gortex). Kinds carry a budget and both ends agree on it (ControlTimeoutFor).status/search_symbols/proxytimeoutnaming the likely causetrack/untrack/reload/enrich_*shutdowndaemon stopcarries the bound itself and falls through to watching the process exit rather than erroring outThree follow-on fixes fell out of that:
daemon stopno longer queues behind the work it is ending.gortex track, which is the unbounded operation holding the lock. Proceeding to the MCP path costs nothing to be wrong about — that path does not need the controller mutex, and a genuinely untracked repo still comes back asrepo_not_tracked.What this does not fix
Worth stating plainly, because #370 has been reproduced five times and the next report should carry the right evidence.
This makes the failure bounded, attributable and recoverable. It does not make the daemon fast at the moment indexing completes.
EndCoordinatedBulkLoad(internal/graph/store_sqlite/bulk_load.go:349) holds the store write gate across two FTS merges, the index rebuild loop, andANALYZE— a single multi-minute critical section on a first cold load of a 20k-file workspace, arriving exactly when #370 says the daemon goes quiet. That is consistent with the reportedUNprocess state, which is a kernel-side uninterruptible wait (disk), not a goroutine parked on a Go mutex — a mutex wait shows asS.So: after this change that window produces a 30s
timeoutresponse naming the cause instead of an infinite hang,daemon stopterminates and leaves the socket and PID file in a startable state, andgortex callstill answers over the MCP path. Shortening the window itself is separate work. A goroutine dump (kill -QUIT) or asamplefrom a wedged daemon would settle whether anything else contributes.Verification
go build ./...,go vet,go test -race ./...— 12,056 tests across 178 packages.New regression coverage, each one mutation-checked to confirm it fails without the fix:
watcher()to the coarse mutex → the two shutdown tests failAddResource→ the blocked-read test fails and the panicking-read test crashes the process, which is the containment gap it guardsCovered: blocked tool call returns a terminal result; the session stays responsive afterwards; the abandoned-handler count drains; caller cancellation stays distinct from a server deadline; the transport-inherited deadline wins; the disable knob works; blocked and panicking resource reads; the fail-fast ceiling; the control timeout policy in both directions (bounded kinds bounded,
trackprovably not clamped); client- and server-side timeout reporting; teardown when the ack cannot be delivered; and the three-state routing decision.Fixes #370
Fixes #300