Repository navigation
fix(storage): make StreamActivities cancel-safe at the source - #1737
Merged
Merged
Conversation
StreamActivities walked the activity bucket inside a BBolt read transaction and sent every match into a channel buffered at 100 with no way to stop. A consumer that stopped reading early (a cancelled 24h stats loop, an activity export whose client went away) left the producer blocked on the send for the life of the process, holding the read transaction and the mmap read lock: OpenTxN stayed at 1, writes could not remap, and db.Close hung. PR #1718 papered over it in its own consumer with a drain goroutine. The producer now takes a context, checks it before every send and selects on ctx.Done() around the send, so cancellation returns from db.View promptly and closes the channel. Every caller passes its request or loop context; the export path cancels on its write-error return; the 24h stats rollup no longer caches a truncated window and Suggest returns the context error instead of a partial result. drainActivities and its test are gone. New storage tests prove the leak (OpenTxN returns to 0, Close returns) and the full-stream path.
Deploying mcpproxy-docs with
|
| Latest commit: |
f26111d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://33b36a2f.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-xenodochial-driscoll.mcpproxy-docs.pages.dev |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 39795114035 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Dumbris
added a commit
that referenced
this pull request
Oct 23, 2026
… send (#1743) closes #1742 ## Hazard `storage.Manager.StreamActivities` walked the activity bucket inside one BBolt read transaction and sent each record into a buffered channel. #1737 made the send cancel-aware, but a consumer that stops reading **without cancelling** (an activity export to a client that keeps the connection open but no longer reads) still parked the producer inside `db.View`. The open read transaction pinned the mmap: the next write that had to grow the file waited on the remap lock for ever, and every later reader (agent-token lookups included) queued behind that pending writer. Result: all authenticated requests hang while `/healthz` keeps answering — the #1742 signature. ## Fix The producer never holds a read transaction across a channel send. Each short `db.View` collects at most 100 matching records (examining at most 1000 keys), leaves the transaction, then sends; the next batch resumes strictly before the last examined key (`Seek` + `Prev`, robust to that key being pruned). Offset, limit and newest-first order are unchanged. Records written after the stream starts are not guaranteed to appear, and records pruned between batches are skipped. ## Tests - `TestStreamActivities_StalledConsumerHoldsNoTransaction` (replaces the test that asserted the leak as the contract): a consumer that stops reading without cancelling leaves `OpenTxN == 0`. - `TestStreamActivities_StalledConsumerDoesNotBlockRemap`: with a stalled stream, a 16 MiB write (forces an mmap remap) completes, and the stream then delivers all 300 records. - `TestStreamActivities_BatchBoundaries`: order, offset/limit across batches, and a sparse filter past the per-transaction scan cap. Both stalled-consumer tests fail on `main` (the remap test times out at 5 s) and pass here. `storage`, `runtime`, `httpapi` pass with `-race`; golangci-lint v2 clean (bare and `-tags server`). ## Live verification Scratch core with a seeded DB (12k activity records, ~1 MB below the mmap boundary), a stalled export client, then a loop of writes + authenticated reads: - `main`: writes and authenticated reads hang after ~200–290 writes; the goroutine dump matches the issue (`StreamActivities.func1.1` parked in `View`, a writer in `(*DB).mmap` on `RWMutex.Lock`, readers in `beginTx` `RLock`). - this branch: 400 writes, file grew 133 → 150 MB (remap happened), reads steady at ~76 ms; a full export returned all 12,000 records ordered with no duplicates. Cross-reviewed by opencode `gpt-6.1-sol`: CLEAN.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hazard
storage.Manager.StreamActivitieswalked 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 onctx.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().OpenTxNstayed at 1, later writes could not remap the file, anddb.Closehung 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 checksctx.Err()before every send and selects onctx.Done()around it, so cancellation returns fromdb.Viewpromptly 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.r.Context(),Stats24handSuggestpass theirctx.Stats24hno longer caches a truncated rollup after cancellation;Suggestreturns 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).drainActivitiesand its test are removed.Tests
internal/storage/activity_stream_cancel_test.go: abandon the stream after one record with 300 matching records, assertOpenTxNreturns to 0 andClosereturns; 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 hungClosefor the full timeout before the fix.TestProfilesService_Stats24hDoesNotCacheTruncatedRollup,TestSuggest_CancelledContextReturnsWithRecordsLeft(updated).-race:internal/storage,internal/httpapi,internal/runtime.golangci-lint(CI config, bare and--build-tags server) reports only pre-existing findings in untouched lines.