Publish redis-worker and add graceful shutdown manager #1810
Conversation
|
WalkthroughThis pull request implements extensive configuration and dependency updates along with several code and schema refinements. It replaces internal redis-worker references with a public package, adds new environment variables for Kubernetes and worker shutdown timeouts, and improves error handling, metrics collection, header extraction, checkpoint API schemas, and shutdown management. Additional adjustments include enhanced test guidelines, updated build configurations for a new redis-worker package, and script improvements for robust cleanup during pre-release publishing. Changes
Sequence Diagram(s)sequenceDiagram
participant S as ShutdownManager
participant H as Registered Handlers
participant P as Process
Note over S, P: Shutdown initiated (SIGTERM/SIGINT)
S->>H: Trigger all registered shutdown handlers concurrently
H-->>S: Return handler statuses
S->>P: Exit process with appropriate signal code
sequenceDiagram
participant C as Client Request
participant WS as WorkloadServer
participant CP as CheckpointClient
Note over C,WS: Handling suspend run request with extended headers
C->>WS: Send request with headers (runnerId, deploymentVersion, projectRef)
WS->>WS: Extract header values using new helper methods
WS->>CP: Call restoreRun with checkpoint body (includes orchestrator info)
CP-->>WS: Return response
WS-->>C: Respond with suspension status
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (6)
packages/core/src/v3/serverOnly/shutdownManager.test.ts (1)
4-9: Question usage of private members in tests
It's a standard approach to use type assertions for accessing private members in a white-box test scenario. However, consider whether you can test the same functionality through the public interface alone to maintain better encapsulation.packages/redis-worker/package.json (2)
14-17: Module type consistency.
You are shipping both CJS and ESM builds while specifying"type": "module". This can cause confusion in some build tools or environments. Consider removing"type": "module"or using separate packages for each module type if you encounter unexpected import behavior.
33-42: Dev dependencies referencing internal packages.
Including@internal/*packages indevDependenciesis usually fine since consumers won't install these, but confirm that any tooling (e.g., test containers, tracing) doesn’t block usage for external contributors.packages/core/src/v3/serverOnly/shutdownManager.ts (2)
4-7: Limited signal handling.
Restricting shutdown signals to “SIGTERM” and “SIGINT” prevents unexpected issues. If your environment sends other signals (like “SIGHUP”), consider extending support to ensure all relevant signals trigger graceful shutdowns.
39-80: Graceful shutdown strategy.
Running all handlers concurrently withPromise.allSettledis a solid approach to ensure no single slow handler prevents others from completing. Logging all failures is helpful for debugging, but consider whether partial failures should block the process from exiting.apps/supervisor/src/workloadServer/index.ts (1)
97-98: Header retrieval logic is fine.
Note that Node.js lowercases header keys, so ensureWORKLOAD_HEADERSkeys match that case. Otherwise, considertoLowerCase().
📜 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 (51)
.changeset/config.json(1 hunks).cursor/rules/webapp.mdc(1 hunks)ai/references/repo.md(1 hunks)ai/references/tests.md(1 hunks)apps/supervisor/package.json(1 hunks)apps/supervisor/src/env.ts(1 hunks)apps/supervisor/src/index.ts(5 hunks)apps/supervisor/src/workloadManager/kubernetes.ts(2 hunks)apps/supervisor/src/workloadServer/index.ts(4 hunks)apps/webapp/app/env.server.ts(3 hunks)apps/webapp/app/v3/commonWorker.server.ts(2 hunks)apps/webapp/app/v3/legacyRunEngineWorker.server.ts(2 hunks)apps/webapp/app/v3/runEngine.server.ts(1 hunks)apps/webapp/package.json(1 hunks)internal-packages/redis-worker/package.json(0 hunks)internal-packages/redis-worker/tsconfig.json(0 hunks)internal-packages/redis-worker/tsconfig.src.json(0 hunks)internal-packages/redis-worker/tsconfig.test.json(0 hunks)internal-packages/redis/src/index.ts(1 hunks)internal-packages/run-engine/package.json(2 hunks)internal-packages/run-engine/src/engine/index.ts(2 hunks)internal-packages/run-engine/src/engine/types.ts(2 hunks)internal-packages/tracing/src/index.ts(1 hunks)package.json(1 hunks)packages/cli-v3/src/entryPoints/managed-run-controller.ts(1 hunks)packages/core/package.json(1 hunks)packages/core/src/utils.ts(1 hunks)packages/core/src/v3/isomorphic/friendlyId.ts(1 hunks)packages/core/src/v3/runEngineWorker/consts.ts(1 hunks)packages/core/src/v3/runEngineWorker/supervisor/http.ts(2 hunks)packages/core/src/v3/runEngineWorker/supervisor/schemas.ts(1 hunks)packages/core/src/v3/runEngineWorker/workload/types.ts(1 hunks)packages/core/src/v3/runEngineWorker/workload/util.ts(1 hunks)packages/core/src/v3/schemas/checkpoints.ts(3 hunks)packages/core/src/v3/schemas/runEngine.ts(1 hunks)packages/core/src/v3/serverOnly/checkpointClient.ts(3 hunks)packages/core/src/v3/serverOnly/httpServer.ts(4 hunks)packages/core/src/v3/serverOnly/index.ts(1 hunks)packages/core/src/v3/serverOnly/k8s.ts(1 hunks)packages/core/src/v3/serverOnly/shutdownManager.test.ts(1 hunks)packages/core/src/v3/serverOnly/shutdownManager.ts(1 hunks)packages/core/src/v3/serverOnly/singleton.ts(1 hunks)packages/redis-worker/package.json(1 hunks)packages/redis-worker/src/queue.test.ts(5 hunks)packages/redis-worker/src/worker.ts(13 hunks)packages/redis-worker/tsconfig.json(1 hunks)packages/redis-worker/tsconfig.src.json(1 hunks)packages/redis-worker/tsconfig.test.json(1 hunks)packages/redis-worker/tsup.config.ts(1 hunks)patches/supports-hyperlinks@2.3.0.patch(1 hunks)scripts/publish-prerelease.sh(3 hunks)
💤 Files with no reviewable changes (4)
- internal-packages/redis-worker/tsconfig.src.json
- internal-packages/redis-worker/tsconfig.json
- internal-packages/redis-worker/tsconfig.test.json
- internal-packages/redis-worker/package.json
🧰 Additional context used
🧬 Code Definitions (12)
apps/webapp/app/v3/runEngine.server.ts (1)
apps/webapp/app/env.server.ts (1) (1)
env(678-678)
packages/core/src/v3/serverOnly/k8s.ts (1)
apps/supervisor/src/env.ts (1) (1)
env(56-56)
packages/core/src/v3/runEngineWorker/workload/util.ts (1)
packages/core/src/v3/runEngineWorker/consts.ts (1) (1)
WORKLOAD_HEADERS(8-13)
apps/webapp/app/v3/legacyRunEngineWorker.server.ts (1)
apps/webapp/app/env.server.ts (1) (1)
env(678-678)
apps/webapp/app/v3/commonWorker.server.ts (1)
apps/webapp/app/env.server.ts (1) (1)
env(678-678)
packages/core/src/v3/serverOnly/shutdownManager.test.ts (1)
packages/core/src/v3/serverOnly/shutdownManager.ts (1) (1)
shutdownManager(89-89)
apps/supervisor/src/workloadManager/kubernetes.ts (1)
apps/supervisor/src/env.ts (1) (1)
env(56-56)
packages/core/src/v3/runEngineWorker/supervisor/http.ts (2)
packages/core/src/v3/runEngineWorker/supervisor/schemas.ts (4) (4)
WorkerApiSuspendRunRequestBody(31-40)WorkerApiSuspendRunRequestBody(41-41)WorkerApiSuspendRunResponseBody(43-45)WorkerApiSuspendRunResponseBody(46-46)packages/core/src/v3/apiClient/core.ts (1) (1)
wrapZodFetch(706-746)
packages/core/src/v3/schemas/checkpoints.ts (1)
packages/core/src/v3/schemas/runEngine.ts (2) (2)
DequeuedMessage(225-260)DequeuedMessage(261-261)
packages/core/src/v3/serverOnly/shutdownManager.ts (1)
packages/core/src/v3/serverOnly/singleton.ts (1) (1)
singleton(1-8)
apps/supervisor/src/index.ts (2)
packages/core/src/v3/serverOnly/k8s.ts (1) (1)
isKubernetesEnvironment(3-16)apps/supervisor/src/env.ts (1) (1)
env(56-56)
packages/cli-v3/src/entryPoints/managed-run-controller.ts (1)
apps/supervisor/src/env.ts (1) (1)
env(56-56)
🪛 Biome (1.9.4)
packages/core/src/v3/serverOnly/httpServer.ts
[error] 210-210: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (130)
packages/core/src/v3/isomorphic/friendlyId.ts (1)
24-26: Optimization to prevent duplicate ID prefixingThis is a good addition that prevents creating redundant prefixes when an ID already has the correct entityName prefix format. This change ensures that if an ID is processed multiple times through
toFriendlyId, you won't end up with malformed IDs likeentityName_entityName_id.internal-packages/tracing/src/index.ts (1)
1-2: TypeScript type imports properly markedGood improvement to explicitly mark imports that are only used as types with the
typekeyword. This follows TypeScript best practices and can help with bundle optimization since type imports are erased during compilation..changeset/config.json (1)
15-15: Package ignore list updated for versioningThe change replaces "proxy" with "supervisor" in the ignore list, which aligns with the PR's goal of restructuring the worker management system. This will affect how these packages are versioned and included in changelogs when using Changesets.
packages/redis-worker/tsconfig.json (1)
1-11: New TypeScript configuration for redis-worker packageThis configuration follows the project's established patterns for TypeScript packages by extending a base configuration and referencing separate configs for source and test code. This is consistent with the PR's goal of publishing the redis-worker as a public package.
package.json (1)
77-78:✅ Verification successful
Added new patched dependency for supports-hyperlinks.
This change adds a patched version of
supports-hyperlinks@2.3.0to the patchedDependencies section. This appears to be part of the transition from using the internal Redis worker to a public@trigger.dev/redis-workerpackage.
🏁 Script executed:
#!/bin/bash # Verify that the patch file exists if [ -f "patches/supports-hyperlinks@2.3.0.patch" ]; then echo "✅ Patch file exists" else echo "❌ Patch file does not exist" fiLength of output: 101
Verified Patched Dependency: Supports-Hyperlinks@2.3.0
The patch file atpatches/supports-hyperlinks@2.3.0.patchhas been confirmed to exist, indicating that the new patched dependency added in thepackage.json(lines 77-78) is correctly set up. This change supports the transition from the internal Redis worker to the public@trigger.dev/redis-workerpackage.
- File:
package.json(Lines 77-78)- Patch Confirmed:
patches/supports-hyperlinks@2.3.0.patchexistsApproved code changes.
packages/core/package.json (2)
192-192: Added Prometheus client for metrics collection.Adding
prom-clientis a good addition to support the metrics collection features mentioned in the PR objectives for both the Redis worker and HTTP server.
195-195: Added std-env for environment variable management.The
std-envpackage provides a standardized way to handle environment variables across different environments, which will help with the graceful shutdown management and environment-specific configurations.packages/redis-worker/tsconfig.test.json (1)
1-11: New TypeScript test configuration for redis-worker package.This configuration correctly sets up the TypeScript environment for testing the redis-worker package. It properly extends the base configuration, includes test files, references the source configuration, and sets appropriate compiler options for testing with Vitest.
This is part of the broader effort to publish the redis-worker as a public package as mentioned in the PR objectives. Having a proper test configuration ensures the package is well-tested before publication.
apps/webapp/app/v3/runEngine.server.ts (1)
20-20: Added shutdown timeout for graceful worker termination.This change adds the
shutdownTimeoutMsconfiguration parameter to the RunEngine initialization, using the environment variableRUN_ENGINE_WORKER_SHUTDOWN_TIMEOUT_MS. This directly supports the PR objective of implementing a graceful shutdown manager to ensure the process exits properly after all shutdown handlers have completed.The parameter is correctly integrated with the existing configuration pattern, maintaining consistency with the codebase's approach to configuration management.
.cursor/rules/webapp.mdc (1)
12-12: Package reference updated correctlyThe reference to the Redis worker has been updated from
@internal/redis-workerto@trigger.dev/redis-worker, which aligns with the PR objective of publishing the redis-worker as a public package.patches/supports-hyperlinks@2.3.0.patch (1)
9-13: Good fix for Cursor terminal hyperlinksThis patch correctly adds support for detecting when the application is running in a Cursor terminal environment by checking for the
CURSOR_TRACE_IDenvironment variable. This addresses the PR objective of "Fixes to Cursor terminal links."ai/references/tests.md (1)
28-32: Clear guidance on test file placementThis addition provides clear guidance on where to place test files, following the common industry practice of co-locating tests with their implementation files. This makes it easier for developers to find and maintain tests.
apps/supervisor/package.json (1)
9-9: Improved dev script with helpful error handlingThe updated dev script now includes error handling that reminds users to run
nvm useif the command fails. This is a good developer experience improvement that will help prevent confusion when the Node.js version isn't set correctly.packages/core/src/v3/serverOnly/index.ts (1)
4-6: Good addition of new exportsThese exports make modules from "singleton.js", "shutdownManager.js", and "k8s.js" available to importers, which aligns with the PR objective of implementing a ShutdownManager to handle graceful shutdowns across components.
packages/core/src/v3/runEngineWorker/workload/types.ts (1)
4-6: Expanded type definition with deployment tracking propertiesThe addition of
deploymentId,deploymentVersion, andprojectRefproperties enhances theWorkloadClientCommonOptionstype with better deployment tracking capabilities.apps/webapp/package.json (1)
56-56: Package dependency updated from internal to publicReplacing
@internal/redis-workerwith@trigger.dev/redis-workeraligns with the PR objective of publishing the redis-worker as a public package.packages/core/src/v3/serverOnly/k8s.ts (1)
1-16: Well-implemented Kubernetes environment detectionThe
isKubernetesEnvironmentfunction provides a clean way to detect if the application is running in a Kubernetes environment by checking common environment variables. The optionaloverrideparameter adds flexibility for testing or forced configurations.The implementation uses the efficient
.some(Boolean)pattern to check if any of the environment variables are present.internal-packages/redis/src/index.ts (1)
1-1: Clean TypeScript import enhancement.The import statement now explicitly identifies
RedisOptionsas a type with thetypekeyword, which improves code clarity and can help with tree-shaking.packages/core/src/v3/serverOnly/singleton.ts (1)
1-8: Well-implemented singleton pattern.This utility function provides a clean way to manage singleton instances across the application. It's type-safe and properly initializes the global storage object.
A few observations:
- Uses
globalThisto store singletons, which works across different JavaScript environments- Properly type-parameterized to ensure type safety
- Initializes the singleton container only once
- Returns the cached instance when available, preventing duplicate initialization
packages/redis-worker/tsconfig.src.json (1)
1-10: Good TypeScript configuration for a publishable package.This configuration is appropriate for a package that's being prepared for publication, with proper source mapping and composite project support.
The configuration:
- Extends the base configuration
- Properly includes source files
- Enables source maps for debugging
- Sets up composite project support for incremental builds
- Includes custom conditions for module resolution
internal-packages/run-engine/src/engine/index.ts (2)
2-2: Package import updated to use published redis-worker.The import has been changed from the internal package to the publicly published package, aligning with the PR objective of publishing the redis-worker.
123-123: Added shutdown timeout configuration.The
shutdownTimeoutMsproperty has been added to the Worker configuration, supporting the new graceful shutdown manager feature. This allows configuring how long the worker should wait for in-progress jobs to complete before stopping.This change aligns with the PR objective of implementing a ShutdownManager to ensure proper process termination after all shutdown handlers complete.
internal-packages/run-engine/src/engine/types.ts (2)
2-2: Change from internal to published packageThe import for Worker and WorkerConcurrencyOptions now comes from the published
@trigger.dev/redis-workerpackage instead of the internal one, which aligns with the PR objective of publishing the redis-worker.
16-16: Added shutdown timeout configurationThe new
shutdownTimeoutMsproperty provides control over worker shutdown behavior, supporting the graceful shutdown manager implementation mentioned in the PR objectives.packages/core/src/v3/runEngineWorker/consts.ts (1)
11-12: Enhanced workload context headersThe addition of
DEPLOYMENT_VERSIONandPROJECT_REFheaders will improve workload tracking and metrics collection capabilities, allowing for better identification of requests by deployment version and project.packages/core/src/utils.ts (1)
5-12: Useful error handling utilityThe new
tryCatchfunction provides a clean pattern for Promise error handling, eliminating nested try/catch blocks and simplifying error management in async code.Example usage:
const [error, result] = await tryCatch(someAsyncOperation()); if (error) { // Handle error } else { // Use result }packages/cli-v3/src/entryPoints/managed-run-controller.ts (1)
154-155: Add deployment metadata to HTTP clientThese additions enhance the WorkloadHttpClient with deployment version and project reference information, which corresponds to the newly added workload headers in the constants file. This provides important context for metrics collection and request identification.
packages/core/src/v3/runEngineWorker/workload/util.ts (1)
11-12: Headers extended for enriched workload contextThe addition of deployment version and project reference headers enhances the contextual information passed with each workload. This change aligns with the PR objectives of improving metrics collection and is consistent with the header constants defined in
consts.ts.packages/core/src/v3/runEngineWorker/supervisor/schemas.ts (1)
146-152: New schema for suspend completion responsesThis schema provides a standardized structure for suspend completion responses, with success/error status reporting. The schema correctly follows the established pattern of other API schemas in this file and properly exports both the Zod schema and inferred TypeScript type.
internal-packages/run-engine/package.json (1)
18-18: Redis worker dependency updated to public packageThis change aligns with the PR objective of publishing the redis-worker by replacing the internal package reference with the public one. This promotes better code reuse and standardization across the project.
apps/webapp/app/v3/commonWorker.server.ts (2)
1-1: Import updated to use published redis-worker packageUpdated import path to use the newly published
@trigger.dev/redis-workerpackage instead of the internal one, consistent with the PR's objective of standardizing on the public package.
83-83: Graceful shutdown timeout added to worker configurationAdded
shutdownTimeoutMsconfiguration which implements the PR objective of improving graceful shutdown management. This ensures the worker will properly complete its tasks before shutting down, with the timeout value controlled via environment variables.apps/webapp/app/v3/legacyRunEngineWorker.server.ts (2)
1-1: Import updated to use published packageThe import statement has been updated to use the published package
@trigger.dev/redis-workerinstead of the internal package. This change aligns with the PR objective of publishing the redis-worker package externally.
70-70:✅ Verification successful
Added shutdown timeout configuration
Adding the
shutdownTimeoutMsproperty improves graceful shutdown handling by allowing configuration of how long the worker should wait before forcefully terminating. This is part of the new ShutdownManager implementation mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash # Check for the environment variable definition rg "LEGACY_RUN_ENGINE_WORKER_SHUTDOWN_TIMEOUT_MS" --type tsLength of output: 302
Approved: Shutdown Timeout Configuration Validated
The shutdown timeout property is correctly wired. The environment variable
LEGACY_RUN_ENGINE_WORKER_SHUTDOWN_TIMEOUT_MSis set with a default value inapps/webapp/app/env.server.ts(default: 60_000) and properly referenced inapps/webapp/app/v3/legacyRunEngineWorker.server.ts. This confirms that the graceful shutdown handling via the new ShutdownManager implementation meets the PR objectives.
- apps/webapp/app/v3/legacyRunEngineWorker.server.ts: Uses
shutdownTimeoutMs: env.LEGACY_RUN_ENGINE_WORKER_SHUTDOWN_TIMEOUT_MS- apps/webapp/app/env.server.ts: Defines
LEGACY_RUN_ENGINE_WORKER_SHUTDOWN_TIMEOUT_MSwith an appropriate defaultscripts/publish-prerelease.sh (5)
38-47: Enhanced error handling with cleanup functionsThe addition of
git_reset()andabort()functions improves code maintainability by centralizing the cleanup logic. This ensures consistent cleanup when the script is interrupted or aborted, preventing Git repository states from being left in an inconsistent state.
48-48: Added trap for interrupt signalsThe trap command ensures that the abort function is called when the script receives an interrupt signal (CTRL+C), providing proper cleanup even when the script execution is forcibly terminated.
62-62: Added pause for manual changesThis pause gives the user an opportunity to make manual adjustments if needed before continuing with the build and publish process, adding flexibility to the workflow.
75-75: Using abort function for consistencyReplaced direct call to
git reset --hard HEADwith theabort()function, which ensures consistent cleanup behavior throughout the script.
80-80: Using git_reset function for consistencyReplaced direct call to
git reset --hard HEADwith thegit_reset()function, maintaining consistency in how Git reset operations are performed throughout the script.apps/supervisor/src/env.ts (1)
50-53:✅ Verification successful
Added Kubernetes configuration environment variables
These new environment variables enhance Kubernetes configuration options:
KUBERNETES_FORCE_ENABLED: Allows forcing Kubernetes modeKUBERNETES_NAMESPACE: Configures the Kubernetes namespace (default: "default")EPHEMERAL_STORAGE_SIZE_LIMITandEPHEMERAL_STORAGE_SIZE_REQUEST: Define storage resourcesThese additions provide better flexibility and control for Kubernetes deployments, with sensible defaults.
🏁 Script executed:
#!/bin/bash # Check where these environment variables are used in the codebase rg "env\.KUBERNETES_NAMESPACE|env\.EPHEMERAL_STORAGE_SIZE" --type tsLength of output: 506
Attention: Kubernetes Environment Variables Added and Verified
The addition in
apps/supervisor/src/env.ts(lines 50-53) correctly introduces the following environment variables using sensible defaults:
- KUBERNETES_FORCE_ENABLED: Enables forced Kubernetes mode.
- KUBERNETES_NAMESPACE: Sets the Kubernetes namespace (default is
"default").- EPHEMERAL_STORAGE_SIZE_LIMIT & EPHEMERAL_STORAGE_SIZE_REQUEST: Define storage resource limits and requests.
Verification using
rgconfirms these variables are referenced appropriately in the codebase:
apps/kubernetes-provider/src/index.ts: Utilizesprocess.env.KUBERNETES_NAMESPACE.apps/supervisor/src/workloadManager/kubernetes.ts: References bothEPHEMERAL_STORAGE_SIZE_LIMITandEPHEMERAL_STORAGE_SIZE_REQUESTalong with the namespace.No issues were found with their usage, and the integration provides enhanced control over Kubernetes deployments.
packages/redis-worker/tsup.config.ts (1)
1-28: Well-structured tsup configuration for redis-worker packageThis configuration:
- Builds from the
src/index.tsentry point- Outputs both CommonJS and ESM formats for maximum compatibility
- Generates TypeScript declaration files
- Enables source maps for better debugging
- Applies tree-shaking to reduce bundle size
- Ensures internal packages and ESM-only packages are always bundled
- Adds a banner for ESM format to support CommonJS require calls
The configuration follows best practices for JavaScript/TypeScript package publishing, making the package widely compatible across different environments.
ai/references/repo.md (1)
17-17: Documentation updated to reflect the new redis-worker packageThe addition of the
@trigger.dev/redis-workerpackage documentation aligns with the PR objective of publishing the redis-worker. This change correctly documents the new public package that replaces the internal implementation.packages/redis-worker/src/queue.test.ts (5)
33-36: Improved error handling in testsAdding null checks for dequeued items ensures that the test fails explicitly when an item isn't dequeued properly, rather than causing potential downstream errors that would be harder to debug.
55-58: Consistent error handling patternThis null check follows the same pattern established earlier, ensuring consistent error handling throughout the test file.
266-268: Robust error checking for multiple dequeued itemsThis check ensures both items in the array exist before proceeding with acknowledgment operations, preventing potential runtime errors.
287-289: Consistent error handling for last itemFollowing the established pattern, this check ensures the last dequeued item exists before acknowledging it.
337-339: Error handling for redriven itemsThis check verifies that items redriven from the Dead Letter Queue are properly dequeued, maintaining the error handling consistency throughout the tests.
packages/core/src/v3/schemas/runEngine.ts (2)
215-221: New checkpoint schema with improved type safetyThe new
DequeueMessageCheckpointschema provides better type safety by using theCheckpointTypeenum instead of a string for thetypeproperty, which helps prevent potential type errors.
224-260: Updated DequeuedMessage schema with optional checkpointThe updated
DequeuedMessageschema now references the newDequeueMessageCheckpointtype for the checkpoint field and marks it as optional. This change aligns with the PR's objective of updating checkpoint functionality.apps/webapp/app/env.server.ts (3)
427-427: Added configuration for Run Engine worker shutdown timeoutThis new environment variable supports the ShutdownManager implementation by providing a configurable timeout for graceful shutdowns of the Run Engine worker processes.
595-595: Added configuration for Legacy Run Engine worker shutdown timeoutThis environment variable ensures consistent shutdown behavior for legacy run engine workers, maintaining compatibility while implementing the new ShutdownManager.
638-638: Added configuration for Common worker shutdown timeoutThis environment variable completes the set of configurations needed for the ShutdownManager to handle graceful shutdowns across all worker types, ensuring that processes exit as expected after shutdown handlers complete.
packages/core/src/v3/serverOnly/shutdownManager.test.ts (11)
1-2: Good import statements
No issues found, the imports from Vitest and the local shutdownManager are used properly.
11-20: Proper test setup
Disabling concurrency ensures that global mocking of process.exit doesn't cause concurrency conflicts. Good approach to reset the manager's state in beforeEach.
22-30: Comprehensive test coverage for registration
The test ensures that the handler is correctly stored. Great job verifying both the handler and the associated signals.
32-40: Duplicate registration validation
Throwing an error for duplicate handler names is an excellent safeguard. The test fully confirms the behavior.
41-47: Proper custom signal registration
Verifies that the specified signal is honored. This helps ensure that only the intended signals trigger the handler.
49-61: Correct multi-handler invocation
Ensures that all relevant handlers are invoked and that the process exit code is correct for SIGTERM. Excellent coverage.
63-75: Specific signal filtering
Correctly ensures that only those handlers registered for SIGTERM are invoked. This is an important separation of concerns.
77-89: Error handling
Good approach verifying that a single failing handler does not block the overall shutdown sequence.
91-100: Idempotent shutdown
Nicely ensures that repeated calls to shutdown do not run the sequence multiple times, preventing duplicates or race conditions.
102-115: Correct exit code assertion
Verifies the correct numeric offset for SIGINT and SIGTERM. This ensures consistent behavior across signals.
117-175: Ensuring asynchronous handlers complete
This test verifies that all asynchronous handlers are awaited and the process only exits afterward. Strong coverage of concurrency aspects.packages/core/src/v3/runEngineWorker/supervisor/http.ts (2)
18-19: New imports for run suspension
Importing WorkerApiSuspendRunRequestBody and WorkerApiSuspendRunResponseBody sets the foundation for the new suspend completion API calls.
225-248: submitSuspendCompletion method
This addition is consistent with other fetch-based methods, leveraging wrapZodFetch for typed responses. It nicely handles both the request body and optional runnerId.packages/core/src/v3/schemas/checkpoints.ts (3)
1-1: Import from runEngine
Includes DequeuedMessage for usage in the new CheckpointServiceRestoreRequestBody definition.
11-15: New properties in CheckpointServiceSuspendRequestBody
The addition of runId, snapshotId, runnerId, projectRef, and deploymentVersion clarifies the context of the suspended checkpoint, aligning with the new design requirements.
34-34: Refined restore request body
Replacing the prior discriminated union with DequeuedMessage ensures the required checkpoint field is included. It's more direct and simplifies the schema.packages/redis-worker/package.json (4)
1-5: Confirm version and package identification.
The package name, scope, and version look accurate for a publicly published package. However, please verify that3.3.17aligns with your release strategy to avoid confusion in the monorepo.
6-8: Publish config check.
Using"access": "public"is correct for a publicly published scope. Ensure you have the correct npm permissions under the@trigger.devscope before publishing.
25-32: Check workspace dependencies.
Referencing@trigger.dev/corewith"workspace:*"is likely correct within the monorepo, but ensure the workspace resolution is configured and pinned to a consistent version at publish time.
46-55: Exports structure looks good.
Declaring multiple entry points (CommonJS, ESM, and types) inpackage.jsonusing theexportsfield is a solid approach for modern Node.js packaging. This is consistent and should provide a smooth developer experience.apps/supervisor/src/workloadManager/kubernetes.ts (2)
19-19: Confirm fallback for Kubernetes namespace.
Replacing the default"default"namespace withenv.KUBERNETES_NAMESPACEis sensible. However, ensure thatKUBERNETES_NAMESPACEis always set or that you provide a fallback in case it’s undefined.
203-213: Environment-based ephemeral storage settings.
Switching from hardcoded storage values to environment variables increases flexibility. Double-check that your environment variablesEPHEMERAL_STORAGE_SIZE_REQUESTandEPHEMERAL_STORAGE_SIZE_LIMITare always properly set and validated during deployment.packages/core/src/v3/serverOnly/shutdownManager.ts (2)
8-22: Constructor attaches signal listeners.
Automatically binding to “SIGTERM” and “SIGINT” inside the constructor ensures consistent behavior across the app. Confirm that no other conflicting shutdown logic is present in the codebase.
89-89: Singleton export is clean.
Exporting a singleshutdownManagerinstance avoids multiple competing signal handlers throughout the code, which fosters consistency.apps/supervisor/src/index.ts (5)
16-20: Imports look consistent and purposeful.
These imports from@trigger.dev/core/v3/serverOnlycorrectly align the HTTP server and environment detection utilities with the new orchestrator logic.
32-32: Ensure the override parameter is properly typed.
Passingenv.KUBERNETES_FORCE_ENABLEDas theoverrideparameter is correct, but double-check that it’s strictly a boolean to avoid unwanted type coercion.Would you like to validate that
KUBERNETES_FORCE_ENABLEDis indeed a boolean inapps/supervisor/src/env.ts?
101-101: Clear orchestration setting.
Using"KUBERNETES"or"DOCKER"based on environment is straightforward. This seamlessly integrates with the new checkpoint logic.
135-137: Concise destructuring of message data.
Separatingcheckpointfrom the rest of the message clarifies the restore path. No issues noted.
149-152: Correct usage of spread for checkpoint restore body.
Merging...restandcheckpointinto a single object is consistent with the new checkpoint client structure. No concerns.apps/supervisor/src/workloadServer/index.ts (7)
107-109: Runner ID extraction.
This reuses the new helper neatly. Implementation looks consistent.
111-113: Deployment ID accessor.
Using the same approach to fetch the header is clear and maintains uniformity.
115-117: Deployment version accessor.
Again, consistency is maintained with the shared method.
119-121: Project reference accessor.
Simple adaptation of the standard header fetch pattern. No issues.
232-233: Extracting new headers in suspend route.
CapturingdeploymentVersionandprojectRefensures sufficient context for checkpoint requests.
235-235: Validate presence of headers.
Checking all required header values protects against invalid requests. Well done.
263-268: Expanded checkpoint suspend body.
IncludingrunnerId,deploymentVersion, andprojectRefaligns with the new checkpoint schema. Looks correct.packages/core/src/v3/serverOnly/checkpointClient.ts (10)
7-7: Pulling in new types.
ImportingCheckpointTypealigns with the updated orchestrator logic. No issues here.
13-13: Added orchestrator property.
Definingorchestrator: CheckpointTypeclarifies which environment engine is controlling checkpoints.
19-19: Constructor refactor.
Accepting a singleCheckpointClientOptionsparameter is cleaner and more scalable for future config.
24-24:bodyparameter included for clarity.
Collecting request data inbodyfosters consistency with the new checkpoint schema.
28-29: Omitted 'type' for dynamic set.
UsingOmit<CheckpointServiceSuspendRequestBodyInput, "type">is a neat approach to let orchestrator logic fill in.
30-45: Suspend run request.
Forming the URL and mergingtypewithbodyis logically sound. No security or concurrency concerns identified.
51-51: Breakout of extra property references.
Listingbody,nicely for future expansions of function signature.
91-91:bodyshaped for restore.
Matches the new schema for restoring runs from checkpoint.
95-95: Adopt new typed body for restore.
Ensures consistent data for the checkpoint restore process.
97-109: Refined restore request.
Using fetch with the updated URL and JSON body is aligned with the new structure. Checking status code first is good.packages/core/src/v3/serverOnly/httpServer.ts (11)
5-6: Adding Prometheus client utilities.
ImportingRegistry,Histogram, andCounterplus atryCatchhelper sets up improved observability and robust error handling.
53-53: Metrics options introduced.
The optionalmetricsconfig (withregister,expose,collect) is flexible for toggling telemetry.Also applies to: 56-60
64-66: Static metric definitions.
httpRequestDurationandhttpRequestTotalare well-named to track request performance.
67-68: Reference to metrics registry.
StoringmetricsRegisterlocally preserves modular design.
77-80: Consumer-provided metrics config.
CapturingcollectMetricsandexposeMetricsensures the server can behave differently across environments.
81-90: Lazy initialization of metrics.
Only defining histograms/counters once avoids redundant metric definitions. This is a valid approach.
91-101: Defensive approach for reusing counters.
Gracefully skipping re-initialization if the counters exist. This prevents accidental re-registration.
102-117: Optional /metrics route exposure.
Serving metrics via/metricsis standard practice, and conditionally exposing it is wise.
121-121: Capturing request time at the start.
AssigningstartTimefromprocess.hrtime()is standard for measuring request duration.
190-200: Error handling with tryCatch simplification.
Using[error] = await tryCatch(...)clearly segregates exceptions from normal returns, simplifying error checks.Also applies to: 202-202
250-266: Intuitive metrics collection method.
Recording request duration (Histogram) and total requests (Counter) is a reliable approach. The label set is thorough.packages/redis-worker/src/worker.ts (21)
11-11: No issue with this new import.
It correctly references the shutdown manager for coordinated shutdown handling.
12-12: Prometheus client import looks good.
Ensures that we can collect and export various metrics.
49-49: Optional shutdown timeout parameter is a welcome addition.
It makes the worker’s shutdown behavior more flexible for different operational needs.
52-52: Optional metrics definition is clear and unobtrusive.
Allows consumers to opt into Prometheus-based metrics when desired.
72-81: Good approach centralizing metric fields in a private object.
This design keeps metric references organized and simplifies checks.
88-88: Storing shutdown timeout in a private field is fine.
Straightforward usage later in the shutdown implementation.
97-97: Defaulting to 60 seconds is sensible.
This ensures a predictable fallback ifshutdownTimeoutMsis not provided.
118-122: Short-circuiting metric setup if no registry is provided makes sense.
Prevents unnecessary processing when metrics are disabled.
124-131: Well-defined enqueue duration histogram.
Naming, labels, and bucket ranges look appropriate for measuring enqueue operations.
132-138: Dequeue duration histogram is consistently set up.
Using the same bucket strategy promotes uniform metrics tracking.
140-147: Job duration histogram is well-tuned.
Including larger buckets is practical for capturing longer job executions.
149-155: Acknowledgment duration histogram is straightforward and consistent.
No concerns here.
157-163: Redrive duration histogram is similarly structured.
Retains a uniform approach to metric collection.
165-171: Reschedule duration histogram setup is sound.
Mirrors the rest of the histogram definitions effectively.
224-228: Visibility timeout validation is helpful.
Graceful error handling ensures each job type defines a proper timeout.
232-245: Histogram-wrapped enqueue call is well-implemented.
Encapsulating the queue operation inwithHistogramgives clear insights into enqueue performance.
311-318: Robust dequeue measurement.
Wrapping the critical I/O call inwithHistogramhelps diagnose queueing performance.
363-372: Wrapping job execution in the jobDuration histogram is excellent.
Provides precise measurement of each job’s execution time.
462-477: Implementation of thewithHistogramhelper is correct.
Starting the timer before awaiting the promise, then stopping in afinallyblock, ensures accurate timings.
503-506: Redrive histogram usage is consistent.
Captures redrive performance, helping track Dead Letter Queue recovery times.
514-514: Registration of the worker with the shutdown manager is well-placed.
Ensures that the worker is integrated into the global shutdown lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/core/src/v3/serverOnly/httpServer.ts (1)
212-212: Potentially unreachable codeThis default response is unlikely to be reached because:
- Success cases return from the route handler
- Error cases return in the catch block
- The code is outside the try/catch/finally structure
Consider removing this line or adding a comment explaining when it would be reached.
- return reply.empty(501); + // This should never be reached as all request paths should return a response + // If we get here, it means there's a bug in the request handling logic + logger.error("No response was sent, this should never happen"); + return reply.empty(500);packages/core/src/v3/serverOnly/shutdownManager.ts (1)
8-91: Nice implementation of the ShutdownManager class.The class provides a robust solution for graceful shutdowns with good error handling and detailed logging. Using Promise.allSettled ensures all handlers can attempt to run even if some fail.
I have one suggestion - consider adding a timeout option for individual handlers to prevent one slow handler from blocking others:
register( name: string, handler: ShutdownHandler, + options?: { timeoutMs?: number }, signals: ShutdownSignal[] = ["SIGTERM", "SIGINT"] ) { if (this.handlers.has(name)) { throw new Error(`Shutdown handler "${name}" already registered`); } - this.handlers.set(name, { handler, signals }); + this.handlers.set(name, { handler, signals, timeoutMs: options?.timeoutMs }); }Then in the shutdown method, you could implement a timeout for each handler:
// Apply timeout if specified const handlerWithTimeout = async (name: string, handler: ShutdownHandler, timeoutMs?: number) => { if (!timeoutMs) return handler(signal); return Promise.race([ handler(signal), new Promise((_, reject) => setTimeout(() => reject(new Error(`Handler "${name}" timed out after ${timeoutMs}ms`)), timeoutMs) ) ]); };packages/redis-worker/src/worker.ts (2)
232-245: Consider adding error metrics.While you're tracking duration metrics, consider also tracking error counts for each operation type to improve observability of failure rates.
constructor(private options: WorkerOptions<TCatalog>) { // Existing code... if (!this.metrics.register) { return; } // Existing histograms... + this.metrics.errors = new Counter({ + name: "redis_worker_errors_total", + help: "The total number of errors encountered", + labelNames: ["worker_name", "operation", "job_type"], + registers: [this.metrics.register], + }); } // Then in withHistogram: private async withHistogram<T>( histogram: Histogram<string> | undefined, promise: Promise<T>, labels?: Record<string, string | number>, + operationType?: string ): Promise<T> { if (!histogram || !this.metrics.register) { return promise; } const end = histogram.startTimer({ worker_name: this.options.name, ...labels }); try { return await promise; } catch (error) { + if (this.metrics.errors && operationType) { + this.metrics.errors.inc({ + worker_name: this.options.name, + operation: operationType, + job_type: labels?.job_type || "unknown" + }); + } + throw error; } finally { end(); } }
527-535: Consider adding a force termination mechanism after timeout.Currently, if the shutdown timeout is reached, an error is logged but worker loops might continue running. Consider adding a mechanism to force termination of lingering worker loops after the timeout.
await Promise.race([ Promise.all(this.workerLoops), Worker.delay(this.shutdownTimeoutMs).then(() => { this.logger.error("Worker shutdown timed out", { signal, shutdownTimeoutMs: this.shutdownTimeoutMs, }); + // Force termination of any remaining work + this.workerLoops.forEach((_, index) => { + this.logger.warn(`Forcefully terminating worker loop ${index}`); + // Set a flag or use another mechanism to force worker loops to exit + this.forceTerminateWorkers = true; + }); }), ]);This would require adding checks in the worker loop to exit immediately if
forceTerminateWorkersis true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/v3/serverOnly/httpServer.ts(4 hunks)packages/core/src/v3/serverOnly/shutdownManager.ts(1 hunks)packages/redis-worker/src/worker.ts(13 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/core/src/v3/serverOnly/shutdownManager.ts (1)
packages/core/src/v3/serverOnly/singleton.ts (1) (1)
singleton(1-8)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (18)
packages/core/src/v3/serverOnly/httpServer.ts (7)
5-6: Good integration of Prometheus metricsAdded necessary imports for metrics collection and the
tryCatchutility for improved error handling.
56-60: Well-designed metrics configuration optionThe metrics configuration object provides good flexibility with three optional flags:
register: For providing the metrics registryexpose: Controls whether to expose the/metricsendpointcollect: Controls whether metrics are collectedThis approach allows fine-grained control over metrics functionality.
64-67: Good use of static properties for metricsUsing static properties for metrics ensures they're shared across all HttpServer instances, which is appropriate for application-wide metrics collection.
77-117: Well-implemented metrics initializationThe metrics initialization logic is robust:
- Only initializes when a registry is provided and collection is enabled
- Prevents re-initialization of metrics
- Properly configures histogram and counter with appropriate labels
- Conditionally exposes the
/metricsendpoint with proper error handling
190-204: Good error handling refactoringThe
tryCatchutility simplifies error handling logic while maintaining proper error reporting.
208-210: Proper metrics collection in finally blockUsing a finally block ensures metrics are collected regardless of request outcome, which is essential for accurate monitoring.
250-267: Well-implemented metrics collection methodThe
collectMetricsmethod:
- Properly handles the case when metrics are disabled
- Correctly calculates request duration using high-resolution timing
- Extracts relevant information from request and response
- Uses consistent labeling for metrics
packages/core/src/v3/serverOnly/shutdownManager.ts (1)
93-93: Good use of the singleton pattern.Using the singleton pattern ensures there's only one instance of the ShutdownManager throughout the application, which is essential for consistent shutdown management.
packages/redis-worker/src/worker.ts (10)
11-12: Importing the new shutdownManager is a good approach.This properly integrates with the new centralized shutdown management system.
49-54: Good addition of the shutdownTimeoutMs option.This allows for configurable shutdown timeouts, which is an important part of graceful shutdown.
72-80: Well-structured metrics object.The metrics object is well-organized with all related metrics grouped together.
97-98: Good default timeout value.Setting a reasonable default of 60 seconds for shutdown timeout is a good practice.
118-171: Comprehensive metrics instrumentation.You've added appropriate metrics for all key operations with good naming conventions and useful label dimensions. The buckets are well-chosen for different operation types.
462-477: Excellent reusable helper method for metrics.The
withHistogrammethod is a clean abstraction that avoids code duplication for metrics tracking across different operations.
514-514: Worker registration with shutdownManager is correct.The worker is now properly registered with a unique name that includes the worker's name, which prevents conflicts in multi-worker scenarios.
517-541: Good implementation of graceful shutdown with timeout.The shutdown method now properly handles signals and includes a timeout mechanism using Promise.race to prevent hanging. The logging is also comprehensive.
544-544: Fixed unregister name mismatch.This fix properly addresses the previous issue where the registration and unregistration used different names.
224-228: Good validation for missing visibility timeout.This check ensures that jobs always have a visibility timeout, preventing potential issues during processing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/redis-worker/src/worker.test.ts (1)
11-13: Use of private API in test cleanupThe
afterEachhook uses type casting to access what appears to be a private method (_reset) on theshutdownManager. While this works for tests, consider whether this API could be formalized:
- If
_resetis meant to be used in tests only, consider exposing a public test-specific method likeresetForTesting()instead of using type casting- If this reset functionality might be needed elsewhere, consider making it part of the public API
Type casting to bypass the type system is a code smell that often indicates API design issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/redis-worker/src/worker.test.ts(1 hunks)
⏰ 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 (2)
packages/redis-worker/src/worker.test.ts (2)
4-4: Import changes related to shutdown managementThe two new imports (
afterEachfrom vitest andshutdownManagerfrom@trigger.dev/core/v3/serverOnly) support the implementation of reset functionality between tests. This aligns with the PR's goal of implementing graceful shutdown management.Also applies to: 8-8
11-13: Important test cleanup for shutdown managementThe addition of this
afterEachhook is critical for test isolation. Without it, the state of theshutdownManagerwould persist between tests, potentially causing unpredictable behavior in the test suite as registered shutdown handlers from one test could affect others.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/v3/serverOnly/shutdownManager.ts (1)
50-93: Consider adding a timeout mechanism for graceful shutdownThe shutdown implementation correctly handles multiple aspects of graceful shutdown including parallel execution of handlers and error handling. However, if a handler takes too long or hangs, it might delay the process exit indefinitely.
Consider adding a timeout mechanism that would force exit after a configurable duration:
async shutdown(signal: ShutdownSignal) { if (!this.isEnabled()) return; if (this.isShuttingDown) return; this.isShuttingDown = true; this.logger.info(`Received ${signal}. Starting graceful shutdown...`); // Get handlers that are registered for this signal const handlersToRun = Array.from(this.handlers.entries()).filter(([_, { signals }]) => signals.includes(signal) ); + // Set a timeout to force exit if handlers take too long + const forceExitTimeout = setTimeout(() => { + this.logger.warn(`Forcing exit after timeout during ${signal} shutdown`); + process.exit(128 + this.signalNumbers[signal]); + }, 30000); // 30 seconds timeout, could be made configurable try { const results = await Promise.allSettled( handlersToRun.map(async ([name, { handler }]) => { try { this.logger.info(`Running shutdown handler: ${name}`); await handler(signal); this.logger.info(`Shutdown handler completed: ${name}`); } catch (error) { this.logger.error(`Shutdown handler failed: ${name}`, { error }); throw error; } }) ); // Log any failures results.forEach((result, index) => { if (result.status === "rejected") { const handlerEntry = handlersToRun[index]; if (handlerEntry) { const [name] = handlerEntry; this.logger.error(`Shutdown handler "${name}" failed:`, { reason: result.reason }); } } }); } catch (error) { this.logger.error("Error during shutdown:", { error }); } finally { + clearTimeout(forceExitTimeout); // Exit with the correct signal number process.exit(128 + this.signalNumbers[signal]); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/v3/serverOnly/shutdownManager.test.ts(1 hunks)packages/core/src/v3/serverOnly/shutdownManager.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/core/src/v3/serverOnly/shutdownManager.test.ts (1)
packages/core/src/v3/serverOnly/shutdownManager.ts (1) (1)
ShutdownManager(9-110)
packages/core/src/v3/serverOnly/shutdownManager.ts (1)
packages/core/src/v3/serverOnly/singleton.ts (1) (1)
singleton(1-8)
⏰ 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: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
packages/core/src/v3/serverOnly/shutdownManager.ts (5)
5-7: Good type safety with restricted signal typesThe intentional limitation to only
SIGTERMandSIGINTsignals is a good practice that prevents unexpected issues with signal handling. This restriction is properly enforced through TypeScript'sExtractutility type.
9-25: Well-structured ShutdownManager initializationThe class has a well-designed constructor that:
- Properly initializes internal state
- Sets up signal handlers conditionally based on test environment
- Provides signal number mapping for correct exit codes
The
disableForTestingparameter makes the class testable without interfering with the actual process signals.
27-38: Clean handler registration with validationThe
registermethod has good validation that prevents duplicate handler registration and clearly defines the default signals. The early return when disabled is an efficient approach.
95-101: Effective environment detection for testingThe
isEnabledmethod correctly determines whether the shutdown manager should be active based on the test environment, which prevents interference with tests while allowing tests to explicitly enable it when needed.
112-112: Well-implemented singleton patternUsing the singleton pattern ensures that only one instance of the ShutdownManager exists across the application, which is crucial for proper signal handling and coordination of shutdown procedures.
packages/core/src/v3/serverOnly/shutdownManager.test.ts (6)
4-11: Well-structured test setup with proper isolationThe test setup properly:
- Disables concurrent execution for signal handling tests
- Mocks
process.exitto prevent actual process termination- Clears all mocks before each test to maintain isolation
This ensures reliable and deterministic test execution without side effects.
12-43: Comprehensive handler registration testsThese tests thoroughly verify the registration functionality:
- Successful registration with default signals
- Error handling for duplicate registrations
- Custom signal registration
The tests properly validate both the presence of handlers and their configured signals.
45-75: Thorough shutdown handler execution testsThese tests effectively verify that:
- All registered handlers for a signal are called with the correct signal parameter
- Only handlers registered for the specific signal are executed
- The process exits with the correct exit code
This ensures the core functionality of the shutdown manager works as expected.
77-91: Good error handling verificationThis test correctly verifies that the shutdown process continues even if some handlers fail, which is crucial for robust graceful shutdown behavior. It confirms both successful and failing handlers are attempted, and the process still exits properly.
93-122: Proper verification of shutdown idempotence and exit codesThese tests effectively verify:
- The shutdown sequence is only executed once even when called multiple times
- The process exits with the correct signal-specific exit code
This ensures reliable behavior when multiple shutdown signals are received.
124-182: Excellent asynchronous handler completion verificationThis test thoroughly verifies that:
- All handlers complete execution before the process exits
- Handlers run in parallel but the exit only happens after all complete
- The sequence of operations is tracked and verified
The test implementation is impressive, with good use of mock implementation overrides and restoration. It's an excellent example of validating asynchronous behavior in a deterministic way.
This PR makes the required changes to publish redis-worker. As it depends on internal packages we're using tsup to bundle them.
Also adds a ShutdownManager to help with challenges around multiple components adding their own graceful shutdown handlers. This ensures amongst other things that the process actually does exit as expect once all handlers have finished executing.
Other important changes:
Summary by CodeRabbit
New Features
@trigger.dev/redis-workerfor Redis worker functionality.HttpServerwith metrics collection capabilities and a new/metricsendpoint.ShutdownManagerfor graceful shutdown handling in response to signals.Bug Fixes
Refactor