feat(FN-5245): swallow deleted-task races in GitHub tracking services
Fixes a race condition where task deletion could race against move-related GitHub tracking logs. Both `GitHubTrackingCommentService` and `GitHubTrackingStateService.handleTaskMoved` are hardened to gracefully swallow deleted-task races, with a regression test covering the delete-after-move scenario. Fusion-Task-Id: FN-5245
This commit is contained in:
committed by
gsxdsm
parent
1b49cbc94d
commit
646e546ef5
@@ -7,9 +7,10 @@ import { TaskStore } from "@fusion/core";
|
||||
import { GitHubTrackingCommentService } from "../github-tracking-comments.js";
|
||||
import { GitHubTrackingStateService } from "../github-tracking-state.js";
|
||||
|
||||
const { mockCommentOnIssue, mockSetIssueState, mockResolveGithubTrackingAuth } = vi.hoisted(() => ({
|
||||
const { mockCommentOnIssue, mockSetIssueState, mockGetIssue, mockResolveGithubTrackingAuth } = vi.hoisted(() => ({
|
||||
mockCommentOnIssue: vi.fn(),
|
||||
mockSetIssueState: vi.fn(),
|
||||
mockGetIssue: vi.fn(),
|
||||
mockResolveGithubTrackingAuth: vi.fn(),
|
||||
}));
|
||||
|
||||
@@ -17,6 +18,7 @@ vi.mock("../github.js", () => ({
|
||||
GitHubClient: vi.fn().mockImplementation(() => ({
|
||||
commentOnIssue: (...args: unknown[]) => mockCommentOnIssue(...args),
|
||||
setIssueState: (...args: unknown[]) => mockSetIssueState(...args),
|
||||
getIssue: (...args: unknown[]) => mockGetIssue(...args),
|
||||
})),
|
||||
}));
|
||||
|
||||
@@ -42,6 +44,7 @@ describe("github tracking unlink flow", () => {
|
||||
beforeEach(async () => {
|
||||
vi.clearAllMocks();
|
||||
mockResolveGithubTrackingAuth.mockReturnValue({ ok: true, auth: { mode: "token", token: "token" } });
|
||||
mockGetIssue.mockResolvedValue({ state: "open" });
|
||||
rootDir = makeTmpDir();
|
||||
globalDir = makeTmpDir();
|
||||
store = new TaskStore(rootDir, globalDir, { inMemoryDb: true });
|
||||
@@ -90,4 +93,33 @@ describe("github tracking unlink flow", () => {
|
||||
// emission is restored.
|
||||
// Replaced with stub: original assertions deferred (see git history). Restore once underlying feature/bug work lands.
|
||||
it("stops all status-sync calls after unlink and does not mutate remote issue during unlink", async () => { expect(true).toBe(true); });
|
||||
|
||||
it("swallows move-after-delete log writes for tracked tasks", async () => {
|
||||
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const task = await store.createTask({
|
||||
description: "unlink delete",
|
||||
githubTracking: { enabled: true },
|
||||
});
|
||||
|
||||
await store.linkGithubIssue(task.id, {
|
||||
owner: "octocat",
|
||||
repo: "hello-world",
|
||||
number: 10,
|
||||
url: "https://github.com/octocat/hello-world/issues/10",
|
||||
createdAt: new Date().toISOString(),
|
||||
});
|
||||
|
||||
await store.moveTask(task.id, "todo");
|
||||
await store.moveTask(task.id, "in-progress");
|
||||
await store.moveTask(task.id, "done");
|
||||
await store.deleteTask(task.id);
|
||||
await flushAsync();
|
||||
|
||||
expect(mockSetIssueState).toHaveBeenCalledWith("octocat", "hello-world", 10, "closed", "completed");
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
`[github-tracking-state] Unable to write log entry for deleted task ${task.id}: Closed linked GitHub tracking issue`,
|
||||
);
|
||||
|
||||
warnSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -191,6 +191,19 @@ export class GitHubTrackingCommentService {
|
||||
this.store.off("task:moved", this.onTaskMoved);
|
||||
}
|
||||
|
||||
private async safeLogDeletedTaskEntry(taskId: string, message: string, details: string): Promise<void> {
|
||||
try {
|
||||
await this.store.logEntry(taskId, message, details);
|
||||
} catch (error) {
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
if (errorMessage.includes(`Task ${taskId} not found`)) {
|
||||
console.warn(`[github-tracking-comments] Unable to write log entry for deleted task ${taskId}: ${message}`);
|
||||
return;
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
private async handleTaskMoved(event: TaskMovedEvent): Promise<void> {
|
||||
if (event.from === event.to) {
|
||||
return;
|
||||
@@ -211,7 +224,7 @@ export class GitHubTrackingCommentService {
|
||||
|
||||
const { owner, repo, number } = issue;
|
||||
if (!owner || !repo || !number) {
|
||||
await this.store.logEntry(
|
||||
await this.safeLogDeletedTaskEntry(
|
||||
event.task.id,
|
||||
"Failed to post GitHub tracking comment",
|
||||
"Linked issue metadata is incomplete",
|
||||
@@ -228,7 +241,7 @@ export class GitHubTrackingCommentService {
|
||||
const globalSettings = (await this.store.getGlobalSettingsStore?.()?.getSettings?.() ?? {}) as Pick<GlobalSettings, never>;
|
||||
const resolution = resolveGithubTrackingAuth({ projectSettings, globalSettings });
|
||||
if (!resolution.ok) {
|
||||
await this.store.logEntry(event.task.id, "Skipped GitHub tracking comment", resolution.message);
|
||||
await this.safeLogDeletedTaskEntry(event.task.id, "Skipped GitHub tracking comment", resolution.message);
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -236,14 +249,14 @@ export class GitHubTrackingCommentService {
|
||||
? new GitHubClient({ token: resolution.auth.token, forceMode: "token" })
|
||||
: new GitHubClient({ forceMode: "gh-cli" });
|
||||
await client.commentOnIssue(owner, repo, number, body);
|
||||
await this.store.logEntry(
|
||||
await this.safeLogDeletedTaskEntry(
|
||||
event.task.id,
|
||||
"Posted GitHub tracking comment",
|
||||
`${owner}/${repo}#${number} (${event.to})`,
|
||||
);
|
||||
} catch (err) {
|
||||
const message = err instanceof Error ? err.message : String(err);
|
||||
await this.store.logEntry(
|
||||
await this.safeLogDeletedTaskEntry(
|
||||
event.task.id,
|
||||
"Failed to post GitHub tracking comment",
|
||||
message,
|
||||
|
||||
@@ -140,7 +140,8 @@ export class GitHubTrackingStateService {
|
||||
|
||||
const { owner, repo, number } = issue;
|
||||
if (!owner || !repo || !number) {
|
||||
await store.logEntry(
|
||||
await this.safeLogDeletedTaskEntry(
|
||||
store,
|
||||
event.task.id,
|
||||
"Failed to update GitHub tracking issue state",
|
||||
"Linked issue metadata is incomplete",
|
||||
@@ -153,7 +154,7 @@ export class GitHubTrackingStateService {
|
||||
const globalSettings = (await store.getGlobalSettingsStore?.()?.getSettings?.() ?? {}) as Pick<GlobalSettings, never>;
|
||||
const resolution = resolveGithubTrackingAuth({ projectSettings, globalSettings });
|
||||
if (!resolution.ok) {
|
||||
await store.logEntry(event.task.id, "Skipped GitHub tracking issue state update", resolution.message);
|
||||
await this.safeLogDeletedTaskEntry(store, event.task.id, "Skipped GitHub tracking issue state update", resolution.message);
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -164,7 +165,7 @@ export class GitHubTrackingStateService {
|
||||
if (decision.action === "close") {
|
||||
const existing = await client.getIssue(owner, repo, number);
|
||||
if (existing?.state === "closed") {
|
||||
await store.logEntry(event.task.id, "Linked GitHub tracking issue already closed", `${owner}/${repo}#${number}`);
|
||||
await this.safeLogDeletedTaskEntry(store, event.task.id, "Linked GitHub tracking issue already closed", `${owner}/${repo}#${number}`);
|
||||
return;
|
||||
}
|
||||
}
|
||||
@@ -189,7 +190,8 @@ export class GitHubTrackingStateService {
|
||||
await updateIssueState();
|
||||
}
|
||||
|
||||
await store.logEntry(
|
||||
await this.safeLogDeletedTaskEntry(
|
||||
store,
|
||||
event.task.id,
|
||||
decision.action === "close"
|
||||
? "Closed linked GitHub tracking issue"
|
||||
@@ -197,7 +199,8 @@ export class GitHubTrackingStateService {
|
||||
`${owner}/${repo}#${number}`,
|
||||
);
|
||||
} catch (err) {
|
||||
await store.logEntry(
|
||||
await this.safeLogDeletedTaskEntry(
|
||||
store,
|
||||
event.task.id,
|
||||
decision.action === "close"
|
||||
? "Failed to close GitHub tracking issue"
|
||||
|
||||
Reference in New Issue
Block a user