FN-5993: prevent PR metadata generation hangs
Bound PR AI metadata generation so the PR dialog falls back cleanly instead of hanging indefinitely. - add a 60s timeout and abort-aware fallback path to dashboard PR metadata generation - forward request abort signals from the PR metadata route and avoid writing responses after disconnects - cover timeout/abort behavior in PR metadata generator and route tests - document the bounded fallback behavior in the dashboard guide Files changed: docs/dashboard-guide.md | 1 + .../src/__tests__/pr-metadata-generator.test.ts | 93 +++++++++- ...it-github.pr-options-preflight-metadata.test.ts | 8 +- packages/dashboard/src/pr-metadata-generator.ts | 199 ++++++++++++++------- .../dashboard/src/routes/register-git-github.ts | 10 ++ 5 files changed, 235 insertions(+), 76 deletions(-) Fusion-Task-Id: FN-5993 Fusion-Task-Lineage: fc49c15a-2359-4d14-b0ae-fed4aecde2fb
This commit is contained in:
@@ -4,21 +4,26 @@ import { join } from "node:path";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import type { Task } from "@fusion/core";
|
||||
|
||||
const { execMock, promptMock } = vi.hoisted(() => ({
|
||||
const { execMock, promptMock, disposeMock } = vi.hoisted(() => ({
|
||||
execMock: vi.fn(),
|
||||
promptMock: vi.fn(),
|
||||
disposeMock: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("node:child_process", () => ({
|
||||
exec: execMock,
|
||||
}));
|
||||
vi.mock("node:child_process", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("node:child_process")>();
|
||||
return {
|
||||
...actual,
|
||||
exec: execMock,
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("@fusion/engine", () => ({
|
||||
listCliAdapterDescriptors: () => [],
|
||||
createFnAgent: vi.fn(async () => ({
|
||||
session: {
|
||||
prompt: promptMock,
|
||||
dispose: vi.fn(),
|
||||
dispose: disposeMock,
|
||||
},
|
||||
})),
|
||||
}));
|
||||
@@ -59,6 +64,8 @@ describe("generatePrMetadata", () => {
|
||||
repoRoot = mkdtempSync(join(tmpdir(), "pr-metadata-"));
|
||||
mkdirSync(join(repoRoot, ".fusion", "tasks", "FN-4991"), { recursive: true });
|
||||
writeFileSync(join(repoRoot, ".fusion", "tasks", "FN-4991", "PROMPT.md"), "# Prompt");
|
||||
promptMock.mockResolvedValue(undefined);
|
||||
disposeMock.mockReset();
|
||||
vi.mocked(createFnAgent).mockImplementation(async ({ onText }: { onText?: (t: string) => void }) => {
|
||||
onText?.(JSON.stringify({
|
||||
title: "feat: add routes",
|
||||
@@ -70,7 +77,7 @@ describe("generatePrMetadata", () => {
|
||||
return {
|
||||
session: {
|
||||
prompt: promptMock,
|
||||
dispose: vi.fn(),
|
||||
dispose: disposeMock,
|
||||
},
|
||||
} as never;
|
||||
});
|
||||
@@ -82,6 +89,7 @@ describe("generatePrMetadata", () => {
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
@@ -98,6 +106,7 @@ describe("generatePrMetadata", () => {
|
||||
expect(result.body).toContain("## Testing");
|
||||
expect(result.body).toContain("## Linked Task");
|
||||
expect(result.templateUsed).toBe(false);
|
||||
expect(promptMock).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ signal: expect.any(AbortSignal) }));
|
||||
});
|
||||
|
||||
it("fills known sections when template exists and preserves unknown headings", async () => {
|
||||
@@ -125,7 +134,7 @@ describe("generatePrMetadata", () => {
|
||||
return {
|
||||
session: {
|
||||
prompt: promptMock,
|
||||
dispose: vi.fn(),
|
||||
dispose: disposeMock,
|
||||
},
|
||||
} as never;
|
||||
});
|
||||
@@ -142,4 +151,74 @@ describe("generatePrMetadata", () => {
|
||||
templateUsed: false,
|
||||
});
|
||||
});
|
||||
|
||||
it("returns fallback when prompt times out", async () => {
|
||||
vi.useFakeTimers();
|
||||
promptMock.mockImplementation(() => new Promise(() => undefined));
|
||||
|
||||
const resultPromise = generatePrMetadata({
|
||||
task: createTask(),
|
||||
repoRoot,
|
||||
settings: {} as never,
|
||||
timeoutMs: 25,
|
||||
});
|
||||
|
||||
await vi.advanceTimersByTimeAsync(25);
|
||||
|
||||
await expect(resultPromise).resolves.toEqual({
|
||||
title: "Route contracts",
|
||||
body: expect.stringContaining("Closes FN-4991"),
|
||||
templateUsed: false,
|
||||
});
|
||||
expect(disposeMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("returns fallback immediately when caller signal is already aborted", async () => {
|
||||
const controller = new AbortController();
|
||||
controller.abort();
|
||||
|
||||
const result = await generatePrMetadata({
|
||||
task: createTask(),
|
||||
repoRoot,
|
||||
settings: {} as never,
|
||||
signal: controller.signal,
|
||||
});
|
||||
|
||||
expect(result).toEqual({
|
||||
title: "Route contracts",
|
||||
body: expect.stringContaining("Closes FN-4991"),
|
||||
templateUsed: false,
|
||||
});
|
||||
expect(createFnAgent).not.toHaveBeenCalled();
|
||||
expect(promptMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns fallback and disposes the session when caller aborts mid-generation", async () => {
|
||||
const controller = new AbortController();
|
||||
let promptStarted!: () => void;
|
||||
const promptStartedPromise = new Promise<void>((resolve) => {
|
||||
promptStarted = resolve;
|
||||
});
|
||||
promptMock.mockImplementation(() => new Promise(() => {
|
||||
promptStarted();
|
||||
}));
|
||||
|
||||
const resultPromise = generatePrMetadata({
|
||||
task: createTask(),
|
||||
repoRoot,
|
||||
settings: {} as never,
|
||||
signal: controller.signal,
|
||||
});
|
||||
|
||||
await promptStartedPromise;
|
||||
controller.abort();
|
||||
|
||||
await expect(resultPromise).resolves.toEqual({
|
||||
title: "Route contracts",
|
||||
body: expect.stringContaining("Closes FN-4991"),
|
||||
templateUsed: false,
|
||||
});
|
||||
expect(promptMock).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ signal: expect.any(AbortSignal) }));
|
||||
expect(disposeMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -134,13 +134,17 @@ describe("PR metadata/preflight/options routes", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("POST /pr/generate-metadata returns generated metadata", async () => {
|
||||
it("POST /pr/generate-metadata returns generated metadata and forwards an abort signal", async () => {
|
||||
const app = createServer(createStore(createTask()));
|
||||
const response = await performRequest(app, "POST", "/api/tasks/FN-001/pr/generate-metadata", "{}", { "content-type": "application/json" });
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.body).toEqual({ title: "Generated title", body: "Generated body", templateUsed: true });
|
||||
expect(mockGeneratePrMetadata).toHaveBeenCalledWith(expect.objectContaining({ task: expect.objectContaining({ id: "FN-001" }), repoRoot: "/tmp/project" }));
|
||||
expect(mockGeneratePrMetadata).toHaveBeenCalledWith(expect.objectContaining({
|
||||
task: expect.objectContaining({ id: "FN-001" }),
|
||||
repoRoot: "/tmp/project",
|
||||
signal: expect.any(AbortSignal),
|
||||
}));
|
||||
});
|
||||
|
||||
it("POST /pr/generate-metadata returns 404 for missing task", async () => {
|
||||
|
||||
@@ -7,6 +7,7 @@ import { resolveTaskPlanningModel } from "@fusion/core";
|
||||
import { createFnAgent } from "@fusion/engine";
|
||||
|
||||
const execAsync = promisify(execCb);
|
||||
const PR_METADATA_TIMEOUT_MS = 60_000;
|
||||
|
||||
export interface GeneratedPrMetadata {
|
||||
title: string;
|
||||
@@ -134,22 +135,58 @@ function fillTemplate(template: string, result: AiMetadataResult, taskId: string
|
||||
return out.join("\n");
|
||||
}
|
||||
|
||||
async function runCommand(command: string, cwd: string): Promise<string> {
|
||||
async function runCommand(command: string, cwd: string, signal?: AbortSignal): Promise<string> {
|
||||
const { stdout } = await execAsync(command, {
|
||||
cwd,
|
||||
timeout: 15_000,
|
||||
maxBuffer: 10 * 1024 * 1024,
|
||||
signal,
|
||||
});
|
||||
return stdout.trim();
|
||||
}
|
||||
|
||||
async function resolveBaseBranch(task: Task, repoRoot: string): Promise<string> {
|
||||
function createAbortError(): Error {
|
||||
const error = new Error("PR metadata generation aborted");
|
||||
error.name = "AbortError";
|
||||
return error;
|
||||
}
|
||||
|
||||
function throwIfAborted(signal?: AbortSignal): void {
|
||||
if (!signal?.aborted) {
|
||||
return;
|
||||
}
|
||||
throw signal.reason instanceof Error ? signal.reason : createAbortError();
|
||||
}
|
||||
|
||||
function waitForAbort(signal: AbortSignal): Promise<never> {
|
||||
return new Promise((_, reject) => {
|
||||
if (signal.aborted) {
|
||||
reject(signal.reason instanceof Error ? signal.reason : createAbortError());
|
||||
return;
|
||||
}
|
||||
signal.addEventListener(
|
||||
"abort",
|
||||
() => reject(signal.reason instanceof Error ? signal.reason : createAbortError()),
|
||||
{ once: true },
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
function isAbortLikeError(error: unknown): boolean {
|
||||
return Boolean(
|
||||
error
|
||||
&& typeof error === "object"
|
||||
&& ("name" in error ? (error as { name?: unknown }).name === "AbortError" : false),
|
||||
);
|
||||
}
|
||||
|
||||
async function resolveBaseBranch(task: Task, repoRoot: string, signal?: AbortSignal): Promise<string> {
|
||||
if (task.prInfo?.baseBranch) {
|
||||
return task.prInfo.baseBranch;
|
||||
}
|
||||
|
||||
try {
|
||||
const stdout = await runCommand("gh repo view --json defaultBranchRef -q .defaultBranchRef.name", repoRoot);
|
||||
const stdout = await runCommand("gh repo view --json defaultBranchRef -q .defaultBranchRef.name", repoRoot, signal);
|
||||
if (stdout) return stdout;
|
||||
} catch {
|
||||
// fallback below
|
||||
@@ -162,81 +199,109 @@ export async function generatePrMetadata(input: {
|
||||
repoRoot: string;
|
||||
settings: ProjectSettings & GlobalSettings;
|
||||
signal?: AbortSignal;
|
||||
timeoutMs?: number;
|
||||
}): Promise<GeneratedPrMetadata> {
|
||||
const { task, repoRoot, settings, signal } = input;
|
||||
const { task, repoRoot, settings, signal, timeoutMs = PR_METADATA_TIMEOUT_MS } = input;
|
||||
const fallback = buildFallback(task);
|
||||
const controller = new AbortController();
|
||||
const abortFromCaller = () => controller.abort(signal?.reason instanceof Error ? signal.reason : createAbortError());
|
||||
const timeoutId = setTimeout(() => controller.abort(createAbortError()), timeoutMs);
|
||||
const combinedSignal = controller.signal;
|
||||
const callerSignalWasActive = Boolean(signal && !signal.aborted);
|
||||
|
||||
const baseBranch = await resolveBaseBranch(task, repoRoot);
|
||||
const [logOut, diffStatOut] = await Promise.all([
|
||||
runCommand(`git log --no-merges ${baseBranch}..HEAD --format=%s%n%b`, repoRoot).catch(() => ""),
|
||||
runCommand(`git diff --stat ${baseBranch}..HEAD`, repoRoot).catch(() => ""),
|
||||
]);
|
||||
|
||||
let promptContent = "";
|
||||
try {
|
||||
const promptPath = join(repoRoot, ".fusion", "tasks", task.id, "PROMPT.md");
|
||||
promptContent = (await readFile(promptPath, "utf8")).trim();
|
||||
} catch {
|
||||
promptContent = "";
|
||||
if (signal) {
|
||||
if (signal.aborted) {
|
||||
abortFromCaller();
|
||||
} else {
|
||||
signal.addEventListener("abort", abortFromCaller, { once: true });
|
||||
}
|
||||
}
|
||||
|
||||
const templatePath = join(repoRoot, ".github", "pull_request_template.md");
|
||||
const templateExists = await access(templatePath).then(() => true).catch(() => false);
|
||||
const template = templateExists ? await readFile(templatePath, "utf8") : "";
|
||||
|
||||
const model = resolveTaskPlanningModel(task, settings as Partial<Settings>);
|
||||
let aiText = "";
|
||||
const { session } = await createFnAgent({
|
||||
cwd: repoRoot,
|
||||
tools: "readonly",
|
||||
defaultProvider: model.provider,
|
||||
defaultModelId: model.modelId,
|
||||
systemPrompt: [
|
||||
"Generate GitHub PR metadata.",
|
||||
"Respond with strict JSON only.",
|
||||
"Schema: {title, summary, changes, testing, linkedTask}",
|
||||
].join("\n"),
|
||||
onText: (delta: string) => {
|
||||
aiText += delta;
|
||||
},
|
||||
});
|
||||
|
||||
try {
|
||||
const contextPrompt = [
|
||||
`Task ID: ${task.id}`,
|
||||
`Task title: ${task.title}`,
|
||||
`Task description: ${task.description ?? ""}`,
|
||||
`Base branch: ${baseBranch}`,
|
||||
"Commit log:",
|
||||
logOut || "(none)",
|
||||
"Diff stat:",
|
||||
diffStatOut || "(none)",
|
||||
"Task prompt:",
|
||||
promptContent || "(none)",
|
||||
].join("\n\n");
|
||||
throwIfAborted(combinedSignal);
|
||||
|
||||
if (signal?.aborted) {
|
||||
throw new Error("Metadata generation aborted");
|
||||
}
|
||||
const baseBranch = await resolveBaseBranch(task, repoRoot, combinedSignal);
|
||||
const [logOut, diffStatOut] = await Promise.all([
|
||||
runCommand(`git log --no-merges ${baseBranch}..HEAD --format=%s%n%b`, repoRoot, combinedSignal).catch(() => ""),
|
||||
runCommand(`git diff --stat ${baseBranch}..HEAD`, repoRoot, combinedSignal).catch(() => ""),
|
||||
]);
|
||||
|
||||
await session.prompt(contextPrompt);
|
||||
} finally {
|
||||
let promptContent = "";
|
||||
try {
|
||||
session.dispose();
|
||||
const promptPath = join(repoRoot, ".fusion", "tasks", task.id, "PROMPT.md");
|
||||
promptContent = (await readFile(promptPath, "utf8")).trim();
|
||||
} catch {
|
||||
// best effort
|
||||
promptContent = "";
|
||||
}
|
||||
|
||||
const templatePath = join(repoRoot, ".github", "pull_request_template.md");
|
||||
const templateExists = await access(templatePath).then(() => true).catch(() => false);
|
||||
const template = templateExists ? await readFile(templatePath, "utf8") : "";
|
||||
|
||||
const model = resolveTaskPlanningModel(task, settings as Partial<Settings>);
|
||||
let aiText = "";
|
||||
const { session } = await createFnAgent({
|
||||
cwd: repoRoot,
|
||||
tools: "readonly",
|
||||
defaultProvider: model.provider,
|
||||
defaultModelId: model.modelId,
|
||||
systemPrompt: [
|
||||
"Generate GitHub PR metadata.",
|
||||
"Respond with strict JSON only.",
|
||||
"Schema: {title, summary, changes, testing, linkedTask}",
|
||||
].join("\n"),
|
||||
onText: (delta: string) => {
|
||||
aiText += delta;
|
||||
},
|
||||
});
|
||||
|
||||
try {
|
||||
const contextPrompt = [
|
||||
`Task ID: ${task.id}`,
|
||||
`Task title: ${task.title}`,
|
||||
`Task description: ${task.description ?? ""}`,
|
||||
`Base branch: ${baseBranch}`,
|
||||
"Commit log:",
|
||||
logOut || "(none)",
|
||||
"Diff stat:",
|
||||
diffStatOut || "(none)",
|
||||
"Task prompt:",
|
||||
promptContent || "(none)",
|
||||
].join("\n\n");
|
||||
|
||||
throwIfAborted(combinedSignal);
|
||||
await Promise.race([
|
||||
(session.prompt as (prompt: string, options?: { signal?: AbortSignal }) => Promise<unknown>)(contextPrompt, { signal: combinedSignal }),
|
||||
waitForAbort(combinedSignal),
|
||||
]);
|
||||
} finally {
|
||||
try {
|
||||
session.dispose();
|
||||
} catch {
|
||||
// best effort
|
||||
}
|
||||
}
|
||||
|
||||
const parsed = parseAiResult(aiText);
|
||||
if (!parsed) {
|
||||
return fallback;
|
||||
}
|
||||
|
||||
const body = templateExists ? fillTemplate(template, parsed, task.id) : buildBody(parsed, task.id);
|
||||
return {
|
||||
title: parsed.title,
|
||||
body,
|
||||
templateUsed: templateExists,
|
||||
};
|
||||
} catch (error) {
|
||||
if (isAbortLikeError(error)) {
|
||||
return fallback;
|
||||
}
|
||||
throw error;
|
||||
} finally {
|
||||
clearTimeout(timeoutId);
|
||||
if (callerSignalWasActive) {
|
||||
signal?.removeEventListener("abort", abortFromCaller);
|
||||
}
|
||||
}
|
||||
|
||||
const parsed = parseAiResult(aiText);
|
||||
if (!parsed) {
|
||||
return fallback;
|
||||
}
|
||||
|
||||
const body = templateExists ? fillTemplate(template, parsed, task.id) : buildBody(parsed, task.id);
|
||||
return {
|
||||
title: parsed.title,
|
||||
body,
|
||||
templateUsed: templateExists,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -4849,6 +4849,10 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void {
|
||||
* Returns: { title, body, templateUsed }
|
||||
*/
|
||||
router.post("/tasks/:id/pr/generate-metadata", async (req, res) => {
|
||||
const controller = new AbortController();
|
||||
const abortRequest = () => controller.abort();
|
||||
req.on("close", abortRequest);
|
||||
|
||||
try {
|
||||
const { store: scopedStore } = await getProjectContext(req);
|
||||
const task = await scopedStore.getTask(req.params.id);
|
||||
@@ -4857,7 +4861,11 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void {
|
||||
task,
|
||||
repoRoot: scopedStore.getRootDir(),
|
||||
settings,
|
||||
signal: controller.signal,
|
||||
});
|
||||
if (req.destroyed || res.writableEnded || res.writableFinished || controller.signal.aborted) {
|
||||
return;
|
||||
}
|
||||
res.json(metadata);
|
||||
} catch (err: unknown) {
|
||||
if (err instanceof ApiError) {
|
||||
@@ -4867,6 +4875,8 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void {
|
||||
throw notFound(`Task ${req.params.id} not found`);
|
||||
}
|
||||
rethrowAsApiError(err, "Failed to generate PR metadata");
|
||||
} finally {
|
||||
req.off("close", abortRequest);
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user