Add eagerStartupCheck to keep the backend health check off the readiness path - #7
Add eagerStartupCheck to keep the backend health check off the readiness path#7raphaeldelio wants to merge 5 commits into
Conversation
…d health check The redis-memory service start awaited a cloud health check plus summary-view ensures before returning. Hosts that gate gateway readiness on service start (e.g. warm pod pools) pay those network round-trips on every assignment - measured at ~270ms of readiness latency per warm-pool assignment. Failures never blocked startup (they only log a warning), so awaiting them buys no additional safety. eagerStartupCheck (default true) preserves the current behavior; when false the same verification runs in the background and failures still surface as log warnings and per-call tool errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The gateway validates plugin config against the manifest schema (additionalProperties: false) before the plugin loads, so a key accepted by parseMemoryConfig but absent from the manifest fails gateway startup. Caught by the warm-pool functional pass. A new parity test now pins the manifest configSchema to ALLOWED_CONFIG_KEYS so the two cannot drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three follow-ups on eagerStartupCheck found while reviewing it for release: - stop() now waits for an in-flight background verification (bounded at 5s, mirroring the capture drain). Previously the promise was untracked, so an ensureView could write during teardown and its log line could land after the "stopped" message - a realistic window in warm pools, where release follows assignment within seconds. - The background call carries a .catch(): verifyBackend swallows provider errors, but a throw from the logger inside its own catch block would have escaped as an unhandled rejection, fatal under Node's default --unhandled-rejections=throw. - Document eagerStartupCheck in the README config table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The config table entry alone was not enough. Two README sections tell the reader to look for the "connected to server" line as the gateway starts: the verification smoke test and the troubleshooting list. With eagerStartupCheck disabled that line arrives after startup instead, so both now say so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| const stopping = services[0].stop().then(() => { | ||
| stopped = true; | ||
| }); | ||
| await Promise.resolve(); |
There was a problem hiding this comment.
src/index.test.ts:820 - this test passes with the stop() wait block deleted.
Both assertions hold either way. await Promise.resolve() advances one microtask, but stop() always awaits captureCoordinator.drain, so it cannot have resolved yet - that asserts stop() is async, not that it waited for the verification. And releaseHealthCheck() runs before await stopping, so the gate is already open when we yield and healthCheckSettled becomes true regardless.
I deleted the whole if (startupVerification) block from stop() and the suite still reported 264 passed / 19 skipped - the same numbers cited in the description.
Fix: record push order into an array, swap the microtask for a real timer gap, assert ["health","stop"]
There was a problem hiding this comment.
Fixed using your approach: push order into an array, 50ms timer gap instead of the microtask, assert ["health", "stop"]. Verified the way you did: deleting the if (startupVerification) block from stop() now fails this test.
| // provider errors, but a throw from the logger inside its own catch | ||
| // block would otherwise escape as an unhandled rejection, which is | ||
| // fatal under Node's default --unhandled-rejections=throw. | ||
| startupVerification = verifyBackend().catch(() => {}); |
There was a problem hiding this comment.
src/index.ts:1262 - a second start() drops the first verification instead of reusing it.
The assign is unconditional, so start() twice without a stop() between replaces the tracked promise. The first verifyBackend() keeps running unreferenced, so stop() waits only for the second and the first can log or ensureView after "stopped". ??= keeps the in-flight promise and skips the duplicate call; start/stop/start still re-verifies, since both paths nil the field.
| startupVerification = verifyBackend().catch(() => {}); | |
| startupVerification ??= verifyBackend().catch(() => {}); |
Use ??= so a second start() cannot orphan the tracked verification, and rewrite the stop()-waits test to assert push order across a timer gap instead of passing vacuously. Both verified by mutation.
1cdaa0e to
5ed9159
Compare
Problem
The plugin's service start assumes startup latency does not matter.
start()awaits a health check against the memory backend, plus summary-view setup on self-hosted, before it returns, and OpenClaw counts plugin service start as part of gateway readiness.For a long-lived gateway that assumption is fine. You boot once, and a few hundred milliseconds is invisible. It stops being fine when the gateway is booted fresh per user session, because then that wait is paid on every session and lands inside somebody's startup time. We hit this in a warm pool where an environment is assigned to a user on demand: tracing put 272ms of the assignment on this one call, on every assignment.
Awaiting the health check also buys no safety. If it fails, the code logs a warning and boots anyway, and every memory tool call already handles provider errors on its own. On the cloud backend, where
summaryViewsis false and there are no views to set up, the await is nothing but a diagnostic ping sitting in the readiness path.Fix
Add
eagerStartupCheck, defaulting totrueso existing behavior is unchanged. Set it tofalseand the same verification runs in the background:start()returns immediately and failures still log warnings.It is opt-in rather than a straight behavior change for two reasons. This is a released plugin, so startup behavior should not shift under existing users to suit one deployment shape. More concretely, on self-hosted backends
start()also ensures summary views, and awaiting that genuinely does guarantee the views exist before the first recall. Deferring gives that guarantee up, so it should be a deliberate choice. On cloud there is nothing to ensure, which is why the deployments that want this flag are the same ones for which it costs nothing.In our warm pool the trace span dropped from 272ms to 6ms and assignment latency fell from 5,455ms to 5,185ms mean over 15 cycles with zero failures.
Since
globalThis.fetchpools connections, the eager check was also warming TLS. Deferring does not push that cost onto the user's first memory call, because the check still starts at service start, just without blocking readiness. Measured in-pod, health costs about 105ms once connected, plus 60ms to 340ms of first-request connection setup.Changes
verifyBackend()extracted;start()awaits it or backgrounds it per config.stop()waits for an in-flight background check, bounded at 5s like the existing capture drain, so anensureViewcannot write during teardown..catch()on the background call: a throw from the logger insideverifyBackend's own catch block would otherwise be an unhandled rejection, fatal under Node's default.openclaw.plugin.jsonconfigSchema.connected to serverline that both sections tell you to look for now arrives after startup rather than during it.Note for reviewers
OpenClaw validates plugin config against the manifest
configSchema(additionalProperties: false) before the plugin loads, so a key the parser accepts but the manifest omits fails gateway startup outright rather than degrading. That bit us in a deployment.ALLOWED_CONFIG_KEYSis now exported and a parity test pins the manifest schema to it, so a key added to only one of the two files fails CI instead of a live gateway.Testing
264 pass, 19 skipped (live tests need credentials). New coverage: default and opt-out, non-boolean rejection, manifest and parser parity,
start()awaiting by default,start()returning early when deferred,stop()waiting for the in-flight check, and a throwing logger not escaping. Also validated on live GKE with the cloud backend, where the gateway boots, sessions restore, and agent turns plus memory store and recall all work.