Skip to content

chore: document intentional await-holding cases#18423

Merged
bolinfest merged 1 commit into
mainfrom
pr18423
Apr 21, 2026
Merged

chore: document intentional await-holding cases#18423
bolinfest merged 1 commit into
mainfrom
pr18423

Conversation

@bolinfest

@bolinfest bolinfest commented Apr 17, 2026

Copy link
Copy Markdown
Collaborator

Why

This PR prepares the stack to enable Clippy await-holding lints that were left disabled in #18178. The mechanical lock-scope cleanup is handled separately; this PR is the documentation/configuration layer for the remaining await-across-guard sites.

Without explicit annotations, reviewers and future maintainers cannot tell whether an await-holding warning is a real concurrency smell or an intentional serialization boundary.

What changed

  • Configures clippy.toml so await_holding_invalid_type also covers tokio::sync::{MutexGuard,RwLockReadGuard,RwLockWriteGuard}.
  • Adds targeted #[expect(clippy::await_holding_invalid_type, reason = ...)] annotations for intentional async guard lifetimes.
  • Documents the main categories of intentional cases: active-turn state transitions that must remain atomic, session-owned MCP manager accesses, remote-control websocket serialization, JS REPL kernel/process serialization, OAuth persistence, external bearer token refresh serialization, and tests that intentionally serialize shared global or session-owned state.
  • For external bearer token refresh, documents the existing serialization boundary: holding cached_token across the provider command prevents concurrent cache misses from starting duplicate refresh commands, and the current behavior is small enough that an explicit expectation is easier to maintain than adding another synchronization primitive.

Verification

  • cargo clippy -p codex-login --all-targets
  • cargo clippy -p codex-connectors --all-targets
  • cargo clippy -p codex-core --all-targets
  • The follow-up PR chore: enable await-holding clippy lints #18698 enables await_holding_invalid_type and await_holding_lock as workspace deny lints, so any undocumented remaining offender will fail Clippy.

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest changed the base branch from main to pr18418 April 17, 2026 23:48
@bolinfest bolinfest requested a review from a team as a code owner April 20, 2026 16:54
bolinfest added a commit that referenced this pull request Apr 20, 2026
This is the second cleanup in the await-holding lint stack. The
higher-level goal, following #18178
and #18398, is to enable Clippy
coverage for guards held across `.await` points without carrying broad
suppressions.

The stack is working toward enabling Clippy's
[`await_holding_lock`](https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock)
lint and the configurable
[`await_holding_invalid_type`](https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_invalid_type)
lint for Tokio guard types.

Several existing fields used `tokio::sync::Mutex<()>` only as
one-at-a-time async gates. Those guards intentionally lived across
`.await` while an operation was serialized. A mutex over `()` suggests
protected data and trips the await-holding lint shape; a single-permit
`tokio::sync::Semaphore` expresses the intended serialization directly.

## What changed

- Replace `Mutex<()>` serialization gates with `Semaphore::new(1)` for
agent identity ensure, exec policy updates, guardian review session
reuse, plugin remote sync, managed network proxy refresh, auth token
refresh, and RMCP session recovery.
- Update call sites from `lock().await` / `try_lock()` to
`acquire().await` / `try_acquire()`.
- Map closed-semaphore errors into the existing local error types, even
though these semaphores are owned for the lifetime of their managers.
- Update session test builders for the new
`managed_network_proxy_refresh_lock` type.

## Verification

- The split stack was verified at the final lint-enabling head with
`just clippy`.





---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18403).
* #18698
* #18423
* #18418
* __->__ #18403
@bolinfest bolinfest force-pushed the pr18423 branch 2 times, most recently from e029db9 to a7cecda Compare April 21, 2026 00:13
@bolinfest bolinfest force-pushed the pr18418 branch 2 times, most recently from aa6580e to 80662ac Compare April 21, 2026 00:17
@bolinfest bolinfest force-pushed the pr18423 branch 2 times, most recently from ad78353 to 793639e Compare April 21, 2026 01:54
Base automatically changed from pr18418 to main April 21, 2026 02:23
bolinfest added a commit that referenced this pull request Apr 21, 2026
## Why

This is part of the follow-up work from #18178 to make Codex ready for
Clippy's
[`await_holding_lock`](https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock)
/
[`await_holding_invalid_type`](https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_invalid_type)
lints.

This bottom PR keeps the scope intentionally small:
`NetworkProxyState::record_blocked()` only needs the state write lock
while it mutates the blocked-request ring buffer and counters. The debug
log payload and `BlockedRequestObserver` callback can be produced after
that lock is released.

## What changed

- Copies the blocked-request snapshot values needed for logging while
updating the state.
- Releases the `RwLockWriteGuard` before logging or notifying the
observer.

## Verification

- `cargo test -p codex-network-proxy`


---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18418).
* #18698
* #18423
* __->__ #18418
@bolinfest bolinfest force-pushed the pr18423 branch 2 times, most recently from aca6ccc to 4883044 Compare April 21, 2026 02:31
@bolinfest bolinfest merged commit d62421d into main Apr 21, 2026
39 of 50 checks passed
@bolinfest bolinfest deleted the pr18423 branch April 21, 2026 05:41
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 21, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants