Repository navigation
Conversation
There was a problem hiding this comment.
The fix looks right from reading the code: each integration handler that reads the registry now waits for plugin activation first, as integration.list already does. I ran out of time before running the #59730 reproduction or the server tests, so neither the fix nor the new test has been run in this review.
Public API: unchanged. The HTTP routes and their schemas stay the same; only the timing on a cold location changes.
One point worth addressing: these handlers use the raw Plugin.awaitActivation. Most other server handlers (agent, provider, model, usage, generate, websearch, voice) use awaitPlugins from packages/server/src/location.ts. It waits the same way, but gives up after 5s with a ServiceUnavailableError. With the raw call, a plugin that never finishes starting leaves integration requests hanging until the client times out. See the inline comment.
Related: #55353
| .handle( | ||
| "integration.get", | ||
| Effect.fn(function* (ctx) { | ||
| yield* Plugin.awaitActivation |
There was a problem hiding this comment.
Most other handlers wait with awaitPlugins from ../location, which wraps Plugin.awaitActivation with a 5s timeout and a ServiceUnavailableError. With the raw call, a plugin that hangs while starting leaves every integration get/connect/oauth/command request waiting indefinitely, where it would otherwise get a clear 503. It would be worth using awaitPlugins here and in the other handlers this PR touches, and probably in integration.list too, so they all behave the same.
There was a problem hiding this comment.
Verified: I ran the #59730 reproduction from source. On the base, the first get, connect/key and oauth requests on a fresh directory return 404, 404 and 500. With this PR they return 200, 204 and 200, the same as a retry. provider.test.ts and tsc -b in packages/server pass.
The earlier point is addressed: every integration handler now uses awaitPlugins, and the 503 is declared in the protocol.
Public API: ServiceUnavailableError (HTTP 503) is added to the declared errors of integration.list, integration.get, integration.connect.key, integration.connect.external, integration.oauth.connect, integration.oauth.status, integration.oauth.complete, integration.command.connect and integration.command.status. The OpenAPI spec and the generated client's declaredStatuses change to match. This only adds an error and is backwards compatible. Clients that handle every error tag explicitly may want to handle the new one.
One small thing: the new 503 test can be dropped (see inline).
| ) | ||
|
|
||
| it.live( | ||
| "integration reads return 503 when plugin activation does not finish", |
There was a problem hiding this comment.
This test checks the 5-second timeout inside the shared awaitPlugins helper, not the fix in this PR, and it waits the full 5 real seconds every run. The first test already covers the integration handlers waiting for plugins. I'd drop this one to keep the change minimal.
There was a problem hiding this comment.
The new commit removes the 503 timeout test, as suggested. I re-ran provider.test.ts and it passes. The rest of the change is the same as before; earlier I ran the #59730 reproduction from source and the cold get, connect/key and oauth requests now succeed on the first try. Public API impact is unchanged from the last review: ServiceUnavailableError (503) is added to the integration endpoints' declared errors, which is backwards compatible. No further concerns.
# Conflicts: # packages/server/src/handlers/integration.ts
There was a problem hiding this comment.
Still fixes the bug after the merge with dev: I re-ran the #59730 reproduction against the merged dev parent and this PR. The first cold get, connect/key and oauth requests go from 404, 404 and 500 to 200, 204 and 200. provider.test.ts, integration-policy.test.ts and tsc -b pass in packages/server, and re-running the client generator leaves no changes. The merge is resolved correctly: awaitPlugins runs before the new policy check from dev. Public API impact is unchanged: ServiceUnavailableError (503) is added to the integration endpoints' declared errors, which is backwards compatible. No further concerns.
Issue for this PR
Fixes #59730
Type of change
What does this PR do?
Integration reads and connection requests could report a plugin-provided integration as missing while its plugin was still starting. Wait for plugin activation before looking up integrations. If activation does not finish within five seconds, return a documented 503 instead of waiting indefinitely. Cancellation remains immediate.
How did you verify your code works?
bun test test/provider.test.ts --rerun-each 10: 10 passes. The controlled HTTP regression returns 404 on the base and waits for the integration with this change. A separate HTTP check verified that a stalled plugin returns 503.bun typecheckandbun run typecheck:tscinpackages/server: passed.bun run generateinpackages/clientandbun run checkat the repository root: passed.Visual
Screenshots / recordings
Not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Fixes #55353
Related: #55353
Public API: adds ServiceUnavailableError (503) to the integration endpoints' declared errors (backwards compatible)