feat: job retry policy via api#601
Conversation
WalkthroughThis change introduces a "retry policy" feature across the API, database, and job dispatch layers. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant API
participant DB
participant JobDispatcher
participant EventsQueue
API->>DB: Create/Update Policy (with maxRetries)
DB-->>API: Policy stored (maxRetries in policy_rule_retry table)
API->>JobDispatcher: Job marked as failure
JobDispatcher->>DB: Fetch policy (get maxRetries)
JobDispatcher->>DB: Count previous retries for job
alt Retries remaining
JobDispatcher->>DB: Create new job for retry
JobDispatcher->>EventsQueue: Dispatch retry job
else Retries exhausted
JobDispatcher->>EventsQueue: Dispatch evaluation for release target
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ 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
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
openapi.v1.json (1)
2696-2701: Same validation / defaulting concerns for create endpointSee previous comment – creation path should perform identical checks & defaulting rules as update.
🧹 Nitpick comments (7)
e2e/tests/api/policies/retry-policy.spec.yaml (1)
24-31: Comprehensive test specification with minor formatting issue.The test setup is well-structured with appropriate entities for testing retry policy functionality. The
maxRetries: 3value is suitable for testing.Fix the missing newline at the end of the file:
maxRetries: 3 +e2e/tests/api/policies/retry-policy.spec.ts (3)
85-85: Consider making timeout values configurable.Hard-coded timeout values make the tests less flexible and may cause flakiness in different environments.
+const SETUP_TIMEOUT = 1_000; +const RETRY_TIMEOUT = 2_000; + test("should retry job until max attempts is reached", async ({ api, page, }) => { const agentId = await initTestEntities(api, builder); - await page.waitForTimeout(1_000); + await page.waitForTimeout(SETUP_TIMEOUT); // ... later in the loop - await page.waitForTimeout(2_000); + await page.waitForTimeout(RETRY_TIMEOUT);Also applies to: 132-132
97-97: Remove debug console.log statements.Console.log statements should be removed from production test code to avoid cluttering test output.
expect(policyResponse.data?.maxRetries).toBeDefined(); - console.log({ policyResponse }); const maxRetries = policyResponse.data?.maxRetries!; // ... and later - console.log({ i, jobResponse });Also applies to: 110-110
79-147: Enhance test robustness with better error handling.The test logic is sound but could benefit from better error handling and more descriptive assertions to improve debugging when tests fail.
Consider wrapping API calls in try-catch blocks or using more descriptive error messages in expect statements to make test failures easier to diagnose:
expect(jobResponse.response.ok).toBe(true); // Could be enhanced to: if (!jobResponse.response.ok) { throw new Error(`Failed to fetch next job: ${jobResponse.response.status} ${jobResponse.response.statusText}`); }openapi.v1.json (2)
6553-6558: Add descriptive context & allow zero-retry scenariosThe
MaxRetriesschema is well-formed but a few small tweaks will improve expressiveness and future-proofing:
- A short description + example makes the field self-documenting in generated SDKs & UIs.
minimum: 1forbids an explicit “no retries” value (0). Ifnull→ “inherit default” and0→ “do not retry” is a desirable distinction, considerminimum: 0."MaxRetries": { "type": "integer", "nullable": true, - "minimum": 1, + "description": "Maximum number of attempts the system will make to run a job. \ +Null to inherit the global default, 0 to disable retries.", + "example": 3, + "minimum": 0, "format": "int32" },Please confirm whether
0should be an allowed value or ifnullalready covers “no retry”.
6650-6652: Property ordering / grouping
maxRetriesnow sits between rollout & the end of the map. To keep diffs tidy in future, consider grouping numeric limit fields (concurrency,maxRetries) together or keeping alphabetical order, but this is purely cosmetic.packages/job-dispatch/src/job-update.ts (1)
303-309: Consider documenting the retry policy selection behavior.The code only uses the first retry policy found. While this appears intentional, it would be helpful to document this behavior for future maintainers.
Add a clarifying comment:
const matchedPolicies = await getMatchedPolicies(db, releaseTarget); const isJobFailed = failedStatuses.includes(updatedJob.status as JobStatus); + // Use the first retry policy found (policies are ordered by priority) const firstRetryPolicy = matchedPolicies.find((p) => p.retry != null)?.retry;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/webservice/src/app/api/v1/policies/[policyId]/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/policies/[policyId]/route.ts(2 hunks)apps/webservice/src/app/api/v1/policies/openapi.ts(3 hunks)e2e/api/entity-fixtures.ts(1 hunks)e2e/api/schema.ts(8 hunks)e2e/tests/api/policies/retry-policy.spec.ts(1 hunks)e2e/tests/api/policies/retry-policy.spec.yaml(1 hunks)openapi.v1.json(4 hunks)packages/db/drizzle/0118_aromatic_polaris.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/policy-relations.ts(2 hunks)packages/db/src/schema/policy.ts(2 hunks)packages/db/src/schema/rules/index.ts(1 hunks)packages/db/src/schema/rules/retry.ts(1 hunks)packages/db/src/schema/rules/rule-relations.ts(2 hunks)packages/db/src/utils/policies/index.ts(1 hunks)packages/events/src/dispatch-jobs.ts(1 hunks)packages/job-dispatch/src/job-update.ts(8 hunks)packages/rule-engine/src/db/create-policy.ts(3 hunks)packages/rule-engine/src/db/update-policy.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
`**/*.ts`: Use TypeScript with explicit types and prefer interfaces for public APIs. Use named imports and group imports by source: standard library first, then external, then inte...
**/*.ts: Use TypeScript with explicit types and prefer interfaces for public APIs.
Use named imports and group imports by source: standard library first, then external, then internal.
Use consistent type imports: 'import type { Type } from "module"'.
packages/db/src/schema/rules/index.tspackages/db/src/utils/policies/index.tsapps/webservice/src/app/api/v1/policies/[policyId]/openapi.tspackages/events/src/dispatch-jobs.tse2e/api/entity-fixtures.tspackages/db/src/schema/policy-relations.tsapps/webservice/src/app/api/v1/policies/[policyId]/route.tspackages/rule-engine/src/db/update-policy.tspackages/db/src/schema/rules/rule-relations.tspackages/db/src/schema/rules/retry.tspackages/rule-engine/src/db/create-policy.tsapps/webservice/src/app/api/v1/policies/openapi.tspackages/db/src/schema/policy.tse2e/api/schema.tspackages/job-dispatch/src/job-update.tse2e/tests/api/policies/retry-policy.spec.ts
`**/*.{js,ts,tsx}`: Formatting must follow Prettier with '@ctrlplane/prettier-config'.
**/*.{js,ts,tsx}: Formatting must follow Prettier with '@ctrlplane/prettier-config'.
packages/db/src/schema/rules/index.tspackages/db/src/utils/policies/index.tsapps/webservice/src/app/api/v1/policies/[policyId]/openapi.tspackages/events/src/dispatch-jobs.tse2e/api/entity-fixtures.tspackages/db/src/schema/policy-relations.tsapps/webservice/src/app/api/v1/policies/[policyId]/route.tspackages/rule-engine/src/db/update-policy.tspackages/db/src/schema/rules/rule-relations.tspackages/db/src/schema/rules/retry.tspackages/rule-engine/src/db/create-policy.tsapps/webservice/src/app/api/v1/policies/openapi.tspackages/db/src/schema/policy.tse2e/api/schema.tspackages/job-dispatch/src/job-update.tse2e/tests/api/policies/retry-policy.spec.ts
`packages/**`: Adhere to file and directory naming conventions specific to each package.
packages/**: Adhere to file and directory naming conventions specific to each package.
packages/db/src/schema/rules/index.tspackages/db/src/utils/policies/index.tspackages/events/src/dispatch-jobs.tspackages/db/drizzle/meta/_journal.jsonpackages/db/drizzle/0118_aromatic_polaris.sqlpackages/db/src/schema/policy-relations.tspackages/rule-engine/src/db/update-policy.tspackages/db/src/schema/rules/rule-relations.tspackages/db/src/schema/rules/retry.tspackages/rule-engine/src/db/create-policy.tspackages/db/src/schema/policy.tspackages/job-dispatch/src/job-update.ts
`**/*.{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.
packages/db/src/schema/rules/index.tspackages/db/src/utils/policies/index.tsapps/webservice/src/app/api/v1/policies/[policyId]/openapi.tspackages/events/src/dispatch-jobs.tse2e/api/entity-fixtures.tspackages/db/src/schema/policy-relations.tsapps/webservice/src/app/api/v1/policies/[policyId]/route.tspackages/rule-engine/src/db/update-policy.tspackages/db/src/schema/rules/rule-relations.tspackages/db/src/schema/rules/retry.tspackages/rule-engine/src/db/create-policy.tsapps/webservice/src/app/api/v1/policies/openapi.tspackages/db/src/schema/policy.tse2e/api/schema.tspackages/job-dispatch/src/job-update.tse2e/tests/api/policies/retry-policy.spec.ts
`apps/**`: Adhere to file and directory naming conventions specific to each package.
apps/**: Adhere to file and directory naming conventions specific to each package.
apps/webservice/src/app/api/v1/policies/[policyId]/openapi.tsapps/webservice/src/app/api/v1/policies/[policyId]/route.tsapps/webservice/src/app/api/v1/policies/openapi.ts
🧬 Code Graph Analysis (7)
packages/db/src/schema/policy-relations.ts (1)
packages/db/src/schema/rules/retry.ts (1)
policyRuleRetry(7-16)
packages/rule-engine/src/db/update-policy.ts (2)
packages/db/src/common.ts (2)
Tx(22-22)buildConflictUpdateColumns(30-46)packages/db/src/schema/policy.ts (2)
UpdatePolicy(223-223)policy(43-63)
packages/db/src/schema/rules/rule-relations.ts (2)
packages/db/src/schema/rules/retry.ts (1)
policyRuleRetry(7-16)packages/db/src/schema/policy.ts (1)
policy(43-63)
packages/db/src/schema/rules/retry.ts (1)
packages/db/src/schema/policy.ts (1)
policy(43-63)
packages/rule-engine/src/db/create-policy.ts (1)
packages/db/src/common.ts (1)
buildConflictUpdateColumns(30-46)
e2e/api/schema.ts (1)
packages/node-sdk/src/schema.ts (1)
components(892-1420)
packages/job-dispatch/src/job-update.ts (7)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/job.ts (3)
Job(144-144)UpdateJob(154-154)JobStatus(145-145)packages/db/src/schema/release.ts (2)
releaseTarget(21-43)ReleaseTarget(45-45)packages/db/src/queries/create-release-job.ts (1)
createReleaseJob(19-95)packages/events/src/dispatch-jobs.ts (1)
dispatchQueueJob(161-166)packages/validators/src/jobs/index.ts (1)
failedStatuses(54-59)
🪛 YAMLlint (1.37.1)
e2e/tests/api/policies/retry-policy.spec.yaml
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (32)
packages/db/drizzle/meta/_journal.json (1)
831-837: Migration journal entry looks correct.The new journal entry follows the established pattern with proper incremental indexing (118), chronological timestamp, and consistent naming convention. The
breakpoints: trueflag correctly indicates this migration introduces schema changes.packages/db/src/schema/rules/index.ts (1)
14-14: LGTM: Proper barrel export for retry rule.The export statement follows the established pattern and correctly exposes the new retry rule module through the index.
e2e/api/entity-fixtures.ts (1)
135-135: LGTM: Appropriate schema extension for retry policy.The
maxRetriesfield is correctly typed as an optional number, which aligns with the retry policy feature and follows the established pattern for optional policy fields.packages/db/src/utils/policies/index.ts (1)
10-10: LGTM: Proper rule registration.The addition of
maxRetries: _truefollows the established pattern and correctly registers the retry rule as an available policy rule type.packages/db/src/schema/policy-relations.ts (2)
18-18: LGTM: Proper import for retry rule.The import statement follows the established pattern and correctly imports the
policyRuleRetryentity needed for the relation definition.
36-36: LGTM: Correct one-to-one relation definition.The
maxRetriesrelation is properly defined as a one-to-one relationship withpolicyRuleRetry. This aligns with the database schema wherepolicyRuleRetryhas a unique constraint onpolicyId, ensuring the one-to-one relationship integrity.apps/webservice/src/app/api/v1/policies/[policyId]/route.ts (1)
49-49: LGTM: Consistent with existing policy rule extraction pattern.The
maxRetriesextraction follows the same nested property access pattern used forconcurrencyandenvironmentVersionRollout. The implementation correctly extracts the value and includes it in the response.Also applies to: 61-61
apps/webservice/src/app/api/v1/policies/[policyId]/openapi.ts (1)
135-137: LGTM: Proper OpenAPI schema documentation.The
maxRetriesproperty is correctly added to the PATCH request body schema using the established$refpattern, maintaining consistency with other policy rule properties.packages/events/src/dispatch-jobs.ts (1)
154-159: LGTM: Clean implementation following established patterns.The new
toDispatchfunction provides a clean interface for dispatching retry jobs and follows the same architectural pattern as existingtoEvaluateandtoComputefunctions.Also applies to: 165-165
packages/db/src/schema/rules/rule-relations.ts (1)
22-22: LGTM: Consistent database relation definition.The
policyRuleRetryRelationsfollows the exact same pattern as other policy rule relations, correctly establishing the one-to-one relationship betweenpolicyRuleRetryandpolicytables.Also applies to: 154-162
packages/rule-engine/src/db/create-policy.ts (1)
96-96: LGTM: Proper implementation of retry rule creation.The
maxRetrieshandling follows the established pattern for policy rules:
- Correctly destructured from input
- Uses appropriate upsert logic with conflict resolution on
policyId- Consistently uses
buildConflictUpdateColumnsfor update behavior- Properly included in the return object
The implementation is consistent with other single-value policy rules like
concurrency.Also applies to: 215-222, 234-234
packages/db/src/schema/rules/retry.ts (2)
7-16: Well-designed retry policy schema.The table structure is solid with appropriate constraints:
- Unique constraint on
policyIdensures one retry rule per policy- Cascade delete maintains referential integrity
- Non-null
maxRetriesenforces explicit retry limits
18-20: Clean Zod schema definition.The insert schema correctly omits the auto-generated
idfield and validatespolicyIdas a UUID string.packages/db/drizzle/0118_aromatic_polaris.sql (1)
1-8: Migration correctly implements the retry policy schema.The SQL structure matches the TypeScript schema definition with proper constraints and foreign key relationships.
packages/db/src/schema/policy.ts (1)
209-216: Correct validation implementation for update schema.The validation logic and error message path are properly implemented for the update schema.
apps/webservice/src/app/api/v1/policies/openapi.ts (2)
78-83: Well-defined MaxRetries schema component.The schema follows the established pattern used by other similar fields like
PolicyConcurrency, with appropriate nullable integer type and minimum validation.
144-146: Properly integrated maxRetries into OpenAPI specification.The
maxRetriesfield is correctly added to both the Policy response schema and request body schema using proper $ref references.Also applies to: 220-222
e2e/tests/api/policies/retry-policy.spec.ts (2)
1-10: LGTM - Clean imports and setup.The imports follow the coding guidelines with proper grouping (standard library, external, internal) and the path setup is appropriate for E2E tests.
66-77: LGTM - Proper test setup and cleanup.The beforeEach and afterEach hooks are properly structured to ensure test isolation and cleanup.
openapi.v1.json (1)
2466-2469: Ensure validation parity for PATCH payload
maxRetriesis now accepted on update, but no additional request-side validation is declared beyond the shared schema reference.
Double-check that server-side code:
- rejects negative integers / non-integers,
- correctly interprets
nullvs missing vs0(if permitted).Otherwise clients could silently send invalid values.
packages/rule-engine/src/db/update-policy.ts (5)
202-220: Excellent implementation following established patterns!The
updateMaxRetriesfunction correctly implements the three-state pattern (undefined/null/value) consistent with other policy rule update functions. The upsert logic with conflict resolution is appropriate for atomic updates.
235-236: Proper integration of maxRetries extraction.The destructuring correctly extracts
maxRetriesfrom the input alongside other policy rule properties.
263-263: Correct concurrent execution of update operations.Adding
updateMaxRetriesto thePromise.allarray ensures it executes concurrently with other policy rule updates within the same transaction.
284-284: Proper extraction of nested maxRetries value.The optional chaining
updatedPolicy.maxRetries?.maxRetriescorrectly handles the database relation structure, matching the pattern used for other nested properties likeconcurrency.
289-289: Complete integration of maxRetries in return object.The returned object properly includes the extracted
maxRetriesvalue, maintaining consistency with the API contract.e2e/api/schema.ts (5)
1418-1419: Well-structured schema definition for MaxRetries.The new
MaxRetriesschema follows the same pattern asPolicyConcurrency, ensuring consistency in the API schema. Theint32format and nullable type are appropriate for retry count values.
1454-1454: Proper integration of maxRetries into Policy schema.The optional
maxRetriesproperty is correctly added to the Policy schema, following the established patterns and positioned logically with other deployment control properties.
1695-1695: Verify the business impact of making approval/rejection reasons optional.The changes make the
reasonfield optional and allow empty request bodies for approval/rejection endpoints. While this improves API flexibility, it may reduce audit trail quality if reasons become less frequently provided.Please verify that making approval/rejection reasons optional aligns with business requirements and audit/compliance needs.
Also applies to: 1756-1756, 1941-1941, 2000-2000
3138-3138: Consistent integration into updatePolicy operation.The
maxRetriesproperty is properly integrated into the updatePolicy request body, maintaining consistency with the Policy schema and following established patterns for optional update fields.
3243-3243: Complete feature integration in upsertPolicy operation.The
maxRetriesproperty integration in upsertPolicy ensures complete API coverage for the retry policy feature, maintaining consistency across all policy-related operations.packages/job-dispatch/src/job-update.ts (2)
99-115: Good refactoring to extract database update logic.The extraction of the database update logic into
dbUpdateJobimproves modularity and makes the code more maintainable.
146-183: Well-structured query for fetching policies with their rules.The updated
getMatchedPoliciesfunction correctly uses left joins for optional policy rules and maintains proper ordering by priority.
| maxRetries: z | ||
| .number() | ||
| .optional() | ||
| .nullable() | ||
| .refine((data) => data == null || data > 0, { | ||
| message: "Retry must be greater than 0 or null", | ||
| path: ["retry"], | ||
| }), |
There was a problem hiding this comment.
Fix inconsistent error message path.
The validation logic is correct, but the error message path should match the field name.
.refine((data) => data == null || data > 0, {
message: "Retry must be greater than 0 or null",
- path: ["retry"],
+ path: ["maxRetries"],
}),🤖 Prompt for AI Agents
In packages/db/src/schema/policy.ts around lines 166 to 173, the error message
path in the refine method is set to "retry" which is inconsistent with the field
name "maxRetries". Update the path property in the refine method to
["maxRetries"] to match the field name for accurate error reporting.
| expect(agentResponse.data?.id).toBeDefined(); | ||
| return agentResponse.data?.id!; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety check for agent ID.
Using non-null assertion without proper validation could lead to runtime errors if the API response doesn't contain an ID.
- expect(agentResponse.data?.id).toBeDefined();
- return agentResponse.data?.id!;
+ expect(agentResponse.data?.id).toBeDefined();
+ const agentId = agentResponse.data?.id;
+ if (!agentId) {
+ throw new Error("Failed to create agent: no ID returned");
+ }
+ return agentId;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(agentResponse.data?.id).toBeDefined(); | |
| return agentResponse.data?.id!; | |
| expect(agentResponse.data?.id).toBeDefined(); | |
| const agentId = agentResponse.data?.id; | |
| if (!agentId) { | |
| throw new Error("Failed to create agent: no ID returned"); | |
| } | |
| return agentId; |
🤖 Prompt for AI Agents
In e2e/tests/api/policies/retry-policy.spec.ts around lines 23 to 24, the code
uses a non-null assertion on agentResponse.data?.id without verifying its
presence, which risks runtime errors if the ID is missing. Add a null safety
check before returning the ID by validating that agentResponse.data?.id is
defined; if not, handle the error appropriately or throw an informative
exception to prevent unsafe access.
There was a problem hiding this comment.
since this is just a test, i'm not really concerned with this
There was a problem hiding this comment.
@adityachoudhari26 Understood! You're right that tests often prioritize simplicity and readability over defensive programming. I'll keep that in mind for future test reviews.
(\ /)
( .. )
o(")(") acknowledging your preference
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.706Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const nextJob = nextJobs?.[0]; | ||
| expect(nextJob).toBeDefined(); | ||
| expect(nextJob?.status).toBe("pending"); | ||
|
|
||
| await api.PATCH("/v1/jobs/{jobId}", { | ||
| params: { | ||
| path: { | ||
| jobId: nextJob?.id!, | ||
| }, | ||
| }, | ||
| body: { | ||
| status: "failure", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety check for job operations.
The job processing logic uses non-null assertions that should be replaced with proper validation.
const nextJob = nextJobs?.[0];
expect(nextJob).toBeDefined();
+ if (!nextJob?.id) {
+ throw new Error("Job missing required ID");
+ }
expect(nextJob?.status).toBe("pending");
await api.PATCH("/v1/jobs/{jobId}", {
params: {
path: {
- jobId: nextJob?.id!,
+ jobId: nextJob.id,
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nextJob = nextJobs?.[0]; | |
| expect(nextJob).toBeDefined(); | |
| expect(nextJob?.status).toBe("pending"); | |
| await api.PATCH("/v1/jobs/{jobId}", { | |
| params: { | |
| path: { | |
| jobId: nextJob?.id!, | |
| }, | |
| }, | |
| body: { | |
| status: "failure", | |
| }, | |
| }); | |
| const nextJob = nextJobs?.[0]; | |
| expect(nextJob).toBeDefined(); | |
| if (!nextJob?.id) { | |
| throw new Error("Job missing required ID"); | |
| } | |
| expect(nextJob?.status).toBe("pending"); | |
| await api.PATCH("/v1/jobs/{jobId}", { | |
| params: { | |
| path: { | |
| jobId: nextJob.id, | |
| }, | |
| }, | |
| body: { | |
| status: "failure", | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In e2e/tests/api/policies/retry-policy.spec.ts around lines 117 to 130, replace
the non-null assertion on nextJob.id with a proper null safety check. Before
using nextJob.id in the PATCH request, add a conditional check to ensure nextJob
is defined and handle the case where it is undefined to avoid runtime errors.
This will make the job operations safer and more robust.
There was a problem hiding this comment.
same as above, I dont' really care about type safety in a test because if something is off the test will fail anyways
There was a problem hiding this comment.
@adityachoudhari26 That's a fair point! You're right that in e2e tests, if the job ID is missing or undefined, the test failure itself would indicate a problem with the system being tested. I'll respect your preference to keep the non-null assertions in test code for simplicity.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:117-130
Timestamp: 2025-06-24T23:53:25.389Z
Learning: User adityachoudhari26 prefers to keep non-null assertions in e2e test code without extensive null safety checks, reasoning that test failures serve the same purpose of catching issues and the extra validation doesn't add much value in test contexts.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| expect(deploymentResponse.data?.id).toBeDefined(); | ||
|
|
||
| const versionTag = faker.string.alphanumeric(10); | ||
| const versionResponse = await api.POST("/v1/deployment-versions", { | ||
| body: { | ||
| deploymentId: deploymentResponse.data?.id!, | ||
| tag: versionTag, | ||
| }, | ||
| }); | ||
| expect(versionResponse.data?.id).toBeDefined(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety checks for deployment creation.
Similar to the agent creation, these non-null assertions should be replaced with proper validation.
expect(deploymentResponse.data?.id).toBeDefined();
+ const deploymentId = deploymentResponse.data?.id;
+ if (!deploymentId) {
+ throw new Error("Failed to create deployment: no ID returned");
+ }
const versionTag = faker.string.alphanumeric(10);
const versionResponse = await api.POST("/v1/deployment-versions", {
body: {
- deploymentId: deploymentResponse.data?.id!,
+ deploymentId,
tag: versionTag,
},
});
expect(versionResponse.data?.id).toBeDefined();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(deploymentResponse.data?.id).toBeDefined(); | |
| const versionTag = faker.string.alphanumeric(10); | |
| const versionResponse = await api.POST("/v1/deployment-versions", { | |
| body: { | |
| deploymentId: deploymentResponse.data?.id!, | |
| tag: versionTag, | |
| }, | |
| }); | |
| expect(versionResponse.data?.id).toBeDefined(); | |
| expect(deploymentResponse.data?.id).toBeDefined(); | |
| const deploymentId = deploymentResponse.data?.id; | |
| if (!deploymentId) { | |
| throw new Error("Failed to create deployment: no ID returned"); | |
| } | |
| const versionTag = faker.string.alphanumeric(10); | |
| const versionResponse = await api.POST("/v1/deployment-versions", { | |
| body: { | |
| deploymentId, | |
| tag: versionTag, | |
| }, | |
| }); | |
| expect(versionResponse.data?.id).toBeDefined(); |
🤖 Prompt for AI Agents
In e2e/tests/api/policies/retry-policy.spec.ts around lines 42 to 51, the code
uses non-null assertions on deploymentResponse.data?.id without validating if
deploymentResponse.data or its id is actually defined. Replace these non-null
assertions with explicit null or undefined checks before using the id, and
handle the case where deploymentResponse.data or id is missing by throwing an
error or failing the test early to ensure null safety.
There was a problem hiding this comment.
@adityachoudhari26 Exactly! The same null safety pattern should be applied consistently throughout the test file. This approach of explicit validation and early error throwing will make the tests more robust and provide clearer failure messages when API calls don't return expected data.
The pattern helps avoid runtime errors from non-null assertions and makes the test failures more debuggable by showing exactly which API call failed to return the expected data.
| expect(policyResponse.data?.maxRetries).toBeDefined(); | ||
| console.log({ policyResponse }); | ||
| const maxRetries = policyResponse.data?.maxRetries!; | ||
| expect(maxRetries).toBeGreaterThan(0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety check for maxRetries.
The non-null assertion should be replaced with proper validation and error handling.
expect(policyResponse.data?.maxRetries).toBeDefined();
- console.log({ policyResponse });
- const maxRetries = policyResponse.data?.maxRetries!;
+ const maxRetries = policyResponse.data?.maxRetries;
+ if (typeof maxRetries !== 'number' || maxRetries <= 0) {
+ throw new Error(`Invalid maxRetries value: ${maxRetries}`);
+ }
expect(maxRetries).toBeGreaterThan(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(policyResponse.data?.maxRetries).toBeDefined(); | |
| console.log({ policyResponse }); | |
| const maxRetries = policyResponse.data?.maxRetries!; | |
| expect(maxRetries).toBeGreaterThan(0); | |
| expect(policyResponse.data?.maxRetries).toBeDefined(); | |
| const maxRetries = policyResponse.data?.maxRetries; | |
| if (typeof maxRetries !== 'number' || maxRetries <= 0) { | |
| throw new Error(`Invalid maxRetries value: ${maxRetries}`); | |
| } | |
| expect(maxRetries).toBeGreaterThan(0); |
🤖 Prompt for AI Agents
In e2e/tests/api/policies/retry-policy.spec.ts around lines 96 to 99, replace
the non-null assertion on maxRetries with a proper null safety check. Add a
conditional to verify maxRetries is not null or undefined before using it, and
handle the case where it might be missing by throwing an error or failing the
test with a clear message. This ensures robust validation without assuming
maxRetries is always present.
There was a problem hiding this comment.
@adityachoudhari26 Perfect! I understand you agree with the null safety approach for handling the maxRetries validation. Replacing the non-null assertion with proper type checking and error handling makes the test more robust and provides clearer failure messages when the API response doesn't contain the expected data.
The suggested change improves the test by:
- Removing assumptions about API response structure
- Providing clear error messages when
maxRetriesis missing or invalid - Following better testing practices with explicit validation
Thanks for confirming this approach!
🐰✨
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:96-99
Timestamp: 2025-06-24T23:53:36.318Z
Learning: In e2e test files, prefer explicit null safety checks and validation over non-null assertions (!). When validating API responses in tests, use type guards and throw descriptive errors rather than assuming values are always present.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const newReleaseJob = await db.transaction((tx) => | ||
| createReleaseJob(tx, { | ||
| id: releaseId, | ||
| versionReleaseId, | ||
| variableReleaseId, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Remove unnecessary transaction wrapper.
The db parameter is already of type Tx (transaction context), and createReleaseJob is designed to work within a transaction. The additional transaction wrapper is redundant.
Apply this fix:
- const newReleaseJob = await db.transaction((tx) =>
- createReleaseJob(tx, {
- id: releaseId,
- versionReleaseId,
- variableReleaseId,
- }),
- );
+ const newReleaseJob = await createReleaseJob(db, {
+ id: releaseId,
+ versionReleaseId,
+ variableReleaseId,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const newReleaseJob = await db.transaction((tx) => | |
| createReleaseJob(tx, { | |
| id: releaseId, | |
| versionReleaseId, | |
| variableReleaseId, | |
| }), | |
| ); | |
| const newReleaseJob = await createReleaseJob(db, { | |
| id: releaseId, | |
| versionReleaseId, | |
| variableReleaseId, | |
| }); |
🤖 Prompt for AI Agents
In packages/job-dispatch/src/job-update.ts around lines 264 to 270, remove the
unnecessary db.transaction wrapper since the db parameter is already a
transaction context (Tx). Instead, call createReleaseJob directly with the db
parameter and the job details object, eliminating the redundant transaction
nesting.
There was a problem hiding this comment.
the tX is just a type meaning db client. if you look at the callers its not actually happening within a tx. even if it was, drizzle supports breakpoints in tx so we would be good either way I believe
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for callers of updateJob function to understand the context
rg "updateJob\(" -A 3 -B 3 --type ts
# Also search for the Tx type definition
rg "type Tx" -A 2 -B 2 --type ts
# Look for createReleaseJob implementation
rg "createReleaseJob" -A 5 -B 2 --type ts
Length of output: 12005
@adityachoudhari26 You're absolutely right, and thank you for the correction! I can see from the codebase that:
Txis indeed just a type alias (export type Tx = Omit<typeof db, "$client">) representing the database client, not necessarily an active transaction context- Looking at the callers of
updateJob, they pass the regulardbclient, not within transactions - Other usages of
createReleaseJobin the codebase (like inevaluate-release-target.tsandredeploy.ts) also wrap it withdb.transaction(), confirming thatcreateReleaseJobexpects to be called within a transaction context - You're also correct that Drizzle supports nested transactions via breakpoints, so even if this were already in a transaction, it would work fine
My apologies for the confusion - the transaction wrapper is indeed necessary here. The code is correct as written.
🐰 ears perk up with curiosity
Thanks for teaching me something new about the codebase architecture!
🧬 Code Graph Analysis Results
Relevant snippets for understanding packages/job-dispatch/src/job-update.ts and the user comment about Tx type usage:
File: packages/db/src/common.ts
Lines: 22-22
export type Tx = Omit<typeof db, "$client">;- Defines the
Txtype as the database client type without the$clientproperty. - This type alias is used in
job-update.tsto type thedbparameter, indicating it represents a database client instance (not necessarily a transaction). - This supports the user's comment that
Txis just a type meaning "db client" and does not imply an actual transaction context.
File: packages/db/src/client.ts
Line: 15
export const db = drizzle(pool, { schema });- Exports the main database client instance
db. - This instance is passed around as
dbortxin other files. - The
Txtype is derived from this client, reinforcing thatTxis a client type, not necessarily a transaction.
File: packages/db/src/queries/create-release-job.ts
Lines: 19-95 (summary)
- Defines
createReleaseJob(tx: Tx, release: {...})which creates a job within a database context typed asTx. - Uses
txto perform multiple queries and inserts. - The function is called inside a transaction in
retryJobinjob-update.tsviadb.transaction((tx) => createReleaseJob(tx, {...})). - This shows that
Txcan be a transaction context but is also used as a general db client type. - Important for understanding how
Txis used injob-update.tsand the user comment about transaction breakpoints.
File: packages/events/src/dispatch-jobs.ts
Lines: 161-166
export const dispatchQueueJob = () => ({
toUpdatedResource: dispatchUpdatedResourceJob,
toEvaluate,
toCompute,
toDispatch,
});- Provides the
dispatchQueueJobfunction used injob-update.tsto dispatch jobs to various queues. - Relevant for understanding the dispatching logic after job updates.
File: packages/validators/src/jobs/index.ts
Lines: 35-43 and 54-59
export const exitedStatus = [
JobStatus.Successful,
JobStatus.InvalidJobAgent,
JobStatus.InvalidIntegration,
JobStatus.ExternalRunNotFound,
JobStatus.Failure,
JobStatus.Cancelled,
JobStatus.Skipped,
];
export const failedStatuses = [
JobStatus.Failure,
JobStatus.InvalidJobAgent,
JobStatus.InvalidIntegration,
JobStatus.ExternalRunNotFound,
];- Defines arrays of job statuses used in
job-update.tsto determine if a job has exited or failed. - Used in logic to decide whether to retry a job or dispatch further evaluation.
File: packages/db/src/schema/job.ts
Lines: 140-154 (summary)
- Defines types
JobMetadata,Job,UpdateJob, andJobStatusbased on the database schema. - These types are used extensively in
job-update.tsfor typing job data and updates.
These snippets provide the necessary context to understand the Tx type usage in job-update.ts, the database client and transaction distinction, and the job update and dispatch logic.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: packages/job-dispatch/src/job-update.ts:264-270
Timestamp: 2025-06-24T23:56:54.778Z
Learning: In this codebase, the `Tx` type is just an alias for the database client type (`Omit<typeof db, "$client">`) and does not necessarily indicate an active transaction context. Functions like `createReleaseJob` need to be called within a transaction, which is why they are wrapped with `db.transaction()` even when the parameter is typed as `Tx`. Drizzle supports nested transactions via breakpoints, so additional transaction wrappers are safe even if already within a transaction.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit