Phase 0 concurrency hardening: crypto async, bounded queue, docs - #607
Conversation
WFL web request handlers run one at a time today, but several docs claimed parallel / "don't block others" behavior. Rewrite them to distinguish the concurrent transport (accept / TLS) from serial application handlers, and prefer "concurrent" over "parallel". Add a panic-strategy gate so the runtime's future catch_unwind-based request-handler fault isolation (Phase 1) cannot be silently undermined: - src/lib.rs: `#[cfg(panic = "abort")] compile_error!` — reflects the crate's real panic strategy; Cargo force-unwinds test/bench harnesses, so it never trips `cargo test`. - Cargo.toml: pin `panic = "unwind"` on the release profile (explicit; the default was already unwind). - CI: a failing-first "assert panic=abort is rejected" step that forces abort via `--config` and fails the job if the build succeeds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPoNkFqfnjC1AcfcQjazky
…, PR-0c) Two runtime DoS mitigations that share the web-server dispatch path in src/interpreter/mod.rs, landed together. PR-0b — spawn_blocking for CPU-heavy crypto: - New src/stdlib/crypto_async.rs routes the 11 deliberately-slow crypto builtins (argon2/scrypt/bcrypt/pbkdf2 hash+verify, hash_password, verify_password, pbkdf2_hmac_sha256) onto Tokio's blocking pool. - The two async native-dispatch arms check the route first and await it, else fall back to the synchronous native. - Heavy compute stays in pub(crate) plain-data helpers in crypto.rs; args are extracted to owned String/u64/usize before the hop, so only plain data crosses the boundary and the interpreter core stays !Send. zeroize and the constant-time compare paths are unchanged. - Chosen over a new Value::AsyncNativeFunction variant to avoid rippling a new arm through every exhaustive Value match. PR-0c — bound the transport->interpreter queue (OOM shed): - New .wflcfg key web_server_request_queue_bound (default 256, zero rejected). - The request channel is now mpsc::channel(bound) instead of unbounded. The warp handler try_sends; on Full it logs and returns a 503 (with Retry-After) via overloaded_response(), without blocking the transport task. Tests: in-crate crypto_async tests (exact routed set; a deterministic off-thread proof on a current_thread runtime; hash/verify round-trips; routed PBKDF2 == direct); in-crate queue_bound_tests (well-formed 503; deterministic over-cap shed); tests/web_queue_bound_test.rs (config parsing). Existing crypto and web-server suites pass unchanged through the new paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPoNkFqfnjC1AcfcQjazky
- Dev Diary entry for Phase 0 concurrency hardening (0a/0b/0c), including the build.rs -> compile_error! dead end and the residual-risk notes. - Flip the Phase 0 rows (0a/0b/0c) to Done in the concurrency phase plan tracker. - CLAUDE.md: add a binding "Docs Must Be Honest (validate docs)" policy — docs describe what actually ships, planned behavior is marked as such, and every user-visible change ships validated docs AND a Dev Diary entry in the same change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPoNkFqfnjC1AcfcQjazky
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPhase 0 hardens concurrency behavior by rejecting ChangesPhase 0 concurrency hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HTTP route
participant WflWebServer
participant Interpreter
participant crypto_async
participant Tokio blocking pool
HTTP route->>WflWebServer: try_send HTTP request
alt queue available
WflWebServer->>Interpreter: deliver request
else queue full
WflWebServer-->>HTTP route: return 503 with Retry-After
end
Interpreter->>crypto_async: route crypto builtin
crypto_async->>Tokio blocking pool: spawn_blocking computation
Tokio blocking pool-->>Interpreter: return hash, verification result, or KDF output
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
Docs/development/concurrency-phase-plan.md (1)
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider checking off completed TODO items.
The tracker marks PR-0a, PR-0b, and PR-0c as ✅ Done, but the individual TODO checkboxes in the detailed sections below (lines 72–81, 103–116, 140–144) remain unchecked
[ ]. Checking them off (or noting why they're left open) would keep the tracker self-consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Docs/development/concurrency-phase-plan.md` around lines 46 - 48, Update the detailed TODO checkboxes for PR-0a, PR-0b, and PR-0c in their respective sections to checked `[x]` status, matching the completed statuses in the phase tracker. If any item remains intentionally incomplete, document the reason instead of marking it complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 53-64: Update the “Assert panic=abort is rejected” workflow step
to capture the failed cargo build output and verify it contains the expected
“WFL requires panic = "unwind"” compile-error message. Continue failing the step
when the build succeeds or when the expected message is absent, while preserving
the success path only for the intended panic-strategy rejection.
In `@Cargo.toml`:
- Around line 100-102: Update the comment above panic = "unwind" in Cargo.toml
to state that enforcement is provided by src/lib.rs’s #[cfg(panic = "abort")]
compile_error!, removing the inaccurate build.rs reference and keeping the
existing rationale intact.
In `@src/interpreter/mod.rs`:
- Around line 5499-5515: Move overload admission ahead of warp::body::bytes()
and body.to_vec() so saturated requests receive overloaded_response() before
buffering their bodies. Add a bounded, non-blocking in-flight body-reader
admission mechanism, release its permit when body collection completes or fails,
and retain the existing sender.try_send queue check for completed WFL requests.
- Around line 9992-10047: Move the queue_bound_tests coverage out of the inline
#[cfg(test)] module in src/interpreter/mod.rs and consolidate it with the
existing queue-bound integration tests under tests/. Place it in the
repository’s feature-oriented *_test.rs file, preserving both
overloaded_response validation and full_queue_sheds_deterministically behavior.
In `@src/stdlib/crypto_async.rs`:
- Around line 150-303: Move the tests from the internal tests module in
crypto_async.rs into tests/crypto_async_test.rs, adapting them to exercise the
public interpreter behavior instead of calling the private route function
directly. Preserve coverage for heavy-crypto routing, thread offloading, round
trips, deterministic PBKDF2 output, and argument errors, using only the public
APIs and existing test setup.
- Around line 73-79: The async crypto routes around extract_password and their
spawn_blocking closures must zeroize owned password buffers before they are
dropped. Wrap password String copies in Zeroizing<String> before moving them
into compute or verify operations, including the corresponding routes at all
referenced blocks; in verify_route, wrap only the password and leave the hash
handling unchanged.
---
Nitpick comments:
In `@Docs/development/concurrency-phase-plan.md`:
- Around line 46-48: Update the detailed TODO checkboxes for PR-0a, PR-0b, and
PR-0c in their respective sections to checked `[x]` status, matching the
completed statuses in the phase tracker. If any item remains intentionally
incomplete, document the reason instead of marking it complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65e10a59-5c5b-4fa6-8e04-cdfb385c3339
📒 Files selected for processing (19)
.github/workflows/ci.ymlCLAUDE.mdCargo.tomlDev diary/2026-07-11-phase-0-concurrency-hardening.mdDocs/01-introduction/key-features.mdDocs/04-advanced-features/async-programming.mdDocs/04-advanced-features/index.mdDocs/04-advanced-features/web-servers.mdDocs/06-best-practices/performance-tips.mdDocs/Archive/README.mdDocs/development/concurrency-phase-plan.mdDocs/reference/configuration-reference.mdsrc/config.rssrc/interpreter/mod.rssrc/lib.rssrc/stdlib/crypto.rssrc/stdlib/crypto_async.rssrc/stdlib/mod.rstests/web_queue_bound_test.rs
|
|
||
| /// Phase 0 (PR-0c): the bounded request queue sheds with a well-formed 503 when | ||
| /// the interpreter is saturated, instead of buffering work without bound. | ||
| #[cfg(test)] | ||
| mod queue_bound_tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn overloaded_response_is_a_well_formed_503() { | ||
| let resp = overloaded_response(); | ||
| assert_eq!(resp.status(), warp::http::StatusCode::SERVICE_UNAVAILABLE); | ||
| assert_eq!( | ||
| resp.headers() | ||
| .get("Content-Type") | ||
| .and_then(|v| v.to_str().ok()), | ||
| Some("text/plain; charset=utf-8") | ||
| ); | ||
| // Content-Length matches the actual body byte count. | ||
| let declared: usize = resp | ||
| .headers() | ||
| .get("Content-Length") | ||
| .and_then(|v| v.to_str().ok()) | ||
| .and_then(|s| s.parse().ok()) | ||
| .expect("Content-Length header present and numeric"); | ||
| assert_eq!(declared, resp.body().len()); | ||
| assert!(!resp.body().is_empty()); | ||
| } | ||
|
|
||
| /// The over-cap decision is deterministic: once a bounded channel is full, | ||
| /// `try_send` reports `Full` (which the warp handler maps to `overloaded_response`), | ||
| /// and never blocks or grows the queue. | ||
| #[tokio::test] | ||
| async fn full_queue_sheds_deterministically() { | ||
| let bound = 4usize; | ||
| let (tx, _rx) = mpsc::channel::<u32>(bound); | ||
|
|
||
| // Fill to capacity — every send within the bound succeeds. | ||
| for i in 0..bound { | ||
| tx.try_send(i as u32) | ||
| .expect("send within capacity succeeds"); | ||
| } | ||
| assert_eq!(tx.max_capacity(), bound); | ||
|
|
||
| // The next send over capacity sheds rather than blocking or growing. | ||
| match tx.try_send(999) { | ||
| Err(mpsc::error::TrySendError::Full(v)) => assert_eq!(v, 999), | ||
| other => panic!("expected Full over capacity, got {other:?}"), | ||
| } | ||
|
|
||
| // The shed maps to a 503. | ||
| assert_eq!( | ||
| overloaded_response().status(), | ||
| warp::http::StatusCode::SERVICE_UNAVAILABLE | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move queue tests under tests/.
This new #[cfg(test)] module is in src/interpreter/mod.rs; the repository requires unit and integration tests under tests/ using feature-oriented _test.rs filenames. Consolidate these with the queue-bound integration coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/interpreter/mod.rs` around lines 9992 - 10047, Move the queue_bound_tests
coverage out of the inline #[cfg(test)] module in src/interpreter/mod.rs and
consolidate it with the existing queue-bound integration tests under tests/.
Place it in the repository’s feature-oriented *_test.rs file, preserving both
overloaded_response validation and full_queue_sheds_deterministically behavior.
Source: Coding guidelines
- ci.yml: the panic=abort gate now asserts our specific compile_error
message ('WFL requires panic = "unwind"') instead of accepting any
non-zero build exit, so an unrelated build failure can't masquerade as
the gate firing.
- Cargo.toml: fix the stale "Enforced by build.rs" comment — the gate lives
in src/lib.rs (`#[cfg(panic = "abort")] compile_error!`).
- crypto_async.rs: wrap the transient password copies in Zeroizing<String>
so they are wiped when the blocking closure that owns them is dropped
(verify keeps the non-secret stored hash as a plain String).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BPoNkFqfnjC1AcfcQjazky
Addresses the remaining CodeRabbit findings on PR #607. Body admission (Stability): the 503 shed previously ran only after warp had already buffered the request body, so N concurrent transport tasks could each allocate up to web_server_max_body_size first. Add a per-listener in-flight Semaphore (sized to web_server_request_queue_bound) and acquire a permit in a filter *before* warp::body::bytes(); on saturation reject with a new `Overloaded` rejection that `handle_overloaded` maps to 503, so the body is never buffered. The permit is held for the request's lifetime and released on drop. The existing try_send queue check is retained as a second gate. Test location (repo convention): move the inline `#[cfg(test)]` modules out of src/. `crypto_async::route` and `interpreter::overloaded_response` are now `pub`, and the tests live in tests/crypto_async_test.rs and tests/web_queue_bound_test.rs. The crypto round-trips now verify through the public route (hash then verify) instead of crate-internal helpers; a new semaphore test covers the admission gate's deterministic shed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPoNkFqfnjC1AcfcQjazky
| /// Public so the interpreter dispatch (and the `tests/crypto_async_test.rs` | ||
| /// integration tests) can drive it; not part of the stable language surface. | ||
| pub fn route( | ||
| name: &str, | ||
| args: &[Value], | ||
| ) -> Option<LocalBoxFuture<'static, Result<Value, RuntimeError>>> { |
| /// Build the 503 response returned when the transport→interpreter request queue | ||
| /// is full (Phase 0, PR-0c). A free function so the shed path can be tested | ||
| /// without standing up a live server. | ||
| pub fn overloaded_response() -> warp::http::Response<Vec<u8>> { | ||
| let body = b"Service Unavailable: the server is overloaded, please retry later\n".to_vec(); |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/interpreter/mod.rs (1)
5485-5492: 🩺 Stability & Availability | 🔵 TrivialIn-flight permit is held for the full request lifetime, including an untimed response wait.
The admission permit is released only when this handler scope ends, which spans
response_receiver.await(Line 5567) — and that await has no timeout. A WFL handler that receives a request but never callsrespondleaves the oneshot unresolved, so the permit is held indefinitely. Afterqueue_boundsuch requests accumulate, every subsequent request is shed with 503, taking the whole server offline. Under the previous unbounded design this same bug only leaked memory; with the bounded semaphore it becomes an availability failure.Consider bounding the transport-side response wait (e.g.
tokio::time::timeout) so a stuck/never-responded request eventually returns 5xx/504 and frees its permit, rather than degrading the server permanently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/interpreter/mod.rs` around lines 5485 - 5492, Bound the response wait in the request handler closure that owns _permit, including the response_receiver.await path, with an appropriate tokio timeout. On timeout, return the existing 5xx/504-style error response and let the handler finish so the admission permit is released; preserve normal response handling when the receiver responds in time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/interpreter/mod.rs`:
- Around line 5485-5492: Bound the response wait in the request handler closure
that owns _permit, including the response_receiver.await path, with an
appropriate tokio timeout. On timeout, return the existing 5xx/504-style error
response and let the handler finish so the admission permit is released;
preserve normal response handling when the receiver responds in time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ae9027e-3bae-4932-b54b-ec713667d573
📒 Files selected for processing (7)
.github/workflows/ci.ymlCargo.tomlDocs/reference/configuration-reference.mdsrc/interpreter/mod.rssrc/stdlib/crypto_async.rstests/crypto_async_test.rstests/web_queue_bound_test.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- Cargo.toml
- Docs/reference/configuration-reference.md
…#607) Two review follow-ups. Availability (CodeRabbit): the in-flight admission permit was held across the untimed response wait, so a WFL handler that never calls `respond` would pin its permit forever — after `web_server_request_queue_bound` such stuck requests, the semaphore is exhausted and every new request is shed with 503, taking the server offline (my admission change had turned a memory leak into an availability failure). Release the permit right after `try_send` enqueues the request: it still bounds concurrent body buffering (bodies are buffered before enqueue), but a request awaiting a response holds neither a permit nor a body. A real per-request response timeout remains Phase 1 scope. Public API (Copilot): `crypto_async::route` returned `futures_util`'s `LocalBoxFuture`, leaking a third-party type into the public signature. Return a plain `std` `Pin<Box<dyn Future<...>>>` (type alias `RoutedFuture`) via `Box::pin`, dropping the `futures_util` import. Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPoNkFqfnjC1AcfcQjazky
| /// Boxed future produced by [`route`]. A plain `std` type (rather than a | ||
| /// `futures_util` alias) so the public signature doesn't leak a third-party type | ||
| /// into the crate's API. It is awaited only on the interpreter thread. | ||
| type RoutedFuture = Pin<Box<dyn Future<Output = Result<Value, RuntimeError>>>>; |
…607) `route` is `pub` and returns `RoutedFuture`, so the alias should share that visibility. (The private alias already compiled — E0446 does not fire for a private alias of fully-public std types, confirmed by CI/tests — but a public alias for a public function's return type is clearer and removes any doubt.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPoNkFqfnjC1AcfcQjazky
| let password = expect_text(&args[0])?; | ||
| let salt = expect_text(&args[1])?; | ||
| let iterations = expect_count("pbkdf2_hmac_sha256", "iterations", &args[2])?; | ||
| let length = expect_count("pbkdf2_hmac_sha256", "length", &args[3])? as usize; | ||
|
|
| check_arg_count(FUNC, args, 4)?; | ||
| let password = Zeroizing::new(expect_text(&args[0])?.to_string()); | ||
| let salt = expect_text(&args[1])?.to_string(); | ||
| let iterations = crypto::expect_count(FUNC, "iterations", &args[2])?; | ||
| let length = crypto::expect_count(FUNC, "length", &args[3])? as usize; | ||
| Ok::<_, RuntimeError>((password, salt, iterations, length)) |
) `length` was cast `u64 as usize` at the call sites, which truncates on 32-bit targets (e.g. wasm32): a value above usize::MAX could wrap to a small number and slip past the MAX_PBKDF2_KEY_LENGTH check, deriving a wrong-length key instead of erroring. Thread the raw u64 into pbkdf2_hmac_sha256_str and convert once with a checked usize::try_from that returns a clear RuntimeError on overflow. No change on 64-bit; existing KDF vector tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPoNkFqfnjC1AcfcQjazky
| async fn handle_overloaded( | ||
| err: warp::Rejection, | ||
| ) -> Result<warp::http::Response<Vec<u8>>, warp::Rejection> { | ||
| if err.find::<Overloaded>().is_some() { | ||
| Ok(overloaded_response()) | ||
| } else { | ||
| Err(err) | ||
| } | ||
| } |
Summary
Implements Phase 0 of the concurrency hardening plan to close three concrete DoS gaps in public WFL web servers without requiring an interpreter rewrite:
Key Changes
0b —
spawn_blockingfor CPU-heavy cryptosrc/stdlib/crypto_async.rswith a name-keyedroute(name, args)function that hops 11 heavy crypto builtins onto the blocking pool:argon2_hash,scrypt_hash,pbkdf2_hash,bcrypt_hash,hash_passwordargon2_verify,scrypt_verify,pbkdf2_verify,bcrypt_verify,verify_passwordpbkdf2_hmac_sha256String,u64,usize) on the interpreter thread before the hop; only that plain data crosses the boundary; the resultingValueis rebuilt after the.await.!Send— noRc/RefCell/Value/Environmentever crosses a thread boundary.src/stdlib/crypto.rsto expose plain-data cores (argon2_hash_str,*_verify_str,pbkdf2_hmac_sha256_str) that can run off-thread.src/interpreter/mod.rs(FunctionCall,ActionCall) now checkroute()first and.awaitit, falling back to synchronous native otherwise.0c — Bounded request queue (OOM shed)
web_server_request_queue_boundinsrc/config.rs(default 256, minimum 1).mpsc::unbounded_channeltompsc::channel(bound).try_send: onFullit logs a warning and returns a well-formed 503 withRetry-Afterheader via newoverloaded_response()helper; onClosedit keeps the existing rejection.0a — Docs honesty +
panic = "unwind"gatesrc/lib.rs:#[cfg(panic = "abort")] compile_error!enforces that the crate is compiled withpanic = "unwind"(required for Phase 1'scatch_unwind-based fault isolation).Cargo.tomlpinspanic = "unwind"explicitly in[profile.release].wait for.Tests
src/stdlib/crypto_async.rsunit tests: exact routed-set map; deterministic off-thread proof (ticker advances during crypto.awaiton single-threaded runtime); routed hash/verify round-trips (argon2, bcrypt); routed PBKDF2 byte-identical to direct helper; argument errorshttps://claude.ai/code/session_01BPoNkFqfnjC1AcfcQjazky
Summary by CodeRabbit
web_server_request_queue_bound, default 256).