fix(debug): replace appendFileSync with persistent WriteStream (#410)#489
fix(debug): replace appendFileSync with persistent WriteStream (#410)#489HiddenPuppy wants to merge 3 commits intoclawwork-ai:mainfrom
Conversation
…ork-ai#410) The debug logger was using appendFileSync on every log event, which blocks the Node.js main-process event loop with synchronous disk I/O. Under high traffic (reconnect storm, large streams, burst IPC) this causes cumulative latency and UI hitches. Changes: - Replace appendFileSync with a persistent fs.createWriteStream backed by a synchronously-opened fd (via openSync) so the log file exists immediately. - Cache the WriteStream for the current day; close and reopen on date change (daily rotation). - Add flush() method that returns a Promise resolved after all pending writes are flushed to the OS. - Wire flush() into the app.before-quit handler to drain pending debug log writes before exit. Fixes clawwork-ai#410
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 optimizes the desktop application's logging mechanism by transitioning from synchronous file operations to a non-blocking, persistent stream-based approach. This change significantly reduces UI hitches and latency caused by blocking I/O on the main thread, while adding necessary lifecycle management for log rotation and graceful shutdown. 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 the 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 counterproductive. 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the debug logger to use a persistent WriteStream instead of synchronous file appends, improving performance by not blocking the event loop. It also introduces a flush method to ensure logs are written to disk. Feedback highlights that the flush call during the application's shutdown sequence is not awaited, potentially leading to lost logs, and recommends adding an error handler to the WriteStream to prevent process crashes on I/O failures.
| isQuitting = true; | ||
| getDebugLogger().info({ domain: 'app', event: 'app.before-quit', data: { installingUpdate: isInstallingUpdate() } }); | ||
| // Flush pending debug log writes before exit | ||
| getDebugLogger().flush(); |
There was a problem hiding this comment.
The flush() method returns a Promise, but it is not being awaited here. Since app.on('before-quit') is a synchronous event handler in Electron, the application will proceed with its shutdown sequence and likely terminate before the asynchronous log writes are completed. To ensure logs are persisted, you should implement a mechanism to wait for the flush to complete before allowing the app to exit. In Electron, use event.preventDefault() to pause the standard shutdown sequence while the asynchronous operation completes.
References
- In Electron, use event.preventDefault() when custom handlers are needed to override or delay standard behavior.
| const filePath = currentFilePath(); | ||
| // Open fd synchronously so the file exists immediately and is append-only | ||
| const fd = openSync(filePath, 'a'); | ||
| writeStream = createWriteStream(filePath, { fd, autoClose: true }); |
There was a problem hiding this comment.
The WriteStream is created without an error handler. If an asynchronous write fails (e.g., due to disk space issues or permission changes), the stream will emit an 'error' event. In Node.js, an unhandled 'error' event on a stream will cause the process to crash. It is recommended to add a listener to handle potential I/O errors.
writeStream = createWriteStream(filePath, { fd, autoClose: true }).on('error', (err) => {
console.error('Debug logger stream error:', err);
});
|
@HiddenPuppy pls fix the ci error. |
clawwork-ai#410) - Add .on('error') handler to WriteStream so disk I/O failures don't crash the process - Fix app.before-quit handler: use event.preventDefault() to pause shutdown, await logger.flush(), then call app.quit() after log writes complete - Guard against re-entrant before-quit with isQuitting flag
|
Updated based on review:
|
The DebugLogger interface now requires flush(), but the pre-init stub logger in debug/index.ts was missing it. Add a no-op async flush that resolves immediately.
2 participants
Problem
createDebugLoggerinpackages/desktop/src/main/debug/logger.tsusesappendFileSyncon every log event — a synchronous call that blocks the Node.js main-process event loop. Under high traffic (reconnect storm, large stream, burst IPC), this causes cumulative latency and UI hitches.Fixes #410
Changes
Persistent
createWriteStream— replacesappendFileSyncwith an asyncWriteStreambacked by a synchronously-opened fd (openSync), so the log file exists immediately but writes are non-blocking.Daily rotation — the stream caches the current day's date; when the date changes, the old stream is
.end()ed and a new one is opened.flush()method — added to theDebugLoggerinterface. Resolves after all pending writes have been flushed to the OS via a zero-byte write marker callback. Enables deterministic test assertions and graceful shutdown.app.before-quitwire-up — callsgetDebugLogger().flush()before shutdown so pending log data isn't lost.Testing
debug-observability,debug-pre-init-buffer,debug-export)Performance
The original code performs 3+ syscalls per event (open + write + close via
appendFileSync). The fix performs 1 async libuvwrite()call with a cached fd. For 10,000 events: