[Bug]: Profiler reports success when final output export fails
- Dominant language
- Go
- Stars
- 1.1k
- Forks
- 137
- Avg merge
- 3d 16h
- Merged PRs (30d)
- 18
Description
### 这个问题是否已经存在?
- [x] 我已经搜索过现有的问题和拉取请求,包括开放、关闭和已合并的条目。
### 部署环境
- Kernel: Linux (the defect is independent of a specific kernel version)
- Architecture: amd64
- OS: Ubuntu under WSL2 for the deterministic reproduction
- Go: 1.24.13
- HUATUO: `upstream/main` at `a1e74a8ef58499c98368e61fa892f168e989bb87`
### 如何复现
#### Affected production path
Local profiler output follows this call chain:
```text
cmd/profiler/run.go:runAction
-> internal/profiler/registry.Profile
-> aggregator.NewPipeline(...).Start()
-> Pipeline.Stop()
-> Pipeline.runAggregateSnapshot()
-> Pipeline.aggregateAndSnapshot(..., true)
-> writeFolded(...) or writeFlameGraph(...)
```
`runAggregateSnapshot` currently handles the final export like this:
```go
if err := p.aggregateAndSnapshot(p.pctx.Ctx, true); err != nil {
p.logAggregateExportError(err)
}
```
`Pipeline.Stop` has no error result:
```go
func (p *Pipeline) Stop() {
// ...
p.wg.Wait()
}
```
`registry.Profile` therefore waits for the failed export but cannot observe it:
```go
pipe.Stop()
return err
```
If sampling and the profiler read loop finish successfully, `Profile` returns
`nil` even when the final folded or flame graph output was not written.
`cmd/profiler/main.go` then exits with status 0.
#### Deterministic reproduction
I added a temporary formatter that writes a prefix and then returns an error:
```go
type partialFailureFormatter struct{}
func (partialFailureFormatter) Write(w io.Writer) error {
if _, err := io.WriteString(w, "partial profile\n"); err != nil {
return err
}
return errors.New("formatter failed after partial write")
}
```
Calling the existing final-output helper reproduced the export failure on the
current upstream commit:
```text
=== RUN TestReproducePartialProfilePublication
write error: failed to write folded data: formatter failed after partial write
published artifact: /tmp/.../perf_1787580056_988610a5c1cdfd78.folded
("partial profile\n")
--- PASS: TestReproducePartialProfilePublication (0.00s)
PASS
```
The helper returns the expected write error. However, when invoked through the
normal pipeline lifecycle, `runAggregateSnapshot` only logs that error and
`Pipeline.Stop`/`registry.Profile` cannot return it to the caller.
The same loss occurs for other final-export errors, for example:
1. the output directory becomes read-only after sampling starts;
2. the filesystem reaches its quota or capacity during final serialization;
3. an SVG/folded formatter fails after processing part of the profile;
4. the final output file cannot be created or written;
5. an upload-mode pipeline fails its final snapshot or storage operation.
#### Actual behavior
- the final export error is written only to the log;
- `Pipeline.Stop` returns no result;
- `registry.Profile` may return `nil`;
- the profiler CLI may exit with status 0;
- automation can treat the profiling task as successful despite missing output;
- for a formatter failure after a partial write, a partial artifact can also be
left in the output directory, but atomic artifact publication is a separate
concern from the error-propagation defect reported here.
#### Expected behavior
- the pipeline must retain the final export result;
- `Pipeline.Stop` must wait for final aggregation/export and return its error;
- repeated or concurrent `Stop` callers must observe the same completed result,
rather than one caller returning before shutdown finishes;
- `registry.Profile` must propagate the pipeline export failure;
- the profiler CLI must exit non-zero when its requested output cannot be
produced;
- existing successful and empty-profile shutdown behavior must remain unchanged;
- periodic upload failures may continue to be logged and retried, but an
unresolved final export failure must reach the caller.
### 修复建议或方案
1. Give `Pipeline` a shutdown-completion result that is written by the final
aggregation worker before shutdown is declared complete.
2. Make `Pipeline.Stop()` return an error and ensure all callers wait for the
same shutdown completion, including repeated and concurrent callers.
3. Record the error from the final `aggregateAndSnapshot(..., true)` call. Keep
periodic snapshot logging behavior so transient periodic failures can still
be retried on later ticks.
4. Propagate the pipeline shutdown error through `registry.Profile`, preserving
an already-existing profiler/context/read-loop error with `errors.Join` rather
than silently replacing either cause.
5. Add regression coverage for:
- successful final export;
- deterministic formatter/export failure;
- repeated `Stop` calls returning the same result;
- concurrent `Stop` calls waiting for the same final export;
- `registry.Profile` returning the final export error;
- preservation of an existing profiler error together with the export error;
- upload/local output behavior that already succeeds.
### 其他
This is not the same problem as PR #442. PR #442 covers `pkg/tracing` task output
store errors after a tracing subprocess exits. This report concerns the separate
continuous profiler aggregation pipeline in `internal/profiler/aggregator` and
the `cmd/profiler -> registry.Profile` exit status.
The issue is operationally important for scripted profiling: callers generally
use the process exit status to decide whether an artifact is valid and available.
A log-only error is insufficient because logs may be redirected, sampled, or not
parsed by the task orchestrator.
Acceptance criteria:
- a failed final profile export causes `registry.Profile` to return a non-nil
error;
- the CLI consequently exits non-zero;
- every `Pipeline.Stop` caller waits for final export completion and receives the
same terminal error;
- successful shutdown behavior remains unchanged;
- regression tests pass under the race detector;
- the fix remains focused on error propagation and does not bundle unrelated
output-format or storage changes.
Contributor guide
Research direction
Start with internal/profiler/aggregator, tracing Pipeline.Stop, runAggregateSnapshot, and aggregateAndSnapshot to understand shutdown completion and final export errors. Then follow cmd/profiler/run.go through registry.Profile to cmd/profiler/main.go, and inspect existing profiler tests or add regression coverage for repeated and concurrent Stop calls. Done means final export failures reach registry.Profile and the CLI exits non-zero while successful shutdown remains unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- cli, observability-sre
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100