Skip to content

fix: clippy under Rust 1.99.0 (async-trait 0.1.92, try_update) - #1559

Merged
thepagent merged 2 commits into
mainfrom
fix/ci-rust-1.99-clippy-code
Oct 3, 2026
Merged

thepagent merged 2 commits into
mainfrom
fix/ci-rust-1.99-clippy-code

Conversation

@chaodu-obk

@chaodu-obk chaodu-obk Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Stable Rust moved to 1.99.0 on 2026-10-01. The CI / check job installs floating stable, so since then cargo clippy --workspace -- -D warnings fails on main and on every PR that touches core code (for example #1557). The failures are in files no recent PR touched:

  • async-trait 0.1.89 expansions trip clippy::double_must_use. That is 16 errors at openab-core/src/adapter.rs:334, openab-core/src/dispatch.rs:123 and openab-mcp/src/mcp/sources.rs:50. Fixed by bumping async-trait to 0.1.92 in the root Cargo.lock; openab-agent/Cargo.lock gets the same bump, with its new syn 3 entry pinned to the same 3.0.3 as root, so a future #[async_trait] use there cannot hit the same lint. syn 3.0.3 was already in the root Cargo.lock, so this adds no new crate version to the repo. Only the async-trait -> syn edge changes.
  • AtomicUsize::fetch_update is deprecated in favor of try_update (openab-cp/src/registry.rs:50). Since 1.99.0, fetch_update is documented as an alias for try_update (Stabilize atomic_try_updateand deprecate fetch_update starting 1.99.0 rust-lang/rust#148590, tracking issue #135894). The two have the same parameter order (set_order, fetch_order), the same return contract (Ok(prev) / Err(prev)), and the same CAS retry loop: the closure may run more than once and is applied once. Memory orderings and the .is_ok() check are therefore unchanged.

This is a stopgap that unblocks CI. It does not fix the root cause, which is that CI floats on stable. See Follow-ups.

Review Contract

Goal

cargo clippy --workspace -- -D warnings and --features unified pass on Rust 1.99.0, so CI / check is green on main again.

Non-goals

  • Toolchain pinning. Pinning has to be decided across CI, Docker builders and local dev together; see Follow-ups.
  • Any behavior change.

Accepted Residual Risks

  • The operator workspace was not clippy-checked on 1.99.0 locally (out of memory compiling aws-sdk-ec2). This PR does not touch operator/.
  • Until a pin lands, the next stable release (1.100) can break CI the same way.

Acceptance Criteria

  • CI / check passes on this PR.
  • No source change other than fetch_update to try_update.

Follow-ups

  • Toolchain pin policy, before Rust 1.100. The options interact:
    • toolchain: on the workflow dtolnay/rust-toolchain steps. This needs workflows permission.
    • A root rust-toolchain.toml. It does not need workflows permission, but the official rust:* images are rustup-based, so it would also override the digest-pinned 1.96.0 builder in Dockerfile.unified and add a toolchain download to every image build.
    • The rust:1-bookworm builders float in about 18 Dockerfiles. A CI-only pin leaves image builds drifting.
  • OutboundBudget::reserve relies on its closure being pure, because it can run more than once. A one-line comment would prevent a future side effect from silently running repeatedly.
  • Reversibility: roll forward only. On floating stable (>= 1.99), a plain git revert restores fetch_update and async-trait 0.1.89 and re-breaks cargo clippy -- -D warnings. A revert is only safe after the CI toolchain is pinned to <= 1.98.

Validation

Linux x86_64, rustc/cargo/clippy 1.99.0:

  • Before (base 739c344d): cargo clippy --workspace -- -D warnings exits 101 with the 16 double_must_use errors above.
  • After:
    • cargo clippy --workspace -- -D warnings: exit 0
    • cargo clippy --workspace --features unified -- -D warnings: exit 0
    • cargo test --workspace: all passed, 0 failed
    • cargo test -p openab-cp: registry::tests::* pass, including outbound_queue_is_bounded_in_bytes_not_only_entries
  • Compatibility:
    • try_update is stable since 1.95.0, and the digest-pinned builder in Dockerfile.unified is Rust 1.96.0.
    • The MSRV of async-trait 0.1.92 and syn 3.0.3 is 1.71.
    • git grep fetch_update -- '*.rs' returns nothing after the change.
  • CI on head a1143f0b: cargo clippy and cargo clippy (unified) pass on runner stable.
  • openab-agent (standalone workspace, same steps as ci-openab-agent.yml) on rustc 1.99.0:
    • cargo clippy --locked -- -D warnings: exit 0
    • cargo test --locked: 62 passed, 0 failed, 11 ignored

Stable moved to Rust 1.99.0 on 2026-10-01 and the CI check job (floating
stable) started failing cargo clippy on main:

- async-trait 0.1.89 expansions trip clippy::double_must_use (16 errors
  in openab-core adapter.rs/dispatch.rs and openab-mcp sources.rs).
  Bump async-trait to 0.1.92 in Cargo.lock.
- AtomicUsize::fetch_update is deprecated in favor of try_update
  (stable since 1.95.0) in openab-cp registry.rs.
@chaodu-obk

This comment has been minimized.

Bump async-trait 0.1.89 -> 0.1.92 in openab-agent/Cargo.lock, pinning the
new syn 3 edge to 3.0.3 to match the root lockfile. Keeps both lockfiles on
the same async-trait version so a future #[async_trait] use in openab-agent
does not re-trigger clippy::double_must_use on Rust >= 1.99.

Verified on rustc 1.99.0 in openab-agent: cargo clippy --locked -- -D warnings
exits 0; cargo test --locked: 62 passed, 0 failed.
@chaodu-obk

chaodu-obk Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Note

LGTM ✅ - Both round-1 blockers are resolved on 798c3d8b: the openab-agent lockfile is aligned to async-trait 0.1.92, and the Reversibility line now accurately says roll-forward only. No remaining 🔴 or 🟡 findings.

What This PR Does

Stable Rust 1.99.0 turned cargo clippy --workspace -- -D warnings red on main (16 clippy::double_must_use errors from async-trait 0.1.89 expansions, plus the AtomicUsize::fetch_update deprecation). This PR bumps async-trait to 0.1.92 in both lockfiles that contain it and renames the single fetch_update call to try_update.

How It Works

  • Cargo.lock (root): async-trait 0.1.89 -> 0.1.92; its syn edge moves to the syn 3.0.3 entry that was already in the lockfile (via ref-cast-impl).
  • openab-agent/Cargo.lock: same async-trait 0.1.92 bump. The new syn 3 entry is pinned with --precise to 3.0.3 (cargo initially selected 3.0.6), so both lockfiles use byte-identical artifacts (async-trait checksum 82f6aeea..., syn 3.0.3 checksum 53e9bae5...). The remaining "syn" -> "syn 2.0.117" lines are cargo's standard disambiguation once two syn versions coexist; no other package version changes.
  • crates/openab-cp/src/registry.rs:50: fetch_update(AcqRel, Acquire, f) -> try_update(AcqRel, Acquire, f). In 1.99.0 fetch_update is documented as an alias for try_update; parameter order, Ok(prev) / Err(prev) contract, and CAS retry loop are identical. reserve() still returns .is_ok().

Findings

# Severity Finding Location
1 🟢 Round-1 F1 resolved: openab-agent/Cargo.lock aligned to async-trait 0.1.92 / syn 3.0.3, same checksums as root openab-agent/Cargo.lock
2 🟢 Round-1 F2 resolved: Reversibility now states roll-forward only and when a revert is safe PR body -> Follow-ups
3 🟢 try_update is a verified zero-behavior-change rename; boundary cases unchanged crates/openab-cp/src/registry.rs:47-56
4 🟢 try_update (not update) correctly preserves the budget refusal path crates/openab-cp/src/registry.rs:50
5 🟢 Supply chain: no new crate versions in the repo, checksums match crates.io, yanked 0.1.90 skipped Cargo.lock, openab-agent/Cargo.lock
6 🟢 async-trait #303 removes only a duplicate #[must_use]; a missing .await is still a compile error -
7 🟢 Docker builder floor verified: Dockerfile.unified digest is Rust 1.96.0 (>= 1.95 required by try_update) Dockerfile.unified:28
8 🟢 Minimal, well-scoped diff; root-cause fix preferred over lint allows or relaxing -D warnings -
Finding Details

🟢 F1: openab-agent lockfile aligned (resolves round-1 F1)

Commit 798c3d8b changes only openab-agent/Cargo.lock (+38 / -27). The only version change is async-trait 0.1.89 -> 0.1.92; one new syn 3.0.3 entry is added for it; all other syn references are rewritten to the qualified syn 2.0.117 form, with no version change. Verified on rustc 1.99.0 with the same steps as ci-openab-agent.yml (standalone [workspace]): cargo clippy --locked -- -D warnings exits 0, and cargo test --locked reports 62 passed, 0 failed, 11 ignored. These local results were produced by the reviewer who pushed the fix and have not been re-run independently; ci-openab-agent.yml is path-triggered on openab-agent/** and will provide the independent signal. One-time cost: the first clean build compiles syn 3 and the new async-trait on the host; there is no runtime or hot-path impact (proc-macro dependency only).

🟢 F2: Reversibility wording (resolves round-1 F2)

The body now says: roll forward only; on floating stable (>= 1.99) a plain git revert restores fetch_update and async-trait 0.1.89 and re-breaks cargo clippy -- -D warnings; a revert is safe only after the CI toolchain is pinned to <= 1.98. This is accurate and correctly scoped to the clippy gate (Docker image builds use cargo build and would still compile).

🟢 F3: try_update rename verified

Checked against the 1.99.0 std docs: fetch_update is deprecated ("renamed to try_update for consistency") and documented as an alias of try_update. Ordering roles (AcqRel success / Acquire failure) are unchanged and valid. Edge cases are identical: checked_add overflow -> None -> no store; next == max accepted; n == 0 succeeds. The existing test outbound_queue_is_bounded_in_bytes_not_only_entries already asserts that a refused reservation does not leak budget and that a later smaller frame is still accepted (registry.rs:506-512). The closure is pure and allocation-free, so CAS retries cost the same as before.

🟢 F4: Right API choice

The sibling AtomicUsize::update (also stable in 1.95) always stores and cannot refuse; using it would have silently removed the byte-budget refusal path. try_update keeps it.

🟢 F5: Supply chain

async-trait 0.1.92 and syn 3.0.3 checksums match crates.io in both lockfiles. Neither version is yanked; both are well past a 7-day age threshold and come from the long-standing maintainer. syn 3.0.3 already existed in the root lockfile, so the repo gains no crate version it did not already have. The yanked 0.1.90 is skipped.

🟢 F6: async-trait #303 loses no protection

0.1.91 updates to syn 3 and fixes receiver mutability (#301, binding-only, no Send / lifetime change). 0.1.92 (#303) drops the generated #[must_use] on trait methods. Upstream tests/ui/must-use.stderr still reports unused pinned boxed Future trait object that must be used for a discarded call, so rustc continues to catch a missing .await; #303 removes only the duplicate diagnostic.

🟢 F7: Builder toolchain floor

try_update raises the effective MSRV to 1.95. The digest pinned in Dockerfile.unified:28 resolves to an image config with RUST_VERSION=1.96.0 (created 2026-06-24). On the previous head, build-builder and every smoke-test-unified (*) job passed with this code. All other Dockerfiles float rust:1-bookworm.

🟢 F8: Scope and approach

Each change maps to one stated cause. Alternatives (#[allow(deprecated)], allowing double_must_use, --cap-lints, or dropping -D warnings) all weaken the gate. Pinning CI to <= 1.98 would only defer the work; pinning to 1.99.0 still requires these exact changes.

Round-1 Resolution
Round-1 finding Status
🟡 F1 openab-agent/Cargo.lock still on async-trait 0.1.89 ✅ Addressed in 798c3d8b (option (a): in-PR bump, syn pinned to 3.0.3)
🟡 F2 Reversibility line inaccurate ✅ Addressed in the PR body

There are no inline review threads or external review comments on this PR.

Non-blocking Follow-ups
  • Toolchain pin policy before Rust 1.100. The pin must cover every floating-stable clippy gate (ci.yml check and operator jobs, ci-openab-agent.yml, ci-agy-acp.yml, ci-auth-proxy.yml, etc.), plus build-binaries.yml (runner-preinstalled Rust) and build-operator-branch.yml (rustup script). Whoever pins must also own a periodic bump process.
  • Declare rust-version = "1.95" so the new floor is explicit rather than implied by the Dockerfile.unified digest. Any future digest re-pin must stay >= 1.95.
  • Optional hardening for the existing OutboundBudget (not introduced by this PR): a checked_add overflow test and a concurrent reserve() test at the exact limit.
  • The one-line purity comment on OutboundBudget::reserve's closure already listed in the PR body.
Baseline Check
  • PR opened: 2026-10-03
  • Base: main @ 739c344d (merge-base 739c344d, not stacked); head 798c3d8b1ab5b112edbf54aa07e8800771af7ba3
  • Diff: 3 files (Cargo.lock, openab-agent/Cargo.lock, crates/openab-cp/src/registry.rs); the only source change is the one-line rename
  • Main already has: syn 3.0.3 in the root Cargo.lock; no other fetch_update call sites anywhere in the tree; operator/, agy-acp/ and crates/platform-schema/ lockfiles contain no async-trait
  • Net-new value: unblocks CI / check on Rust 1.99.0 and removes the latent repeat in openab-agent
  • CI on this head at the time of writing: completed checks are green (including check jobs, validate, changes, validate-packaged-pins, native-sandbox smoke test); build-builder and the remaining smoke tests are still running. This review status covers the review only; CI checks are gated separately.
What's Good (🟢)
  • Root-cause fix at the smallest possible layer; no lint suppression.
  • Both lockfiles now use identical, already-vetted artifacts.
  • Review Contract is complete, and the updated body is accurate about scope, residual risks, and rollback.

5. Three Reasons We Might Not Need This PR

  1. Pin the toolchain instead - pinning CI to 1.98 would turn CI green with zero code churn. Rebuttal: it only defers the work, leaves local stable builds and floating Docker builders red, and still needs a human with workflows permission.
  2. Suppress the lints - #[allow(deprecated)] on one call site plus a double_must_use allow would avoid the lockfile bumps. Rebuttal: it weakens the gate and creates allow-debt that must be removed when fetch_update is eventually removed.
  3. Manual multi-lockfile sync is fragile - this PR initially missed openab-agent/Cargo.lock. Rebuttal: that is now fixed here; a repo-wide lockfile consistency check is a reasonable separate improvement, not a reason to block this fix.

@thepagent
thepagent merged commit 760ff9c into main Oct 3, 2026
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants