Skip to content

fix(network): surface launcher stderr on premature exit in background mode#676

Draft
lwshang wants to merge 3 commits into
mainfrom
fix/597-background-launcher-stderr
Draft

fix(network): surface launcher stderr on premature exit in background mode#676
lwshang wants to merge 3 commits into
mainfrom
fix/597-background-launcher-stderr

Conversation

@lwshang

@lwshang lwshang commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

What

Follow-up to #675 for issue #597. In background mode the native launcher's stderr is redirected to a log file rather than inherited, so when it exits prematurely (e.g. its fixed gateway port is already in use by Docker) the user only saw:

network launcher at ... exited prematurely with status 101

with no indication of why.

This reads the launcher's captured stderr back and folds the tail into the LauncherExitedPrematurely error, so the cause travels with the error:

... exited prematurely with status 101
Launcher error output (from .../network-launcher/stderr.log):
<tail>

How

  • New premature_exit_detail(background, stderr_file) in launcher.rs, called from the premature-exit arm of the select!:
    • Foreground: returns empty — stderr is already inherited and streamed live by fix: Stop suppressing launcher stdio #675, so the detail is shown, just rendered differently. No change.
    • Background: reads stderr.log, tails it (last 50 lines, then 8 KB, cut on a char boundary), and embeds it. Best-effort: on an unreadable/empty file it falls back to pointing at the log path rather than masking the exit status.
  • Adds a detail: String field to the error variant and appends {detail} to the #[snafu(display)].

Tests

  • Unit tests for the tail bounds (line cap, byte cap, trimming), the foreground no-op, the captured-output path, and the unreadable-file fallback.
  • A #[cfg(unix)] integration test that occupies a port with a raw TcpListener (which passes icp-cli's own descriptor-based port check but makes the launcher fail to bind the gateway), runs network start --background, and asserts the captured output is attached to the error without pinning the exact launcher / pocket-ic wording. It also reaps any process spawned under the test's isolated temp home so it doesn't leak an orphaned pocket-ic.

Scope / follow-ups

Uses Refs #597 rather than Fixes because two related items are intentionally out of scope and are tracked separately:

  1. Docker launcher pathspawn_docker_launcher has the same class of bug via a separate ContainerExitedPrematurely error and a different mechanism (needs docker logs, and can't be covered by this unix/native integration test). → network start: Docker launcher discards container output on premature exit (same as #597 for native) #677
  2. Orphaned pocket-ic on the failure path — the launcher spawns pocket-ic and only cleans it up on the success path; on error/panic it's left running. → pocket-ic left orphaned when launcher exits on the error/panic path icp-cli-network-launcher#76

🤖 Generated with Claude Code

… mode

In background mode the native launcher's stderr is redirected to a log
file rather than inherited, so when it exits prematurely (e.g. its fixed
gateway port is already taken) the user only saw "exited prematurely with
status 101" with no cause.

Read the log tail back and fold it into the LauncherExitedPrematurely
error (bounded to the last 50 lines / 8 KB, char-boundary safe, fail-soft
if the file is unreadable). Foreground is unchanged: its stderr is already
inherited and streamed live (#675), so the detail suffix stays empty there.

Adds a unix-only integration test that occupies a port with a raw TCP
listener (which passes icp-cli's own descriptor-based port check but makes
the launcher fail to bind), runs `network start --background`, and asserts
the captured output is attached to the error without pinning the exact
launcher / pocket-ic wording.

Refs #597

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

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.

Pull request overview

Surfaces captured native launcher errors when background startup fails prematurely.

Changes:

  • Appends a bounded stderr tail to premature-exit errors.
  • Adds unit and integration coverage for error reporting.
  • Cleans up orphaned test processes.

Reviewed changes

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

File Description
crates/icp/src/network/managed/launcher.rs Reads, bounds, and reports launcher stderr.
crates/icp-cli/tests/network_tests.rs Adds port-conflict regression coverage.

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

Comment thread crates/icp-cli/tests/network_tests.rs Outdated
Comment thread crates/icp-cli/tests/network_tests.rs
lwshang and others added 2 commits July 24, 2026 20:57
The test is #[cfg(unix)], but TcpListener was imported at module scope,
so on Windows (where the test is compiled out) it was an unused import
and failed the build under -D warnings. Move the import into the test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Require the "Launcher error output" header instead of also accepting the
  empty/unreadable-log fallback, so the integration test fails if the real
  launcher stderr is never read/wired (the header is our own string, so it
  still doesn't depend on pocket-ic's wording).
- Move the orphaned-pocket-ic cleanup into an RAII guard that reaps on Drop,
  so it still runs when an assertion panics — the failure path is exactly the
  one that leaves pocket-ic orphaned.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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