Skip to content

Commit 33c22db

Browse files
committed
fix: address review findings in PR coleam00#962
- Remove duplicate 'returns null when no resumable run exists' test that was accidentally left in alongside the newly-added one - Fix twoDaysAgo timestamp arithmetic (was 48 minutes, not 2 days) - Add test for last_activity_at IS NULL branch in findResumableRun - Change warn → error + add errorType in findResumableRun catch block - Add errorType to workflow.ts signal cleanup catch - Add errorType to cli.ts failOrphanedRuns startup catch - Add inline comment clarifying nowMinusDays(3) uses param index, not day count
1 parent eefddb4 commit 33c22db

4 files changed

Lines changed: 29 additions & 13 deletions

File tree

packages/cli/src/cli.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,8 @@ async function main(): Promise<number> {
219219
void createWorkflowStore()
220220
.failOrphanedRuns()
221221
.catch((err: unknown) => {
222-
getLog().warn({ err }, 'cli.fail_orphans_failed');
222+
const e = err as Error;
223+
getLog().warn({ err: e, errorType: e.constructor.name }, 'cli.fail_orphans_failed');
223224
});
224225

225226
// Validate working directory exists

packages/cli/src/commands/workflow.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,11 @@ export async function workflowRunCommand(
532532
return undefined;
533533
})
534534
.catch((err: unknown) => {
535-
getLog().error({ err: err as Error }, 'workflow.termination_cleanup_failed');
535+
const e = err as Error;
536+
getLog().error(
537+
{ err: e, errorType: e.constructor.name },
538+
'workflow.termination_cleanup_failed'
539+
);
536540
})
537541
.finally(() => {
538542
process.exit(1);

packages/core/src/db/workflows.test.ts

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -492,16 +492,8 @@ describe('workflows database', () => {
492492
expect(params).toEqual(['feature-development', '/repo/path', 1]);
493493
});
494494

495-
test('returns null when no resumable run exists', async () => {
496-
mockQuery.mockResolvedValueOnce(createQueryResult([]));
497-
498-
const result = await findResumableRun('feature-development', '/repo/path');
499-
500-
expect(result).toBeNull();
501-
});
502-
503495
test('returns a stale running run (no activity for >1 day)', async () => {
504-
const twoDaysAgo = new Date(Date.now() - 2 * 24 * 60 * 1000).toISOString();
496+
const twoDaysAgo = new Date(Date.now() - 2 * 24 * 60 * 60 * 1000).toISOString();
505497
const staleRun = {
506498
...mockWorkflowRun,
507499
status: 'running' as const,
@@ -519,6 +511,22 @@ describe('workflows database', () => {
519511
expect(params).toEqual(['feature-development', '/repo/path', 1]);
520512
});
521513

514+
test('returns a running run with null last_activity_at (never recorded activity)', async () => {
515+
const staleRun = {
516+
...mockWorkflowRun,
517+
status: 'running' as const,
518+
working_path: '/repo/path',
519+
last_activity_at: null,
520+
};
521+
mockQuery.mockResolvedValueOnce(createQueryResult([staleRun]));
522+
523+
const result = await findResumableRun('feature-development', '/repo/path');
524+
525+
expect(result).toEqual(staleRun);
526+
const [query] = mockQuery.mock.calls[0] as [string, unknown[]];
527+
expect(query).toContain('last_activity_at IS NULL');
528+
});
529+
522530
test('returns null when no resumable run exists', async () => {
523531
mockQuery.mockResolvedValueOnce(createQueryResult([]));
524532

packages/core/src/db/workflows.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ export async function findResumableRun(
234234
AND working_path = $2
235235
AND (
236236
status IN ('failed', 'paused')
237-
OR (status = 'running' AND (last_activity_at IS NULL OR last_activity_at < ${dialect.nowMinusDays(3)}))
237+
OR (status = 'running' AND (last_activity_at IS NULL OR last_activity_at < ${dialect.nowMinusDays(3)})) -- nowMinusDays(3): param index 3, value bound as $3 = 1 day
238238
)
239239
ORDER BY started_at DESC
240240
LIMIT 1`,
@@ -244,7 +244,10 @@ export async function findResumableRun(
244244
return row ? normalizeWorkflowRun(row) : null;
245245
} catch (error) {
246246
const err = error as Error;
247-
getLog().warn({ err, workflowName, workingPath }, 'db.workflow_run_find_resumable_failed');
247+
getLog().error(
248+
{ err, errorType: err.constructor.name, workflowName, workingPath },
249+
'db.workflow_run_find_resumable_failed'
250+
);
248251
throw new Error(`Failed to find resumable run: ${err.message}`);
249252
}
250253
}

0 commit comments

Comments
 (0)