feat: service state#667
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces a major refactor of the health check and worker monitoring system. It replaces the gateway's pull-based health monitoring with a push-based system, where workers report their state to the gateway. New scheduling and state management components are added, configuration options are updated, and extensive changes are made across service interfaces, lifecycle management, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant Gateway
participant HealthManager
participant ScheduleManager
loop Periodic State Report
Worker->>Gateway: reportWorker(id, state)
Gateway->>WorkerManager: reportWorker(id, state)
WorkerManager->>Worker: reportState(state)
end
Note over Worker,Gateway: Worker pushes state to Gateway instead of Gateway polling Worker
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 19
🔭 Outside diff range comments (1)
packages/services/test/gateway/fixtures/remotes.fixture.ts (1)
22-25: Theprovide()method is inconsistent with other methodsWhile all other methods in this test fixture have been updated to return resolved promises, the
provide()method still throwsNotImplemented. This inconsistency could cause unexpected test failures when this method is called.Apply this diff to maintain consistency:
- provide(filename: string): Promise<File> - { - throw new NotImplemented(); - } + provide(filename: string): Promise<File> + { + // Return a dummy file for testing + return Promise.resolve(new File(filename, '')); + }Note: You'll need to verify that the
Fileconstructor accepts these parameters or adjust accordingly.
♻️ Duplicate comments (1)
packages/middleware/src/definitions/States.ts (1)
2-8: Duplicate definition – keep in sync with shared source.See comment on
packages/health/src/definitions/States.ts; the same concerns (missing states & duplication) apply here.
🧹 Nitpick comments (15)
packages/health/src/definitions/States.ts (1)
10-12: Minor: inline type alias to drop the temporaryKeyssymbol.You can shrink the code by skipping the intermediate
Keysalias:-type Keys = keyof typeof States; -type State = typeof States[Keys]; +export type State = typeof States[keyof typeof States];This removes an otherwise unused type.
packages/middleware/src/definitions/States.ts (1)
10-12: Apply the streamlined type alias pattern here as well.See earlier nitpick for a concise
Statedeclaration.packages/services/test/repository/LocalRepository.spec.ts (1)
37-38: Tests for the new state behaviour are still TODO – risk of un-covered regressions.The repository gained lifecycle/state logic; without dedicated tests we can’t guarantee correct state transitions or health reporting.
Action items:
- Add positive/negative cases (e.g.
.start()twice should not change state,.stop()before.start()rejects, etc.).- Include state assertions in existing file-provide scenarios if state affects behaviour.
packages/services/test/gateway/LocalGateway.spec.ts (1)
48-49: Gateway state handling lacks test coverage.Similar to the repository suite, the gateway’s new state machine (and worker-reported state handling) isn’t exercised. Missing tests will hide contract breaks between gateway and workers.
Recommend mirroring the critical paths:
•start → addWorker → reportWorker → stophappy-path
• illegal transitions (e.g.addWorkerwhenSTOPPED)
• worker eviction when no reports receivedpackages/services/test/worker/LocalWorker.spec.ts (1)
69-69: Address the missing test coverage for state functionality.The TODO comment indicates that tests for the new state management functionality are missing. Given that the PR introduces significant state management changes to
LocalWorker(includingHealthManager,StateManager, andReportManager), comprehensive test coverage is essential to ensure the reliability of the state reporting system.Would you like me to generate test cases for the state management functionality, including state transitions, health reporting, and integration with the gateway?
packages/services/test/gateway/fixtures/remoteWorkers.fixture.ts (1)
35-45: Consider avoiding implicit state transitions in fixtures.Manually mutating
worker.statechanges internalReportedStateManagercounters without exercising the normalstart()/stop()flow.
For behaviour-driven tests this is fine, but for integration-style tests it may hide lifecycle regressions.migrations/migrate-from-0.9.x-to-0.10.0.md (1)
17-27: Minor wording nit – “After” reads clearer than “Current”.“Before / After” is the conventional pair for migration docs.
-**Current**: +**After**:packages/services/test/worker/fixtures/sourcingManager.fixture.ts (1)
2-4: Resolve the manager path relative to the fixture file to avoid brittle./resolution.Running tests from a different CWD can break this fixture.
-import { LocalSourcingManager } from '@jitar/sourcing'; -export const sourcingManager = new LocalSourcingManager('./'); +import path from 'node:path'; +import { LocalSourcingManager } from '@jitar/sourcing'; + +const root = path.resolve(__dirname, '../../../..'); // adjust as needed +export const sourcingManager = new LocalSourcingManager(root);packages/services/src/worker/RemoteWorkerBuilder.ts (1)
11-16: Consider adding threshold validation.The constructor should validate that thresholds are positive numbers to prevent potential issues with state management.
constructor(remoteBuilder: RemoteBuilder, unavailableThreshold?: number, stoppedThreshold?: number) { this.#remoteBuilder = remoteBuilder; + + if (unavailableThreshold !== undefined && unavailableThreshold <= 0) + { + throw new Error('unavailableThreshold must be a positive number'); + } + + if (stoppedThreshold !== undefined && stoppedThreshold <= 0) + { + throw new Error('stoppedThreshold must be a positive number'); + } + this.#unavailableThreshold = unavailableThreshold; this.#stoppedThreshold = stoppedThreshold; }packages/services/src/common/RequestPool.ts (1)
41-49: Consider adding queue size limits to prevent unbounded growth.The queue can grow without limits, potentially causing memory issues under high load.
Consider adding a maximum queue size parameter and rejecting requests when the limit is reached:
+const DEFAULT_MAX_QUEUE_SIZE = 1000; + export default class RequestPool implements Runner { readonly #runner: Runner; readonly #size: number; + readonly #maxQueueSize: number; readonly #queue: QueueItem[] = []; #started = false; #activeCount = 0; - constructor(runner: Runner, size = DEFAULT_POOL_SIZE) + constructor(runner: Runner, size = DEFAULT_POOL_SIZE, maxQueueSize = DEFAULT_MAX_QUEUE_SIZE) { this.#runner = runner; this.#size = size; + this.#maxQueueSize = maxQueueSize; }Then in the
runmethod, check the queue size before adding.packages/runtime/src/client/Client.ts (1)
49-49: Consider error handling for RequestPool lifecycleThe
start()andstop()methods manage theRequestPoollifecycle, but there's no error handling ifRequestPool.start()orRequestPool.stop()fail.Consider wrapping the RequestPool operations in try-catch blocks or ensuring they're included in the Promise.all error handling:
async start(): Promise<void> { await this.#setUp(); - this.#requestPool.start(); + try { + await this.#requestPool.start(); + } catch (error) { + await this.#tearDown(); + throw error; + } }Also applies to: 54-54
packages/health/src/HealthManager.ts (1)
17-17: Consider thread safety for state transitionsWhile the state management is well structured, there could be race conditions if
start()andstop()are called concurrently. The state transitions are not atomic.Consider adding a mutex or similar synchronization mechanism to ensure state transitions are atomic:
private stateMutex = new Mutex(); // You'd need to implement or import a Mutex async start(): Promise<void> { await this.stateMutex.acquire(); try { if (this.#state !== States.STOPPED) { return; } this.#state = States.STARTING; } finally { this.stateMutex.release(); } await this.#loadHealthChecks(); this.#state = States.STARTED; }Also applies to: 38-38, 52-52
packages/services/src/gateway/WorkerMonitor.ts (1)
39-42: Consider preventing overlapping monitor operationsThe synchronous
#monitormethod spawns async operations via#checkWorker. If the monitoring interval is shorter than the time it takes to check all workers, operations could overlap.Consider adding a flag to prevent overlapping operations:
+private isMonitoring = false; + #monitor(): void { + if (this.isMonitoring) { + return; + } + + this.isMonitoring = true; this.#workerManager.workers.forEach(worker => this.#checkWorker(worker)); + + // Reset flag after all checks complete + Promise.all( + Array.from(this.#workerManager.workers).map(worker => this.#checkWorker(worker)) + ).finally(() => { + this.isMonitoring = false; + }); }packages/services/src/repository/RemoteRepository.ts (1)
72-77: Consider adding state validation in updateState().The method can be called at any time, potentially causing unexpected state transitions (e.g., from "stopping" to "available").
Add state validation to prevent inappropriate transitions:
async updateState(): Promise<State> { + if (this.#stateManager.isNotStarted()) + { + return this.#stateManager.state; + } + const healthy = await this.isHealthy(); return this.#stateManager.setAvailability(healthy); }packages/runtime/src/server/Server.ts (1)
324-334: Consider optimizing state validation for better performance.The method recreates the states array on each call. Consider using a Set constant for faster lookup:
+const VALID_STATES = new Set(Object.values(States)); + #translateState(state: string): State { - const states = Object.values(States) as string[]; - - if (states.includes(state) === false) + if (!VALID_STATES.has(state)) { throw new BadRequest('Invalid state value'); } return state as State; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
documentation/docs/deploy/health-checks.md(1 hunks)documentation/docs/fundamentals/runtime-services.md(4 hunks)migrations/migrate-from-0.9.x-to-0.10.0.md(1 hunks)packages/configuration/src/server/definitions/GatewayConfiguration.ts(1 hunks)packages/configuration/src/server/definitions/RemoteWorkerConfiguration.ts(1 hunks)packages/configuration/src/server/definitions/ServerConfiguration.ts(3 hunks)packages/configuration/src/server/definitions/WorkerConfiguration.ts(2 hunks)packages/configuration/src/server/index.ts(1 hunks)packages/configuration/test/server/fixtures/configuration.fixture.ts(3 hunks)packages/health/src/HealthManager.ts(2 hunks)packages/health/src/definitions/States.ts(1 hunks)packages/health/src/index.ts(1 hunks)packages/http/src/HttpRemote.ts(2 hunks)packages/http/src/HttpServer.ts(2 hunks)packages/middleware/src/MiddlewareManager.ts(1 hunks)packages/middleware/src/definitions/States.ts(1 hunks)packages/middleware/src/index.ts(1 hunks)packages/runtime/src/client/Client.ts(6 hunks)packages/runtime/src/client/ClientBuilder.ts(2 hunks)packages/runtime/src/server/Server.ts(10 hunks)packages/runtime/src/server/ServerBuilder.ts(5 hunks)packages/runtime/src/server/types/ReportWorkerRequest.ts(1 hunks)packages/services/package.json(1 hunks)packages/services/src/Service.ts(1 hunks)packages/services/src/common/Remote.ts(2 hunks)packages/services/src/common/ReportedStateManager.ts(1 hunks)packages/services/src/common/RequestPool.ts(1 hunks)packages/services/src/common/StateManager.ts(1 hunks)packages/services/src/common/definitions/States.ts(1 hunks)packages/services/src/dummy/DummyProvider.ts(3 hunks)packages/services/src/dummy/DummyRunner.ts(3 hunks)packages/services/src/gateway/Gateway.ts(1 hunks)packages/services/src/gateway/LocalGateway.ts(4 hunks)packages/services/src/gateway/RemoteGateway.ts(5 hunks)packages/services/src/gateway/WorkerBalancer.ts(2 hunks)packages/services/src/gateway/WorkerManager.ts(3 hunks)packages/services/src/gateway/WorkerMonitor.ts(1 hunks)packages/services/src/index.ts(2 hunks)packages/services/src/proxy/LocalProxy.ts(4 hunks)packages/services/src/repository/LocalRepository.ts(3 hunks)packages/services/src/repository/RemoteRepository.ts(4 hunks)packages/services/src/worker/LocalWorker.ts(6 hunks)packages/services/src/worker/RemoteWorker.ts(3 hunks)packages/services/src/worker/RemoteWorkerBuilder.ts(1 hunks)packages/services/src/worker/ReportManager.ts(1 hunks)packages/services/src/worker/Worker.ts(1 hunks)packages/services/test/gateway/LocalGateway.spec.ts(1 hunks)packages/services/test/gateway/fixtures/healthManager.fixture.ts(1 hunks)packages/services/test/gateway/fixtures/localGateways.fixture.ts(1 hunks)packages/services/test/gateway/fixtures/remoteWorkers.fixture.ts(2 hunks)packages/services/test/gateway/fixtures/remotes.fixture.ts(2 hunks)packages/services/test/gateway/fixtures/sourcingManager.fixture.ts(1 hunks)packages/services/test/repository/LocalRepository.spec.ts(1 hunks)packages/services/test/repository/fixtures/healthManager.fixture.ts(1 hunks)packages/services/test/repository/fixtures/localRepositories.fixture.ts(1 hunks)packages/services/test/worker/LocalWorker.spec.ts(1 hunks)packages/services/test/worker/fixtures/executionManager.fixture.ts(1 hunks)packages/services/test/worker/fixtures/healthManager.fixture.ts(1 hunks)packages/services/test/worker/fixtures/localWorkers.fixture.ts(1 hunks)packages/services/test/worker/fixtures/sourcingManager.fixture.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
packages/services/test/worker/fixtures/sourcingManager.fixture.ts (1)
Learnt from: basmasking
PR: MaskingTechnology/jitar#638
File: packages/runtime/src/server/ServerBuilder.ts:7-8
Timestamp: 2025-05-13T18:15:31.425Z
Learning: In the Jitar codebase, always import the `Proxy` class from `@jitar/services` as `LocalProxy` to avoid shadowing the built-in JavaScript `Proxy` global object.
packages/services/test/gateway/fixtures/sourcingManager.fixture.ts (1)
Learnt from: basmasking
PR: MaskingTechnology/jitar#638
File: packages/runtime/src/server/ServerBuilder.ts:7-8
Timestamp: 2025-05-13T18:15:31.425Z
Learning: In the Jitar codebase, always import the `Proxy` class from `@jitar/services` as `LocalProxy` to avoid shadowing the built-in JavaScript `Proxy` global object.
packages/services/src/index.ts (1)
Learnt from: basmasking
PR: MaskingTechnology/jitar#638
File: packages/runtime/src/server/ServerBuilder.ts:7-8
Timestamp: 2025-05-13T18:15:31.425Z
Learning: In the Jitar codebase, always import the `Proxy` class from `@jitar/services` as `LocalProxy` to avoid shadowing the built-in JavaScript `Proxy` global object.
packages/services/src/proxy/LocalProxy.ts (1)
Learnt from: basmasking
PR: MaskingTechnology/jitar#638
File: packages/runtime/src/server/ServerBuilder.ts:7-8
Timestamp: 2025-05-13T18:15:31.425Z
Learning: In the Jitar codebase, always import the `Proxy` class from `@jitar/services` as `LocalProxy` to avoid shadowing the built-in JavaScript `Proxy` global object.
packages/runtime/src/server/Server.ts (1)
Learnt from: basmasking
PR: MaskingTechnology/jitar#638
File: packages/runtime/src/server/ServerBuilder.ts:7-8
Timestamp: 2025-05-13T18:15:31.425Z
Learning: In the Jitar codebase, always import the `Proxy` class from `@jitar/services` as `LocalProxy` to avoid shadowing the built-in JavaScript `Proxy` global object.
packages/runtime/src/server/ServerBuilder.ts (1)
Learnt from: basmasking
PR: MaskingTechnology/jitar#638
File: packages/runtime/src/server/ServerBuilder.ts:7-8
Timestamp: 2025-05-13T18:15:31.425Z
Learning: In the Jitar codebase, always import the `Proxy` class from `@jitar/services` as `LocalProxy` to avoid shadowing the built-in JavaScript `Proxy` global object.
🧬 Code Graph Analysis (19)
packages/services/test/gateway/fixtures/remoteWorkers.fixture.ts (1)
packages/services/src/worker/RemoteWorker.ts (1)
RemoteWorker(20-108)
packages/services/test/worker/fixtures/executionManager.fixture.ts (2)
packages/execution/src/ExecutionManager.ts (1)
ExecutionManager(25-160)packages/services/test/worker/fixtures/sourcingManager.fixture.ts (1)
sourcingManager(4-4)
packages/services/test/repository/fixtures/healthManager.fixture.ts (2)
packages/health/src/HealthManager.ts (1)
HealthManager(15-160)packages/services/test/gateway/fixtures/sourcingManager.fixture.ts (1)
sourcingManager(4-4)
packages/services/src/worker/Worker.ts (4)
packages/services/src/worker/LocalWorker.ts (4)
id(60-60)id(62-62)state(64-64)state(66-66)packages/services/src/worker/RemoteWorker.ts (4)
id(39-39)id(41-41)state(43-43)state(45-45)packages/services/src/common/StateManager.ts (2)
state(9-9)state(11-11)packages/services/src/common/definitions/States.ts (1)
State(15-15)
packages/services/test/worker/fixtures/sourcingManager.fixture.ts (2)
packages/services/test/gateway/fixtures/sourcingManager.fixture.ts (1)
sourcingManager(4-4)packages/sourcing/src/LocalSourcingManager.ts (1)
LocalSourcingManager(7-16)
packages/services/src/Service.ts (1)
packages/services/src/common/definitions/States.ts (1)
State(15-15)
packages/services/test/gateway/fixtures/sourcingManager.fixture.ts (2)
packages/services/test/worker/fixtures/sourcingManager.fixture.ts (1)
sourcingManager(4-4)packages/sourcing/src/LocalSourcingManager.ts (1)
LocalSourcingManager(7-16)
packages/services/src/gateway/WorkerBalancer.ts (2)
packages/services/src/gateway/WorkerMonitor.ts (2)
workers(39-42)worker(44-52)packages/services/src/gateway/WorkerManager.ts (1)
workers(18-21)
packages/services/test/worker/fixtures/healthManager.fixture.ts (2)
packages/health/src/HealthManager.ts (1)
HealthManager(15-160)packages/services/test/worker/fixtures/sourcingManager.fixture.ts (1)
sourcingManager(4-4)
packages/runtime/src/client/ClientBuilder.ts (2)
packages/runtime/src/client/Client.ts (1)
Client(19-94)packages/health/src/HealthManager.ts (1)
HealthManager(15-160)
packages/services/src/worker/ReportManager.ts (1)
packages/services/src/worker/LocalWorker.ts (1)
LocalWorker(30-287)
packages/services/test/worker/fixtures/localWorkers.fixture.ts (1)
packages/services/src/worker/LocalWorker.ts (1)
LocalWorker(30-287)
packages/http/src/HttpRemote.ts (3)
packages/services/src/worker/RemoteWorker.ts (4)
id(39-39)id(41-41)state(43-43)state(45-45)packages/runtime/src/server/Server.ts (1)
state(324-334)packages/services/src/index.ts (1)
State(5-5)
packages/services/test/gateway/fixtures/healthManager.fixture.ts (2)
packages/health/src/HealthManager.ts (1)
HealthManager(15-160)packages/services/test/gateway/fixtures/sourcingManager.fixture.ts (1)
sourcingManager(4-4)
packages/services/test/gateway/fixtures/localGateways.fixture.ts (2)
packages/services/src/gateway/LocalGateway.ts (4)
url(43-43)trustKey(47-47)trustKey(144-147)LocalGateway(24-148)packages/services/test/gateway/fixtures/values.fixture.ts (1)
VALUES(2-8)
packages/services/src/common/ReportedStateManager.ts (2)
packages/services/src/common/StateManager.ts (3)
StateManager(5-64)state(9-9)state(11-11)packages/services/src/common/definitions/States.ts (1)
State(15-15)
packages/services/src/gateway/WorkerManager.ts (5)
packages/services/src/worker/LocalWorker.ts (4)
id(60-60)id(62-62)state(64-64)state(66-66)packages/services/src/worker/RemoteWorker.ts (4)
id(39-39)id(41-41)state(43-43)state(45-45)packages/services/src/common/StateManager.ts (2)
state(9-9)state(11-11)packages/services/src/common/definitions/States.ts (1)
State(15-15)packages/services/src/gateway/WorkerMonitor.ts (1)
worker(44-52)
packages/services/src/proxy/LocalProxy.ts (2)
packages/services/src/common/StateManager.ts (1)
StateManager(5-64)packages/services/src/common/definitions/States.ts (1)
State(15-15)
packages/runtime/src/server/Server.ts (3)
packages/services/src/worker/RemoteWorkerBuilder.ts (1)
RemoteWorkerBuilder(5-27)packages/services/src/gateway/LocalGateway.ts (2)
state(45-45)LocalGateway(24-148)packages/services/src/common/definitions/States.ts (1)
State(15-15)
🔇 Additional comments (73)
packages/health/src/definitions/States.ts (1)
2-8: Same constant defined in at least two packages – centralise to remove duplication.
Statesis copy-pasted here and inpackages/middleware/.... Duplicates invite drift (and already did – see previous comment).
Consider publishing one source of truth (e.g.@jitar/core-states) and re-exporting from the consuming packages.[ suggest_optional_refactor ]
packages/services/package.json (1)
21-21: Wildcard dependency on@jitar/healthjeopardises reproducible builds.Using
"*"means any future breaking change in@jitar/healthcan silently breakservices. Pin with a caret or exact version after verifying compatibility, e.g.:- "@jitar/health": "*", + "@jitar/health": "^0.7.0",Also consider enabling Renovate/Dependabot to automate upgrades with CI safety nets.
packages/middleware/src/index.ts (1)
2-2: LGTM: Well-structured export statement.The export statement correctly follows TypeScript conventions by exporting both the default export as
Statesand the type export forState. This provides consumers with access to both the state constants and the type definition, which is consistent with the state management pattern implemented across packages.packages/health/src/index.ts (1)
2-2: LGTM: Consistent export pattern.The export statement follows the same pattern as other packages in the codebase, providing both the default export as
Statesand the type export forState. This consistency across packages helps maintain a uniform API for state management.packages/services/src/worker/Worker.ts (2)
3-3: LGTM: Appropriate import for state management.The import of
Statetype from the common definitions is correctly placed and necessary for the interface extension.
11-11: LGTM: Well-designed interface extension.The addition of the state setter to the
Workerinterface is well-designed. Based on the relevant code snippets, different worker implementations handle state setting differently:
LocalWorkersets state directly:this.#stateManager.state = stateRemoteWorkerreports state:this.#stateManager.report(state)This interface design allows for implementation-specific state management strategies while maintaining a consistent API.
packages/services/test/worker/fixtures/localWorkers.fixture.ts (2)
5-5: LGTM: Appropriate fixture dependency.The import of
healthManagerfrom the fixture file is correct and necessary to support the newLocalWorkerconstructor requirements.
11-12: LGTM: Consistent dependency injection.The addition of
healthManagerto bothLocalWorkerconstructor calls is consistent and correct. This ensures that both the public and protected worker instances have the necessary health management capabilities for the new state management system.packages/services/test/gateway/fixtures/remoteWorkers.fixture.ts (1)
3-3: Import ofStatesis valid –States.tsusesexport default States. No changes needed.packages/configuration/src/server/index.ts (1)
7-7: LGTM – new export is correctly wired.No issues found; keeps public API in sync with new configuration type.
documentation/docs/deploy/health-checks.md (1)
16-16: Good terminology clarification!The change from "health" to "availability status" better reflects the architectural shift where health checks provide input data for availability determination, rather than being the availability itself. This aligns well with the new state management system introduced in this PR.
packages/configuration/src/server/definitions/GatewayConfiguration.ts (2)
6-6: Good naming improvement with proper breaking change handling.The rename from
monitortomonitorIntervalsignificantly improves clarity about what this configuration property controls. The change is consistently applied to both the type definition and validation scheme.
14-14: Validation scheme correctly updated.The validation scheme is properly updated to match the renamed property, maintaining consistency between the type definition and validation rules.
packages/services/test/gateway/fixtures/healthManager.fixture.ts (1)
1-6: Well-structured test fixture!The fixture follows the established pattern for test fixtures, properly importing dependencies and creating a reusable
HealthManagerinstance. The implementation is clean and focused on its single responsibility.packages/services/test/gateway/fixtures/localGateways.fixture.ts (2)
4-4: Proper integration of healthManager dependency.The import of
healthManagerfrom the new fixture file is clean and follows the established pattern for test fixtures.
10-11: Consistent healthManager integration across gateway instances.Both public and protected gateway instances now properly receive the
healthManagerdependency, maintaining consistency with the updatedLocalGatewayconstructor requirements.packages/services/src/Service.ts (3)
2-2: Appropriate import for state management.The import of
Statetype from the common definitions is properly scoped and supports the new state management functionality.
8-8: Well-designed state property addition.The readonly
stateproperty provides a clean way to access the current service state without allowing external mutation. This is a good design pattern for state management.
18-18: Proper async state update method.The
updateState()method returningPromise<State>is well-designed for asynchronous state updates. This allows services to refresh their state based on health checks or other factors and return the updated state.packages/services/test/repository/fixtures/healthManager.fixture.ts (1)
1-6: LGTM! Clean fixture implementation.The fixture correctly instantiates
HealthManagerwith the requiredsourcingManagerdependency, following the established fixture pattern in the codebase.packages/services/src/common/Remote.ts (2)
5-5: Good addition of State type import.The import is correctly placed and necessary for the new
reportWorkermethod signature.
21-21: LGTM! Worker state reporting method added.The
reportWorkermethod signature is consistent with the interface pattern and correctly accepts worker ID and state parameters for the new push-based health monitoring system.packages/services/test/repository/fixtures/localRepositories.fixture.ts (2)
4-4: Good integration of healthManager dependency.The import correctly adds the required
healthManagerfixture for the LocalRepository constructor updates.
12-13: LGTM! Constructor calls updated with healthManager.Both repository instances now correctly receive the
healthManagerparameter, maintaining consistency with the new LocalRepository constructor signature.packages/configuration/src/server/definitions/WorkerConfiguration.ts (2)
9-9: Good addition of reportInterval configuration property.The optional
reportIntervalproperty correctly supports the new worker state reporting feature with proper typing.
19-19: LGTM! Validation scheme updated correctly.The validation scheme properly defines
reportIntervalas an optional integer, maintaining consistency with the type definition.packages/services/test/worker/fixtures/healthManager.fixture.ts (1)
1-6: LGTM! Consistent fixture implementation for worker tests.The fixture correctly creates a
HealthManagerinstance for worker tests, maintaining consistency with the repository fixture pattern while providing proper test isolation.packages/services/test/gateway/fixtures/sourcingManager.fixture.ts (1)
1-5: LGTM! Clean fixture implementation.The fixture follows established patterns and provides a proper
LocalSourcingManagerinstance for testing. The use of'./'as the location parameter is appropriate for test contexts.packages/services/test/worker/fixtures/executionManager.fixture.ts (1)
5-7: LGTM! Proper dependency injection.The addition of
sourcingManagerparameter to theExecutionManagerconstructor is correct and aligns with the constructor signature requirements. The import and usage are properly implemented.packages/services/src/dummy/DummyProvider.ts (3)
6-7: LGTM! Proper imports for state management.The imports for
StatesandStatetype are correctly added to support the new state management functionality.
16-19: LGTM! Appropriate state getter implementation.The
stategetter returningStates.AVAILABLEis appropriate for a dummy provider implementation that should always report as available.
41-44: LGTM! Consistent updateState implementation.The
updateStatemethod returningStates.AVAILABLEis consistent with thestategetter and appropriate for a dummy provider that maintains a constant available state.packages/configuration/src/server/definitions/ServerConfiguration.ts (3)
9-9: LGTM! Proper import for new configuration type.The import of
RemoteWorkerConfigurationand its validation scheme is correctly added to support the new remote worker configuration feature.
24-24: LGTM! Consistent optional configuration property.The addition of
remoteWorker?: RemoteWorkerConfigurationfollows the same pattern as other optional service configurations in the type definition.
41-42: LGTM! Proper validation scheme update.The validation scheme is correctly updated to include the new
remoteWorkerfield with proper group validation using the importedremoteWorkerValidationScheme.packages/configuration/test/server/fixtures/configuration.fixture.ts (4)
2-2: LGTM! Proper import update.The import statement is correctly updated to include the new
RemoteWorkerConfigurationtype along with existing configuration types.
12-12: LGTM! Consistent property renaming.The renaming from
monitortomonitorIntervalaligns with the broader configuration updates mentioned in the AI summary and maintains consistency across the codebase.
17-17: LGTM! Appropriate test configuration values.The
remoteWorkerConfigurationconstant withunavailableThreshold: 6000andstoppedThreshold: 18000provides reasonable test values for the new remote worker monitoring thresholds.
31-32: LGTM! Proper configuration extension.The
SERVER_CONFIGURATIONis correctly extended to include the newremoteWorkerconfiguration, completing the integration of the new remote worker configuration feature.packages/services/src/gateway/Gateway.ts (1)
10-12: LGTM! Interface changes align with push-based state management.The updated interface correctly reflects the shift to ID-based worker management and adds the necessary
reportWorkermethod for state reporting. The changes are consistent with the broader architecture updates.packages/services/src/dummy/DummyRunner.ts (2)
16-19: LGTM! Appropriate dummy implementation.The dummy implementation correctly returns a constant
AVAILABLEstate, which is suitable for testing scenarios where predictable behavior is needed.
46-49: LGTM! Consistent with state getter implementation.The
updateStatemethod appropriately returns the same constant state as the getter, maintaining consistency in the dummy implementation.packages/http/src/HttpRemote.ts (1)
107-119: LGTM! Well-implemented HTTP state reporting.The implementation follows established patterns in the class:
- Uses the
#callRemotehelper for consistent error handling- Proper JSON serialization of the state
- Correct HTTP method and content-type headers
- Endpoint structure is consistent with other worker operations
packages/services/src/worker/RemoteWorkerBuilder.ts (1)
18-26: LGTM! Clean builder implementation.The build method is well-structured:
- Proper delegation to RemoteBuilder
- Efficient conversion of procedures array to Set
- Clean parameter passing to RemoteWorker constructor
packages/http/src/HttpServer.ts (2)
66-66: LGTM: Route registration follows established pattern.The new route is properly registered alongside other worker-related endpoints in the
#setupRoutesmethod.
193-223: LGTM: Implementation follows established patterns.The
#reportWorkermethod correctly follows the same validation, error handling, and response transformation patterns used by other methods in the class.packages/runtime/src/client/ClientBuilder.ts (2)
4-4: LGTM: Import added for health management integration.The HealthManager import is properly added to support the new health management functionality.
34-34: LGTM: Health manager integration follows established pattern.The implementation correctly follows the same pattern as other builder methods:
- Creates the health manager using the sourcing manager
- Passes it to the Client constructor
- Maintains consistent method naming and structure
Also applies to: 38-38, 41-44
packages/services/src/common/definitions/States.ts (1)
2-15: LGTM: Well-structured state definition following TypeScript best practices.The implementation correctly uses:
as constassertion to ensure literal types- Descriptive state names covering the full lifecycle
- Standard export pattern with default and named exports
- Proper TypeScript type derivation for Keys and State
packages/services/src/gateway/WorkerManager.ts (3)
4-4: LGTM: State type import added for worker state management.The State type import is properly added to support the new worker state reporting functionality.
68-73: LGTM: Worker state reporting method correctly implemented.The
reportWorkermethod properly:
- Uses the existing
getWorkermethod for ID validation and retrieval- Sets the worker's state, which triggers internal state manager updates
- Follows the established error handling pattern (throws UnknownWorker for invalid IDs)
75-94: LGTM: Worker removal by ID is properly implemented.The updated
removeWorkermethod correctly:
- Changes signature to accept ID string instead of Worker instance
- Returns the removed Worker object for caller use
- Uses existing
getWorkermethod for validation and retrieval- Maintains the same cleanup logic for balancers
This change aligns with the ID-based worker management approach shown in the
WorkerMonitorusage.packages/configuration/src/server/definitions/RemoteWorkerConfiguration.ts (1)
1-19: LGTM! Clean configuration type definition.The type definition and validation scheme are well-structured and consistent. Good use of
as constfor the validation scheme.packages/services/src/common/ReportedStateManager.ts (2)
26-47: Discard thread-safety concern for report() and update()JavaScript’s single-threaded event loop means there are no simultaneous in-method executions—
report()andupdate()cannot truly run concurrently. If you need specific ordering between them in async flows, enforce it at the call site (e.g. viaawaitor chaining), but no additional locking or synchronization is required here.Likely an incorrect or invalid review comment.
33-47: Fix incorrect interval calculation in update() method.The current interval calculation
this.#lastUpdate - this.#lastReportdoesn't represent the time since the last report. This will cause incorrect state transitions.update(): State { - this.#lastUpdate = Date.now(); + const now = Date.now(); + const interval = now - this.#lastReport; - const interval = this.#lastUpdate - this.#lastReport; - if (interval >= this.#unavailableThreshold) { this.state = interval >= this.#stoppedThreshold ? States.STOPPED : States.UNAVAILABLE; } return this.state; }Likely an incorrect or invalid review comment.
packages/services/src/index.ts (1)
2-6: LGTM! Export reorganization looks good.The exports are properly organized and follow the established patterns. Good adherence to the LocalProxy naming convention to avoid shadowing the built-in Proxy.
Also applies to: 22-22
packages/services/test/gateway/fixtures/remotes.fixture.ts (1)
6-7: Good integration of state managementThe import path updates and the addition of the
reportWorkermethod properly align with the new state management architecture.Also applies to: 43-47
packages/runtime/src/client/Client.ts (2)
45-72: Clean integration of health management and request poolingThe changes properly integrate the
HealthManagerandRequestPoolinto the Client lifecycle. The async modifications tostart(),stop(), andrunInternal()methods align well with the new architecture.
24-24: Verify Client implements theRunnerinterfacePlease ensure that the
Clientclass you’re passing intonew RequestPool(this)satisfies theRunnerinterface imported from@jitar/execution. In particular:
- Confirm the exact method signatures of
Runner(e.g., return types forstart(),stop(), andrun(request)).- Verify that
Clientdefines each required method with matching names and types:
start(): …stop(): …run(request: Request): Promise<Response>- If there’s a mismatch (for example,
Client.start()returningPromise<void>vs.void), adjust either the interface implementation or theClientmethods accordingly so TypeScript enforces the correct contract.packages/services/src/common/StateManager.ts (1)
5-64: Well-structured state management implementationThe
StateManagerclass provides a clean and focused implementation for lifecycle state management. The state transition methods are clearly named, and the helper methods provide useful abstractions for state checking.packages/health/src/HealthManager.ts (1)
33-43: Excellent state management integrationThe addition of state checks and transitions in the
start()andstop()methods prevents redundant operations and provides clear lifecycle management. The pattern of checking current state, transitioning through intermediate states, and setting final states is well implemented.Also applies to: 47-57
packages/services/src/gateway/WorkerMonitor.ts (1)
3-3: Clean refactoring to state-based monitoringThe refactoring from frequency-based health checks to state-based monitoring aligns well with the new architecture. The naming changes from "frequency" to "interval" improve clarity.
Also applies to: 7-7, 12-12, 16-16, 19-19, 26-26, 31-31, 36-36
packages/services/src/gateway/RemoteGateway.ts (2)
98-101: LGTM! Clean delegation pattern for worker state reporting.The new
reportWorkermethod properly delegates to the remote service.
103-106: removeWorker signature change is safe to mergeAll call sites across the repo have been updated to use a string
id, and there are no remaining invocations that pass aWorkerobject. No further action is needed.packages/runtime/src/server/Server.ts (1)
196-218: LGTM! Well-structured worker reporting implementation.The new
reportWorkermethod correctly validates the state and delegates to the gateway. The error handling and logging are appropriate.packages/services/src/gateway/LocalGateway.ts (2)
49-64: Well-implemented lifecycle management with proper state transitions.The start method correctly:
- Guards against redundant starts
- Sets appropriate state transitions
- Starts dependencies in parallel for efficiency
- Updates availability state based on health
117-127: LGTM! Clean separation of concerns for worker management.The worker reporting and removal methods properly delegate to the WorkerManager and ensure proper cleanup by stopping the worker after removal.
packages/services/src/repository/LocalRepository.ts (1)
50-93: Consistent implementation of state and health management.The LocalRepository correctly implements the same lifecycle and health management pattern as other services, ensuring consistency across the codebase.
packages/services/src/worker/LocalWorker.ts (3)
72-99: Excellent lifecycle management with proper sequencing.The start method correctly:
- Guards against redundant starts
- Starts core services in parallel
- Properly sequences gateway registration before starting the report manager
- Updates availability state based on health
101-128: Proper cleanup sequence prevents race conditions.The stop method correctly stops the ReportManager before removing the worker from the gateway, preventing potential race conditions where reports might be sent after deregistration.
157-165: Safe implementation of gateway reporting.The method properly guards against missing gateway or worker ID, making it safe to call in all scenarios.
packages/runtime/src/server/ServerBuilder.ts (2)
25-42: Clean integration of health management across all services.The build method correctly creates the HealthManager early and consistently passes it to all local services, ensuring uniform health monitoring capabilities.
167-173: LGTM! Proper handling of optional configuration values.The method correctly extracts the optional threshold values from the configuration and creates the RemoteWorkerBuilder instance.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/scheduling/test/dummy.spec.ts (1)
6-6: Address the TODO comment for comprehensive test coverage.Consider adding actual tests for the scheduling functionality to ensure the
ScheduleManagerandScheduledTaskclasses work correctly.Would you like me to help generate unit tests for the scheduling package functionality?
packages/runtime/src/client/ClientBuilder.ts (1)
34-34: Consider making the log level configurable.The logger is hardcoded to use
WARNlevel. Consider making this configurable through theClientConfigurationto allow different logging levels for different environments.type ClientConfiguration = { remoteUrl: string; middleware?: Middleware[]; segments?: Segment[]; + logLevel?: LogLevels; };- const logger = new Logger(LogLevels.WARN); + const logger = new Logger(configuration.logLevel ?? LogLevels.WARN);packages/services/src/gateway/WorkerManager.ts (1)
145-160: Consider error handling in the monitoring logic.The monitoring methods look well-structured, but consider adding error handling for the worker state updates, especially if
updateState()orremoveWorker()can throw exceptions.async #checkWorker(worker: Worker): Promise<void> { + try + { const state = await worker.updateState(); if (state === States.STOPPED) { this.removeWorker(worker.id as string); } + } + catch (error) + { + // Log error but continue monitoring other workers + console.warn(`Failed to check worker ${worker.id}:`, error); + } }packages/services/src/common/StateManager.ts (1)
36-57: Consider the error handling in stop method.The stop method sets the state to UNAVAILABLE on error, which may not be semantically correct. If stopping fails, the service might still be running but in an unknown state.
Consider keeping the state as STOPPING on error or adding a new ERROR state to better represent the failure condition:
catch (error: unknown) { - this.setUnavailable(); + // Keep as STOPPING to indicate incomplete shutdown + // this.setStopping(); + // Or consider adding an ERROR state throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
documentation/docs/fundamentals/runtime-services.md(5 hunks)packages/health/src/HealthManager.ts(2 hunks)packages/middleware/src/MiddlewareManager.ts(1 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/Client.ts(4 hunks)packages/runtime/src/client/ClientBuilder.ts(3 hunks)packages/runtime/src/server/ServerBuilder.ts(5 hunks)packages/scheduling/CHANGELOG.md(1 hunks)packages/scheduling/README.md(1 hunks)packages/scheduling/package.json(1 hunks)packages/scheduling/src/ScheduleManager.ts(1 hunks)packages/scheduling/src/ScheduledTask.ts(1 hunks)packages/scheduling/src/index.ts(1 hunks)packages/scheduling/src/types/Task.ts(1 hunks)packages/scheduling/test/dummy.spec.ts(1 hunks)packages/scheduling/tsconfig.json(1 hunks)packages/scheduling/vite.config.ts(1 hunks)packages/services/package.json(1 hunks)packages/services/src/common/RequestPool.ts(1 hunks)packages/services/src/common/StateManager.ts(1 hunks)packages/services/src/common/errors/RequestCancelled.ts(1 hunks)packages/services/src/gateway/LocalGateway.ts(3 hunks)packages/services/src/gateway/RemoteGateway.ts(5 hunks)packages/services/src/gateway/WorkerBalancer.ts(1 hunks)packages/services/src/gateway/WorkerManager.ts(6 hunks)packages/services/src/gateway/WorkerMonitor.ts(0 hunks)packages/services/src/index.ts(2 hunks)packages/services/src/proxy/LocalProxy.ts(4 hunks)packages/services/src/repository/LocalRepository.ts(3 hunks)packages/services/src/repository/RemoteRepository.ts(4 hunks)packages/services/src/worker/LocalWorker.ts(6 hunks)packages/services/src/worker/RemoteWorker.ts(3 hunks)packages/services/src/worker/Worker.ts(1 hunks)packages/services/test/gateway/WorkerMonitor.spec.ts(0 hunks)packages/services/test/gateway/fixtures/index.ts(0 hunks)packages/services/test/gateway/fixtures/localGateways.fixture.ts(1 hunks)packages/services/test/gateway/fixtures/remoteWorkers.fixture.ts(2 hunks)packages/services/test/gateway/fixtures/scheduleManager.fixture.ts(1 hunks)packages/services/test/gateway/fixtures/workerManagers.fixture.ts(1 hunks)packages/services/test/gateway/fixtures/workerMonitors.fixture.ts(0 hunks)packages/services/test/worker/fixtures/localWorkers.fixture.ts(1 hunks)packages/services/test/worker/fixtures/scheduleManager.fixture.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/services/test/gateway/fixtures/index.ts
- packages/services/test/gateway/WorkerMonitor.spec.ts
- packages/services/test/gateway/fixtures/workerMonitors.fixture.ts
- packages/services/src/gateway/WorkerMonitor.ts
✅ Files skipped from review due to trivial changes (11)
- packages/scheduling/CHANGELOG.md
- packages/services/src/common/errors/RequestCancelled.ts
- packages/runtime/package.json
- packages/services/test/worker/fixtures/scheduleManager.fixture.ts
- packages/scheduling/src/types/Task.ts
- packages/scheduling/src/index.ts
- packages/scheduling/vite.config.ts
- packages/scheduling/README.md
- packages/scheduling/package.json
- packages/scheduling/tsconfig.json
- packages/services/test/gateway/fixtures/scheduleManager.fixture.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/services/package.json
- packages/services/test/worker/fixtures/localWorkers.fixture.ts
- packages/services/test/gateway/fixtures/localGateways.fixture.ts
- packages/services/test/gateway/fixtures/remoteWorkers.fixture.ts
- packages/services/src/gateway/WorkerBalancer.ts
- packages/services/src/repository/RemoteRepository.ts
- packages/middleware/src/MiddlewareManager.ts
- packages/services/src/worker/RemoteWorker.ts
- packages/services/src/index.ts
- documentation/docs/fundamentals/runtime-services.md
- packages/health/src/HealthManager.ts
- packages/runtime/src/client/Client.ts
- packages/services/src/common/RequestPool.ts
- packages/services/src/proxy/LocalProxy.ts
- packages/services/src/repository/LocalRepository.ts
- packages/services/src/gateway/RemoteGateway.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: petermasking
PR: MaskingTechnology/jitar#667
File: packages/health/src/definitions/States.ts:2-8
Timestamp: 2025-07-14T13:50:01.152Z
Learning: In the Jitar codebase, there are different state enums for different purposes:
- Health manager states (packages/health/src/definitions/States.ts) track health check initialization: STARTING, STARTED, STOPPING, STOPPED
- Service states (packages/services/src/common/definitions/States.ts) track service availability: STARTING, AVAILABLE, UNAVAILABLE, STOPPING, STOPPED
- The health manager's STARTED state indicates health checks are loaded/initialized, not that the service is available
- Service availability is determined by the service layer based on health check results, not by the health manager's state
packages/services/src/worker/Worker.ts (1)
Learnt from: petermasking
PR: MaskingTechnology/jitar#667
File: packages/health/src/definitions/States.ts:2-8
Timestamp: 2025-07-14T13:50:01.152Z
Learning: In the Jitar codebase, there are different state enums for different purposes:
- Health manager states (packages/health/src/definitions/States.ts) track health check initialization: STARTING, STARTED, STOPPING, STOPPED
- Service states (packages/services/src/common/definitions/States.ts) track service availability: STARTING, AVAILABLE, UNAVAILABLE, STOPPING, STOPPED
- The health manager's STARTED state indicates health checks are loaded/initialized, not that the service is available
- Service availability is determined by the service layer based on health check results, not by the health manager's state
packages/services/src/common/StateManager.ts (1)
Learnt from: petermasking
PR: MaskingTechnology/jitar#667
File: packages/health/src/definitions/States.ts:2-8
Timestamp: 2025-07-14T13:50:01.152Z
Learning: In the Jitar codebase, there are different state enums for different purposes:
- Health manager states (packages/health/src/definitions/States.ts) track health check initialization: STARTING, STARTED, STOPPING, STOPPED
- Service states (packages/services/src/common/definitions/States.ts) track service availability: STARTING, AVAILABLE, UNAVAILABLE, STOPPING, STOPPED
- The health manager's STARTED state indicates health checks are loaded/initialized, not that the service is available
- Service availability is determined by the service layer based on health check results, not by the health manager's state
packages/services/src/worker/LocalWorker.ts (1)
Learnt from: petermasking
PR: MaskingTechnology/jitar#667
File: packages/health/src/definitions/States.ts:2-8
Timestamp: 2025-07-14T13:50:01.152Z
Learning: In the Jitar codebase, there are different state enums for different purposes:
- Health manager states (packages/health/src/definitions/States.ts) track health check initialization: STARTING, STARTED, STOPPING, STOPPED
- Service states (packages/services/src/common/definitions/States.ts) track service availability: STARTING, AVAILABLE, UNAVAILABLE, STOPPING, STOPPED
- The health manager's STARTED state indicates health checks are loaded/initialized, not that the service is available
- Service availability is determined by the service layer based on health check results, not by the health manager's state
packages/services/src/gateway/LocalGateway.ts (1)
Learnt from: petermasking
PR: MaskingTechnology/jitar#667
File: packages/health/src/definitions/States.ts:2-8
Timestamp: 2025-07-14T13:50:01.152Z
Learning: In the Jitar codebase, there are different state enums for different purposes:
- Health manager states (packages/health/src/definitions/States.ts) track health check initialization: STARTING, STARTED, STOPPING, STOPPED
- Service states (packages/services/src/common/definitions/States.ts) track service availability: STARTING, AVAILABLE, UNAVAILABLE, STOPPING, STOPPED
- The health manager's STARTED state indicates health checks are loaded/initialized, not that the service is available
- Service availability is determined by the service layer based on health check results, not by the health manager's state
packages/runtime/src/server/ServerBuilder.ts (2)
Learnt from: petermasking
PR: MaskingTechnology/jitar#667
File: packages/health/src/definitions/States.ts:2-8
Timestamp: 2025-07-14T13:50:01.152Z
Learning: In the Jitar codebase, there are different state enums for different purposes:
- Health manager states (packages/health/src/definitions/States.ts) track health check initialization: STARTING, STARTED, STOPPING, STOPPED
- Service states (packages/services/src/common/definitions/States.ts) track service availability: STARTING, AVAILABLE, UNAVAILABLE, STOPPING, STOPPED
- The health manager's STARTED state indicates health checks are loaded/initialized, not that the service is available
- Service availability is determined by the service layer based on health check results, not by the health manager's state
Learnt from: basmasking
PR: MaskingTechnology/jitar#638
File: packages/runtime/src/server/ServerBuilder.ts:7-8
Timestamp: 2025-05-13T18:15:31.425Z
Learning: In the Jitar codebase, always import the `Proxy` class from `@jitar/services` as `LocalProxy` to avoid shadowing the built-in JavaScript `Proxy` global object.
🧬 Code Graph Analysis (3)
packages/scheduling/src/ScheduledTask.ts (1)
packages/logging/src/Logger.ts (1)
Logger(5-125)
packages/services/test/gateway/fixtures/workerManagers.fixture.ts (3)
packages/services/src/gateway/WorkerManager.ts (1)
WorkerManager(13-161)packages/services/test/gateway/fixtures/scheduleManager.fixture.ts (1)
scheduleManager(5-5)packages/services/test/gateway/fixtures/remoteWorkers.fixture.ts (1)
REMOTE_WORKERS(47-56)
packages/scheduling/src/ScheduleManager.ts (2)
packages/scheduling/src/ScheduledTask.ts (1)
ScheduledTask(8-67)packages/logging/src/Logger.ts (1)
Logger(5-125)
🔇 Additional comments (44)
packages/services/src/worker/Worker.ts (1)
11-13: Well-designed interface extension for state management.The new methods
isAvailable()andreportState()align perfectly with the PR objectives of enabling push-based state reporting. The method signatures are appropriate and theStatetype import ensures type safety.packages/services/test/gateway/fixtures/workerManagers.fixture.ts (1)
5-9: Correctly updated fixture to match new WorkerManager constructor.The changes properly integrate the
scheduleManagerdependency required by the updatedWorkerManagerconstructor, ensuring tests continue to work with the new scheduling functionality.packages/scheduling/src/ScheduledTask.ts (2)
40-54: Excellent recursive scheduling approach.The use of
setTimeoutwith recursive scheduling is a good design choice that prevents overlapping task executions and provides better control over timing compared tosetInterval.
56-66: Proper error handling implementation.The error handling in
#executeTaskcorrectly catches exceptions and logs them without interrupting the scheduling cycle, which is essential for maintaining system reliability.packages/scheduling/src/ScheduleManager.ts (3)
19-26: Well-designed task creation method.The
createmethod properly instantiatesScheduledTaskwith the shared logger and adds it to the collection. The pattern of returning the created task enables fluent usage.
28-38: Robust task removal implementation.The
removemethod correctly handles the case where the task doesn't exist by checking the index before removal. This prevents errors and maintains collection integrity.
40-48: Efficient bulk operations for lifecycle management.The
startAllandstopAllmethods provide convenient bulk operations for managing all scheduled tasks, which is essential for coordinated lifecycle management.packages/runtime/src/client/ClientBuilder.ts (4)
4-7: LGTM! Clean integration of new dependencies.The imports are properly organized and the new dependencies (HealthManager, Logger, LogLevels, ScheduleManager) are correctly imported from their respective packages.
38-41: Good refactoring of manager creation order.The managers are created in the correct dependency order, and the schedule manager is appropriately created after the sourcing manager setup.
69-72: LGTM! Clean implementation of schedule manager builder.The new private method follows the same pattern as other builder methods and correctly instantiates the ScheduleManager with the provided logger.
43-43: No positional parameters—order check unnecessaryThe
Clientconstructor accepts a singleConfigurationobject (not six positional args), so passingnew Client({ remoteUrl, remote, healthManager, middlewareManager, executionManager, scheduleManager });is correct and property order is irrelevant.
Likely an incorrect or invalid review comment.
packages/services/src/gateway/WorkerManager.ts (5)
3-6: LGTM! Clean import organization.The imports are properly organized and include all necessary types for the new scheduling and state management functionality.
19-24: Good integration with ScheduleManager.The constructor properly initializes the monitoring task with the provided schedule manager and interval. The task creation follows the expected pattern.
33-41: LGTM! Clear lifecycle management methods.The start/stop methods provide clear control over the monitoring task lifecycle, which is essential for proper resource management.
86-91: Good addition of state reporting functionality.The
reportWorkermethod properly retrieves the worker and updates its state, which is essential for the new push-based monitoring system.
93-112: removeWorker return type change is internal and non-breakingThe signature change from
voidtoWorkeronly affectsWorkerManagerand is consumed byLocalGateway, which still returnsPromise<void>. All external callers invokeGateway.removeWorkerand remain unaffected.packages/services/src/common/StateManager.ts (5)
5-5: LGTM! Clean type definition.The
Tasktype clearly defines the expected signature for async operations used in start/stop methods.
15-34: Good implementation of start method with proper error handling.The start method correctly prevents multiple starts and handles errors by reverting to STOPPED state. The early return for already started services is a good optimization.
59-62: Good implementation of isStarted method.The method correctly identifies a service as started when it's not in STOPPED state, which aligns with the service lifecycle.
89-94: LGTM! Useful utility method.The
setAvailabilitymethod provides a clean way to set availability based on a boolean condition and returns the new state for convenience.
106-114: Clean implementation of state checking helpers.The private helper methods provide a clean abstraction for state checking operations and follow the single responsibility principle.
packages/services/src/gateway/LocalGateway.ts (7)
4-7: LGTM! Clean import organization.The imports are properly organized and include all necessary types for the new state management functionality.
19-21: Good dependency injection refactoring.The configuration now requires healthManager and workerManager instances, which promotes better testability and follows dependency injection principles.
30-42: Clean state management integration.The StateManager is properly initialized and the state getter provides clean access to the current gateway state.
48-57: Good coordinated start sequence.The start method properly coordinates starting both health and worker managers concurrently, followed by state updates. Using
Promise.allfor concurrent starts is efficient.
80-85: LGTM! Clean state update logic.The
updateStatemethod properly determines availability based on health status and updates the state manager accordingly.
94-96: Good integration with worker lifecycle.Starting the worker before adding it to the manager ensures proper initialization order.
104-114: Clean separation of worker reporting and removal.The split of functionality into
reportWorkerfor state updates andremoveWorkerfor lifecycle management is a good architectural decision that aligns with the new push-based system.packages/services/src/worker/LocalWorker.ts (8)
3-8: LGTM! Clean import organization.The imports are properly organized and include all necessary types for the new state management, health monitoring, and scheduling functionality.
25-44: Good dependency injection and task setup.The constructor properly accepts the new dependencies and sets up the scheduled reporting task with the provided interval. The StateManager initialization is consistent with other components.
67-67: LGTM! Clean state access.The state getter provides clean access to the current worker state from the StateManager.
75-96: Good coordinated start sequence.The start method properly coordinates starting execution and health managers, handles gateway registration, and starts the reporting task. The sequential operations are well-structured.
100-119: Clean stop sequence with proper cleanup.The stop method properly stops the reporting task, handles gateway deregistration, and stops the managers in the correct order. The cleanup sequence is well-designed.
131-144: Good health method delegation.The health-related methods properly delegate to the HealthManager, maintaining clean separation of concerns.
146-161: LGTM! Clean state management methods.The
updateStateandreportStatemethods provide clean interfaces for state management and reporting. The guard clauses inreportStateprevent unnecessary operations.
284-289: Good implementation of scheduled reporting.The private
#reportmethod cleanly orchestrates the state update and reporting cycle for the scheduled task.packages/runtime/src/server/ServerBuilder.ts (8)
7-8: LGTM: Clean integration of new dependencies.The import statements correctly add the required
ScheduleManagerand new service components. TheLocalProxyimport follows the established pattern to avoid shadowing the built-in JavaScriptProxyobject.
35-37: LGTM: Well-structured manager initialization.The early initialization of both managers follows good practices and ensures they're available for all service builders. The dependency injection pattern is cleanly implemented.
42-44: LGTM: Proper integration of RemoteWorkerBuilder.The extraction of the remote worker builder construction into a separate method and its integration into the Server constructor is well-structured and maintains clean separation of concerns.
47-53: LGTM: Consistent manager propagation across service builders.The systematic passing of both managers to all relevant service builders (except repository which only needs health manager) is consistent and aligns with the new state management architecture.
85-88: LGTM: Proper configuration property migration and WorkerManager integration.The rename from
monitortomonitorIntervalmaintains backward compatibility expectations, and theWorkerManagerconstruction with bothscheduleManagerandmonitorIntervalcorrectly implements the push-based worker monitoring system described in the PR objectives.
102-106: LGTM: Comprehensive worker configuration integration.The addition of
reportIntervalconfiguration and passing of both managers to theLocalWorkerconstructor properly implements the worker state reporting mechanism. The systematic integration maintains consistency with the existing codebase patterns.
171-174: LGTM: Simple and correct ScheduleManager factory method.The
#buildScheduleManagermethod follows the established pattern used by other manager builders in the class and correctly passes the logger dependency.
176-182: LGTM: Proper RemoteWorkerBuilder configuration extraction.The method correctly extracts optional configuration properties using safe optional chaining and constructs the
RemoteWorkerBuilderwith appropriate parameters. The threshold configurations align with the fine-grained monitoring mentioned in the PR objectives.
| @@ -0,0 +1,9 @@ | |||
|
|
|||
| # Jitar Build | |||
|
2 participants
Fixes #561
Fixes #565
Changes proposed in this pull request:
@MaskingTechnology/jitar