Thread context.Context through modelsdev store API#1747
Conversation
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
/review |
There was a problem hiding this comment.
Review Summary
Found 2 HIGH severity race condition bugs introduced by removing sync.OnceValues without replacement synchronization.
The Store type documents "All methods are safe for concurrent use" but the new GetDatabase() implementation lacks synchronization primitives (no mutex, no sync.Once). Multiple concurrent calls can:
- Both pass the nil check and call
loadDatabase()simultaneously (wasting API calls) - Both write to
s.db, creating a data race
Additionally, there's a blocking network call in the TUI event loop that can freeze the UI for up to 30 seconds.
Critical: These bugs will cause data races and UI freezes under concurrent usage.
Replace the lazy-loaded closure-based Store with a simple struct that holds a cache file path and loads the database on first GetDatabase call. Pass caller-provided context to the HTTP request in fetchFromAPI instead of using context.Background(). NewStore() remains context-free (no I/O). All Store methods (GetDatabase, GetProvider, GetModel, ResolveModelAlias) and ModelSupportsReasoning accept context for the network call. Updated all callsites, using context.Background() only in TUI callbacks where no caller context is available. Assisted-By: cagent
|
/review |
There was a problem hiding this comment.
Review Summary
Found 3 high severity and 2 medium severity concurrency/context issues in the changed code. The most critical issues are:
- Mutex held during blocking network I/O - Can freeze all concurrent requests for up to 30s
- UI event handlers block with context.TODO() - Acknowledged by TODOs in the code, will freeze TUI
- context.Background() in cost calculations - Cannot respect cancellation
The high severity issues should be addressed before merging as they can cause significant user-facing freezes.
2 participants
Pass caller-provided context to the HTTP request in fetchFromAPI instead of using context.Background(). This allows cancellation and deadline propagation when fetching from the models.dev API.
Updated all public Store methods (GetDatabase, GetProvider, GetModel, ResolveModelAlias) and ModelSupportsReasoning to accept context. Changed the lazy loading from sync.OnceValues to sync.Once to support the context parameter. Updated all callsites across the codebase, using context.Background() only in TUI callbacks and pricing calculation callbacks where no caller context is available.
Assisted-By: cagent