Skip to content

feat: enable /logs API endpoint in standalone (Docker CE) mode#824

Open
ilopezluna wants to merge 2 commits intomainfrom
enable-logs-endpoint-standalone
Open

feat: enable /logs API endpoint in standalone (Docker CE) mode#824
ilopezluna wants to merge 2 commits intomainfrom
enable-logs-endpoint-standalone

Conversation

@ilopezluna
Copy link
Copy Markdown
Contributor

When MODEL_RUNNER_LOG_DIR is not set, auto-create a default log directory /tmp/model-runner-logs and set up tee logging so that all output goes to both stderr and bracket- timestamped log files (for the /logs API endpoint).

Changes:

  • Add BracketWriter to pkg/logging that prefixes each line with [timestamp] in the format expected by MergeLogs
  • Auto-create log directory and open service + engine log files
  • Use io.MultiWriter to tee slog output to stderr and log files
  • Configure ServerLogFactory for engine backend logs
  • Always enable /logs endpoint when a log directory is available

When MODEL_RUNNER_LOG_DIR is not set, auto-create a default log
directory (/tmp/model-runner-logs) and set up tee logging so that
all output goes to both stderr (for docker logs) and bracket-
timestamped log files (for the /logs API endpoint).

Changes:
- Add BracketWriter to pkg/logging that prefixes each line with
  [timestamp] in the format expected by MergeLogs
- Auto-create log directory and open service + engine log files
- Use io.MultiWriter to tee slog output to stderr and log files
- Configure ServerLogFactory for engine backend logs
- Always enable /logs endpoint when a log directory is available
- Fix comments that incorrectly stated Docker Desktop uses
  MODEL_RUNNER_LOG_DIR (it uses its own log infrastructure)
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 enables the /logs API endpoint across all deployment modes by automatically creating a default log directory and implementing file-backed logging using a new BracketWriter. The changes improve operational visibility and debugging capabilities, supported by new unit tests and goroutine leak detection. A critical issue was identified in pkg/logging/logging.go at lines 117-123, where the BracketWriter.Write method risks data loss by clearing its internal buffer before confirming a successful write to the underlying writer; the buffer must only be advanced after the write succeeds.

Move buffer advancement after the successful write to the underlying
writer. Previously, the line was removed from the buffer before the
write, so a failed write (e.g., full disk) would lose the log entry.

Also avoids an unnecessary string allocation by passing the byte
slice directly to fmt.Fprintf.
@ilopezluna ilopezluna marked this pull request as ready for review April 2, 2026 14:43
@ilopezluna ilopezluna requested a review from a team April 2, 2026 14:43
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The new LogDir() comment suggests the server auto-creates a default directory, but this behavior actually lives in main; consider updating the comment (or moving the logic) so the function comment accurately reflects its behavior.
  • BracketWriter currently buffers until a newline is seen and never flushes partial lines, which can cause unbounded memory growth and dropped logs on process exit; consider adding a Flush/Close path or a max-buffer safeguard.
  • Engine logging uses a shared *os.File with separate BracketWriters created per backend; if multiple backends write concurrently, their prefix+line writes to the shared file may interleave—consider either sharing a single BracketWriter per file or serializing access at a higher level.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new LogDir() comment suggests the server auto-creates a default directory, but this behavior actually lives in main; consider updating the comment (or moving the logic) so the function comment accurately reflects its behavior.
- BracketWriter currently buffers until a newline is seen and never flushes partial lines, which can cause unbounded memory growth and dropped logs on process exit; consider adding a Flush/Close path or a max-buffer safeguard.
- Engine logging uses a shared *os.File with separate BracketWriters created per backend; if multiple backends write concurrently, their prefix+line writes to the shared file may interleave—consider either sharing a single BracketWriter per file or serializing access at a higher level.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

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.

2 participants