dylibso / dylibso/observe-sdk

AdapterBase.Stop() can drop a trace event that's still in flight when called

Open
#176 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
188
Forks
8
PR merge metrics
No merged PRs in 30d

Description

`AdapterBase`'s consumer goroutine (in `Start()`) races the trace-event channel against the stop signal:

```go
go func() {
for {
select {
case <-ctx.Done():
return
case event := <-b.TraceEvents:
a.HandleTraceEvent(event)
case <-b.stop:
return
}
}
}()
```

`Stop()` sends to `b.stop` immediately:

```go
func (b *AdapterBase) Stop(wait bool) {
b.stop <- true
if wait {
b.eventBucket.Wait()
}
}
```

If a trace event is still being finalized and pushed onto the buffered `TraceEvents` channel when `Stop()` is called, there's a real race between that send and `stop` arriving at the `select`. When `stop` wins, the goroutine returns immediately - the pending event is never read, never reaches `HandleTraceEvent`/the event bucket, and is silently lost. `Stop(true)`'s `wait` guarantee (waiting for the event bucket to flush) doesn't help here, since the event never made it into the bucket in the first place.

**Reproduction:** call a wasm function through an instrumented plugin, then call `adapter.Stop(true)` right after the call returns (instead of sleeping an arbitrary amount of time first). The trace for that call is dropped intermittently/reliably depending on timing - because "the call returned" and "the trace event has been pushed to `TraceEvents`" aren't actually synchronized with each other.

This is separate from the `EventBucket.Wait()`/`Add()` ordering issue (a related but distinct bug in `bucket.go`, which I'm sending as its own fix).

**Possible directions** (not proposing a specific one - this touches real design tradeoffs for the project): `Stop()` could drain `TraceEvents` until empty before honoring `stop`, or `HandleTraceEvent`/the trace-finalization path could give the caller an explicit "this instance's events are all queued" signal to wait on before calling `Stop()` at all.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by inspecting AdapterBase.Start(), AdapterBase.Stop(), the TraceEvents channel, and the consumer goroutine shown in the issue; reproduce the race by calling an instrumented wasm function and immediately invoking Stop(true). Compare the shutdown behavior with the event-finalization path, and consider the stated drain or explicit-queueing alternatives. Done means Stop(true) no longer loses an event that was still being queued.

Written by the indexing model from the issue text.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.