fix(debug): buffer pre-init events and flush to real logger on init#448
fix(debug): buffer pre-init events and flush to real logger on init#448HiddenPuppy wants to merge 2 commits intomainfrom
Conversation
When getDebugLogger() is called before initDebugLogger(), events were silently dropped with noop stubs that returned empty DebugEvent objects. This fix implements a buffered noop logger that: - Captures up to 256 events in memory before initialization - Warns to console with buffer size when events are logged before init - Flushes all buffered events to the real logger when initDebugLogger runs Also adds unit tests for the pre-init buffering behavior. Fixes #412
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness of the debug logging system by preventing the silent loss of log events that occur during application startup. By introducing a temporary buffer, the system now ensures that diagnostic information generated before the logger is fully initialized is preserved and subsequently processed once the logger becomes available. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Hi @HiddenPuppy, DetailsInstructions for interacting with me using comments are available here. |
There was a problem hiding this comment.
Code Review
This pull request introduces a pre-initialization buffering mechanism for the debug logger in the Electron main process, ensuring that events captured before the logger is fully initialized are preserved and flushed later. Feedback focuses on improving security by sanitizing logs before they are buffered in memory, correcting the buffer eviction logic to drop the oldest events first as intended, and improving test portability by using platform-agnostic temporary directories.
| @@ -1,32 +1,106 @@ | |||
| import { BrowserWindow } from 'electron'; | |||
| import type { DebugEvent } from '@clawwork/shared'; | |||
| import type { DebugEvent, LogEventInput } from '@clawwork/shared'; | |||
There was a problem hiding this comment.
Import sanitizeForLog as a value to ensure that buffered events are properly sanitized before being stored in memory, preventing potential leakage of sensitive information.
| import type { DebugEvent, LogEventInput } from '@clawwork/shared'; | |
| import { sanitizeForLog, type DebugEvent, type LogEventInput } from '@clawwork/shared'; |
|
|
||
| const flushToLogger = (logger: DebugLogger): void => { | ||
| for (const event of preInitBuffer) { | ||
| logger.log({ ...event, level: event.level as LogEventInput['level'] }); |
There was a problem hiding this comment.
The cast and object spreading are redundant here. DebugEvent is compatible with the logger's log method input, and the original timestamp will be preserved during the spread inside the real logger's implementation.
| logger.log({ ...event, level: event.level as LogEventInput['level'] }); | |
| logger.log(event); |
| const event: DebugEvent = { | ||
| ts: new Date().toISOString(), | ||
| level, | ||
| domain: input.domain, | ||
| event: input.event, | ||
| traceId: input.traceId, | ||
| feature: input.feature, | ||
| message: input.message, | ||
| gatewayId: input.gatewayId, | ||
| sessionKey: input.sessionKey, | ||
| taskId: input.taskId, | ||
| runId: input.runId, | ||
| requestId: input.requestId, | ||
| wsFrameId: input.wsFrameId, | ||
| seq: input.seq, | ||
| attempt: input.attempt, | ||
| durationMs: input.durationMs, | ||
| ok: input.ok, | ||
| error: input.error, | ||
| data: input.data, | ||
| }; | ||
|
|
||
| if (preInitBuffer.length < MAX_PRE_INIT_BUFFER) { | ||
| preInitBuffer.push(event); | ||
| } |
There was a problem hiding this comment.
This block can be simplified while addressing two issues:
- Security: Sanitizing events before buffering prevents sensitive data (like tokens) from sitting un-redacted in memory or being exposed via
getRecentEvents()before initialization. - Logic: The PR description states that the oldest events should be dropped first, but the current implementation drops new events once the buffer is full.
const event = sanitizeForLog({
...input,
ts: new Date().toISOString(),
level,
} as DebugEvent);
preInitBuffer.push(event);
if (preInitBuffer.length > MAX_PRE_INIT_BUFFER) {
preInitBuffer.shift();
}| @@ -0,0 +1,75 @@ | |||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | |||
|
|
||
| expect(isDebugLoggerInitialized()).toBe(false); | ||
|
|
||
| const tempDir = `/tmp/test-debug-${Date.now()}`; |
- Import sanitizeForLog to redact sensitive data before buffering - Fix buffer eviction: now drops oldest events (shift) instead of ignoring new ones - Simplify flushToLogger to pass event directly - Add test for oldest-event eviction behavior Review feedback from #448
Summary
getDebugLogger()is called beforeinitDebugLogger(), events were silently dropped with noop stubs that returned emptyDebugEventobjectsinitDebugLoggeris calledChanges
packages/desktop/src/main/debug/index.tscreateNoopLogger()factory that creates a logger with an in-memory bufferisDebugLoggerInitialized()export for testing[debug] Logger not initialized yetmessage and buffer sizeinitDebugLogger()runspackages/desktop/test/debug/debug-logger.test.tsTest plan
pnpm --filter @clawwork/desktop testpassespnpm lintpassesFixes #412