Add durable background job framework with DB-backed QStash retry #4130
Add durable background job framework with DB-backed QStash retry #4130devkiran wants to merge 17 commits into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds a QStash-backed job framework, shared processing and retry routes, and Prisma-backed deferred job persistence. Folder and partner-tag deletion flows now use transactional cleanup and background job handlers. ChangesJob Framework and Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant JobFramework
participant QStash
participant ProcessRoute
participant Registry
participant JobHandler
participant Prisma
Caller->>JobFramework: dispatch(payload)
JobFramework->>QStash: publish job envelope
QStash->>ProcessRoute: POST signed envelope
ProcessRoute->>Registry: loadJob(jobName)
Registry-->>ProcessRoute: JobDefinition
ProcessRoute->>JobHandler: execute(payload)
JobHandler->>Prisma: perform background cleanup
ProcessRoute-->>QStash: 2xx or 500 response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/api/folders/[folderId]/route.ts (1)
142-190: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMove the delete decision into the transaction
linksCountcan go stale before the delete commits; if a link is attached in that window, the folder is hard-deleted,onDelete: SetNulldetaches the new link, andfolderDeletedJobnever runs because the snapshot count was0. Recompute inside the transaction, or always take the soft-delete/job path and let the job decide when the folder can be removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/folders/`[folderId]/route.ts around lines 142 - 190, The delete/soft-delete choice in the folder handler is being made before the transaction, so `linksCount` in the folder route can become stale and lead to a hard delete when a link is added just before commit. Move the decision logic inside the `prisma.$transaction` in the folder delete flow, using the existing `prisma.folder.delete`, `prisma.folder.update`, and `folderDeletedJob` path based on a fresh count taken within the transaction, or default to the soft-delete/job path and let the job finalize removal when safe.
🧹 Nitpick comments (5)
apps/web/prisma/schema/job.prisma (1)
3-15: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a composite index matching the retry-cron query pattern.
The retry cron (
/api/cron/queue/retry) querieswhere: { attempts: { lt: MAX_ATTEMPTS } }, orderBy: { createdAt: "asc" }, but onlycreatedAtis indexed. Under extended QStash outages theJobtable could grow, and this query would need to filter onattemptswithout index support.♻️ Suggested composite index
- @@index([createdAt]) + @@index([attempts, createdAt])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/prisma/schema/job.prisma` around lines 3 - 15, The retry cron query in the Job model filters by attempts and orders by createdAt, but the schema only indexes createdAt. Update the Job prisma model to add a composite index that matches the `/api/cron/queue/retry` access pattern, using the existing Job fields attempts and createdAt so the retry path can use an index when scanning jobs.apps/web/lib/jobs/handlers/partner-tag-deleted-job.ts (2)
29-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using the structured logger instead of
console.log/console.error.The shared job processor (
apps/web/app/api/jobs/process/[jobName]/route.ts) uses a structuredloggerwithlogger.flush(). Aligning handler logging with that convention would improve observability consistency across the job framework.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/jobs/handlers/partner-tag-deleted-job.ts` around lines 29 - 41, The logging in partnerTagDeletedJob should use the shared structured logger instead of console.error so it matches the job framework’s observability pattern. Update the partner-tag-deleted-job handler to log through the same logger convention used by the shared job processor in process/[jobName]/route.ts, and keep the skip messages for the partnerTagId and partnerTag checks there. Make sure any logger usage is compatible with the existing logger.flush() flow so job logs are consistently captured.
73-85: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded
linksToUpdatequery per batch.Unlike the
programPartnerTagquery (capped atCRON_BATCH_SIZE), thislink.findManyhas notakelimit. A single batch of up to 100 program-partner-tag associations could resolve to a very large number of links for prolific partners/programs, risking a slow or oversizedrecordLinkcall within one job invocation.Consider capping/chunking the links query (or the
recordLinkcall) similarly to the association batching above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/jobs/handlers/partner-tag-deleted-job.ts` around lines 73 - 85, The links lookup in partner-tag-deleted-job is unbounded, unlike the earlier programPartnerTag batch capped by CRON_BATCH_SIZE, so it can pull too many records into one job run. Update the linksToUpdate query in the handler that builds the prisma.link.findMany result to add batching behavior, either by applying a take limit with chunked pagination or by splitting the resulting links before calling recordLink. Keep the fix aligned with the existing CRON_BATCH_SIZE flow so recordLink only processes a bounded set per invocation.apps/web/lib/jobs/handlers/folder-deleted-job.ts (2)
30-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
console.errorused for expected/benign skip paths."Folder not found" and "not marked for deletion" are normal outcomes (e.g., duplicate QStash delivery after the folder was already deleted), not error conditions. Logging them as errors risks noisy alerting on expected behavior;
console.warn/console.logwould be more appropriate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/jobs/handlers/folder-deleted-job.ts` around lines 30 - 42, The skip paths in folderDeletedJob are expected outcomes, not errors, so replace the console.error calls used when the folder is missing or folder.projectId is not empty with a lower-severity log such as console.warn or console.log. Update the logging in folderDeletedJob to keep the same contextual messages while avoiding error-level noise for benign duplicate deliveries or already-processed deletions.
44-56: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMissing composite index confirmed by the inline TODO.
findManyfilters byfolderIdand sorts bycreatedAtwithout a[folderId, createdAt]index (per the Line 50 TODO). At scale this forces a sort over an unindexed/partially-indexed set on every batch iteration of a job that may run many times per large folder. Worth adding the composite index as a quick follow-up.Want me to open a schema change for
@@index([folderId, createdAt])onLink?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/jobs/handlers/folder-deleted-job.ts` around lines 44 - 56, The folder-deleted job’s batch query in folder-deleted-job.ts is still relying on filtering by folderId and ordering by createdAt without an efficient composite index. Add a schema change on the Link model for @@index([folderId, createdAt]) (or the equivalent Prisma index definition) so the linksToUpdate query can use it; keep the existing findMany call and its folderId/createdAt access pattern unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/app/`(ee)/api/cron/queue/retry/route.ts:
- Around line 49-107: The retry cron handler in route.ts treats any successful
qstash.batchJSON(entries) call as a full success, but it must also verify each
batch item returned a messageId before deleting rows. Add the same per-entry
response validation used in jobs/index.ts around the qstash.batchJSON result,
and only call prisma.job.deleteMany when every entry was published successfully;
otherwise keep the failed jobs in the retry queue and follow the existing error
handling path.
In `@apps/web/app/api/folders/`[folderId]/route.ts:
- Around line 192-203: The folder deletion flow in the route handler leaves a
durability gap because `folderDeletedJob.dispatch` is only triggered after the
transaction completes via `waitUntil`, so a crash/timeout can strand the folder
in a half-deleted state. Update the `route.ts` deletion path around
`folderDeletedJob.dispatch` to make the job enqueue durable as part of the same
transaction (or verify and rely on `defineJob.dispatch` synchronously persisting
retry state), using the existing `folderDeletedJob` and transaction block as the
anchor points.
In `@apps/web/lib/api/folders/delete-workspace-folders.ts`:
- Around line 78-84: The folder-deletion flow in deleteWorkspaceFolders
currently writes the deletion marker in the transaction and only calls
folderDeletedJob.dispatchBatch afterward, which can leave folders stuck with
projectId set to an empty string if the process dies in between. Move the
dispatchBatch call so the job queue is populated before the transaction commits,
and keep the existing deleteWorkspaceFolders and folderDeletedJob flow aligned
so the marker write and job enqueue happen atomically from the caller’s
perspective.
In `@apps/web/lib/jobs/handlers/partner-tag-deleted-job.ts`:
- Around line 58-89: The batch in partnerTagDeletedJob is not retry-safe because
programPartnerTag.deleteMany runs before recordLink, so a failed Tinybird ingest
leaves QStash retries with nothing left to replay. In the partner-tag deletion
handler, capture the pre-delete snapshot of the affected links and build the
Tinybird payload from that snapshot by filtering the removed tag out of
programEnrollment.programPartnerTags, then call recordLink before deleting the
programPartnerTag rows. Keep the payload order unchanged, and make sure the
deleteMany happens only after recordLink succeeds.
In `@apps/web/lib/jobs/index.ts`:
- Around line 199-235: persistBackgroundJobs currently loses deferred timing
when options.delay is a string, so retry rows for jobs from DispatchJobInput may
be replayed immediately. Update the scheduledFor derivation inside
persistBackgroundJobs to handle both numeric and string delay values, or persist
the original delay alongside the job record, and ensure the retry path can
reconstruct the correct schedule. Keep the fix localized to the scheduling logic
that builds jobs before prisma.job.createMany, using the existing options,
replayOptions, and scheduledFor fields.
In `@apps/web/lib/jobs/registry.ts`:
- Around line 17-33: The loadJob name-mismatch error in the registry path is
escaping the route’s existing error handling. Update the caller in
process/[jobName]/route.ts so the await loadJob(jobName) step is included in the
same try/catch as job.execute(...), preserving the axiom logging and flush
behavior for registry misconfigurations; use the loadJob and job.execute flow to
locate the fix.
---
Outside diff comments:
In `@apps/web/app/api/folders/`[folderId]/route.ts:
- Around line 142-190: The delete/soft-delete choice in the folder handler is
being made before the transaction, so `linksCount` in the folder route can
become stale and lead to a hard delete when a link is added just before commit.
Move the decision logic inside the `prisma.$transaction` in the folder delete
flow, using the existing `prisma.folder.delete`, `prisma.folder.update`, and
`folderDeletedJob` path based on a fresh count taken within the transaction, or
default to the soft-delete/job path and let the job finalize removal when safe.
---
Nitpick comments:
In `@apps/web/lib/jobs/handlers/folder-deleted-job.ts`:
- Around line 30-42: The skip paths in folderDeletedJob are expected outcomes,
not errors, so replace the console.error calls used when the folder is missing
or folder.projectId is not empty with a lower-severity log such as console.warn
or console.log. Update the logging in folderDeletedJob to keep the same
contextual messages while avoiding error-level noise for benign duplicate
deliveries or already-processed deletions.
- Around line 44-56: The folder-deleted job’s batch query in
folder-deleted-job.ts is still relying on filtering by folderId and ordering by
createdAt without an efficient composite index. Add a schema change on the Link
model for @@index([folderId, createdAt]) (or the equivalent Prisma index
definition) so the linksToUpdate query can use it; keep the existing findMany
call and its folderId/createdAt access pattern unchanged.
In `@apps/web/lib/jobs/handlers/partner-tag-deleted-job.ts`:
- Around line 29-41: The logging in partnerTagDeletedJob should use the shared
structured logger instead of console.error so it matches the job framework’s
observability pattern. Update the partner-tag-deleted-job handler to log through
the same logger convention used by the shared job processor in
process/[jobName]/route.ts, and keep the skip messages for the partnerTagId and
partnerTag checks there. Make sure any logger usage is compatible with the
existing logger.flush() flow so job logs are consistently captured.
- Around line 73-85: The links lookup in partner-tag-deleted-job is unbounded,
unlike the earlier programPartnerTag batch capped by CRON_BATCH_SIZE, so it can
pull too many records into one job run. Update the linksToUpdate query in the
handler that builds the prisma.link.findMany result to add batching behavior,
either by applying a take limit with chunked pagination or by splitting the
resulting links before calling recordLink. Keep the fix aligned with the
existing CRON_BATCH_SIZE flow so recordLink only processes a bounded set per
invocation.
In `@apps/web/prisma/schema/job.prisma`:
- Around line 3-15: The retry cron query in the Job model filters by attempts
and orders by createdAt, but the schema only indexes createdAt. Update the Job
prisma model to add a composite index that matches the `/api/cron/queue/retry`
access pattern, using the existing Job fields attempts and createdAt so the
retry path can use an index when scanning jobs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c93fef7a-626b-4c10-a1d0-4e4219416e51
📒 Files selected for processing (17)
apps/web/app/(ee)/api/cron/folders/delete/route.tsapps/web/app/(ee)/api/cron/queue/retry/route.tsapps/web/app/(ee)/api/cron/tags/delete-partner-tag/route.tsapps/web/app/api/folders/[folderId]/route.tsapps/web/app/api/jobs/process/[jobName]/route.tsapps/web/lib/actions/partners/tags/delete-partner-tag.tsapps/web/lib/api/create-id.tsapps/web/lib/api/folders/delete-workspace-folders.tsapps/web/lib/api/folders/queue-folder-deletion.tsapps/web/lib/api/tags/mark-partner-tag-deleted.tsapps/web/lib/api/tags/queue-tag-deletion.tsapps/web/lib/jobs/handlers/folder-deleted-job.tsapps/web/lib/jobs/handlers/partner-tag-deleted-job.tsapps/web/lib/jobs/index.tsapps/web/lib/jobs/registry.tsapps/web/prisma/schema/job.prismaapps/web/vercel.json
💤 Files with no reviewable changes (5)
- apps/web/app/(ee)/api/cron/folders/delete/route.ts
- apps/web/app/(ee)/api/cron/tags/delete-partner-tag/route.ts
- apps/web/lib/api/tags/mark-partner-tag-deleted.ts
- apps/web/lib/api/folders/queue-folder-deletion.ts
- apps/web/lib/api/tags/queue-tag-deletion.ts
| // Persist jobs that could not be published to QStash. The | ||
| // /api/cron/queue/retry cron republishes them and deletes the rows on success. | ||
| async function persistBackgroundJobs(inputs: DispatchJobInput[]) { | ||
| const jobs = inputs.map(({ name, payload, options }) => { | ||
| let scheduledFor: Date | null = null; | ||
|
|
||
| if (options?.notBefore) { | ||
| scheduledFor = new Date(options.notBefore * 1000); | ||
| } else if (typeof options?.delay === "number") { | ||
| scheduledFor = new Date(Date.now() + options.delay * 1000); | ||
| } | ||
|
|
||
| const replayOptions = { | ||
| ...(options?.deduplicationId && { | ||
| deduplicationId: options.deduplicationId, | ||
| }), | ||
| ...(options?.retries !== undefined && { retries: options.retries }), | ||
| ...(options?.queue && { queue: options.queue }), | ||
| ...(options?.flowControl && { flowControl: options.flowControl }), | ||
| ...(options?.label && { label: options.label }), | ||
| }; | ||
|
|
||
| return { | ||
| id: createId({ prefix: "job_" }), | ||
| name, | ||
| payload: payload as Prisma.InputJsonValue, | ||
| options: replayOptions as Prisma.InputJsonValue, | ||
| scheduledFor, | ||
| }; | ||
| }); | ||
|
|
||
| await prisma.job.createMany({ | ||
| data: jobs, | ||
| }); | ||
|
|
||
| return jobs; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n## files\n'
git ls-files | rg '^(apps/web/lib/jobs/index\.ts|apps/web/lib/jobs/|.*DispatchJobInput|.*qstash|.*upstash|.*job|.*cron.*)$' || true
printf '\n## search DispatchJobInput and delay usage\n'
rg -n "type DispatchJobInput|interface DispatchJobInput|DispatchJobInput|options\\?\\.delay|delay:" apps/web -g '!**/node_modules/**' || true
printf '\n## search buildReplayRequest and scheduledFor\n'
rg -n "buildReplayRequest|scheduledFor" apps/web -g '!**/node_modules/**' || true
printf '\n## inspect target file slice\n'
sed -n '150,280p' apps/web/lib/jobs/index.tsRepository: dubinc/dub
Length of output: 30186
🏁 Script executed:
set -euo pipefail
printf '\n## inspect DispatchJobInput and request builder\n'
sed -n '1,180p' apps/web/lib/jobs/index.ts
printf '\n## inspect QStash helpers/types\n'
sed -n '1,220p' apps/web/lib/webhook/qstash.ts
printf '\n## inspect replay route\n'
sed -n '1,140p' apps/web/app/(ee)/api/cron/queue/retry/route.tsRepository: dubinc/dub
Length of output: 8837
🏁 Script executed:
set -euo pipefail
printf '\n## inspect replay route\n'
sed -n '1,140p' 'apps/web/app/(ee)/api/cron/queue/retry/route.ts'
printf '\n## inspect job callers that pass delay strings\n'
rg -n 'delay:\s*["'"'"'][^"'"'"']+["'"'"']' apps/web/lib apps/web/app -g '!**/node_modules/**' || true
printf '\n## inspect job callers that pass delay from non-numeric expressions\n'
rg -n 'delay:\s*[^0-9{(][^,}]*' apps/web/lib apps/web/app -g '!**/node_modules/**' || trueRepository: dubinc/dub
Length of output: 6781
🌐 Web query:
Upstash QStash PublishRequest delay string duration examples documented delay "3s" "30s"
💡 Result:
Upstash QStash supports delay values specified as a string duration when publishing messages [1][2]. This delay indicates how long QStash should wait after receiving the message before attempting to deliver it [3]. The duration format follows a standard syntax where you combine a numeric value with a time unit [1][4]. The supported time units are: - ns: nanoseconds - us (or µs): microseconds - ms: milliseconds - s: seconds - m: minutes - h: hours - d: days [1] Examples of valid duration strings include: - 3s (3 seconds) [5][2] - 10s (10 seconds) [1] - 1m (1 minute) [1] - 30m (30 minutes) [1] - 2h (2 hours) [1] - 7d (7 days) [1] When using the QStash SDKs, you can pass this duration string directly to the delay parameter [2]. For example, in the Python SDK: client.message.publish_json( url="https://my-api...", body={"hello": "world"}, delay="3s",) [2] Additionally, these duration formats are compatible with HTTP headers like Upstash-Delay when using the REST API directly [1][4]. Note that this delay feature is distinct from the Upstash-Retry-Delay header, which uses a different syntax (mathematical expressions) to control retry timing [4].
Citations:
- 1: https://upstash.com/docs/qstash/features/delay
- 2: https://upstash.com/docs/qstash/sdks/py/examples/publish
- 3: https://upstash.com/docs/qstash/overall/apiexamples
- 4: https://upstash.com/docs/qstash/features/retry
- 5: https://upstash.com/docs/qstash/sdks/ts/examples/publish
Preserve string delays when persisting deferred jobs. scheduledFor only handles numeric delay, but QStash also accepts duration strings like "30s". If publish fails for a job dispatched with a string delay, the retry row is stored without a schedule, so the cron republish runs it immediately instead of honoring the delay. Normalize string delays here or persist the original delay.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/lib/jobs/index.ts` around lines 199 - 235, persistBackgroundJobs
currently loses deferred timing when options.delay is a string, so retry rows
for jobs from DispatchJobInput may be replayed immediately. Update the
scheduledFor derivation inside persistBackgroundJobs to handle both numeric and
string delay values, or persist the original delay alongside the job record, and
ensure the retry path can reconstruct the correct schedule. Keep the fix
localized to the scheduling logic that builds jobs before prisma.job.createMany,
using the existing options, replayOptions, and scheduledFor fields.
|
@coderabbitai full review please |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/web/lib/jobs/handlers/folder-deleted-job.ts (2)
30-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a non-error log level for expected skip paths.
Both the "folder not found" and "not marked for deletion" branches are benign, expected control-flow outcomes (e.g., idempotent re-execution after the folder was already fully deleted), yet they're logged via
console.error. This can pollute error-monitoring/alerting for non-error conditions.♻️ Suggested logging level change
if (!folder) { - console.error( + console.warn( `[folderDeletedJob] Folder ${folderId} not found. Skipping...`, ); return; } if (folder.projectId !== "") { - console.error( + console.warn( `[folderDeletedJob] Folder ${folderId} not marked for deletion. Skipping...`, ); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/jobs/handlers/folder-deleted-job.ts` around lines 30 - 42, The benign skip paths in folderDeletedJob are being logged as errors even though they are expected control flow. Update the logging in folderDeletedJob for the “folder not found” and “not marked for deletion” branches to use a non-error level such as console.warn or console.log, while keeping the same skip messages and early returns so idempotent re-execution does not trigger error monitoring.
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated magic sentinel
""for "pending deletion" across files.
folder.projectId === ""is used here as the "marked for deletion" sentinel, and the same literal is written independently inapps/web/app/api/folders/[folderId]/route.tsandapps/web/lib/api/folders/delete-workspace-folders.ts. Consider extracting a shared constant (e.g.DELETED_FOLDER_PROJECT_ID) to avoid divergence/typos across these three call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/jobs/handlers/folder-deleted-job.ts` at line 37, The “pending deletion” sentinel is duplicated as an empty string across multiple folder deletion call sites, which risks divergence. Introduce a shared constant such as DELETED_FOLDER_PROJECT_ID and use it in folder-deleted-job.ts, the folders API route, and delete-workspace-folders.ts instead of hardcoding "". Update the relevant checks and assignments around folder.projectId to reference the shared symbol consistently.apps/web/lib/api/folders/delete-workspace-folders.ts (1)
79-84: 🚀 Performance & Scalability | 🔵 TrivialChunk large QStash batches by payload size
dispatchBatchdoesn’t have a documented per-request message-count cap; the limit is total batch/message size. IffolderIdscan grow enough to hit your plan’s publish limit, split the dispatch into smaller batches first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/api/folders/delete-workspace-folders.ts` around lines 79 - 84, The batch dispatch in delete-workspace-folders uses folderDeletedJob.dispatchBatch on the full folderIds list, which can exceed QStash publish limits by payload size. Update the dispatch logic in delete-workspace-folders to split folderIds into smaller chunks before calling dispatchBatch, keeping each batch within safe size limits while preserving the existing label mapping from folderId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/app/`(ee)/api/cron/queue/retry/route.ts:
- Around line 19-21: The lock release in the queue retry cron handler can delete
a newer owner’s lock after TTL expiry. Update the `finally` block in the retry
route so it verifies the current Redis value for `LOCK_KEY` still matches the
value set by this run before calling `redis.del`, and only release when the
current execution still owns the lock. Use the existing `LOCK_KEY` and the
acquisition logic around `redis.set`/`acquired` to locate the fix.
In `@apps/web/app/api/jobs/process/`[jobName]/route.ts:
- Line 10: The POST handler in route.ts is logging arbitrary job payloads via
withAxiomBodyLog and also emitting the full envelope in the handler body, so
switch this endpoint to a no-body or redacted logging wrapper and remove any
full payload/body logging from the route. Keep the logs limited to safe metadata
such as jobName, message timing, status, and coarse success/failure details, and
update the POST handler and any related logging helpers accordingly.
In `@apps/web/lib/jobs/handlers/folder-deleted-job.ts`:
- Line 37: The batch query in folderDeletedJobHandler still sorts by createdAt
DESC after filtering by folderId, so the existing folderId index alone is not
enough. Update the relevant model/schema used by the folder-deleted job to add a
composite index covering folderId and createdAt (matching the DESC ordering),
then make sure the query path in folderDeletedJobHandler continues using that
indexed ordering for the 500-row batches and recursive dispatch.
---
Nitpick comments:
In `@apps/web/lib/api/folders/delete-workspace-folders.ts`:
- Around line 79-84: The batch dispatch in delete-workspace-folders uses
folderDeletedJob.dispatchBatch on the full folderIds list, which can exceed
QStash publish limits by payload size. Update the dispatch logic in
delete-workspace-folders to split folderIds into smaller chunks before calling
dispatchBatch, keeping each batch within safe size limits while preserving the
existing label mapping from folderId.
In `@apps/web/lib/jobs/handlers/folder-deleted-job.ts`:
- Around line 30-42: The benign skip paths in folderDeletedJob are being logged
as errors even though they are expected control flow. Update the logging in
folderDeletedJob for the “folder not found” and “not marked for deletion”
branches to use a non-error level such as console.warn or console.log, while
keeping the same skip messages and early returns so idempotent re-execution does
not trigger error monitoring.
- Line 37: The “pending deletion” sentinel is duplicated as an empty string
across multiple folder deletion call sites, which risks divergence. Introduce a
shared constant such as DELETED_FOLDER_PROJECT_ID and use it in
folder-deleted-job.ts, the folders API route, and delete-workspace-folders.ts
instead of hardcoding "". Update the relevant checks and assignments around
folder.projectId to reference the shared symbol consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc0f647e-1ce6-49d9-8012-267ec6151d3d
📒 Files selected for processing (17)
apps/web/app/(ee)/api/cron/folders/delete/route.tsapps/web/app/(ee)/api/cron/queue/retry/route.tsapps/web/app/(ee)/api/cron/tags/delete-partner-tag/route.tsapps/web/app/api/folders/[folderId]/route.tsapps/web/app/api/jobs/process/[jobName]/route.tsapps/web/lib/actions/partners/tags/delete-partner-tag.tsapps/web/lib/api/create-id.tsapps/web/lib/api/folders/delete-workspace-folders.tsapps/web/lib/api/folders/queue-folder-deletion.tsapps/web/lib/api/tags/mark-partner-tag-deleted.tsapps/web/lib/api/tags/queue-tag-deletion.tsapps/web/lib/jobs/handlers/folder-deleted-job.tsapps/web/lib/jobs/handlers/partner-tag-deleted-job.tsapps/web/lib/jobs/index.tsapps/web/lib/jobs/registry.tsapps/web/prisma/schema/job.prismaapps/web/vercel.json
💤 Files with no reviewable changes (5)
- apps/web/lib/api/tags/mark-partner-tag-deleted.ts
- apps/web/lib/api/folders/queue-folder-deletion.ts
- apps/web/app/(ee)/api/cron/folders/delete/route.ts
- apps/web/lib/api/tags/queue-tag-deletion.ts
- apps/web/app/(ee)/api/cron/tags/delete-partner-tag/route.ts
|
@coderabbitai full review please |
|
✅ Action performedFull review finished. |
2 participants
Summary by CodeRabbit