fix(acp): name the method in request-timeout errors - #4733
Conversation
`AcpError::Timeout` carried only a `Duration`, so every timeout produced
the same sentence regardless of which call actually hung:
Request timeout — agent did not respond within 60s
That is the message a user reports, and it is not enough to place the
fault. Sessions are created lazily inside the prompt task, so a stalled
`session/new` and a stalled `session/prompt` look identical from the
outside — one of them is bounded at 60s and the other at 900s idle, and
the operator cannot tell which they hit. Diagnosing block#4098 needed a trace
across two codebases and the timeout constants to work out that only
`session/new` could produce that number.
`send_request` already has the method in hand, and it is `&'static str`
at every call site, so carrying it costs nothing:
Request timeout — agent did not respond to session/new within 60s
The two directly-constructed timeouts in `pool.rs` name theirs too
(`session/set_model`, `session/set_config_option`), so no timeout in the
crate is anonymous. Match sites become `Timeout { .. }`; nothing else
changes, and the classification in `lib.rs` and `pool.rs` behaves
exactly as before.
Verified on Windows: `cargo check`, `cargo clippy --all-targets` (0
warnings) and `cargo fmt --check` clean for the crate. Not offering the
crate's test suite as evidence: on this box it is non-deterministic
against untouched main (consistent with block#2492).
Signed-off-by: SmokeDev <test@test.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6f9870465
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| return Err(AcpError::Timeout(MODEL_SWITCH_TIMEOUT)); | ||
| return Err(AcpError::Timeout { | ||
| method: "session/set_model", |
There was a problem hiding this comment.
When ModelSwitchMethod::ConfigOption takes longer than the 5-second outer timeout, this branch reports session/set_model even though the request was actually session/set_config_option. That preserves an incorrect method name in the user-facing timeout this change is intended to disambiguate; select the method from the ModelSwitchMethod variant here.
Useful? React with 👍 / 👎.
…timeout `apply_model_switch` serves both `ModelSwitchMethod` variants — ConfigOption sends `session/set_config_option`, SetModel sends `session/set_model` — but the outer-timeout branch hardcoded `session/set_model`, so a ConfigOption switch that blew the 5s budget would name a method it never sent. That is worse than the anonymous timeout this PR set out to fix: a wrong name sends whoever reads it looking in the wrong place. Derived from the variant instead. `method_label` beside it stays what it was — prose for humans, not an RPC name. Caught by the automated review on this PR. Signed-off-by: SmokeDev <test@test.com>
|
Good catch — fixed in
Now derived from the variant: let rpc_method = match method {
ModelSwitchMethod::ConfigOption { .. } => "session/set_config_option",
ModelSwitchMethod::SetModel { .. } => "session/set_model",
};
|
1 participant
Problem
AcpError::Timeoutcarries only aDuration, so every request timeout produces the same sentence regardless of which call hung:That is the message users report, and it is not enough to place the fault. Sessions are created lazily inside the prompt task, so from the outside a stalled
session/newand a stalledsession/promptare indistinguishable — even though one is bounded atREQUEST_TIMEOUT(60s) and the other atDEFAULT_IDLE_TIMEOUT_SECS(900s).Concretely: diagnosing #4098 ("every managed prompt hangs until 60s") required tracing two codebases and reasoning backwards from the timeout constants to establish that only
session/newcould produce that number. The error itself should have said so.Change
send_requestalready has the method in hand, and it is&'static strat every call site, so carrying it costs nothing:The two directly-constructed timeouts in
pool.rsname theirs as well (session/set_model,session/set_config_option), so no timeout in the crate is anonymous.Match sites become
Timeout { .. }. Nothing else changes — the transport-class error classification inlib.rsand the retry handling inpool.rsbehave exactly as before.Verification
On Windows:
cargo check -p buzz-acp,cargo clippy -p buzz-acp --all-targets(0 warnings) andcargo fmt -p buzz-acp -- --check(no diffs) all clean.Not offering the crate's test suite as evidence: on this box it is non-deterministic against untouched
main(different failures per run, consistent with #2492), so a before/after comparison would be noise rather than a receipt.Note on a related asymmetry
Not changed here, but worth flagging while you have the file open:
session/newgets 60s while the prompt it precedes gets 900s idle / 7200s hard. An agent that legitimately needs ~90s to provision a session can therefore never succeed on any runtime, no matter how patient the prompt budget is. Happy to open that separately if it's worth addressing.Related: #4098. Duplicates: searched open PRs for
AcpError::Timeoutandrequest timeout— none found.