diff --git a/CHANGELOG.md b/CHANGELOG.md index ab6fd3efd..3341ef93a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - `merge_merge_request` no longer strips `sha`, which GitLab 19.2+ groups may require when merging ([#665](https://github.com/zereight/gitlab-mcp/issues/665)) - `list_job_artifacts` no longer fails when GitLab returns `mode: null` on artifact tree entries ([#661](https://github.com/zereight/gitlab-mcp/issues/661)) - `create_tag` no longer fails when GitLab omits `release` from the tag response ([#653](https://github.com/zereight/gitlab-mcp/issues/653)) +- `wait_for_pipeline`, `wait_for_job`, `play_pipeline_jobs`, and `rebase_merge_request` now report `Timed out waiting for terminal status` when a status request is still running at `timeout_seconds`, instead of `The operation was aborted due to timeout` ##### Deprecated diff --git a/index.ts b/index.ts index b01fadafd..1c5f09e38 100644 --- a/index.ts +++ b/index.ts @@ -228,6 +228,7 @@ import { import { checkForNewVersion } from "./utils/version-check.js"; import { assertGitLabVersionAtLeast } from "./utils/gitlab-version-gate.js"; import { fetchWithValidatedRedirects } from "./utils/safe-redirect-fetch.js"; +import { fetchStatusBeforeDeadline } from "./utils/poll-deadline.js"; import { parseGitLabVersionApiResponse, type GitLabInstanceVersionMetadata, @@ -9267,11 +9268,10 @@ async function getMergeRequestRebaseStatusBeforeDeadline( mergeRequestIid: string | number, deadline: number ): Promise<{ status: string; mergeRequest: GitLabMergeRequest }> { - const remaining = deadline - Date.now(); - if (remaining <= 0) { - throw new Error("Timed out waiting for terminal status"); - } - return getMergeRequestRebaseStatus(projectId, mergeRequestIid, AbortSignal.timeout(remaining)); + return fetchStatusBeforeDeadline( + signal => getMergeRequestRebaseStatus(projectId, mergeRequestIid, signal), + deadline + ); } /** @@ -9349,11 +9349,7 @@ async function waitForStatus( pollIntervalSeconds = 5 ): Promise { const deadline = Date.now() + timeoutSeconds * 1000; - const fetchBeforeDeadline = async (): Promise => { - const remaining = deadline - Date.now(); - if (remaining <= 0) throw new Error("Timed out waiting for terminal status"); - return fetchStatus(AbortSignal.timeout(remaining)); - }; + const fetchBeforeDeadline = (): Promise => fetchStatusBeforeDeadline(fetchStatus, deadline); let value = await fetchBeforeDeadline(); while (!terminalStatuses.has(value.status)) { const remaining = deadline - Date.now(); diff --git a/test/test-rebase-merge-request.ts b/test/test-rebase-merge-request.ts index a8bf6c66d..55824c9a2 100644 --- a/test/test-rebase-merge-request.ts +++ b/test/test-rebase-merge-request.ts @@ -26,6 +26,7 @@ const MR_FORBIDDEN = "6"; const MR_STALE_THEN_MOVED = "7"; const MR_NOOP = "8"; const MR_RECHECK_TIMEOUT = "9"; +const MR_SLOW_POLL = "10"; function buildMergeRequest(iid: string, overrides: Record = {}) { return { @@ -236,6 +237,7 @@ describe("rebase_merge_request tool", () => { MR_STALE_THEN_MOVED, MR_NOOP, MR_RECHECK_TIMEOUT, + MR_SLOW_POLL, ]) { addRebaseHandler(iid); } @@ -272,6 +274,24 @@ describe("rebase_merge_request tool", () => { { rebase_in_progress: false, sha: NEW_SHA }, ]); + // The head read answers at once; the poll after the rebase answers after the 1s timeout. + const slowPollQueries: Array> = []; + statusQueries.set(MR_SLOW_POLL, slowPollQueries); + mockGitLab.addMockHandler( + "get", + `/projects/${TEST_PROJECT_ID}/merge_requests/${MR_SLOW_POLL}`, + (req, res) => { + slowPollQueries.push({ ...req.query }); + if (slowPollQueries.length === 1) { + res.json(buildMergeRequest(MR_SLOW_POLL, { rebase_in_progress: false })); + return; + } + setTimeout(() => { + res.json(buildMergeRequest(MR_SLOW_POLL, { rebase_in_progress: true })); + }, 1500); + } + ); + mockGitLab.addMockHandler( "put", `/projects/${TEST_PROJECT_ID}/merge_requests/${MR_FORBIDDEN}/rebase`, @@ -365,6 +385,24 @@ describe("rebase_merge_request tool", () => { ); }); + test("rebase_merge_request reports a timeout when a poll is still in flight at the deadline", async () => { + await assert.rejects( + () => + callTool( + "rebase_merge_request", + { + project_id: TEST_PROJECT_ID, + merge_request_iid: MR_SLOW_POLL, + timeout_seconds: 1, + poll_interval_seconds: 1, + }, + env() + ), + /Timed out waiting for terminal status/ + ); + assert.strictEqual(statusQueries.get(MR_SLOW_POLL)?.length, 2); + }); + test("rebase_merge_request keeps waiting when the first finished poll still has the old sha", async () => { const result = await callTool( "rebase_merge_request", diff --git a/test/utils/poll-deadline.test.ts b/test/utils/poll-deadline.test.ts new file mode 100644 index 000000000..d68780d23 --- /dev/null +++ b/test/utils/poll-deadline.test.ts @@ -0,0 +1,118 @@ +import assert from "node:assert/strict"; +import { createServer } from "node:http"; +import type { AddressInfo } from "node:net"; +import { describe, test } from "node:test"; +import { fetchStatusBeforeDeadline, POLL_TIMEOUT_MESSAGE } from "../../utils/poll-deadline.js"; + +// AbortSignal.timeout does not keep the process alive, so hold it open until the abort. +function waitForAbort(signal: AbortSignal): Promise { + return new Promise(resolve => { + if (signal.aborted) { + resolve(); + return; + } + const keepAlive = setInterval(() => {}, 1000); + signal.addEventListener( + "abort", + () => { + clearInterval(keepAlive); + resolve(); + }, + { once: true } + ); + }); +} + +async function rejectAfterAbort(signal: AbortSignal, error: () => unknown): Promise { + await waitForAbort(signal); + throw error(); +} + +describe("When fetchStatusBeforeDeadline runs", () => { + test("should return the fetched value before the deadline", async () => { + const value = await fetchStatusBeforeDeadline(async () => "finished", Date.now() + 1000); + + assert.equal(value, "finished"); + }); + + test("should time out without fetching once the deadline has passed", async () => { + let fetched = false; + + await assert.rejects( + fetchStatusBeforeDeadline(async () => { + fetched = true; + return "finished"; + }, Date.now() - 1), + { message: POLL_TIMEOUT_MESSAGE } + ); + assert.equal(fetched, false); + }); + + test("should report an abort by the deadline signal as a timeout", async () => { + let reason: unknown; + + await assert.rejects( + fetchStatusBeforeDeadline( + signal => + rejectAfterAbort(signal, () => { + reason = signal.reason; + return signal.reason; + }), + Date.now() + 20 + ), + (error: Error) => { + assert.equal(error.message, POLL_TIMEOUT_MESSAGE); + assert.equal(error.cause, reason); + return true; + } + ); + }); + + test("should report a body read that starts after the deadline as a timeout", async () => { + const server = createServer((_req, res) => { + res.writeHead(200, { "content-type": "application/json" }); + res.end('{"status":"running"}'); + }); + await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as AddressInfo; + + try { + await assert.rejects( + fetchStatusBeforeDeadline(async signal => { + const response = await fetch(`http://127.0.0.1:${port}/`, { signal }); + await waitForAbort(signal); + return response.json(); + }, Date.now() + 1000), + (error: Error) => { + assert.equal(error.message, POLL_TIMEOUT_MESSAGE); + // An abort of the request itself would leave the TimeoutError reason here. + assert.equal((error.cause as Error).name, "AbortError"); + return true; + } + ); + } finally { + server.closeAllConnections(); + await new Promise(resolve => server.close(resolve)); + } + }); + + test("should keep another error that lands after the deadline", async () => { + const failure = new Error("GitLab API error: 500 Internal Server Error"); + + await assert.rejects( + fetchStatusBeforeDeadline(signal => rejectAfterAbort(signal, () => failure), Date.now() + 20), + error => error === failure + ); + }); + + test("should keep an error that lands before the deadline", async () => { + const failure = new Error("GitLab API error: 404 Not Found"); + + await assert.rejects( + fetchStatusBeforeDeadline(async () => { + throw failure; + }, Date.now() + 1000), + error => error === failure + ); + }); +}); diff --git a/utils/poll-deadline.ts b/utils/poll-deadline.ts new file mode 100644 index 000000000..419fb12d9 --- /dev/null +++ b/utils/poll-deadline.ts @@ -0,0 +1,31 @@ +export const POLL_TIMEOUT_MESSAGE = "Timed out waiting for terminal status"; + +function isAbortError(error: unknown): boolean { + return error instanceof Error && error.name === "AbortError"; +} + +/** + * Fetch one poll result inside the deadline. When the deadline signal aborts the + * request, the wait has timed out, so report that instead of the abort. Any other + * rejection passes through unchanged, even one that lands after the deadline. + */ +export async function fetchStatusBeforeDeadline( + fetchStatus: (signal: AbortSignal) => Promise, + deadline: number +): Promise { + const remaining = deadline - Date.now(); + if (remaining <= 0) { + throw new Error(POLL_TIMEOUT_MESSAGE); + } + const signal = AbortSignal.timeout(remaining); + try { + return await fetchStatus(signal); + } catch (error) { + // fetch rejects with signal.reason itself when this signal aborts it. A body read + // that starts after the abort throws a new AbortError instead. + if (signal.aborted && (error === signal.reason || isAbortError(error))) { + throw new Error(POLL_TIMEOUT_MESSAGE, { cause: error }); + } + throw error; + } +}