fix: Incorrect/missing logs on new schedule engine runs #2185
Conversation
Also added the ability to recover schedules in the schedule engine via an Admin API endpoint in the new schedule engine
WalkthroughThis set of changes introduces a new schedule recovery mechanism for environments, spanning backend API, scheduling engine, and Redis-based job queue layers. A new API route handler allows administrators to trigger recovery of scheduled tasks for a specified environment, verifying authentication and permissions. The scheduling engine gains a public method to recover schedules by checking for missing jobs in the queue and re-registering them as needed, with detailed logging and result reporting. Redis queue and worker classes now support direct job retrieval by ID, with corresponding tests. Buffering logic for event queries is enhanced by extending time ranges on both ends. Optional timestamp overrides are added and propagated through task triggering and tracing components. Comprehensive tests validate the recovery process and queue retrieval features. ✨ 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/webapp/app/v3/services/triggerTaskV1.server.ts (1)
315-317: Duplicate timestamp conversion logic detected.This is the same conversion pattern as in
apps/webapp/app/runEngine/concerns/traceEvents.server.tslines 42-44. As mentioned in the previous file review, consider extracting this logic into a shared utility function to improve maintainability.
🧹 Nitpick comments (3)
apps/webapp/app/runEngine/concerns/traceEvents.server.ts (1)
42-44: Consider extracting timestamp conversion logic to reduce duplication.The timestamp conversion from Date to BigInt nanoseconds is correct, but this exact pattern appears in multiple files. Consider creating a utility function like
dateToNanoseconds(date: Date): bigintto improve maintainability.Example utility function:
+function dateToNanoseconds(date: Date): bigint { + return BigInt(date.getTime()) * BigInt(1000000); +} startTime: request.options?.overrideCreatedAt - ? BigInt(request.options.overrideCreatedAt.getTime()) * BigInt(1000000) + ? dateToNanoseconds(request.options.overrideCreatedAt) : undefined,internal-packages/schedule-engine/src/engine/index.ts (1)
708-711: Optimize verbose logging of large objects.Logging entire schedule and instance objects can be verbose and may contain sensitive information. Consider logging only essential identifiers.
- this.logger.info("Recovering schedule", { - schedule, - instance, - }); + this.logger.info("Recovering schedule", { + scheduleId: schedule.id, + instanceId: instance.id, + environmentId: instance.environmentId, + });internal-packages/schedule-engine/test/scheduleRecovery.test.ts (1)
12-30: Consider extracting common test setup.The test setup code is repetitive across all test cases. Consider extracting common setup logic into a helper function to improve maintainability.
+function createTestEngine(redisOptions: any, prisma: any) { + const mockDevConnectedHandler = vi.fn().mockResolvedValue(true); + const triggerCalls: TriggerScheduledTaskParams[] = []; + + return { + engine: new ScheduleEngine({ + prisma, + redis: redisOptions, + distributionWindow: { seconds: 10 }, + worker: { + concurrency: 1, + disabled: true, + pollIntervalMs: 1000, + }, + tracer: trace.getTracer("test", "0.0.0"), + onTriggerScheduledTask: async (params) => { + triggerCalls.push(params); + return { success: true }; + }, + isDevEnvironmentConnectedHandler: mockDevConnectedHandler, + }), + mockDevConnectedHandler, + triggerCalls, + }; +}Also applies to: 117-135, 221-239, 337-355
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/webapp/app/routes/admin.api.v1.environments.$environmentId.schedules.recover.ts(1 hunks)apps/webapp/app/runEngine/concerns/traceEvents.server.ts(1 hunks)apps/webapp/app/runEngine/types.ts(1 hunks)apps/webapp/app/v3/eventRepository.server.ts(1 hunks)apps/webapp/app/v3/services/triggerTaskV1.server.ts(1 hunks)apps/webapp/app/v3/taskEventStore.server.ts(3 hunks)internal-packages/schedule-engine/src/engine/index.ts(2 hunks)internal-packages/schedule-engine/test/scheduleEngine.test.ts(1 hunks)internal-packages/schedule-engine/test/scheduleRecovery.test.ts(1 hunks)packages/redis-worker/src/queue.test.ts(1 hunks)packages/redis-worker/src/queue.ts(3 hunks)packages/redis-worker/src/worker.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
apps/webapp/app/runEngine/types.ts (1)
24-24: LGTM! Clean addition of timestamp override support.The optional
overrideCreatedAtproperty is well-typed and clearly named, enabling explicit control over task creation timestamps while maintaining backward compatibility.apps/webapp/app/v3/eventRepository.server.ts (1)
973-973: LGTM! Backward-compatible startTime support.The modification properly supports optional
startTimewhile maintaining existing behavior through the fallback togetNowInNanoseconds(). This enables precise timestamp control when needed.apps/webapp/app/v3/taskEventStore.server.ts (1)
85-91: LGTM! Well-implemented time buffering for partitioned queries.The symmetric time buffering approach properly ensures complete event coverage when querying partitioned tables. The consistent implementation across both
findManyandfindTraceEventsmethods and the correct seconds-to-milliseconds conversion make this a solid enhancement for partitioned event queries.Also applies to: 144-147
packages/redis-worker/src/queue.ts (2)
449-467: LGTM - Redis Lua script implementation is correct.The script properly handles the non-existent job case and correctly retrieves both the serialized item and its score from Redis data structures.
642-647: LGTM - Type signature matches implementation.The interface correctly defines the Redis command signature.
packages/redis-worker/src/worker.ts (1)
385-387: LGTM - Simple and correct delegation.The implementation correctly exposes the queue's
getJobfunctionality at the worker level.internal-packages/schedule-engine/test/scheduleEngine.test.ts (1)
4-6: Minor import reordering - no functional impact.packages/redis-worker/src/queue.test.ts (1)
79-129: Excellent test coverage for the new getJob method.The test comprehensively covers:
- Retrieving existing jobs by ID with correct data structure
- Proper null return for non-existent job IDs
- Proper cleanup in the finally block
apps/webapp/app/routes/admin.api.v1.environments.$environmentId.schedules.recover.ts (1)
11-31: LGTM - Proper authentication and authorization implementation.The security flow correctly:
- Authenticates with personal access token
- Verifies user existence
- Enforces admin privilege requirement
This is appropriate for a sensitive admin operation.
internal-packages/schedule-engine/src/engine/index.ts (3)
11-11: LGTM: Import addition is appropriate.The added imports for
TaskScheduleandTaskScheduleInstanceare necessary for the new recovery functionality and properly typed.
742-776: LGTM: Recovery logic is well-implemented.The private helper method correctly:
- Checks for existing jobs before creating new ones
- Uses appropriate error handling patterns
- Provides clear logging for debugging
- Returns meaningful status indicators
The implementation prevents duplicate job creation and follows the existing codebase patterns.
778-780: LGTM: Simple and appropriate delegation.The
getJobmethod properly exposes the worker's functionality and maintains the abstraction layer.internal-packages/schedule-engine/test/scheduleRecovery.test.ts (5)
1-6: LGTM: Proper test setup and imports.The imports and test setup are appropriate for testing the schedule recovery functionality with containerized dependencies.
8-111: LGTM: Comprehensive test for basic recovery scenario.This test properly verifies:
- Initial state (no job exists)
- Recovery process execution
- Final state validation (job created, instance updated)
- Proper resource cleanup
The test structure and assertions are thorough and appropriate.
113-215: LGTM: Important duplicate prevention test.This test correctly validates that the recovery mechanism doesn't create duplicate jobs when active jobs already exist. The job ID comparison ensures the same job instance is preserved.
217-331: LGTM: Good coverage of multiple schedules scenario.The test properly validates batch recovery functionality by:
- Creating multiple schedules with different configurations
- Verifying all schedules are recovered correctly
- Checking individual job creation and instance updates
The loop-based verification approach is efficient and thorough.
333-395: LGTM: Important edge case coverage.This test ensures the recovery mechanism handles empty environments gracefully without throwing errors. This is crucial for production stability.
apps/webapp/app/routes/admin.api.v1.environments.$environmentId.schedules.recover.ts
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal-packages/schedule-engine/test/scheduleRecovery.test.ts (4)
8-111: Test logic is solid, but consider refactoring setup code.This test comprehensively covers the basic recovery scenario. The verification steps are thorough, checking both job creation and database updates. However, the test setup code (lines 34-84) is quite lengthy and will likely be duplicated across test cases.
Consider extracting the common setup logic into helper functions:
+// Helper function to create test organization and project +async function createTestOrgAndProject(prisma: any, suffix: string) { + const organization = await prisma.organization.create({ + data: { + title: `${suffix} Test Org`, + slug: `${suffix.toLowerCase()}-test-org`, + }, + }); + + const project = await prisma.project.create({ + data: { + name: `${suffix} Test Project`, + slug: `${suffix.toLowerCase()}-test-project`, + externalRef: `${suffix.toLowerCase()}-test-ref`, + organizationId: organization.id, + }, + }); + + return { organization, project }; +} + +// Similar helpers for environment and schedule creation
113-215: Excellent test for duplicate prevention, but setup duplication continues.This test is crucial for ensuring the recovery mechanism doesn't create duplicate jobs. The logic properly:
- Registers a schedule first
- Runs recovery
- Verifies the same job ID persists
The assertion on line 210 comparing deduplication keys is particularly valuable for ensuring job integrity.
The setup code (lines 139-189) is nearly identical to the first test. This reinforces the need for setup helper functions to improve maintainability.
217-331: Well-structured test for multiple schedule recovery.This test effectively validates that recovery works for multiple schedules simultaneously. The loop structure (lines 275-302) is clean and creates appropriately varied test data with different cron expressions and identifiers.
The verification loops (lines 305-308, 314-318, 321-326) are comprehensive and ensure all schedules are properly recovered.
The same setup duplication pattern continues here. Consider consolidating the common setup code across all test cases.
1-396: Comprehensive test suite with room for setup optimization.This test suite provides excellent coverage of the schedule recovery functionality with well-structured test cases. The tests validate key scenarios including basic recovery, duplicate prevention, multiple schedules, and edge cases.
Strengths:
- Comprehensive scenario coverage
- Proper use of containerized testing
- Good assertions and verification steps
- Appropriate timeouts and cleanup handling
Areas for improvement:
- Code duplication: The setup code is repeated across tests and could be extracted into helper functions
- Additional test scenarios: Consider adding tests for error conditions (database failures, Redis failures, invalid schedule data)
Would you like me to help create helper functions to reduce the setup code duplication, or generate additional test cases for error scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal-packages/schedule-engine/test/scheduleRecovery.test.ts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.26.0)
internal-packages/schedule-engine/test/scheduleRecovery.test.ts
161-161: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
162-162: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
internal-packages/schedule-engine/test/scheduleRecovery.test.ts (3)
1-6: LGTM: Clean imports and proper test setup.The imports are well-organized and include all necessary dependencies for the schedule recovery tests.
333-395: Good edge case coverage for empty environments.This test appropriately covers the scenario where recovery is called on an environment without schedules. The use of
expect().resolves.not.toThrow()is the correct pattern for verifying graceful handling.While this test has less setup duplication due to not creating schedules, it still shares the organization/project/environment creation pattern with other tests.
161-162: Static analysis false positive: These are test API keys.The static analysis tool flagged these as potential API keys, but they are clearly test fixtures following the pattern
tr_[test_name]_test_1234andpk_[test_name]_test_1234. These are safe test values, not actual secrets.
Also added the ability to recover schedules in the schedule engine via an Admin API endpoint in the new schedule engine