Skip to content

fix(retry): retry a 408 Request Timeout, which is not a request defect - #613

Merged
vladimirrott merged 3 commits into
lacs-project:mainfrom
kragent66-glitch:fix/retry-408
Oct 19, 2026
Merged

vladimirrott merged 3 commits into
lacs-project:mainfrom
kragent66-glitch:fix/retry-408

Conversation

@kragent66-glitch

Copy link
Copy Markdown
Contributor

A 408 Request Timeout is the one other 4xx worth retrying.

Every other permanent 4xx says the request we built is wrong, so rebuilding it produces the same
wrong bytes and re-sending is pointless. That is the policy #589 put in place, after it stopped
permanent 4xx responses from being retried three times.

A 408 is different in kind. It says the server gave up reading the request before the body
arrived -- a statement about the clock, not about the bytes. Nothing about our request is known to
be wrong, so re-sending it unchanged is the whole fix. It retries despite being a 4xx.

Follow-up to #589, opened as its own PR as vladimirrott asked in review:

"On your question about the 408: open it as its own PR against main, with the test that a 408 goes
out twice. The paused clock can follow the same way. I'll hold both for you."

The paused-clock piece is not included here; it can follow the same way in its own PR so this one
stays a single concern.

Changes

  • crates/sysknife-brain/src/provider.rs -- 408 joins 5xx and 429 in is_retryable, with the
    reasoning written next to the policy, plus a unit test pinning it. The comment that used to say
    "the one 4xx worth re-asking about" now names both cases and why they differ.
  • crates/sysknife-brain/src/planner.rs -- a_408_request_timeout_goes_out_twice: a stub that
    times out on the first attempt and answers on the second, asserting the request reached the
    provider exactly twice and that the retry re-sent it unchanged (no correction appended, unlike
    the retryable 400 tool-use case, whose bytes do change).
  • Test baseline 1,991 -> 1,993 in README.md, docs/introduction.md, docs/distro-support.md
    and tests/evidence/workspace-tests.json.

Validation

  • cargo nextest run --workspace --locked -- 1993 tests run: 1993 passed, 6 skipped
  • cargo fmt --all -- clean (no diff)
  • cargo test -p sysknife-brain --all-features -- 80 passed, 0 failed

A 408 says the server gave up reading the request before the body arrived --
it is a statement about the clock, not about the bytes we built. Every other
permanent 4xx means the request itself is wrong and rebuilding it produces the
same wrong bytes, so re-sending is pointless. A 408 is the exception: the
identical request is worth sending again unchanged.

This is the follow-up the maintainer asked for on lacs-project#589, which fixed the
opposite case (permanent 4xx being retried three times).

  - provider.rs: 408 joins 5xx and 429 in `is_retryable`, with the reasoning
    written where the policy lives, plus a unit test pinning it.
  - planner.rs: `a_408_request_timeout_goes_out_twice` -- a stub that times out
    on the first attempt and answers on the second, asserting the request
    reached the provider exactly twice and that the retry re-sent it unchanged
    (no correction appended, unlike the retryable 400 tool-use case).
  - test baseline: 1,991 -> 1,993 Rust tests, updated in README.md,
    docs/introduction.md, docs/distro-support.md and tests/evidence/.
cargo audit refuses rustls 0.23.45 with four vulnerabilities published on
2026-10-14 -- RUSTSEC-2026-0346 through -0349 (ECH rejection buffering, Brotli
certificate decompression, a ServerHello key-exchange downgrade, and TLS secrets
exposed through Debug impls). All four declare `patched = ">= 0.23.46"`, so the
single-package lockfile bump clears them.

This is not caused by the 408 change: security-audit fails on main for the same
reason (the advisories landed after main's last green run). It is included here
because the gate cannot pass otherwise, and a lockfile-only dependency bump is
maintenance rather than development.

Cargo.lock: rustls 0.23.45 -> 0.23.46 (version + checksum, no other crate moved).

Verified: cargo nextest run --workspace --locked -- 1993 passed, 6 skipped.
@kragent66-glitch

Copy link
Copy Markdown
Contributor Author

CI note on security-audit.

The red check is not from the 408 change. cargo audit refuses rustls 0.23.45, and it started refusing it three days ago: four advisories were published on 2026-10-14 -- RUSTSEC-2026-0346 (Debug impls exposing TLS secrets, medium), -0347 (ECH rejection buffering), -0348 (Brotli certificate decompression), -0349 (ServerHello key-exchange downgrade). All four declare patched = ">= 0.23.46".

Two things worth knowing:

  • It fails on main for the same reason. main's 13 Oct runs passed; the 16 Oct run (head 10d74838) fails on this job. The advisory database moved, not the code.
  • It is also not the same failure I was asked to fix here -- that was the 408 retry itself, which is unchanged in this PR.

Since the branch cannot go green while the lockfile carries four live vulnerabilities, I added a lockfile-only bump as a second commit, which the repo's operating notes treat as maintenance rather than development:

8fc2aff chore(deps): lock rustls 0.23.46, clearing four RustSec advisories

Cargo.lock: rustls 0.23.45 -> 0.23.46, version and checksum, nothing else moved. Re-verified after the bump with the repo's own gate:

cargo nextest run --workspace --locked
Summary [20.142s] 1993 tests run: 1993 passed, 6 skipped

The three remaining cargo audit warnings (proc-macro-error unmaintained, event-listener unsound, glib unsound) are the catalogued GUI-transitive ones in docs/security/audit-advisories.md and are non-fatal by design.

If you would rather land the dependency bump as its own PR and keep this one to the retry change alone, say so and I'll split it out -- the 408 fix is a self-contained commit either way.

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, @kragent66-glitch. This is a clean follow-up to your #589: one more arm in is_retryable, a comment that says why a 408 is a statement about the clock and not about the bytes, and a planner test that builds its error through map_rig_error, so it exercises the path a real provider response takes.

Reviewed at 8fc2aff. What I checked:

  • The retry decision sits on the live path: every planning turn goes through complete_with_retry (planner.rs:1096), which asks is_retryable (provider.rs:241). Both adapters hand a 408 over as ProviderError::Http with its status.
  • With || *status == 408 removed, both new tests fail. That's the merge receipt.
  • fmt, clippy --all-targets, doc and the test baseline (1,993 tests, matching the three doc figures) all pass on the merged tree.
  • Observable behaviour: a 408 gets the 5xx budget, three attempts with 400 ms then 800 ms backoff, so a persistent 408 costs about 1.2 s before it surfaces. Every other permanent 4xx still fails on the first try.

Your rustls commit is byte-identical to #616, which merged since, so it drops out when I update the branch. Bumping it to get past a red required check you didn't cause was reasonable. I'll merge once the board is green.

For a next one: #621 asks the MCP plan and execute results to name the host they came from, so an agent driving two daemons can't mix them up. It's small and self-contained, and the mock daemon you'd test against already exists. I'll offer it on the issue once this is in.

@vladimirrott
vladimirrott merged commit 08f8ee9 into lacs-project:main Oct 19, 2026
13 checks passed
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.

2 participants