fix(exec): preserve child exit code when OTLP export fails (#360) - #26
Conversation
There was a problem hiding this comment.
Pull request overview
Preserves the executed child process’ exit code when OTLP export/flush fails, so SoftFail doesn’t mask real command failures (fixes #360).
Changes:
- Capture
Diag.ExecExitCodebefore OTLP export/flush work occurs inexec. - Update
SoftFailto exit with the captured child exit code when it’s non-zero. - Add an integration fixture ensuring exit code propagates when export fails.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| otelcli/exec.go | Captures the child’s exit code earlier so later OTLP export failures don’t overwrite it. |
| otelcli/config.go | Makes SoftFail preserve a captured non-zero exec exit code (instead of defaulting to soft-fail semantics). |
| data_for_test.go | Adds a fixture validating child exit code propagation under export failure conditions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // #360: exec child exit code should propagate even when OTLP export fails | ||
| { | ||
| { | ||
| Name: "#360 exec child exit code preserved when export fails", | ||
| Config: FixtureConfig{ | ||
| CliArgs: []string{"exec", | ||
| "--endpoint", "{{endpoint}}", | ||
| "--timeout", "100ms", | ||
| "--", "/bin/sh", "-c", "exit 42", | ||
| }, | ||
| StopServerBeforeExec: true, | ||
| TestTimeoutMs: 2000, | ||
| IsLongTest: true, | ||
| }, | ||
| Expect: Results{ | ||
| Config: otelcli.DefaultConfig(), | ||
| }, | ||
| CheckFuncs: []CheckFunc{ | ||
| func(t *testing.T, f Fixture, r Results) { | ||
| if r.ExitCode != 42 { | ||
| t.Errorf("expected exit code 42 from child process, got %d", r.ExitCode) | ||
| } | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The test asserts the exit code, but it doesn’t assert that the OTLP export actually failed (the condition that triggers the regression). This can produce a false positive if export doesn’t error for some reason. Consider adding an assertion that the run logged/returned an export/client.Stop() error (e.g., checking captured stderr/output or a structured error field in Results, whichever the harness provides).
| CliArgs: []string{"exec", | ||
| "--endpoint", "{{endpoint}}", | ||
| "--timeout", "100ms", | ||
| "--", "/bin/sh", "-c", "exit 42", |
There was a problem hiding this comment.
Hard-coding /bin/sh makes this fixture non-portable (e.g., Windows runners). If CI/test execution targets multiple OSes, consider using an OS-appropriate command (or a small helper test binary) or adding a runtime/platform skip/guard in the fixture system for non-POSIX environments.
173f6e1 to
64eb757CompareSoftFail called os.Exit(0) or os.Exit(1) regardless of the child process exit code. When exec ran a command that exited 42, then OTLP export failed, the exit code was lost. Two changes: 1. Capture Diag.ExecExitCode before OTLP export (was after) 2. SoftFail checks Diag.ExecExitCode and uses it when non-zero Fixes #360 🤖 Claude <claude@anthropic.com>
64eb757 to
5ccedacCompareUh oh!
There was an error while loading. Please reload this page.
SoftFail lost child exit code when OTLP export failed. Now captures ExecExitCode before export and SoftFail uses it when non-zero. Fixes #360