Skip to content

fix(cli): expose retryable ACP plugin activation readiness errors - #57237

Open
surlymochan wants to merge 2 commits into
anomalyco:devfrom
surlymochan:acp-activation-retry
Open

surlymochan wants to merge 2 commits into
anomalyco:devfrom
surlymochan:acp-activation-retry

Conversation

@surlymochan

@surlymochan surlymochan commented Oct 20, 2026 •

Copy link
Copy Markdown

Issue for this PR

Fixes #57204

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Cold ACP sessions can exhaust catalog activation waits and fail with a generic internal error. Retry transient plugin-activation reads using the existing bounded readiness sequence. If activation is still pending, return a documented, recognizable error: data.errorName = CatalogNotReady, service = plugin, retryable = true. Clients can retry the same new/load/resume request later.

No timeout constants or endpoints are added. This does not promise that arbitrarily slow activation succeeds on the first request. Other service errors and genuine plugin failures retain their existing behavior; closing ACP cancels its pending wait.

How did you verify your code works?

Managed cloud, official source ACP CLI/private server, isolated HOME and synthetic local plugins:

  • A 15-second setup previously failed at about 10.2s; the readiness retry succeeds at about 15.4s/15.7s for new/resume.
  • A 40-second setup first returns explicit not-ready at 20.394s, finishes at 40.326s, then the same process resumes the original session in 194ms. Session listing confirms it was retained.
  • Permanently pending setup returns explicit not-ready at 20.430s. A throwing setup keeps the original logged failure and existing plugin isolation, without being misreported as pending. All probe subprocesses exited normally; no model requests were made.
  • The new error-contract regression fails on the previous head. Final four focused CLI files: 23 tests / 90 assertions passed, covering new/load/resume, unrelated errors and shutdown cancellation. CLI typecheck, focused lint/format/diff checks, documentation typecheck/generated checks and documentation build passed.

No real IDE/Windows or external provider run is claimed. The full repository check was not rerun; the earlier Turbo typecheck SIGSEGV (139) remains unresolved.

Screenshots / recordings

N/A — ACP readiness behavior.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

AI-assisted implementation and verification; final diff reviewed before publication.


Added by the opencode bot (AI-generated).

Related: #55937

@github-actions

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reproduced #57204 on the base and checked the fix by running its script against both checkouts from source. With a 15s plugin activation, session/resume fails after 10.1s with ServiceUnavailableError on the base. With this PR it succeeds after 15.6s. Resume without the delay is unaffected (0.7s on both). The catalog tests pass (8/8) and packages/cli typechecks.

Public API: unchanged. This is internal to ACP's catalog loading.

The main concern is that this raises the limit rather than removing it (see the inline comment). With a 40s activation, resume still fails after 20.2s with the same generic -32603 / ServiceUnavailableError. That error is what the issue asks to avoid, and slow $HOME activation (#55937) can easily take longer than 20s. The new tests are compact and use the existing wire fixture, so no concerns there.

Related: #55937

)
// A timed-out catalog read can still be waiting on activation, so recheck readiness before retrying it.
return yield* attempt.pipe(
Effect.retry({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This retry uses the existing poll schedule (upTo 5s). Each retry also waits up to 5s polling plugin.list and another 5s in the server's awaitPlugins. So the real limit is now set by three separate 5s timeouts, about 20s in total, and it isn't stated anywhere. I checked: with a 40s activation, session/resume still fails at 20.2s with the same generic ServiceUnavailableError the issue reports, so a slow $HOME activation (#55937) will still hit it.

Could ACP wait for activation explicitly instead, with a clear limit or none? For example, an await-activation call like handlers/rpc.ts does with Plugin.awaitActivation. Or, if a limit is kept, have it fail with an error a client can recognise as "not ready yet". Either way the behaviour wouldn't depend on how these timeouts happen to add up.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 963afe0. Bounded activation exhaustion now returns documented data {errorName: "CatalogNotReady", service: "plugin", retryable: true}, so clients can distinguish it and retry without discarding the session. A real 40-second plugin returned not-ready at 20.394s, completed at 40.326s, and the same process resumed the original session in 194ms. Permanently pending setup remains bounded; throwing setup retains existing plugin isolation and is not mislabeled as pending. Twenty-three focused tests and CLI/documentation checks pass. The title and body describe this final error contract. Please re-review the updated head.

@surlymochan
surlymochan marked this pull request as draft October 20, 2026 01:15
@surlymochan surlymochan changed the title fix(cli): retry catalog readiness during plugin activation fix(cli): expose retryable ACP plugin activation readiness errors Oct 20, 2026
@surlymochan
surlymochan marked this pull request as ready for review October 20, 2026 01:24
@RaviTharuma

Copy link
Copy Markdown

Tested the failure mode on 2.1.2 with a real-world slow activation: a large skill library takes about 13–17s, and there are no synthetic plugins. The first session/new fails at about 10s today, and a re-send succeeds. With the retry budget in this PR (about 20s before CatalogNotReady), that cold start should succeed on the first request. Could the release notes document CatalogNotReady (retryable: true) as the signal clients should retry on? We currently match ServiceUnavailableError in a client-side proxy and would switch to that.

@surlymochan

Copy link
Copy Markdown
Author

Thanks for testing this against a real slow activation — a 13–17s skill library with no synthetic plugins is exactly the case this targets, and your timings line up with what I measured.

Yes: CatalogNotReady with retryable: true is intended to be documented as the signal clients should retry on, and the ACP docs added in this PR already state it that way (the "Plugin activation readiness" section). The contract, as implemented:

  • Match on the error data, not the message text: data.errorName === "CatalogNotReady", data.service === "plugin", data.retryable === true, on a -32603 error from session/new, session/load, or session/resume.
  • It is only emitted after the server's own bounded readiness retries are exhausted (about 20s in total), so treat it as "not ready yet" for that request, not as a failure of the request itself. Re-issue the same request later; for session/load/session/resume, a failed attempt does not delete the session, so retrying the same session is safe. In my 40s-activation run, the first request returned this error at ~20.4s and the same process resumed the original session in 194ms once activation finished.
  • It is deliberately narrow: only plugin-activation exhaustion carries service/retryable. Other ServiceUnavailableError cases (for example a database or unspecified service) are not retried server-side and reach the client without this marker, so keying your proxy on retryable: true won't make you retry genuine failures. The raw plugin ServiceUnavailableError you match today is consumed by the retry loop and converted into this error, so switching your match to CatalogNotReady + retryable: true is the right move.
  • There is no server-provided retry delay; cadence/backoff is up to the client, and each retried request gets a fresh bounded wait server-side.

With a 13–17s activation you should fall inside the ~20s budget and succeed on the first request; CatalogNotReady is the fallback for activations that run longer than that. I can't write the release-notes text myself, but the docs section in this PR is the canonical wording they can point to.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

session/resume fails right after initialize with "Location plugins did not finish activating"

2 participants