Skip to content

internal/process: surface upstream output on premature exit - #897

Open
davidbarnhart wants to merge 1 commit into
mostlygeek:mainfrom
davidbarnhart:fix/surface-model-load-errors
Open

internal/process: surface upstream output on premature exit#897
davidbarnhart wants to merge 1 commit into
mostlygeek:mainfrom
davidbarnhart:fix/surface-model-load-errors

Conversation

@davidbarnhart

Copy link
Copy Markdown

When a model's process exits before becoming ready, clients received the
opaque "upstream command exited prematurely" with none of the upstream
server's output, so a bad launch (for example an invalid argument) could not
be diagnosed from the client. Include the captured process log so the real
cause reaches the caller; because WaitReady subscribers share the start
result, every waiting request benefits too.

  • prematureExit now appends processLogger.GetHistory() when non-empty
  • clear the process log before each launch so only the current attempt's
    output is surfaced, not accumulated history from earlier launches

fixes #896

When a model's process exits before becoming ready, clients received the
opaque "upstream command exited prematurely" with none of the upstream
server's output, so a bad launch (for example an invalid argument) could not
be diagnosed from the client. Include the captured process log so the real
cause reaches the caller; because WaitReady subscribers share the start
result, every waiting request benefits too.

- prematureExit now appends processLogger.GetHistory() when non-empty
- clear the process log before each launch so only the current attempt's
  output is surfaced, not accumulated history from earlier launches
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 82f8e231-bfdf-4ec8-86d3-b31c611f76a8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 5, 2026

Copy link
Copy Markdown

Greptile Summary

This PR surfaces the captured process output in the "upstream command exited prematurely" error, making startup failures diagnosable from the client. It also clears the circular log buffer before each launch so only the current attempt's output appears in the error.

  • prematureExit now appends processLogger.GetHistory() to the error message when the buffer is non-empty, and all WaitReady subscribers share this richer result.
  • processLogger.Clear() is called immediately before cmd.Start() so accumulated output from earlier restarts cannot bleed into a new attempt's failure message.

Confidence Score: 5/5

Safe to merge. The change is small and well-scoped: two call sites in doStart, both guarded by the existing cmdDone-closed invariant that ensures all subprocess I/O has drained before GetHistory is read.

Clear() is placed before cmd.Start() so no subprocess output can arrive before the buffer is reset. GetHistory() is called only inside prematureExit(), which is reachable only after cmdDone closes — and cmdDone is closed after cmd.Wait() returns, which drains all I/O goroutines. The bufferMu mutex in Monitor correctly serializes any concurrent writes (e.g., from a cmd.Cancel callback racing with GetHistory) so there is no data race. The new test exercises the full path with a real subprocess and checks both the error category and the surfaced output text.

No files require special attention.

Important Files Changed

Filename Overview
internal/process/process_command.go Adds processLogger.Clear() before cmd.Start() and enriches prematureExit() with GetHistory(); ordering and synchronization are correct because cmdDone is closed only after cmd.Wait() (which drains all I/O), so GetHistory() is always called after all subprocess writes have completed.
internal/process/process_command_test.go Adds TestProcessCommand_PrematureExitSurfacesOutput, which passes an unknown flag to the simple-responder binary and verifies both the premature-exit error text and that the upstream flag error appears in the returned error message.

Reviews (1): Last reviewed commit: "internal/process: surface upstream outpu..." | Re-trigger Greptile

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.

Model load failures return opaque "upstream command exited prematurely" without upstream output

1 participant