Skip to content

fix: crud operations on var sets trigger reconciliations #912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adityachoudhari26 merged 2 commits into from
Apr 6, 2026
+7 −0
Merged

fix: crud operations on var sets trigger reconciliations #912

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d74b7c
fix: crud operations on var sets trigger reconciliations
adityachoudhari26 Apr 6, 2026
348fbc0
remove from tx
adityachoudhari26 Apr 6, 2026
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
7 changes: 7 additions & 0 deletions apps/api/src/routes/v1/workspaces/variable-sets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
takeFirstOrNull,
} from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import { enqueueAllReleaseTargetsDesiredVersion } from "@ctrlplane/db/reconcilers";
import * as schema from "@ctrlplane/db/schema";

const listVariableSets: AsyncTypedHandler<
Expand Down Expand Up @@ -87,6 +88,8 @@ const createVariableSet: AsyncTypedHandler<
return vs;
});

enqueueAllReleaseTargetsDesiredVersion(db, workspaceId);
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

enqueueAllReleaseTargetsDesiredVersion returns a Promise, but it's invoked without await/error handling. If enqueueing fails (DB error, deadlock, etc.), this becomes an unhandled rejection and the reconciler work may silently not be scheduled. If this is intended to be fire-and-forget, consider explicitly discarding the promise and handling errors (e.g., void ... .catch(...)); otherwise await it so failures surface predictably.

Suggested change
enqueueAllReleaseTargetsDesiredVersion(db, workspaceId);
void enqueueAllReleaseTargetsDesiredVersion(db, workspaceId).catch((error) => {
console.error(
"Failed to enqueue release targets desired version reconciliation",
{ workspaceId, error },
);
});

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 6, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n apps/api/src/routes/v1/workspaces/variable-sets.ts | head -220

Repository: ctrlplanedev/ctrlplane

Length of output: 7287

🏁 Script executed:

rg -A 5 -B 5 "logger\." apps/api/src/routes/v1/workspaces/variable-sets.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 48

🏁 Script executed:

rg "logger" apps/api/src/routes/ -t ts --max-count 5 | head -20

Repository: ctrlplanedev/ctrlplane

Length of output: 753

🏁 Script executed:

grep -r "import.*logger" apps/api/src/routes/tfe/ --include="*.ts" | head -5

Repository: ctrlplanedev/ctrlplane

Length of output: 230

🏁 Script executed:

sed -n '110,120p' apps/api/src/routes/tfe/run_notification.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 314

🏁 Script executed:

sed -n '1,30p' apps/api/src/routes/tfe/run_notification.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 1281

🏁 Script executed:

sed -n '100,125p' apps/api/src/routes/tfe/run_notification.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 573

🏁 Script executed:

rg "void enqueueAllReleaseTargetsDesiredVersion" apps/api/src/routes/ -A 1

Repository: ctrlplanedev/ctrlplane

Length of output: 48

🏁 Script executed:

rg "void.*catch" apps/api/src/routes/ -B 1 -A 1 | head -30

Repository: ctrlplanedev/ctrlplane

Length of output: 48

🏁 Script executed:

rg "enqueueAllReleaseTargetsDesiredVersion" apps/api/src/routes/v1/workspaces/variable-sets.ts -B 2 -A 2

Repository: ctrlplanedev/ctrlplane

Length of output: 626

🏁 Script executed:

sed -n '138,176p' apps/api/src/routes/v1/workspaces/variable-sets.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 1054

🏁 Script executed:

rg -B 5 -A 5 "if.*workspaceId.*null" apps/api/src/routes/tfe/run_notification.ts

Repository: ctrlplanedev/ctrlplane

Length of output: 314

Add explicit fire-and-forget handling with error logging for all three enqueue calls.

The floating promises at lines 91, 169, and 196 can cause unhandled rejections if the queue write fails after the response is sent. These should use the pattern void enqueueAllReleaseTargetsDesiredVersion(...).catch(err => logger.warn(...)).

Line 169 requires special attention: this call sits inside the db.transaction() callback and uses tx, so the enqueue operation may complete (or fail) after the transaction itself commits. Either await it or wrap it with the explicit void/catch pattern.

You'll also need to import logger: import { logger } from "@ctrlplane/logger";

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/routes/v1/workspaces/variable-sets.ts` at line 91, The three
calls to enqueueAllReleaseTargetsDesiredVersion are currently fire-and-forget
and can produce unhandled rejections; import logger from "@ctrlplane/logger" and
wrap each call as an explicit fire-and-forget: void
enqueueAllReleaseTargetsDesiredVersion(...).catch(err =>
logger.warn("enqueueAllReleaseTargetsDesiredVersion failed", { err, workspaceId
})) for the calls at the top-level (lines ~91 and ~196), and for the call inside
the db.transaction callback (around line ~169 / inside db.transaction(tx => {
... })), either await the enqueue before returning from the transaction or wrap
it the same way with void ... .catch to ensure errors are logged if the queue
write fails after the transaction commits.


res.status(201).json(created);
};

Expand Down Expand Up @@ -167,6 +170,7 @@ const updateVariableSet: AsyncTypedHandler<
});

if (updated == null) throw new NotFoundError("Variable set not found");
enqueueAllReleaseTargetsDesiredVersion(db, workspaceId);
res.status(202).json(updated);
};

Expand All @@ -187,6 +191,9 @@ const deleteVariableSet: AsyncTypedHandler<
.then(takeFirstOrNull);

if (deleted == null) throw new NotFoundError("Variable set not found");

enqueueAllReleaseTargetsDesiredVersion(db, workspaceId);

res.status(202).json(deleted);
Comment on lines 191 to 197
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

enqueueAllReleaseTargetsDesiredVersion is async but is called without await/error handling. If enqueueing fails, this can result in an unhandled rejection and reconciliations not being scheduled. Either await it (and decide what response behavior you want on failure) or explicitly fire-and-forget with error capture (e.g., void ... .catch(...)).

Copilot uses AI. Check for mistakes.
};

Expand Down
Loading