Use redis worker for run heartbeats and alerts #1669
Conversation
|
Warning Rate limit exceeded@ericallam has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis pull request introduces extensive configuration and service enhancements. New configuration parameters for both legacy run engine workers and common workers are added, including Redis connectivity and concurrency options. Several worker services and alerting systems have been refactored to remove deprecated task handlers and streamline enqueuing flows by eliminating redundant transaction parameters. Additionally, internal Redis worker packages have been upgraded with enhanced type safety, telemetry, and concurrency management. Changes also include modified TypeScript configurations and dependency additions to support the new Redis worker functionality, alongside updates in task triggers for error simulation. Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 4
🧹 Nitpick comments (9)
internal-packages/redis-worker/src/queue.test.ts (2)
33-42: UsetoMatchObjectfor concise and readable assertionsTo simplify the test assertions, consider using
expect(...).toMatchObject({...})instead ofexpect(...).toEqual(expect.objectContaining({...})). This makes the code more concise and improves readability.Apply this diff to update the assertion:
- expect(first).toEqual( - expect.objectContaining({ - id: "1", - job: "test", - item: { value: 1 }, - visibilityTimeoutMs: 2000, - attempt: 0, - timestamp: expect.any(Date), - }) - ); + expect(first).toMatchObject({ + id: "1", + job: "test", + item: { value: 1 }, + visibilityTimeoutMs: 2000, + attempt: 0, + timestamp: expect.any(Date), + });
232-251: Refactor repeated assertions into a helper functionMultiple test cases use similar assertions to verify dequeued items. Consider refactoring these into a helper function or custom matcher to reduce code duplication and enhance maintainability.
For example, you could create a helper function:
function expectDequeuedItem(item: QueueItem<any>, expected: Partial<QueueItem<any>>) { expect(item).toMatchObject({ visibilityTimeoutMs: expected.visibilityTimeoutMs, attempt: expected.attempt, timestamp: expect.any(Date), ...expected, }); }And use it in your tests:
expectDequeuedItem(first, { id: "1", job: "test", item: { value: 1 }, visibilityTimeoutMs: 2000, attempt: 0, });Also applies to: 263-271, 318-327
apps/webapp/app/v3/commonWorker.server.ts (1)
22-23: Avoid logging sensitive connection detailsLogging the Redis host in debug statements can expose sensitive infrastructure details. Consider removing or obfuscating such information in logs, especially in production environments.
Apply this diff to modify the log statement:
- logger.debug(`👨🏭 Initializing common worker at host ${env.COMMON_WORKER_REDIS_HOST}`); + logger.debug(`👨🏭 Initializing common worker`);internal-packages/redis-worker/src/worker.ts (2)
11-11: Add type annotations forpLimitimportTo maintain type safety, specify the type for the
pLimitimport from thep-limitlibrary. This helps prevent any TypeScript compilation issues and improves code clarity.Apply this diff to add type annotations:
-import pLimit from "p-limit"; +import pLimit, { Limit } from "p-limit";
75-99: Review concurrency limiter configuration for performanceThe implementation introduces a global concurrency limiter using
p-limit. Ensure that thelimitvalue is appropriately set to balance performance and resource utilization. An excessively low limit might hinder throughput, while a high limit could strain system resources.apps/webapp/app/v3/legacyRunEngineWorker.server.ts (1)
28-36: Consider adjusting visibility timeout and retry settings.The current settings might need adjustment:
- 60 seconds visibility timeout might be too short for long-running tasks
- 3 retry attempts might not be sufficient for transient failures
Consider:
- visibilityTimeoutMs: 60_000, - retry: { - maxAttempts: 3, - }, + visibilityTimeoutMs: 300_000, // 5 minutes + retry: { + maxAttempts: 5, + backoff: { + type: "exponential", + minMs: 1000, + maxMs: 60000, + }, + },apps/webapp/app/v3/taskRunHeartbeatFailed.server.ts (1)
37-39: Update log messages to use new class name.The log messages still reference the old class name
RequeueTaskRunService. Update them to maintain consistency.- logger.error("[RequeueTaskRunService] Task run not found", { + logger.error("[TaskRunHeartbeatFailedService] Task run not found", { runId, }); - "[RequeueTaskRunService] Failing task run because the heartbeat failed and it's PENDING but locked", + "[TaskRunHeartbeatFailedService] Failing task run because the heartbeat failed and it's PENDING but locked", { taskRun } - logger.debug("[RequeueTaskRunService] Nacking task run", { taskRun }); + logger.debug("[TaskRunHeartbeatFailedService] Nacking task run", { taskRun }); - logger.debug("[RequeueTaskRunService] Failing task run", { taskRun }); + logger.debug("[TaskRunHeartbeatFailedService] Failing task run", { taskRun }); - logger.debug("[RequeueTaskRunService] Removing task run from queue", { taskRun }); + logger.debug("[TaskRunHeartbeatFailedService] Removing task run from queue", { taskRun }); - logger.debug("[RequeueTaskRunService] Requeueing task run", { taskRun }); + logger.debug("[TaskRunHeartbeatFailedService] Requeueing task run", { taskRun }); - logger.error("[RequeueTaskRunService] Error signaling run cancellation", { + logger.error("[TaskRunHeartbeatFailedService] Error signaling run cancellation", {Also applies to: 48-50, 65-65, 74-74, 93-93, 104-104, 138-141
references/v3-catalog/src/trigger/simple.ts (1)
109-111: Rename task to reflect delayed return.The task name
immediateReturnis misleading as it now includes a 20-second delay.-export const immediateReturn = task({ - id: "immediateReturn", +export const delayedReturn = task({ + id: "delayedReturn",internal-packages/redis-worker/package.json (1)
24-24: Consider documenting the reason for disabling file parallelism.While disabling file parallelism can help with test stability, it would be helpful to document the specific reason in a comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/services/worker.server.ts(1 hunks)apps/webapp/app/v3/commonWorker.server.ts(1 hunks)apps/webapp/app/v3/legacyRunEngineWorker.server.ts(1 hunks)apps/webapp/app/v3/marqs/index.server.ts(2 hunks)apps/webapp/app/v3/marqs/v3VisibilityTimeout.server.ts(1 hunks)apps/webapp/app/v3/services/alerts/deliverAlert.server.ts(2 hunks)apps/webapp/app/v3/services/alerts/performDeploymentAlerts.server.ts(2 hunks)apps/webapp/app/v3/services/alerts/performTaskAttemptAlerts.server.ts(0 hunks)apps/webapp/app/v3/services/alerts/performTaskRunAlerts.server.ts(2 hunks)apps/webapp/app/v3/services/createDeployedBackgroundWorker.server.ts(1 hunks)apps/webapp/app/v3/services/deploymentIndexFailed.server.ts(1 hunks)apps/webapp/app/v3/services/failDeployment.server.ts(1 hunks)apps/webapp/app/v3/services/finalizeDeployment.server.ts(1 hunks)apps/webapp/app/v3/services/finalizeTaskRun.server.ts(1 hunks)apps/webapp/app/v3/services/timeoutDeployment.server.ts(1 hunks)apps/webapp/app/v3/taskRunHeartbeatFailed.server.ts(1 hunks)apps/webapp/package.json(1 hunks)apps/webapp/remix.config.js(1 hunks)apps/webapp/tsconfig.json(1 hunks)internal-packages/redis-worker/package.json(2 hunks)internal-packages/redis-worker/src/index.ts(1 hunks)internal-packages/redis-worker/src/queue.test.ts(7 hunks)internal-packages/redis-worker/src/queue.ts(11 hunks)internal-packages/redis-worker/src/telemetry.ts(1 hunks)internal-packages/redis-worker/src/worker.test.ts(5 hunks)internal-packages/redis-worker/src/worker.ts(4 hunks)internal-packages/redis-worker/tsconfig.json(1 hunks)references/v3-catalog/src/trigger/simple.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/v3/services/alerts/performTaskAttemptAlerts.server.ts
✅ Files skipped from review due to trivial changes (1)
- internal-packages/redis-worker/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (48)
internal-packages/redis-worker/src/queue.ts (9)
16-24: New type definitions enhance type safety and clarityThe addition of
AnyMessageCatalogandQueueItemtypes improves type safety and readability. This allows for better abstraction and more robust type checking throughout the queue implementation.
26-34: Addition ofAnyQueueItemtype for flexibilityIntroducing the
AnyQueueItemtype provides a non-generic version ofQueueItem, which increases flexibility when dealing with queues that may handle various message types without specified catalogs.
55-55: Ensure consistent key hashing in Redis clustersUsing curly braces in the
keyPrefix({queue:${name}:}) ensures that all Redis keys for this queue are hashed to the same slot in a clustered environment. This is crucial for data consistency and efficient key retrieval.
129-129: Enhanced type safety indequeuemethodUpdating the return type of the
dequeuemethod toPromise<Array<QueueItem<TMessageCatalog>>>enhances type safety and clarifies the expected structure of dequeued items.
141-149: Includetimestampin dequeued items for better trackingBy extracting the
scorefrom Redis and converting it to aDate, the dequeued items now include atimestamp. This enhancement allows for improved tracking of when items were enqueued or became available.
355-355: Modify Lua script to returnscorefor timestamp inclusionUpdating the
dequeueItemsLua script to includescorein the returned results ensures that thetimestampcan be accurately set for each dequeued item. This change aligns the script with the updated TypeScript logic.
396-397: Accurate timestamp retrieval using Redis server timeUsing
redis.call('TIME')in themoveToDeadLetterQueueLua script ensures that the current server time is accurately captured, which is important for setting timestamps consistently across different nodes.
425-427: Consistent timestamp calculation inredriveFromDeadLetterQueueSimilar to the previous change, using
redis.call('TIME')here ensures that items redriven from the dead letter queue have accurate timestamps based on the server time.
460-461: Update TypeScript definitions to match updated Lua scriptModifying the
dequeueItemsmethod's TypeScript definitions to include thescorein the returned array aligns the type definitions with the changes in the Lua script. This ensures type consistency and prevents potential runtime errors.internal-packages/redis-worker/src/queue.test.ts (5)
50-59: UsetoMatchObjectfor concise and readable assertionsAs previously mentioned, replacing
toEqual(expect.objectContaining({...}))withtoMatchObject({...})simplifies the assertion.
90-99: UsetoMatchObjectfor concise and readable assertionsAs previously mentioned, consider updating the assertion to use
toMatchObjectfor better readability.
141-149: UsetoMatchObjectfor concise and readable assertionsAs previously mentioned, updating the assertion to
toMatchObjectenhances code clarity.
176-184: UsetoMatchObjectfor concise and readable assertionsAs earlier suggested, consider using
toMatchObjectto simplify the test assertion.
193-201: UsetoMatchObjectfor concise and readable assertionsRepeating the suggestion to utilize
toMatchObjectfor a more concise assertion.apps/webapp/app/v3/services/alerts/performTaskRunAlerts.server.ts (2)
7-7: Update import to reflect new worker implementationThe import statement now brings in
commonWorkerfrom~/v3/commonWorker.server, replacing the previousworkerQueue. Ensure that this change is consistently applied throughout the codebase to prevent any discrepancies.
64-70: Confirm correct job registration incommonWorkerEnsure that the job
"v3.performTaskRunAlerts"is correctly registered in thecommonWorkercatalog and that it has the appropriate handler defined. This is crucial for the enqueued job to be processed successfully.Run the following script to verify job registration:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if 'v3.performTaskRunAlerts' is registered in 'commonWorker' ast-grep --pattern $'commonWorker = new RedisWorker({ $$$ catalog: { $$$ "v3.performTaskRunAlerts": $_ $$$ }, $$$ })' apps/webapp/app/v3/commonWorker.server.tsLength of output: 180
I'll generate a new shell script using
rgto search for"v3.performTaskRunAlerts"in the file to gather more definitive information about its registration. This will help verify whether the job is indeed registered in the commonWorker catalog.
🏁 Script executed:
#!/bin/bash # Searching for the registration of 'v3.performTaskRunAlerts' in commonWorker.server.ts using ripgrep rg -A 5 -B 5 '"v3\.performTaskRunAlerts"' apps/webapp/app/v3/commonWorker.server.tsLength of output: 720
Job Registration Verified for
v3.performTaskRunAlertsThe script output shows that the job is properly registered in the
commonWorkercatalog with the expected schema and handler. Specifically, the registration includes:
- A schema validating that
runIdis a string.- A handler that invokes
PerformTaskRunAlertsServicewith the providedrunId.apps/webapp/app/v3/commonWorker.server.ts (2)
11-91: Ensure secure handling of Redis credentials and TLS settingsSensitive information such as Redis passwords and TLS configurations should be handled securely. Verify that environment variables like
COMMON_WORKER_REDIS_PASSWORDare encrypted or stored securely, and consider enforcing TLS connections by default to enhance security.[security]
82-88: Validate worker startup configurationThe worker starts only if
env.WORKER_ENABLED === "true". Ensure that this environment variable is correctly set in all deployment environments to prevent unintended behavior where the worker is not processing jobs.internal-packages/redis-worker/src/worker.ts (1)
262-329: Ensure accurate calculation in retry logicIn the
processItemmethod, verify that the retry delay calculation correctly respects both the default and catalog-specific retry settings. Edge cases wherecalculateNextRetryDelaymight returnundefinedor unexpected values should be handled to prevent infinite retries or immediate movement to the dead-letter queue.Consider adding unit tests to validate
calculateNextRetryDelaybehavior under various scenarios.internal-packages/redis-worker/src/telemetry.ts (1)
3-31: LGTM! Well-structured telemetry implementation.The
startSpanfunction is well-implemented with:
- Comprehensive error handling for different error types
- Proper span cleanup in finally block
- Type-safe implementation using generics
apps/webapp/remix.config.js (1)
14-15: LGTM! Required dependencies added for Redis worker implementation.The additions of
p-limitandyocto-queueare appropriate for implementing concurrency control in the Redis worker.apps/webapp/app/v3/marqs/v3VisibilityTimeout.server.ts (2)
5-13: LGTM! Clean implementation of Graphile visibility timeout.The renamed class maintains the same interface while using the new
TaskRunHeartbeatFailedService.
15-28: LGTM! Well-structured implementation for legacy run engine worker.The new class properly:
- Formats heartbeat job IDs with a prefix
- Sets correct availability time for heartbeats
- Handles cleanup through worker acknowledgment
apps/webapp/app/v3/services/failDeployment.server.ts (1)
52-52: LGTM! Simplified service interface.Removed unnecessary
this._prismaparameter as part of the Redis worker refactor.apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
42-42: LGTM!The simplified call to
PerformDeploymentAlertsService.enqueuealigns with the broader refactoring of worker services.apps/webapp/app/v3/services/deploymentIndexFailed.server.ts (1)
69-69: LGTM!The simplified call to
PerformDeploymentAlertsService.enqueuemaintains consistency with other services.apps/webapp/app/v3/services/alerts/performDeploymentAlerts.server.ts (2)
49-59: LGTM!The simplified alert creation and delivery process maintains data integrity while reducing complexity.
Also applies to: 61-61
64-70: Verify Redis worker configuration.The implementation using
commonWorkerlooks good, but ensure Redis connection settings are properly configured in the environment.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for Redis configuration in environment files rg -A 1 'REDIS_(HOST|PORT|USERNAME|PASSWORD|TLS)'Length of output: 21950
Action: Redis Worker Configuration is Properly Set Up
The Redis connection settings are correctly defined across the codebase. In particular:
- The environment configuration in
apps/webapp/app/env.server.tsdefines all necessary Redis variables (e.g.,REDIS_HOST,REDIS_PORT) and maps them to worker-specific settings (e.g.,COMMON_WORKER_REDIS_HOST).- The configurations in
apps/webapp/app/v3/commonWorker.server.tsand related files use these environment variables, including appropriate fallbacks and TLS options.Please ensure that the production environment provides the proper values, but from the code and configuration standpoint, everything is in order.
apps/webapp/app/v3/legacyRunEngineWorker.server.ts (1)
10-18: Ensure secure Redis connection configuration.The Redis configuration looks good with TLS support, but consider the following security aspects:
- TLS is enabled by default (disabled only when explicitly set)
- Connection details are properly sourced from environment variables
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for secure Redis configuration rg 'REDIS_TLS_DISABLED.*true'Length of output: 2025
Secure Redis configuration verified in legacyRunEngineWorker.server.ts.
The TLS setup is handled properly by enabling TLS by default unless the worker-specific environment variable explicitly disables it. This configuration matches our security expectations.
apps/webapp/app/v3/services/finalizeDeployment.server.ts (1)
127-127: LGTM! Simplified alert enqueuing.The removal of the Prisma transaction parameter aligns with the migration to Redis worker for alerts.
apps/webapp/app/v3/taskRunHeartbeatFailed.server.ts (1)
12-12: LGTM! Class name better reflects its purpose.The rename from
RequeueTaskRunServicetoTaskRunHeartbeatFailedServiceimproves code clarity.apps/webapp/app/v3/services/createDeployedBackgroundWorker.server.ts (1)
151-151: LGTM! Consistent with alert processing changes.The removal of the Prisma transaction parameter aligns with the migration to Redis worker for alerts.
references/v3-catalog/src/trigger/simple.ts (1)
132-134: LGTM! Explicit retry configuration.Adding explicit retry configuration with
maxAttempts: 1improves error handling clarity.internal-packages/redis-worker/src/worker.test.ts (2)
37-37: LGTM! Streamlined worker initialization.The change improves code readability by chaining the
.start()call directly to worker initialization.
51-51: Verify if the increased timeout is sufficient.The timeout has been increased from 600ms to 2000ms. While this should reduce flaky tests, please ensure this value is sufficient across different environments and load conditions.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any test retries or timeouts in recent test runs git log -n 10 --pretty=format:"%H" | while read -r commit; do echo "Checking test results for commit: $commit" git show $commit:internal-packages/redis-worker/src/worker.test.ts 2>/dev/null | grep -A 5 -B 5 "timeout" doneLength of output: 1546
The 2‑second delay appears sufficient given the current test configuration.
- The tests in internal-packages/redis-worker/src/worker.test.ts use a generous per‑test timeout (30,000 ms), so a 2‑second wait is well within the safe range.
- No evidence was found (or reported manually) of test flakiness or failures due to the new delay value.
apps/webapp/app/v3/services/finalizeTaskRun.server.ts (1)
132-132: LGTM! Simplified alert enqueuing.The removal of the Prisma instance aligns with the migration to Redis-based workers for alerts processing.
apps/webapp/app/env.server.ts (4)
378-408: LGTM! Consistent Redis configuration pattern.The Redis configuration for the legacy run engine worker follows the established pattern of falling back to global Redis settings.
410-414: Verify the default concurrency settings for common worker.The default values are:
- 2 workers
- 10 tasks per worker (higher than legacy worker)
- 1000ms poll interval
- 50ms immediate poll interval
- 100 concurrent tasks limit
The higher tasks per worker setting (10 vs 1) suggests different performance characteristics. Please validate these settings under load.
Consider documenting the reasoning behind the different tasks-per-worker settings between legacy and common workers.
416-444: LGTM! Consistent Redis configuration pattern.The Redis configuration for the common worker follows the same pattern as other workers, ensuring consistency across the codebase.
372-376: Verify the default concurrency settings for legacy run engine worker.The default values are:
- 2 workers
- 1 task per worker
- 1000ms poll interval
- 50ms immediate poll interval
- 100 concurrent tasks limit
Please ensure these values are optimal for your production workload.
apps/webapp/app/services/worker.server.ts (1)
647-647: LGTM! Task handler moved to Redis worker.The empty handler with the comment clearly indicates that this functionality is now handled by the Redis worker, which aligns with the PR's objective of using Redis workers for run management.
apps/webapp/app/v3/services/alerts/deliverAlert.server.ts (2)
31-31: LGTM: Import of commonWorker.The import of
commonWorkeraligns with the broader shift towards using Redis workers for job processing.
1096-1103: LGTM: Simplified enqueue method.The refactored
enqueuemethod:
- Removes the transaction parameter, simplifying the interface
- Uses a consistent ID format with
alert:${alertId}- Properly handles optional
runAtparameterapps/webapp/app/v3/marqs/index.server.ts (1)
33-33: LGTM: Updated visibility timeout strategy.The change to use
V3LegacyRunEngineWorkerVisibilityTimeoutaligns with the broader shift towards Redis workers for job processing and heartbeat management.Also applies to: 1614-1614
internal-packages/redis-worker/package.json (1)
14-14: LGTM: Added p-limit dependency.The addition of
p-limitis appropriate for managing concurrency in Redis worker job processing.internal-packages/redis-worker/tsconfig.json (1)
4-4: LGTM: Added DOM.AsyncIterable support.The addition of
DOM.AsyncIterableto the TypeScript compiler options enables proper typing for async iteration in DOM contexts.apps/webapp/tsconfig.json (1)
36-39: Validate New Path Mappings for Worker Modules.
The new entries for"@internal/redis-worker"and its submodules have been added alongside the existing"@internal/zod-worker"mappings. Please verify that the relative paths ("../../internal-packages/redis-worker/src/index"and"../../internal-packages/redis-worker/src/*") correctly reflect the project’s folder structure and that TypeScript can resolve the modules as expected.apps/webapp/package.json (1)
53-55: Confirm Consistency of Redis Worker Dependency.
The dependency"@internal/redis-worker": "workspace:*"has been added, which aligns with the updated path mappings intsconfig.json. Ensure that the workspace version being used is up to date and that its API surface is fully compatible with the consumers in the web application.
| runHeartbeat: async ({ payload }) => { | ||
| const service = new TaskRunHeartbeatFailedService(); | ||
|
|
||
| await service.call(payload.runId); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for task failures.
The job handler should include proper error handling and logging.
runHeartbeat: async ({ payload }) => {
+ try {
const service = new TaskRunHeartbeatFailedService();
await service.call(payload.runId);
+ } catch (error) {
+ logger.error("Failed to process run heartbeat", {
+ error,
+ runId: payload.runId,
+ });
+ throw error; // Re-throw to trigger retry
+ }
},📝 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.
| runHeartbeat: async ({ payload }) => { | |
| const service = new TaskRunHeartbeatFailedService(); | |
| await service.call(payload.runId); | |
| }, | |
| runHeartbeat: async ({ payload }) => { | |
| try { | |
| const service = new TaskRunHeartbeatFailedService(); | |
| await service.call(payload.runId); | |
| } catch (error) { | |
| logger.error("Failed to process run heartbeat", { | |
| error, | |
| runId: payload.runId, | |
| }); | |
| throw error; // Re-throw to trigger retry | |
| } | |
| }, |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal-packages/redis-worker/vitest.config.ts (1)
7-12: LGTM! Sequential test execution is appropriate for Redis operations.The changes to disable parallel execution and enforce single-threaded testing are well-suited for Redis-based functionality:
- Prevents potential race conditions in Redis operations
- Ensures consistent test execution order
- Helps maintain data consistency during tests
Consider documenting these configuration choices in the README to help other developers understand why parallel execution is disabled for Redis worker tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal-packages/redis-worker/vitest.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
Summary by CodeRabbit
New Features
Refactor
Chores