docs: add comprehensive CLAUDE.md for AI assistant context#2
docs: add comprehensive CLAUDE.md for AI assistant context#2
Conversation
Provides codebase structure, build/test commands, architecture overview, key conventions (error handling, logging, concurrency, testing patterns), and configuration reference. Also removes CLAUDE.md from .gitignore. https://claude.ai/code/session_01M2KuJyY5KdDzjedzC9b123
Each configured symbol (BTC/USD, ETH/USD, etc.) gets an independent pipeline: adapters → normalizer → fan-out → engine. A MultiEngine wrapper merges all per-symbol engines for the API layer. Key changes: - config: add SymbolConfig with per-symbol source lists; backward compat migration from legacy single-symbol format - normalizer: accept canonicalSymbol param instead of hardcoded mapping - engine/multi.go: MultiEngine aggregates per-symbol engines, merges snapshot/tick channels, routes LatestState(symbol) lookups - api: Engine interface now has LatestState(symbol) and Symbols(); REST handlers accept ?symbol= query param; WSMessage includes symbol - main.go: per-symbol pipeline orchestration with shared raw writer - config.yaml.example: multi-symbol format with commented ETH example https://claude.ai/code/session_01M2KuJyY5KdDzjedzC9b123
…k suite - Add slow client eviction (configurable max drops before disconnect) - Add max connection limit to prevent resource exhaustion - Add per-client connection tracking (connected_at timestamp) - Move slow client cleanup outside read lock to reduce contention - Add broadcast duration and eviction/rejection metrics - Add 17 engine benchmarks (median, canonical, venue refs, quality, pipeline) - Add 8 WebSocket broadcast benchmarks (1-1000 clients, filtering, slow mix) - Add tests for connection limits and slow client eviction https://claude.ai/code/session_01M2KuJyY5KdDzjedzC9b123
📝 WalkthroughWalkthroughThis PR introduces multi-symbol support to the btick Bitcoin price oracle by restructuring the event-processing pipeline from single-symbol to per-symbol channels, adding a MultiEngine for aggregating separate SnapshotEngine instances, implementing WebSocket connection limits and slow-client eviction, and updating configuration to support symbol-specific exchange sources. Changes
Sequence Diagram(s)sequenceDiagram
participant Adapters
participant Normalizer as Normalizer<br/>(per Symbol)
participant Engine as SnapshotEngine<br/>(per Symbol)
participant MultiEngine
participant Storage
participant API/WS
loop Per Symbol (BTC/USD, ETH/USD, etc.)
Adapters->>Normalizer: RawEvent (symbol-tagged)
Normalizer->>Normalizer: Deduplicate<br/>Stamp CanonicalSymbol
Normalizer->>Engine: Normalized Event
Engine->>Engine: Update VenueState<br/>Compute Median
Engine->>MultiEngine: Snapshot1s + CanonicalTick
end
MultiEngine->>Storage: Merged Snapshots
Storage->>Storage: Persist State
MultiEngine->>API/WS: Merged Ticks
API/WS->>API/WS: Per-Symbol Latest Price<br/>Fan-out to Clients
API/WS-->>Clients: WebSocket Broadcast<br/>(with Symbol field)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces multi-symbol support and enhanced WebSocket operational controls, alongside adding a new CLAUDE.md repository context document and updating config examples.
Changes:
- Add multi-symbol configuration (
symbols[]) with backward-compatible migration from legacy single-symbol config. - Add
MultiEngineand update API/WS paths to be symbol-aware (?symbol=, WS messages includesymbol), plus WS connection limiting + slow-client eviction with new metrics. - Add/expand benchmarks for engine and WebSocket broadcast performance; add
CLAUDE.mdand un-ignore it.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/btick/main.go |
Builds per-symbol pipelines and wires MultiEngine into the API server |
internal/config/config.go |
Adds symbols[] config + WS limits config with defaults + legacy migration |
internal/engine/multi.go |
New engine aggregator that merges snapshot/tick channels and routes LatestState by symbol |
internal/api/server.go |
Engine interface updated for symbols; WS hub constructed with symbol list |
internal/api/handlers.go |
Adds ?symbol= resolution for latest/health endpoints |
internal/api/websocket.go |
Adds symbol to WS messages; max client limit + slow-client eviction + metrics |
internal/metrics/metrics.go |
Adds WS eviction/rejection/broadcast counters + broadcast duration histogram |
internal/normalizer/normalizer.go |
Stamps configured canonical symbol onto outgoing events; New() signature extended |
internal/*/*_test.go / internal/*/benchmark_test.go |
Updates tests for new APIs and adds benchmarks |
config.yaml.example |
Migrates example config to symbols[] + WS limit settings |
CLAUDE.md |
New assistant context/architecture/conventions doc |
.gitignore |
Stops ignoring CLAUDE.md |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, sym := range cfg.Symbols { | ||
| canonical := sym.Canonical | ||
| symbols = append(symbols, canonical) | ||
| symLogger := logger.With("symbol", canonical) | ||
|
|
||
| // Channels for this symbol's pipeline. | ||
| rawCh := make(chan domain.RawEvent, 10000) | ||
| normalizedCh := make(chan domain.RawEvent, 10000) | ||
| engineCh := make(chan domain.RawEvent, 10000) | ||
|
|
||
| // Collect source names for dedup shard pre-init. | ||
| sourceNames := make([]string, 0, len(sym.Sources)) | ||
| for _, src := range sym.Sources { | ||
| sourceNames = append(sourceNames, src.Name) | ||
| } | ||
|
|
||
| // Start feed adapters for this symbol. | ||
| for _, src := range sym.Sources { | ||
| if !src.Enabled { | ||
| symLogger.Info("source disabled, skipping", "source", src.Name) | ||
| continue | ||
| } | ||
| s := src | ||
| safeGo(&wg, cancel, logger, "adapter-"+canonical+"-"+s.Name, func() { | ||
| startAdapter(ctx, s, rawCh, symLogger) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The per-symbol pipeline loop reuses variables declared with := inside the loop body (e.g., rawCh, normalizedCh, engineCh, symLogger). The goroutines started in this loop capture those variables, so subsequent iterations overwrite them and can cause adapters/normalizers/fan-out to publish into the wrong symbol’s channels/logger. Create a per-iteration scope (e.g., wrap the pipeline construction in a helper function or an inner { ... } block) and/or pass the channels/logger into goroutines as parameters so each symbol’s goroutines retain the correct values.
| for _, sym := range cfg.Symbols { | ||
| canonical := sym.Canonical | ||
| symbols = append(symbols, canonical) | ||
| symLogger := logger.With("symbol", canonical) | ||
|
|
There was a problem hiding this comment.
canonical := sym.Canonical is used without validation. If a symbol entry omits canonical (empty string), this will create an engine keyed by "" and produce events/WS messages with an empty symbol. Consider validating each symbol config (non-empty canonical, and ideally uniqueness) and failing fast with a clear error before starting goroutines.
| var wg sync.WaitGroup | ||
| for _, eng := range engines { | ||
| e := eng | ||
| wg.Add(2) | ||
| go func() { | ||
| defer wg.Done() | ||
| for snap := range e.SnapshotCh() { | ||
| m.snapshotCh <- snap | ||
| } | ||
| }() | ||
| go func() { | ||
| defer wg.Done() | ||
| for tick := range e.TickCh() { | ||
| m.tickCh <- tick | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| // Close merged channels once all sources are done. | ||
| go func() { | ||
| wg.Wait() | ||
| close(m.snapshotCh) | ||
| close(m.tickCh) | ||
| }() | ||
|
|
There was a problem hiding this comment.
The merged-channel closer goroutine (wg.Wait() then close(...)) will never run because SnapshotEngine.SnapshotCh() / TickCh() are not closed anywhere, so the per-engine fan-in goroutines never finish. Either make SnapshotEngine close its output channels on shutdown, or change MultiEngine to accept a context.Context and exit fan-in goroutines on cancellation instead of ranging until close.
| var wg sync.WaitGroup | |
| for _, eng := range engines { | |
| e := eng | |
| wg.Add(2) | |
| go func() { | |
| defer wg.Done() | |
| for snap := range e.SnapshotCh() { | |
| m.snapshotCh <- snap | |
| } | |
| }() | |
| go func() { | |
| defer wg.Done() | |
| for tick := range e.TickCh() { | |
| m.tickCh <- tick | |
| } | |
| }() | |
| } | |
| // Close merged channels once all sources are done. | |
| go func() { | |
| wg.Wait() | |
| close(m.snapshotCh) | |
| close(m.tickCh) | |
| }() | |
| for _, eng := range engines { | |
| e := eng | |
| go func() { | |
| for snap := range e.SnapshotCh() { | |
| m.snapshotCh <- snap | |
| } | |
| }() | |
| go func() { | |
| for tick := range e.TickCh() { | |
| m.tickCh <- tick | |
| } | |
| }() | |
| } |
| go func() { | ||
| defer wg.Done() | ||
| for snap := range e.SnapshotCh() { | ||
| m.snapshotCh <- snap | ||
| } | ||
| }() | ||
| go func() { | ||
| defer wg.Done() | ||
| for tick := range e.TickCh() { | ||
| m.tickCh <- tick | ||
| } | ||
| }() |
There was a problem hiding this comment.
Fan-in goroutines write to m.snapshotCh / m.tickCh with a blocking send. If the API broadcast loop slows/stops, these goroutines can block indefinitely and stop draining per-engine channels, increasing drop pressure upstream. To match the rest of the pipeline’s non-blocking channel convention, consider using a non-blocking send (with a drop metric/log) when the merged output channels are full.
| func (h *WSHub) HandleWS(w http.ResponseWriter, r *http.Request) { | ||
| // Check connection limit before upgrading. | ||
| h.mu.RLock() | ||
| currentCount := len(h.clients) | ||
| h.mu.RUnlock() | ||
|
|
||
| if currentCount >= h.wsCfg.MaxClientCount() { | ||
| h.logger.Warn("ws connection rejected: max clients reached", "max", h.wsCfg.MaxClientCount()) | ||
| metrics.IncWSRejected() | ||
| http.Error(w, `{"error":"too many connections"}`, http.StatusServiceUnavailable) | ||
| return | ||
| } |
There was a problem hiding this comment.
The max-client check is subject to a race: multiple concurrent requests can all observe len(h.clients) < max under the read lock, then each proceed to Upgrade and add themselves, exceeding MaxClientCount(). To enforce a hard limit, re-check under the write lock before inserting (and close/reject if over), or maintain an atomic/locked counter that reserves a slot before upgrading.
| // evictClients forcefully disconnects slow clients that exceeded the drop threshold. | ||
| func (h *WSHub) evictClients(clients []*wsClient) { | ||
| h.mu.Lock() | ||
| for _, c := range clients { | ||
| if _, ok := h.clients[c]; !ok { | ||
| continue // already removed | ||
| } | ||
| delete(h.clients, c) | ||
| c.logger.Warn("evicting slow ws client", | ||
| "total_drops", c.dropCount.Load(), | ||
| "connected_for", time.Since(c.connectedAt).Round(time.Second), | ||
| ) | ||
| close(c.sendCh) | ||
| if c.conn != nil { | ||
| _ = c.conn.Close() | ||
| } | ||
| metrics.IncWSEvicted() | ||
| } |
There was a problem hiding this comment.
evictClients holds h.mu.Lock() while closing client connections. Network/socket closes can block and will stall all concurrent HandleWS and Broadcast calls while the lock is held. Consider removing the clients from the map under the lock, then closing sendCh/conn outside the lock to keep the critical section small.
| # CLAUDE.md — btick | ||
|
|
||
| ## Project Overview | ||
|
|
||
| btick is a **Bitcoin price oracle service** for prediction market settlement. It aggregates real-time BTC/USD price data from four exchanges (Binance, Coinbase, Kraken, OKX) and produces a canonical, manipulation-resistant median price with sub-second precision. | ||
|
|
||
| **Tech stack:** Go 1.23, PostgreSQL 17+ with TimescaleDB 2.24+, gorilla/websocket, pgx/v5, shopspring/decimal. | ||
|
|
There was a problem hiding this comment.
PR description/title indicates this is a docs-only change (adding CLAUDE.md and un-ignoring it), but the diff includes substantial functional changes (multi-symbol pipelines, new Engine interface, WebSocket connection limiting/eviction, new metrics). Please update the PR title/description to reflect the actual scope so reviewers can assess risk appropriately.
|
|
||
| Config loaded from `config.yaml` (see `config.yaml.example`). Environment variable expansion supported via `${VAR}` syntax. `DATABASE_URL` env var overrides config DSN. | ||
|
|
||
| Key sections: `server`, `database`, `sources` (per-exchange), `pricing` (median mode, outlier %, freshness), `storage` (retention, batching), `health`. |
There was a problem hiding this comment.
The configuration summary lists sources as a top-level key section, but the example config and code now prefer symbols (with per-symbol sources). Update this line to mention symbols (and optionally note the legacy canonical_symbol + sources fallback) to avoid misleading readers.
| Key sections: `server`, `database`, `sources` (per-exchange), `pricing` (median mode, outlier %, freshness), `storage` (retention, batching), `health`. | |
| Key sections: `server`, `database`, `symbols` (per-symbol `sources`; legacy `canonical_symbol` + top-level `sources` still supported), `pricing` (median mode, outlier %, freshness), `storage` (retention, batching), `health`. |
| // NewMultiEngine creates a MultiEngine from a set of per-symbol engines. | ||
| // It merges their snapshot and tick channels into single output channels. | ||
| func NewMultiEngine(engines map[string]*SnapshotEngine, symbols []string) *MultiEngine { | ||
| m := &MultiEngine{ | ||
| engines: engines, | ||
| symbols: symbols, | ||
| snapshotCh: make(chan domain.Snapshot1s, 100*len(engines)), | ||
| tickCh: make(chan domain.CanonicalTick, 1000*len(engines)), | ||
| } | ||
|
|
||
| // Merge per-engine channels into the combined output channels. | ||
| var wg sync.WaitGroup | ||
| for _, eng := range engines { | ||
| e := eng | ||
| wg.Add(2) | ||
| go func() { | ||
| defer wg.Done() | ||
| for snap := range e.SnapshotCh() { | ||
| m.snapshotCh <- snap | ||
| } | ||
| }() | ||
| go func() { | ||
| defer wg.Done() | ||
| for tick := range e.TickCh() { | ||
| m.tickCh <- tick | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| // Close merged channels once all sources are done. | ||
| go func() { | ||
| wg.Wait() | ||
| close(m.snapshotCh) | ||
| close(m.tickCh) | ||
| }() | ||
|
|
||
| return m | ||
| } |
There was a problem hiding this comment.
MultiEngine introduces new routing/merging behavior (LatestState(symbol) dispatch + snapshot/tick fan-in), but there are no tests covering correct routing, symbol ordering, and shutdown/close behavior of the merged channels. Given the engine package has extensive tests, please add focused unit tests for MultiEngine to prevent regressions (especially around goroutine lifecycle and channel closure).
| // Backward compat: migrate legacy single-symbol config into Symbols slice. | ||
| if len(cfg.Symbols) == 0 && len(cfg.Sources) > 0 { | ||
| canonical := cfg.CanonicalSymbol | ||
| if canonical == "" { | ||
| canonical = "BTC/USD" | ||
| } | ||
| cfg.Symbols = []SymbolConfig{{ | ||
| Canonical: canonical, | ||
| Sources: cfg.Sources, | ||
| }} | ||
| } |
There was a problem hiding this comment.
Config.Load now migrates legacy single-symbol fields into cfg.Symbols and WSConfig has new defaulting helpers (MaxClientCount/SlowClientMaxDropCount), but config tests only cover DB pool sizing. Please add unit tests for the new load/migration behavior (and WS defaults) to ensure backward compatibility and default values don’t regress.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/api/handlers.go (1)
15-25: Consider returning 404 for unknown symbols.When a user requests an unknown symbol (e.g.,
?symbol=XYZ/USD),LatestStatereturnsniland the handler responds with 503 "no data yet". This conflates "symbol not configured" with "symbol exists but no data received yet".For better API clarity, consider validating the symbol against
s.engine.Symbols()and returning 404 for unknown symbols.♻️ Optional: Validate symbol exists
func (s *Server) resolveSymbol(r *http.Request) string { if sym := r.URL.Query().Get("symbol"); sym != "" { return sym } syms := s.engine.Symbols() if len(syms) > 0 { return syms[0] } return "" } + +func (s *Server) isValidSymbol(sym string) bool { + if sym == "" { + return false + } + for _, s := range s.engine.Symbols() { + if s == sym { + return true + } + } + return false +}Then in handlers, check validity before calling
LatestState.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers.go` around lines 15 - 25, Modify symbol handling to distinguish unknown symbols from “no data”. After resolving the symbol (in resolveSymbol or where handlers call it), check it against s.engine.Symbols() or use a helper like isKnownSymbol(symbol string) that queries s.engine.Symbols(); if the symbol is not present return HTTP 404 from the handler instead of proceeding to call s.engine.LatestState (which returns nil for no-data). Update affected handlers that call resolveSymbol and LatestState to perform this validation and return 404 for unknown symbols and keep the existing 503 behavior only when LatestState returns nil for a known symbol.CLAUDE.md (2)
29-53: Add language specifier to fenced code block.The project structure code block should have a language specifier for proper syntax highlighting and to satisfy markdown linting. Use
textorplaintextfor directory listings.-``` +```text cmd/btick/main.go # Entry point, goroutine orchestration, graceful shutdown🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 29 - 53, The fenced project-structure code block in CLAUDE.md is missing a language specifier; update the opening fence from ``` to ```text (or ```plaintext) so the directory listing is treated as plain text for syntax highlighting and markdown linting; modify the single code block that contains the directory tree (the block starting with the cmd/btick/main.go line) to use ```text as the opening fence.
57-64: Add language specifier to architecture diagram code block.Same as above — use
textorplaintextfor ASCII diagrams.-``` +```text Per symbol (BTC/USD, ETH/USD, ...):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 57 - 64, Update the ASCII diagram code block that begins with "Per symbol (BTC/USD, ETH/USD, ...):" to include a language specifier (e.g., ```text) so the block is fenced as plaintext; locate the block containing the lines "Exchanges → Adapters[sym] → Normalizer[sym] → Fan-out[sym]" and the subsequent arrows to "shared Writer" and "MultiEngine → API/WebSocket" and change the opening fence to ```text (or ```plaintext).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CLAUDE.md`:
- Around line 29-53: The fenced project-structure code block in CLAUDE.md is
missing a language specifier; update the opening fence from ``` to ```text (or
```plaintext) so the directory listing is treated as plain text for syntax
highlighting and markdown linting; modify the single code block that contains
the directory tree (the block starting with the cmd/btick/main.go line) to use
```text as the opening fence.
- Around line 57-64: Update the ASCII diagram code block that begins with "Per
symbol (BTC/USD, ETH/USD, ...):" to include a language specifier (e.g., ```text)
so the block is fenced as plaintext; locate the block containing the lines
"Exchanges → Adapters[sym] → Normalizer[sym] → Fan-out[sym]" and the subsequent
arrows to "shared Writer" and "MultiEngine → API/WebSocket" and change the
opening fence to ```text (or ```plaintext).
In `@internal/api/handlers.go`:
- Around line 15-25: Modify symbol handling to distinguish unknown symbols from
“no data”. After resolving the symbol (in resolveSymbol or where handlers call
it), check it against s.engine.Symbols() or use a helper like
isKnownSymbol(symbol string) that queries s.engine.Symbols(); if the symbol is
not present return HTTP 404 from the handler instead of proceeding to call
s.engine.LatestState (which returns nil for no-data). Update affected handlers
that call resolveSymbol and LatestState to perform this validation and return
404 for unknown symbols and keep the existing 503 behavior only when LatestState
returns nil for a known symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d0c8008-6045-449d-a018-f23acf87461e
📒 Files selected for processing (16)
.gitignoreCLAUDE.mdcmd/btick/main.goconfig.yaml.exampleinternal/api/benchmark_test.gointernal/api/handlers.gointernal/api/handlers_test.gointernal/api/server.gointernal/api/websocket.gointernal/api/websocket_test.gointernal/config/config.gointernal/engine/benchmark_test.gointernal/engine/multi.gointernal/metrics/metrics.gointernal/normalizer/normalizer.gointernal/normalizer/normalizer_test.go
Provides codebase structure, build/test commands, architecture overview,
key conventions (error handling, logging, concurrency, testing patterns),
and configuration reference. Also removes CLAUDE.md from .gitignore.
https://claude.ai/code/session_01M2KuJyY5KdDzjedzC9b123
Summary by CodeRabbit
New Features
max_clientsconfiguration to protect service availabilityDocumentation
Chores