Give merge_conflicts its own deadline instead of the 30s request default - #38
Merged
Conversation
The "Resolve Script Conflicts" action ran both its dry-run preview and the
real merge through a client left on mcpClient.ts's general-purpose
DEFAULT_REQUEST_TIMEOUT_MS (30s). A merge's runtime scales with the size of the
load order, so on a large one it simply cannot finish in that window.
Hit for real on a 274-mod install with 44 conflicting script files: the preview
alone exceeded 30s, so resolveScriptConflicts reported "Failed to preview
script-conflict merges" and returned at the preview stage - the real merge below
it never ran. Because a deploy had already cleared mod0000_MergedFiles, that left
an empty merged mod and a game that wouldn't start, with ~200 script compile
errors naming members the merge is supposed to add to vanilla scripts
('scmcc' is not a member of 'CNewNPC', Could not find function 'SSS_AddSkillSlot',
and so on). Vortex's log recorded only the transport error:
[WARN] witcherscriptmerger-vortex: resolveScriptConflicts failed
{"error":"WSM MCP request 'tools/call' timed out after 30000ms"}
Both call sites now pass MERGE_CALL_TIMEOUT_MS (10 minutes, matching
nexusDownloader.ts's own precedent for an operation whose runtime is the user's
data rather than a round trip). The preview gets it too, deliberately: a dry run
does the entire scan-and-three-way-merge computation and only skips the writes,
so sizing it as if it were cheap is exactly what broke.
The deadline is passed per call - callTool/request take an optional override -
rather than raised on the client at connect time, because requestTimeoutMs also
bounds the initialize handshake, and a WSM process that fails to start should
keep failing fast instead of hanging for ten minutes.
A timeout also now reports what it means for the install ("nothing was merged",
plus the note that an empty or stale merged mod can stop the game starting)
rather than surfacing a bare transport error; the original error is still passed
through as the notification detail.
9 new tests (217 total): the timeout reaching both call sites, connect not being
widened, the per-call override winning over the client default, not leaking to
later calls on the same client, the handshake keeping the short default, and the
timeout vs. non-timeout notification wording. typecheck and lint clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FP8H6rBLCGPBFRSVsF3Kgw
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.
What's wrong
The "Resolve Script Conflicts" action ran both its dry-run preview and the real merge through a client left on
mcpClient.ts's general-purposeDEFAULT_REQUEST_TIMEOUT_MS(30s).runMergeConflictsWorkflowcalledconnect({ exePath, env })with norequestTimeoutMs, so everymerge_conflictscall inherited it.A merge's runtime scales with the size of the load order. On a large one it simply cannot finish in 30 seconds.
How it failed
Hit for real on a 274-mod install with 44 conflicting script files. The preview alone exceeded 30s, so
resolveScriptConflictsreported "Failed to preview script-conflict merges" and returned at the preview stage (resolveAction.ts:141) — the real merge below it never ran. Vortex's log recorded only the transport error:Because a deploy had already cleared
mod0000_MergedFiles, this left an empty merged mod and a game that wouldn't start — ~200 script compile errors naming members the merge is supposed to add to vanilla scripts:Nothing in that error wall points at a timed-out MCP call, which is what made it expensive to diagnose.
The fix
MERGE_CALL_TIMEOUT_MS(10 minutes), matchingnexusDownloader.ts'sDEFAULT_DOWNLOAD_TIMEOUT_MSprecedent for "an operation whose runtime is the user's data, not a round trip".callTool/requesttake an optionaltimeoutMsoverride. RaisingrequestTimeoutMsatconnecttime would also bound theinitializehandshake — and a handshake that hasn't answered in a few seconds means a WSM process that failed to start, which should keep failing fast rather than inherit a merge-sized deadline.Note the sibling call sites (
conflictScan.ts,coexistenceGuard.ts) deliberately go the other way with 15s timeouts, and their comments explain why the 30s default is meant for short calls. Nothing had gone the other direction for the one call that legitimately takes minutes.Verification
9 new tests, 217 total, all passing.
npm run typecheckandnpm run lintclean.New
src/mcpClient.test.tscovers the real client's deadline plumbing against a mockedchild_process.spawnand fake timers:initializehandshake keeps the client defaultExtended
src/resolveAction.test.ts:dryRun: true)dryRun: false)connectis not handed a widenedrequestTimeoutMsOne test-harness note worth flagging for review: the fake child answers
initializesynchronously rather than viaqueueMicrotask/setTimeout, becausevi.useFakeTimerscontrols both and the handshake reply has to land without advancing time — advancing time is what triggers the deadlines under test. That's safe becauserequest()registers its pending entry before callingwriteMessage, so a response can never arrive "too early". The tests also deliberately skipclient.close(), whose grace period is itself a timer that fake timers would never fire.Not covered here
This fixes the timeout. It does not address the fact that the merged mod can be left empty by a deploy in the first place —
mod0000_MergedFilesis registered as a Vortex-managed mod (VK=…inmods.settings) pointing at an empty staging folder, so a purge/deploy cycle can wipe merge results that WSM, not Vortex, actually owns. Worth a follow-up.Separately,
[game-witcher] failed to ascertain merged mod namelogs an ENOENT for<game>\WitcherScriptMerger\WitcherScriptMerger.exe.config; on a real install that folder is empty (WSM sits at the game root) and .NET Core names the fileWitcherScriptMerger.dll.config. It falls back to the correct default, so it's benign today, but the path expectation is wrong on both counts.AI-assisted development
Produced by Claude Code (Opus 5), including the log-driven diagnosis on the affected install, the fix, and the tests. Per
CONTRIBUTING.md: the failure above was observed on a real 274-mod install, not inferred, and the test/typecheck/lint results are from real runs.