Skip to content

🪦 feat: Log Request Tombstones and Memory Watermarks, Closing Out Abandoned Requests as 499 - #197

Open
danny-avila wants to merge 1 commit into
mainfrom
pr/redo-request-tombstones-memory-watermarks
Open

danny-avila wants to merge 1 commit into
mainfrom
pr/redo-request-tombstones-memory-watermarks

Conversation

@danny-avila

Copy link
Copy Markdown
Contributor

Pull Request Template

⚠️ Before Submitting a PR, Please Review:

  • Please ensure that you have thoroughly read and understood the Contributing Docs before submitting your Pull Request.

⚠️ Documentation Updates Notice:

  • Kindly note that documentation updates are managed in this repository: librechat.ai

Summary

The panel's Bun server logs its two startup lines and nothing else: no line per request and no memory readings. Memory only shows up when Prometheus scrapes /metrics. When the process crashes or is OOMKilled, nothing records which request it was handling or how memory grew before it. This PR supersedes #102 by @dustinhealy. It keeps that PR's request tombstones and memory watermarks, written fresh on today's main. It also closes out requests the client abandons, which #102 left open on current main.

Request tombstones. Each request logs [req] GET /path when it arrives, before the handler runs, and [req] GET /path 200 12ms once it is answered. An arrival line with no completion line names a request the process died holding. Carried over from #102:

  • Only the path is logged, never the query string, which can carry OAuth exchange codes. Paths are cut at 200 characters.
  • At most 200 requests are logged per 10-second window. The next window opens with [req] suppressed N requests in prior window, so a request flood cannot turn into a log flood but is still visible.
  • ADMIN_PANEL_REQUEST_LOG=false turns the log off. The metrics are recorded either way.

Abandoned requests close out as 499. The completion line and the HTTP metrics use one status: 499 when req.signal.aborted is set by the time the handler returns (the rule #181 added to the metrics), otherwise the response's own status.

  • A request the client abandoned before it was answered is closed out at once, and its body is not wrapped.
  • A streamed body is closed out once, on whichever comes first: the body drains, the body fails (stream-error), or the request's abort signal fires (client-closed, with the status the response was sent with).

#102 waited only on the response body. Bun neither reads nor cancels the body of a request the client abandoned, so on current main every abandoned server function left an arrival line with no completion line, which looks exactly like a crash. #102 also counted those requests as 500. In the live check below, #102's approach left 9 of 9 forced aborts open. With this PR, none were left open.

Bodies left alone. A HEAD response and the Bun.file static routes are logged at once and returned untouched. Bun sends no body for HEAD: it reads one chunk and then cancels the stream, so a wrapped HEAD body would end in a misleading client-closed. Wrapping a file body would lose Bun's sendfile path and the Content-Length it derives from the file.

Every other body passes through a JavaScript stream, and Bun cannot tell its length. Server-function responses, which main sends with Content-Length, now go out with Transfer-Encoding: chunked. Browsers and TanStack's client read both the same way, SSR pages were already chunked, and #102 did the same.

Memory watermarks. Every 10 seconds the server compares RSS with ADMIN_PANEL_MEMORY_LOG_THRESHOLDS_MB (default 256,384,448 MiB). Each time RSS climbs past a threshold it logs [mem] rss=391Mi heapUsed=213Mi (crossed 384Mi), reporting the highest threshold crossed. A threshold logs again only after RSS drops back below it. This records when memory jumped even if no request was in flight at the time.

Where it lives. The wrapper and its helpers are in src/server/logging.ts, and its types are in src/types/server.ts. server.ts is excluded from tsc and ESLint, so the code moved into src/server, where tsc, ESLint and Vitest check it. server.ts only wires the module up, and the Docker image already copies src/server. Both variables are documented in .env.example.

Not carried over from #102.

  • The Bun.serve error() hook and the catch/rethrow-500 path. Bun 1.3.11 and later already log an uncaught handler throw with its stack and answer 500. TanStack Start also catches handler errors itself and answers 500, so handler.fetch does not reject to Bun.
  • isProbeRequest. /health is a static route and never reaches the wrapper, and neither does /metrics.

Change Type

  • New feature (non-breaking change which adds functionality)

Testing

Unit tests in src/server/logging.test.ts (Node environment, real prom-client counters, console.log spied):

  • A client that leaves before the response: the original response comes back unwrapped, the completion line reads 499, and the metrics count 499 and no 500.
  • A client that leaves mid-stream when nothing reads or cancels the body: exactly one 200 Nms client-closed line, and a later cancel adds nothing. A cancel that lands while a read is pending also reports once.
  • A streamed body is logged only after it drains, and the query string never appears in the log. A body that fails mid-transfer ends in stream-error.
  • HEAD responses and file bodies (monitorBody: false) are returned untouched and closed out at once.
  • With the request log off, nothing is logged and the metrics still count.
  • Flood guard: the cap within a window, the suppressed count reported once when the next window opens (also several windows later), and the budget reset each window. Path truncation is covered too.
  • Memory: threshold parsing; watermark hysteresis (one log per upward crossing, highest threshold reported, re-armed only below the threshold); and the 10-second tick logs [mem] lines until it is stopped.

Each behaviour was checked against its own test by removing it. Without the abort listener, the mid-stream test fails. If abandoned bodies are wrapped, or the response status is recorded instead of 499, the 499 test fails. If HEAD or file bodies are wrapped, those tests fail. Without the settle-once guard, the two mid-stream tests fail.

Live check with a production build against a LibreChat backend:

Check Result
Two fresh sign-ins, each followed by /, /configuration, /access, /users and /grants 63 and 64 arrivals, each with exactly one completion
36 forced aborts of server functions (curl --max-time 0.003, with the session cookie) 32 completed as 499, 4 were answered within 3 ms (200); no arrival left open after 15 s; /metrics shows status_code="499" 32
The same aborts with #102's approach (body wrap only, no abort handling) 9 of 9 arrivals left open
A standalone Bun.serve running the same wrapper over a slow 10-chunk stream: full read, cut mid-stream, client gone before the handler answered, HEAD 200 1101ms, one 200 350ms client-closed, 499 150ms, HEAD closed out at once
A standalone Bun.serve: a HEAD answered with a raw stream, and Response.json with and without the wrapper Bun reads one chunk of the HEAD stream, then cancels it; Response.json goes out with Content-Length unwrapped and Transfer-Encoding: chunked wrapped
HEAD and GET of a static asset closed out at once, Content-Length kept
/login?redirect=/grants&code=… logged as [req] GET /login
230 requests in under 100 ms 200 logged, then [req] suppressed 30 requests in prior window
ADMIN_PANEL_MEMORY_LOG_THRESHOLDS_MB=50,80 [mem] rss=118Mi heapUsed=29Mi (crossed 80Mi) on the first tick; at the defaults, RSS (about 120 MiB) logs nothing
ADMIN_PANEL_REQUEST_LOG=false no [req] lines; the metrics still counted the aborts as 499

No console errors or failed requests while browsing.

Test Configuration:

  • Bun 1.4.0, TanStack Start 1.169.4, LibreChat dev, MongoDB replica set.
  • The panel's production build (bun run build + bun server.ts).
  • npx tsc --noEmit, npx eslint src/ e2e/ --max-warnings 0, npx prettier --check on the changed files, bun run test:lint-rules, npx vitest run (3366 tests), bun run build.

Checklist

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code
  • I have commented in any complex areas of my code
  • I have made pertinent documentation changes
  • My changes do not introduce new warnings
  • I have written tests demonstrating that my changes are effective or that my feature works
  • Local unit tests pass with my changes

…ndoned Requests as 499

The Bun server logged nothing per request and nothing about memory, so a
crash or OOMKill left no trace of the request in flight or of the climb that
preceded it. Each request now logs "[req] METHOD /path" on arrival, before the
handler runs, and "[req] METHOD /path STATUS Nms" once it is answered, so an
arrival without a completion names a request the process died holding. Paths
are logged without query strings and cut at 200 characters, and at most 200
requests are logged per 10 seconds, with the number held back reported when
the next window opens. ADMIN_PANEL_REQUEST_LOG=false turns the log off.

The log and the HTTP metrics record one status: 499 when the client abandoned
the request before it was answered, else the response's own. Bun neither reads
nor cancels the body of an abandoned request, so such a request is closed out
at once instead of waiting on its body, and a streamed body settles on the
first of drain, error, or the request's abort signal, once. HEAD responses and
the Bun.file static routes are logged at once and their bodies left untouched.
Every other body passes through a stream Bun cannot size, so server-function
responses now go out chunked instead of with a Content-Length.

A 10-second check logs "[mem] rss=... heapUsed=... (crossed ...)" each time
RSS climbs past one of ADMIN_PANEL_MEMORY_LOG_THRESHOLDS_MB (default
256,384,448 MiB); a threshold re-arms once RSS drops back below it.

The wrapper and its helpers live in src/server/logging.ts, where tsc, eslint
and vitest cover them; server.ts only wires them up. Both variables are
documented in .env.example.

Builds on the request tombstones and memory watermarks proposed in #102.
@danny-avila

Copy link
Copy Markdown
Contributor Author

@codex review

Pass 1. Head: 1799910

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-27T09:37:48.015948Z 1799910 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 1799910909

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant