From 7335e52ab31a6fd72a453dc2c73925a9279ce08d Mon Sep 17 00:00:00 2001 From: Aiden McClelland Date: Fri, 20 Mar 2026 13:35:24 -0600 Subject: [PATCH] fix: daemon lifecycle cleanup and error logging improvements - Refactor HealthDaemon to use a tracked session (AbortController + awaitable promise) instead of fire-and-forget health check loops, preventing health checks from running after a service is stopped - Stop health checks before terminating daemon to avoid false crash reports during intentional shutdown - Guard onExit callbacks with AbortSignal to prevent stale session callbacks - Add logErrorOnce utility to deduplicate repeated error logging - Fix SystemForEmbassy.stop() to capture clean promise before deleting ref - Treat SIGTERM (signal 15) as successful exit in subcontainer sync - Fix asError to return original Error instead of wrapping in new Error - Remove unused ExtendedVersion import from Backups.ts --- container-runtime/package-lock.json | 2 +- .../Systems/SystemForEmbassy/index.ts | 15 ++-- core/src/service/effects/subcontainer/sync.rs | 2 +- sdk/base/lib/util/asError.ts | 2 +- sdk/base/lib/util/index.ts | 1 + sdk/base/lib/util/logErrorOnce.ts | 9 +++ sdk/package/lib/backup/Backups.ts | 1 - sdk/package/lib/mainFn/CommandController.ts | 3 +- sdk/package/lib/mainFn/Daemon.ts | 11 +-- sdk/package/lib/mainFn/HealthDaemon.ts | 79 +++++++++++-------- 10 files changed, 73 insertions(+), 52 deletions(-) create mode 100644 sdk/base/lib/util/logErrorOnce.ts diff --git a/container-runtime/package-lock.json b/container-runtime/package-lock.json index e47b4d89a..31478b8f4 100644 --- a/container-runtime/package-lock.json +++ b/container-runtime/package-lock.json @@ -37,7 +37,7 @@ }, "../sdk/dist": { "name": "@start9labs/start-sdk", - "version": "0.4.0-beta.61", + "version": "0.4.0-beta.62", "license": "MIT", "dependencies": { "@iarna/toml": "^3.0.0", diff --git a/container-runtime/src/Adapters/Systems/SystemForEmbassy/index.ts b/container-runtime/src/Adapters/Systems/SystemForEmbassy/index.ts index 10b1d7ddc..1159efccc 100644 --- a/container-runtime/src/Adapters/Systems/SystemForEmbassy/index.ts +++ b/container-runtime/src/Adapters/Systems/SystemForEmbassy/index.ts @@ -445,15 +445,14 @@ export class SystemForEmbassy implements System { } callCallback(_callback: number, _args: any[]): void {} async stop(): Promise { - const { currentRunning } = this - this.currentRunning?.clean() + const clean = this.currentRunning?.clean({ + timeout: fromDuration( + (this.manifest.main["sigterm-timeout"] as any) || "30s", + ), + }) delete this.currentRunning - if (currentRunning) { - await currentRunning.clean({ - timeout: fromDuration( - (this.manifest.main["sigterm-timeout"] as any) || "30s", - ), - }) + if (clean) { + await clean } } diff --git a/core/src/service/effects/subcontainer/sync.rs b/core/src/service/effects/subcontainer/sync.rs index 3999a0cd9..e37207ced 100644 --- a/core/src/service/effects/subcontainer/sync.rs +++ b/core/src/service/effects/subcontainer/sync.rs @@ -768,7 +768,7 @@ pub fn exec( stderr_thread.map(|t| t.join().unwrap()); if let Some(code) = exit.code() { std::process::exit(code); - } else if exit.success() { + } else if exit.success() || exit.signal() == Some(15) { Ok(()) } else { Err(Error::new( diff --git a/sdk/base/lib/util/asError.ts b/sdk/base/lib/util/asError.ts index dddb4aafe..a9a58dfcb 100644 --- a/sdk/base/lib/util/asError.ts +++ b/sdk/base/lib/util/asError.ts @@ -8,7 +8,7 @@ */ export const asError = (e: unknown) => { if (e instanceof Error) { - return new Error(e as any) + return e } if (typeof e === 'string') { return new Error(`${e}`) diff --git a/sdk/base/lib/util/index.ts b/sdk/base/lib/util/index.ts index 22cf037ba..e807b2280 100644 --- a/sdk/base/lib/util/index.ts +++ b/sdk/base/lib/util/index.ts @@ -32,3 +32,4 @@ export { deepEqual } from './deepEqual' export { AbortedError } from './AbortedError' export * as regexes from './regexes' export { stringFromStdErrOut } from './stringFromStdErrOut' +export { logErrorOnce } from './logErrorOnce' diff --git a/sdk/base/lib/util/logErrorOnce.ts b/sdk/base/lib/util/logErrorOnce.ts new file mode 100644 index 000000000..356c8d42a --- /dev/null +++ b/sdk/base/lib/util/logErrorOnce.ts @@ -0,0 +1,9 @@ +const loggedErrors = new WeakSet() + +export function logErrorOnce(err: unknown) { + if (typeof err === 'object' && err !== null) { + if (loggedErrors.has(err)) return + loggedErrors.add(err) + } + console.error(err) +} diff --git a/sdk/package/lib/backup/Backups.ts b/sdk/package/lib/backup/Backups.ts index 05efb2554..836844d4e 100644 --- a/sdk/package/lib/backup/Backups.ts +++ b/sdk/package/lib/backup/Backups.ts @@ -2,7 +2,6 @@ import * as T from '../../../base/lib/types' import * as child_process from 'child_process' import * as fs from 'fs/promises' import { Affine, asError } from '../util' -import { ExtendedVersion, VersionRange } from '../../../base/lib' import { InitKind, InitScript } from '../../../base/lib/inits' /** Default rsync options used for backup and restore operations */ diff --git a/sdk/package/lib/mainFn/CommandController.ts b/sdk/package/lib/mainFn/CommandController.ts index d8f290aa3..95c2315c7 100644 --- a/sdk/package/lib/mainFn/CommandController.ts +++ b/sdk/package/lib/mainFn/CommandController.ts @@ -7,6 +7,7 @@ import { Drop, splitCommand } from '../util' import * as cp from 'child_process' import * as fs from 'node:fs/promises' import { DaemonCommandType, ExecCommandOptions, ExecFnOptions } from './Daemons' +import { logErrorOnce } from '../../../base/lib/util/logErrorOnce' /** * Low-level controller for a single running process inside a subcontainer (or as a JS function). @@ -220,6 +221,6 @@ export class CommandController< } } onDrop(): void { - this.term().catch(console.error) + this.term().catch(logErrorOnce) } } diff --git a/sdk/package/lib/mainFn/Daemon.ts b/sdk/package/lib/mainFn/Daemon.ts index fcbf1c9cb..5ce08ce85 100644 --- a/sdk/package/lib/mainFn/Daemon.ts +++ b/sdk/package/lib/mainFn/Daemon.ts @@ -1,5 +1,6 @@ import * as T from '../../../base/lib/types' import { asError } from '../../../base/lib/util/asError' +import { logErrorOnce } from '../../../base/lib/util/logErrorOnce' import { Drop } from '../util' import { SubContainer, @@ -64,7 +65,7 @@ export class Daemon< ) const res = new Daemon(subc, startCommand) effects.onLeaveContext(() => { - res.term({ destroySubcontainer: true }).catch((e) => console.error(e)) + res.term({ destroySubcontainer: true }).catch((e) => logErrorOnce(e)) }) return res } @@ -86,7 +87,7 @@ export class Daemon< if (this.commandController) await this.commandController .term({}) - .catch((err) => console.error(err)) + .catch((err) => logErrorOnce(err)) try { this.commandController = await this.startCommand() if (!this.shouldBeRunning) { @@ -97,7 +98,7 @@ export class Daemon< const success = await this.commandController.wait().then( (_) => true, (err) => { - console.error(err) + if (this.shouldBeRunning) logErrorOnce(err) return false }, ) @@ -147,7 +148,7 @@ export class Daemon< this.onExitFns = [] } if (this.exiting) { - await this.exiting.catch(console.error) + await this.exiting.catch(logErrorOnce) if (termOptions?.destroySubcontainer) { await this.subcontainer?.destroy() } @@ -172,6 +173,6 @@ export class Daemon< this.onExitFns.push(fn) } onDrop(): void { - this.term().catch((e) => console.error(asError(e))) + this.term().catch((e) => logErrorOnce(asError(e))) } } diff --git a/sdk/package/lib/mainFn/HealthDaemon.ts b/sdk/package/lib/mainFn/HealthDaemon.ts index e8c194e94..ad3050af5 100644 --- a/sdk/package/lib/mainFn/HealthDaemon.ts +++ b/sdk/package/lib/mainFn/HealthDaemon.ts @@ -4,14 +4,6 @@ import { Ready } from './Daemons' import { Daemon } from './Daemon' import { SetHealth, Effects, SDKManifest } from '../../../base/lib/types' -const oncePromise = () => { - let resolve: (value: T) => void - const promise = new Promise((res) => { - resolve = res - }) - return { resolve: resolve!, promise } -} - export const EXIT_SUCCESS = 'EXIT_SUCCESS' as const /** @@ -29,6 +21,7 @@ export class HealthDaemon { private resolveReady: (() => void) | undefined private resolvedReady: boolean = false private readyPromise: Promise + private session: { abort: AbortController; done: Promise } | null = null constructor( readonly daemon: Daemon | null, readonly dependencies: HealthDaemon[], @@ -54,7 +47,7 @@ export class HealthDaemon { }) { this.healthWatchers = [] this.running = false - this.healthCheckCleanup?.() + await this.stopSession() await this.daemon?.term({ ...termOptions, @@ -77,20 +70,25 @@ export class HealthDaemon { if (newStatus) { console.debug(`Launching ${this.id}...`) - this.setupHealthCheck() + this.startSession() this.daemon?.start() this.started = performance.now() } else { console.debug(`Stopping ${this.id}...`) - this.daemon?.term() - await this.turnOffHealthCheck() + await this.stopSession() + await this.daemon?.term() } } - private healthCheckCleanup: (() => Promise) | null = null - private async turnOffHealthCheck() { - await this.healthCheckCleanup?.() + private async stopSession() { + if (!this.session) return + this.session.abort.abort() + await this.session.done + this.session = null + this.resetReady() + } + private resetReady() { this.resolvedReady = false this.readyPromise = new Promise( (resolve) => @@ -100,8 +98,14 @@ export class HealthDaemon { }), ) } - private async setupHealthCheck() { + + private startSession() { + this.session?.abort.abort() + + const abort = new AbortController() + this.daemon?.onExit((success) => { + if (abort.signal.aborted) return if (success && this.ready === 'EXIT_SUCCESS') { this.setHealth({ result: 'success', message: null }) } else if (!success) { @@ -116,42 +120,49 @@ export class HealthDaemon { }) } }) + + const done = + this.ready === 'EXIT_SUCCESS' + ? Promise.resolve() + : this.runHealthCheckLoop(abort.signal) + + this.session = { abort, done } + } + + private async runHealthCheckLoop(signal: AbortSignal): Promise { if (this.ready === 'EXIT_SUCCESS') return - if (this.healthCheckCleanup) return const trigger = (this.ready.trigger ?? defaultTrigger)(() => ({ lastResult: this._health.result, })) - const { promise: status, resolve: setStatus } = oncePromise<{ - done: true - }>() - const { promise: exited, resolve: setExited } = oncePromise() - new Promise(async () => { - if (this.ready === 'EXIT_SUCCESS') return + const aborted = new Promise<{ done: true }>((resolve) => + signal.addEventListener('abort', () => resolve({ done: true }), { + once: true, + }), + ) + + try { for ( - let res = await Promise.race([status, trigger.next()]); + let res = await Promise.race([aborted, trigger.next()]); !res.done; - res = await Promise.race([status, trigger.next()]) + res = await Promise.race([aborted, trigger.next()]) ) { const response: HealthCheckResult = await Promise.resolve( this.ready.fn(), ).catch((err) => { return { - result: 'failure', + result: 'failure' as const, message: 'message' in err ? err.message : String(err), } }) + if (signal.aborted) break await this.setHealth(response) } - setExited(null) - }).catch((err) => console.error(`Daemon ${this.id} failed: ${err}`)) - - this.healthCheckCleanup = async () => { - setStatus({ done: true }) - await exited - this.healthCheckCleanup = null - return null + } catch (err) { + if (!signal.aborted) { + console.error(`Daemon ${this.id} health check failed: ${err}`) + } } }