Skip to content

Fix(form-cleanup): improve token cleanup with batching and absolute time handling #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
se348 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix(form-cleanup): improve token cleanup with batching and absolute time handling #9

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
783105e
fix: cleanup unsubmitted forms with batch deletion and correct time h…
se348 Sep 4, 2025
8f40897
fix: fetch from db fields that are used only.
se348 Sep 5, 2025
aef7ad2
chore: add detailed comments.
se348 Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 67 additions & 65 deletions tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,84 +1,86 @@
/* Context:
This is a scheduled job that runs every day at midnight to clean up forms that users started filling in but didn't submit which are older than 7 days.
When a user visits a public form, a token is generated and stored in the database.
This token is used to identify the user and link the answers to the entity.
An entity is the owner of data in the database, separated as it could be a business or an individual but has been decoupled from a login/user.
If the user does not submit the form, the token and the entity should be deleted after 7 days.
This is to prevent the database from being cluttered with unused tokens and entities.
*/

/* Task Instructions:
* 1. Read and understand the code below
* 2. Identify ALL issues in the code (there are multiple)
* 3. Fix the issues and create a working solution
* 4. Create a PR with clear commit messages
* 5. Record a 3-5 minute Loom video explaining:
* - What issues you found
* - How you fixed them
* - Any trade-offs you considered
*
* Focus on: correctness, performance, error handling, and code clarity
* Expected time: 45-60 minutes
*/

// For the purpose of this test you can ignore that the imports are not working.
import type { JobScheduleQueue } from "@prisma/client";
import { prisma } from "../endpoints/middleware/prisma";
import { update_job_status } from "./generic_scheduler";
import { update_job_status } from "./generic_scheduler";
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify import paths resolve from test location.

From tests/, check that ../endpoints/middleware/prisma and ./generic_scheduler exist and export the expected symbols.

Run:

🏁 Script executed:

#!/bin/bash
set -euo pipefail
# prisma export
rg -nP 'export\s+(const|let|var)\s+prisma\b' -C2
# generic_scheduler export
rg -nP 'export\s+(async\s+)?function\s+update_job_status\b|export\s+const\s+update_job_status\b' -C2

Length of output: 70

🏁 Script executed:

#!/bin/bash
# Display imports in the test file
echo "---- Imports in tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts ----"
grep -n "import" -C1 tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts || true

# Check for generic_scheduler file relative to test location
echo -e "\n---- File existence: generic_scheduler relative to test ----"
ls -l tests/unsubmitted_forms/generic_scheduler.ts tests/unsubmitted_forms/generic_scheduler.js 2>/dev/null || echo "No generic_scheduler.* in tests/unsubmitted_forms"

# Search for generic_scheduler.ts in project
echo -e "\n---- Find generic_scheduler.ts in project ----"
find . -maxdepth 4 -type f -name "generic_scheduler.ts" 2>/dev/null || echo "generic_scheduler.ts not found"

# Check export of update_job_status in generic_scheduler files
echo -e "\n---- Exports of update_job_status ----"
grep -R "export.*update_job_status" -n tests/unsubmitted_forms/generic_scheduler* src/generic_scheduler.ts* 2>/dev/null || echo "No export update_job_status found"

# Check for prisma file resolution relative to test location
echo -e "\n---- File existence: prisma relative to test ----"
ls -l tests/unsubmitted_forms/../endpoints/middleware/prisma.ts tests/unsubmitted_forms/../endpoints/middleware/prisma/index.ts 2>/dev/null || echo "No prisma file at tests/unsubmitted_forms/../endpoints/middleware"

# Find actual prisma file in project
echo -e "\n---- Find prisma files in project ----"
find . -type f | grep -E "endpoints/middleware/prisma(\.ts|/index\.ts)" || echo "Prisma file not found"

# Check exports in prisma file
echo -e "\n---- Exports in endpoints/middleware/prisma.ts ----"
grep -R "export" -n endpoints/middleware/prisma.ts 2>/dev/null || echo "No exports found"

Length of output: 2022

Fix broken imports in tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts
Lines 2–3: the paths ../endpoints/middleware/prisma and ./generic_scheduler don’t exist—update these to the correct relative locations of the modules that export prisma and update_job_status so the imports resolve.

🤖 Prompt for AI Agents 
In tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts around lines 2–3, the
imports for prisma and update_job_status reference non-existent paths; locate
the actual modules that export `prisma` (previously referenced as
../endpoints/middleware/prisma) and `update_job_status` (previously
./generic_scheduler) and replace those import paths with the correct relative
paths from this test file to those module files, ensuring the exported names
match (named vs default) and updating the import syntax if needed so the imports
resolve correctly.


type RelationshipStatus = "new" | "submitted";
type JobStatus = "completed" | "failed";

// Number of records to process in one batch.
// This value can be fine-tuned depending on:
// - database size
// - indexes available
// - workload characteristics
// Larger batches = fewer transactions but more memory/lock contention.
// Smaller batches = safer but potentially slower overall.
// Reasoning behind it is in my pr description.
const BATCH_SIZE = 20
const EXPIRATION_DAYS = 7

/**
* Cleans up expired unsubmitted public form tokens and all associated data.
*
* This scheduled job runs in batches and performs deletions:
* 1. Deletes all corpus items linked to the entities
* 2. Deletes all relationships linked to the batch of tokens
* 3. Deletes all expired tokens in the batch
* 4. Deletes all associated entities
*
* The function uses absolute date calculation (midnight 7 days ago) to avoid missing tokens
* if the job is delayed or re-queued. Batches reduce database load by minimizing repeated
* opening and closing of connections, while still ensuring correctness.
*
* @param {JobScheduleQueue} job - The scheduled job object containing job metadata.
* @throws Will throw an error if any database operation fails, updating the job status to 'failed'.
*/
export const cleanup_unsubmitted_forms = async (job: JobScheduleQueue) => {
try {
//Find forms that were created 7 days ago and have not been submitted
const sevenDaysAgo = new Date(Date.now() - 7 * 24 * 60 * 60);
const sevenDaysAgoPlusOneDay = new Date(
sevenDaysAgo.getTime() + 24 * 60 * 60 * 1000
);
// Calculate absolute EXPIRATION_DAYS day-old date (midnight EXPIRATION_DAYS days ago)
const today = new Date();
today.setHours(0, 0, 0, 0);
const sevenDaysAgo = new Date(today.getTime() - EXPIRATION_DAYS * 24 * 60 * 60 * 1000);

Comment on lines +37 to 41
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

UTC cutoff to avoid local-time skew; rename to cutoff.

setHours(0,0,0,0) uses local time; if DB stores UTC, this shifts by timezone and can delete too much/too little.

Apply:

-    const today = new Date();
-    today.setHours(0, 0, 0, 0);
-    const sevenDaysAgo = new Date(today.getTime() - EXPIRATION_DAYS * 24 * 60 * 60 * 1000);
+    const cutoff = new Date();
+    cutoff.setUTCHours(0, 0, 0, 0);
+    cutoff.setUTCDate(cutoff.getUTCDate() - EXPIRATION_DAYS);

And use it in the query:

-        where: { createdAt: { lt: sevenDaysAgo } },
+        where: { createdAt: { lt: cutoff } },

Also applies to: 44-44

const expiredTokens = await prisma.publicFormsTokens.findMany({
where: {
createdAt: {
gte: sevenDaysAgo, // greater than or equal to 7 days ago
lt: sevenDaysAgoPlusOneDay, // but less than 7 days ago + 1 day
while (true) {
const expiredTokens = await prisma.publicFormsTokens.findMany({
where: { createdAt: { lt: sevenDaysAgo } },
select: {
entityId: true,
token: true,
productId: true,
},
},
});
take: BATCH_SIZE,
});

for (const token of expiredTokens) {
const relationship = await prisma.relationship.findFirst({
if (expiredTokens.length === 0) break; // exit when no more expired tokens

const entityIds = expiredTokens
.map((t) => t.entityId)
.filter(Boolean) as string[];
const tokenValues = expiredTokens.map((t) => t.token);

const relationships = await prisma.relationship.findMany({
where: {
product_id: token.productId,
status: "new",
product_id: { in: expiredTokens.map((t) => t.productId) },
status: "new" as RelationshipStatus,
},
select: {
id: true,
},
});

if (relationship) {
await prisma.$transaction([
// Delete relationship
prisma.relationship.delete({
where: { id: relationship.id },
}),
// // Delete the token
prisma.publicFormsTokens.delete({
where: { token: token.token },
}),
// Delete all corpus items associated with the entity
prisma.new_corpus.deleteMany({
where: {
entity_id: token.entityId || "",
},
}),
// Delete the entity (company)
prisma.entity.delete({
where: { id: token.entityId || "" },
}),
]);
}
await prisma.$transaction([
prisma.new_corpus.deleteMany({ where: { entity_id: { in: entityIds } } }),
prisma.relationship.deleteMany({ where: { id: { in: relationships.map((r) => r.id) } } }),
prisma.publicFormsTokens.deleteMany({ where: { token: { in: tokenValues } } }),
prisma.entity.deleteMany({ where: { id: { in: entityIds } } }),
]);
}

await update_job_status(job.id, "completed");
await update_job_status(job.id, "completed" as JobStatus);
} catch (error) {
// TODO: This is not a good way to loggging.
// We should use loggers like winston. Check my pr description for more.
console.error("Error cleaning up unsubmitted forms:", error);
await update_job_status(job.id, "failed");
await update_job_status(job.id, "failed" as JobStatus);
throw error;
}
};