feat: introduce log directory size limit (unbounded disk usage prevention)#9234
feat: introduce log directory size limit (unbounded disk usage prevention)#9234markolazic01 wants to merge 6 commits intoChainSafe:unstablefrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 969a83db34
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| handleExceptions: true, | ||
| maxFiles: opts.file.dailyRotate, | ||
| // ensure that there are always at least 2 files in rotation. | ||
| maxFiles: maxFileSizeInBytes != null ? Math.max(opts.file.dailyRotate || 2, 2) : opts.file.dailyRotate, |
There was a problem hiding this comment.
This change forces maxFiles to at least 2 when size limiting is enabled, but startup cleanup still prunes using the raw args.logFileDailyRotate value (cleanOldLogFiles in the same module). With --logFileDailyRotate 1 (and default size cap enabled), runtime rotation keeps 2 files while restart cleanup can delete anything older than 1 day, removing the backup file the new logic is explicitly trying to preserve.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This behavior seems reasonable to me, as it happens per user specific choice. Choosing --logFileDailyRotate 1 will still provide additional protection during the day, the deletion afterwards can be considered to be what the user wants.
Will keep as is but also remain open for further discussion.
![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 introduces a new configuration option, logFileDirMaxSize, to limit the total size of rotated log files by calculating a per-file size limit. Feedback highlights that the total size limit might not be strictly enforced if a very high number of rotation files are configured due to the 1MB minimum per file. Additionally, a suggestion was made to simplify the logic that ensures at least two files are kept in rotation when a size limit is applied.
|
Resolved a lint error from previous action run |
1 participant
Motivation
Introduce a CLI flag that sets a log directory size limit, preventing unbounded disk usage during error storms (as seen in #7561 where daily logs reached 50-60GB).
Description
Adds a new CLI flag --logFileDirMaxSize (default 2048MB) that caps the total size of the log directory by computing a per-file size limit and passing it to winston-daily-rotate-file.
The per-file limit is calculated as logFileDirMaxSize / maxFiles, ensuring the total across all rotated files stays within the budget. maxFiles is forced to a minimum of 2 so there is always at least one backup file available for diagnosis when the current file rotates.
This feature only applies when daily rotation is enabled (the default). When rotation is disabled (--logFileDailyRotate 0), the size limit is not enforced because winston.transports.File does not properly support maxFiles — it either keeps a single file that grows indefinitely, or deletes the only file when it reaches the limit, leaving no logs to diagnose issues.
Tested using scripts that directly exercise the logger rotation and size limit behavior.
Closes #7561
AI Assistance Disclosure
Used Claude to ping-pong on a solution, discover potential edge-cases, for code validation.