Batch Guardian log writes and drain them on shutdown

[rksharma-owg]

What changed

Guardian envelope and session-context logs currently call appendFileSync on the request path. Batch their existing JSONL records through bounded asynchronous append streams, and drain both streams after active requests finish during shutdown.

Each sink flushes after 64 records or 100 ms from its first queued record. Queued and in-flight writes count toward limits of 4 MiB and 4,096 records. Overflow drops and counts only the new record, preserves queued records, and accepts later traffic when capacity returns. The first drop and shutdown total are reported. A real disk error disables the sink and reports once without changing the policy decision.

SIGINT and SIGTERM use one memoized guardian.close() path, with trace export enabled or disabled. Closing refuses new ACS requests and gives active requests and both log drains one negotiated timeout (currently five seconds), then shuts down the trace exporter within its separate deadline. An expired drain is reported and pending connections are force-closed. Abrupt termination, disk errors, and expired deadlines can lose records, as documented in the Guardian README.

Log schemas and record ordering stay the same; visibility is delayed. This batches existing reference-implementation logs and retains #244's OpenTelemetry exporter. It does not implement the remaining acceptance criteria of #90.

Which issue does this implement

Refs #90

Base branch

• integration, because this touches a reference implementation and tests

Type of change

• Reference implementation or adapter
• Documentation

I tested this

• Merged current integration: 8a459524530b320c70c68d1d0b85bbcabb3771e6
• uv run pytest -v: 918 passed, 1 skipped locally; 919 passed on Linux in the fork
• uv run mkdocs build --strict: passed
• From reference-implementations/agt, bun run typecheck: passed
• From reference-implementations/agt, bun test with Bun 1.3.11: 1,251 passed, 4 existing skips, 0 failed locally and in the fork

The overflow-recovery regression fails against the previous PR head and passes with the correction. Tests cover thresholds, sparse traffic, record ordering, UTF-8 byte and record limits including in-flight writes, overflow recovery, throwing reporters, disk errors, file permissions, active-request shutdown, forced deadline shutdown, and standalone SIGINT/SIGTERM with tracing on and genuinely unset tracing variables.

The four existing Bun skips cover the optional UPSTREAM_BUNDLE comparison and proxy/runtime regressions unsupported on Bun 1.3.11. The local Python skip requires a case-sensitive filesystem and passes on Linux.

Fork preflight passed for contribution db8a3e8b2acad30ea2cf33f7e82a2d3642aeb783 and the unchanged integration baseline: complete Bun tests/type check, Python guards, strict documentation build, landing page, 44 schemas, and roadmap fixture. The baseline passed 1,231 Bun tests with the same four skips and 919 Python tests. The fork-only workflow is excluded from this PR. Upstream checks are reported separately from this fork validation.

Checklist

• Commits are signed off with git commit -s (required by the DCO)
• Prose follows STYLE.md
• No secrets, tokens, or internal URLs in the diff

Security

This changes log buffering and shutdown durability. It does not change policy decisions or report a vulnerability. Buffering is bounded; records can be lost under the documented overflow, failure, and shutdown conditions.

[rocklambros]

Hi @rksharma-owg, this is a heads-up about #244, the OpenTelemetry trace exporter. It touches the same shutdown code as this PR.

#244 adds a process.once("SIGINT"/"SIGTERM") handler at the end of main.ts, but only when trace export is on. It also makes close() in server.ts call traceExporter.shutdown() after server.stop(true). This PR adds an unconditional signal handler and a memoized, draining close() in the same places.

Whichever PR lands second should end up with one signal handler, the unconditional one from this PR. It should also have one memoized close() that stops the server gracefully, drains both logs, and then calls traceExporter.shutdown(). Two handlers that each call process.exit(0) would race, and the first to finish could cut the other's drain short. The top-level README.md gap paragraph is also edited by both PRs, so that needs merging too.

Nothing to change right now. I wanted both PR authors to know before it turns into a surprise merge conflict. I left the same note on #244.

[Lrd0036]

Since this replaces the envelope-log and session-context-log writes, could the new paths create their directory with mode: 0o700 and their files with mode: 0o600?

mkdirSync(dirname(path), { recursive: true, mode: 0o700 });
createWriteStream(path, { flags: "a", mode: 0o600 });

That's the same fix Rock asked #244 to use for its OTLP file, and it closes the file-mode half of #71 inside this rewrite rather than in a follow-up that would conflict with it. mode applies only when a file or directory is created, so logs and an .acs/ directory left by earlier runs keep their old permissions.

[rocklambros]

@rksharma-owg, a note for when you rebase this onto integration. Since this PR was opened, PR #244 changed close() in packages/guardian/src/server.ts to wait for in-flight requests inside a Promise.race with a deadline, so a stuck request can't hold shutdown forever. Please keep that race when you resolve the conflict, and run your log drain inside the same deadline. Separately, issue #274 is changing server.stop(true) to server.stop() inside that race, so in-flight responses are delivered. That change is one token and shouldn't conflict with yours. Your server.ts and main.ts currently conflict with integration, so a rebase is needed either way.

[github-actions]

This pull request's description says it closes #90, which is on the project roadmap. When it merges, GitHub closes that issue and the roadmap counts it as delivered. Please confirm the change delivers the whole issue before merging.

[rksharma-owg]

@rocklambros, updated in a52aef5 by merging current integration and resolving the shutdown conflicts with #244.

There is now one unconditional signal handler and one memoized close(). Graceful server.stop() and both log drains share the negotiated request deadline (currently five seconds), followed by trace exporter shutdown with its existing three-second deadline. An expired drain is reported. The owner-only creation modes remain covered, and existing file/directory permissions are preserved.

The two new deadline regressions fail before the correction and pass afterwards. SIGINT/SIGTERM tests cover trace export enabled and disabled. Fork validation passed for this commit and the unchanged integration baseline. The updated commit passed 1,247 Bun tests with two existing skips, type checking, and 919 Python guards in the fork. Upstream reference implementation checks and Python guards/site build also passed with the same test counts; Pages deployment is intentionally skipped for PRs.

Could you please re-review the updated PR?

[President]

@rksharma-owg hi,
Good change: decisions no longer wait for disk writes.

BUT 3 things:

1. A full buffer can stop all logging until the Guardian restarts.
   batchLogWrites destroys the stream when a new record would exceed 4 MiB or 4096 records.
   Queued records can be lost too.

Reproduced with a test stream taking 50 ms per write:
five ~1 MB lines at once, then one small line a second later.
Result: one "log buffer limit exceeded" error, zero lines written.

In a running Guardian, about five requests near the 1 MiB limit can cause the same failure.
The buffer fills when requests arrive faster than the disk writes them.

I'd skip and count the new record when the buffer is full:

• queued records still get written
• new records are accepted again once there is space
• a real disk error may still close the log

(#282 asks for the same recovery for trace outputs: count dropped records and recover.)

2. Shutdown without traces is never tested.
   The "trace export off" case in test/log-shutdown.test.ts sets ACS_OTEL_FILE and ACS_OTEL_ENDPOINT to "".
   server.ts enables the trace exporter when either value is not undefined.
   Empty strings therefore keep traces on.

Remove both keys from the child environment for this case (also the ones inherited from process.env).

3. Merging this PR would close #90 for different work.
   #90 asks for span batching, and #244 already added it (BatchSpanProcessor).
   This PR batches log writes.
   I'd change "Closes #90" to "related to #90".

[President]

@rksharma-owg - thanks, all three fixed in db8a3e8. Looks good from my side.