fix(selfhost,observability): close three durable-wrong-state gaps, label the latency histogram, probe the browser, and add memory to host pressure (#9544, #9545) (#9547)

Author: JSONbored

apps/loopover-ui/src/lib/selfhost-env-reference.ts

@@ -119,7 +119,7 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [
   },
   {
     name: "BROWSER_WS_ENDPOINT",
-    firstReference: "src/selfhost/stubs/puppeteer.ts",
+    firstReference: "src/selfhost/health.ts",
   },
   {
     name: "CLAUDE_AI_EFFORT",
@@ -333,6 +333,10 @@ export const SELFHOST_ENV_REFERENCE_ROWS: SelfHostEnvReferenceRow[] = [
     name: "MAINTENANCE_ADMISSION_MAX_HOST_LOAD",
     firstReference: "src/selfhost/maintenance-admission.ts",
   },
+  {
+    name: "MAINTENANCE_ADMISSION_MAX_HOST_MEMORY",
+    firstReference: "src/selfhost/maintenance-admission.ts",
+  },
   {
     name: "MAINTENANCE_ADMISSION_MAX_LIVE_AGE_MS",
     firstReference: "src/selfhost/maintenance-admission.ts",
@@ -698,7 +702,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [
   "| `ANTHROPIC_AI_MODEL` | `src/selfhost/ai.ts` |",
   "| `ANTHROPIC_API_KEY` | `src/selfhost/ai-config.ts` |",
   "| `BACKUP_ACKNOWLEDGED` | `src/server.ts` |",
-  "| `BROWSER_WS_ENDPOINT` | `src/selfhost/stubs/puppeteer.ts` |",
+  "| `BROWSER_WS_ENDPOINT` | `src/selfhost/health.ts` |",
   "| `CLAUDE_AI_EFFORT` | `src/selfhost/ai.ts` |",
   "| `CLAUDE_AI_FIRST_OUTPUT_TIMEOUT_MS` | `src/selfhost/ai.ts` |",
   "| `CLAUDE_AI_MODEL` | `src/selfhost/ai.ts` |",
@@ -752,6 +756,7 @@ export const SELFHOST_ENV_REFERENCE_MARKDOWN = [
   "| `MAINTENANCE_ADMISSION_MAX_BACKLOG_CONVERGENCE_PENDING` | `src/selfhost/maintenance-admission.ts` |",
   "| `MAINTENANCE_ADMISSION_MAX_DEFER_AGE_MS` | `src/selfhost/maintenance-admission.ts` |",
   "| `MAINTENANCE_ADMISSION_MAX_HOST_LOAD` | `src/selfhost/maintenance-admission.ts` |",
+  "| `MAINTENANCE_ADMISSION_MAX_HOST_MEMORY` | `src/selfhost/maintenance-admission.ts` |",
   "| `MAINTENANCE_ADMISSION_MAX_LIVE_AGE_MS` | `src/selfhost/maintenance-admission.ts` |",
   "| `MAINTENANCE_ADMISSION_MAX_LIVE_PENDING` | `src/selfhost/maintenance-admission.ts` |",
   "| `MAINTENANCE_ADMISSION_MAX_PENDING` | `src/selfhost/maintenance-admission.ts` |",

src/selfhost/blob-store.ts

@@ -5,7 +5,8 @@
 // surface those two paths use; every other R2Bucket method is unused on self-host. Node-only (fs import never
 // reaches the Worker bundle — wired in server.ts behind REVIEW_AUDIT_DIR). MODULAR + off by default: unset
 // REVIEW_AUDIT_DIR ⇒ no REVIEW_AUDIT binding ⇒ captures degrade to on-demand exactly as before.
-import { mkdir, readFile, rm, writeFile } from "node:fs/promises";
+import { mkdir, readFile, rename, rm, writeFile } from "node:fs/promises";
+import { randomUUID } from "node:crypto";
 import { dirname, resolve, sep } from "node:path";
 
 /** Build a filesystem-backed REVIEW_AUDIT store rooted at `baseDir`. Keys are app-generated
@@ -48,7 +49,27 @@ export function createFsBlobStore(baseDir: string): R2Bucket {
     async put(key: string, value: ReadableStream | ArrayBuffer | ArrayBufferView | string | Blob | null): Promise<R2Object> {
       const target = pathFor(key);
       await mkdir(dirname(target), { recursive: true });
-      await writeFile(target, Buffer.from(await new Response(value ?? "").arrayBuffer()));
+      // #9487: write to a unique temp path, then rename. Keys here are INPUT-HASH-ADDRESSED
+      // (`loopover/shots/<hash>.png`), so a half-written file from a mid-write kill is never retried or
+      // overwritten -- the next lookup for that same input hits it, finds a file, and serves a truncated PNG
+      // as a permanently "valid" cache entry. rename(2) is atomic within a filesystem, so a reader sees
+      // either no file or the complete one, never a partial. Same tmp+rename the config writer already does
+      // (private-config.ts's atomicWriteWithBackup) -- and the temp name carries a UUID so two concurrent
+      // puts of the same key cannot clobber each other's in-progress file.
+      const tmpTarget = `${target}.tmp-${randomUUID()}`;
+      try {
+        await writeFile(tmpTarget, Buffer.from(await new Response(value ?? "").arrayBuffer()));
+        await rename(tmpTarget, target);
+      } catch (error) {
+        // Never leave the temp file behind on a failed write -- otherwise a full disk or a mid-write crash
+        // accretes orphans in the same directory the real objects live in. Best-effort: the original error is
+        // what the caller needs, not a cleanup failure.
+        /* v8 ignore next -- the cleanup's own failure arm: `force: true` already swallows ENOENT, so this only
+           fires for something like an unwritable directory, in which case the ORIGINAL write error is what the
+           caller needs. Unreachable without mocking fs, and mocking it here would test the mock. */
+        await rm(tmpTarget, { force: true }).catch(() => undefined);
+        throw error;
+      }
       return { key } as unknown as R2Object;
     },
     /** Remove a stored object. A missing file is not an error (matches R2's own delete-is-idempotent

src/selfhost/health.ts

@@ -143,6 +143,75 @@ export function codexAuthReadinessProbe(
   };
 }
 
+/**
+ * #9487/#9464: readiness probe for the browserless endpoint backing visual capture.
+ *
+ * Every other dependency the review path needs has a probe (Redis, Qdrant, the GitHub App, codex);
+ * `BROWSER_WS_ENDPOINT` had none, so a browserless outage was invisible in `/ready` AND in Prometheus while it
+ * silently changed gate OUTCOMES — the screenshot-table gate reads absent evidence as a close signal. #9464
+ * stopped that from closing PRs; this makes the outage itself visible instead of inferred after the fact.
+ *
+ * Only registered when the endpoint is configured — an instance with visual review switched off is not
+ * degraded for lacking a browser, so an unconditional probe would make `/ready` red for every deployment that
+ * never wanted the feature.
+ *
+ * Probes the endpoint's HTTP sibling rather than opening a websocket + launching Chromium: a readiness check
+ * must be cheap enough to run on every poll, and a real launch costs a browser process. browserless answers
+ * `/json/version` over plain HTTP on the same host/port as its `ws://` endpoint, which is exactly the
+ * "is the service accepting connections" signal wanted here.
+ *
+ * Result is cached like the codex probe (same `cacheMs` shape, same single-flight guard) so a poll loop can't
+ * turn readiness into its own load source against an already-struggling backend.
+ */
+export function browserEndpointReadinessProbe(
+  env: Record<string, string | undefined>,
+  fetchImpl: (url: string) => Promise<{ ok: boolean }>,
+  cacheMs = 30_000,
+): ReadinessProbe | null {
+  const endpoint = (env.BROWSER_WS_ENDPOINT ?? "").trim();
+  if (!endpoint) return null;
+  const versionUrl = browserVersionUrl(endpoint);
+  // A configured-but-unparseable endpoint is a real misconfiguration, and one the ordinary capture path would
+  // only surface as a per-shot render failure. Fail readiness closed rather than skipping the probe.
+  if (versionUrl === null) return { name: "browser_endpoint", check: () => Promise.resolve(false) };
+  let cached: boolean | undefined;
+  let cachedUntil = 0;
+  let inFlight: Promise<boolean> | undefined;
+  return {
+    name: "browser_endpoint",
+    check: () => {
+      const now = Date.now();
+      if (cached !== undefined && now < cachedUntil) return Promise.resolve(cached);
+      if (inFlight) return inFlight;
+      inFlight = fetchImpl(versionUrl)
+        .then((response) => response.ok)
+        .catch(() => false)
+        .then((ok) => {
+          cached = ok;
+          cachedUntil = Date.now() + cacheMs;
+          return ok;
+        })
+        .finally(() => {
+          inFlight = undefined;
+        });
+      return inFlight;
+    },
+  };
+}
+
+/** The `http(s)://<host>/json/version` sibling of a `ws(s)://` browserless endpoint, or null when the
+ *  configured value is not a parseable ws/wss URL. Query strings (browserless carries `?token=`) are dropped:
+ *  the version endpoint needs no auth and the token must never end up in a probe URL that could be logged. */
+function browserVersionUrl(endpoint: string): string | null {
+  try {
+    const url = new URL(endpoint);
+    if (url.protocol !== "ws:" && url.protocol !== "wss:") return null;
+    return `${url.protocol === "wss:" ? "https:" : "http:"}//${url.host}/json/version`;
+  } catch {
+    return null;
+  }
+}
+
 /** Boot-time DATA-SAFETY advisory. A single SQLite file with no acknowledged backup is a data-loss SPOF — yet
  *  `/ready` would still answer 200, so an operator can run with zero durability believing they're healthy. Returns
  *  the warning to log at boot (or null on Postgres, or once the operator sets `BACKUP_ACKNOWLEDGED=true` after

src/selfhost/host-pressure.ts

@@ -2,7 +2,7 @@
 // `node:os`'s loadavg() has no meaningful signal on Cloudflare Workers -- this module is imported ONLY by the
 // self-host Node queue backends (sqlite-queue.ts / pg-queue.ts), never by src/index.ts's Worker bundle, so a
 // static `node:os` import here is safe (mirrors the existing `hostname` import in selfhost/posthog.ts).
-import { cpus, loadavg } from "node:os";
+import { cpus, freemem, loadavg, totalmem } from "node:os";
 
 /** The 1-minute load average normalized per logical core, so the SAME threshold means the same thing on a
  *  4-vCPU box as a 32-vCPU box. Best-effort and fail-open: any error, or a reading that can't possibly be a
@@ -20,3 +20,37 @@ export function hostLoadAvg1PerCore(): number | null {
     return null;
   }
 }
+
+/**
+ * #9487: the fraction of host memory currently IN USE (0..1), or `null` when unavailable.
+ *
+ * Host pressure watched CPU only, but on the deployment this was found on (a GPU box running Ollama at
+ * ~9.9 GiB alongside browserless at ~1.5 GiB) memory is the realistic killer: nothing observed it, nothing
+ * shed load for it, and the OOM killer made the decision instead — which takes the whole container, losing
+ * every in-flight job, rather than deferring one maintenance job.
+ *
+ * Same fail-open contract as {@link hostLoadAvg1PerCore}: any error, or a reading that cannot be a real
+ * ratio, yields `null` ("signal unavailable"), never a misleading 0. A caller must treat `null` as "skip this
+ * check".
+ *
+ * HONEST LIMIT, shared with the load signal above: `node:os` reports the HOST's memory, so under a container
+ * memory limit (cgroup) this understates pressure — the container can be at its own ceiling while the host
+ * looks idle. Reading `/sys/fs/cgroup/memory.current` would fix that and is deliberately not done here: it is
+ * Linux- and cgroup-v2-specific, and this module is a best-effort *hint* for admission, not an accounting
+ * boundary. The same caveat already applies to loadavg-over-container-cores.
+ */
+export function hostMemoryUsedFraction(): number | null {
+  try {
+    const total = totalmem();
+    const free = freemem();
+    if (!Number.isFinite(total) || total <= 0) return null;
+    if (!Number.isFinite(free) || free < 0) return null;
+    const used = (total - free) / total;
+    // A free reading above total would put this outside 0..1 -- treat any impossible ratio as unavailable
+    // rather than clamping, so a broken platform reading can never masquerade as "no pressure".
+    if (used < 0 || used > 1) return null;
+    return used;
+  } catch {
+    return null;
+  }
+}

src/selfhost/load-file-secrets.ts

@@ -27,8 +27,9 @@ export function loadFileSecrets(
     const target = key.slice(0, -"_FILE".length);
     if (env[target]) continue; // an explicit value wins
     const path = env[key] as string;
+    let value: string;
     try {
-      env[target] = readFile(path).trim();
+      value = readFile(path).trim();
     } catch (error) {
       console.error(
         JSON.stringify({
@@ -43,5 +44,26 @@ export function loadFileSecrets(
         }`,
       );
     }
+    // #9487: an EMPTY (zero-byte or whitespace-only) secret file is as fatal as a missing one. Setting
+    // `env[target] = ""` looks like a successful load, but every downstream `nonBlank()` reads "" as
+    // UNCONFIGURED and preflight.ts deliberately skips absent values -- so a truncated
+    // GITHUB_WEBHOOK_SECRET file booted an instance that silently rejected every webhook. Directly adjacent
+    // to the known secret-rotation footgun on edge-nl-01, where a file is rewritten in place: the window in
+    // which it is momentarily empty is exactly when a container restart reads it.
+    //
+    // Checked OUTSIDE the try above on purpose: throwing inside it would be caught by that catch and
+    // re-reported as "unreadable", collapsing two genuinely different operator problems (a bad path/permission
+    // vs a truncated write) into one misleading message and the wrong log event.
+    if (value === "") {
+      console.error(
+        JSON.stringify({
+          level: "error",
+          event: "selfhost_secret_file_empty",
+          var: key,
+        }),
+      );
+      throw new Error(`Secret file for ${key} (${path}) is empty; an empty secret silently reads as unconfigured downstream. Write the value, or unset ${key}.`);
+    }
+    env[target] = value;
   }
 }

src/selfhost/maintenance-admission.ts

@@ -108,6 +108,12 @@ export interface MaintenancePressureSignals {
   oldestMaintenancePendingAgeMs: number | null;
   /** Null when unavailable (see host-pressure.ts) -- a caller must treat null as "skip this check". */
   hostLoadAvg1PerCore: number | null;
+  /** #9487: fraction of host memory in use (0..1), or null when unavailable -- same skip-on-null contract as
+   *  hostLoadAvg1PerCore above. Pressure watched CPU only, but on a box also running Ollama (~9.9 GiB) and
+   *  browserless (~1.5 GiB) memory is the realistic killer, and nothing observed it: the OOM killer decided
+   *  instead, taking the whole container and every in-flight job with it rather than deferring one
+   *  maintenance job. */
+  hostMemoryUsedFraction: number | null;
   /** #selfhost-backlog-convergence: pending+processing count of `agent-regate-pr` jobs tagged
    *  `foreground_lane='backlog'` (queue-fairness.ts) -- the backlog-convergence sweeper's own output, DISTINCT
    *  from `livePendingCount` (which is priority-gated, not lane-gated, and includes fresh webhook/foreground
@@ -128,6 +134,8 @@ export interface MaintenanceAdmissionConfig {
   maxLiveJobAgeMs: number;
   maxMaintenancePendingCount: number;
   maxHostLoadAvg1PerCore: number;
+  /** #9487: defer maintenance above this fraction of host memory in use. */
+  maxHostMemoryUsedFraction: number;
   maxBacklogConvergencePendingCount: number;
   deferMs: number;
   maxDeferAgeMs: number;
@@ -146,6 +154,10 @@ const DEFAULT_MAX_LIVE_PENDING_COUNT = 5;
 const DEFAULT_MAX_LIVE_JOB_AGE_MS = 2 * 60_000;
 const DEFAULT_MAX_MAINTENANCE_PENDING_COUNT = 15;
 const DEFAULT_MAX_HOST_LOAD_AVG1_PER_CORE = 1.5;
+// #9487: 0.92 leaves genuine headroom before the OOM killer without deferring on ordinary steady-state usage
+// -- a box running a resident model server sits legitimately high (page cache plus a multi-GiB model), so a
+// tighter default would defer maintenance permanently on exactly the deployment this was found on.
+const DEFAULT_MAX_HOST_MEMORY_USED_FRACTION = 0.92;
 // Deliberately more permissive than maxLivePendingCount (5): a real incident's backlog-convergence sweep can
 // legitimately queue several PRs across several repos at once (BACKLOG_CONVERGENCE_SWEEP_MAX_PRS=5 per repo per
 // sweep, selfhost/backlog-convergence.ts) without that alone meaning maintenance must fully yield -- only a
@@ -200,6 +212,10 @@ export function resolveMaintenanceAdmissionConfig(): MaintenanceAdmissionConfig
       "MAINTENANCE_ADMISSION_MAX_HOST_LOAD",
       DEFAULT_MAX_HOST_LOAD_AVG1_PER_CORE,
     ),
+    maxHostMemoryUsedFraction: parsePositiveFloatEnv(
+      "MAINTENANCE_ADMISSION_MAX_HOST_MEMORY",
+      DEFAULT_MAX_HOST_MEMORY_USED_FRACTION,
+    ),
     maxBacklogConvergencePendingCount: parsePositiveIntEnv("MAINTENANCE_ADMISSION_MAX_BACKLOG_CONVERGENCE_PENDING", {
       min: 0,
       fallback: DEFAULT_MAX_BACKLOG_CONVERGENCE_PENDING_COUNT,
@@ -225,6 +241,7 @@ export type MaintenanceAdmissionReason =
   | "maintenance_pending_high"
   | "maintenance_pending_high_drain"
   | "host_load_high"
+  | "host_memory_high"
   | "pressure_clear";
 
 export interface MaintenanceAdmissionDecision {
@@ -268,18 +285,25 @@ export function evaluateMaintenanceAdmission(
   }
   const hostLoadHigh =
     signals.hostLoadAvg1PerCore !== null && signals.hostLoadAvg1PerCore > config.maxHostLoadAvg1PerCore;
+  // #9487: memory sits beside CPU as a peer pressure dimension, with the identical null-means-skip contract.
+  const hostMemoryHigh =
+    signals.hostMemoryUsedFraction !== null && signals.hostMemoryUsedFraction > config.maxHostMemoryUsedFraction;
   if (signals.maintenancePendingCount > config.maxMaintenancePendingCount) {
     if (nowMs - pendingSinceMs >= config.maintenanceDrainAgeMs) {
       // Host load is re-checked HERE, gating the drain escape specifically: draining more maintenance work onto
       // an already CPU-overloaded box is exactly what host_load_high exists to prevent. A job that hasn't hit
       // drain age yet is denied `maintenance_pending_high` regardless of host load (unchanged from before this
       // escape existed) -- this check only ever changes the outcome for a job the drain would otherwise admit.
       if (hostLoadHigh) return { admit: false, reason: "host_load_high" };
+      // Memory gates the drain escape for the same reason load does: draining more work onto a box that is
+      // already near its memory ceiling is what invites the OOM kill this signal exists to avoid.
+      if (hostMemoryHigh) return { admit: false, reason: "host_memory_high" };
       return { admit: true, reason: "maintenance_pending_high_drain" };
     }
     return { admit: false, reason: "maintenance_pending_high" };
   }
   if (hostLoadHigh) return { admit: false, reason: "host_load_high" };
+  if (hostMemoryHigh) return { admit: false, reason: "host_memory_high" };
   return { admit: true, reason: "pressure_clear" };
 }