diff --git a/CHANGELOG.md b/CHANGELOG.md index b450836..3b61866 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ ### Fixes +- Bound each worker sidecar hello attempt to 30 seconds, retry failed connections, and cancel + pending hello calls and reconnect delays when the worker stops. + ## v0.4.0 (2026-07-31) ### Changes diff --git a/packages/durabletask-js/src/utils/backoff.util.ts b/packages/durabletask-js/src/utils/backoff.util.ts index 7209790..c5fa45a 100644 --- a/packages/durabletask-js/src/utils/backoff.util.ts +++ b/packages/durabletask-js/src/utils/backoff.util.ts @@ -130,18 +130,32 @@ export class ExponentialBackoff { * Waits for the current backoff delay, then increments the attempt count * and calculates the next delay. * - * @returns Promise that resolves after the delay. + * @param signal Optional signal that cancels the pending delay. + * @returns Promise that resolves after the delay or rejects when aborted. */ - async wait(): Promise { + async wait(signal?: AbortSignal): Promise { const delay = this._calculateDelayWithJitter(); - await new Promise((resolve) => setTimeout(resolve, delay)); + if (signal?.aborted) { + throw signal.reason; + } + + await new Promise((resolve, reject) => { + const onAbort = () => { + clearTimeout(timeoutId); + signal?.removeEventListener("abort", onAbort); + reject(signal?.reason); + }; + const timeoutId = setTimeout(() => { + signal?.removeEventListener("abort", onAbort); + resolve(); + }, delay); + + signal?.addEventListener("abort", onAbort, { once: true }); + }); this._attemptCount++; - this._currentDelayMs = Math.min( - this._currentDelayMs * this._multiplier, - this._maxDelayMs, - ); + this._currentDelayMs = Math.min(this._currentDelayMs * this._multiplier, this._maxDelayMs); } /** diff --git a/packages/durabletask-js/src/worker/task-hub-grpc-worker.ts b/packages/durabletask-js/src/worker/task-hub-grpc-worker.ts index 56bad09..6adf399 100644 --- a/packages/durabletask-js/src/worker/task-hub-grpc-worker.ts +++ b/packages/durabletask-js/src/worker/task-hub-grpc-worker.ts @@ -40,6 +40,7 @@ import { /** Default timeout in milliseconds for graceful shutdown. */ const DEFAULT_SHUTDOWN_TIMEOUT_MS = 30000; +const HELLO_TIMEOUT_MS = 30000; /** * Options for creating a TaskHubGrpcWorker. @@ -88,6 +89,8 @@ export class TaskHubGrpcWorker { private _backoff: ExponentialBackoff; private _versioning?: VersioningOptions; private _workItemFilters?: WorkItemFilters | "auto"; + private _abortController: AbortController | null; + private _helloCall: grpc.ClientUnaryCall | null; /** * Creates a new TaskHubGrpcWorker instance. @@ -179,6 +182,8 @@ export class TaskHubGrpcWorker { }); this._versioning = resolvedVersioning; this._workItemFilters = resolvedWorkItemFilters; + this._abortController = null; + this._helloCall = null; } /** @@ -195,13 +200,28 @@ export class TaskHubGrpcWorker { * Creates a new gRPC client and retries the worker. * Properly closes the old client to prevent connection leaks. */ - private async _createNewClientAndRetry(): Promise { + private async _createNewClientAndRetry(signal?: AbortSignal): Promise { + if (signal?.aborted) { + return; + } + // Close the old stub to prevent connection leaks if (this._stub) { this._stub.close(); } - await this._backoff.wait(); + try { + await this._backoff.wait(signal); + } catch (err) { + if (signal?.aborted) { + return; + } + throw err; + } + + if (signal?.aborted) { + return; + } const newClient = new GrpcClient( this._hostAddress, @@ -212,8 +232,8 @@ export class TaskHubGrpcWorker { this._stub = newClient.stub; // Do not await - run in background - this.internalRunWorker(newClient, true).catch((err) => { - if (!this._stopWorker) { + this.internalRunWorker(newClient, true, signal).catch((err) => { + if (!signal?.aborted && !this._stopWorker) { WorkerLogs.workerError(this._logger, err); } }); @@ -388,13 +408,17 @@ export class TaskHubGrpcWorker { throw new Error("The worker is already running."); } + this._stopWorker = false; + this._backoff.reset(); + const abortController = new AbortController(); + this._abortController = abortController; const client = new GrpcClient(this._hostAddress, this._grpcChannelOptions, this._tls, this._grpcChannelCredentials); this._stub = client.stub; // Run in background but catch any unhandled errors to prevent unhandled rejections - this.internalRunWorker(client).catch((err) => { + this.internalRunWorker(client, false, abortController.signal).catch((err) => { // Only log if the worker wasn't stopped intentionally - if (!this._stopWorker) { + if (!abortController.signal.aborted) { WorkerLogs.workerError(this._logger, err); } }); @@ -402,16 +426,58 @@ export class TaskHubGrpcWorker { this._isRunning = true; } - async internalRunWorker(client: GrpcClient, isRetry: boolean = false): Promise { + async internalRunWorker(client: GrpcClient, _isRetry: boolean = false, signal?: AbortSignal): Promise { try { // send a "Hello" message to the sidecar to ensure that it's listening - await callWithMetadata(client.stub.hello.bind(client.stub), new Empty(), this._metadataGenerator); + const helloMetadata = await this._getMetadata(); + if (signal?.aborted) { + return; + } + await new Promise((resolve, reject) => { + let helloCall: grpc.ClientUnaryCall | undefined = undefined; + let settled = false; + const finish = (err?: Error) => { + if (settled) return; + settled = true; + signal?.removeEventListener("abort", onAbort); + if (this._helloCall === helloCall) { + this._helloCall = null; + } + if (err) { + reject(err); + } else { + resolve(); + } + }; + const onAbort = () => { + helloCall?.cancel(); + finish(signal?.reason); + }; + signal?.addEventListener("abort", onAbort, { once: true }); + try { + helloCall = client.stub.hello( + new Empty(), + helloMetadata, + { deadline: new Date(Date.now() + HELLO_TIMEOUT_MS) }, + (err) => finish(err ?? undefined), + ); + if (!settled) { + this._helloCall = helloCall; + } + } catch (err) { + const normalizedError = err instanceof Error ? err : new Error(String(err)); + finish(normalizedError); + } + }); // Reset backoff on successful connection this._backoff.reset(); // Stream work items from the sidecar (pass metadata for insecure connections) const metadata = await this._getMetadata(); + if (signal?.aborted) { + return; + } const request = this._buildGetWorkItemsRequest(); const stream = client.stub.getWorkItems(request, metadata); @@ -449,7 +515,7 @@ export class TaskHubGrpcWorker { // Wait for the stream to end or error stream.on("end", () => { - if (this._stopWorker) { + if (signal?.aborted || this._stopWorker) { WorkerLogs.streamEnded(this._logger); stream.removeAllListeners(); stream.destroy(); @@ -460,8 +526,8 @@ export class TaskHubGrpcWorker { stream.on("error", () => {}); // Prevent unhandled "error" after cleanup stream.destroy(); WorkerLogs.streamRetry(this._logger, this._backoff.peekNextDelay()); - this._createNewClientAndRetry().catch((retryErr) => { - if (!this._stopWorker) { + this._createNewClientAndRetry(signal).catch((retryErr) => { + if (!signal?.aborted && !this._stopWorker) { WorkerLogs.workerError(this._logger, retryErr instanceof Error ? retryErr : new Error(String(retryErr))); } }); @@ -469,7 +535,7 @@ export class TaskHubGrpcWorker { stream.on("error", (err: Error) => { // Ignore cancellation errors when the worker is being stopped intentionally - if (this._stopWorker) { + if (signal?.aborted || this._stopWorker) { return; } WorkerLogs.streamErrorInfo(this._logger, err); @@ -482,24 +548,21 @@ export class TaskHubGrpcWorker { stream.on("error", () => {}); // Prevent unhandled "error" after cleanup stream.destroy(); WorkerLogs.streamRetry(this._logger, this._backoff.peekNextDelay()); - this._createNewClientAndRetry().catch((retryErr) => { - if (!this._stopWorker) { + this._createNewClientAndRetry(signal).catch((retryErr) => { + if (!signal?.aborted && !this._stopWorker) { WorkerLogs.workerError(this._logger, retryErr instanceof Error ? retryErr : new Error(String(retryErr))); } }); }); } catch (err) { - if (this._stopWorker) { + if (signal?.aborted || this._stopWorker) { // ignoring the error because the worker has been stopped return; } const error = err instanceof Error ? err : new Error(String(err)); WorkerLogs.streamError(this._logger, error); - if (!isRetry) { - throw error; - } WorkerLogs.connectionRetry(this._logger, this._backoff.peekNextDelay()); - await this._createNewClientAndRetry(); + await this._createNewClientAndRetry(signal); return; } } @@ -513,19 +576,27 @@ export class TaskHubGrpcWorker { throw new Error("The worker is not running."); } + const abortController = this._abortController; + const helloCall = this._helloCall; + const responseStream = this._responseStream; this._stopWorker = true; + abortController?.abort(); + if (this._helloCall === helloCall) { + helloCall?.cancel(); + this._helloCall = null; + } // Cancel stream first while error handlers are still attached // This allows the error handler to suppress CANCELLED errors - this._responseStream?.cancel(); + responseStream?.cancel(); // Wait for the stream to react to cancellation using events rather than a fixed delay. // This avoids race conditions caused by relying on timing alone. - if (this._responseStream) { + if (responseStream) { try { await withTimeout( new Promise((resolve) => { - const stream = this._responseStream!; + const stream = responseStream; // Any of these events indicates the stream has processed cancellation / is closing. stream.once("end", resolve); stream.once("close", resolve); @@ -540,8 +611,11 @@ export class TaskHubGrpcWorker { } // Now safe to remove listeners and destroy - this._responseStream?.removeAllListeners(); - this._responseStream?.destroy(); + responseStream?.removeAllListeners(); + responseStream?.destroy(); + if (this._responseStream === responseStream) { + this._responseStream = null; + } // Wait for pending work items to complete with timeout if (this._pendingWorkItems.size > 0) { @@ -564,6 +638,9 @@ export class TaskHubGrpcWorker { this._stub.close(); } this._isRunning = false; + if (this._abortController === abortController) { + this._abortController = null; + } // Brief pause to allow gRPC cleanup // https://github.com/grpc/grpc-node/issues/1563#issuecomment-829483711 diff --git a/packages/durabletask-js/test/backoff.spec.ts b/packages/durabletask-js/test/backoff.spec.ts index 63830d5..24aeaae 100644 --- a/packages/durabletask-js/test/backoff.spec.ts +++ b/packages/durabletask-js/test/backoff.spec.ts @@ -81,6 +81,29 @@ describe("ExponentialBackoff", () => { await backoff.wait(); expect(backoff.currentDelayMs).toBe(100); // 50 * 10 = 500, capped at 100 }); + + it("should abort without advancing the backoff state", async () => { + jest.useFakeTimers(); + const backoff = new ExponentialBackoff({ + initialDelayMs: 1000, + multiplier: 2, + jitterFactor: 0, + }); + const controller = new AbortController(); + const abortError = new Error("stopped"); + + try { + const waitPromise = backoff.wait(controller.signal); + controller.abort(abortError); + + await expect(waitPromise).rejects.toBe(abortError); + expect(backoff.attemptCount).toBe(0); + expect(backoff.currentDelayMs).toBe(1000); + expect(jest.getTimerCount()).toBe(0); + } finally { + jest.useRealTimers(); + } + }); }); describe("reset", () => { diff --git a/packages/durabletask-js/test/worker-startup.spec.ts b/packages/durabletask-js/test/worker-startup.spec.ts new file mode 100644 index 0000000..cf55cbb --- /dev/null +++ b/packages/durabletask-js/test/worker-startup.spec.ts @@ -0,0 +1,134 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as grpc from "@grpc/grpc-js"; +import { Empty } from "google-protobuf/google/protobuf/empty_pb"; +import { GrpcClient } from "../src/client/client-grpc"; +import * as stubs from "../src/proto/orchestrator_service_grpc_pb"; +import { Logger, NoOpLogger } from "../src/types/logger.type"; +import { TaskHubGrpcWorker } from "../src/worker/task-hub-grpc-worker"; + +type HelloCallback = (error: grpc.ServiceError | null, response: Empty) => void; + +function useStub(hello: jest.Mock): jest.SpyInstance { + const stub = { + hello, + getWorkItems: jest.fn(), + close: jest.fn(), + } as unknown as stubs.TaskHubSidecarServiceClient; + return jest.spyOn(GrpcClient.prototype as any, "_generateClient").mockReturnValue(stub); +} + +function getCallback(args: any[]): HelloCallback { + return args[args.length - 1] as HelloCallback; +} + +async function flushPromises(): Promise { + for (let i = 0; i < 5; i++) { + await Promise.resolve(); + } +} + +async function stopWorker(worker: TaskHubGrpcWorker): Promise { + const stop = worker.stop(); + await jest.advanceTimersByTimeAsync(1000); + await stop; +} + +describe("TaskHubGrpcWorker startup", () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + jest.restoreAllMocks(); + }); + + it("sets a 30-second deadline on every hello and retries an initial failure", async () => { + const deadlines: number[] = []; + const hello = jest.fn((...args: any[]) => { + deadlines.push((args[2] as grpc.CallOptions).deadline!.valueOf() - Date.now()); + getCallback(args)(new Error("unavailable") as grpc.ServiceError, new Empty()); + return { cancel: jest.fn() } as unknown as grpc.ClientUnaryCall; + }); + const logger = { + error: jest.fn(), + warn: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + } satisfies Logger; + useStub(hello); + const worker = new TaskHubGrpcWorker({ logger }); + + await expect(worker.start()).resolves.toBeUndefined(); + await flushPromises(); + await jest.advanceTimersByTimeAsync(2000); + + expect(hello).toHaveBeenCalledTimes(2); + expect(deadlines).toEqual([30000, 30000]); + expect(logger.error).toHaveBeenCalled(); + + await stopWorker(worker); + }); + + it("logs and retries when hello throws synchronously", async () => { + const hello = jest + .fn() + .mockImplementationOnce(() => { + throw new Error("synchronous hello failure"); + }) + .mockImplementation(() => ({ cancel: jest.fn() }) as unknown as grpc.ClientUnaryCall); + const logger = { + error: jest.fn(), + warn: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + } satisfies Logger; + useStub(hello); + const worker = new TaskHubGrpcWorker({ logger }); + + await worker.start(); + await flushPromises(); + await jest.advanceTimersByTimeAsync(2000); + + expect(logger.error).toHaveBeenCalled(); + expect(hello).toHaveBeenCalledTimes(2); + + await stopWorker(worker); + }); + + it("cancels a pending hello when stopped", async () => { + const cancel = jest.fn(); + const hello = jest.fn(() => ({ cancel }) as unknown as grpc.ClientUnaryCall); + useStub(hello); + const worker = new TaskHubGrpcWorker({ logger: new NoOpLogger() }); + + await worker.start(); + await flushPromises(); + await stopWorker(worker); + + expect(cancel).toHaveBeenCalledTimes(1); + expect((worker as any)._isRunning).toBe(false); + }); + + it("cancels reconnect backoff and prevents a stopped run from reconnecting", async () => { + const hello = jest.fn((...args: any[]) => { + getCallback(args)(new Error("unavailable") as grpc.ServiceError, new Empty()); + return { cancel: jest.fn() } as unknown as grpc.ClientUnaryCall; + }); + const generateClient = useStub(hello); + const worker = new TaskHubGrpcWorker({ logger: new NoOpLogger() }); + + await worker.start(); + await flushPromises(); + expect(jest.getTimerCount()).toBeGreaterThan(0); + + await stopWorker(worker); + await jest.advanceTimersByTimeAsync(60000); + + expect(generateClient).toHaveBeenCalledTimes(1); + expect(hello).toHaveBeenCalledTimes(1); + expect(jest.getTimerCount()).toBe(0); + }); +}); diff --git a/packages/durabletask-js/test/worker-stream-recovery.spec.ts b/packages/durabletask-js/test/worker-stream-recovery.spec.ts index aab4d5e..a1e205c 100644 --- a/packages/durabletask-js/test/worker-stream-recovery.spec.ts +++ b/packages/durabletask-js/test/worker-stream-recovery.spec.ts @@ -32,7 +32,7 @@ function createMockClient(): { mockStream.cancel = jest.fn(); const stub = { - hello: (_req: any, _metadata: any, callback: (err: any, res: any) => void) => { + hello: (_req: any, _metadata: any, _options: any, callback: (err: any, res: any) => void) => { callback(null, {}); return {} as any; },