feat: push provider changes to gateways via WatchAIProviders - #26746
feat: push provider changes to gateways via WatchAIProviders#26746dannykopping wants to merge 3 commits into
Conversation
…Providers Add a WatchAIProviders streaming RPC to the ProviderConfigurator service so a running standalone AI Gateway refetches its provider set when the provider configuration changes. The server forwards AIProvidersChangedChannel events (published by the provider CRUD endpoints) as payload-free signals; the gateway refetches via GetAIProviders on each signal. Bumps the aibridged API to v1.2.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 8 findings (4 P3, 1 P4, 3 Nit), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel. 3 P3, 1 P4, 3 Nit. Reviewed against 44b11df..3b6f968. Round 2Churn guard: PROCEED. 7/7 addressed. Panel: 1 P3 new (CRF-8). Reviewed against 44b11df..40c6317. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
dannykopping
changed the title
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean streaming RPC addition with solid test coverage (53% test density). The signal-on-subscribe pattern that closes the race window between loadProviders and the watch stream is well-considered. The coalescing buffer with non-blocking write is textbook, and the error-signals-as-refetch-triggers is strictly better than the embedded path's skip-on-error behavior.
3 P3, 1 P4, 3 Nit. No blockers.
Multiple reviewers independently noted the asymmetry where SubscribeProviderReload (embedded path, pre-existing) skips reload on pubsub delivery errors, while WatchAIProviders correctly signals. The new code does it right; the pre-existing embedded path would benefit from the same fix in a follow-up.
"The one asymmetry I noticed: SubscribeProviderReload skips reloads on pubsub drop errors, while the new WatchAIProviders callback signals on all events including drops. The new code's behavior is strictly better." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
- Use context.AfterFunc to bind stream lifecycle to the server (CRF-2) - Log pubsub delivery errors in the watch callback (CRF-7) - Clarify the reload-failure warn log (CRF-3) - Drain the standalone gateway watch goroutine before srv.Close() (CRF-4) - Add a test that a failed reload does not stop the watch loop (CRF-1) - Rename import alias coderpubsub to coderdpubsub (CRF-6) - Drop comments that restate the code (CRF-5)
|
/coder-agents-review |
There was a problem hiding this comment.
All 7 R1 findings addressed cleanly in 40c6317. The context.AfterFunc replacement (CRF-2), WaitGroup addition (CRF-4), and new TestWatchProviderReloadContinuesAfterReloadError test (CRF-1) are all well-executed.
1 new P3 from the R2 panel. The CRF-4 fix (WaitGroup) introduced a defer ordering issue that can deadlock the process on the HTTP server error path.
"If the watch goroutine is inside Client() waiting for a daemon reconnection, watchCancel() does nothing because Client() only checks lifecycleCtx, not the watch context. watchWG.Wait() blocks. srv.Close() never executes. Deadlock." (Pariston)
🤖 This review was automatically generated with Coder Agents.
| providerLogger.Warn(watchCtx, "ai provider watch loop exited", slog.Error(err)) | ||
| } | ||
| }() | ||
| defer func() { |
There was a problem hiding this comment.
P3 [CRF-8] Defer ordering can deadlock shutdown on the HTTP server error path.
LIFO defer execution order:
listener.Close()watchCancel(); watchWG.Wait()srv.Close()
watchWG.Wait() (step 2) runs before srv.Close() (step 3). srv.Close() cancels lifecycleCtx, which is the only context Server.Client() blocks on (aibridged.go:137-141). If the watch goroutine is inside Client() waiting for a daemon reconnection when the HTTP server returns an error, watchCancel() cancels watchCtx but Client() doesn't observe it. watchWG.Wait() hangs. srv.Close() never runs. Process hangs until an external SIGTERM cancels ctx (the parent of lifecycleCtx).
Trigger: HTTP server error (rare) while the daemon has lost its coderd connection (watch goroutine stuck in Client()). Both must coincide.
The comment at line 132 ("The deferred cancel+wait drains this goroutine before srv.Close() runs at shutdown") is correct for the normal signal path (ctx canceled -> lifecycleCtx canceled -> Client() unblocks) but wrong for the serveErr path.
Fix: cancel lifecycleCtx before waiting, by calling srv.Close() inside the watch defer. shutdownOnce makes the second srv.Close() from the earlier defer idempotent:
defer func() {
watchCancel()
srv.Close() // Cancel lifecycleCtx so Client() unblocks.
watchWG.Wait()
}()(Pariston)
🤖
There was a problem hiding this comment.
Fixed in c931be3. The deferred drain now calls srv.Close() (which cancels the daemon lifecycle context that Client() blocks on) before watchWG.Wait(), so a watch goroutine stuck reconnecting unblocks on the HTTP server error path. Close is idempotent via shutdownOnce, so the earlier deferred srv.Close is a no-op.
srv.Close cancels the daemon lifecycle context, the only context WatchProviderReload's blocking Client() call observes. Call it before watchWG.Wait() so a watch goroutine waiting to reconnect unblocks on the HTTP server error path instead of hanging. Close is idempotent (CRF-8).
|
Superseded by #26797 (branch renamed to |
1 participant
Adds a
WatchAIProvidersstreaming RPC to theProviderConfiguratorservice so a running standalone AI Gateway refetches its provider set when the provider configuration changes. The server subscribes toAIProvidersChangedChannel(published by the provider CRUD endpoints) and forwards each event as a payload-free signal, plus one signal on subscribe; the gateway callsGetAIProviderson each signal to rebuild its pool. The aibridged API is bumped to v1.2.Env-seeded providers don't need a signal: seeding finishes before coderd serves the gateway connection, so the gateway's initial fetch already reflects the seeded set. A periodic idempotent reload on the pool reloader is intentionally out of scope here and tracked separately.
Refs https://linear.app/codercom/issue/AIGOV-465/publish-provider-seed-completion-message-after-seedaiprovidersfromenv
Depends on #26605