Skip to content
This repository was archived by the owner on May 15, 2026. It is now read-only.

Commit 30f2aa2

Browse files
committed
fix: address roomote review concerns
- Improve calculateTaskStorageSize() logging to show actual error message instead of generic 'directory not found' - Serialize task deletions when using provider-backed deleteTaskById to avoid taskHistory state race conditions (parallel deletion kept for filesystem-only operations)
1 parent 9c92200 commit 30f2aa2

2 files changed

Lines changed: 56 additions & 45 deletions

File tree

src/utils/task-history-retention.ts

Lines changed: 51 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ export async function purgeOldTasks(
246246
return { purgedCount: 0, cutoff }
247247
}
248248

249-
// Phase 3: Delete tasks in parallel
249+
// Phase 3: Delete tasks
250250
if (dryRun) {
251251
for (const { metadata, reason } of tasksToDelete) {
252252
logv(`[Retention][DRY RUN] Would delete task ${metadata.taskId} (${reason}) @ ${metadata.taskDir}`)
@@ -257,52 +257,61 @@ export async function purgeOldTasks(
257257
return { purgedCount: tasksToDelete.length, cutoff }
258258
}
259259

260-
logv(`[Retention] Phase 3: Deleting ${tasksToDelete.length} tasks (concurrency: ${DELETION_CONCURRENCY})`)
261-
const deleteLimit = pLimit(DELETION_CONCURRENCY)
260+
// Helper function to delete a single task
261+
const deleteTask = async (metadata: TaskMetadata, reason: string): Promise<boolean> => {
262+
let deleted = false
262263

263-
const deleteResults = await Promise.all(
264-
tasksToDelete.map(({ metadata, reason }) =>
265-
deleteLimit(async (): Promise<boolean> => {
266-
let deleted = false
264+
try {
265+
if (deleteTaskById) {
266+
logv(`[Retention] Deleting task ${metadata.taskId} via provider @ ${metadata.taskDir} (${reason})`)
267+
await deleteTaskById(metadata.taskId, metadata.taskDir)
268+
deleted = !(await pathExists(metadata.taskDir))
269+
} else {
270+
logv(`[Retention] Deleting task ${metadata.taskId} via fs.rm @ ${metadata.taskDir} (${reason})`)
271+
await fs.rm(metadata.taskDir, { recursive: true, force: true })
272+
deleted = !(await pathExists(metadata.taskDir))
273+
}
274+
} catch (e) {
275+
// Primary deletion failed, try fallback
276+
logv(
277+
`[Retention] Primary deletion failed for ${metadata.taskId}: ${
278+
e instanceof Error ? e.message : String(e)
279+
}`,
280+
)
281+
}
267282

268-
try {
269-
if (deleteTaskById) {
270-
logv(
271-
`[Retention] Deleting task ${metadata.taskId} via provider @ ${metadata.taskDir} (${reason})`,
272-
)
273-
await deleteTaskById(metadata.taskId, metadata.taskDir)
274-
deleted = !(await pathExists(metadata.taskDir))
275-
} else {
276-
logv(`[Retention] Deleting task ${metadata.taskId} via fs.rm @ ${metadata.taskDir} (${reason})`)
277-
await fs.rm(metadata.taskDir, { recursive: true, force: true })
278-
deleted = !(await pathExists(metadata.taskDir))
279-
}
280-
} catch (e) {
281-
// Primary deletion failed, try fallback
282-
logv(
283-
`[Retention] Primary deletion failed for ${metadata.taskId}: ${
284-
e instanceof Error ? e.message : String(e)
285-
}`,
286-
)
287-
}
283+
// Fallback: simplified removal
284+
if (!deleted) {
285+
deleted = await removeDir(metadata.taskDir)
286+
}
288287

289-
// Fallback: simplified removal
290-
if (!deleted) {
291-
deleted = await removeDir(metadata.taskDir)
292-
}
288+
if (!deleted) {
289+
log?.(`[Retention] Failed to delete task ${metadata.taskId} @ ${metadata.taskDir}: directory still present`)
290+
} else {
291+
logv(`[Retention] Deleted task ${metadata.taskId} (${reason}) @ ${metadata.taskDir}`)
292+
}
293293

294-
if (!deleted) {
295-
log?.(
296-
`[Retention] Failed to delete task ${metadata.taskId} @ ${metadata.taskDir}: directory still present`,
297-
)
298-
} else {
299-
logv(`[Retention] Deleted task ${metadata.taskId} (${reason}) @ ${metadata.taskDir}`)
300-
}
294+
return deleted
295+
}
301296

302-
return deleted
303-
}),
304-
),
305-
)
297+
let deleteResults: boolean[]
298+
299+
if (deleteTaskById) {
300+
// Sequential deletion when using provider-backed deletion to avoid taskHistory state races
301+
logv(`[Retention] Phase 3: Deleting ${tasksToDelete.length} tasks sequentially (provider-backed)`)
302+
deleteResults = []
303+
for (const { metadata, reason } of tasksToDelete) {
304+
const result = await deleteTask(metadata, reason)
305+
deleteResults.push(result)
306+
}
307+
} else {
308+
// Parallel deletion for filesystem-only operations (no shared state)
309+
logv(`[Retention] Phase 3: Deleting ${tasksToDelete.length} tasks (concurrency: ${DELETION_CONCURRENCY})`)
310+
const deleteLimit = pLimit(DELETION_CONCURRENCY)
311+
deleteResults = await Promise.all(
312+
tasksToDelete.map(({ metadata, reason }) => deleteLimit(() => deleteTask(metadata, reason))),
313+
)
314+
}
306315

307316
const purged = deleteResults.filter(Boolean).length
308317

src/utils/task-storage-size.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,11 @@ export async function calculateTaskStorageSize(
5151
try {
5252
const entries = await fs.readdir(tasksDir, { withFileTypes: true })
5353
taskCount = entries.filter((d) => d.isDirectory()).length
54-
} catch {
55-
// Tasks directory doesn't exist yet
56-
log?.(`[TaskStorageSize] Tasks directory not found at ${tasksDir}`)
54+
} catch (e) {
55+
// Tasks directory doesn't exist yet (or is unreadable)
56+
log?.(
57+
`[TaskStorageSize] Failed to read tasks directory at ${tasksDir}: ${e instanceof Error ? e.message : String(e)}`,
58+
)
5759
return defaultResult
5860
}
5961

0 commit comments

Comments
 (0)