Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions apps/chronos/src/routes/timetable/_router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { timetableFactory } from '#routes/timetable/_factory';
import { getCohortsForTimetable } from '#routes/timetable/cohort';
import { importRoute } from '#routes/timetable/import';
import {
cleanupOrphanedCohortsHandler,
deleteTimetable,
getAllTimetables,
getAllValidTimetables,
Expand Down Expand Up @@ -47,6 +48,10 @@ export const timetableRouter = timetableFactory
.patch('/timetables/:id', ...updateTimetable)
.get('/timetables/:id/preview-delete', ...previewDeleteTimetable)
.delete('/timetables/:id', ...deleteTimetable)
.post(
'/timetables/cleanup-orphaned-cohorts',
...cleanupOrphanedCohortsHandler
)
.post('/import', ...importRoute)
// Substitution routes
.get('/substitutions', ...getAllSubstitutions)
Expand Down
53 changes: 51 additions & 2 deletions apps/chronos/src/routes/timetable/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { requireAuthentication, requireAuthorization } from '#middleware/auth';
import { dispatchImmediateNotification } from '#utils/notifications/engine';
import { filcExt } from '#utils/openapi';
import { getActiveTimetableId } from '#utils/timetable/active';
import { cleanupOrphanedCohorts } from '#utils/timetable/cleanup';
import { dateToYYYYMMDD } from '#utils/timetable/date';
import { createSelectSchema } from '#utils/zod';
import { timetableFactory } from './_factory';
Expand Down Expand Up @@ -247,7 +248,7 @@ export const deleteTimetable = timetableFactory.createHandlers(
describeRoute({
...filcExt('Timetable', '@unit Timetable', true),
description:
'Delete a timetable and all its related data. Cohorts survive.',
'Delete a timetable and all its related data, including orphaned cohorts.',
responses: {
200: {
content: {
Expand Down Expand Up @@ -330,6 +331,9 @@ export const deleteTimetable = timetableFactory.createHandlers(
.where(inArray(user.id, userIds));
notifiedUserIds.push(...userIds);
}

// Delete orphaned cohorts that are no longer linked to any timetable
await tx.delete(cohort).where(inArray(cohort.id, orphanedCohortIds));
}

await tx.delete(timetable).where(eq(timetable.id, id));
Expand Down Expand Up @@ -374,7 +378,8 @@ const previewDeleteResponseSchema = z.object({
export const previewDeleteTimetable = timetableFactory.createHandlers(
describeRoute({
...filcExt('Timetable', '@unit Timetable', true),
description: 'Preview the impact of deleting a timetable.',
description:
'Preview the impact of deleting a timetable. Orphaned cohorts will be deleted along with the timetable.',
responses: {
200: {
content: {
Expand Down Expand Up @@ -513,3 +518,47 @@ export const previewDeleteTimetable = timetableFactory.createHandlers(
});
}
);

const cleanupOrphanedCohortsResponseSchema = z.object({
data: z.object({
affectedUserCount: z.number().int(),
deletedCohortIds: z.array(z.string()),
}),
success: z.literal(true),
});

export const cleanupOrphanedCohortsHandler = timetableFactory.createHandlers(
describeRoute({
...filcExt('Timetable', '@unit Timetable', true),
description:
'Delete all cohorts that are no longer linked to any timetable (orphaned). Users referencing those cohorts will have their cohortId nullified.',
responses: {
200: {
content: {
'application/json': {
schema: resolver(cleanupOrphanedCohortsResponseSchema),
},
},
description: 'Cleanup summary',
},
},
tags: ['Timetable'],
}),
requireAuthentication,
requireAuthorization('import:timetable'),
async (c) => {
try {
const summary = await cleanupOrphanedCohorts();

return c.json<SuccessResponse<typeof summary>>({
data: summary,
success: true,
});
} catch (error) {
logger.error('Failed to cleanup orphaned cohorts: ', { error });
throw new HTTPException(StatusCodes.INTERNAL_SERVER_ERROR, {
message: 'Failed to cleanup orphaned cohorts',
});
}
}
);
74 changes: 74 additions & 0 deletions apps/chronos/src/utils/timetable/cleanup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { getLogger } from '@logtape/logtape';
import { inArray } from 'drizzle-orm';
import { db } from '#database';
import { user } from '#database/schema/authentication';
import { cohort, cohortTimetableMtm } from '#database/schema/timetable';

const logger = getLogger(['chronos', 'timetable', 'cleanup']);

export type OrphanCleanupSummary = {
/** IDs of the deleted cohort rows. */
deletedCohortIds: string[];
/** Number of users whose cohortId was nullified. */
affectedUserCount: number;
};

/**
* Find and delete cohorts that are not linked to any timetable via the
* `cohort_timetable_mtm` table. For each orphan, any user referencing that
* cohort has their `cohortId` set to NULL before the cohort row is removed.
*
* The destructive part of the operation (user nullification + cohort deletion)
* runs inside a single database transaction.
*/
export async function cleanupOrphanedCohorts(): Promise<OrphanCleanupSummary> {
// Find all cohort IDs that are referenced in the MTM table
const linkedRows = await db
.selectDistinct({ cohortId: cohortTimetableMtm.cohortId })
.from(cohortTimetableMtm);

const linkedSet = new Set(linkedRows.map((r) => r.cohortId));

// Find all cohort IDs
const allCohortRows = await db.select({ id: cohort.id }).from(cohort);

// Cohorts whose ID does not appear in the MTM table are orphaned
const orphanedCohortIds = allCohortRows
.map((r) => r.id)
.filter((id) => !linkedSet.has(id));
Comment on lines +26 to +38

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Move orphan discovery into the same transaction as the destructive writes.

Line 26–38 computes orphanedCohortIds before Line 49 starts the transaction. A concurrent link insert/delete can stale that set and make the subsequent nullify/delete act on outdated membership.

Proposed fix
 export async function cleanupOrphanedCohorts(): Promise<OrphanCleanupSummary> {
-  // Find all cohort IDs that are referenced in the MTM table
-  const linkedRows = await db
-    .selectDistinct({ cohortId: cohortTimetableMtm.cohortId })
-    .from(cohortTimetableMtm);
-
-  const linkedSet = new Set(linkedRows.map((r) => r.cohortId));
-
-  // Find all cohort IDs
-  const allCohortRows = await db.select({ id: cohort.id }).from(cohort);
-
-  // Cohorts whose ID does not appear in the MTM table are orphaned
-  const orphanedCohortIds = allCohortRows
-    .map((r) => r.id)
-    .filter((id) => !linkedSet.has(id));
-
-  if (orphanedCohortIds.length === 0) {
-    logger.info('No orphaned cohorts found.');
-    return { affectedUserCount: 0, deletedCohortIds: [] };
-  }
-
-  logger.info('Found orphaned cohorts', { count: orphanedCohortIds.length });
-
   let affectedUserCount = 0;
+  let deletedCohortIds: string[] = [];
 
   await db.transaction(async (tx) => {
+    const linkedRows = await tx
+      .selectDistinct({ cohortId: cohortTimetableMtm.cohortId })
+      .from(cohortTimetableMtm);
+    const linkedSet = new Set(linkedRows.map((r) => r.cohortId));
+
+    const allCohortRows = await tx.select({ id: cohort.id }).from(cohort);
+    const orphanedCohortIds = allCohortRows
+      .map((r) => r.id)
+      .filter((id) => !linkedSet.has(id));
+
+    if (orphanedCohortIds.length === 0) {
+      logger.info('No orphaned cohorts found.');
+      return;
+    }
+
+    deletedCohortIds = orphanedCohortIds;
+
     // Nullify cohortId on users referencing orphaned cohorts
     const affectedUsers = await tx
       .select({ id: user.id })
@@
     await tx.delete(cohort).where(inArray(cohort.id, orphanedCohortIds));
     logger.info('Deleted orphaned cohorts', {
       count: orphanedCohortIds.length,
     });
   });
 
-  return { affectedUserCount, deletedCohortIds: orphanedCohortIds };
+  return { affectedUserCount, deletedCohortIds };
 }

Also applies to: 49-71

🤖 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/chronos/src/utils/timetable/cleanup.ts` around lines 26 - 38, The orphan
discovery logic that computes linkedSet, allCohortRows, and orphanedCohortIds is
executed outside the database transaction that begins at line 49, creating a
race condition where concurrent link operations can make the orphanedCohortIds
set stale before the destructive writes execute. Move the entire orphan
discovery block starting with the selectDistinct call for linkedRows through the
filter that computes orphanedCohortIds into the same transaction that contains
the subsequent nullify and delete operations, ensuring the discovery and writes
happen atomically and safely.


if (orphanedCohortIds.length === 0) {
logger.info('No orphaned cohorts found.');
return { affectedUserCount: 0, deletedCohortIds: [] };
}

logger.info('Found orphaned cohorts', { count: orphanedCohortIds.length });

let affectedUserCount = 0;

await db.transaction(async (tx) => {
// Nullify cohortId on users referencing orphaned cohorts
const affectedUsers = await tx
.select({ id: user.id })
.from(user)
.where(inArray(user.cohortId, orphanedCohortIds));

if (affectedUsers.length > 0) {
const userIds = affectedUsers.map((u) => u.id);
await tx
.update(user)
.set({ cohortId: null })
.where(inArray(user.id, userIds));
affectedUserCount = userIds.length;
logger.info('Nullified cohortId for users', { count: userIds.length });
Comment on lines +51 to +63

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Avoid stale-ID updates when nullifying user cohort assignments.

Line 51–63 selects users, then updates by user.id. If a user’s cohortId changes between those statements, this can nullify a newly assigned valid cohort. Update directly with the cohortId IN orphanedCohortIds predicate and collect IDs from returning().

Proposed fix
-    const affectedUsers = await tx
-      .select({ id: user.id })
-      .from(user)
-      .where(inArray(user.cohortId, orphanedCohortIds));
-
-    if (affectedUsers.length > 0) {
-      const userIds = affectedUsers.map((u) => u.id);
-      await tx
-        .update(user)
-        .set({ cohortId: null })
-        .where(inArray(user.id, userIds));
-      affectedUserCount = userIds.length;
-      logger.info('Nullified cohortId for users', { count: userIds.length });
-    }
+    const updatedUsers = await tx
+      .update(user)
+      .set({ cohortId: null })
+      .where(inArray(user.cohortId, orphanedCohortIds))
+      .returning({ id: user.id });
+
+    affectedUserCount = updatedUsers.length;
+    if (affectedUserCount > 0) {
+      logger.info('Nullified cohortId for users', { count: affectedUserCount });
+    }
🤖 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/chronos/src/utils/timetable/cleanup.ts` around lines 51 - 63, Remove the
separate select query that collects affected user IDs first. Instead, directly
update the user table where cohortId is in orphanedCohortIds, and use the
returning() method to collect the affected userIds from the update result. This
eliminates the race condition where a user's cohortId could be assigned a valid
value between the select and update operations, ensuring only users currently
having orphaned cohortIds get nullified.

}

// Delete the orphaned cohort rows
await tx.delete(cohort).where(inArray(cohort.id, orphanedCohortIds));
logger.info('Deleted orphaned cohorts', {
count: orphanedCohortIds.length,
});
});

return { affectedUserCount, deletedCohortIds: orphanedCohortIds };
}