Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
770 changes: 299 additions & 471 deletions AGENTS.md

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ USER bun
# Nitro bundles all JS and traces native deps (@takumi-rs/core) into .output/server/node_modules — no bun install needed
COPY --chown=bun:bun --from=build /usr/src/app/.output ./.output
COPY --chown=bun:bun --from=build /usr/src/app/server/cluster.ts ./cluster.ts
# cluster.ts imports this at runtime; it must sit next to it.
COPY --chown=bun:bun --from=build /usr/src/app/server/worker-exit.ts ./worker-exit.ts
EXPOSE 8080
# start-period covers migrations on worker 0 plus the staggered sibling spawn
HEALTHCHECK --interval=30s --timeout=5s --start-period=60s --retries=3 \
Expand Down
6 changes: 5 additions & 1 deletion bunfig.toml
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
[test]
root = "src"
# Was "src", which silently made server/ untestable — server/worker-exit.ts
# shipped a bug that mislabelled every OOM kill for a month. Scripts pass the
# directories explicitly (`bun test src server`) so app/'s React Native suites,
# which use the jest preset rather than bun's runner, stay out of the way.
root = "."
18 changes: 16 additions & 2 deletions compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ services:
- "5080:5080"
volumes:
- telem_data:/data
# No healthcheck, and deliberately not a `depends_on` for the server.
# Two reasons, both checked rather than assumed:
# 1. The image is distroless — no /bin/sh, no curl, no wget (verified on
# the host), so any `test:` here would report permanently unhealthy and
# make `docker ps` lie in the opposite direction.
# 2. The app must not refuse to boot because its telemetry backend is
# down. The log transport already tolerates connection-refused and the
# OTLP exporter retries; gating startup on OpenObserve would turn
# observability into a SPOF for the product.
otel-collector:
image: otel/opentelemetry-collector-contrib:latest
restart: unless-stopped
Expand Down Expand Up @@ -94,7 +103,11 @@ services:
PDS_ADMIN_PASSWORD: "CHANGE_ME_admin_password"
OPEN_OBSERVE_USER: "user@bookhive.buzz"
OPEN_OBSERVE_PASSWORD: "password"
OPEN_OBSERVE_URL: "http://localhost:5080"
# Must be container-reachable: inside this container `localhost` is the
# app itself, not OpenObserve. Both the logs transport and the OTLP trace
# exporter derive their endpoints from this, so a wrong value silently
# disables observability rather than failing loudly.
OPEN_OBSERVE_URL: "http://openobserve:5080"
Comment on lines +106 to +110

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '30,115p' compose.yaml

for file in src/workers/open-observe-worker.ts server/plugins/otel-sdk.ts src/logger/index.ts; do
  if [ -f "$file" ]; then
    printf '\n--- %s ---\n' "$file"
    rg -n -C 8 'OPEN_OBSERVE_URL|ECONNREFUSED|connection refused|retry|disable|fetch|OTLP' "$file" || true
  fi
done

Repository: nperez0111/bookhive

Length of output: 9227


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- compose services relevant ---'
python3 - <<'PY'
import yaml
from pathlib import Path
p=Path('compose.yaml')
data=yaml.safe_load(p.read_text())
for name in ['server','openobserve','otel-collector']:
    svc=data.get('services',{}).get(name,{})
    print(name, {
        'depends_on': svc.get('depends_on'),
        'healthcheck': svc.get('healthcheck'),
        'ports': svc.get('ports'),
    })
PY

printf '\n--- openobserve worker flush tail ---\n'
sed -n '85,140p' src/workers/open-observe-worker.ts

printf '\n--- otel plugin lifecycle tail ---\n'
sed -n '20,120p' server/plugins/otel-sdk.ts

printf '\n--- logger setup context ---\n'
sed -n '1,80p' src/logger/index.ts

Repository: nperez0111/bookhive

Length of output: 308


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- compose services relevant ---'
python3 - <<'PY'
import json
from pathlib import Path

path = Path('compose.yaml')
text = path.read_text().splitlines()
# extract relevant service blocks as YAML is not available; simple state machine
state = []
for i,line in enumerate(text,1):
    if line.startswith('  server:') or line.startswith('  openobserve:') or line.startswith('  otel-collector:'):
        state.append([i, line.strip().rstrip(':'), {}])
    elif len(state):
        k=line.strip().split(':', 1)[0]
        if k in ('depends_on','healthcheck') and state[-1][1] in ('server','openobserve','otel-collector'):
            state[-1][2][k]=True
        elif k and k[0] != ',' and k[0] not in '-#':
            if state[-1][1] == 'otel-collector' and state[-1][2] == {}:
                # avoid config keys after depends_on line if parsed inline
                pass
for item in state:
    if not item[2]:
        print(item[1], {'depends_on': None, 'healthcheck': None})
    elif item[2].get('healthcheck'):
        print(item[1], {'depends_on': item[2].get('depends_on'), 'healthcheck': True})
    else:
        print(item[1], {'depends_on': item[2].get('depends_on'), 'healthcheck': False})
PY

printf '\n--- openobserve worker flush tail ---\n'
sed -n '85,140p' src/workers/open-observe-worker.ts

printf '\n--- otel plugin lifecycle tail ---\n'
sed -n '20,120p' server/plugins/otel-sdk.ts

printf '\n--- logger setup context ---\n'
sed -n '1,80p' src/logger/index.ts

Repository: nperez0111/bookhive

Length of output: 4942


Tie server startup to OpenObserve readiness.

server targets http://openobserve:5080, but openobserve has no healthcheck and server does not depend on it; otel-collector uses dependent so it can start before server and openobserve are both running. Add a healthcheck to openobserve:latest and make server depend on openobserve.service_healthy, or use an explicit startup health-check wrapper, so logs and traces are not configured against a service that is too early to accept connections.

🤖 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 `@compose.yaml` around lines 97 - 101, In compose.yaml, add a healthcheck to
the openobserve service and update the server service’s dependency condition to
require openobserve.service_healthy before startup. Preserve the existing
OPEN_OBSERVE_URL and ensure the dependency uses the healthcheck result rather
than simple container startup.

IMGPROXY_URL: "http://imgproxy:8080"
IMGPROXY_KEY: "CHANGE_ME_imgproxy_key_hex"
IMGPROXY_SALT: "CHANGE_ME_imgproxy_salt_hex"
Expand All @@ -104,7 +117,8 @@ services:
- data:/data
# Leave the other ~4 host CPUs for pds/imgproxy/openobserve and future services.
cpus: "4"
# ~1GB shared SQLite mmap page cache + ~300MB private per worker.
# ~300-400MB anonymous per worker. The old budget assumed a ~1GB SQLite
# mmap on top; DB_MMAP_SIZE now defaults to 0, so that term is gone.
mem_limit: 3g
# The commented out section below is an example of how to define a PostgreSQL
# database that your application can use. `depends_on` tells Docker Compose to
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
"dev": "bunx --bun vp dev",
"build": "bun --bun run lexgen && bunx --bun vp build",
"preview": "bun run .output/server/index.mjs",
"test": "bun test src",
"test:run": "bun test src --no-watch",
"test": "bun test src server",
"test:run": "bun test src server --no-watch",
"lint": "vp lint src --type-aware --type-check && vp fmt --write",
"typecheck": "vp lint src --type-aware --type-check && vp fmt --write",
"format": "vp fmt",
Expand Down
82 changes: 44 additions & 38 deletions server/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@
* must pass /healthcheck before the siblings spawn — that ordering is the
* migration barrier for the non-primary workers.
*
* Not bundled — the Dockerfile copies this file verbatim and Bun runs the TS
* source directly. Zero dependencies.
* Not bundled — the Dockerfile copies this file and ./worker-exit.ts verbatim
* and Bun runs the TS source directly. Zero external dependencies.
*/
import { classifyWorkerExit, readProcessMemoryKb } from "./worker-exit.ts";

const concurrency = Math.max(1, Number(process.env["WEB_CONCURRENCY"]) || 4);
const port = process.env["PORT"] ?? "8080";
Expand All @@ -27,45 +28,41 @@ function log(message: string) {
}

/**
* Worker deaths were invisible: the app logged 82 errors against 171,145
* user-visible 502s on 2026-08-01 because the failure mode was process death,
* not an exception. A cgroup OOM kill arrives as signal SIGKILL with a null
* exit code, so emit it as a JSON line the log pipeline can count.
* Last memory sample per worker index. `/proc/<pid>` is gone by the time
* `onExit` fires, so a worker killed for using 2 GB would otherwise report no
* memory at all — exactly the number an OOM investigation needs.
*/
// Bun reports signalCode as a number.
const SIGNAL_NAMES: Record<number, string> = {
2: "SIGINT",
6: "SIGABRT",
9: "SIGKILL",
11: "SIGSEGV",
15: "SIGTERM",
};
const lastMemory = new Map<number, { rss_kb?: number; anon_kb?: number }>();
const MEMORY_SAMPLE_MS = 15_000;

function signalName(signalCode: number | null): string | null {
if (signalCode === null) return null;
return SIGNAL_NAMES[signalCode] ?? `SIG${signalCode}`;
function sampleWorkerMemory() {
for (const [index, proc] of children) {
const sample = readProcessMemoryKb(proc.pid);
if (sample) lastMemory.set(index, sample);
}
}

/** Emits the structured line and hands the classification back, so the
* human-readable restart message below reads the same `likely_oom` rather than
* re-deriving it from the raw signal — that duplicate condition is how the
* two logs could disagree about whether a kill was an OOM. */
function logWorkerExit(
index: number,
pid: number | null,
exitCode: number | null,
signalCode: number | null,
signalCode: number | string | null | undefined,
uptimeMs: number,
) {
const signal = signalName(signalCode);
console.error(
JSON.stringify({
level: 50,
time: Date.now(),
msg: "worker_exit",
worker: index,
code: exitCode,
signal,
// A cgroup OOM kill arrives as SIGKILL with no exit code.
likely_oom: signal === "SIGKILL",
uptime_ms: uptimeMs,
}),
);
const event = classifyWorkerExit({
index,
pid,
exitCode,
signalCode,
uptimeMs,
memory: lastMemory.get(index) ?? null,
});
console.error(JSON.stringify({ time: Date.now(), ...event }));
return event;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

function spawnWorker(index: number) {
Expand All @@ -75,11 +72,17 @@ function spawnWorker(index: number) {
env: { ...process.env, WORKER_INDEX: String(index) },
stdout: "inherit",
stderr: "inherit",
onExit(_proc, exitCode, signalCode) {
onExit(exited, exitCode, signalCode) {
children.delete(index);
// A SIGTERM we sent ourselves is not a failure — don't page on it.
if (shuttingDown) return;
logWorkerExit(index, exitCode, signalCode, Date.now() - startedAt);
const exit = logWorkerExit(
index,
exited?.pid ?? null,
exitCode,
signalCode,
Date.now() - startedAt,
);
const now = Date.now();
const recent = (restartTimes.get(index) ?? []).filter((t) => now - t < 60_000);
recent.push(now);
Expand All @@ -90,11 +93,11 @@ function spawnWorker(index: number) {
return;
}
const backoffMs = Math.min(1000 * 2 ** (recent.length - 1), 15_000);
const signal = signalName(signalCode);
const anon = lastMemory.get(index)?.anon_kb;
log(
`worker ${index} exited (code ${exitCode}, signal ${signal ?? "none"}${
signal === "SIGKILL" ? ", likely OOM" : ""
}), restarting in ${backoffMs}ms`,
`worker ${index} exited (code ${exitCode}, signal ${exit.signal ?? "none"}${
exit.likely_oom ? ", likely OOM" : ""
}${anon ? `, last anon ${Math.round(anon / 1024)}MB` : ""}), restarting in ${backoffMs}ms`,
);
setTimeout(() => spawnWorker(index), backoffMs);
},
Expand Down Expand Up @@ -137,6 +140,9 @@ function shutdown(code: number) {
process.on("SIGTERM", () => shutdown(0));
process.on("SIGINT", () => shutdown(0));

const memoryTimer = setInterval(sampleWorkerMemory, MEMORY_SAMPLE_MS);
memoryTimer.unref?.();

spawnWorker(0);
if (concurrency > 1) {
await waitForPrimaryHealthy();
Expand Down
23 changes: 22 additions & 1 deletion server/plugins/otel-sdk.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
/**
* Nitro plugin: OpenTelemetry SDK lifecycle.
* Starts the SDK on boot and shuts it down gracefully on close.
*
* Traces reach OpenObserve at `${OPEN_OBSERVE_URL}/api/bookhive/v1/traces`, so
* `OPEN_OBSERVE_URL` must be **container-reachable** — `http://openobserve:5080`
* on the shared `backbone` network. Never `localhost`, which inside this
* container is the app itself. The repo's compose.yaml said `localhost` while
* the deployment said `openobserve`, and that mismatch is exactly how this
* pipeline was once misdiagnosed as dead. It is not: verified 2026-08-02, the
* `traces/default` stream holds 13.7M spans and is current.
*/
import { definePlugin } from "nitro";
import { NodeSDK } from "@opentelemetry/sdk-node";
Expand All @@ -25,7 +33,20 @@ export default definePlugin((nitroApp) => {
const sdk = new NodeSDK({
serviceName: "bookhive",
traceExporter,
instrumentations: [getNodeAutoInstrumentations()],
instrumentations: [
getNodeAutoInstrumentations({
// A span per filesystem operation. Every static asset read, every
// SQLite-adjacent stat, in every worker — the standard advice is to
// leave this off, and an unconfigured `getNodeAutoInstrumentations()`
// turns it on.
"@opentelemetry/instrumentation-fs": { enabled: false },
// Inbound requests already get a root span from request-tracing.ts and
// a route span from src/middleware/otel-middleware.ts. A third would be
// noise. Outbound stays on: a PDS that stops answering and Goodreads
// refusing us are precisely what the incidents were about.
"@opentelemetry/instrumentation-http": { ignoreIncomingRequestHook: () => true },
}),
],
});

if (!env.isDev) {
Expand Down
147 changes: 147 additions & 0 deletions server/worker-exit.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
import { describe, it, expect } from "bun:test";
import { classifyWorkerExit, readProcessMemoryKb, signalName } from "./worker-exit.ts";

describe("signalName", () => {
// The bug: Bun passes the signal *name*, its types claim a number, and the
// number-keyed lookup fell through to `SIG${code}` — producing "SIGSIGKILL"
// in production for every one of 148 OOM kills.
it("passes through a name Bun already prefixed", () => {
expect(signalName("SIGKILL")).toBe("SIGKILL");
expect(signalName("SIGTERM")).toBe("SIGTERM");
});

it("still maps a numeric code, in case Bun's types become honest", () => {
expect(signalName(9)).toBe("SIGKILL");
expect(signalName(15)).toBe("SIGTERM");
});

it("is null for a clean exit", () => {
expect(signalName(null)).toBeNull();
expect(signalName(undefined)).toBeNull();
});

it("prefixes a bare name", () => {
expect(signalName("KILL")).toBe("SIGKILL");
});
});

describe("classifyWorkerExit", () => {
it("flags a cgroup OOM kill", () => {
const event = classifyWorkerExit({
index: 1,
pid: 4242,
exitCode: null,
signalCode: "SIGKILL",
uptimeMs: 775_653,
});

expect(event.signal).toBe("SIGKILL");
expect(event.likely_oom).toBe(true);
expect(event.msg).toBe("worker_exit");
expect(event.level).toBe(50);
expect(event.worker).toBe(1);
expect(event.pid).toBe(4242);
expect(event.uptime_ms).toBe(775_653);
});

it("does not flag a graceful exit as an OOM", () => {
const event = classifyWorkerExit({
index: 0,
exitCode: 0,
signalCode: null,
uptimeMs: 1_000,
});
expect(event.signal).toBeNull();
expect(event.likely_oom).toBe(false);
});

it("does not flag SIGTERM as an OOM", () => {
const event = classifyWorkerExit({
index: 0,
exitCode: null,
signalCode: "SIGTERM",
uptimeMs: 1_000,
});
expect(event.likely_oom).toBe(false);
});

it("does not flag a SIGKILL that carried an exit code", () => {
// Something other than the kernel's OOM killer produced this.
const event = classifyWorkerExit({
index: 0,
exitCode: 137,
signalCode: "SIGKILL",
uptimeMs: 1_000,
});
expect(event.likely_oom).toBe(false);
});

it("carries the last memory sample so a kill is attributable", () => {
const event = classifyWorkerExit({
index: 2,
exitCode: null,
signalCode: "SIGKILL",
uptimeMs: 700_000,
memory: { rss_kb: 2_580_000, anon_kb: 1_779_772 },
});
expect(event.anon_kb).toBe(1_779_772);
expect(event.rss_kb).toBe(2_580_000);
});

it("omits memory fields entirely when there is no sample", () => {
const event = classifyWorkerExit({
index: 0,
exitCode: null,
signalCode: "SIGKILL",
uptimeMs: 1,
memory: null,
});
expect("anon_kb" in event).toBe(false);
expect("rss_kb" in event).toBe(false);
});

it("serializes to a single JSON log line", () => {
const line = JSON.parse(
JSON.stringify(
classifyWorkerExit({
index: 1,
exitCode: null,
signalCode: "SIGKILL",
uptimeMs: 5,
}),
),
);
expect(line.msg).toBe("worker_exit");
expect(line.signal).toBe("SIGKILL");
expect(line.likely_oom).toBe(true);
});
});

describe("readProcessMemoryKb", () => {
const SMAPS = [
"Rss: 1624780 kB",
"Pss: 1019518 kB",
"Shared_Clean: 1028192 kB",
"Private_Dirty: 554892 kB",
"Anonymous: 554892 kB",
].join("\n");

it("parses Rss and Anonymous out of smaps_rollup", () => {
expect(readProcessMemoryKb(1, () => SMAPS)).toEqual({
rss_kb: 1_624_780,
anon_kb: 554_892,
});
});

it("returns null off Linux, where procfs does not exist", () => {
expect(
readProcessMemoryKb(1, () => {
throw new Error("ENOENT");
}),
).toBeNull();
});

it("returns null for output with neither field", () => {
expect(readProcessMemoryKb(1, () => "Pss: 12 kB")).toBeNull();
});
});
Loading