feat(timetable): add cleanup for orphaned cohorts#243
Open
Dasa122 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughAdds a ChangesOrphaned Cohort Cleanup
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as _router.ts
participant Handler as cleanupOrphanedCohortsHandler
participant Utility as cleanupOrphanedCohorts
participant DB as Database
Client->>Router: POST /timetables/cleanup-orphaned-cohorts
Router->>Handler: delegate
Handler->>Utility: cleanupOrphanedCohorts()
Utility->>DB: SELECT DISTINCT cohortId FROM cohortTimetableMtm
Utility->>DB: SELECT id FROM cohort
Utility->>DB: UPDATE user SET cohortId=NULL WHERE cohortId IN orphaned
Utility->>DB: DELETE FROM cohort WHERE id IN orphaned
Utility-->>Handler: OrphanCleanupSummary
Handler-->>Client: 200 { deletedCohortIds, affectedUserCount }
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: 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 |
Member
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/chronos/src/routes/timetable/index.ts (1)
322-333: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a single conditional update to prevent clobbering concurrent cohort changes.
Line 322–333 has the same select-then-update-by-ID pattern. A user reassigned to a valid cohort between those statements can be incorrectly nulled, and then incorrectly notified.
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)); - notifiedUserIds.push(...userIds); - } + const updatedUsers = await tx + .update(user) + .set({ cohortId: null }) + .where(inArray(user.cohortId, orphanedCohortIds)) + .returning({ id: user.id }); + if (updatedUsers.length > 0) { + notifiedUserIds.push(...updatedUsers.map((u) => u.id)); + }🤖 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/routes/timetable/index.ts` around lines 322 - 333, There is a race condition in the select-then-update pattern where users with orphaned cohorts are first queried and then updated by ID. Between the select and update, another transaction could reassign a user to a valid cohort, causing the update to incorrectly null out that valid cohort assignment. Refactor the code to combine the select and update into a single conditional update statement by including the original orphaned cohort condition (inArray(user.cohortId, orphanedCohortIds)) directly in the WHERE clause of the update operation, and capture the affected row count to determine which users were actually updated and should be notified.
🤖 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/chronos/src/utils/timetable/cleanup.ts`:
- Around line 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.
- Around line 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.
---
Outside diff comments:
In `@apps/chronos/src/routes/timetable/index.ts`:
- Around line 322-333: There is a race condition in the select-then-update
pattern where users with orphaned cohorts are first queried and then updated by
ID. Between the select and update, another transaction could reassign a user to
a valid cohort, causing the update to incorrectly null out that valid cohort
assignment. Refactor the code to combine the select and update into a single
conditional update statement by including the original orphaned cohort condition
(inArray(user.cohortId, orphanedCohortIds)) directly in the WHERE clause of the
update operation, and capture the affected row count to determine which users
were actually updated and should be notified.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c727da05-eee7-4d5a-8363-af38af9193a1
📒 Files selected for processing (3)
apps/chronos/src/routes/timetable/_router.tsapps/chronos/src/routes/timetable/index.tsapps/chronos/src/utils/timetable/cleanup.ts
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.23.0)
apps/chronos/src/routes/timetable/index.ts
[WARNING] 527-527: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
🔇 Additional comments (2)
apps/chronos/src/routes/timetable/_router.ts (1)
5-5: LGTM!Also applies to: 51-54
apps/chronos/src/routes/timetable/index.ts (1)
336-339: 🗄️ Data Integrity & IntegrationNo action needed — FK delete order is correct.
The
cohortTimetableMtmtable hasonDelete: 'cascade'on bothcohortId(line 158) andtimetableId(line 161) foreign keys. Deletingcohortbeforetimetableis entirely safe because:
- Deleting
cohortcascades deletion of matchingcohortTimetableMtmentries via thecohortIdFK.- Deleting
timetablecascades deletion of remainingcohortTimetableMtmentries via thetimetableIdFK.- No reverse constraint exists (timetable does not reference cohort).
The explicit deletion of orphaned cohorts is intentional and does not violate any constraints.
> Likely an incorrect or invalid review comment.
Comment on lines
+26
to
+38
| 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)); |
There was a problem hiding this comment.
🗄️ 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.
Comment on lines
+51
to
+63
| 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 }); |
There was a problem hiding this comment.
🗄️ 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a new feature to clean up orphaned cohorts—cohorts that are no longer linked to any timetable—and ensures users referencing these cohorts are updated accordingly. It also updates the timetable deletion process to remove orphaned cohorts automatically and adds a dedicated API endpoint for manual cleanup. The documentation for affected endpoints has been improved to reflect these changes.
Cohort Cleanup Functionality:
cleanupOrphanedCohortsincleanup.tsthat finds cohorts not linked to any timetable, nullifiescohortIdfor users referencing them, and deletes the orphaned cohorts in a single transaction./timetables/cleanup-orphaned-cohortswith handlercleanupOrphanedCohortsHandlerto trigger this cleanup manually; it returns a summary of affected users and deleted cohorts. [1] [2] [3]Timetable Deletion Improvements:
closes #200
Summary by CodeRabbit
New Features
Improvements