feat: cron to enqueue inactive organizations for deletion #2418
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughAdds two BullMQ queues/workers for notifying and scheduling deletion of inactive organizations, integrates them into build-server, makes organization deletion scheduling configurable, wraps deleteOrganization in a DB transaction, standardizes stalled-job log keys, and updates BullMQ/ioredis versions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
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: 5
🧹 Nitpick comments (5)
controlplane/src/bin/db-cleanup.ts (2)
88-101: Remove@ts-ignoreand simplify chunk logic.The
@ts-ignoresuppresses a type error for a condition that's always false (MAX_DEGREE_OF_PARALLELISM === 1when it's defined as5). Consider removing the special case or making it configurable if the single-threaded path is needed.function chunkArray<T>(data: T[]): T[][] { - // @ts-ignore - if (MAX_DEGREE_OF_PARALLELISM === 1) { - return [data]; - } - const chunks: T[][] = []; const organizationsPerChunk = Math.ceil(ORGANIZATIONS_PER_BUCKET / MAX_DEGREE_OF_PARALLELISM); for (let i = 0; i < data.length; i += organizationsPerChunk) { chunks.push(data.slice(i, i + organizationsPerChunk)); } - return chunks; }
117-117: Consider using the pino logger consistently.A pino logger is created on line 62, but the script uses
console.log/console.errorfor output (lines 117, 134, 138, 141, 156, 189). For consistency with the rest of the codebase and better structured logging, consider using the pino logger throughout.Also applies to: 138-138, 141-141, 156-156, 189-189
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (2)
86-87: UsegetOrganizationAdminsinstead of filtering all members.
OrganizationRepositoryhas agetOrganizationAdminsmethod that directly returns admin members. This avoids loading RBAC data for all members just to filter.- const organizationMembers = await orgRepo.getMembers({ organizationID: org.id }); - const orgAdmins = organizationMembers.filter((m) => m.rbac.isOrganizationAdmin); + const orgAdmins = await orgRepo.getOrganizationAdmins({ organizationID: org.id });
100-100: Avoid directprocess.envaccess; passwebBaseUrlvia configuration.The worker accesses
process.env.WEB_BASE_URLdirectly, which is inconsistent with other workers that receive configuration through their input options. This also makes testing harder.+// In createNotifyOrganizationDeletionQueuedWorker input: + webBaseUrl: string; // In handler: - restoreLink: `${process.env.WEB_BASE_URL}/${org.slug}/settings`, + restoreLink: `${this.input.webBaseUrl}/${org.slug}/settings`,controlplane/src/core/build-server.ts (1)
406-411: PasswebBaseUrlto the worker for consistency.The worker uses
process.env.WEB_BASE_URLdirectly, butopts.auth.webBaseUrlis available here. Pass it to maintain consistency with how other components receive configuration.createNotifyOrganizationDeletionQueuedWorker({ redisConnection: fastify.redisForWorker, db: fastify.db, logger, mailer: mailerClient, + webBaseUrl: opts.auth.webBaseUrl, }),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controlplane/src/bin/db-cleanup.ts(1 hunks)controlplane/src/core/build-server.ts(4 hunks)controlplane/src/core/repositories/OrganizationRepository.ts(2 hunks)controlplane/src/core/routes.ts(2 hunks)controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/bin/db-cleanup.ts
🧬 Code graph analysis (5)
controlplane/src/core/repositories/OrganizationRepository.ts (1)
controlplane/src/core/constants.ts (1)
delayForManualOrgDeletionInDays(10-10)
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (3)
controlplane/src/core/workers/Worker.ts (2)
IQueue(3-7)IWorker(9-11)controlplane/src/core/services/Mailer.ts (1)
Mailer(13-101)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1681)
controlplane/src/bin/db-cleanup.ts (5)
controlplane/src/core/plugins/redis.ts (1)
createRedisConnections(29-86)controlplane/src/core/workers/DeleteOrganizationWorker.ts (1)
DeleteOrganizationQueue(20-62)controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (1)
NotifyOrganizationDeletionQueuedQueue(18-60)controlplane/src/db/schema.ts (2)
organizations(1266-1289)auditLogs(1936-1972)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1681)
controlplane/src/core/routes.ts (1)
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (1)
NotifyOrganizationDeletionQueuedQueue(18-60)
controlplane/src/core/build-server.ts (1)
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (2)
NotifyOrganizationDeletionQueuedQueue(18-60)createNotifyOrganizationDeletionQueuedWorker(112-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
controlplane/src/core/repositories/OrganizationRepository.ts (1)
942-970: LGTM! Clean extension of deletion scheduling.The optional
deleteDelayInDaysparameter provides flexibility for different deletion workflows while maintaining backward compatibility by falling back todelayForManualOrgDeletionInDays.controlplane/src/core/routes.ts (1)
24-24: LGTM! Consistent queue wiring.The new queue follows the established pattern for queue registration in RouterOptions.
Also applies to: 52-52
controlplane/src/bin/db-cleanup.ts (2)
114-116: Clarify thestartOfMonthusage for inactivity threshold.Using
startOfMonth(subDays(now, MIN_INACTIVITY_DAYS))creates a threshold that varies depending on the current day of the month. For example, running on Dec 16 vs Dec 1 yields different threshold dates. Was this intentional for batch alignment, or should it simply besubDays(now, MIN_INACTIVITY_DAYS)?
49-54: The review comment is incorrect. The code properly handles Redis TLS configuration:
redis.hostis never undefined due to the default valueprocess.env.REDIS_HOST || 'localhost'inget-config.ts, making the!assertion on line 50 redundant but harmless.The
tlsproperty is correctly handled as optional. Inget-config.ts, it's conditionally set to an object only when TLS environment variables are present, otherwiseundefined. TheRedisPluginOptionsinterface inredis.tsproperly definestlsas optional with all its properties optional.
createRedisConnectionssafely checks each TLS property before use (lines 45, 49, 54 in redis.ts), validating file paths before reading.No validation is needed because TLS configuration is properly optional and defensively implemented.
Likely an incorrect or invalid review comment.
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (1)
18-60: LGTM! Queue implementation follows established patterns.The queue configuration with exponential backoff and job retention is consistent with other queues in the codebase.
controlplane/src/core/build-server.ts (1)
401-412: LGTM! Queue and worker wiring follows established patterns.The new notification queue and worker are registered consistently with other queues in the build server, using the same connection patterns and passing the mailer client appropriately.
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (2)
76-78: Throwing error when mailer not configured causes unnecessary retries.When the mailer is not configured, throwing an error triggers the retry mechanism (6 attempts with exponential backoff). This will cause the job to fail repeatedly for approximately 1.3 hours before giving up, wasting resources.
Consider logging a warning and returning early instead:
if (!this.input.mailer) { - throw new Error('Mailer service not configured'); + this.input.logger.warn('Mailer service not configured, skipping notification'); + return; }
87-101: Handle case where organization has no admins.If
orgAdminsis empty (line 87), the email will be sent with an emptyreceiverEmailsarray (line 95). Consider adding a guard to skip the email or log a warning:const orgAdmins = organizationMembers.filter((m) => m.rbac.isOrganizationAdmin); + + if (orgAdmins.length === 0) { + this.input.logger.warn({ organizationId: org.id }, 'No admins found for organization, skipping notification'); + return; + } const intl = Intl.DateTimeFormat(undefined, {
🧹 Nitpick comments (3)
controlplane/src/core/workers/ReactivateOrganizationWorker.ts (1)
115-116: LGTM! Typo fix aligns logging with other workers.The change from
joinIdtojobIdcorrects a typo and ensures consistent logging across workers.Optional: Rename the parameter for clarity
The
jobparameter in the stalled event callback is actually the job ID string, not a Job object. Consider renaming it tojobIdfor clarity:- worker.on('stalled', (job) => { - log.warn({ jobId: job }, `Job stalled`); + worker.on('stalled', (jobId) => { + log.warn({ jobId }, `Job stalled`); });Based on learnings, this change aligns with similar updates across other workers in the PR.
controlplane/src/core/workers/DeactivateOrganizationWorker.ts (1)
127-129: LGTM! Key name fix improves consistency.The change from
joinIdtojobIdcorrectly aligns the log key with the actual value being logged and standardizes the approach across workers.Optional: Rename parameter for clarity
The callback parameter
jobreceives the jobId string (per BullMQ's stalled event signature), not a Job object. Consider renaming it tojobIdfor clarity:- worker.on('stalled', (job) => { - log.warn({ jobId: job }, `Job stalled`); + worker.on('stalled', (jobId) => { + log.warn({ jobId }, `Job stalled`); });controlplane/src/core/workers/CacheWarmerWorker.ts (1)
133-135: Good typo fix; consider logging just the job ID for consistency.The key name correction from
joinIdtojobIdis excellent and aligns with the broader pattern across workers.For consistency with the error handler on line 112 (which logs
jobId: job.id), consider logging just the job ID rather than the entire job object. This keeps logs concise and matches the established pattern in this file.🔎 Optional refinement for consistency:
- log.warn({ jobId: job }, `Job stalled`); + log.warn({ jobId: job.id }, `Job stalled`);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
controlplane/src/bin/db-cleanup.ts(1 hunks)controlplane/src/core/bufservices/organization/deleteOrganization.ts(1 hunks)controlplane/src/core/repositories/OrganizationRepository.ts(1 hunks)controlplane/src/core/workers/CacheWarmerWorker.ts(1 hunks)controlplane/src/core/workers/DeactivateOrganizationWorker.ts(1 hunks)controlplane/src/core/workers/DeleteOrganizationAuditLogsWorker.ts(1 hunks)controlplane/src/core/workers/DeleteOrganizationWorker.ts(1 hunks)controlplane/src/core/workers/DeleteUserQueue.ts(1 hunks)controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts(1 hunks)controlplane/src/core/workers/ReactivateOrganizationWorker.ts(1 hunks)controlplane/test/test-util.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- controlplane/src/core/workers/DeleteOrganizationAuditLogsWorker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/bin/db-cleanup.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/bufservices/organization/deleteOrganization.ts
📚 Learning: 2025-07-01T13:53:54.146Z
Learnt from: wilsonrivera
Repo: wundergraph/cosmo PR: 1919
File: controlplane/src/core/repositories/OrganizationGroupRepository.ts:193-224
Timestamp: 2025-07-01T13:53:54.146Z
Learning: In the Cosmo codebase, database transactions are typically managed at the service layer (e.g., in buf services like deleteOrganizationGroup.ts), where repositories are instantiated with the transaction handle and all operations within those repositories are automatically part of the same transaction.
Applied to files:
controlplane/src/core/bufservices/organization/deleteOrganization.ts
🧬 Code graph analysis (2)
controlplane/src/core/repositories/OrganizationRepository.ts (2)
controlplane/src/core/workers/DeleteOrganizationWorker.ts (1)
DeleteOrganizationQueue(20-62)controlplane/src/core/constants.ts (1)
delayForManualOrgDeletionInDays(10-10)
controlplane/test/test-util.ts (1)
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (1)
NotifyOrganizationDeletionQueuedQueue(18-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
controlplane/src/core/workers/DeleteUserQueue.ts (1)
143-143: LGTM: Logging key corrected.The change from
joinIdtojobIdcorrectly aligns the log metadata with the actual job identifier, consistent with similar corrections across other workers in this PR.controlplane/src/core/workers/DeleteOrganizationWorker.ts (1)
154-154: LGTM: Logging key corrected.The change from
joinIdtojobIdcorrectly aligns the log metadata with the actual job identifier, matching the pattern applied across other workers.controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (1)
128-128: LGTM: Logging key is correct.The stalled event handler correctly uses
jobIdfor the log metadata.controlplane/src/core/bufservices/organization/deleteOrganization.ts (1)
28-140: Transaction scope correctly established.The transaction wrapper correctly ensures all repository operations execute atomically. The repository instantiation with the transaction handle (lines 29-31) follows the established pattern for this codebase.
However, note that
queueOrganizationDeletion(lines 100-104) performs a queue operation (addJob) that cannot be rolled back if a subsequent operation within this transaction fails. This is acceptable for notification-type queues but worth documenting.Based on learnings, the transactional approach at the service layer is appropriate for coordinating multiple repository operations.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
controlplane/src/bin/db-cleanup.ts (1)
149-161: Critical: Nested transaction and Redis atomicity issues remain unaddressed.This is the same issue previously flagged: the outer transaction at line 151 wraps
processChunkOfOrganizations, but within that function,orgRepo.queueOrganizationDeletion(line 199) starts its own transaction, creating nested transactions. Additionally, the Redis notification job (lines 207-211) is enqueued outside the DB transaction boundary, risking inconsistency if the transaction fails after the job is enqueued.Required fixes:
- Refactor
orgRepo.queueOrganizationDeletionto accept and use the transaction context (tx) passed from the outer transaction, avoiding nested transactions- Move Redis job enqueueing inside the repository method or implement an outbox pattern to ensure the notification is only sent if the DB transaction commits successfully
Run the following script to verify the transaction handling in
OrganizationRepository.queueOrganizationDeletion:#!/bin/bash # Check if queueOrganizationDeletion starts its own transaction ast-grep --pattern $'queueOrganizationDeletion($$$) { $$$ transaction($$$) $$$ }'
🧹 Nitpick comments (1)
controlplane/src/bin/db-cleanup.ts (1)
56-72: Consider removing unused redisWorker connection.The script connects and pings both
redisQueueandredisWorker, but onlyredisQueueis used to initialize the queue instances. TheredisWorkerconnection appears unnecessary for this script.🔎 Proposed simplification
- const { redisQueue, redisWorker } = await createRedisConnections({ + const { redisQueue } = await createRedisConnections({ host: redis.host!, port: Number(redis.port), password: redis.password, tls: redis.tls, }); await redisQueue.connect(); - await redisWorker.connect(); - await redisWorker.ping(); await redisQueue.ping(); // ... rest of code ... } finally { redisQueue.disconnect(); - redisWorker.disconnect();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controlplane/src/bin/db-cleanup.ts(1 hunks)controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts(1 hunks)controlplane/test/test-util.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- controlplane/test/test-util.ts
- controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/bin/db-cleanup.ts
📚 Learning: 2025-07-01T13:53:54.146Z
Learnt from: wilsonrivera
Repo: wundergraph/cosmo PR: 1919
File: controlplane/src/core/repositories/OrganizationGroupRepository.ts:193-224
Timestamp: 2025-07-01T13:53:54.146Z
Learning: In the Cosmo codebase, database transactions are typically managed at the service layer (e.g., in buf services like deleteOrganizationGroup.ts), where repositories are instantiated with the transaction handle and all operations within those repositories are automatically part of the same transaction.
Applied to files:
controlplane/src/bin/db-cleanup.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
controlplane/src/bin/db-cleanup.ts (2)
128-135: LGTM! Proper filtering for inactive organizations.The WHERE clause correctly filters for organizations that:
- Are not already queued for deletion
- Are not deactivated (avoiding duplicates)
- Were created before the inactivity threshold
- Have no billing plan or are on the developer plan
This addresses the previous review concerns about excluding deactivated organizations and checking billing plans.
184-194: LGTM! Proper inactivity verification.The audit log check correctly verifies whether an organization has had any activity within the inactivity window. Organizations with recent activity are appropriately skipped, ensuring only truly inactive organizations are enqueued for deletion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controlplane/src/bin/db-cleanup.ts (2)
122-122: Remove unused userId field from query.The
userIdfield is selected but never used inprocessChunkOfOrganizations. Consider removing it to slightly reduce query overhead.🔎 Proposed fix
.select({ id: schema.organizations.id, slug: schema.organizations.slug, - userId: schema.organizations.createdBy, plan: schema.organizationBilling.plan, })And update the type signature on line 173:
- organizations: { id: string; slug: string; userId: string | null }[]; + organizations: { id: string; slug: string }[];
182-182: Reuse the parent logger for consistency.Creating a new
pino()logger here instead of reusing the logger from line 62 leads to inconsistent logging across the script. The queue constructors (lines 67-68) receive the parent logger, but the repository receives a separate instance.🔎 Proposed fix
Pass the logger as a parameter to
processChunkOfOrganizations:async function processChunkOfOrganizations({ organizations, db, inactivityThreshold, deleteOrganizationQueue, notifyOrganizationDeletionQueuedQueue, + logger, }: { organizations: { id: string; slug: string; userId: string | null }[]; db: PostgresJsDatabase<typeof schema>; inactivityThreshold: Date; deleteOrganizationQueue: DeleteOrganizationQueue; notifyOrganizationDeletionQueuedQueue: NotifyOrganizationDeletionQueuedQueue; + logger: pino.Logger; }) { const queuedAt = new Date(); const deletesAt = addDays(queuedAt, DELAY_FOR_ORG_DELETION_IN_DAYS); - const orgRepo = new OrganizationRepository(pino(), db, undefined); + const orgRepo = new OrganizationRepository(logger, db, undefined);And update the call site around line 152:
return processChunkOfOrganizations({ organizations: chunk, db: tx, inactivityThreshold, deleteOrganizationQueue, notifyOrganizationDeletionQueuedQueue, + logger, });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/bin/db-cleanup.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/bin/db-cleanup.ts
📚 Learning: 2025-07-01T13:53:54.146Z
Learnt from: wilsonrivera
Repo: wundergraph/cosmo PR: 1919
File: controlplane/src/core/repositories/OrganizationGroupRepository.ts:193-224
Timestamp: 2025-07-01T13:53:54.146Z
Learning: In the Cosmo codebase, database transactions are typically managed at the service layer (e.g., in buf services like deleteOrganizationGroup.ts), where repositories are instantiated with the transaction handle and all operations within those repositories are automatically part of the same transaction.
Applied to files:
controlplane/src/bin/db-cleanup.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (2)
controlplane/src/bin/db-cleanup.ts (2)
17-28: Well-defined configuration constants.The constants are clearly documented and provide sensible defaults:
- 3-month inactivity threshold gives adequate time before considering deletion
- 7-day deletion delay provides a reasonable recovery window
- Parallelism and batch size are appropriate for a background cleanup script
128-138: LGTM! Query correctly filters target organizations.The selection criteria are well-implemented:
- Excludes already-queued and deactivated organizations (avoiding duplicates)
- Properly scopes to free-tier plans (developer or null)
- Correctly identifies single-member organizations via HAVING clause
This addresses the plan-check concern and prevents re-queueing deactivated organizations.
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts:
- Around line 129-133: In QueueInactiveOrganizationsDeletionWorker, the audit
activity check is currently bypassed because the `continue` is commented out;
restore the original behavior by uncommenting the `continue` inside the
conditional that checks `if (auditLogs.length > 0 && auditLogs[0].count > 0)` so
that organizations with recent audit activity are skipped from deletion
processing, and optionally add a short debug log (using the existing logger)
inside that branch to indicate the org was skipped due to recent audit logs.
- Around line 180-202: The query currently selects
schema.organizations.createdBy as userId but that field may not correspond to
the lone remaining member; update retrieveOrganizationsWithSingleUser to join
the organizationsMembers table (alias it, e.g. om) and select om.userId as the
remaining member ID instead of schema.organizations.createdBy, ensuring the join
used in the FROM/innerJoin includes om and that om.userId is included in the
GROUP BY (or aggregated) so the HAVING COUNT(...) = 1 still works and returns
the actual remaining member.
🧹 Nitpick comments (4)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (2)
22-22: Convert empty interface to a type alias.Per static analysis, an empty interface is equivalent to
{}. Use a type alias for clarity and to follow best practices.🔎 Proposed fix
-export interface QueueInactiveOrganizationsDeletionInput {} +export type QueueInactiveOrganizationsDeletionInput = Record<string, never>;
215-222: Consider reducing concurrency for scheduled job.This worker processes a single scheduled job hourly. A concurrency of 10 is excessive and won't provide any benefit. Consider setting
concurrency: 1to match the expected workload.🔎 Proposed fix
const worker = new Worker<QueueInactiveOrganizationsDeletionInput>( QueueName, (job) => new QueueInactiveOrganizationsDeletionWorker(input).handler(job), { connection: input.redisConnection, - concurrency: 10, + concurrency: 1, }, );controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (2)
105-105: Avoid directprocess.envaccess; inject via config.Accessing
process.env.WEB_BASE_URLdirectly breaks the configuration injection pattern used elsewhere. Consider passingwebBaseUrlthrough the worker's input options for consistency and testability.
94-97: Specify a consistent locale for date formatting.Using
Intl.DateTimeFormat(undefined, ...)relies on the system's default locale, which can produce inconsistent date formats across environments. Consider using a fixed locale (e.g.,'en-US') or allowing the locale to be configured.🔎 Proposed fix
- const intl = Intl.DateTimeFormat(undefined, { + const intl = Intl.DateTimeFormat('en-US', { dateStyle: 'medium', timeStyle: 'short', });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
controlplane/package.jsoncontrolplane/src/core/build-server.tscontrolplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.tscontrolplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts
🧰 Additional context used
🧬 Code graph analysis (3)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (5)
controlplane/src/core/workers/Worker.ts (1)
IQueue(3-7)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1679)controlplane/src/core/workers/DeleteOrganizationWorker.ts (1)
DeleteOrganizationQueue(20-62)controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (1)
NotifyOrganizationDeletionQueuedQueue(18-60)controlplane/src/db/schema.ts (1)
auditLogs(1936-1972)
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (4)
controlplane/src/core/workers/Worker.ts (2)
IQueue(3-7)IWorker(9-11)controlplane/src/core/routes.ts (1)
opts(62-67)controlplane/src/core/services/Mailer.ts (1)
Mailer(13-101)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1679)
controlplane/src/core/build-server.ts (2)
controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (2)
NotifyOrganizationDeletionQueuedQueue(18-60)createNotifyOrganizationDeletionQueuedWorker(117-139)controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (2)
QueueInactiveOrganizationsDeletionQueue(24-80)createQueueInactiveOrganizationsDeletionWorker(205-231)
🪛 Biome (2.1.2)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts
[error] 22-22: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (2)
24-80: LGTM!The queue implementation correctly uses
upsertJobSchedulerfor scheduled execution. The hourly cron pattern'0 0 * * * *'(BullMQ format with seconds) is appropriate for periodic cleanup checks. The disabledaddJob/getJobmethods make sense since this queue only supports scheduled jobs.
100-103: Verify thestartOfMonthnormalization is intentional.Using
startOfMonth(subDays(now, MIN_INACTIVITY_DAYS))can extend the inactivity window beyond 90 days. For example, on Jan 15, this would result in Oct 1 (~106 days ago) instead of Oct 17. If the intent is exactly 90 days, usesubDays(now, MIN_INACTIVITY_DAYS)directly.controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (2)
18-60: LGTM!The queue implementation follows the established pattern from other workers in the codebase, with appropriate retry settings and error handling.
117-139: LGTM!The worker factory follows the established pattern with appropriate error and stall event handlers. The high concurrency (100) is reasonable for notification jobs that may be batched.
controlplane/src/core/build-server.ts (3)
56-63: LGTM!The imports for the new queue and worker modules are correctly added.
405-434: LGTM!The wiring of the new
NotifyOrganizationDeletionQueuedQueueandQueueInactiveOrganizationsDeletionQueuefollows the established pattern. Dependencies are correctly injected, workers are pushed tobullWorkersfor graceful shutdown, andscheduleJob()is called to register the recurring cron job.
437-441: LGTM!The raw-body plugin configuration with
global: falseandencoding: 'utf8'is appropriate for webhook-specific usage.controlplane/package.json (1)
65-78: No breaking changes or security advisories with bullmq 5.66.4 and ioredis 5.8.2.Both versions are valid and have no known security vulnerabilities. Since these are patch releases within the same major version (v5), no breaking changes are expected. The Redis configuration already correctly uses
maxRetriesPerRequest: nullas required for BullMQ v5 workers.
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts
Show resolved
Hide resolved
…organizations' into wilson/eng-7753-delete-inactive-organizations
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts:
- Around line 180-202: In retrieveOrganizationsWithSingleUser replace selecting
schema.organizations.createdBy for the remaining member with the actual member
from organizationsMembers: select something like
MAX(schema.organizationsMembers.userId) (alias userId) instead of
schema.organizations.createdBy, keep the HAVING COUNT = 1 and groupBy
schema.organizations.id (and organizationBilling.plan as used), so the query
returns the single remaining organizationsMembers.userId rather than the
creator.
🧹 Nitpick comments (3)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (3)
22-22: Replace empty interface with a type alias.The empty interface is equivalent to
{}and violates best practices. Since this job input carries no data, use a type alias instead.🔎 Proposed fix
-export interface QueueInactiveOrganizationsDeletionInput {} +export type QueueInactiveOrganizationsDeletionInput = Record<string, never>;
52-65: Consider documenting or refactoring the schedule-only queue pattern.The
addJobandgetJobmethods throw errors indicating they shouldn't be called directly, which violates theIQueueinterface contract. While this appears intentional (since the queue is only used viascheduleJob), it may confuse future maintainers.Consider one of the following:
- Add JSDoc comments explaining this queue is schedule-only
- Create a separate interface for scheduled queues that doesn't require these methods
- Return rejected promises instead of throwing synchronously
67-79: Consider reducing the schedule frequency.The cron pattern
'0 0 * * * *'runs every hour to check for 90-day inactivity. Since organization activity changes slowly, running hourly is inefficient. A daily schedule (e.g.,'0 0 0 * * *'at midnight) or weekly schedule would be more appropriate and reduce system load.🔎 Proposed fix (daily schedule)
pattern: '0 0 * * * *', + // Run once daily at midnight + pattern: '0 0 0 * * *',
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts
🧬 Code graph analysis (1)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (5)
controlplane/src/core/workers/Worker.ts (2)
IQueue(3-7)IWorker(9-11)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1679)controlplane/src/core/workers/DeleteOrganizationWorker.ts (1)
DeleteOrganizationQueue(20-62)controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (1)
NotifyOrganizationDeletionQueuedQueue(18-60)controlplane/src/db/schema.ts (1)
auditLogs(1936-1972)
🪛 Biome (2.1.2)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts
[error] 22-22: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
🔇 Additional comments (1)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (1)
142-144: No issue found - the code is logically safe.The non-null assertion on
sess.start!is justified by the short-circuit evaluation in the filter condition. The first part(sess.lastAccess || sess.start)ensures at least one timestamp is present before the second part executes. Ifsess.lastAccessis falsy,sess.startmust be truthy to have passed the initial check, making the non-null assertion safe. Sessions with both timestamps missing are correctly filtered out.Likely an incorrect or invalid review comment.
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts
Show resolved
Hide resolved
wilsonrivera
changed the title
wilsonrivera
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (4)
22-22: Replace empty interface with a type alias.Empty interfaces are equivalent to
{}and should use type aliases instead.♻️ Proposed fix
-export interface QueueInactiveOrganizationsDeletionInput {} +export type QueueInactiveOrganizationsDeletionInput = {};
52-65: Consider documenting why manual job operations are disabled.The
addJobandgetJobmethods throw errors whileremoveJobreturns a no-op result. Since this queue is only invoked viascheduleJob(), this appears intentional but may confuse future maintainers.Consider adding JSDoc comments explaining that this queue only supports scheduled execution, or evaluate whether implementing
IQueueis semantically correct for a schedule-only queue.
142-144: Session filtering logic could be simplified.The fallback
sess.lastAccess || sess.start!with a non-null assertion is repeated and could be extracted for clarity.♻️ Proposed refactor
- const numberOfSessionsRecentlyActive = userSessions.filter( - (sess) => (sess.lastAccess || sess.start) && new Date(sess.lastAccess || sess.start!) >= inactivityThreshold, - ).length; + const numberOfSessionsRecentlyActive = userSessions.filter((sess) => { + const timestamp = sess.lastAccess || sess.start; + return timestamp && new Date(timestamp) >= inactivityThreshold; + }).length;
163-178: LGTM! Consider adding error handling for queue operations.The method correctly queues both the deletion and notification jobs. The flow is appropriate.
If either queue operation fails, the organization's
queuedForDeletionAttimestamp (set byqueueOrganizationDeletion) may be inconsistent with the queued jobs. Consider wrapping both operations in a try-catch or using a database transaction ifqueueOrganizationDeletionsupports it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controlplane/package.jsoncontrolplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts
🧰 Additional context used
🧬 Code graph analysis (1)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (5)
controlplane/src/core/workers/Worker.ts (1)
IQueue(3-7)controlplane/src/core/repositories/OrganizationRepository.ts (1)
OrganizationRepository(50-1679)controlplane/src/core/services/Keycloak.ts (1)
Keycloak(10-451)controlplane/src/core/workers/DeleteOrganizationWorker.ts (1)
DeleteOrganizationQueue(20-62)controlplane/src/core/workers/NotifyOrganizationDeletionQueuedWorker.ts (1)
NotifyOrganizationDeletionQueuedQueue(18-60)
🪛 Biome (2.1.2)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts
[error] 22-22: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (8)
controlplane/src/core/workers/QueueInactiveOrganizationsDeletionWorker.ts (5)
1-21: LGTM!Imports and constants are appropriate. The 90-day inactivity threshold and 7-day deletion delay align with the PR objectives.
100-110: LGTM!The handler correctly calculates the inactivity threshold (90 days, normalized to start of month) and deletion deadline (7 days from now). Early return for empty result set is appropriate.
129-133: Audit log activity check is working correctly.The
continuestatement is properly in place, ensuring organizations with recent audit activity are skipped from deletion consideration. This resolves the critical issue flagged in previous reviews.
205-231: LGTM!The factory function properly configures the BullMQ worker with appropriate concurrency, error handling, and stall detection.
67-79: Cron pattern is correct for monthly execution.The pattern
'0 0 0 1 * *'correctly executes at midnight (00:00:00) on the 1st of every month, matching BullMQ's six-field cron format (second minute hour day-of-month month day-of-week). This aligns with the intended behavior.controlplane/package.json (3)
65-65: Confirm intentional exact version pinning.The caret prefix has been removed from both
bullmqandioredis, pinning them to exact versions. This prevents automatic patch and minor updates, which could delay important security fixes. Most other dependencies in this file retain the caret (e.g.,@aws-sdk/client-s3,axios).Is this exact pinning intentional for stability with the new workers, or should semver ranges be preserved to allow automatic patch updates?
Also applies to: 78-78
78-78: The ioredis 5.8.2 update is compatible with BullMQ 5.66.4 and contains no breaking changes from 5.4.1 (only bug fixes). The current redis configuration properly implements the requiredmaxRetriesPerRequest: nullsetting for BullMQ workers.
65-65: The BullMQ version jump (5.10.0 → 5.66.4) introduces breaking changes, but the code correctly implements them.While this is a substantial version upgrade spanning 56 minor versions, verification confirms that critical breaking changes have been properly addressed:
- Redis connections are correctly passed to all Queue and Worker constructors
- Event listeners use the new Worker pattern (
worker.on('stalled')instead of QueueScheduler)- Redis configuration includes
maxRetriesPerRequest: nullrequired for BullMQ workers with ioredis v5- Both new workers (NotifyOrganizationDeletionQueuedWorker and QueueInactiveOrganizationsDeletionWorker) follow the correct implementation pattern
No known security vulnerabilities exist in bullmq v5.66.4, and ioredis v5.8.2 is compatible.
The goal of this PR is to introduce a script to enqueue the deletion of inactive organizations and send a message so owners can prevent the deletion before the time runs out.
Currently only organizations that have been inactive for more than 3 months and only have a single member are considered for deletion.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist