Skip to content

Commit 2605d28

Browse files
authored
fix(storage): make StreamActivities cancel-safe at the source (#1737)
## Hazard `storage.Manager.StreamActivities` walked the activity bucket inside a BBolt read transaction and sent every matching record into a channel buffered at 100, with no context or done channel. Any consumer that stopped reading early — the 24h profile stats loop breaking on `ctx.Err()`, an activity export whose client disconnected mid-stream — left the producer blocked on the send for the life of the process, holding the read transaction and the mmap read lock: `Stats().OpenTxN` stayed at 1, later writes could not remap the file, and `db.Close` hung on shutdown. #1718 worked around it in its own consumer with a drain goroutine (`drainActivities`); the two older consumers still leaked. ## Fix - `StreamActivities(ctx, filter)` at the source: the producer checks `ctx.Err()` before every send and selects on `ctx.Done()` around it, so cancellation returns from `db.View` promptly and closes the channel. Cancellation is not logged as a stream error. The signature changed everywhere (storage → runtime → server → httpapi controller interface and its test doubles); no external callers. - Consumers pass their request/loop context: the httpapi export cancels a per-request context on its write-error return, summary and scoped usage pass `r.Context()`, `Stats24h` and `Suggest` pass their `ctx`. `Stats24h` no longer caches a truncated rollup after cancellation; `Suggest` returns the context error after the loop instead of a partial window (a pre-cancelled context now yields zero records, so the in-loop check alone would have returned a clean empty result). - `drainActivities` and its test are removed. ## Tests - `internal/storage/activity_stream_cancel_test.go`: abandon the stream after one record with 300 matching records, assert `OpenTxN` returns to 0 and `Close` returns; the no-cancel variant proves the transaction really is pinned without cancellation (guards the first test against passing vacuously); pre-cancelled context yields nothing; live context streams all. The first test hung `Close` for the full timeout before the fix. - `TestProfilesService_Stats24hDoesNotCacheTruncatedRollup`, `TestSuggest_CancelledContextReturnsWithRecordsLeft` (updated). - Green under `-race`: `internal/storage`, `internal/httpapi`, `internal/runtime`. `golangci-lint` (CI config, bare and `--build-tags server`) reports only pre-existing findings in untouched lines.
1 parent 27eb24a commit 2605d28

17 files changed

Lines changed: 238 additions & 81 deletions

‎internal/httpapi/activity.go‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -747,8 +747,12 @@ func (s *Server) handleExportActivity(w http.ResponseWriter, r *http.Request) {
747747
w.Header().Set("Content-Disposition", "attachment; filename=\""+filename+"\"")
748748
w.Header().Set("Cache-Control", "no-cache")
749749

750-
// Stream activities
751-
activityCh := s.controller.StreamActivities(filter)
750+
// Stream activities. The context is cancelled on return, so a write
751+
// error (client gone) releases the producer's read transaction instead of
752+
// leaving it parked on a full channel for the life of the process.
753+
streamCtx, cancelStream := context.WithCancel(r.Context())
754+
defer cancelStream()
755+
activityCh := s.controller.StreamActivities(streamCtx, filter)
752756

753757
// Write CSV header if format is CSV
754758
if format == "csv" {
@@ -939,9 +943,9 @@ func (s *Server) handleActivitySummary(w http.ResponseWriter, r *http.Request) {
939943
toolCounts := make(map[string]int)
940944
perServer := make(map[string]*perServerAccumulator)
941945

942-
// The stream holds a read transaction open until the channel is drained or
943-
// closed, so this loop must always run to completion.
944-
for a := range s.controller.StreamActivities(filter) {
946+
// The producer releases its read transaction when the request context is
947+
// cancelled, so a client that disconnects mid-scan does not pin the store.
948+
for a := range s.controller.StreamActivities(r.Context(), filter) {
945949
totalCount++
946950

947951
// "How many rows" (totalCount) and "how many calls" (callCount) are
@@ -1398,7 +1402,7 @@ func (s *Server) handleActivityUsage(w http.ResponseWriter, r *http.Request) {
13981402

13991403
var resp *contracts.UsageAggregateResponse
14001404
if params.scopeFiltered() {
1401-
resp = s.scopedUsageResponse(params, time.Now().UTC())
1405+
resp = s.scopedUsageResponse(r.Context(), params, time.Now().UTC())
14021406
} else {
14031407
snap := s.controller.UsageSnapshot()
14041408
tokens, _ := s.controller.GetTokenSavings()
@@ -1416,7 +1420,7 @@ func (s *Server) handleActivityUsage(w http.ResponseWriter, r *http.Request) {
14161420
// admission rule instead. Per-tool metrics are therefore window-bounded here
14171421
// (lifetime-cumulative on the unfiltered path), and the global tokens-saved
14181422
// headline is omitted because it cannot be re-derived for a subset.
1419-
func (s *Server) scopedUsageResponse(p usageParams, now time.Time) *contracts.UsageAggregateResponse {
1423+
func (s *Server) scopedUsageResponse(ctx context.Context, p usageParams, now time.Time) *contracts.UsageAggregateResponse {
14201424
filter := storage.ActivityFilter{
14211425
// Every record Apply may admit, including the call_tool_* mirrors it
14221426
// drops itself: the population a cold-start rebuild scans.
@@ -1433,9 +1437,9 @@ func (s *Server) scopedUsageResponse(p usageParams, now time.Time) *contracts.Us
14331437
filter.AllowedServers = p.allowed
14341438
}
14351439
agg := internalRuntime.NewUsageAggregate()
1436-
// The stream holds a read transaction open until drained, so it always runs
1437-
// to completion.
1438-
for rec := range s.controller.StreamActivities(filter) {
1440+
// The producer releases its read transaction when the request context is
1441+
// cancelled, so a client that disconnects mid-scan does not pin the store.
1442+
for rec := range s.controller.StreamActivities(ctx, filter) {
14391443
agg.Apply(rec)
14401444
}
14411445
agg.UpdatedAt = now

‎internal/httpapi/activity_auth_identity_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package httpapi
22

33
import (
4+
"context"
45
"encoding/json"
56
"net/http"
67
"testing"
@@ -166,7 +167,7 @@ type streamingActivityController struct {
166167
mockActivityController
167168
}
168169

169-
func (m *streamingActivityController) StreamActivities(filter storage.ActivityFilter) <-chan *storage.ActivityRecord {
170+
func (m *streamingActivityController) StreamActivities(_ context.Context, filter storage.ActivityFilter) <-chan *storage.ActivityRecord {
170171
ch := make(chan *storage.ActivityRecord, len(m.activities))
171172
for _, a := range m.activities {
172173
if filter.Matches(a) {

‎internal/httpapi/activity_call_parity_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package httpapi
22

33
import (
4+
"context"
45
"encoding/json"
56
"net/http"
67
"net/http/httptest"
@@ -49,7 +50,7 @@ func (m *callParityController) GetCurrentConfig() *config.Config {
4950
return &config.Config{APIKey: "test-key", Observability: config.DefaultObservabilityConfig()}
5051
}
5152

52-
func (m *callParityController) StreamActivities(filter storage.ActivityFilter) <-chan *storage.ActivityRecord {
53+
func (m *callParityController) StreamActivities(_ context.Context, filter storage.ActivityFilter) <-chan *storage.ActivityRecord {
5354
ch := make(chan *storage.ActivityRecord)
5455
go func() {
5556
defer close(ch)

‎internal/httpapi/activity_scope_params_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package httpapi
22

33
import (
4+
"context"
45
"encoding/csv"
56
"encoding/json"
67
"net/http"
@@ -52,7 +53,7 @@ func (c *scopeParamsController) ListActivities(f storage.ActivityFilter) ([]*sto
5253
return out, total, nil
5354
}
5455

55-
func (c *scopeParamsController) StreamActivities(f storage.ActivityFilter) <-chan *storage.ActivityRecord {
56+
func (c *scopeParamsController) StreamActivities(_ context.Context, f storage.ActivityFilter) <-chan *storage.ActivityRecord {
5657
c.lastFilter = f
5758
ch := make(chan *storage.ActivityRecord, len(c.records))
5859
for _, r := range c.records {

‎internal/httpapi/activity_summary_scope_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package httpapi
22

33
import (
4+
"context"
45
"encoding/json"
56
"fmt"
67
"net/http"
@@ -49,7 +50,7 @@ func (m *summaryScopeController) ListActivities(filter storage.ActivityFilter) (
4950
return matched, total, nil
5051
}
5152

52-
func (m *summaryScopeController) StreamActivities(filter storage.ActivityFilter) <-chan *storage.ActivityRecord {
53+
func (m *summaryScopeController) StreamActivities(_ context.Context, filter storage.ActivityFilter) <-chan *storage.ActivityRecord {
5354
ch := make(chan *storage.ActivityRecord)
5455
go func() {
5556
defer close(ch)

‎internal/httpapi/contracts_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ func (m *MockServerController) AggregateToolUsage(_ time.Time) (map[string]stora
302302
return map[string]storage.ToolUsageStat{}, nil
303303
}
304304
func (m *MockServerController) UsageSnapshot() *internalRuntime.UsageAggregate { return nil }
305-
func (m *MockServerController) StreamActivities(_ storage.ActivityFilter) <-chan *storage.ActivityRecord {
305+
func (m *MockServerController) StreamActivities(_ context.Context, _ storage.ActivityFilter) <-chan *storage.ActivityRecord {
306306
ch := make(chan *storage.ActivityRecord)
307307
close(ch)
308308
return ch

‎internal/httpapi/scope_round9_harness_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package httpapi
22

33
import (
4+
"context"
45
"fmt"
56
"time"
67

@@ -93,7 +94,7 @@ func (c *scopeController) ListActivities(filter storage.ActivityFilter) ([]*stor
9394
return matched, total, nil
9495
}
9596

96-
func (c *scopeController) StreamActivities(filter storage.ActivityFilter) <-chan *storage.ActivityRecord {
97+
func (c *scopeController) StreamActivities(_ context.Context, filter storage.ActivityFilter) <-chan *storage.ActivityRecord {
9798
ch := make(chan *storage.ActivityRecord, len(scopeActivities()))
9899
for _, rec := range scopeActivities() {
99100
if filter.Matches(rec) {

‎internal/httpapi/security_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ func (m *baseController) AggregateToolUsage(_ time.Time) (map[string]storage.Too
352352
return map[string]storage.ToolUsageStat{}, nil
353353
}
354354
func (m *baseController) UsageSnapshot() *runtime.UsageAggregate { return nil }
355-
func (m *baseController) StreamActivities(_ storage.ActivityFilter) <-chan *storage.ActivityRecord {
355+
func (m *baseController) StreamActivities(_ context.Context, _ storage.ActivityFilter) <-chan *storage.ActivityRecord {
356356
ch := make(chan *storage.ActivityRecord)
357357
close(ch)
358358
return ch

‎internal/httpapi/server.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ type ServerController interface {
264264
// Activity logging (RFC-003)
265265
ListActivities(filter storage.ActivityFilter) ([]*storage.ActivityRecord, int, error)
266266
GetActivity(id string) (*storage.ActivityRecord, error)
267-
StreamActivities(filter storage.ActivityFilter) <-chan *storage.ActivityRecord
267+
StreamActivities(ctx context.Context, filter storage.ActivityFilter) <-chan *storage.ActivityRecord
268268
// AggregateToolUsage rolls up tool_call activity per (server,tool) since the
269269
// given time. Backs the global tools page usage columns (spec 050).
270270
AggregateToolUsage(since time.Time) (map[string]storage.ToolUsageStat, error)

‎internal/runtime/profiles_service.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -316,10 +316,7 @@ func (s *ProfilesService) Stats24h(ctx context.Context, viewer ViewerScope) Acti
316316
StartTime: now.Add(-24 * time.Hour),
317317
AllowedServers: filterServers,
318318
}
319-
for rec := range s.rt.StreamActivities(filter) {
320-
if ctx.Err() != nil {
321-
break
322-
}
319+
for rec := range s.rt.StreamActivities(ctx, filter) {
323320
blocked := rec.Status == "blocked"
324321
isCall := rec.Type == storage.ActivityTypeToolCall
325322
if !isCall && !blocked {
@@ -341,6 +338,10 @@ func (s *ProfilesService) Stats24h(ctx context.Context, viewer ViewerScope) Acti
341338
add(stats.ByProfile, rec.EffectiveProfile())
342339
add(stats.ByClient, rec.ClientID)
343340
}
341+
if ctx.Err() != nil {
342+
// The stream closed early: a truncated rollup is not worth caching.
343+
return stats
344+
}
344345

345346
s.statsMu.Lock()
346347
s.statsCache[key] = cachedStats{at: now, stats: stats}

0 commit comments

Comments
 (0)