fix(engine-client): a call that is never answered is a failure, not a wait - #678
Merged
Conversation
… wait `fetch` has no timeout, so a connection the engine ACCEPTS and never answers hangs forever. Only the drain was bounded; every other call — including the one `kild watch` polls with — waited indefinitely. That defeats a guarantee already shipped. `kild watch` distinguishes a quiet engine (exit 2) from a dead one (exit 3), and a hung call sits inside a single poll past the whole window, reporting neither. The verb looks patient at exactly the moment it has stopped working, which is the failure it was built to remove. `engineFetch` now applies a 30s backstop when a caller supplies no signal of its own. Deliberately generous: it exists to stop an unbounded wait, not to budget latency, so it has to clear the slowest legitimate call — a land's merge, a new kild's worktree — without argument. A caller's own signal wins outright rather than being combined, because a narrower deadline was chosen for a reason and capping it at the default would make the tighter bound a lie. `kild watch` bounds each poll to its own cadence, floored at 1s so a deliberately tiny --interval cannot turn a healthy engine into a failing one. A hang is then an ordinary tolerated failure, and three of them an honest `unreachable`. The timeout is reported as what it is. An abort surfaces as "the operation was aborted", which tells a caller nothing about whether to retry — and a hook that swallowed it would be swallowing the one symptom separating a wedged engine from a quiet one. It now names the path and says the engine accepted the connection and did not answer. One test I wrote was wrong and I moved it rather than weakening it: a 5s hang never trips a 30s default, so it proved nothing. The wording is now asserted where the path is actually reachable — the drain's own 1.5s bound, and watch's per-poll bound — instead of behind a 30s test. Gates: 442 tests, typecheck, lint, e2e 70/70. Dogfooded against a real server that accepts and never answers: exit 3 in 4s, where before it would have waited out the full 300s window and reported nothing.
…t unheard Review found this PR made a confidently-wrong exit reachable, which is worse than the hang it fixed. Six findings; all addressed. THE CRITICAL ONE, and it is the same defect for the third time on this feature: two decisions answering one question. The per-poll bound used the interval; the loop's deadline used the deadline. With `--interval 10 --timeout 3` against a hung engine the first poll blocked ten seconds, overran the window threefold, and then reported QUIET — asserting the engine had answered and had nothing, after every request had failed. `watchRequestTimeout` now takes what remains of the window as a required argument, so the two answers cannot drift because there is only one. The other half: "quiet" is a claim ABOUT the engine — that it responded and had nothing new. A watcher that never heard from it cannot make that claim. A window closing with no successful poll now exits `unreachable`, not `quiet`. That is a behaviour change and one existing test asserted the old answer; its expectation was wrong and is corrected rather than relaxed — what it exists to check is the elapsed time, which is unchanged. A CLIENT ABORT DOES NOT REACH THE ENGINE. Nothing passes a signal server-side and no git command it runs is cancellable, so a timed-out `land` may still merge and a timed-out `rm` may still remove the tree. Reporting that as a plain failure is the worst available answer: the operator retries a merge that already happened. Both now report the outcome as UNKNOWN and say to check before retrying — the same rule the disposal path already follows for its discard list. `kild show` swallowed every message failure into an empty log. Harmless while a hung engine hung visibly; a confident lie the moment a timeout made the call return. It reports the failure now. The timeout is decided by WHOSE signal fired, not by matching an error name. Name-matching would relabel the first future caller that cancels for any other reason — a Ctrl-C — as "the engine did not answer", the opposite of true. The original error rides along as `cause`. The backstop is overridable via KILD_ENGINE_TIMEOUT_MS. 30s is a guess about somebody else's disk, and an operator whose merge legitimately exceeds it needs a way out that is not editing the source. It also closes the coverage gap the review proved: removing the default left the suite green, because both hang tests supply their own signals and bypass it. A test now goes through `kild log`, which supplies none. Also corrected a claim of mine in the comment: `kild new` is NOT bounded by this call. `spawn` returns without awaiting and the worktree is created in the child. `land` and `rm` are the calls this budget actually has to clear. Gates: 445 tests, typecheck, lint, e2e 70/70. Dogfooded the exact reported regression against a server that accepts and never answers: 3040ms and exit 3, where it was 10036ms and exit 2.
Two defects I introduced in the previous round and found by probing my own change rather than waiting to be told. `mayHaveHappened` decided "this may have completed" by searching the error text for "timed out". That is reading intent out of prose, which this codebase forbids — and it is wrong for the exact reason the rule exists: an engine-side failure relayed verbatim can contain those words. A `git merge` that failed with "operation timed out talking to the object store" would have been reported as a merge that might have gone through, sending an operator to look for work that never happened. The timeout is now a type, `EngineTimeout`, and the decision is `instanceof`. `KILD_ENGINE_TIMEOUT_MS` was `Number(raw ?? 30_000)` with no validation. An empty value or "0" parses to zero, which aborts every request the instant it is made — indistinguishable from an engine refusing to talk, and caused by a variable the operator set to help. Non-numeric values reached `AbortSignal` and surfaced as a RangeError naming neither the variable nor the fix. It is parsed once now and an unusable value is refused by name. Both were in the fix for a review finding, which is the third time on this branch that a repair has needed a repair. The difference is that these two were found by asking what my own change could now do wrong — the string match and the unvalidated env read were both visible the moment I looked at them as inputs rather than as implementation. Gates: 451 tests, typecheck, lint, e2e 70/70.
…es get tests Second review: NO-MERGE, one critical and three important. All addressed. CRITICAL, and the same one-decision-two-places class again, in a new spot. Removing `kild show`'s blanket catch made it CRASH on an orphan kild. `GET /:id` answers for an orphan tree — resolveKild falls back to an orphan identity — while `GET /:id/messages` 404s, because the registry's log map holds live and archived kilds only. Two routes, two answers to "does this exist". The swallow had been hiding that disagreement; removing it turned it into an exit 1 with no output at all, against a case `kild show` has an explicit display branch for, and directly contradicting the comment I had just written two lines above it. Now a 404 is tolerated and nothing else is. It branches on a typed `EngineHttpError` carrying the status rather than on the message text — the same reason the timeout became a type last commit: reading meaning out of an error string is reading intent out of prose. The abort branch now reads the SIGNAL'S REASON instead of asking whose signal it was. `AbortSignal.timeout` sets a TimeoutError reason and a deliberate cancellation does not, so both deadlines are timeouts and each says which one it was. My previous version tested only "was it ours", which quietly regressed the drain — a caller-supplied deadline — to a bare "The operation timed out." naming neither the path nor the cause. The env validation is read per call, not at module load. Evaluated at import it threw before dispatch()'s handler existed, so a fat-fingered override killed EVERY command, including ones that make no engine call, with a raw Bun stack trace instead of this CLI's `error: <message>`. The review proved by reverting that the two highest-severity fixes had NO coverage — remove `mayHaveHappened` from land and rm, or revert the typed timeout, and all 451 tests still passed. Both are pinned now, along with the orphan case, and I verified the same way rather than assuming: reverting each fix in a scratch copy fails 1, 1 and 3 tests respectively. Gates: 455 tests, typecheck, lint, e2e 70/70. `kild show` verified against a real orphan on the running engine.
The verification pass cleared this branch and found one gap by the same method that caught the last two criticals: reverting the per-call env read failed zero tests. Both versions exit 1 with KILD_ENGINE_TIMEOUT_MS in the text — the reverted one as a raw Bun stack trace naming a source line, the fixed one as the CLI's own formatted error — and asserting only the substring could not tell them apart. That is the fix having no coverage at all, which is exactly the shape that hid the previous two rounds. Asserting the absence of the stack trace closes it; reverting the fix now fails the suite. Gates: 455 tests, typecheck, lint, e2e 70/70.
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.
fetchhas no timeout of its own, so a connection the engine accepts and never answers hangs forever. OnlydrainInboxwas bounded; every other call — including thekildMessagesthatkild watchpolls with — waited indefinitely.Why this is urgent rather than tidy
It defeats a guarantee that shipped two hours ago.
kild watchexists to distinguish a quiet engine (exit 2) from a dead one (exit 3). A hung call sits inside a single poll past the entire window and reports neither — so the verb looks patient at precisely the moment it has stopped working. That is the failure it was built to remove, reintroduced underneath it.Found by the
kild watchreview, correctly left out of that PR, and helm's agent independently argued it should jump the queue for the same reason: it is the one open item that undermines something already shipped.The fix
engineFetchapplies a 30s backstop when a caller supplies no signal. Deliberately generous — it exists to stop an unbounded wait, not to budget latency, so it must clear the slowest legitimate call (a land's merge, a new kild's worktree) without argument.A caller's own signal wins outright rather than being combined. A narrower deadline was chosen for a reason, and silently capping it at the default would make the tighter bound a lie.
kild watchnow bounds each poll to its own cadence, floored at 1s so a deliberately tiny--intervalcannot turn a healthy engine into a failing one. A hang becomes an ordinary tolerated failure, and three of those an honestunreachable.The message says what happened
An abort surfaces as "the operation was aborted", which tells a caller nothing about whether to retry — and a hook that swallowed it would be swallowing the one symptom separating a wedged engine from a quiet one. It now reads:
A test of mine was wrong
I wrote one asserting the timeout wording via a 5s hang. A 5s hang never trips a 30s default, so it proved nothing and passed for the wrong reason. Moved rather than weakened — the wording is now asserted where the path is genuinely reachable: the drain's own 1.5s bound (loud when run as a verb, silent inside the hook) and watch's per-poll bound.
Gates
bun testbun run typecheckbun run lint./scripts/e2e.shDogfooded against a real server that accepts the connection and never answers: exit 3 in 4s, where before it would have waited out the full 300s window and reported nothing.