Skip to content

per-run-log-pid-suffix#1783

Closed
aboimpinto wants to merge 2 commits into
Hmbown:mainfrom
aboimpinto:fix/pid-log-suffix
Closed

per-run-log-pid-suffix#1783
aboimpinto wants to merge 2 commits into
Hmbown:mainfrom
aboimpinto:fix/pid-log-suffix

Conversation

@aboimpinto
Copy link
Copy Markdown
Contributor

@aboimpinto aboimpinto commented May 18, 2026

Problem

Multiple concurrent TUI instances write to the same log file (tui-YYYY-MM-DD.log), causing interleaved entries and file contention.

Fix

Append process ID to log filename: tui-YYYY-MM-DD-<PID>.log.

One line change in crates/tui/src/runtime_log.rs.

Closes #1782

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the TUI logging strategy by appending the process ID to the log filename, which prevents log clobbering when multiple instances are running simultaneously. Review feedback highlights that this change may lead to excessive log accumulation and recommends implementing a pruning mechanism for old logs, as well as updating the module documentation to reflect the new behavior.

let log_path = log_dir.join(format!("tui-{date}.log"));
// Per-run suffix avoids multiple concurrent TUI instances writing to the
// same file. Process ID is unique enough per boot.
let log_path = log_dir.join(format!("tui-{date}-{}.log", std::process::id()));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Including the PID in the log filename prevents concurrent TUI instances from clobbering the same log file. However, this change introduces a few concerns:\n\n1. Log Accumulation: This shifts the logging strategy from 'daily-rolling' to 'per-run'. Without an automated pruning mechanism (similar to the one used for spillover files in main.rs), this will lead to an unbounded accumulation of log files in ~/.deepseek/logs/ over time.\n2. Documentation Mismatch: The module-level documentation (lines 1-4) still describes the logging as 'daily-rolling', which is now inaccurate.\n\nConsider implementing a pruning task to remove logs older than a certain threshold (e.g., 7 days) to maintain disk health.

@aboimpinto aboimpinto mentioned this pull request May 18, 2026
@aboimpinto
Copy link
Copy Markdown
Contributor Author

Good point about log accumulation. Two thoughts:

1. This isn't a new problem. The existing single-file approach already accumulates — the old tui-YYYY-MM-DD.log file was 3MB after one day of development. PID files don't create more total bytes; they just partition them into debuggable per-instance files instead of one multi-agent mess where nobody can trace their own session.

2. Pruning is a separate concern. I've opened Issue #1784 to track it. Proposed: delete logs older than 7 days (configurable via env, 7-day default). That's the right scope for a follow-up PR rather than blocking this one-liner.

I'll push a doc update to the module comment now.

@Hmbown Hmbown mentioned this pull request May 20, 2026
11 tasks
@Hmbown
Copy link
Copy Markdown
Owner

Hmbown commented May 21, 2026

Thanks for the per-run log naming fix. This was harvested into v0.8.40 release PR #1823 as commit a595edd, and #1823 is now CI-green. Closing as superseded by the release branch; thank you for helping make logs easier to inspect.

@Hmbown Hmbown closed this May 21, 2026
@Hmbown Hmbown mentioned this pull request May 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PID-log-prefix

2 participants