Skip to content

Remove the merge_conflicts deadline entirely rather than raising it - #39

Open
TheValiantOne wants to merge 1 commit into
mainfrom
fix/remove-merge-call-timeout
Open

Remove the merge_conflicts deadline entirely rather than raising it#39
TheValiantOne wants to merge 1 commit into
mainfrom
fix/remove-merge-call-timeout

Conversation

@TheValiantOne

Copy link
Copy Markdown
Owner

Follow-up to #38, which gave merge_conflicts a ten-minute deadline instead of mcpClient.ts's 30s default.

Why that wasn't the right fix

A wall-clock limit is the wrong instrument for this call at any value. A merge's runtime is a function of the user's load order, with no bound anyone can pick in advance — so a deadline can only ever fire on a merge that is running normally and simply needs longer. And firing means abandoning it mid-flight, which is exactly how the merged mod ended up empty and the game unable to start in the original incident.

Ten minutes makes that rarer. It doesn't make it correct.

The change

Both call sites — dry-run preview and real merge — now pass NO_REQUEST_TIMEOUT.

Waiting indefinitely is safe because liveness, not the clock, detects a dead server. WsmMcpClient's constructor already wires failAllPending to the child's exit, error and stdin-error events, so a WSM process that crashes, is killed, or closes its pipes rejects every in-flight request immediately with a WsmMcpProcessError. A deadline adds nothing on top of that except the ability to give up on a process that is alive and still working.

Mechanically:

  • request() takes number | null | undefinedundefined means "client default", null (NO_REQUEST_TIMEOUT) means "no deadline"; no timer is created in that case.
  • PendingRequest.timer widens to allow undefined. clearTimeout(undefined) is a no-op, so both settle paths are unchanged.
  • Still applied per call, not as connect()'s client-wide requestTimeoutMs — that also bounds the initialize handshake, where a few seconds of silence means WSM failed to start and should keep failing fast.
  • A timeout can therefore no longer originate from the merge itself, so reportFailure's timeout wording now points at WSM failing to start rather than being slow.

Verification

219 tests, all passing. npm run typecheck and npm run lint clean, rebased on current main (post-#38).

Reworked src/mcpClient.test.ts around the two properties that now matter:

  • a call made with NO_REQUEST_TIMEOUT never settles after an hour of fake time (previously: rejects at the override)
  • it still rejects promptly when the child exits (exited unexpectedly (code 1)) — the liveness net that makes an unbounded wait safe
  • it still rejects when stdin errors (broken pipe)
  • a call with no override still uses the client default
  • an unbounded call in flight does not stop later calls on the same client timing out
  • the initialize handshake keeps the short default

src/resolveAction.test.ts now asserts both merge calls carry null, and that connect is still not handed a widened requestTimeoutMs.

Note on the trade-off

There's no cancel affordance for a merge in flight — if WSM genuinely wedges while alive, the action waits with its activity notification up until the process is killed. I think that's the right side to err on: an unbounded wait with visible progress beats silently abandoning a merge and leaving an empty merged mod, which is the failure this whole thread came from. Worth flagging explicitly for review, since it's the one behaviour a deadline did buy.

AI-assisted development

Produced by Claude Code (Opus 5). Per CONTRIBUTING.md: the test/typecheck/lint results are from real runs, and the failure mode motivating this was observed on a real 274-mod install, not inferred.

The previous commit gave merge_conflicts a ten-minute deadline instead of
mcpClient.ts's 30s default. That was the wrong fix: a wall-clock limit is the
wrong instrument for this call at any value. A merge's runtime is a function of
the user's load order, with no bound anyone can pick in advance, so a deadline
can only ever fire on a merge that is running normally and simply needs longer -
and firing means abandoning it mid-flight, which is how the merged mod ended up
empty and the game unable to start in the first place.

Both call sites (dry-run preview and real merge) now pass NO_REQUEST_TIMEOUT.

Waiting indefinitely is safe because liveness, not the clock, is what detects a
dead server: WsmMcpClient's constructor already wires failAllPending to the
child's exit, error and stdin-error events, so a WSM process that crashes, is
killed, or closes its pipes rejects every in-flight request immediately with a
WsmMcpProcessError. A deadline adds nothing to that except the ability to give up
on a process that is alive and still working.

Still applied per call, not as connect()'s client-wide requestTimeoutMs: that
also bounds the initialize handshake, where a few seconds of silence means WSM
failed to start and should keep failing fast. request() now takes
number | null | undefined - undefined means "client default", null means "no
deadline" - and PendingRequest.timer widens to allow undefined
(clearTimeout(undefined) is a no-op, so settle paths are unchanged).

A timeout can therefore no longer originate from the merge itself, so
reportFailure's timeout wording now points at WSM failing to start rather than
being slow.

Tests updated (219 total): the merge call never settles after an hour of fake
time; it still rejects promptly when the child exits or its stdin errors (the
liveness net that makes this safe); a call with no override still uses the client
default; an unbounded call does not stop later calls on the same client timing
out; and the handshake keeps the short default. typecheck and lint clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FP8H6rBLCGPBFRSVsF3Kgw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant