Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion apps/api/src/app/app.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import { DbManagerModule } from '../modules/dbmanager/dbmanager.module.js';
import { WorkspacesModule } from '../modules/workspaces/workspaces.module.js';
import { StitchesModule } from '../modules/stitches/stitches.module.js';
import { SchedulerModule } from '../modules/scheduler/scheduler.module.js';
import { WebhooksModule } from '../modules/webhooks/webhooks.module.js';
import { ShutdownService } from '../core/shutdown.service.js';

@Module({
imports: [
Expand Down Expand Up @@ -90,8 +92,9 @@ import { SchedulerModule } from '../modules/scheduler/scheduler.module.js';
WorkspacesModule,
StitchesModule,
SchedulerModule,
WebhooksModule,
],
controllers: [AppController],
providers: [AppService],
providers: [AppService, ShutdownService],
})
export class AppModule {}
126 changes: 126 additions & 0 deletions apps/api/src/core/shutdown.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import { ShutdownService } from './shutdown.service.js';
import type { INestApplication } from '@nestjs/common';

describe('ShutdownService', () => {
let service: ShutdownService;
let processOnceSpy: ReturnType<typeof vi.spyOn>;
let processExitSpy: ReturnType<typeof vi.spyOn>;

beforeEach(() => {
vi.useFakeTimers();
service = new ShutdownService();
// Stub process.once so no real OS signal handlers are registered between tests.
// The spy still captures calls so we can extract handlers and invoke them manually.
processOnceSpy = vi
.spyOn(process, 'once')
.mockImplementation((_event, _listener) => process);
processExitSpy = vi
.spyOn(process, 'exit')
.mockImplementation(() => undefined as never);
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

afterEach(() => {
vi.useRealTimers();
vi.restoreAllMocks();
});

/** Extract the handler registered for a given signal via process.once. */
function getHandler(signal: string): (() => void) | undefined {
const call = processOnceSpy.mock.calls.find((c) => c[0] === signal);
return call?.[1] as (() => void) | undefined;
}

it('enableShutdownHooks registers handlers for SIGTERM and SIGINT', () => {
const app = { close: vi.fn() } as unknown as INestApplication;

service.enableShutdownHooks(app);

const registeredSignals = processOnceSpy.mock.calls.map((c) => c[0]);
expect(registeredSignals).toContain('SIGTERM');
expect(registeredSignals).toContain('SIGINT');
});

it('is idempotent — calling enableShutdownHooks twice only registers handlers once', () => {
const app = { close: vi.fn() } as unknown as INestApplication;

service.enableShutdownHooks(app);
service.enableShutdownHooks(app);

const sigtermCalls = processOnceSpy.mock.calls.filter(
(c) => c[0] === 'SIGTERM',
);
expect(sigtermCalls).toHaveLength(1);

const sigintCalls = processOnceSpy.mock.calls.filter(
(c) => c[0] === 'SIGINT',
);
expect(sigintCalls).toHaveLength(1);
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

it('on signal, calls app.close() and then process.exit(0)', async () => {
const closeMock = vi.fn().mockResolvedValue(undefined);
const app = { close: closeMock } as unknown as INestApplication;

service.enableShutdownHooks(app);

const handler = getHandler('SIGTERM');
expect(handler).toBeDefined();
handler!();

await vi.advanceTimersByTimeAsync(0);

expect(closeMock).toHaveBeenCalledOnce();
expect(processExitSpy).toHaveBeenCalledWith(0);
});

it('on signal, if app.close() rejects, calls process.exit(1)', async () => {
const closeMock = vi.fn().mockRejectedValue(new Error('close failed'));
const app = { close: closeMock } as unknown as INestApplication;

service.enableShutdownHooks(app);

const handler = getHandler('SIGTERM');
expect(handler).toBeDefined();
handler!();
Comment thread
coderabbitai[bot] marked this conversation as resolved.

await vi.advanceTimersByTimeAsync(0);

expect(closeMock).toHaveBeenCalledOnce();
expect(processExitSpy).toHaveBeenCalledWith(1);
});

it('does not call app.close() a second time when a duplicate signal fires while draining', async () => {
const closeMock = vi.fn().mockReturnValue(new Promise(() => {}));
const app = { close: closeMock } as unknown as INestApplication;

service.enableShutdownHooks(app);

const handler = getHandler('SIGTERM');
expect(handler).toBeDefined();

// Fire SIGTERM twice in quick succession
handler!();
handler!();

await vi.advanceTimersByTimeAsync(0);

expect(closeMock).toHaveBeenCalledOnce();
});

it('hard deadline calls process.exit(1) after 30s', async () => {
const app = {
close: vi.fn().mockReturnValue(new Promise(() => {})),
} as unknown as INestApplication;

service.enableShutdownHooks(app);

const handler = getHandler('SIGTERM');
expect(handler).toBeDefined();
handler!();

await vi.advanceTimersByTimeAsync(30_000);

expect(processExitSpy).toHaveBeenCalledWith(1);
});
});
79 changes: 79 additions & 0 deletions apps/api/src/core/shutdown.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { Injectable, Logger } from '@nestjs/common';
import type { INestApplication } from '@nestjs/common';

/** Hard drain timeout before force-exiting the process. */
const DRAIN_TIMEOUT_MS = 30_000;

/**
* ShutdownService -- graceful SIGTERM/SIGINT drain.
*
* Registers OS signal handlers that call app.close(), which triggers
* OnModuleDestroy hooks on all providers (including QueueService.stopConsuming()).
* A hard 30-second deadline force-exits the process if drain stalls.
*
* Call enableShutdownHooks(app) immediately after app.listen() in main.ts.
*/
@Injectable()
export class ShutdownService {
private readonly logger = new Logger(ShutdownService.name);
/** Prevents re-entrant shutdown if multiple signals arrive while draining. */
private shuttingDown = false;
/** Prevents duplicate listener registration if enableShutdownHooks is called more than once. */
private shutdownHooksEnabled = false;

enableShutdownHooks(app: INestApplication): void {
if (this.shutdownHooksEnabled) {
this.logger.warn(
'Shutdown hooks already registered — ignoring duplicate call',
);
return;
}
this.shutdownHooksEnabled = true;
// process.once ensures each signal fires the handler at most once, even if
// the OS delivers duplicates before the first handler finishes executing.
for (const signal of ['SIGTERM', 'SIGINT'] as const) {
process.once(signal, () => void this.shutdown(app, signal));
}
Comment on lines +34 to +36

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify all process-level signal handlers and confirm whether shutdown is centralized.
rg -n -C2 "process\.(once|on)\('SIGTERM'|process\.(once|on)\('SIGINT'"

Repository: pramodnarayana/nexiom

Length of output: 540


🏁 Script executed:

#!/bin/bash
# Get full context of database client initialization and signal handler registration
sed -n '39,51p' packages/database/src/client.ts

Repository: pramodnarayana/nexiom

Length of output: 479


🏁 Script executed:

#!/bin/bash
# Get ShutdownService implementation to see how it coordinates shutdown
cat -n apps/api/src/core/shutdown.service.ts | head -80

Repository: pramodnarayana/nexiom

Length of output: 3342


🏁 Script executed:

#!/bin/bash
# Check if there are any other competing signal handlers in the codebase
rg -n "process\.(once|on)\(" --type ts --type js | grep -E "(SIGTERM|SIGINT|SIGHUP|SIGKILL)" | head -20

Repository: pramodnarayana/nexiom

Length of output: 229


Unify SIGTERM/SIGINT ownership to avoid shutdown races.

ShutdownService registers signal handlers that call app.close() (lines 34-36), but packages/database/src/client.ts:48-49 independently registers the same signals with process.once() to call pool!.end() directly. Since process.once() fires handlers in registration order with no guaranteed sequencing, the pool may close before app.close() completes its OnModuleDestroy chain, violating the "in-flight queries finish cleanly" invariant. Remove the direct signal handlers from the database client; instead, register an OnModuleDestroy hook in the database module to drain the pool as part of the coordinated app.close() flow.

Additionally, at line 72, Logger.error() receives an Error object as the second argument, but the signature expects a string trace. Call err.stack or use a proper error formatter to preserve stack diagnostics:

this.logger.error('Error during graceful shutdown -- forcing exit', err.stack || String(err));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/core/shutdown.service.ts` around lines 34 - 36, ShutdownService
registers signal handlers that call app.close(), but the database client also
calls process.once(...) to pool!.end(), causing race conditions; remove the
direct signal handlers from the database client
(packages/database/src/client.ts) and instead implement OnModuleDestroy in the
database module (e.g., add a class implementing OnModuleDestroy with a method
that calls pool!.end()) so draining the pool runs as part of the coordinated
app.close() shutdown path; also update the ShutdownService logger call that
passes an Error object to Logger.error (the call where it logs "Error during
graceful shutdown -- forcing exit") to pass err.stack or a stringified error
(err.stack || String(err)) so the trace is preserved.

this.logger.log('Graceful shutdown hooks registered (SIGTERM, SIGINT)');
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

private async shutdown(app: INestApplication, signal: string): Promise<void> {
if (this.shuttingDown) {
this.logger.log(
`Already shutting down — ignoring duplicate signal ${signal}`,
);
return;
}
this.shuttingDown = true;

this.logger.log(
`Received ${signal} -- draining in-flight work (timeout ${DRAIN_TIMEOUT_MS / 1_000}s)`,
);

// Hard deadline -- force-exits if drain takes too long.
const deadline = setTimeout(() => {
this.logger.error(
`Drain deadline exceeded (${DRAIN_TIMEOUT_MS / 1_000}s) -- forcing exit`,
);
process.exit(1);
}, DRAIN_TIMEOUT_MS);
// Allow the event loop to exit naturally if everything else closes first.
deadline.unref();

try {
// app.close() triggers OnModuleDestroy on all providers.
// QueueService.stopConsuming() drains SQS consumers here.
await app.close();
clearTimeout(deadline);
this.logger.log('Graceful shutdown complete');
process.exit(0);
} catch (err) {
clearTimeout(deadline);
this.logger.error(
'Error during graceful shutdown -- forcing exit',
err instanceof Error ? (err.stack ?? err.message) : String(err),
);
process.exit(1);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}
}
Loading
Loading