fix(indexer): handle SIGTERM/SIGINT with orderly shutdown - #3554
Conversation
📝 WalkthroughWalkthroughChangesThe indexer now uses Indexer application lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/indexer/package.jsonESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/indexer/src/index.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. apps/indexer/src/lib/server-logger/server-logger.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3554 +/- ##
==========================================
+ Coverage 75.39% 75.40% +0.01%
==========================================
Files 1165 1165
Lines 30316 30316
Branches 7540 7540
==========================================
+ Hits 22856 22860 +4
- Misses 6587 6591 +4
+ Partials 873 865 -8
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this introduces a new shutdown subsystem wired into a production-critical process (indexer), so I think it's worth a human pass before merging.
What was reviewed:
- Shutdown ordering: verified registration order (database → block cache → node accessor → scheduler → http server) unwinds correctly in reverse (http server closed first, database last), matching the stated intent.
- Deadline/force-exit path, error continuation between steps, and idempotency on repeated signals — all covered by the new unit tests.
- Scheduler.stop() and NodeAccessor.stop() interval clearing and in-flight work draining look correct against their callers.
Extended reasoning...
Overview
This PR adds a GracefulShutdown manager to apps/indexer that listens for SIGINT/SIGTERM and tears down registered resources (HTTP server, scheduler, node accessor, LevelDB caches, Sequelize pool) in reverse-registration order within a bounded deadline, force-exiting on timeout. It also adds stop() methods to Scheduler and NodeAccessor to clear timers and drain in-flight work, and introduces Vitest as a new unit-test runner for this app (previously untested) along with a thorough spec file for the shutdown manager.
Security risks
None identified. No auth, crypto, or user-facing input handling is touched. The change is purely about process lifecycle management for an internal backend service.
Level of scrutiny
This is a production-critical code path for the indexer (a service that syncs blockchain data continuously), even though the change itself is not security-sensitive. It introduces a new abstraction with some non-trivial async/ordering logic (deadline racing, LIFO teardown, drain loops) rather than being a purely mechanical change, so I lean toward wanting a human to sanity-check the wiring and ordering decisions, even though I did not find a concrete bug.
Other factors
The PR includes solid unit test coverage for the new GracefulShutdown class (reverse order, error continuation, deadline handling, idempotent signal handling, actual signal wiring), and the ordering logic checked out against the registration call sites in index.ts. This is also the first unit test infrastructure added to apps/indexer (new vitest.config.ts and test scripts), which is a reasonable but notable addition to the app's tooling.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
apps/indexer/src/index.ts (1)
165-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
LoggerServicefor the added application logs.Lines 166 and 170 add direct console logging. Replace both calls with the repository
LoggerService.As per coding guidelines, use
LoggerServiceinstead ofconsole.log,console.warn, orconsole.error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/indexer/src/index.ts` around lines 165 - 172, Replace the direct console.log and console.error calls in the server startup and initialization catch block with the repository’s LoggerService, preserving their existing messages and error context. Update the logging around app.listen and the catch handling without changing the surrounding shutdown or Sentry behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/indexer/src/chain/dataStore.ts`:
- Around line 47-49: Update closeCaches to use Promise.allSettled for
blocksDb.close() and blockResultsDb.close(), ensuring both cache closures settle
before shutdown continues; after both complete, propagate any rejection so
failures remain visible.
In `@apps/indexer/src/chain/nodeAccessor.ts`:
- Around line 67-77: Serialize saveNodeStatus executions so periodic writes
cannot overlap with the final persistence in stop(). Track and await any
in-flight save after clearing saveStatusInterval, then call and await the final
saveNodeStatus() only after the previous write completes.
In `@apps/indexer/src/index.ts`:
- Around line 169-173: Update the initialization catch block in the startup flow
to invoke the registered node accessor cleanup after reporting the error, then
terminate the process with a non-zero exit code. Ensure the initialization
failure is not swallowed and the cleanup runs when setup succeeds far enough to
register the accessor.
In `@apps/indexer/src/shared/shutdown/graceful-shutdown.spec.ts`:
- Around line 60-67: Update the “triggers shutdown on SIGINT and SIGTERM” test
to exercise both signals, preferably by parameterizing or iterating over SIGINT
and SIGTERM. For each signal, emit it through proc, then assert the registered
dispose mock runs and exit is called with 0, ensuring both handlers are
meaningfully covered.
- Around line 76-83: In the GracefulShutdown test setup, replace the
EventEmitter-based process cast with a typed mock created as
Pick<NodeJS.Process, "on">. Capture the handlers registered through the mock’s
on method so the listener test can invoke and assert them without relying on an
unsafe cast.
In `@apps/indexer/src/shared/shutdown/graceful-shutdown.ts`:
- Around line 71-72: Remove the deadlineTimer.unref() call in the graceful
shutdown deadline setup so the timeout remains referenced and its rejection path
can run until the shutdown deadline, including invoking exit(1) when required.
- Around line 40-45: The GracefulShutdown constructor currently defaults to
console-based logging; add `@akashnetwork/logging` as a runtime dependency and
replace the default logger in GracefulShutdown with a new LoggerService
instance, while preserving the caller-provided logger when supplied.
---
Nitpick comments:
In `@apps/indexer/src/index.ts`:
- Around line 165-172: Replace the direct console.log and console.error calls in
the server startup and initialization catch block with the repository’s
LoggerService, preserving their existing messages and error context. Update the
logging around app.listen and the catch handling without changing the
surrounding shutdown or Sentry behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4243b5fd-85a9-42a4-b4ff-58272bf742bb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
apps/indexer/package.jsonapps/indexer/src/chain/dataStore.tsapps/indexer/src/chain/nodeAccessor.tsapps/indexer/src/index.tsapps/indexer/src/scheduler.tsapps/indexer/src/shared/shutdown/graceful-shutdown.spec.tsapps/indexer/src/shared/shutdown/graceful-shutdown.tsapps/indexer/vitest.config.ts
The indexer had no signal handling, so container restarts killed the process mid-work: the Postgres pool and LevelDB caches were never closed (risking cache corruption and "connection terminated unexpectedly" errors), background timers kept firing, and in-flight HTTP requests were dropped. Bootstrap the app through `startServer`/`shutdownServer`, ported from the other services but made container agnostic so they work without tsyringe: initializers and teardown are passed in as options instead of resolved from a DI container, and the logger is narrowed to a structural `ServerLogger` the indexer satisfies with `console`. On SIGTERM/SIGINT the HTTP server stops accepting requests first, then acquired resources are disposed in reverse acquisition order (scheduler, node accessor, LevelDB caches, Sequelize pool). Give the scheduler and node accessor stop() methods that clear their timers and drain in-flight work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c908bd0 to
100148a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The periodic save interval discarded its promise, so nothing tracked an in-flight write. On shutdown, stop() cleared the interval and immediately saved again, and waitForAllFinished() only polls node.activeQueries so it returns right away when no RPC queries are outstanding. A tick firing just before SIGTERM meant two concurrent writeFile calls to nodeStatus.json, interleaving into a file that loadNodeStatus() then fails to parse on the next boot. Chain each save onto the previous one inside saveNodeStatus, so the final save in stop() queues behind the periodic one without stop() needing its own tracking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c2549c0 to
b3f3725
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
apps/indexer/src/index.ts (1)
67-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
LoggerServicefor lifecycle messages.Lines 72 and 74 use
console. Lines 183 and 188 also pass and useconsolefor lifecycle logging. Inject the configuredLoggerServiceand pass it tostartServerso shutdown and startup events remain structured.As per coding guidelines, use
LoggerServiceinstead ofconsole.log,console.warn, orconsole.error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/indexer/src/index.ts` around lines 67 - 75, Replace the lifecycle console logging in disposeAcquiredResources and the startup/shutdown paths around startServer with the configured LoggerService. Inject or obtain the existing LoggerService, pass it through startServer, and use its structured logging methods for both successful and failed lifecycle events while preserving Sentry exception capture.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/indexer/src/index.ts`:
- Around line 162-164: Move the database disposal registration in startServer
before awaiting initDatabase(), so the Sequelize pool is closed even when
initialization fails. Keep the existing disposer name and sequelize.close
callback, and leave the block cache registration unchanged.
In `@apps/indexer/src/lib/server-logger/server-logger.ts`:
- Around line 3-10: Update the ServerLogger contract and startServer callers to
use LoggerService for lifecycle logging instead of console-compatible structural
typing. Remove the console-specific documentation and ensure the production
startup path passes the configured LoggerService instance.
In `@apps/indexer/src/lib/shutdown-server/shutdown-server.spec.ts`:
- Around line 11-13: Set server.listening to true in the first three close-path
tests using the mock ServerType setup, ensuring they exercise the branch that
calls server.close; leave the onShutdown callback failure test unchanged.
In `@apps/indexer/src/lib/shutdown-server/shutdown-server.ts`:
- Around line 8-33: Update shutdownServer to accept a shutdown deadline and
forced non-zero exit callback, then race both server.close completion and
onShutdown completion against that deadline. Ensure the deadline invokes the
forced-exit callback when either server closure or cleanup remains pending,
while preserving current error logging and resolving promptly after shutdown
handling completes.
In `@apps/indexer/src/lib/start-server/start-server.spec.ts`:
- Around line 13-16: Update the startServer test cleanup in the afterEach hook
to be asynchronous, await startedServer.close() until its callback completes,
and reset startedServer afterward so each test begins without stale server
state.
In `@apps/indexer/src/lib/start-server/start-server.ts`:
- Around line 65-67: Remove the processEvents "exit" listener that calls
shutdown, while preserving graceful cleanup for "SIGTERM" and "SIGINT" in
start-server.ts. Delete the corresponding exit-handler tests in
apps/indexer/src/lib/start-server/start-server.spec.ts (lines 136-146); no other
shutdown behavior requires changes.
---
Nitpick comments:
In `@apps/indexer/src/index.ts`:
- Around line 67-75: Replace the lifecycle console logging in
disposeAcquiredResources and the startup/shutdown paths around startServer with
the configured LoggerService. Inject or obtain the existing LoggerService, pass
it through startServer, and use its structured logging methods for both
successful and failed lifecycle events while preserving Sentry exception
capture.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb8698e6-7f64-4e99-8135-297ea86fcb2d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
apps/indexer/package.jsonapps/indexer/src/chain/dataStore.tsapps/indexer/src/chain/nodeAccessor.tsapps/indexer/src/index.tsapps/indexer/src/lib/server-logger/server-logger.tsapps/indexer/src/lib/shutdown-server/shutdown-server.spec.tsapps/indexer/src/lib/shutdown-server/shutdown-server.tsapps/indexer/src/lib/start-server/app-initializer.tsapps/indexer/src/lib/start-server/start-server.spec.tsapps/indexer/src/lib/start-server/start-server.tsapps/indexer/src/scheduler.tsapps/indexer/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/indexer/vitest.config.ts
- apps/indexer/src/chain/dataStore.ts
- apps/indexer/src/chain/nodeAccessor.ts
- apps/indexer/src/scheduler.ts
- apps/indexer/package.json
| /** | ||
| * Structural subset of `LoggerService` so server lifecycle helpers stay usable | ||
| * in apps without a logging package wired in — `console` satisfies it as is. | ||
| */ | ||
| export interface ServerLogger { | ||
| info(message: ServerLogMessage): void; | ||
| error(message: ServerLogMessage): void; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use LoggerService for lifecycle logging.
Line 5 documents console as a supported logger. The production startup path passes console to startServer. Change this contract and its callers to pass LoggerService.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/indexer/src/lib/server-logger/server-logger.ts` around lines 3 - 10,
Update the ServerLogger contract and startServer callers to use LoggerService
for lifecycle logging instead of console-compatible structural typing. Remove
the console-specific documentation and ensure the production startup path passes
the configured LoggerService instance.
Source: Coding guidelines
| export async function shutdownServer(server: ServerType, appLogger: ServerLogger, onShutdown?: () => void | Promise<void>): Promise<void> { | ||
| return new Promise(resolve => { | ||
| const shutdown = (error?: unknown) => { | ||
| if (error) { | ||
| appLogger.error({ event: "SERVER_CLOSE_ERROR", error }); | ||
| } | ||
|
|
||
| Promise.resolve(onShutdown?.()) | ||
| .catch(error => { | ||
| appLogger.error({ event: "ON_SHUTDOWN_ERROR", error }); | ||
| }) | ||
| .finally(() => { | ||
| resolve(); | ||
| }); | ||
| }; | ||
|
|
||
| try { | ||
| if (server.listening) { | ||
| server.close(shutdown); | ||
| } else { | ||
| shutdown(); | ||
| } | ||
| } catch (error) { | ||
| shutdown(error); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Enforce the shutdown deadline.
server.close can wait for open connections, and onShutdown can remain pending. This path has no deadline or forced non-zero exit. A stuck request or cleanup task can therefore block container termination indefinitely.
Pass a deadline and force-exit callback through this lifecycle path. Race server closure and cleanup against that deadline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/indexer/src/lib/shutdown-server/shutdown-server.ts` around lines 8 - 33,
Update shutdownServer to accept a shutdown deadline and forced non-zero exit
callback, then race both server.close completion and onShutdown completion
against that deadline. Ensure the deadline invokes the forced-exit callback when
either server closure or cleanup remains pending, while preserving current error
logging and resolving promptly after shutdown handling completes.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/indexer/src/chain/nodeAccessor.ts (2)
79-79: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle periodic save failures.
The interval ignores the promise from
saveNodeStatus(). IfwriteNodeStatusFile()fails, the rejected promise can become unhandled and terminate the process or omit the failure from logs. Catch the promise in the interval callback and report the error throughLoggerService.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/indexer/src/chain/nodeAccessor.ts` at line 79, Update the interval callback assigned to saveStatusInterval so it handles the promise returned by saveNodeStatus(), catches writeNodeStatusFile() failures, and reports the error through LoggerService instead of allowing an unhandled rejection.Source: Coding guidelines
82-90: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftStop
NodeInfostatus timers and status fetches before the final save.
stop()clears onlysaveStatusInterval.NodeInfo.loadFromSavedNodeInfo()creates status timers, andNodeInfo.updateStatus()is not tracked inactiveQueries. ThereforewaitForAllFinished()can resolve while status timers or status fetches remain active. Those tasks can keep the process alive until the shutdown deadline and can modify status after the final save.Add a
NodeInfo.stop()lifecycle method that clears status timers and waits for active status updates. Call it before the finalsaveNodeStatus().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/indexer/src/chain/nodeAccessor.ts` around lines 82 - 90, Update NodeAccessor.stop() to stop every NodeInfo before the final save, and add a NodeInfo.stop() lifecycle method that clears its status timers and awaits any in-flight updateStatus() work. Invoke NodeInfo.stop() before saveNodeStatus(), while preserving the existing saveStatusInterval cleanup and waitForAllFinished() behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/indexer/src/chain/nodeAccessor.ts`:
- Line 79: Update the interval callback assigned to saveStatusInterval so it
handles the promise returned by saveNodeStatus(), catches writeNodeStatusFile()
failures, and reports the error through LoggerService instead of allowing an
unhandled rejection.
- Around line 82-90: Update NodeAccessor.stop() to stop every NodeInfo before
the final save, and add a NodeInfo.stop() lifecycle method that clears its
status timers and awaits any in-flight updateStatus() work. Invoke
NodeInfo.stop() before saveNodeStatus(), while preserving the existing
saveStatusInterval cleanup and waitForAllFinished() behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7422c6aa-b0e9-43f1-abf3-f32db17eab24
📒 Files selected for processing (2)
apps/indexer/src/chain/dataStore.tsapps/indexer/src/chain/nodeAccessor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/indexer/src/chain/dataStore.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
apps/indexer/src/chain/nodeAccessor.ts:67-77— stop() can race with an in-flight periodic saveNodeStatus() write: clearInterval() only stops future ticks, and waitForAllFinished() only waits on node queries (not file writes), so if a 30s periodic save is mid-write when stop() runs, its own final saveNodeStatus() call starts a second concurrent fs.promises.writeFile to the same nodeStatus.json path. Concurrent writes to the same file are unordered in Node and could corrupt the file, which would then throw on the next startup since loadNodeStatus() does JSON.parse with no try/catch.Extended reasoning...
NodeAccessor.stop()(apps/indexer/src/chain/nodeAccessor.ts:67-77) is new code added by this PR to support graceful shutdown. It clears the periodic save interval, waits for in-flight node queries to finish, and then performs one finalsaveNodeStatus()call to persist state before the process exits:public async stop(): Promise<void> { if (this.saveStatusInterval) { clearInterval(this.saveStatusInterval); this.saveStatusInterval = null; } await this.waitForAllFinished(); await this.saveNodeStatus(); }
The problem is that
clearIntervalonly prevents future invocations of the interval callback — it does nothing to a callback that has already fired and is currently executing.saveNodeStatus()does an un-guardedawait fs.promises.writeFile(savedNodeInfoPath, ...), so if the 30-second periodic tick fires just beforestop()runs, that write can still be in flight whenstop()reaches its ownsaveNodeStatus()call at line 77.waitForAllFinished()does not protect against this — it only pollsnode.activeQueriesfor each node, which tracks outstanding RPC queries, not file I/O. It has no relationship tosaveNodeStatus()or the pendingwriteFile. In fact, during real shutdown there typically are no active queries left by the timestop()runs (the scheduler and HTTP server are already being torn down), sowaitForAllFinished()returns almost immediately with no delay that would incidentally let the in-flight write finish first.The result: two concurrent
fs.promises.writeFilecalls can targetnodeStatus.jsonat the same time — one from the periodic tick, one fromstop(). Node explicitly documents that concurrent writes to the same file are unsafe and unordered; the two writes are not guaranteed to be byte-identical (node success/error counts and timestamps can differ between the tick-time snapshot and the post-drain snapshot), so interleaving can leave a truncated or malformed JSON file. SinceloadNodeStatus()doesJSON.parse(file)with no try/catch, a corrupted file would throw on the very next process startup, causinginitApp()to fail — directly undermining the goal of this PR, which is a clean, non-disruptive restart path.Step-by-step reproduction:
- The periodic
setInterval(() => this.saveNodeStatus(), 30_000)fires and beginsawait fs.promises.writeFile(savedNodeInfoPath, largeJsonA). - Milliseconds later, SIGTERM arrives and the shutdown sequence calls
nodeAccessor.stop(). stop()callsclearInterval(...), which only cancels the next scheduled tick — it does not, and cannot, cancel the write already in progress.waitForAllFinished()checksnode.activeQueriesfor all nodes; since there are no in-flight RPC queries at that moment, it returns immediately without waiting for the pending write.stop()callssaveNodeStatus()again, starting a secondfs.promises.writeFile(savedNodeInfoPath, jsonB)to the same path while the first write from step 1 is still being flushed to disk.- The two writes interleave at the OS/filesystem level; the file on disk ends up as neither
jsonAnorjsonBbut some mixture (e.g., trailing bytes from the longer write), which is not valid JSON. - On the next process start,
loadNodeStatus()reads this file and callsJSON.parse(file), which throws, andinitApp()fails — the indexer cannot start until the corrupt file is manually removed.
The fix is small: track the in-flight save as a promise (e.g.,
private pendingSave: Promise<void> | null) and havesaveNodeStatus()reuse/await it, or havestop()await any in-flight save before issuing the final one, so all writes tonodeStatus.jsonare serialized.This is a narrow race — the window is only the duration of the
writeFilecall (typically low milliseconds) against a 30-second interval — so it will rarely trigger in practice. Given the low probability but real mechanism and the fact that it lives entirely in the shutdown-safety code this PR introduces, this is worth flagging as a nit rather than a blocking issue. - The periodic
There was a problem hiding this comment.
Two nits were found in this run (Sequelize disposer registered too late on init failure, and signal handlers registered too late during startup) — both are edge-case gaps in an otherwise solid graceful-shutdown implementation, but given this PR changes the indexer's startup/shutdown lifecycle I'll leave this for a human to weigh in on before merging.
Extended reasoning...
Overview
This PR adds a GracefulShutdown-style lifecycle to apps/indexer: a generic startServer/shutdownServer pair, an AppInitializer contract, disposer tracking in index.ts (Sequelize pool, LevelDB caches, node accessor, scheduler), and stop()" methods on SchedulerandNodeAccessorthat clear timers and drain in-flight work. It also serializesNodeAccessorfile writes to prevent corruptednodeStatus.json`, and adds a first-ever Vitest setup (with good coverage) for the indexer app.
Security risks
None identified. No auth, crypto, or user-facing input handling is touched — this is purely process lifecycle management (signal handling, resource disposal, timers).
Level of scrutiny
This deserves a solid look rather than a rubber stamp: it changes how a production service starts up and shuts down, replacing a previous no-op process model with signal handling, and a bug here manifests only under specific timing conditions (a signal or init failure arriving at exactly the wrong moment) that are hard to catch via normal testing/staging. The bug hunting pass already surfaced two real ordering gaps (disposer registered after a failure-prone await, and signal handlers registered only after a potentially long-running beforeStart), both scoped as nits since they only affect the init-failure / mid-startup-signal windows rather than steady-state operation, but they do run against the PR's own stated goal of preventing corruption/leaks on restart.
Other factors
The PR includes solid new unit test coverage for the added start-server/shutdown-server modules, and CodeRabbit already raised closely related points (deadline enforcement, disposer ordering, sync-only exit listener semantics) that overlap with what was found this run. Given the number of open review threads and the criticality of getting shutdown ordering right for this service, a human sign-off is warranted here rather than an autonomous approval.
Address PR review findings: - register SIGTERM/SIGINT handlers before `beforeStart` so a signal during a long startup (e.g. RebuildAll) still disposes acquired resources, and abort serving when shutdown was requested mid-startup - drop the `exit` listener: Node terminates as soon as such a listener returns, so the awaited shutdown chain could never complete through that path - race shutdown against a bounded deadline and force-exit with a non-zero code when server close or cleanup stays pending - register the database and block cache disposers before `initDatabase()` so a failure in a later init step still closes the pool and LevelDB caches - pass `LoggerService` instead of `console` as the server lifecycle logger - make the mocked server `listening` explicit and await server close in the test teardown Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/indexer/src/lib/start-server/start-server.ts`:
- Around line 48-75: Coordinate the startup and shutdown flows in the
start-server lifecycle so shutdown waits for beforeStart and all ON_APP_START
hooks that have begun to settle before invoking stopAppOnce. Track the startup
promise, check isShuttingDown before starting subsequent lifecycle work and
before launching each initializer, and ensure concurrent initializer rejection
still waits for other started hooks; add regression coverage for a signal during
a slow initializer and for concurrent startup failure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10b9bf2e-e466-4f7b-913f-3a2708732460
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
apps/indexer/package.jsonapps/indexer/src/index.tsapps/indexer/src/lib/server-logger/server-logger.tsapps/indexer/src/lib/shutdown-server/shutdown-server.spec.tsapps/indexer/src/lib/start-server/start-server.spec.tsapps/indexer/src/lib/start-server/start-server.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/indexer/src/lib/shutdown-server/shutdown-server.spec.ts
- apps/indexer/src/lib/server-logger/server-logger.ts
- apps/indexer/package.json
- apps/indexer/src/index.ts
| let server: ServerType | undefined; | ||
| let isShuttingDown = false; | ||
| const shutdown = once(async (reason: string) => { | ||
| isShuttingDown = true; | ||
| logger.info({ event: "APP_SERVER_SHUTDOWN_REQUESTED", reason }); | ||
| const forceExitTimer = setTimeout(() => { | ||
| logger.error({ event: "APP_SHUTDOWN_TIMEOUT", reason, shutdownTimeoutMs }); | ||
| forceExit(); | ||
| }, shutdownTimeoutMs); | ||
| forceExitTimer.unref(); | ||
|
|
||
| try { | ||
| if (server) { | ||
| await shutdownServer(server, logger, stopAppOnce); | ||
| } else { | ||
| await stopAppOnce(); | ||
| } | ||
| } finally { | ||
| clearTimeout(forceExitTimer); | ||
| } | ||
| }); | ||
|
|
||
| processEvents.on("SIGTERM", () => shutdown("SIGTERM")); | ||
| processEvents.on("SIGINT", () => shutdown("SIGINT")); | ||
|
|
||
| try { | ||
| await options.beforeStart?.(); | ||
| await Promise.all(initializers.map(initializer => initializer[ON_APP_START]())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Wait for startup hooks before application cleanup.
If a signal arrives while beforeStart or an ON_APP_START hook is pending, shutdown calls stopAppOnce immediately. Pending startup work then continues. In particular, initializers can start after beforeStart resolves even when shutdown already started.
Because stopAppOnce uses once, an initializer that finishes after its stop hook ran will not receive another stop call. The same race occurs when one Promise.all startup hook rejects while another hook is still pending.
Coordinate shutdown with all started startup hooks settling before stopAppOnce runs. Check isShuttingDown before starting subsequent lifecycle work. Add regression coverage for a slow initializer with a signal and for concurrent initializer failure.
Also applies to: 97-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/indexer/src/lib/start-server/start-server.ts` around lines 48 - 75,
Coordinate the startup and shutdown flows in the start-server lifecycle so
shutdown waits for beforeStart and all ON_APP_START hooks that have begun to
settle before invoking stopAppOnce. Track the startup promise, check
isShuttingDown before starting subsequent lifecycle work and before launching
each initializer, and ensure concurrent initializer rejection still waits for
other started hooks; add regression coverage for a signal during a slow
initializer and for concurrent startup failure.
| forceExit(); | ||
| }, shutdownTimeoutMs); | ||
| forceExitTimer.unref(); | ||
|
|
||
| try { | ||
| if (server) { | ||
| await shutdownServer(server, logger, stopAppOnce); | ||
| } else { | ||
| await stopAppOnce(); | ||
| } | ||
| } finally { | ||
| clearTimeout(forceExitTimer); | ||
| } | ||
| }); | ||
|
|
||
| processEvents.on("SIGTERM", () => shutdown("SIGTERM")); | ||
| processEvents.on("SIGINT", () => shutdown("SIGINT")); | ||
|
|
||
| try { | ||
| await options.beforeStart?.(); | ||
| await Promise.all(initializers.map(initializer => initializer[ON_APP_START]())); | ||
|
|
||
| if (isShuttingDown) { | ||
| logger.info({ event: "SERVER_START_ABORTED" }); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🔴 SIGTERM/SIGINT handlers are registered before await options.beforeStart?.() (start-server.ts), and shutdown() never awaits or cancels the still-running beforeStart promise before disposing resources — when a signal lands mid-startup, server is still undefined so stopAppOnce() runs disposeAcquiredResources() (closing Sequelize and the LevelDB caches) immediately, even though initApp() may still be mid-syncBlocks() (ExecutionMode.RebuildAll, which can run for hours) actively writing to those same stores. This directly violates the reverse-order-disposal invariant the PR itself documents ("producers stop before the stores they write to are closed") for the rebuild/maintenance code paths, and reproduces the exact 'connection terminated unexpectedly' / LevelDB-not-open failure class this PR sets out to eliminate. Consider gating long-running beforeStart work on an isShuttingDown check, or having shutdown() await/cancel the in-flight beforeStart promise before disposing resources.
Extended reasoning...
The bug. startServer() in apps/indexer/src/lib/start-server/start-server.ts registers the SIGTERM/SIGINT handlers before awaiting options.beforeStart?.():
processEvents.on("SIGTERM", () => shutdown("SIGTERM"));
processEvents.on("SIGINT", () => shutdown("SIGINT"));
try {
await options.beforeStart?.();
await Promise.all(initializers.map(initializer => initializer[ON_APP_START]()));
if (isShuttingDown) { ... return undefined; }
...shutdown() sets isShuttingDown = true and then does:
if (server) {
await shutdownServer(server, logger, stopAppOnce);
} else {
await stopAppOnce();
}Since server is only assigned after beforeStart resolves, a signal that arrives during beforeStart takes the else branch and calls stopAppOnce() directly — which runs options.onStop?.() (disposeAcquiredResources() in index.ts), closing sequelize and the LevelDB caches (closeCaches()) via disposeAcquiredResources. Nothing in this path awaits, cancels, or otherwise coordinates with the beforeStart promise that is still executing concurrently on the event loop. The isShuttingDown flag is only consulted after beforeStart resolves (to skip starting the HTTP server) — it does nothing to stop beforeStart's in-flight work before disposal runs.
The triggering code path. In index.ts, beforeStart is wired to initApp(). For ExecutionMode.RebuildAll, initApp() does await syncBlocks(), which the codebase itself notes can run "for hours," continuously writing to sequelize (via ORM writes) and to blocksDb/blockResultsDb (LevelDB, via dataStore.ts). If a SIGTERM/SIGINT lands anywhere in that window, disposeAcquiredResources() closes the Sequelize pool and the LevelDB handles while syncBlocks() is still issuing queries and .put() calls against them.
Why nothing prevents this today. The PR's own disposal ordering — disposeAcquiredResources() iterates resources in reverse acquisition order specifically "so producers stop before the stores they write to are closed" — assumes every producer is a registered resource with a cooperative stop() (as the Scheduler and NodeAccessor now are). syncBlocks() inside beforeStart is not: it's a bare awaited call with no AbortController threaded through it and no isShuttingDown check inside its loop. So the invariant the PR establishes holds for SyncOnly (where syncBlocks runs via the scheduler, whose stop() drains runningPromise), but not for the rebuild modes where the producer is unregistered and un-cancellable.
Impact. Concurrently closing the Postgres pool while syncBlocks() is mid-query produces the exact "connection terminated unexpectedly" class of error the PR's description calls out as the motivating problem. Concurrently closing the LevelDB handles while writes are in flight throws "Database is not open" from the pending .put() calls. This is a real regression against the PR's stated goal, and it's not a contrived scenario — Kubernetes rolling restarts / redeploys can send SIGTERM at any point in a pod's lifecycle, including mid-rebuild, which is precisely the multi-hour operation this PR was written to protect.
Step-by-step proof.
- Operator starts the indexer with
ACTIVE_CHAINexecution modeRebuildAll. startServer()registersSIGTERM/SIGINThandlers, then callsawait options.beforeStart?.()→initApp().initApp()completesinitDatabase()(registering thedatabaseandblock cachedisposers) and entersawait syncBlocks(), which runs for an extended period, actively queryingsequelizeand writing toblocksDb/blockResultsDb.- A SIGTERM arrives (e.g. a k8s rolling deploy or manual restart).
shutdown("SIGTERM")runs;serveris stillundefined(the HTTP server hasn't been created yet), so theelsebranch callsawait stopAppOnce()directly.stopAppOnce()runsoptions.onStop()=disposeAcquiredResources(), which closessequelizeand the LevelDB caches — whilesyncBlocks()(still running from step 3) is mid-query/mid-write against those same handles.- The interleaved writes/queries against now-closing/closed resources surface as failed queries and LevelDB "not open" errors — the exact failure modes the PR was written to eliminate.
Existing tests don't catch this. The "stops app without starting the server when a signal arrives while starting up" test in start-server.spec.ts uses beforeStart: () => delay(50), an inert delay with no resource usage, so it can't observe the resource-disposal race.
Fix direction. Either have syncBlocks()/initApp() cooperatively check an isShuttingDown signal (or an AbortSignal) passed down from startServer, so long rebuild work exits promptly on shutdown before disposal runs; or have shutdown() await the in-flight beforeStart promise (with a bounded timeout, similar to the existing shutdown deadline) before invoking stopAppOnce(), so disposal doesn't start until the producer has actually stopped touching the resources.
| export async function closeCaches(): Promise<void> { | ||
| await Promise.allSettled([blocksDb.close(), blockResultsDb.close()]); | ||
| } |
There was a problem hiding this comment.
🟡 closeCaches() (dataStore.ts:47-49) uses Promise.allSettled but never inspects the settled results, so it always resolves successfully even if blocksDb.close() or blockResultsDb.close() actually rejects. Since it's registered as the 'block cache' disposer, this silently defeats disposeAcquiredResources()'s try/catch, which relies on a thrown error to log RESOURCE_DISPOSE_ERROR and report to Sentry — unlike the sequelize/nodeAccessor/scheduler disposers, a failed cache close here goes completely unnoticed. Suggest inspecting the settled results and re-throwing if either close rejected.
Extended reasoning...
The bug: closeCaches() in apps/indexer/src/chain/dataStore.ts:47-49 is:
export async function closeCaches(): Promise<void> {
await Promise.allSettled([blocksDb.close(), blockResultsDb.close()]);
}Promise.allSettled never rejects — it resolves with an array of PromiseSettledResult objects regardless of whether the underlying promises fulfilled or rejected. Because the code awaits it directly and returns, closeCaches() always resolves successfully, even when blocksDb.close() or blockResultsDb.close() genuinely throws (e.g. an I/O error, or the LevelDB handle already being in a bad state).
The code path that triggers it: closeCaches is registered in index.ts as disposeOnShutdown({ name: "block cache", dispose: () => closeCaches() }). On SIGTERM/SIGINT, disposeAcquiredResources() iterates the acquired resources in reverse order and wraps each dispose() call in a try/catch specifically so it can log RESOURCE_DISPOSE_ERROR and call Sentry.captureException(error) on failure, then logs RESOURCE_DISPOSED on success. Because closeCaches() swallows its own rejection via allSettled, that try/catch can never observe a LevelDB close failure — it will always log RESOURCE_DISPOSED for "block cache" even when one or both closes actually failed.
Why existing code doesn't prevent it: Every sibling disposer in this same list propagates its own errors: sequelize.close() rejects directly, nodeAccessor.stop() awaits saveNodeStatus()/writeNodeStatusFile() (a fs.promises.writeFile that can reject), and scheduler.stop()'s Promise.allSettled over running tasks is a deliberate "attempt both, don't fail the disposer on a task's own error" design — but closeCaches uses the same pattern for the close calls themselves, which is exactly the operation whose success the disposer contract needs to observe. There's nothing else downstream that inspects the settled array.
Step-by-step proof:
- Operator sends SIGTERM;
disposeAcquiredResources()reaches the "block cache" entry and callsresource.dispose(), i.e.closeCaches(). - Inside
closeCaches(), supposeblocksDb.close()rejects (e.g. underlying LevelDB reports an I/O error while flushing). Promise.allSettled([...])still resolves (fulfilled), with an element{ status: "rejected", reason: <the I/O error> }in its result array — whichcloseCaches()never reads.await Promise.allSettled(...)completes without throwing;closeCaches()returns normally.- Back in
disposeAcquiredResources(), thetryblock aroundawait resource.dispose()sees no exception, so it logslogger.info({ event: "RESOURCE_DISPOSED", resource: "block cache" })— a false-positive success log — andSentry.captureExceptionis never called for a real close failure.
Impact: This is real but scoped to observability during a process that's already shutting down. It does not block shutdown progress (the loop would have continued past a throwing disposer anyway, since each iteration is individually try/caught) and doesn't cause data loss — LevelDB is crash-safe via its WAL/MANIFEST, so a failed close doesn't itself corrupt data. Its only effect is that a genuine LevelDB close failure goes completely unlogged and unreported to Sentry, which cuts against this PR's explicit goal of surfacing cache-shutdown problems to avoid "risking cache corruption" on unclean shutdown.
Fix: Inspect the settled results and re-throw if either rejected, e.g.:
export async function closeCaches(): Promise<void> {
const results = await Promise.allSettled([blocksDb.close(), blockResultsDb.close()]);
const rejected = results.find(r => r.status === "rejected");
if (rejected) throw rejected.reason;
}This still lets both close() calls run to completion (avoiding Promise.all's short-circuit, which would only be a minor difference here) while restoring the same throw-on-failure guarantee the other disposers provide.
2 participants
Why
Closes CON-269
What
The indexer had no signal handling, so container restarts killed the process mid-work: the Postgres pool and LevelDB caches were never closed (risking cache corruption and "connection terminated unexpectedly" errors), background timers kept firing, and in-flight HTTP requests were dropped.
Add a GracefulShutdown manager that, on SIGTERM/SIGINT, tears down registered resources in reverse acquisition order (HTTP server first, database pool last) within a bounded deadline, force-exiting with a non-zero code if the deadline is exceeded. Wire the HTTP server, scheduler, node accessor, LevelDB caches, and Sequelize pool into it, and give the scheduler and node accessor stop() methods that clear their timers and drain in-flight work.
Summary by CodeRabbit