Skip to content

Commit 134a2dc

Browse files
committed
fix(orchestration): sweep settles the parent + posts rollup (review blocker 8)
The panel/rollup/parent-settle side-effects lived ONLY on the live TaskTable-stream path (refreshPanelAndSettle in orchestration-reconciler). When the last child's terminal event was lost — the exact failure the aws-samples#303 sweep exists to recover — the sweep advanced the DDB rows to terminal but NEVER settled the Linear parent, so the epic showed 🔄 In Progress forever. Worse, the sweep's top guard 'if (allTerminal) return 0' skipped an already-fully-terminal-but-unsettled epic entirely, guaranteeing it never closed. Exported refreshPanelAndSettle and call it from the sweep in BOTH spots: (a) the top all-terminal guard now settles-then-returns instead of bare-return; (b) when the release pass finds nothing releasable AND all children are terminal (a child went terminal during this sweep's lost-terminal recovery). It's idempotent (panel body edit is a no-op if unchanged; parent-state mirror claimed once via claimRollup), so racing the live reconciler is safe. Not circular (the reconciler doesn't import the sweep). +2 settle regression tests.
1 parent 7edf144 commit 134a2dc

3 files changed

Lines changed: 80 additions & 6 deletions

File tree

cdk/src/handlers/orchestration-reconciler.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,7 @@ async function reconcileTerminalChild(evt: TerminalTaskEvent): Promise<void> {
648648
* edit is idempotent (same body = no-op), so it always runs; the parent-STATE
649649
* mirror is claimed once via ``claimRollup`` on the first all-terminal caller.
650650
*/
651-
async function refreshPanelAndSettle(
651+
export async function refreshPanelAndSettle(
652652
orchestrationId: string,
653653
children: readonly OrchestrationChildRow[],
654654
meta: { linear_workspace_id: string; parent_linear_issue_id: string; status_comment_id?: string; release_context: { channel_source?: string } },

cdk/src/handlers/reconcile-stranded-orchestrations.ts

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ import {
5858
GetCommand,
5959
UpdateCommand,
6060
} from '@aws-sdk/lib-dynamodb';
61+
import { refreshPanelAndSettle } from './orchestration-reconciler';
6162
import { createTaskCore } from './shared/create-task-core';
6263
import { logger } from './shared/logger';
6364
import { readConcurrencyBudget, releaseReadyChildren } from './shared/orchestration-release';
@@ -123,12 +124,25 @@ async function reconcileOrchestration(orchestrationId: string): Promise<number>
123124
const snap = await loadOrchestration(ddb, ORCHESTRATION_TABLE, orchestrationId);
124125
if (!snap) return 0;
125126

126-
// Skip orchestrations already fully terminal — nothing to recover.
127-
const allTerminal = snap.children.every((c) => TERMINAL_CHILD.has(c.child_status));
128-
if (allTerminal) return 0;
129-
130127
const now = new Date().toISOString();
131128

129+
// Already fully terminal: no children to release/recover — BUT the parent may
130+
// never have been settled (review blocker #8: the last terminal event was lost,
131+
// so the DDB rows are all terminal yet the Linear parent is stuck 🔄 In Progress
132+
// forever). Attempt the idempotent settle before returning, instead of the old
133+
// bare skip that guaranteed the epic never closed on the lossy path.
134+
const allTerminal = snap.children.every((c) => TERMINAL_CHILD.has(c.child_status));
135+
if (allTerminal) {
136+
try {
137+
await refreshPanelAndSettle(orchestrationId, snap.children, snap.meta, now);
138+
} catch (err) {
139+
logger.warn('Sweep settle (already-terminal epic) failed (will retry next sweep)', {
140+
orchestration_id: orchestrationId, error: err instanceof Error ? err.message : String(err),
141+
});
142+
}
143+
return 0;
144+
}
145+
132146
// 1a. Recover a child STUCK in the transient ``releasing`` state (review #3
133147
// flip-then-create): a releaser won the blocked|ready→releasing claim but
134148
// crashed before createTaskCore (or before its rollback). No task exists,
@@ -226,7 +240,31 @@ async function reconcileOrchestration(orchestrationId: string): Promise<number>
226240
})
227241
.map((c) => ({ ...c, child_status: 'ready' as const }));
228242

229-
if (releasableRows.length === 0) return 0;
243+
if (releasableRows.length === 0) {
244+
// review blocker #8: nothing left to release. If EVERY child is now terminal
245+
// but the parent was never settled — because the last child's terminal stream
246+
// event was lost (the exact failure this sweep exists to recover) — the epic
247+
// is 'done' in DynamoDB yet stuck showing 🔄 In Progress in Linear forever.
248+
// The panel/rollup/settle side-effects lived ONLY on the (lossy) live-event
249+
// path. Call the shared settle here so the sweep closes the loop: post the
250+
// rollup + transition the parent. refreshPanelAndSettle is idempotent (body
251+
// edit is a no-op if unchanged; the parent-state mirror is claimed once), so
252+
// racing the live reconciler is safe.
253+
const freshAllTerminal = fresh.children.every((c) => TERMINAL_CHILD.has(c.child_status));
254+
if (freshAllTerminal) {
255+
try {
256+
await refreshPanelAndSettle(orchestrationId, fresh.children, fresh.meta, now);
257+
logger.warn('Stranded orchestration settled by sweep — parent was never settled (lost terminal event)', {
258+
orchestration_id: orchestrationId, parent_linear_issue_id: fresh.meta.parent_linear_issue_id,
259+
});
260+
} catch (err) {
261+
logger.warn('Sweep settle failed (will retry next sweep)', {
262+
orchestration_id: orchestrationId, error: err instanceof Error ? err.message : String(err),
263+
});
264+
}
265+
}
266+
return 0;
267+
}
230268

231269
// #331: throttle the sweep's releases to the free budget too.
232270
const budget = USER_CONCURRENCY_TABLE

cdk/test/handlers/reconcile-stranded-orchestrations.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,15 @@ jest.mock('../../src/handlers/shared/logger', () => ({
7777
logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() },
7878
}));
7979

80+
// review blocker #8: the sweep now imports refreshPanelAndSettle from the live
81+
// reconciler to settle an epic whose last terminal event was lost. Mock it so we
82+
// can assert the sweep CALLS it (its real body is exercised by the reconciler's
83+
// own suite).
84+
const refreshPanelAndSettleMock = jest.fn();
85+
jest.mock('../../src/handlers/orchestration-reconciler', () => ({
86+
refreshPanelAndSettle: (...args: unknown[]) => refreshPanelAndSettleMock(...args),
87+
}));
88+
8089
process.env.ORCHESTRATION_TABLE_NAME = 'OrchestrationTable';
8190
process.env.TASK_TABLE_NAME = 'TaskTable';
8291

@@ -111,6 +120,7 @@ beforeEach(() => {
111120
orch.clear(); tasksTbl.clear(); fakeSend.mockClear();
112121
createTaskCoreMock.mockReset();
113122
createTaskCoreMock.mockResolvedValue({ statusCode: 201, body: JSON.stringify({ data: { task_id: 'new-task' } }) });
123+
refreshPanelAndSettleMock.mockReset();
114124
});
115125

116126
describe('#303 stranded-orchestration backstop', () => {
@@ -136,6 +146,32 @@ describe('#303 stranded-orchestration backstop', () => {
136146
expect(statusOf('o2', 'B')).toBe('released');
137147
});
138148

149+
// review blocker #8: the sweep must SETTLE the parent (post rollup + transition
150+
// Linear), not just flip DDB rows — else a lost last-terminal event leaves the
151+
// epic 'done' in DDB but stuck 🔄 In Progress in Linear forever.
152+
test('lost LAST terminal event: the only remaining child completes → sweep settles the parent', async () => {
153+
seed('o-settle', [
154+
{ sk: 'A', status: 'succeeded' },
155+
{ sk: 'B', deps: ['A'], status: 'released', taskId: 'task-B' },
156+
]);
157+
tasksTbl.set('task-B', { task_id: 'task-B', status: 'COMPLETED', build_passed: true });
158+
await handler();
159+
// B advanced to terminal in DDB…
160+
expect(statusOf('o-settle', 'B')).toBe('succeeded');
161+
// …and, crucially, the parent was settled (the whole point of blocker #8).
162+
expect(refreshPanelAndSettleMock).toHaveBeenCalled();
163+
expect(createTaskCoreMock).not.toHaveBeenCalled(); // nothing left to release
164+
});
165+
166+
test('already fully-terminal but unsettled epic → sweep still settles (idempotent)', async () => {
167+
seed('o-done', [
168+
{ sk: 'A', status: 'succeeded' },
169+
{ sk: 'B', deps: ['A'], status: 'succeeded' },
170+
]);
171+
await handler();
172+
expect(refreshPanelAndSettleMock).toHaveBeenCalledWith('o-done', expect.any(Array), expect.any(Object), expect.any(String));
173+
});
174+
139175
test('lost TERMINAL event with build_passed=false: A→failed, B→skipped, no release', async () => {
140176
seed('o3', [
141177
{ sk: 'A', status: 'released', taskId: 'task-A' },

0 commit comments

Comments
 (0)