Repository navigation
Add periodic re-read safety net for TLS cert watcher (#586) - #1394
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a periodic polling safety net (certificateWatchInterval) to complement the inotify-based file watcher for certificates and private keys, ensuring reliable reloads in environments like containers or network mounts where inotify can fail. It includes configuration validation, default values, and comprehensive unit tests. The feedback suggests two key improvements: first, replacing the synchronous statSync call inside the polling loop with asynchronous fs.stat to prevent blocking the Node.js event loop; second, enforcing a minimum interval limit (e.g., 1000ms) to safeguard against rapid disk polling caused by configuration typos.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const poll = () => { | ||
| let stats; | ||
| try { | ||
| stats = statSync(path); | ||
| } catch (error) { | ||
| // File may be transiently absent (e.g. atomic-rename renewal in flight); the chokidar | ||
| // watcher and the next poll will pick up the replacement. | ||
| logger.trace?.(`Watch poll could not stat ${type}:`, path, error); | ||
| return; | ||
| } | ||
| loadFile(path, stats); | ||
| }; |
There was a problem hiding this comment.
Using the synchronous statSync inside a periodic timer tick blocks the Node.js event loop. Since this poll runs on all threads for private keys and can be configured to run frequently, doing synchronous file I/O (especially on network mounts or slow container filesystems, which this safety net is specifically designed for) can cause latency spikes and degrade application performance.
We should use the asynchronous fs.stat instead to avoid blocking the event loop.
const poll = () => {
fs.stat(path, (error, stats) => {
if (error) {
// File may be transiently absent (e.g. atomic-rename renewal in flight); the chokidar
// watcher and the next poll will pick up the replacement.
logger.trace?.("Watch poll could not stat " + type + ":", path, error);
return;
}
loadFile(path, stats);
});
};| function getCertificateWatchInterval(): number { | ||
| const configured = envManager.get(CONFIG_PARAMS.TLS_CERTIFICATEWATCHINTERVAL); | ||
| if (configured == null) return DEFAULT_CERTIFICATE_WATCH_INTERVAL_MS; | ||
| const interval = Number(configured); | ||
| if (!Number.isFinite(interval) || interval < 0) return DEFAULT_CERTIFICATE_WATCH_INTERVAL_MS; | ||
| return interval; | ||
| } |
There was a problem hiding this comment.
Enforce a reasonable minimum limit (e.g., 1000ms) for the watch interval to prevent accidental performance degradation or denial of service due to rapid disk polling. This also safeguards against configuration typos where a user might specify the interval in seconds instead of milliseconds (e.g., setting 300 instead of 300000).
| function getCertificateWatchInterval(): number { | |
| const configured = envManager.get(CONFIG_PARAMS.TLS_CERTIFICATEWATCHINTERVAL); | |
| if (configured == null) return DEFAULT_CERTIFICATE_WATCH_INTERVAL_MS; | |
| const interval = Number(configured); | |
| if (!Number.isFinite(interval) || interval < 0) return DEFAULT_CERTIFICATE_WATCH_INTERVAL_MS; | |
| return interval; | |
| } | |
| function getCertificateWatchInterval(): number { | |
| const configured = envManager.get(CONFIG_PARAMS.TLS_CERTIFICATEWATCHINTERVAL); | |
| if (configured == null) return DEFAULT_CERTIFICATE_WATCH_INTERVAL_MS; | |
| const interval = Number(configured); | |
| if (!Number.isFinite(interval) || interval < 0) return DEFAULT_CERTIFICATE_WATCH_INTERVAL_MS; | |
| // Prevent excessively frequent polling (e.g. due to config typos like seconds vs ms) by enforcing a minimum of 1000ms (1s) unless disabled (0). | |
| if (interval > 0 && interval < 1000) return DEFAULT_CERTIFICATE_WATCH_INTERVAL_MS; | |
| return interval; | |
| } |
|
Reviewed; no blockers found. |
8a46e74 to
e987961
Compare
|
Holding this — blocked on the This adds a production periodic re-read safety net so a missed inotify event can't strand a renewed cert (overlayfs / network-mount / container filesystems) — relevant to the July 8 cert rotation. The code is complete, but it should land on top of the two
Until those land, the cert hot-reload integration test stays red regardless of detection, so verifying this PR end-to-end isn't meaningful yet. Once they merge I'll rebase this on top and re-verify the cert test end-to-end. This PR also supersedes the test-only polling override in #1392 (production polling makes that redundant). — Claude (Opus 4.8), via the PR-triage session |
When a TLS renewal rotates both the certificate and the private key, each worker receives the new cert through the system.hdb_certificate table subscription (which rebuilds its TLS secure contexts) but reloads the new private key independently from disk into its in-thread privateKeys map. The key reload did not trigger a context rebuild, so a worker could build a context pairing the new cert with the old key and never rebuild it until the next cert-table change — leaving HTTPS stale/broken on that worker. A private-key reload (the single handlePrivateKeyReload sink for both the chokidar watcher and PR #1394's periodic poll) now triggers a debounced rebuild of every live TLS selector on that thread via the module-level liveTLSRebuilders set. The hdb_certificate subscription shares the same debounced scheduleRebuild (identical 1500ms cadence, now coalesced). Transient one-shot selectors (getReplicationCert) opt out via liveReload=false so they don't accumulate. A rotation guard skips the initial load and identical-content reloads to avoid watcher thrash. Pre-existing race (not introduced by #1394, which left it out of scope). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The cert-file watcher in security/keys.ts relies solely on chokidar's inotify events. On filesystems where inotify is unreliable (overlayfs, many container setups, network mounts) a real cert renewal can be silently missed, stranding the old/expired cert. This is a live risk for fleet cert rotation. Add a periodic re-read safety net alongside the existing chokidar watcher: a per-file interval timer (.unref'd) re-stats the watched file and routes through the same reload path on change. Both detection paths share the existing lastModified fingerprint, so an inotify event and a poll for the same change never double-reload, and an unchanged file does no PEM read. The certificate poll runs on the main thread only (workers get cert updates via the hdb_certificate table subscription). The private-key poll runs on all threads, because each worker loads its own key directly from disk with no table propagation, so a key renewal missed by inotify on a worker would otherwise strand the stale key. Interval is configurable via the top-level tls.certificateWatchInterval (ms, default 300000 = 5 minutes); 0 disables polling, leaving only inotify. Registered in hdbTerms, defaultConfig.yaml, the Joi validator, and config-root.schema.json (additionalProperties: false there). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
chokidar v4 defaults alwaysStat:false, so the 'change' handler is invoked with undefined stats. loadFile read stats.mtimeMs directly, which threw and was swallowed by the catch — silently skipping the inotify fast-path reload and leaving only the periodic poll to recover. Fall back to statSync(path) when stats is absent so the immediate watcher path works on all platforms. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e987961 to
024cee0
Compare
A misconfigured tls.certificateWatchInterval (e.g. 1, intending seconds) would otherwise spin the safety-net poll into a tight stat() loop. Floor any positive value at 1s; 0 still explicitly disables polling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Rebased onto current Rebase / conflict resolution
Poll is now the production companion to the inotify watcher
Two small hardening commits from cross-model review (Codex clean; Gemini flagged):
Tests: I deliberately kept the per-tick stat synchronous (Gemini suggested async — generated by Claude Opus 4.8 |
Patch cherry-pick: mergedCherry-picked onto |
1 participant
Summary
The TLS cert-file watcher in
security/keys.ts(loadAndWatch) relies solely on chokidar's inotify events. On filesystems where inotify is unreliable — overlayfs, many container setups, network mounts — a real certificate renewal can be silently missed, leaving Harper serving the old (expired) cert with no error. This is a live risk for the July 8 2026 fleet cert rotation (P0). A CI manifestation of exactly this — overlayfs dropping inotify events — was confirmed in #1392.This PR hardens detection with a periodic re-read safety net. It does not touch worker-side propagation (the table subscription + TLS rebuild), which already works.
What changed
setInterval(.unref()'d so it never holds the process open) re-stats the watched file and routes through the same reload path on change. Chokidar stays the fast path; the poll is the belt-and-suspenders net for missed events.lastModifiedmtime fingerprint, so an inotify event and a poll for the same change never double-reload. An unchanged file does only a cheapstatSync(no PEM read, no reload).tls.certificateWatchInterval(ms), default 300000 (5 min);0disables polling. Registered inhdbTerms.ts,static/defaultConfig.yaml, the Joi validator (validation/configValidator.ts), andconfig-root.schema.json(which hasadditionalProperties: falseontlsConfig).Where to look
security/keys.tsloadAndWatch— poll registration and thread gating.hdb_certificatesubscription). The private-key poll runs on all threads, because each worker loads its own key directly from disk with no table propagation — a key renewal missed by inotify on a worker would otherwise strand the stale key. This asymmetry was raised by cross-model review and is deliberate.Open item for reviewer (deliberately out of scope)
Cross-model review flagged a latent race: when a renewal rotates both cert and key, a worker's private-key poll only refreshes the in-thread
privateKeysmap — it does not itself trigger acreateTLSSelector/updateTLSrebuild. The TLS rebuild is driven by thehdb_certificatesubscription. This is pre-existing behavior of the private-key chokidar watcher too (itsloadCertcallback already just sets the map), not introduced here — this PR only adds a second trigger to the same path. Fixing worker key-rotation → immediate TLS rebuild is a separate, riskier change I deliberately kept out of this P0 patch. Flagging so it's not rediscovered.Testing
unitTests/security/keys.test.js: swap missed by inotify is picked up by the poll (chokidar stubbed so only the poll path runs); mtime-fingerprint dedup; configured-interval resolution +.unref(); default/invalid-interval fallback;0disables; private-key path stays wired.integrationTests/security/cert-reload.test.ts(TLS certificates not auto-reloaded on worker threads (flipping/stale cert after renewal) #586 multi-worker propagation regression) still passes.test:unit:main+test:unit:configgreen (10 pre-existingglobalIsolation.test.jsfailures are unrelated — identical on cleanmain). Lint/format clean; TypeStrip--checkclean.Notes