fix: Fixed issues and a working solution on unsubmitted forms#12
Conversation
WalkthroughAdded .idea ignore rule in tests/.gitignore. Appended retrospective notes to tests/end_of_week_issues/README.md. Refactored tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts: simplified age-based filtering, handled orphan tokens, introduced parallel counts to determine safe entity deletion, performed transactional deletes, and added error handling with job status updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Job as Cleanup Job
participant DB as Database
participant Tx as Transaction
Job->>DB: Query tokens where createdAt < (now - 7 days) AND submittedAt IS NULL
loop For each token
alt token has no entityId
Job->>DB: Delete token
else token has entityId
par Count operations
Job->>DB: Count submitted tokens for entity
Job->>DB: Count relationships with status != "new" for entity
end
Job->>Job: safeToDeleteEntity = (submittedCount == 0 && nonNewRelCount == 0)
Job->>DB: Delete token
alt safeToDeleteEntity
Job->>Tx: Begin transaction
Tx->>DB: Delete "new" relationships for entity
Tx->>DB: Delete corpus items for entity
Tx->>DB: Delete entity
Tx-->>Job: Commit
else Not safe
Note right of Job: Keep entity and non-new relationships
end
end
end
rect rgb(255,240,240)
Note over Job: Error path
Job-->>Job: Log error
Job->>DB: Update job status = "failed"
Job-->>Job: Rethrow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 1
🧹 Nitpick comments (8)
tests/.gitignore (1)
1-1: Scope ignore to repo root or use directory form.Placing
.idea/*undertests/won’t ignore IDE files created at the repo root. Prefer adding.idea/to the root.gitignore, or if you intend to keep it scoped here, use.idea/(trailing slash) for clarity.tests/end_of_week_issues/README.md (1)
28-32: Fix typos and align heading with bullet count.Minor language fixes; also add a 4th bullet to match the “Four Bulleted” heading.
- My Four Bulleted Retrospective - - An issue was resported following a deployment that happened for v1.3 and seems testing was improper - - A need to work on a deployment strategy that is behind an automated rolback configurations in the event of a issues - - A need to discuss and work on monitory, alerts and notifications to pick up on issues before clients do + My Four‑Bullet Retrospective + - An issue was reported after the v1.3 deployment; pre‑release testing was insufficient. + - Adopt an automated rollback strategy (one‑click/auto rollback on health/SLO breach). + - Improve monitoring, alerting, and notifications to detect issues before clients do. + - Introduce canary releases/feature flags to limit blast radius during rollouts.tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (6)
31-31: Name constants clearly.Use an uppercase constant; this improves readability and avoids “no_of_ms” ambiguity.
-const no_of_ms = 24 * 60 * 60 * 1000; +const DAY_MS = 24 * 60 * 60 * 1000;
36-43: Consistent naming for dates.CamelCase for variables; also adjust usage below.
-const cutoff_date = new Date(Date.now() - 7 * no_of_ms); +const cutoffDate = new Date(Date.now() - 7 * DAY_MS); ... - createdAt: { lt: cutoff_date }, + createdAt: { lt: cutoffDate },
40-45: Potential large result set; consider pagination and index.
findManywithouttake/orderBycan load too many rows. Use paginated batches (take,cursor,orderBy: { createdAt: 'asc' }) and ensure an index on(submittedAt, createdAt)(or at leastcreatedAt) to keep the daily job predictable.
75-100: Re-check invariants inside the transaction to close race windows.Between the pre-check and the transaction, another process could submit a token or promote a relationship out of “new.” Validate again inside
txbefore cascading deletes.- if (safeToDeleteEntity) { + if (safeToDeleteEntity) { + // Re-check inside the transaction to avoid races + const [submittedNow, otherUnsubmittedNow, nonNewRelNow] = await Promise.all([ + tx.publicFormsTokens.count({ where: { entityId, submittedAt: { not: null } } }), + tx.publicFormsTokens.count({ where: { entityId, submittedAt: null } }), + tx.relationship.count({ where: { entity_id: entityId, NOT: { status: "new" } } }), + ]); + if (submittedNow > 0 || otherUnsubmittedNow > 0 || nonNewRelNow > 0) { + return; // abort entity/children delete; token delete already done + } // remove new relationships tied to this entity await tx.relationship.deleteMany({ where: { entity_id: entityId, status: "new", }, }); // Delete all corpus items associated with the entity await tx.new_corpus.deleteMany({ where: { entity_id: entityId }, }); // Delete the entity (company) await tx.entity.delete({ where: { id: entityId }, }); }
104-108: Prefer structured logs and avoid leaking sensitive values.Log with context (job id, counts), but don’t include raw tokens or PII. Consider using your logger with severity and event codes; keep
console.errorfor tests only.
40-101: Optional: per-record error isolation and batching.A single failure currently aborts the whole run. Wrap the per-token body in its own try/catch and keep tallies so the job can complete best‑effort, then report summary stats. Also consider deleting expired orphan tokens in bulk via one
deleteManybefore the loop.
📜 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 (3)
tests/.gitignore(1 hunks)tests/end_of_week_issues/README.md(1 hunks)tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts(1 hunks)
🔇 Additional comments (1)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (1)
49-52: Good: orphan-token fast path.Early delete for tokens without
entityIdkeeps the loop simple and prevents unnecessary lookups.
| const [submittedTokenCount, nonNewRelCount] = await Promise.all([ | ||
| prisma.publicFormsTokens.count({ | ||
| where: { | ||
| entityId, | ||
| submittedAt: { not: null }, | ||
| }, | ||
| }), | ||
| prisma.relationship.count({ | ||
| where: { | ||
| entity_id: entityId, | ||
| NOT: { status: "new" }, // anything other than 'new' blocks deletion | ||
| }, | ||
| }), | ||
| ]); | ||
|
|
||
| const safeToDeleteEntity = submittedTokenCount === 0 && nonNewRelCount === 0; | ||
|
|
There was a problem hiding this comment.
Blocker: entity delete can violate FK if other unsubmitted tokens exist.
You only check for submitted tokens and non‑new relationships. If the entity has another unsubmitted token, deleting the entity will fail and abort the whole job. Include a count of other unsubmitted tokens for the entity (excluding the current token) in the safety gate.
- const [submittedTokenCount, nonNewRelCount] = await Promise.all([
+ const [submittedTokenCount, nonNewRelCount, otherUnsubmittedTokenCount] = await Promise.all([
prisma.publicFormsTokens.count({
where: {
entityId,
submittedAt: { not: null },
},
}),
prisma.relationship.count({
where: {
entity_id: entityId,
NOT: { status: "new" }, // anything other than 'new' blocks deletion
},
}),
+ prisma.publicFormsTokens.count({
+ where: {
+ entityId,
+ submittedAt: null,
+ token: { not: token.token }, // exclude the token we're processing
+ },
+ }),
]);
- const safeToDeleteEntity = submittedTokenCount === 0 && nonNewRelCount === 0;
+ const safeToDeleteEntity =
+ submittedTokenCount === 0 &&
+ nonNewRelCount === 0 &&
+ otherUnsubmittedTokenCount === 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [submittedTokenCount, nonNewRelCount] = await Promise.all([ | |
| prisma.publicFormsTokens.count({ | |
| where: { | |
| entityId, | |
| submittedAt: { not: null }, | |
| }, | |
| }), | |
| prisma.relationship.count({ | |
| where: { | |
| entity_id: entityId, | |
| NOT: { status: "new" }, // anything other than 'new' blocks deletion | |
| }, | |
| }), | |
| ]); | |
| const safeToDeleteEntity = submittedTokenCount === 0 && nonNewRelCount === 0; | |
| const [submittedTokenCount, nonNewRelCount, otherUnsubmittedTokenCount] = await Promise.all([ | |
| prisma.publicFormsTokens.count({ | |
| where: { | |
| entityId, | |
| submittedAt: { not: null }, | |
| }, | |
| }), | |
| prisma.relationship.count({ | |
| where: { | |
| entity_id: entityId, | |
| NOT: { status: "new" }, // anything other than 'new' blocks deletion | |
| }, | |
| }), | |
| prisma.publicFormsTokens.count({ | |
| where: { | |
| entityId, | |
| submittedAt: null, | |
| token: { not: token.token }, // exclude the token we're processing | |
| }, | |
| }), | |
| ]); | |
| const safeToDeleteEntity = | |
| submittedTokenCount === 0 && | |
| nonNewRelCount === 0 && | |
| otherUnsubmittedTokenCount === 0; |
🤖 Prompt for AI Agents
In tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts around lines 57 to 73,
the safety check only counts submitted tokens and non‑new relationships but
misses other unsubmitted tokens for the same entity, which can cause FK delete
failures; add a third Promise.count that counts publicFormsTokens with the same
entityId, submittedAt: null, and id !== currentTokenId (exclude the current
token being deleted), include that count in the Promise.all result, and only
mark safeToDeleteEntity true when submittedTokenCount === 0, nonNewRelCount ===
0, and otherUnsubmittedCount === 0; update the safeToDeleteEntity condition
accordingly.
1 participant
PR Description
This PR addresses critical issues in the unsubmitted_forms scheduled job to ensure safe and reliable cleanup of entities and tokens:
Fixes
Improvements
Reviewer Notes
Summary by CodeRabbit
Bug Fixes
Documentation
Chores