fix(cleanup): UTC cutoff, Composite-cursor Batching, Safe Entity deletion, FK-safe order#10
Conversation
… (createdAt, token)
WalkthroughAdds a batched, cursor-paginated cleanup job and README to remove public form tokens and related data older than a UTC-midnight 7-day cutoff; introduces per-batch transactions, safe-entity checks, retries, lifecycle logging, and an added log-evidence section in incident analysis docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Scheduler
participant JobRunner
participant DB
Note right of Scheduler #EEF6FF: scheduled trigger (daily)
Scheduler->>JobRunner: start cleanup job
Note right of JobRunner #F6FFF0: compute UTC-midnight cutoff (7 days)
loop per-batch (cursor by createdAt, token)
JobRunner->>DB: SELECT batch of tokens < cutoff ORDER BY createdAt, token LIMIT BATCH_SIZE
DB-->>JobRunner: batch(rows)
alt batch empty
JobRunner->>JobRunner: finish loop
else
JobRunner->>DB: query other tokens for batch entityIds
DB-->>JobRunner: referencing tokens
JobRunner->>DB: BEGIN TRANSACTION
Note right of DB #FFF7EE: transactional deletions (ordered)
JobRunner->>DB: DELETE FROM new_corpus WHERE entity_id IN (safe_entity_ids)
JobRunner->>DB: DELETE FROM publicFormsTokens WHERE token IN (batch_tokens)
JobRunner->>DB: DELETE FROM entities WHERE id IN (safe_entity_ids)
JobRunner->>DB: DELETE FROM relationships WHERE product_id IN (batch_products) AND status='new'
JobRunner->>DB: COMMIT
JobRunner->>JobRunner: advance cursor, log progress
end
end
JobRunner->>Scheduler: mark job completed (or failed on error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge Base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (4)
50-76: Composite-cursor pagination: correct comparator; add supporting indexesThe OR comparator correctly implements (createdAt, token) > (lastCreatedAt, lastToken). To keep scans efficient at scale, ensure compound indexes exist:
- publicFormsTokens(createdAt asc, token asc)
- publicFormsTokens(entityId asc, createdAt asc) for the entity lookups below.
Would you like me to generate a migration with these indexes?
121-136: Relationship cleanup scope may over-delete; restrict by entity when possibleDeleting by product_id + status "new" could remove rows unrelated to the entities actually being purged (shared product). Prefer scoping by entity where schema allows.
- prisma.relationship.deleteMany({ - where: { - product_id: { in: productIdsInBatch }, - status: "new", - // If relationships are entity-scoped, uncomment the next line - // entity_id: { in: entityIdsSafeToDelete } - }, - }) + prisma.relationship.deleteMany({ + where: { + product_id: { in: productIdsInBatch }, + status: "new", + entity_id: { in: entityIdsSafeToDelete } // ensure schema supports this + }, + })If relationships are not entity-scoped, consider deferring this optional cleanup or adding tighter predicates.
53-64: Unsubmitted-only filtering (schema-dependent)If the schema tracks submission state (e.g., submittedAt or isSubmitted), add it to both the page query and the “other tokens” check to ensure we only clean up unsubmitted flows.
I can wire this in once you confirm the exact column(s).
153-156: Consider “completed_with_errors” status when totalFailedChunks > 0You already count failed chunks; surfacing that in the terminal status can help operators distinguish clean vs. partial runs.
tests/unsubmitted_forms/README.md (3)
6-15: Document entity-safety nuance and failure cursor policyAdd that entities are deleted only when no tokens remain for them (not just no unexpired tokens) to avoid FK issues/orphans, and that the job advances the cursor only after successful batch commits (retry on failures).
23-27: Operational status for partial successSince batches can fail but the job continues, note how operators should interpret totals and whether a “completed_with_errors” status is used when failed chunks > 0.
28-37: Indexing requirementsCall out the recommended indexes explicitly to ensure predictable performance:
- publicFormsTokens(createdAt, token)
- publicFormsTokens(entityId, createdAt)
- relationship(product_id, status) if the optional cleanup is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
tests/unsubmitted_forms/README.md(1 hunks)tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts(1 hunks)
🔇 Additional comments (2)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (2)
39-42: UTC cutoff calculation looks correctZeroing to UTC midnight then subtracting 7 days avoids drift from late job starts.
90-105: Verify FK referential action and broaden tokens check
- Confirm whether publicFormsTokens.entityId → entity.id is defined with ON DELETE CASCADE.
- If it isn’t, remove the
createdAt: { gte: cutoff }filter so you keep any other token (expired or not) before deleting its entity.
…or only on success
… scope relationship cleanup by entity, cursor advance only on success with retries; docs: indexes, cursor policy
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
tests/end_of_week_issues/analysis.md (3)
3-3: Soften causality claim; call it correlation“Direct correlation” reads as causal. Recommend: “strong correlation observed; needs controlled rollback/flip to establish causation.”
5-15: Clarify the ‘Status’ column semanticsMixing HTTP states (“OK”) with outcome labels (“Timeout”) is ambiguous. Either use HTTP codes (e.g., 200/504) or add a separate “Outcome” column.
-| Response Time (ms) | Status | +| Response Time (ms) | HTTP Code | Outcome | - | 120 | `OK` | + | 120 | 200 | `OK` | - | 750 | `Timeout` | + | 750 | 504 | `Timeout` |
16-16: Remove stray line artifactThere’s a lone “16” line at the end. Delete it to avoid rendering issues.
-16tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (4)
52-76: Composite-cursor pagination looks correct; consider native cursor if a unique composite existsThe OR predicate matches (createdAt asc, token asc) traversal. If you can add a unique index on (createdAt, token), you could switch to Prisma’s cursor to offload the OR to tuple comparison.
// If unique index exists: await prisma.publicFormsTokens.findMany({ where: { createdAt: { lt: cutoff } }, orderBy: [{ createdAt: "asc" }, { token: "asc" }], cursor: last ? { createdAt_token: { createdAt: last.createdAt, token: last.token } } : undefined, skip: last ? 1 : 0, take: BATCH_SIZE, });Also ensure indexes:
- publicFormsTokens(createdAt)
- publicFormsTokens(entityId)
158-159: Add small exponential backoff between retriesAvoid hammering the DB on transient errors.
- continue; // retry same page; cursor unchanged + await new Promise((r) => setTimeout(r, Math.min(1000, 100 * retriesForPage))); + continue; // retry same page; cursor unchanged
174-176: Include job id in error log for correlationMakes failure logs grep-able by job id.
- console.error("Error cleaning up unsubmitted forms:", error); + console.error(`[Cleanup Job ${job.id}] Error cleaning up unsubmitted forms:`, error);
83-90: Minor: rename variables for claritytokenIdsInBatch → tokensInPage; entityIdsInBatch → entitiesInPage. Not required, but improves readability when scanning logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
tests/unsubmitted_forms/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (3)
tests/end_of_week_issues/analysis.md(1 hunks)tests/unsubmitted_forms/README.md(1 hunks)tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unsubmitted_forms/README.md
🔇 Additional comments (4)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (4)
40-42: UTC midnight cutoff is computed correctlysetUTCHours(0,0,0,0) then subtracting 7 days yields the intended “midnight UTC 7 days ago”.
109-109: Typed ops as Prisma promises — good fixUsing Prisma.PrismaPromise[] avoids the $transaction overload union trap.
71-76: Verify ‘take’ batch size and DB limitsSome databases have parameter limits that make IN (...) with 500 values borderline. If you see “too many SQL variables,” drop to 200–300.
93-108: Ensure database index on publicFormsTokens.entityId
Add or verify an index onentityIdin your Prisma schema or generated migrations to keep the “other tokens” lookup performant at scale.
| await update_job_status(job.id, "completed"); | ||
| console.log( | ||
| `[Cleanup Job ${job.id}] Completed in ${new Date().getTime() - jobStartTime.getTime()}ms. Deleted tokens: ${totalDeletedTokens}. Failed chunks: ${totalFailedChunks}.` | ||
| ); | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard ‘completed’ status on success only
If you adopt the throw above, this block won’t run on failure. Otherwise, add a success flag and set status accordingly.
- await update_job_status(job.id, "completed");
+ await update_job_status(job.id, "completed");(No change needed if you throw on failure.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts around lines 169 to 173,
the code unconditionally calls update_job_status(job.id, "completed") even when
an error may occur; ensure the job status is set to "completed" only on success
by either moving the update_job_status call into the try block after all work
succeeds or introduce a success flag (set true at end of try) and in the
finally/outer scope call update_job_status(job.id, success ? "completed" :
"failed"); also ensure any thrown errors are allowed to propagate or are logged
before setting the failed status so failure paths are accurately recorded.
2 participants
Summary
This PR corrects and hardens the scheduled cleanup job for unsubmitted public forms. It fixes time math and date filtering, adds deterministic pagination and batching, protects entities from accidental deletion, and enforces an FK-safe deletion order. It also improves observability via job status updates and structured logs.
Context / Problem
The original job had several correctness and reliability issues:
What changed
Key implementation details
createdAt < cutoff.orderBy createdAt asc, token asc; next page wherecreatedAt > lastCreatedAtOR (createdAt == lastCreatedAtANDtoken > lastToken).createdAt ≥ cutoffreferencing those entities; if found, keep the entity.new_corpus.deleteMany(children) →publicFormsTokens.deleteMany→entity.deleteMany(parents).relationship.deleteManyforproduct_id INbatch andstatus = "new"(can be scoped further byentity_idif schema requires).Files changed
tests/unsubmitted_forms/cleanup_unsubmitted_forms.tstests/unsubmitted_forms/README.md(design/ops notes)Why this approach
Assumptions
update_job_status(jobId, status)exists and updates job state in the scheduler.publicFormsTokens:token,entityId,productId,createdAt.new_corpusreferencing entities viaentity_id.entitywith primary keyid.relationshipwithproduct_id(and optionallyentity_id) andstatus.submittedAtorisSubmitted), you can add those filters to the where clause as an extra guard.Operational guidance
publicFormsTokens(createdAt)publicFormsTokens(entityId)How to test locally
in_progress → completed.createdAt < cutoffare gone.Trade-offs considered
Future work (optional)
entity_idif schema requires.Checklist
Reference
Summary by CodeRabbit
Chores
Documentation
Tests