Delete project #1913
Delete project #1913
Conversation
- Don’t schedule tasks if the project is deleted - Delete queues from the master queues - Add the old delete project UI back in
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes implement project deletion functionality in the web application. A new "delete" action is added to the project settings route and its schema, which validates the project slug. When triggered, the delete action calls the DeleteProjectService, which removes associated environment queues from both the MARQS system and RunEngine master queues. Additionally, the scheduled task service now checks if a project is deleted to disable tasks. New methods were introduced in both MARQS and RunEngine code to support the queue removal process. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Project Settings UI
participant Route as Route Handler
participant DeleteService as DeleteProjectService
participant MarQS
participant RunEngine
User->>UI: Submit delete project form
UI->>Route: Trigger "delete" action
Route->>DeleteService: Invoke deletion process
DeleteService->>MarQS: Remove environment queues (orgId, envId)
DeleteService->>RunEngine: Remove environment queues (masterQueue, orgId, projectId)
RunEngine-->>DeleteService: Confirm queue removal
DeleteService-->>Route: Return deletion status
Route-->>UI: Redirect with success/error message
sequenceDiagram
participant Scheduler
participant TriggerService as TriggerScheduledTaskService
participant Project
Scheduler->>TriggerService: Invoke scheduled task
TriggerService->>Project: Check if project.deletedAt is set
alt Project is deleted
TriggerService->>Scheduler: Log debug message & exit task
else Project is active
TriggerService->>Scheduler: Proceed with scheduled task execution
end
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webapp/app/services/deleteProject.server.ts (2)
50-54: Consider explicit error handling or logging whenmarqsis unavailable.You're using optional chaining (
marqs?) to remove queues. Ifmarqsisnullorundefined, queue removal would silently fail. It may be beneficial to throw or log an error whenmarqsis not available, ensuring consistency of your system’s state.
55-69: Handle potential concurrency or partial failures for master queue removals.Currently, the removals are executed sequentially and without explicit error handling per queue. Consider using
Promise.allfor concurrency and capturing/logging individual errors for each queue to ensure robust, scalable cleanup.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (2)
54-74: Optional: Implement case-insensitive slug matching.This validation step correctly ensures the user enters the exact project slug. Consider supporting case-insensitive matching or user-friendly matching if your slug rules permit, to reduce user confusion with capitalization.
261-300: UI design suggestion: Strengthen the destructive action confirmation.The “Danger zone” section and matching slug check is an effective safeguard. For added clarity, consider an optional modal confirmation or visible warning text to reinforce the irreversible nature of deletion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx(5 hunks)apps/webapp/app/services/deleteProject.server.ts(3 hunks)apps/webapp/app/v3/marqs/index.server.ts(1 hunks)apps/webapp/app/v3/services/triggerScheduledTask.server.ts(1 hunks)internal-packages/run-engine/src/engine/index.ts(1 hunks)internal-packages/run-engine/src/run-queue/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal-packages/run-engine/src/engine/index.ts (1)
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (1)
projectId(454-474)
apps/webapp/app/services/deleteProject.server.ts (2)
apps/webapp/app/v3/marqs/index.server.ts (1)
marqs(2152-2152)apps/webapp/app/v3/runEngine.server.ts (1)
engine(9-9)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/webapp/app/v3/services/triggerScheduledTask.server.ts (1)
46-54: Well-implemented check for deleted projects.This addition appropriately prevents scheduled tasks from running if the associated project has been deleted, complementing the existing check for deleted organizations.
internal-packages/run-engine/src/engine/index.ts (1)
778-792: Good addition of method to handle project deletion queue cleanup.This method properly delegates the queue removal logic to the
runQueueimplementation, maintaining a clean separation of concerns.internal-packages/run-engine/src/run-queue/index.ts (1)
686-717: Well-implemented queue cleanup method using Redis scanning pattern.The implementation correctly uses
zscanStreamto efficiently find and remove queues associated with a deleted project without blocking Redis operations.This approach has the following benefits:
- Uses non-blocking scan operations instead of potentially expensive commands like
KEYS- Processes data in chunks with the stream API
- Only performs the
ZREMoperation when needed (i.e., when matching queues are found)- Properly handles the zscanStream result format which returns alternating members and scores
apps/webapp/app/v3/marqs/index.server.ts (1)
196-226: Well-implemented MARQS queue cleanup for project deletion.This method follows the same efficient pattern as in the RunQueue implementation, using
zscanStreamfor non-blocking removal of queues associated with a deleted project.The implementation is consistent with the RunQueue version but adapted to the MARQS system's key patterns, ensuring that both queue systems get properly cleaned up during project deletion.
apps/webapp/app/services/deleteProject.server.ts (2)
3-4: LGTM on newly added imports.These imports appropriately bring in the
marqsandenginesingletons for queue operations.
38-40: Helpful explanations of the deletion effects.The added comments clarify the consequences of marking a project as deleted. They provide a nice overview of disabling API keys and schedules.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx (3)
30-34: New imports look good.These additions cleanly incorporate the message redirection utilities, the DeleteProjectService, logger, and path builders.
124-143: Project deletion action is well implemented.The try/catch block with
logger.errorensures errors are logged, and user feedback is provided via redirect with an error message. This is a solid approach to error handling.
171-189: Good usage of a separate form state for deletion.You've introduced a dedicated form with its own schema validation for the delete action. This approach is clean and keeps functionality well-isolated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/app/services/deleteProject.server.ts (1)
17-70: Consider additional safeguards for this destructive operation.Project deletion is a critical operation that requires careful handling. The current implementation primarily focuses on queue cleanup but might benefit from additional safeguards.
Consider these enhancements:
- Add an optional confirmation parameter to prevent accidental deletions:
public async call(options: Options & { confirmation?: string }) { // Require explicit confirmation for non-development environments if (process.env.NODE_ENV !== "development" && options.confirmation !== project.slug) { throw new Error("Confirmation required for project deletion. Please provide the project slug as confirmation."); } // Rest of the method... }
- Implement a soft-delete grace period before permanent queue removal:
// First mark as deleted (this disables functionality) await this.#prismaClient.project.update({ where: { id: project.id }, data: { deletedAt: new Date() }, }); // Schedule queue cleanup for later (e.g., after 7 days) // This allows for potential recovery if deletion was accidental await this.#prismaClient.scheduledTask.create({ data: { type: "QUEUE_CLEANUP", scheduledFor: new Date(Date.now() + 7 * 24 * 60 * 60 * 1000), // 7 days later data: { projectId: project.id, organizationId: project.organization.id }, }, });These additions would improve safety while maintaining the core functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/services/deleteProject.server.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/webapp/app/services/deleteProject.server.ts (2)
apps/webapp/app/v3/marqs/index.server.ts (1)
marqs(2152-2152)apps/webapp/app/v3/runEngine.server.ts (1)
engine(9-9)
🔇 Additional comments (4)
apps/webapp/app/services/deleteProject.server.ts (4)
3-4: Appropriate imports for the new queue management functionality.The new imports provide access to the necessary MARQS and RunEngine services, aligning with the PR's objective to properly handle queues during project deletion.
38-41: Good cleanup of MARQS queues for each environment.The code correctly iterates through all environments to remove their queues from the MARQS system. However, note the use of optional chaining (
?.) which suggestsmarqsmight be null in some cases.Consider adding error handling to log any failures and continue with the deletion process, rather than letting the entire operation fail if queue removal encounters an issue.
// Remove queues from MARQS for (const environment of project.environments) { - await marqs?.removeEnvironmentQueuesFromMasterQueue(project.organization.id, environment.id); + try { + await marqs?.removeEnvironmentQueuesFromMasterQueue(project.organization.id, environment.id); + } catch (error) { + console.error(`Failed to remove MARQS queues for environment ${environment.id}:`, error); + // Continue with deletion despite queue removal failure + } }
43-57: Thorough cleanup of RunEngine master queues.The implementation properly identifies all worker groups and their master queues before removing the environment queues.
There are a few considerations to improve this implementation:
- Unlike the MARQS code, there's no optional chaining on
engine. Is this intentional?- Consider handling errors and using parallel processing for better performance:
const engineMasterQueues = workerGroups.map((group) => group.masterQueue); -for (const masterQueue of engineMasterQueues) { - await engine.removeEnvironmentQueuesFromMasterQueue({ - masterQueue, - organizationId: project.organization.id, - projectId: project.id, - }); -} +// Process all master queues in parallel +await Promise.allSettled( + engineMasterQueues.map(async (masterQueue) => { + try { + await engine.removeEnvironmentQueuesFromMasterQueue({ + masterQueue, + organizationId: project.organization.id, + projectId: project.id, + }); + } catch (error) { + console.error(`Failed to remove RunEngine queues for master queue ${masterQueue}:`, error); + // Continue with deletion despite queue removal failure + } + }) +);This improves efficiency by processing master queues in parallel while ensuring the deletion process continues even if some queue removals fail.
59-62: Clear and informative comments about the deletion process.The comments effectively explain the implications of marking a project as deleted, including disabling API keys and schedules.
Given the critical nature of this operation, consider wrapping all operations in a transaction to ensure data consistency if any step fails:
public async call(options: Options) { + return this.#prismaClient.$transaction(async (tx) => { const projectId = await this.#getProjectId(options); - const project = await this.#prismaClient.project.findFirst({ + const project = await tx.project.findFirst({ // existing query... }); // queue removal code... - await this.#prismaClient.project.update({ + await tx.project.update({ where: { id: project.id, }, data: { deletedAt: new Date(), }, }); + }); }This ensures that all database operations are atomic, maintaining data integrity if an error occurs.
|
We need to deal with environment master queues |
* Delete project - Don’t schedule tasks if the project is deleted - Delete queues from the master queues - Add the old delete project UI back in * Mark the project as deleted last * Fix for overriding local variable * Added a todo for deleting env queues * Remove todo
* WIP on secret env vars * Editing individual env var values is working * Sort the env vars by the key * Deleting values * Allowing setting secret env vars * Added medium switch style * Many style changes to the env var form * “Copy text” -> “Copy” * Draw a divider between hidden buttons * Env var tweaks * Don’t show Dev:you anymore * Grouping the same env var keys together * Styles improved * Improved styling of edit panel * Fix bun detection, dev flushing, and init command (#1914) * update nypm to support text-based bun lockfiles * add nypm changeset * handle dev flushing failures gracefully * fix path normalization for init.ts * add changesets * chore: remove pre.json after exiting pre mode * init command to install v4-beta packages * Revert "chore: remove pre.json after exiting pre mode" This reverts commit f5694fd. * make init default to cli version for all packages * Release 4.0.0-v4-beta.1 (#1916) * chore: Update version for release (v4-beta) * Release 4.0.0-v4-beta.1 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: nicktrn <55853254+nicktrn@users.noreply.github.com> * Both run engines will only lock to versions they can handle (#1922) * run engine v1 will only lock to v1 deployments * run engine v2 will only lock to managed v2 deployments * test: create background worker and deployment with correct engine version * Add links to and from deployments (#1921) * link from deployments tasks to filtered runs view * jump to deployment * don't add version links for dev (yet) * Fix current worker deployment getter (#1924) * only return last v1 deployment in the shared queue consumer * be explicit about only returning managed deployments * Add a docs page for the human-in-the-loop example project (#1919) * Add a docs page for the human-in-the-loop example project * Order guides, example projects and example tasks alphabetically in the docs list * Managed run controller revamp (#1927) * update nypm to support text-based bun lockfiles * fix retry spans * only download debug logs if admin * add nypm changeset * pull out env override logic * use runner env gather helper * handle dev flushing failures gracefully * fix path normalization for init.ts * add logger * add execution heartbeat service * add snapshot poller service * fix poller * add changesets * create socket in constructor * enable strictPropertyInitialization * deprecate dequeue from version * start is not async * dependency injection in prep for tests * add warm start count to all controller logs * add restore count * pull out run execution logic * temp disable pre * add a controller log when starting an execution * refactor execution and squash some bugs * cleanup completed docker containers by default * execution fixes and logging improvements * don't throw afet abort cleanup * poller should use private interval * rename heartbeat service file * rename HeartbeatService to IntervalService * restore old heartbeat service but deprecate it * use the new interval service everywhere * Revert "temp disable pre" This reverts commit e03f417. * add changeset * replace all run engine find uniques with find first * Release 4.0.0-v4-beta.2 (#1928) * chore: Update version for release (v4-beta) * Release 4.0.0-v4-beta.2 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: nicktrn <55853254+nicktrn@users.noreply.github.com> * Remove batch ID carryover for non-batch waits (#1930) * add failing test case * do not carry over previous batch id when blocking with waitpoint * delete irrelevant test * Delete project (#1913) * Delete project - Don’t schedule tasks if the project is deleted - Delete queues from the master queues - Add the old delete project UI back in * Mark the project as deleted last * Fix for overriding local variable * Added a todo for deleting env queues * Remove todo * Improve usage flushing (#1931) * add flush to global usage api * enable controller debug logs * initialize usage manager after env overrides * add previous run id to more debug logs * add changeset * For secret env vars, don’t return the value * Added a new env var repository function for getting secrets with redactions * Test task for env vars * Delete heartbeat file, merge mess up --------- Co-authored-by: nicktrn <55853254+nicktrn@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Saadi Myftija <saadi.myftija@gmail.com>
Summary by CodeRabbit