Skip to content

[Fix] fix command dry run#15697

Merged
charlesBochet merged 1 commit intomainfrom
fix--dry-run-command
Nov 7, 2025
Merged

[Fix] fix command dry run#15697
charlesBochet merged 1 commit intomainfrom
fix--dry-run-command

Conversation

@ijreilly
Copy link
Copy Markdown
Contributor

@ijreilly ijreilly commented Nov 7, 2025

No description provided.

);
});

this.hasRunOnce = true;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The hasRunOnce instance variable incorrectly prevents CleanOrphanedRoleTargetsCommand from processing more than one workspace.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The CleanOrphanedRoleTargetsCommand utilizes a hasRunOnce instance variable. When runMigrationCommand() iterates through multiple workspaces, this hasRunOnce flag is set to true after the first workspace completes its processing (or dry run). Subsequent workspaces then encounter an early return check (if (this.hasRunOnce) { return; }), causing them to be skipped entirely. This prevents the command from cleaning orphaned role targets across all intended workspaces.

💡 Suggested Fix

Remove the hasRunOnce instance variable and its associated early return check from CleanOrphanedRoleTargetsCommand. This command operates on workspace-specific data and should not use a global run-once mechanism.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location:
packages/twenty-server/src/database/commands/upgrade-version-command/1-11/1-11-clean-orphaned-role-targets.command.ts#L78

Potential issue: The `CleanOrphanedRoleTargetsCommand` utilizes a `hasRunOnce` instance
variable. When `runMigrationCommand()` iterates through multiple workspaces, this
`hasRunOnce` flag is set to `true` after the first workspace completes its processing
(or dry run). Subsequent workspaces then encounter an early return check (`if
(this.hasRunOnce) { return; }`), causing them to be skipped entirely. This prevents the
command from cleaning orphaned role targets across all intended workspaces.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

Fixed a bug where the CleanOrphanedRoleTargetsCommand would run multiple times in dry-run mode when processing multiple workspaces. The hasRunOnce flag was only being set after actual deletions (line 91), but not in the dry-run early return path. This caused the command to execute repeatedly on each workspace during dry-run mode instead of stopping after the first execution.

Key Changes:

  • Added this.hasRunOnce = true; in the dry-run block (line 78) to ensure the flag is set even when no actual deletions occur
  • This makes the dry-run behavior consistent with the actual execution behavior (which already sets the flag at line 91)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The fix is a simple one-line addition that correctly mirrors the existing pattern at line 91. The logic is straightforward: setting the hasRunOnce flag in the dry-run path ensures consistent behavior between dry-run and actual execution modes. No edge cases or potential issues identified.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
packages/twenty-server/src/database/commands/upgrade-version-command/1-11/1-11-clean-orphaned-role-targets.command.ts 5/5 Adds missing hasRunOnce = true flag in dry-run mode to prevent command from running multiple times

Sequence Diagram

sequenceDiagram
    participant Runner as ActiveOrSuspended<br/>WorkspacesMigration<br/>CommandRunner
    participant Command as CleanOrphaned<br/>RoleTargets<br/>Command
    participant DB as Database

    Runner->>Command: runOnWorkspace(workspace1)
    Command->>Command: Check hasRunOnce flag
    alt hasRunOnce is true
        Command->>Runner: Early return
    else hasRunOnce is false
        Command->>DB: Query orphaned role targets
        alt isDryRun is true
            Command->>Command: Log dry-run output
            Command->>Command: Set hasRunOnce = true (FIX)
            Command->>Runner: Return
        else isDryRun is false
            Command->>DB: Delete orphaned records
            Command->>Command: Set hasRunOnce = true
            Command->>Runner: Return
        end
    end

    Runner->>Command: runOnWorkspace(workspace2)
    Command->>Command: Check hasRunOnce flag
    Command->>Runner: Early return (hasRunOnce is true)
Loading

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 7, 2025

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:37699

This environment will automatically shut down when the PR is closed or after 5 hours.

Copy link
Copy Markdown
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

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

LGTM

@charlesBochet charlesBochet merged commit 0992d80 into main Nov 7, 2025
50 checks passed
@charlesBochet charlesBochet deleted the fix--dry-run-command branch November 7, 2025 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants