fix: crud operations on var sets trigger reconciliations#912
fix: crud operations on var sets trigger reconciliations#912adityachoudhari26 merged 2 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded calls to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR ensures that variable set CRUD operations in the API trigger reconciliation by enqueueing desired-version work for all release targets in the workspace.
Changes:
- Add
enqueueAllReleaseTargetsDesiredVersioncalls after variable set create and delete. - Add
enqueueAllReleaseTargetsDesiredVersioncall during variable set update.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| enqueueAllReleaseTargetsDesiredVersion(tx, workspaceId); | ||
|
|
There was a problem hiding this comment.
enqueueAllReleaseTargetsDesiredVersion is async, but this call is not awaited and is passed the transaction handle (tx). Because the transaction callback can resolve before the reconciler finishes its first awaited query, the reconciler may try to use a closed transaction, causing intermittent failures/unhandled rejections. Move this enqueue call outside the transaction (using db), or await it inside the transaction if it must be part of the same atomic unit.
| return vs; | ||
| }); | ||
|
|
||
| enqueueAllReleaseTargetsDesiredVersion(db, workspaceId); |
There was a problem hiding this comment.
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.
| enqueueAllReleaseTargetsDesiredVersion(db, workspaceId); | |
| void enqueueAllReleaseTargetsDesiredVersion(db, workspaceId).catch((error) => { | |
| console.error( | |
| "Failed to enqueue release targets desired version reconciliation", | |
| { workspaceId, error }, | |
| ); | |
| }); |
| .then(takeFirstOrNull); | ||
|
|
||
| if (deleted == null) throw new NotFoundError("Variable set not found"); | ||
|
|
||
| enqueueAllReleaseTargetsDesiredVersion(db, workspaceId); | ||
|
|
||
| res.status(202).json(deleted); |
There was a problem hiding this comment.
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(...)).
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/workspaces/variable-sets.ts`:
- Line 169: The enqueueAllReleaseTargetsDesiredVersion call is being invoked
with the transaction handle (tx) inside the transaction callback which can close
before the fire-and-forget enqueue completes; move the call out of the
transaction and invoke enqueueAllReleaseTargetsDesiredVersion(db, workspaceId)
after the transaction completes (i.e., after the commit/transaction block ends)
so it uses the global db handle like the create/delete paths, and chain a
.catch(err => logger.warn(...)) to log any enqueue failures following the
existing fire-and-forget pattern.
- 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.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 073765bb-6a25-4595-be8d-8370cb3e120f
📒 Files selected for processing (1)
apps/api/src/routes/v1/workspaces/variable-sets.ts
| return vs; | ||
| }); | ||
|
|
||
| enqueueAllReleaseTargetsDesiredVersion(db, workspaceId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/api/src/routes/v1/workspaces/variable-sets.ts | head -220Repository: ctrlplanedev/ctrlplane
Length of output: 7287
🏁 Script executed:
rg -A 5 -B 5 "logger\." apps/api/src/routes/v1/workspaces/variable-sets.tsRepository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
rg "logger" apps/api/src/routes/ -t ts --max-count 5 | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 753
🏁 Script executed:
grep -r "import.*logger" apps/api/src/routes/tfe/ --include="*.ts" | head -5Repository: ctrlplanedev/ctrlplane
Length of output: 230
🏁 Script executed:
sed -n '110,120p' apps/api/src/routes/tfe/run_notification.tsRepository: ctrlplanedev/ctrlplane
Length of output: 314
🏁 Script executed:
sed -n '1,30p' apps/api/src/routes/tfe/run_notification.tsRepository: ctrlplanedev/ctrlplane
Length of output: 1281
🏁 Script executed:
sed -n '100,125p' apps/api/src/routes/tfe/run_notification.tsRepository: ctrlplanedev/ctrlplane
Length of output: 573
🏁 Script executed:
rg "void enqueueAllReleaseTargetsDesiredVersion" apps/api/src/routes/ -A 1Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
rg "void.*catch" apps/api/src/routes/ -B 1 -A 1 | head -30Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
rg "enqueueAllReleaseTargetsDesiredVersion" apps/api/src/routes/v1/workspaces/variable-sets.ts -B 2 -A 2Repository: ctrlplanedev/ctrlplane
Length of output: 626
🏁 Script executed:
sed -n '138,176p' apps/api/src/routes/v1/workspaces/variable-sets.tsRepository: ctrlplanedev/ctrlplane
Length of output: 1054
🏁 Script executed:
rg -B 5 -A 5 "if.*workspaceId.*null" apps/api/src/routes/tfe/run_notification.tsRepository: 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.
Resolves #911
Summary by CodeRabbit