feat: restrict only one active job per release target #551
Conversation
WalkthroughThis update introduces new helper functions and modifies job evaluation and update logic to prevent concurrent active jobs for the same release target. It refactors job completion handling, replacing callback-based flows with direct database queries and conditional job creation, and enhances concurrency control within both worker and job update modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as evaluateReleaseTargetWorker
participant DB as Database
Worker->>DB: getReleaseTargetIdFromRelease(releaseId)
DB-->>Worker: releaseTargetId
Worker->>DB: getActiveJobForReleaseTarget(releaseTargetId)
DB-->>Worker: activeJob or null
alt Active job exists
Worker-->>Worker: Return early, no job created
else No active job
Worker-->>DB: Proceed to create new job
end
sequenceDiagram
participant updateJob as updateJob
participant DB as Database
updateJob->>updateJob: getIsJobJustCompleted(prevJob, updatedJob)
alt Not just completed
updateJob-->>updateJob: Return updatedJob
else Just completed
updateJob->>DB: getReleaseTargetFromJob(jobId)
DB-->>updateJob: releaseTarget or null
alt No releaseTarget
updateJob-->>updateJob: Log warning, return updatedJob
else Found releaseTarget
updateJob->>DB: getLatestReleaseForReleaseTarget(releaseTargetId)
DB-->>updateJob: latestRelease
alt No latestRelease or already has releaseJob
updateJob-->>updateJob: Return updatedJob
else
updateJob->>DB: createReleaseJob(latestRelease)
end
end
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
96-116: Query returns full joined rows although only the existence check is required
getActiveJobForReleaseTargetfetches complete records from four tables, while the caller only needs to know whether any active job exists. Selecting a single constant (or the job id) is both faster and lighter on the DB-connector’s deserialiser:- db - .select() + db + .select({ id: schema.job.id })This is a micro-optimisation, but given that the function may run very frequently it is worth the two-line change.
packages/job-dispatch/src/job-update.ts (1)
113-129: Row shape assumptions – prefer explicit column list
getLatestReleaseForReleaseTargetrelies on Drizzle’s default join-shape, later accessinglatestRelease.releaseandlatestRelease.release_job.
While this works today, explicit projection improves readability and guards against future refactors that might rename or add joins:- db - .select() + db + .select({ + release: schema.release, + release_job: schema.releaseJob, + })Not mandatory, but it makes downstream property access type-safe.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/event-worker/src/workers/evaluate-release-target.ts(4 hunks)packages/job-dispatch/src/job-update.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/workers/evaluate-release-target.tspackages/job-dispatch/src/job-update.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
apps/event-worker/src/workers/evaluate-release-target.ts (2)
212-217: Potential race window between duplicate checks and job insertThe release-target active-job check runs outside the transaction that creates the new
releaseJob. Two parallel workers can therefore both read “no active job” and then proceed to insert, violating the one-active-job invariant.If the invariant must be strict, consider wrapping the “active job check + createReleaseJob” in a single serialisable transaction on the job tables (or acquire a
FOR UPDATElock onrelease_targethere as well).
84-95:⚠️ Potential issue
getReleaseTargetIdFromReleasemay throw when the release row is missing
takeFirst()will throw (or returnundefined– depending on the dialect helper) when the release row is not found.
Because the subsequent destructuring{ releaseTargetId }is unconditional, the worker would crash with a runtime error instead of cleanly short-circuiting.A defensive change costs almost nothing and shields us from edge-cases (e.g. the release was deleted between the earlier insert and this query, or a malformed ID gets through).
- .then(takeFirst) - .then(({ releaseTargetId }) => releaseTargetId); + .then(takeFirstOrNull) + .then((row) => row?.releaseTargetId ?? null);Call-sites should be prepared to handle a possible
nullreturn (currently only one call-site – lines 212-217 – would need a null-check).⛔ Skipped due to learnings
Learnt from: adityachoudhari26 PR: ctrlplanedev/ctrlplane#395 File: packages/api/src/router/environment-page/resources/router.ts:40-45 Timestamp: 2025-03-24T18:46:38.894Z Learning: The `takeFirst` utility function in the codebase (from `@ctrlplane/db`) throws an Error with message "Found non unique or inexistent value" if the result array doesn't contain exactly one element, making additional null/undefined checks unnecessary after its use.packages/job-dispatch/src/job-update.ts (1)
84-91: Nice, concise helper – one nit
getIsJobJustCompletedis clear and expressive. 👍
(Optional) You could inline the boolean variables to save a couple of lines, but current form is perfectly readable.
| const getReleaseTargetFromJob = (jobId: string) => | ||
| db | ||
| .select() | ||
| .from(schema.releaseJob) | ||
| .innerJoin( | ||
| schema.release, | ||
| eq(schema.releaseJob.releaseId, schema.release.id), | ||
| ) | ||
| .innerJoin( | ||
| schema.versionRelease, | ||
| eq(schema.release.versionReleaseId, schema.versionRelease.id), | ||
| ) | ||
| .innerJoin( | ||
| schema.releaseTarget, | ||
| eq(schema.versionRelease.releaseTargetId, schema.releaseTarget.id), | ||
| ) | ||
| .where(eq(schema.job.id, jobId)) | ||
| .then(takeFirstOrNull) | ||
| .then((result) => result?.release_target ?? null); | ||
|
|
There was a problem hiding this comment.
Incorrect WHERE clause – schema.job not joined, causes invalid SQL
The query filters with eq(schema.job.id, jobId) while the job table is not part of the FROM … JOIN chain. At runtime most SQL engines will raise “column reference ‘job.id’ is ambiguous / missing FROM-clause entry”.
Fix by filtering on the joined releaseJob.jobId, or join the job table explicitly:
- .where(eq(schema.job.id, jobId))
+ .where(eq(schema.releaseJob.jobId, jobId))or
+ .innerJoin(schema.job, eq(schema.releaseJob.jobId, schema.job.id))
+ .where(eq(schema.job.id, jobId))Without this fix updateJob will always fail when it reaches getReleaseTargetFromJob, preventing automatic chaining of jobs.
Summary by CodeRabbit