feat: log rotation

[alexhancock]

Simple log rotation to prevent runaway log file storage. Will fix #4965 and #4365

Approach:

• Rotates log content to new files daily
• Deletes log files after two weeks
• Sets up a new llm subdirectory for llm-request logs so they can use the same cleanup strategy

Notes:

• There wasn't a super natural place to call the cleanup function for llm requests, so I coupled calls to it when we do cleanup for cli and server dirs
• This will require a one time manual rm of logs at the top level of the log dir, as I moved llm-requests to a new subdir, but after that should work for everything automatically

[Copilot]

Pull Request Overview

This PR reorganizes the logging structure by moving LLM request logs into a dedicated llm subdirectory and implementing automated cleanup of old log files. The changes establish better log organization and prevent log directory bloat by removing files older than two weeks.

Key changes:

• LLM request logs are now stored in logs/llm/ instead of directly in logs/
• New cleanup_old_logs() function removes logs older than 14 days
• Log rotation changed from NEVER to DAILY for both CLI and server

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
crates/goose/src/providers/utils.rs	Updates LLM request log paths to use logs/llm subdirectory
crates/goose/src/logging.rs	Adds cleanup_old_logs() function for removing logs older than 14 days
crates/goose-server/src/logging.rs	Integrates log cleanup calls and switches to daily log rotation
crates/goose-cli/src/logging.rs	Integrates log cleanup calls and switches to daily log rotation

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment incorrectly states 'older than one week', but the code on line 41 implements a two-week threshold (14 * 24 * 60 * 60 seconds). Update the comment to match the actual implementation.

Suggested change

	// If the file/directory is older than one week, remove it
	// If the file/directory is older than two weeks, remove it

[sheikhlimon]

I think it solves this #4365 one too... i was working on this then saw new requirements.. so I put it on hold..

[DOsinga]

we actually have two setup_logging methods, one for the server and one for the cli. the get_log_directory is shared though. I think the right move would be to either just have one and use the name here (don't put it in a server or cli folder, just have a server.log, cli.log and an mcp.log) but that's a rather bigger refactor. or we can just rename this function to prepare_log_directory that also does the clean up? definitely need to set up the log rotation also for the cli though

[DOsinga]

I don't think this works - we were previously doing our own timestamping, so this is just going to produce log files of the form:

20251104_143052.log.2025-11-04

so we can use the rotation, but then just get rid of the old timestamp code or just leave it as is

[DOsinga]

I'd drop the context

[DOsinga]

I would only remove dirs here - that's what we expect and if there are files, maybe somebody put them there for a reason? i.e. copied a log file out of the sub folder for save keeping? also not sure about logging inside of a logger

[Copilot]

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

crates/goose/src/providers/utils.rs:521

• Calling prepare_log_directory in the finish method triggers log cleanup on every request completion. This could cause unnecessary filesystem operations and performance overhead for high-frequency requests. Consider caching the logs directory path in the RequestLog struct during initialization in the start method, or implementing a rate-limiting mechanism for cleanup operations.

        self.write_json(&serde_json::json!({

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The error result from cleanup_old_logs is silently discarded. While this may be intentional to prevent cleanup failures from blocking log directory preparation, it would be better to log the error for debugging purposes. Consider using if let Err(e) = cleanup_old_logs(component) { tracing::warn!(\"Failed to cleanup old logs: {}\", e); } to provide visibility into cleanup failures.

Suggested change

	let _ = cleanup_old_logs(component);
	if let Err(e) = cleanup_old_logs(component) {
	tracing::warn!("Failed to cleanup old logs: {}", e);
	}

[Copilot]

The cleanup_old_logs function is missing documentation. Add a doc comment explaining its purpose, parameters, and behavior (e.g., that it removes log files and directories older than 14 days).

Suggested change

	/// Removes log files and directories older than 14 days for the specified component.
	///
	/// # Arguments
	///
	/// * `component` - The component name (e.g., "cli", "server", "debug") whose log directory will be cleaned.
	///
	/// # Behavior
	///
	/// Scans the log directory for the given component and removes any files or subdirectories
	/// whose last modification time is older than 14 days. Ignores missing directories.
	///
	/// Returns `Ok(())` on success, or an error if the log directory cannot be read.

[alexhancock]

@DOsinga Addressed all feedback. Reaping now works the same way for all three of cli, server, llm using a unified prepare_log_directory which also does cleanup. File structure now looks like:

13:38:55 ~/.local/state/goose $ tree logs
logs
├── cli
│   └── 2025-11-04
│       ├── 20251104_132253.log
│       └── 20251104_132300.log
├── llm
│   └── 2025-11-04
│       ├── llm_request.0.jsonl
│       ├── llm_request.1.jsonl
│       ├── llm_request.2.jsonl
│       └── llm_request.3.jsonl
└── server
    └── 2025-11-04
        ├── 20251104_133833-goosed.log
        └── 20251104_133844-mcp-developer.log

The content of the llm ones is a little different in the .jsonl format so I left the files themselves alone for that, but made the directory structure nicer.

[Copilot]

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Errors from cleanup_old_logs are silently ignored. This could hide issues like permission problems or I/O errors. Consider logging a warning when cleanup fails or propagating the error if it's critical. Example: cleanup_old_logs(component).ok() with a log message, or remove the let _ = if the error truly doesn't matter.

[Copilot]

The cleanup logic only removes directories but ignores files. For components like 'llm' that create .jsonl files directly in date-based subdirectories, old files will never be cleaned up. Either remove the is_dir() check to also clean up files, or add an else branch to handle files: else { let _ = fs::remove_file(&path); }

Suggested change

	let _ = fs::remove_dir_all(&path);
	let _ = fs::remove_dir_all(&path);
	} else {
	let _ = fs::remove_file(&path);

[Copilot]

Errors from fs::remove_dir_all are silently ignored. If a directory cannot be removed (e.g., due to permissions or a file being in use), the cleanup will silently fail and the old logs will accumulate. Consider logging a warning or debug message when removal fails.

Suggested change

	let _ = fs::remove_dir_all(&path);
	if let Err(e) = fs::remove_dir_all(&path) {
	eprintln!("Warning: Failed to remove directory {:?}: {}", path, e);
	}

[DOsinga]

we should undo this; the llm logs are special and huge; keep only 10 of them and we send them out with the diagnostics

[alexhancock]

will do