Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions vortex-extension/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,15 @@ does anything for any other game.
whole-file merge failed but merging function-by-function succeeded), and - only on
confirmation - spawns a second process to run the real merge and shows its result. v1
scope, deliberately: merges every detected conflict in one pass; there's no per-file
selection or custom merge-order override yet. Both `merge_conflicts` calls get their own
ten-minute deadline (`MERGE_CALL_TIMEOUT_MS`) rather than `mcpClient.ts`'s
general-purpose 30s `DEFAULT_REQUEST_TIMEOUT_MS` — a merge's runtime scales with the
load order, and the dry-run preview costs the same as the merge it previews (it does the
full three-way merge and only skips the writes). The deadline is passed per call, so the
`initialize` handshake keeps the short default and a WSM process that fails to start
still fails fast.
selection or custom merge-order override yet. Both `merge_conflicts` calls run with **no
deadline** (`MERGE_CALL_TIMEOUT` = `NO_REQUEST_TIMEOUT`) rather than `mcpClient.ts`'s
general-purpose 30s `DEFAULT_REQUEST_TIMEOUT_MS` — a merge's runtime is the user's load
order, so any wall-clock limit can only fire on a merge that is working normally, and the
dry-run preview costs the same as the merge it previews (it does the full three-way merge
and only skips the writes). That's safe because the client fails every in-flight request
the moment the WSM process exits, errors, or closes its pipes, so liveness — not the
clock — detects a dead server. Applied per call, so the `initialize` handshake keeps the
short default and a WSM process that fails to start still fails fast.
- **A merge-history dashboard tile** (`src/mergeHistoryDashlet.ts`): lists every merge
WSM has already recorded (via its MCP `list_merges` tool) - relative path, which merged
mod folder holds the result, and each source mod's recorded hash - with a manual
Expand Down
66 changes: 48 additions & 18 deletions vortex-extension/src/mcpClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
const { spawnMock } = vi.hoisted(() => ({ spawnMock: vi.fn() }));
vi.mock('child_process', () => ({ spawn: spawnMock }));

const { WsmMcpClient } = await import('./mcpClient');
const { WsmMcpClient, NO_REQUEST_TIMEOUT } = await import('./mcpClient');

/** A newline-delimited-JSON stand-in for a spawned WSM `mcp` process. Answers
* `initialize` (so connect() resolves) and, by default, nothing else - leaving a
Expand Down Expand Up @@ -52,6 +52,16 @@ function fakeChild() {
return { child, written };
}

/** Tracks settlement without letting an expected rejection escape as unhandled. */
function track<T>(p: Promise<T>) {
const state = { settled: false, rejected: false, error: undefined as unknown };
p.then(
() => { state.settled = true; },
(err) => { state.settled = true; state.rejected = true; state.error = err; },
);
return state;
}

describe('WsmMcpClient request deadlines', () => {
beforeEach(() => {
spawnMock.mockReset();
Expand All @@ -65,22 +75,44 @@ describe('WsmMcpClient request deadlines', () => {
async function connectFake(requestTimeoutMs?: number) {
const { child, written } = fakeChild();
spawnMock.mockReturnValue(child);
return { client: await WsmMcpClient.connect({ exePath: 'C:\\wsm\\fake.exe', requestTimeoutMs }), written };
const client = await WsmMcpClient.connect({ exePath: 'C:\\wsm\\fake.exe', requestTimeoutMs });
return { client, child, written };
}

// The bug this whole change exists for: merge_conflicts used to inherit the 30s
// general-purpose default, and a big load order's merge (or its equally expensive
// dry-run preview) blew straight through it.
it('honors a per-call timeout override instead of the client default', async () => {
// dry-run preview) blew straight through it. A merge now gets no deadline at all.
it('never times out a call made with NO_REQUEST_TIMEOUT, however long it runs', async () => {
const { client } = await connectFake(30_000);

const pending = client.mergeConflicts({ dryRun: true }, 600_000);
const assertion = expect(pending).rejects.toThrow(/timed out after 600000ms/);
const state = track(client.mergeConflicts({ dryRun: true }, NO_REQUEST_TIMEOUT));

// an hour of wall clock - far past the 30s client default
await vi.advanceTimersByTimeAsync(60 * 60 * 1000);

expect(state.settled).toBe(false);
});

// Liveness, not the clock, is what makes an unbounded wait safe: if WSM dies the
// request must still reject promptly rather than hang forever.
it('still rejects an unbounded call when the WSM process exits', async () => {
const { client, child } = await connectFake(30_000);

const pending = client.mergeConflicts({ dryRun: true }, NO_REQUEST_TIMEOUT);
const assertion = expect(pending).rejects.toThrow(/exited unexpectedly \(code 1\)/);

child.emit('exit', 1);

await assertion;
});

it('still rejects an unbounded call when the WSM process fails at the pipe level', async () => {
const { client, child } = await connectFake(30_000);

const pending = client.mergeConflicts({ dryRun: true }, NO_REQUEST_TIMEOUT);
const assertion = expect(pending).rejects.toThrow(/stdin error: broken pipe/);

// Well past the 30s client default - must NOT have rejected yet.
await vi.advanceTimersByTimeAsync(120_000);
// ...and now past the override.
await vi.advanceTimersByTimeAsync(600_000);
child.stdin.emit('error', new Error('broken pipe'));

await assertion;
});
Expand All @@ -96,23 +128,21 @@ describe('WsmMcpClient request deadlines', () => {
await assertion;
});

// The override must stay scoped to the one call. If it leaked onto the client it would
// also bound the initialize handshake, and a WSM process that fails to start would hang
// for the full merge-sized deadline instead of failing fast.
it('does not let a per-call override leak into later calls on the same client', async () => {
// The unbounded wait must stay scoped to the one call. If it leaked onto the client it
// would also cover the initialize handshake, and a WSM process that fails to start would
// hang forever instead of failing fast.
it('does not let an unbounded call stop later calls on the same client from timing out', async () => {
const { client } = await connectFake(30_000);

const long = client.mergeConflicts({ dryRun: true }, 600_000);
const longAssertion = expect(long).rejects.toThrow(/timed out after 600000ms/);
const unbounded = track(client.mergeConflicts({ dryRun: true }, NO_REQUEST_TIMEOUT));

const short = client.getStatus();
const shortAssertion = expect(short).rejects.toThrow(/timed out after 30000ms/);

await vi.advanceTimersByTimeAsync(30_000);
await shortAssertion;

await vi.advanceTimersByTimeAsync(600_000);
await longAssertion;
expect(unbounded.settled).toBe(false);
});

it('uses the client default for the initialize handshake', async () => {
Expand Down
54 changes: 37 additions & 17 deletions vortex-extension/src/mcpClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@ const MCP_PROTOCOL_VERSION = '2025-06-18';

const DEFAULT_REQUEST_TIMEOUT_MS = 30_000;

/**
* Pass as a call's `timeoutMs` to wait indefinitely instead of applying a deadline.
*
* Safe because this client does not rely on a wall-clock deadline to notice a dead
* server: the constructor 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`. What a
* deadline adds on top of that is only the ability to give up on a process that is alive
* and still working - which, for a merge, is precisely the case you must not abandon.
*/
export const NO_REQUEST_TIMEOUT = null;

/** How much of the child process's stderr to retain for diagnostics on failure. */
const STDERR_TAIL_LIMIT = 4000;

Expand Down Expand Up @@ -217,7 +229,10 @@ interface McpToolCallResult {
interface PendingRequest {
resolve: (value: unknown) => void;
reject: (reason: unknown) => void;
timer: ReturnType<typeof setTimeout>;
/** `undefined` for a request made with `NO_REQUEST_TIMEOUT` - there is no timer to
* clear when it settles. `clearTimeout(undefined)` is a no-op, so every call site
* stays unchanged. */
timer: ReturnType<typeof setTimeout> | undefined;
}

export class WsmMcpClient {
Expand Down Expand Up @@ -323,16 +338,17 @@ export class WsmMcpClient {
}

/**
* `timeoutMs` overrides this client's `requestTimeoutMs` for this one call. Use it for a
* tool whose runtime scales with the user's data rather than being bounded by round-trip
* latency - `merge_conflicts` above all (see `resolveAction.ts`'s
* `MERGE_CALL_TIMEOUT_MS`). Overriding per-call, rather than raising the whole client's
* `requestTimeoutMs` at `connect` time, is deliberate: that value also bounds the
* `initialize` handshake, and a handshake that hasn't answered in a few seconds means a
* WSM process that failed to start, not one that's working hard - it should fail fast
* instead of inheriting a merge-sized deadline.
* `timeoutMs` overrides this client's `requestTimeoutMs` for this one call; pass
* `NO_REQUEST_TIMEOUT` (null) to wait indefinitely. Use the latter for a tool whose
* runtime is the user's data rather than a round trip - `merge_conflicts` above all (see
* `resolveAction.ts`'s `MERGE_CALL_TIMEOUT`).
*
* Overriding per-call, rather than changing the whole client's `requestTimeoutMs` at
* `connect` time, is deliberate: that value also bounds the `initialize` handshake, and a
* handshake that hasn't answered in a few seconds means a WSM process that failed to
* start, not one that's working hard - it should keep failing fast.
*/
async callTool<T = unknown>(name: string, args?: Record<string, unknown>, timeoutMs?: number): Promise<T> {
async callTool<T = unknown>(name: string, args?: Record<string, unknown>, timeoutMs?: number | null): Promise<T> {
const result = (await this.request('tools/call', {
name,
arguments: args ?? {},
Expand Down Expand Up @@ -369,7 +385,7 @@ export class WsmMcpClient {
return this.callTool<ScanConflictsResult>('scan_conflicts');
}

mergeConflicts(args?: MergeConflictsArgs, timeoutMs?: number): Promise<MergeConflictsResult> {
mergeConflicts(args?: MergeConflictsArgs, timeoutMs?: number | null): Promise<MergeConflictsResult> {
return this.callTool<MergeConflictsResult>('merge_conflicts', args as Record<string, unknown> | undefined, timeoutMs);
}

Expand Down Expand Up @@ -422,16 +438,20 @@ export class WsmMcpClient {
}
}

private request(method: string, params?: unknown, timeoutMs?: number): Promise<unknown> {
private request(method: string, params?: unknown, timeoutMs?: number | null): Promise<unknown> {
const id = this.nextId++;
const message: JsonRpcRequest = { jsonrpc: '2.0', id, method, params };
const deadlineMs = timeoutMs ?? this.requestTimeoutMs;
// `undefined` means "use the client default"; `NO_REQUEST_TIMEOUT` (null) means
// "no deadline at all" - see its own comment for why that's safe here.
const deadlineMs = timeoutMs === undefined ? this.requestTimeoutMs : timeoutMs;

return new Promise((resolve, reject) => {
const timer = setTimeout(() => {
this.pending.delete(id);
reject(new Error(`WSM MCP request '${method}' timed out after ${deadlineMs}ms`));
}, deadlineMs);
const timer = deadlineMs === NO_REQUEST_TIMEOUT
? undefined
: setTimeout(() => {
this.pending.delete(id);
reject(new Error(`WSM MCP request '${method}' timed out after ${deadlineMs}ms`));
}, deadlineMs);

this.pending.set(id, { resolve, reject, timer });
this.writeMessage(message);
Expand Down
34 changes: 17 additions & 17 deletions vortex-extension/src/resolveAction.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,15 @@ function fakeConnect(outcomes: Array<{ result?: MergeConflictsResult; error?: Er
const calls: WsmMcpClientOptions[] = [];
// Every mergeConflicts invocation, so a test can assert what deadline the call was
// given - see the MERGE_CALL_TIMEOUT_MS tests below.
const mergeCalls: Array<{ args: unknown; timeoutMs: number | undefined }> = [];
const mergeCalls: Array<{ args: unknown; timeoutMs: number | null | undefined }> = [];
let closedCount = 0;
let callIndex = 0;

const connect = vi.fn(async (options: WsmMcpClientOptions): Promise<WsmMergeClient> => {
calls.push(options);
const outcome = outcomes[callIndex++];
return {
mergeConflicts: async (args?: unknown, timeoutMs?: number) => {
mergeConflicts: async (args?: unknown, timeoutMs?: number | null) => {
mergeCalls.push({ args, timeoutMs });
if (outcome.error) {
throw outcome.error;
Expand Down Expand Up @@ -357,23 +357,23 @@ describe('resolveScriptConflicts', () => {
// mcpClient.ts's general-purpose 30s DEFAULT_REQUEST_TIMEOUT_MS, the dry-run preview
// exceeded it, and resolveScriptConflicts bailed at the preview stage - the real merge
// never ran, leaving an unmerged mod0000_MergedFiles and a game that wouldn't start.
// Both calls must carry the long, merge-sized deadline: a preview does the same
// scan-and-merge computation as the real merge and only skips the writes, so sizing it
// as if it were cheap is precisely what broke.
const TEN_MINUTES_MS = 10 * 60 * 1000;

it('gives the dry-run preview a merge-sized timeout, not the general-purpose request default', async () => {
//
// Both calls must now carry NO deadline (null): a merge's runtime is the user's load
// order, so any wall-clock limit can only ever fire on a merge that is working normally.
// A preview is not the cheap one either - it does the same scan-and-merge computation and
// only skips the writes.
it('gives the dry-run preview no deadline, not the general-purpose request default', async () => {
const { api } = fakeApi({ toolPath: 'C:\\wsm\\WitcherScriptMerger.Headless.exe' });
const { connect, mergeCalls } = fakeConnect([{ result: mergeResult() }]);

await resolveScriptConflicts(api, { connect });

expect(mergeCalls).toHaveLength(1);
expect(mergeCalls[0].timeoutMs).toBe(TEN_MINUTES_MS);
expect(mergeCalls[0].timeoutMs).toBeNull();
expect((mergeCalls[0].args as { dryRun?: boolean }).dryRun).toBe(true);
});

it('gives the real merge the same merge-sized timeout as the preview', async () => {
it('gives the real merge no deadline either', async () => {
const { api } = fakeApi({
toolPath: 'C:\\wsm\\WitcherScriptMerger.Headless.exe',
dialogResponses: [{ action: 'Merge Now' }],
Expand All @@ -385,14 +385,14 @@ describe('resolveScriptConflicts', () => {
await resolveScriptConflicts(api, { connect });

expect(mergeCalls).toHaveLength(2);
expect(mergeCalls[0].timeoutMs).toBe(TEN_MINUTES_MS);
expect(mergeCalls[1].timeoutMs).toBe(TEN_MINUTES_MS);
expect(mergeCalls[0].timeoutMs).toBeNull();
expect(mergeCalls[1].timeoutMs).toBeNull();
expect((mergeCalls[1].args as { dryRun?: boolean }).dryRun).toBe(false);
});

// The long deadline belongs to the merge call alone. connect() must NOT be handed it as
// The unbounded wait belongs to the merge call alone. connect() must NOT be handed it as
// the client-wide requestTimeoutMs, which also bounds the initialize handshake - a WSM
// process that fails to start should still fail fast rather than hang for ten minutes.
// process that fails to start must still fail fast rather than hang forever.
it('does not widen the client-wide request timeout (the initialize handshake must still fail fast)', async () => {
const { api } = fakeApi({ toolPath: 'C:\\wsm\\WitcherScriptMerger.Headless.exe' });
const { connect, calls } = fakeConnect([{ result: mergeResult() }]);
Expand All @@ -402,19 +402,19 @@ describe('resolveScriptConflicts', () => {
expect(calls[0].requestTimeoutMs).toBeUndefined();
});

it('explains that nothing was merged when the merge call times out, rather than surfacing a bare transport error', async () => {
it('explains that nothing was merged when a call times out, rather than surfacing a bare transport error', async () => {
const { api, errorNotifications } = fakeApi({
toolPath: 'C:\\wsm\\WitcherScriptMerger.Headless.exe',
});
const timeout = new Error("WSM MCP request 'tools/call' timed out after 600000ms");
const timeout = new Error("WSM MCP request 'initialize' timed out after 30000ms");
const { connect } = fakeConnect([{ error: timeout }]);

await resolveScriptConflicts(api, { connect });

expect(errorNotifications).toHaveLength(1);
const shown = errorNotifications[0].message;
expect(shown).toContain('nothing was merged');
expect(shown).toContain('10 minutes');
expect(shown).toContain('as long as it needs');
// the original error is still handed over as the detail, not swallowed
expect(errorNotifications[0].detail).toBe(timeout);
});
Expand Down
Loading
Loading