From 93742e7de124995a7d4006d69b476f582aff563a Mon Sep 17 00:00:00 2001 From: Dotta Date: Thu, 23 Apr 2026 10:04:57 -0500 Subject: [PATCH] Address subtree hold review findings --- packages/db/src/migrations/meta/_journal.json | 2 +- .../issue-tree-control-routes.test.ts | 44 +++++++++++++++++++ server/src/routes/issue-tree-control.ts | 20 +++++++-- server/src/services/heartbeat.ts | 13 +++--- server/src/services/issue-tree-control.ts | 10 ++++- server/src/services/issues.ts | 9 +--- 6 files changed, 77 insertions(+), 21 deletions(-) diff --git a/packages/db/src/migrations/meta/_journal.json b/packages/db/src/migrations/meta/_journal.json index 3fbd9e30ea0..2262a428bec 100644 --- a/packages/db/src/migrations/meta/_journal.json +++ b/packages/db/src/migrations/meta/_journal.json @@ -467,7 +467,7 @@ { "idx": 66, "version": "7", - "when": 1776786127000, + "when": 1776903901000, "tag": "0066_issue_tree_holds", "breakpoints": true } diff --git a/server/src/__tests__/issue-tree-control-routes.test.ts b/server/src/__tests__/issue-tree-control-routes.test.ts index a95635fb864..efdf6b9b17a 100644 --- a/server/src/__tests__/issue-tree-control-routes.test.ts +++ b/server/src/__tests__/issue-tree-control-routes.test.ts @@ -256,4 +256,48 @@ describe("issue tree control routes", () => { ); expect(res.body.hold.status).toBe("released"); }); + + it("releases a restore hold if the restore application fails", async () => { + const app = await createApp({ + type: "board", + userId: "user-1", + companyIds: ["company-2"], + source: "session", + isInstanceAdmin: false, + }); + mockTreeControlService.createHold.mockResolvedValue({ + hold: { + id: "66666666-6666-4666-8666-666666666666", + mode: "restore", + reason: "restore subtree", + }, + preview: { + mode: "restore", + totals: { affectedIssues: 1 }, + warnings: [], + activeRuns: [], + }, + }); + mockTreeControlService.restoreIssueStatusesForHold.mockRejectedValue(new Error("restore failed")); + mockTreeControlService.releaseHold.mockResolvedValue({ + id: "66666666-6666-4666-8666-666666666666", + mode: "restore", + status: "released", + }); + + const res = await request(app) + .post("/api/issues/11111111-1111-4111-8111-111111111111/tree-holds") + .send({ mode: "restore", reason: "restore subtree" }); + + expect(res.status).toBe(500); + expect(mockTreeControlService.releaseHold).toHaveBeenCalledWith( + "company-2", + "11111111-1111-4111-8111-111111111111", + "66666666-6666-4666-8666-666666666666", + expect.objectContaining({ + reason: "Restore operation failed before subtree status updates completed", + metadata: { cleanup: "restore_failed_before_apply" }, + }), + ); + }); }); diff --git a/server/src/routes/issue-tree-control.ts b/server/src/routes/issue-tree-control.ts index ebcca8f9c01..1a0ff062c23 100644 --- a/server/src/routes/issue-tree-control.ts +++ b/server/src/routes/issue-tree-control.ts @@ -159,10 +159,22 @@ export function issueTreeControlRoutes(db: Db) { } if (result.hold.mode === "restore") { - const statusUpdate = await treeControlSvc.restoreIssueStatusesForHold(root.companyId, root.id, result.hold.id, { - reason: result.hold.reason, - actor: actorInput, - }); + let statusUpdate; + try { + statusUpdate = await treeControlSvc.restoreIssueStatusesForHold(root.companyId, root.id, result.hold.id, { + reason: result.hold.reason, + actor: actorInput, + }); + } catch (error) { + await treeControlSvc.releaseHold(root.companyId, root.id, result.hold.id, { + reason: "Restore operation failed before subtree status updates completed", + metadata: { + cleanup: "restore_failed_before_apply", + }, + actor: actorInput, + }).catch(() => null); + throw error; + } if (statusUpdate.restoreHold) { result = { ...result, hold: statusUpdate.restoreHold }; } diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index bba7760e128..0618bea9be7 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -80,7 +80,10 @@ import { sanitizeRuntimeServiceBaseEnv, } from "./workspace-runtime.js"; import { issueService } from "./issues.js"; -import { issueTreeControlService } from "./issue-tree-control.js"; +import { + ISSUE_TREE_CONTROL_INTERACTION_WAKE_REASONS, + issueTreeControlService, +} from "./issue-tree-control.js"; import { getIssueContinuationSummaryDocument, refreshIssueContinuationSummary, @@ -1252,17 +1255,11 @@ function shouldRequireIssueCommentForWake( ); } -const ISSUE_INTERACTION_WAKE_REASONS = new Set([ - "issue_commented", - "issue_reopened_via_comment", - "issue_comment_mentioned", -]); - function allowsIssueInteractionWake( contextSnapshot: Record | null | undefined, ) { const wakeReason = readNonEmptyString(contextSnapshot?.wakeReason); - if (!wakeReason || !ISSUE_INTERACTION_WAKE_REASONS.has(wakeReason)) return false; + if (!wakeReason || !ISSUE_TREE_CONTROL_INTERACTION_WAKE_REASONS.has(wakeReason)) return false; return Boolean(deriveCommentId(contextSnapshot, null)); } diff --git a/server/src/services/issue-tree-control.ts b/server/src/services/issue-tree-control.ts index c94d59aeacc..ca57ebb4b0e 100644 --- a/server/src/services/issue-tree-control.ts +++ b/server/src/services/issue-tree-control.ts @@ -70,6 +70,12 @@ type RestoreTreeStatusResult = TreeStatusUpdateResult & { const TERMINAL_ISSUE_STATUSES = new Set(["done", "cancelled"]); const ACTIVE_RUN_STATUSES = ["queued", "running"] as const; const DEFAULT_RELEASE_POLICY: IssueTreeHoldReleasePolicy = { strategy: "manual" }; +const MAX_PAUSE_HOLD_GATE_DEPTH = 15; +export const ISSUE_TREE_CONTROL_INTERACTION_WAKE_REASONS: ReadonlySet = new Set([ + "issue_commented", + "issue_reopened_via_comment", + "issue_comment_mentioned", +] as const); function normalizeReleasePolicy( releasePolicy: IssueTreeHoldReleasePolicy | null | undefined, @@ -421,8 +427,9 @@ export function issueTreeControlService(db: Db) { const holdByRootIssueId = new Map(activePauseHolds.map((hold) => [hold.rootIssueId, hold])); let currentIssueId: string | null = issueId; const visited = new Set(); + let depth = 0; - while (currentIssueId && !visited.has(currentIssueId)) { + while (currentIssueId && !visited.has(currentIssueId) && depth < MAX_PAUSE_HOLD_GATE_DEPTH) { visited.add(currentIssueId); const hold = holdByRootIssueId.get(currentIssueId); if (hold) { @@ -443,6 +450,7 @@ export function issueTreeControlService(db: Db) { .where(and(eq(issues.id, currentIssueId), eq(issues.companyId, companyId))) .then((rows) => rows[0] ?? null); currentIssueId = parent?.parentId ?? null; + depth += 1; } return null; diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index d0a043af218..c611c73d51d 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -37,6 +37,7 @@ import { redactCurrentUserText } from "../log-redaction.js"; import { resolveIssueGoalId, resolveNextIssueGoalId } from "./issue-goal-fallback.js"; import { getDefaultCompanyGoal } from "./goals.js"; import { + ISSUE_TREE_CONTROL_INTERACTION_WAKE_REASONS, issueTreeControlService, type ActiveIssueTreePauseHoldGate, } from "./issue-tree-control.js"; @@ -49,12 +50,6 @@ const ISSUE_LIST_RELATED_QUERY_CHUNK_SIZE = 500; export const MAX_CHILD_ISSUES_CREATED_BY_HELPER = 25; const MAX_CHILD_COMPLETION_SUMMARIES = 20; const CHILD_COMPLETION_SUMMARY_BODY_MAX_CHARS = 500; -const ISSUE_INTERACTION_WAKE_REASONS = new Set([ - "issue_commented", - "issue_reopened_via_comment", - "issue_comment_mentioned", -]); - function assertTransition(from: string, to: string) { if (from === to) return; if (!ALL_ISSUE_STATUSES.includes(to)) { @@ -968,7 +963,7 @@ export function issueService(db: Db) { readStringFromRecord(run?.contextSnapshot, "reason"); return Boolean( wakeReason && - ISSUE_INTERACTION_WAKE_REASONS.has(wakeReason) && + ISSUE_TREE_CONTROL_INTERACTION_WAKE_REASONS.has(wakeReason) && readLatestWakeCommentId(run?.contextSnapshot), ); }