feat: synchronise provider changes with WatchAIProviders - #26797
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
/coder-agents-review |
WatchAIProviders
0195db9 to
7d4af4c
Compare
d64b939 to
8c82616
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 4 | Last posted: Round 4, 11 findings (2 P3, 4 P4, 5 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledgedCRF-8 (Nit, coderd/aibridged/reload.go:80) - Retry pair duplicated without named constant
Round logRound 1Panel. 2 P3, 3 P4, 4 Nit. 0 dropped. Reviewed against 0195db96128..d64b939ad3f. Round 2BLOCKED. All 9 findings silent. No PR file changes between rounds (only unrelated frontend files in rebase). No author responses on any thread. No review. Round 3Churn guard: PROCEED. 7 fixed, 1 fixed (CRF-3, pre-existing code), 1 contested (CRF-8). Panel: CRF-8 closed (5/5). 1 P4, 1 Nit new. Reviewed against a886804..6df6fd9. Round 4Churn guard: PROCEED. CRF-10 fixed, CRF-11 fixed. All findings resolved. Reviewed against a886804..b3291f6. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Clean work. The streaming RPC design is well-structured: payload-free signals, coalescing via buffered channel, initial signal on subscribe to close the gap between loadProviders and the watch stream. Shutdown ordering in aigatewaystart.go is correct. Test coverage is solid at 57% density, exercising reconnection, reload failure resilience, and clean shutdown.
2 P3, 3 P4, 4 Nit.
"A server that accepts the WatchAIProviders call but closes the stream before sending the initial signal will cause the client to reconnect at 50ms intervals indefinitely. No exponential backoff accumulates because every successful TCP+DRPC handshake resets it." (Hisoka)
coderd/aibridged/reload.go:41
P4 [CRF-3] SubscribeProviderReload returns early on a pubsub delivery error, skipping the reload. WatchAIProviders (aibridgedserver.go:814-817) logs the same error and still calls notify(), triggering a client refetch. The WatchAIProviders behavior is correct: a dropped pubsub message means a possibly-missed CRUD event, so reloading ensures convergence. The embedded path's skip-on-error means a persistent delivery failure silently prevents convergence until the next clean event.
This is pre-existing code, but the inconsistency now matters: the standalone path handles this scenario correctly while the embedded path does not. The fix is one line: remove the early return so the callback falls through to reload. (Ryosuke P3, Zoro P4, Bisky P4)
🤖
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Round 2 is blocked. The PR files are unchanged since round 1 (the head SHA changed only due to a stack rebase), and none of the 9 open findings have an author response or code fix.
Unaddressed findings:
- CRF-1 (P3): Backoff resets on stream open before any Recv, allowing tight reconnect loop.
- CRF-2 (P3): WatchProviderReload doc promises ctx-only cancellation but client() blocks independently.
- CRF-3 (P4): SubscribeProviderReload skips reload on pubsub delivery errors; WatchAIProviders correctly signals.
- CRF-4 (P4): No test for pubsub-error-triggers-signal contract.
- CRF-5 (P4): No test for client() dial failure path.
- CRF-6 (Nit): Use wg.Go instead of Add/go/Done triple.
- CRF-7 (Nit): Parameter named
clientinstead ofclientFn. - CRF-8 (Nit): Retry pair duplicated without named constant.
- CRF-9 (Nit): Doc comment carries content beyond the method contract.
Further review is blocked until the author responds to or pushes fixes for the open findings. At minimum, the two P3s (CRF-1, CRF-2) need a response: fix, contest, or defer with a ticket.
🤖 This review was automatically generated with Coder Agents.
8c82616 to
dfee053
Compare
98e6909 to
a886804
Compare
dfee053 to
6df6fd9
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 8 fixed findings from R1 verified. The received flag (CRF-1) correctly prevents backoff reset before any signal is received. The doc comment (CRF-2) now accurately states the ClientFunc dependency. SubscribeProviderReload (CRF-3) now reloads on delivery errors, matching WatchAIProviders. New tests lock the pubsub-error-triggers-signal contract (CRF-4) and the dial-failure backoff path (CRF-5). Test coverage is up to 65% density with 337 test lines.
CRF-8 (retry constant): panel closed 5/5. The three call sites serve different retry contexts where independent tuning is plausible.
1 P4, 1 Nit new. No blocking issues.
"The hint-based design matches the existing pubsub channel, which also carries empty payloads. Pushing full configs would couple the stream schema to the provider model and increase message size for no meaningful latency gain." (Pariston)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 11 findings resolved across 4 rounds. CRF-10 and CRF-11 verified fixed in b3291f6: TestWatchAIProvidersStopsOnLifecycleCancel locks the lifecycle cancellation contract, and captureListenerPubsub now has its compile-time interface assertion.
Test coverage is at 67% density (377 test lines / 561 effective lines) covering the critical paths: initial signal, publish signals, delivery errors, reconnection after stream drop, retry after dial failure, continuation after reload failure, and lifecycle context cancellation. No open findings.
🤖 This review was automatically generated with Coder Agents.
| // before serving. | ||
| // | ||
| // It runs until ctx is canceled, then returns ctx.Err(). Cancellation requires | ||
| // both ctx and the underlying ClientFunc to unblock: during reconnection |
There was a problem hiding this comment.
It would be nice to be able to unblock ClientFunc using received ctx so it would be clear that there are no go-routines leaking.
I added context aware client acquisition function in later version of base branch: https://github.com/coder/coder/pull/26605/changes#diff-c360083edc6d0d454da4d4c7c4d5d19714deb8daf6e4c33bca9963438e396540R170
There was a problem hiding this comment.
Addressed, good call
| } | ||
|
|
||
| r := retry.New(50*time.Millisecond, 10*time.Second) | ||
| for { |
There was a problem hiding this comment.
nit: would be shorter:
for r := retry.New(50*time.Millisecond, 10*time.Second); r.Wait(ctx) {
....
}
return ctx.Err()
There was a problem hiding this comment.
This would wait 50ms before the first attempt, no?
There was a problem hiding this comment.
I think first Wait should be instant since Delay is set to 0 in New.
Later Delay is set to Floor on return which will take effect on next call.
…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.
- 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)
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).
- Reset watch backoff only after a signal is received, avoiding a tight reconnect loop when the server fails before the first Recv. - Document that WatchProviderReload cancellation needs both ctx and the ClientFunc to unblock. - Reload on dropped-message delivery errors in SubscribeProviderReload to match WatchAIProviders reconverge semantics. - Rename the ClientFunc parameter to clientFn for package consistency. - Use sync.WaitGroup.Go in the standalone gateway watch goroutine. - Trim the WatchAIProviders doc comment and proto response comment. - Add tests for the pubsub-error signal path and the dial-failure backoff path.
…face - Add TestWatchAIProvidersStopsOnLifecycleCancel to lock that canceling the server lifecycle context stops the handler while the stream context stays open, guarding against a shutdown goroutine leak. - Add a compile-time interface assertion for captureListenerPubsub to match the sibling test helpers.
Add ClientFuncWithContext and use it for WatchProviderReload and PoolRPCReloader, so blocking client acquisition (waiting for the daemon to connect to coderd) unblocks when the caller's context is canceled. Previously the reloader acquired its client via a constructor-time closure that ignored Reload's context. On the standalone gateway's serve-error exit path, a watch goroutine blocked inside Reload would not observe watchCancel, and watchWG.Wait deadlocked unless srv.Close ran first. The srv.Close drain workaround is now unnecessary and removed; TestWatchProviderReloadCancelUnblocksClient locks the cancellation contract.
a886804 to
48bc957
Compare
b3291f6 to
3b84826
Compare
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