feat(jobs): Add data retention jobs#4128
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@TheodoreSpeaks let's consolidate the migrations into a single one, just delete the existing ones and run it once over all the changes in shcema.ts |
57194bd to
a3c1bab
Compare
a3c1bab to
63f7b59
Compare
|
@BugBot review |
PR SummaryHigh Risk Overview Replaces the previous monolithic Adds three new background cleanup tasks ( Adds an Enterprise settings surface for configuring retention: a new Reviewed by Cursor Bugbot for commit 4d76165. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@BugBot review |
0cb8970 to
9cb8dba
Compare
Greptile SummaryAdds three data retention background jobs (soft-delete cleanup, log cleanup, task/chat cleanup) dispatched via Trigger.dev or an inline fallback, with an enterprise-gated UI and API for per-workspace configuration. The migration replaces full soft-delete indexes with partial indexes and adds three retention columns to
Confidence Score: 3/5Not safe to merge as-is: the S3 cleanup gap will permanently orphan workspace_file objects in object storage on every cleanup run. One confirmed P1 data-integrity bug (workspace_file S3 objects never deleted) that will silently accumulate orphaned cloud storage objects on each cron execution. Everything else — batching logic, auth, migration, Trigger.dev wiring, enterprise UI — is well-structured. apps/sim/background/cleanup-soft-deletes.ts — cleanupWorkspaceFileStorage must also cover the workspaceFile (singular) table Important Files Changed
Sequence DiagramsequenceDiagram
participant Cron as Cron (GET /api/cron/*)
participant Dispatcher as dispatchCleanupJobs
participant Queue as JobQueue (Trigger.dev / DB)
participant Task as Background Task
participant DB as Database
participant S3 as Object Storage
participant Copilot as Copilot Backend
Cron->>Dispatcher: dispatchCleanupJobs(jobType, retentionColumn)
Dispatcher->>Queue: enqueue free-tier job
Dispatcher->>Queue: enqueue paid-tier job
Dispatcher->>DB: query enterprise workspaces with non-NULL retention
Dispatcher->>Queue: batchTrigger enterprise jobs
Queue->>Task: run(payload)
Task->>DB: resolveTierWorkspaceIds or lookup workspace retention
Task->>DB: SELECT expiring rows (batched, LIMIT 2000)
Task->>S3: delete associated files (pre-deletion)
Task->>Copilot: POST /api/tasks/cleanup (chat IDs)
Task->>DB: DELETE rows by ID
Task-->>Queue: complete
Reviews (2): Last reviewed commit: "fix lint" | Re-trigger Greptile |
|
@greptile review |
| async function cleanupWorkspaceFileStorage( | ||
| workspaceIds: string[], | ||
| retentionDate: Date | ||
| ): Promise<{ filesDeleted: number; filesFailed: number }> { | ||
| const stats = { filesDeleted: 0, filesFailed: 0 } | ||
|
|
||
| if (!isUsingCloudStorage() || workspaceIds.length === 0) return stats | ||
|
|
||
| const filesToDelete = await db | ||
| .select({ key: workspaceFiles.key }) | ||
| .from(workspaceFiles) | ||
| .where( | ||
| and( | ||
| inArray(workspaceFiles.workspaceId, workspaceIds), | ||
| isNotNull(workspaceFiles.deletedAt), | ||
| lt(workspaceFiles.deletedAt, retentionDate) | ||
| ) | ||
| ) | ||
| .limit(BATCH_SIZE * MAX_BATCHES_PER_TABLE) | ||
|
|
||
| for (const file of filesToDelete) { | ||
| try { | ||
| await StorageService.deleteFile({ key: file.key, context: 'workspace' }) | ||
| stats.filesDeleted++ | ||
| } catch (error) { | ||
| stats.filesFailed++ | ||
| logger.error(`Failed to delete storage file ${file.key}:`, { error }) | ||
| } | ||
| } | ||
|
|
||
| return stats | ||
| } |
There was a problem hiding this comment.
workspaceFile S3 objects orphaned on hard-delete
cleanupWorkspaceFileStorage queries only the workspaceFiles (plural) table, but CLEANUP_TARGETS also includes workspaceFile (singular — workspace_file table). That table has a key column pointing directly to an S3 object. When cleanupTable purges those DB rows, the S3 objects are never removed, leaving permanent storage orphans.
The function needs a parallel query over the workspaceFile table and should call StorageService.deleteFile for each matching key before cleanupTable deletes the rows.
| logRetentionHours: ws.logRetentionHours, | ||
| softDeleteRetentionHours: ws.softDeleteRetentionHours, | ||
| taskCleanupHours: ws.taskCleanupHours, | ||
| } |
There was a problem hiding this comment.
Enterprise effective values ignore configured defaults when null
Medium Severity
For enterprise workspaces, the effective retention values are set directly from the workspace's configured columns (ws.logRetentionHours, etc.) with no fallback. When an enterprise workspace hasn't configured any retention, all effective values are null (meaning "forever"/no cleanup). This means the dispatcher's isNotNull(retentionCol) check never matches, so no cleanup job is ever dispatched. Enterprise workspaces that want the system's enterprise defaults (which are also null) behave identically to those who explicitly chose "Forever" — there's no way to distinguish "not yet configured" from "intentionally set to never." This prevents an enterprise admin from expecting any out-of-the-box cleanup behavior.
Reviewed by Cursor Bugbot for commit adb8f71. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4d76165. Configure here.
| .where(eq(workspace.id, payload.workspaceId)) | ||
| .limit(1) | ||
|
|
||
| if (!ws?.hours) return null |
There was a problem hiding this comment.
Falsy check skips enterprise cleanup for zero-hour retention
Low Severity
resolveCleanupScope uses !ws?.hours which is a falsy check that treats 0 as equivalent to null/not-configured. While the Zod validation on the PUT endpoint enforces min(24), the database column is an unconstrained integer. If a value of 0 were ever stored (e.g., via migration or direct DB access), cleanup would silently skip that workspace instead of applying zero-hour retention. A strict null check like ws?.hours == null would be more correct.
Reviewed by Cursor Bugbot for commit 4d76165. Configure here.
| function hoursToDisplayDays(hours: number | null): string { | ||
| if (hours === null) return 'never' | ||
| return String(Math.round(hours / 24)) | ||
| } |
There was a problem hiding this comment.
Select displays broken state for non-standard hour values
Low Severity
hoursToDisplayDays converts hours to a day string via Math.round(hours / 24), but the result may not match any entry in DAY_OPTIONS. The API accepts any integer between 24 and 43800, so values like 36 hours would produce '2' — not in the select options. This causes the Select component to have a value with no matching SelectItem, resulting in a blank or broken display for enterprise workspaces whose retention was set via direct API call.
Reviewed by Cursor Bugbot for commit 4d76165. Configure here.
Summary
Add data retention jobs. 3 jobs created:
Type of Change
Testing
Checklist
Screenshots/Videos