fix(logs-cleanup): listing active workspaces into mem + download time streaming lims#4692
fix(logs-cleanup): listing active workspaces into mem + download time streaming lims#4692icecrasher321 wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview
Multipart upload and CSV import endpoints ( Background/infra changes: retention cleanup dispatch now pages active workspaces to avoid loading all into memory, adds per-job Reviewed by Cursor Bugbot for commit f79c50f. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f79c50f. Configure here.
| for (const [idx, ws] of planChunks.entries()) { | ||
| chunks.push({ | ||
| plan: housekeepingPlan, | ||
| workspaceIds: [], |
There was a problem hiding this comment.
Cleanup chunk labels collide across pagination pages
Low Severity
The paginated buildCleanupChunks generates chunk labels independently per page, so the same plan appearing across multiple pages produces duplicate labels. For example, if page 1 yields one "free" chunk and page 2 also yields one "free" chunk, both get label: 'free'. The old non-paginated code processed all workspaces at once, ensuring unique labels. This causes duplicate inline job IDs (inline:${jobType}:${payload.label}) and ambiguous log entries.
Reviewed by Cursor Bugbot for commit f79c50f. Configure here.
| runGlobalHousekeeping: true, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Housekeeping chunk always created separately, never piggybacks on existing
Low Severity
The refactored housekeeping logic always creates a new synthetic chunk with empty workspaceIds and runGlobalHousekeeping: true. The old code first checked if a chunk for the housekeeping plan already existed and would mark that existing chunk instead, avoiding a redundant job. Now an extra housekeeping-only chunk is always dispatched even when plan chunks already exist, creating unnecessary job overhead.
Reviewed by Cursor Bugbot for commit f79c50f. Configure here.
Greptile SummaryThis PR introduces two broad improvements: (1) a
Confidence Score: 3/5The stream-limit and log-compaction changes are safe, but the cleanup dispatcher has behavioral regressions in its scheduled dispatch path. The refactored dispatcher inverts execution priority so in-process inline runs before Trigger.dev, changes job-queue enqueuing from parallel to sequential, and makes chunk labels non-unique across pagination pages — all in scheduled code that directly controls how log retention jobs are distributed. apps/sim/lib/billing/cleanup-dispatcher.ts requires the most attention: the inline-before-Trigger.dev ordering, non-unique page-scoped chunk labels, and sequential enqueue loop all need review before this ships to production. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[dispatchCleanupJobs] --> B[buildCleanupChunks paginated loop]
B --> C{more pages?}
C -- yes --> D[listActiveWorkspaceCleanupScopeRowsPage afterCursor]
D --> E[resolvePlanTypes for page rows]
E --> F[push non-enterprise chunks per page]
F --> G[push enterprise chunks per page]
G --> C
C -- no --> H[append housekeeping empty chunk]
H --> I{shouldExecuteInline?}
I -- yes NEW --> J[run inline sequentially return early]
I -- no --> K{isTriggerAvailable?}
K -- yes --> L[batchTrigger with concurrencyKey]
K -- no --> M[jobQueue sequential loop no runner]
|
| const inlineRunner = shouldExecuteInline() ? await buildCleanupRunner(jobType) : undefined | ||
| if (inlineRunner) { | ||
| let succeeded = 0 | ||
| let failed = 0 | ||
|
|
||
| for (const payload of chunks) { | ||
| try { | ||
| await inlineRunner(payload, new AbortController().signal) | ||
| jobIds.push(`inline:${jobType}:${payload.label}`) | ||
| succeeded++ | ||
| } catch (error) { | ||
| failed++ | ||
| logger.error(`[${jobType}] Inline cleanup chunk failed:`, { | ||
| plan: payload.plan, | ||
| label: payload.label, | ||
| error, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| logger.info(`[${jobType}] Inline cleanup chunks: ${succeeded} succeeded, ${failed} failed`) | ||
| return { jobIds, jobCount: jobIds.length, chunkCount: chunks.length, workspaceCount } | ||
| } | ||
|
|
||
| if (isTriggerAvailable()) { |
There was a problem hiding this comment.
The execution order was reversed: inline execution is now checked and returned from before isTriggerAvailable(), whereas the old code checked Trigger.dev first and only fell back to the inline/job-queue path when Trigger.dev was unavailable. In any environment where both shouldExecuteInline() and isTriggerAvailable() return true simultaneously (e.g., database async backend with TRIGGER_SECRET_KEY set), cleanup jobs will now run synchronously in-process rather than through Trigger.dev, losing distributed concurrency control, visibility, and retry semantics.
Summary
WIP
Fixes #(issue)
Type of Change
Testing
How has this been tested? What should reviewers focus on?
Checklist
Screenshots/Videos