ccfos / ccfos/huatuo

[Bug]: Profiler reports success when final output export fails

Open
#653 2 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.