diff --git a/docs/10-tracing-observability.md b/docs/10-tracing-observability.md index 4568202e..188b1916 100644 --- a/docs/10-tracing-observability.md +++ b/docs/10-tracing-observability.md @@ -39,7 +39,7 @@ During cancellation: Trace finalization persists independently with a detached context (`context.WithoutCancel`), 5-second timeout, and exponential backoff retry (3 tries, max 10 total via retry queue). This ensures trace status writes don't fail silently due to cancellation. -Stale recovery worker runs every 30 seconds and catches zombie traces (stuck in "running" state) older than 10 minutes. Threshold is conservative because it measures against `start_time`, not last activity — a tighter value would kill healthy long-running agent runs. (Follow-up: add `last_span_at` column to enable aggressive recovery of inactive runs.) Context values (traceID, collector) survive cancellation — only `ctx.Done()` and `ctx.Err()` change. +Stale recovery is **currently disabled**. The implementation exists but the background loop is not started in `Collector.Start()`. Reason: the sweep condition is `start_time < NOW() - threshold`, which measures trace age rather than inactivity. Any threshold low enough to be useful (2–10 min) would kill healthy long-running agent runs (research chains, large code generation, extended shell commands). Re-enable only after adding a `last_span_at` column so recovery can gate on "no activity for N minutes" instead of "started > N min ago". Until then, zombie traces from gateway crashes may remain `running` in the DB — an accepted safety-net gap traded against false kills. The primary abort path (router 2-phase abort + `trace.status` WS event) handles the common case. Context values (traceID, collector) survive cancellation — only `ctx.Done()` and `ctx.Err()` change. --- diff --git a/internal/tracing/collector.go b/internal/tracing/collector.go index 66ecd082..cb0f6a5b 100644 --- a/internal/tracing/collector.go +++ b/internal/tracing/collector.go @@ -146,15 +146,28 @@ func (c *Collector) SetStatusBroadcaster(b StatusBroadcaster) { c.broadcastStatus = b } -// Start begins the background flush loop, retry worker, and stale recovery loop. +// Start begins the background flush loop and retry worker. +// +// NOTE: staleRecoveryLoop is intentionally NOT started. The current implementation +// sweeps traces by `start_time`, which would kill legitimate long-running agent +// runs (research chains, large code generation, long shell commands routinely +// exceed 10 minutes). Re-enable only after adding a `last_span_at` column so +// recovery can gate on "no activity for N minutes" instead of "started > N min +// ago". Until then, crashed/orphaned traces may remain `running` in DB — the +// primary abort path (router 2-phase + trace.status WS event) handles the +// common case; this is a safety-net gap we accept over false kills. func (c *Collector) Start() { - c.wg.Add(3) // flushLoop + retryWorker + staleRecoveryLoop + c.wg.Add(2) // flushLoop + retryWorker (staleRecoveryLoop disabled — see note above) go c.flushLoop() go c.retryWorker() - go c.staleRecoveryLoop() + // go c.staleRecoveryLoop() // disabled: would kill healthy long runs. See Start() godoc. slog.Info("tracing collector started") } +// keep staleRecoveryLoop reachable to silence "unused" linter; re-enabled in +// Start() once last_span_at-based recovery lands. +var _ = (*Collector).staleRecoveryLoop + // Stop gracefully shuts down the collector, flushing remaining spans. func (c *Collector) Stop() { close(c.stopCh)