Skip to content

fix(acp): stop two guaranteed-to-fail requests on every connect - #704

Open
Adam-Dalloul wants to merge 2 commits into
xintaofei:mainfrom
Adam-Dalloul:fix/stale-config-pref-on-connect
Open

fix(acp): stop two guaranteed-to-fail requests on every connect#704
Adam-Dalloul wants to merge 2 commits into
xintaofei:mainfrom
Adam-Dalloul:fix/stale-config-pref-on-connect

Conversation

@Adam-Dalloul

Copy link
Copy Markdown
Contributor

Two things my own logs do on every connect. Both are in the ACP connect path, so one PR.

1. A saved config preference is replayed to agents that do not have the option

29 occurrences in a week, the only ERROR class in the file:

[ACP] failed to apply preferred config 'fast'='off' on connect:
  Internal error: {"details": "Unknown config option: fast"}

saveConfigPreference is the only writer of codeg:selector-prefs and it fires on a user pick. Once an agent stops advertising the option its selector leaves the UI, so there is no pick left to make, the entry never changes, and apply_preferred_session_options re-sends it forever. It is not the value either: an agent that advertises no effort option rejects effort the same way at every value, so a clamp could not have helped.

The replay now sends a preference only when the agent currently advertises the id. mode keeps the documented exception, since codex-acp's applySessionConfigOption switches on configId alone with no advertised-list check, and a rejection of that deliberately speculative send drops to debug.

Nothing is deleted. The store is keyed per agentType but an option's presence is narrower than that: cursor-agent hangs fast and the thinking parameters off the model in effect, so the same agent advertises the id under one model and not another. Dropping the entry would retire a preference the user still wants. Keeping it costs a skipped round-trip and it applies again the moment the option returns, including inside the same replay, because model goes first and the list it returns is what the remaining ids are checked against.

2. resume and load both ask for a rollout that is gone

[ACP] session/resume failed (Internal error: {"details": "no rollout found
  for thread id 019a…"}); falling back to session/load
[ACP] session/load failed (Internal error: {"details": "no rollout found
  for thread id 019a…"}), falling back to session/new

Falling through on a resume failure is right in general, and every other error still does. This one cause is the exception: both methods resolve the same per-thread rollout record, so the answer is the store saying the record does not exist and a second lookup cannot change it. The resume error is now handed to the load block as its own result, so the entire ladder runs on it unchanged, minus the redundant request and the duplicate warning, and the log names the method that actually answered.

classify_session_load_failure also learns the wording. It is -32002 in everything but the code, exactly like the claude 0.58.1 bodies already matched on the wire message, so it takes the existing resource_not_found verdict and the user gets the localized banner instead of the raw JSON-RPC body. Custom agents keep their silent local recovery.

Against current main (7096339); neither had moved. Related: #396, whose third bug is the first half of this. #612 also touches selector-prefs-storage.ts, but only its parsing; this PR changes a comment there that described a self-healing path which cannot happen.

Tests: four in connection.rs covering the advertised gate, the mode exception, a model-scoped preference surviving the model pick, the new classification, and the resume causes that must still fall through. No new user-visible strings.

Saved selector preferences are replayed on every connect, and an id the
connected agent does not offer is rejected every time. The reporter's logs
carry 29 of these in a week, the only ERROR class in them:

  [ACP] failed to apply preferred config 'fast'='off' on connect:
    Internal error: {"details": "Unknown config option: fast"}

The entry cannot clear itself. `saveConfigPreference` is the only writer and
it fires on a user pick, but once the agent stops advertising the option its
selector leaves the UI, so there is no pick left to make and the value is
re-sent forever. Nor is the value the problem: an agent that advertises no
`effort` option rejects `effort` identically at every value, so clamping the
value could never have helped either.

`apply_preferred_session_options` now sends a preference only when the agent
currently advertises the id. `mode` keeps the documented exception, because
codex-acp's `applySessionConfigOption` switches on `configId` alone with no
advertised-list check, and a rejection of a deliberately speculative send
logs at debug rather than error.

Nothing is deleted. The store is keyed per agentType but an option's
presence is narrower than the agent: cursor-agent hangs `fast` and the
thinking parameters off the model in effect, so the same agent advertises
the id under one model and not under another. Dropping the entry would
retire a preference the user still has a use for. Keeping it costs a skipped
round-trip, and it applies again as soon as the option returns — including
within the same replay, since `model` goes first and the option set that
comes back with it is what the remaining ids are checked against.
Reopening a conversation whose codex rollout is gone puts the same failure
on the wire twice, then reports it as a raw JSON-RPC body:

  [ACP] session/resume failed (Internal error: {"details": "no rollout
    found for thread id 019a…"}); falling back to session/load
  [ACP] session/load failed (Internal error: {"details": "no rollout
    found for thread id 019a…"}), falling back to session/new

Falling through on a resume failure is right in general — resume is
unstable and not equivalent to load, so a resume-specific error must never
deny a load that might still succeed. This one cause is the exception: both
methods resolve the same per-thread rollout record, so "no rollout found for
thread id" is the store reporting the record does not exist, and asking a
second time cannot make it appear.

The resume error is now carried into the load block as its own result. The
whole ladder runs unchanged on it — classification, the banner, the
session/new fallback — minus one guaranteed-to-fail request and its
duplicate warning, and the log names the method that actually answered.

`classify_session_load_failure` also learns the wording. It is -32002 in
everything but the code, exactly like the claude 0.58.1 cases already
matched on the wire message, so it takes the existing `resource_not_found`
verdict and the user sees the localized "session failed to load, reload or
start a new conversation" banner instead of the raw error body. Custom
agents, whose history is codeg's own transcript, keep their silent local
recovery.
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.

1 participant