[pi0x]

Follows up on the crash-safety concern raised on #266 ("missed logs prior to a crash"). I stress-tested the batched logger against real process deaths (child processes on Node 22/24, Deno, Bun) and closed the gaps that are actually closable.

What the stress test showed

Scenarios where every buffered line already arrived before this PR: clean exit, same-tick process.exit(), uncaught exception, unhandled rejection (Node runs exit hooks on its default crash path), and SIGTERM/SIGINT with srvx graceful shutdown active.

Scenarios that lost the final event-loop turn of lines:

• SIGHUP/SIGTERM/SIGINT with default handling — no listener anywhere, so the runtime terminates without emitting exit. Happens when graceful shutdown is off (explicit false, the CI/TEST env carve-out, or log() used standalone with a non-srvx server). Fixable — fixed here.
• SIGKILL / OOM killer / V8 heap OOM (SIGABRT) — no userland code runs. Not fixable by hooks, but the pre-perf(log): batch stdout writes and cache the timestamp #266 unbatched logger did survive these for already-logged lines (each write lands in the kernel pipe buffer synchronously). Addressed via an opt-out.

Changes

• Signal flush hooks. hookExit() now also registers a SIGHUP/SIGTERM/SIGINT listener (signal-exit style): if it turns out to be the only listener at delivery time, it flushes synchronously, removes itself and re-raises — the process still dies by the signal with exit semantics untouched (verified: parent observes signal: SIGTERM, not an exit code). When any other listener exists (e.g. srvx graceful shutdown), it defers entirely and the regular flush path delivers the lines. Registration uses prependListener so a process.once(sig, cleanup) registered earlier is still counted at delivery time — once wrappers self-remove before dispatch, and an appended handler would miscount them as absent and re-raise mid-cleanup. The listeners are unref'd on all three runtimes, so an idle process still exits naturally (covered by a test).
• batch: false option. Writes each line in the request's own turn instead of coalescing per event-loop turn — narrows a hard kill's loss window to the write in flight, matching the pre-perf(log): batch stdout writes and cache the timestamp #266 logger (verified: lines survive a same-tick SIGKILL on Node, Deno and Bun). It funnels through the shared buffer + drain logic, so it respects backpressure and can never overtake lines a batched logger is still holding; flush() gained a mid-drain guard so a stale scheduled flush can't feed a full stream when both modes are mixed.

Verification

All child-process scenarios re-run after the change — buffered lines delivered in every runnable path:

scenario	before	after
default-handled SIGHUP / SIGTERM / SIGINT	❌ lost	✅ flushed, dies by signal
SIGTERM with graceful shutdown / other listener (incl. once)	✅	✅ (hook defers)
same-tick process.exit(), uncaught exception, natural end	✅	✅
same-tick SIGKILL, batch: false	❌ lost	✅ in kernel buffer
same-tick SIGKILL, batched	❌	❌ (≤ 1 turn, inherent)

Tests spawn test/fixtures/log-crash.mjs and assert on what actually reaches the pipe plus how the process died — crash-flush behavior only shows in a process that really terminates. Suite also gained batch: false write/backpressure/mixed-mode cases and listener cleanup so repeated log.ts imports don't trip the max-listeners warning.

An independent clean-context review validated the design and surfaced the once race, the missing SIGHUP, the mid-drain guard and a doc overclaim — all addressed in the second commit (details in the comment below).

pnpm test green (1215 passed), lint and types clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added a batch option for request logging.
  • Logging is batched by default; setting batch: false writes each request log immediately.
  • Added crash-safe flushing for buffered logs during normal exits, signals, and uncaught exceptions.

• Documentation

  • Expanded middleware logging guidance with batching examples and shutdown behavior.

• Bug Fixes

  • Improved reliability of log output during process termination and stdout backpressure.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1aca0b5d-6306-4661-8192-e4296acb987f

📥 Commits

Reviewing files that changed from the base of the PR and between b645cd4 and 52efdee.

📒 Files selected for processing (2)

• docs/1.guide/4.middleware.md
• test/log.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• docs/1.guide/4.middleware.md

---

📝 Walkthrough

Walkthrough

Changes

The logging middleware adds configurable batching, synchronous flushing for buffered output, signal-aware shutdown handling, documentation, and tests covering immediate writes, backpressure, listener cleanup, and process termination.

Logging batching and shutdown behavior

Layer / File(s)	Summary
Batch option and emission selection src/log.ts, docs/1.guide/4.middleware.md	Adds LogOptions.batch, selects batched or immediate emission, and documents batching and crash safety.
Buffered writes and shutdown hooks src/log.ts	Adds immediate writes, preserves buffering during backpressure, synchronously flushes pending output, and handles termination signals.
Emission and termination coverage test/log.test.ts, test/fixtures/log-crash.mjs	Tests batching, backpressure, listener cleanup, and buffered output across exit and signal scenarios.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• h3js/srvx#266: Both changes modify logging buffering and shutdown flush behavior.

Suggested reviewers: pi0

Poem

A rabbit logs softly, line after line,
Batches hop neatly in orderly time.
When signals come knocking, the buffers all flow,
Through stdout’s burrow in one steady show.
“Hop!” says the logger, “No crumbs left behind!”

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly reflects the main logger changes: flushing on default-handled SIGTERM/SIGINT and adding the batch option.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/log-crash-flush

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/srvx@274

commit: 52efdee

[pi0x]

Ran an independent clean-context review over the implementation; it confirmed the core mechanism but found four issues, fixed in b645cd4:

1. process.once race in the sole-listener check. A once cleanup handler registered before the logger self-removes ahead of dispatch, so the logger's appended handler saw listenerCount === 1 and re-raised mid-cleanup. The handlers are now registered with prependListener, so they run before any once wrapper disappears from the count. Covered by the new once-cleanup fixture scenario.
2. SIGHUP was missing — it default-terminates without exit exactly like SIGTERM/SIGINT (terminal close is its common delivery), and the docs claimed only hard kills could drop lines. Added to the hook and to the fixture tests.
3. Mixed batched + unbatched flush invariant. A flush scheduled by a batched line could fire after an unbatched write had started a drain wait, feeding the full stream and stacking a duplicate drain listener. flush() now early-returns while draining (which also let writeNow drop its own guard). Pinned by the new mid-drain test.
4. Doc overclaim. batch: false lines still buffer during a drain wait, and a piped stdout write is asynchronous — so the honest guarantee is "narrows a hard kill's loss window to the write in flight", not "never buffered". Wording adjusted in the JSDoc and guide.

Full suite green after the changes (1215 passed), scenarios re-verified on Node, Deno and Bun.

🤖 Generated with Claude Code

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (4)

test/log.test.ts (2)

241-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise batch: false through a real pipe and immediate exit.

This mock proves call timing only; it cannot reveal asynchronous stdout buffering. Add a child scenario using oversized output followed by same-tick process.exit() to validate the documented durability guarantee. (nodejs.org)

🤖 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 `@test/log.test.ts` around lines 241 - 266, Add a child-process scenario
alongside the existing batch-disabled test that runs the real logging path
through a pipe, emits oversized output, and calls process.exit() in the same
tick. Assert the parent receives the complete log output, validating the
durability guarantee for log({ batch: false }); retain the current call-timing
assertions.

---

366-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover a one-shot signal listener registered before log().

The current persistent listener is added afterward, so it misses the ordering case where a prior once listener removes itself before the logger checks listenerCount. (nodejs.org)

🤖 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 `@test/log.test.ts` around lines 366 - 370, Add coverage for a SIGTERM one-shot
listener registered before log() in the handled-sigterm test setup. Ensure the
once listener executes and removes itself before the logger’s persistent
listener checks listenerCount, while preserving the expected URLs and successful
exit code assertions.

test/fixtures/log-crash.mjs (2)

6-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a batch-disabled immediate-exit scenario.

Instantiate log({ batch: false }), emit enough data to exercise pipe buffering, and exit in the same tick so the child test validates actual persistence rather than mocked invocation timing.

🤖 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 `@test/fixtures/log-crash.mjs` around lines 6 - 10, Update the fixture’s
middleware setup to instantiate log with batch disabled, then emit enough
requests or log data to exercise pipe buffering and terminate the child process
in the same tick. Keep the scenario focused on validating persisted output
rather than mocked invocation timing.

---

21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a one-shot handler registered before log().

A scenario using process.once("SIGTERM", ...) before constructing the middleware reproduces the listener-ordering failure and verifies graceful-shutdown deferral.

🤖 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 `@test/fixtures/log-crash.mjs` around lines 21 - 25, Extend the
"handled-sigterm" scenario in the log-crash fixture by registering a one-shot
SIGTERM handler with process.once before constructing or invoking the middleware
log(). Keep the existing graceful-shutdown assertions and ensure this
pre-registered handler verifies the logger defers shutdown rather than
force-killing the process.

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/log.ts`:
- Around line 125-149: Ensure the non-batched logging path does not rely on
asynchronous stdout buffering before exit: update writeNow()/flushSync() in
src/log.ts (lines 125-149) to use a synchronous write end-to-end, or narrow the
documented durability guarantee to batched logging. Update the guarantee wording
in docs/1.guide/4.middleware.md (lines 79-83) accordingly, and add or adjust the
batch:false crash coverage in test/log.test.ts (lines 241-266) and
test/fixtures/log-crash.mjs (lines 6-10) if synchronous durability remains
promised.
- Around line 173-191: Update the signal ownership logic in src/log.ts lines
173-191 so it detects listeners registered before the logger before one-shot
handlers can detach, rather than relying only on the current listenerCount(sig);
preserve the existing flush and re-raise behavior for logger-owned signals. Add
coverage for a once-before-log() handler in test/log.test.ts lines 366-370 and
test/fixtures/log-crash.mjs lines 21-25.

---

Nitpick comments:
In `@test/fixtures/log-crash.mjs`:
- Around line 6-10: Update the fixture’s middleware setup to instantiate log
with batch disabled, then emit enough requests or log data to exercise pipe
buffering and terminate the child process in the same tick. Keep the scenario
focused on validating persisted output rather than mocked invocation timing.
- Around line 21-25: Extend the "handled-sigterm" scenario in the log-crash
fixture by registering a one-shot SIGTERM handler with process.once before
constructing or invoking the middleware log(). Keep the existing
graceful-shutdown assertions and ensure this pre-registered handler verifies the
logger defers shutdown rather than force-killing the process.

In `@test/log.test.ts`:
- Around line 241-266: Add a child-process scenario alongside the existing
batch-disabled test that runs the real logging path through a pipe, emits
oversized output, and calls process.exit() in the same tick. Assert the parent
receives the complete log output, validating the durability guarantee for log({
batch: false }); retain the current call-timing assertions.
- Around line 366-370: Add coverage for a SIGTERM one-shot listener registered
before log() in the handled-sigterm test setup. Ensure the once listener
executes and removes itself before the logger’s persistent listener checks
listenerCount, while preserving the expected URLs and successful exit code
assertions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8dbfae63-eca0-4032-bb16-891c2e4fd445

📥 Commits

Reviewing files that changed from the base of the PR and between c239925 and bfac4c6.

📒 Files selected for processing (4)

• docs/1.guide/4.middleware.md
• src/log.ts
• test/fixtures/log-crash.mjs
• test/log.test.ts