coze-dev / coze-dev/cozeloop-go

Span setters can race with Finish and crash the exporter

Open
#39 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
58
Forks
20
PR merge metrics
No merged PRs in 30d

Description

## Description

`Span.Finish` is not fully serialized with span mutations, while the trace exporter reads several mutable span maps directly. A setter that starts before `Finish` can therefore complete after the span has been marked finished and handed to the processor, racing with asynchronous export.

This can terminate the entire process with:

```text
fatal error: concurrent map iteration and map write
```

Even when it does not panic, the exporter may observe an inconsistent combination of tags, input/output metadata, baggage, duration, or size information.

## Root cause

There are two related synchronization gaps.

### 1. Setter/Finish lifecycle TOCTOU

Setters such as `SetTags` check `isFinished` before acquiring `Span.lock`. The check and mutation are not one atomic lifecycle operation:

1. A setter observes `isFinished == false` and is descheduled before acquiring the lock.
2. `Finish` marks the span finished, finalizes it, and invokes the span processor.
3. The setter resumes, acquires the lock, and mutates the span after it has become exportable.
4. The asynchronous exporter reads the same state concurrently.

The early atomic check is useful as a fast path, but it cannot replace checking the finished state again after acquiring the span lock.

### 2. Exporter reads mutable maps directly

`transferToUploadSpanAndFile` and its helpers directly access:

- `TagMap`
- `SystemTagMap`
- `multiModalityKeyMap`

`parseTag` ranges over these maps, while input/output conversion also performs direct lookups. Those reads do not participate in the span locking protocol.

## Impact

- A concurrent map read/write can cause an unrecoverable runtime fatal error rather than a recoverable panic.
- A finished span can be mutated after its finalization boundary.
- Composite updates such as prompt, baggage, and multimodal input/output metadata can be only partially visible to `Finish` or the exporter.
- Other concurrently read fields, including baggage, duration, and accumulated byte size, can also be observed inconsistently.

## Reproduction

A focused stress test can overlap exporter conversion with a late, lock-protected map mutation. Without synchronization, repeated execution reproduces the runtime fatal error; with `go test -race`, reads in exporter conversion/tag parsing race with span map writes.

The lifecycle gap can also be exercised by running multiple setters concurrently with multiple `Finish` calls and asserting that:

- `OnSpanEnd` runs exactly once;
- no setter changes span state after `Finish` returns;
- the race detector reports no conflicting access.

## Expected behavior

`Finish` must be the linearization boundary for span mutation:

- a setter either commits completely before finalization, or observes the finished state under the lock and performs no mutation;
- all final derived fields are calculated from one locked state;
- the processor is invoked only after the span lock is released;
- the exporter consumes one consistent snapshot instead of traversing live mutable maps.

## Proposed fix

- Serialize the `isFinished` transition and final span derivation with `Span.lock`.
- Make every setter re-check `isFinished` after acquiring the lock.
- Commit composite setters such as input/output, prompt, and baggage as one locked operation.
- Capture `TagMap`, `SystemTagMap`, and `multiModalityKeyMap` as one read-locked exporter snapshot and use it throughout conversion.
- Protect the related baggage, duration, and byte-size read paths with the same locking discipline.
- Add regression tests for exporter snapshot safety, post-`Finish` immutability, and concurrent setters versus `Finish`.

## Validation

The focused trace production sources and the three new regression tests pass repeated normal and race-detector runs:

```text
normal concurrency tests: 50 iterations passed
race-detector tests: 20 iterations passed
focused go vet: passed
```

The package-wide `go test ./internal/trace` command is currently blocked by pre-existing test compilation failures on `main`: one test refers to `UploadSpan` without the `entity` qualifier, and two tests call the previous four-argument `NewBatchSpanProcessor` signature.

Contributor guide

Open the contributing guide

Research direction

Trace Span.Finish, the span setters, transferToUploadSpanAndFile, and parseTag first, then inspect how Span.lock protects their shared state. Run the focused concurrency and race-detector tests described in the issue. Done means Finish is the mutation boundary, exporters consume one consistent snapshot, setters cannot mutate afterward, and OnSpanEnd runs once without race reports.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, observability-sre
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.