[lenny] fix(test): deterministic fake_llm steer/cancel assertions - #46
Merged
Conversation
Two tests raced the wire and failed intermittently across every open PR, including PRs that touch no Rust at all (#20, #25), corrupting CI signal. steer_rejected_on_run_id_mismatch: the drain loop broke as soon as the prompt response arrived, so a turn finishing before the steer rejection left saw_reject false. Use recv_until on the steer id, matching the idiom the sibling steer tests already use. cancelled_turn_...before_response: the round-2 gate was released right after writing session/cancel, so round 2 could end the turn before the cancel was dequeued (stopReason end_turn, not cancelled). Await the cancel ack first -- the handler awaits cancel_session before replying, so the ack proves cancellation is registered. Verified: 30/30 stress runs green; both tests still fail under mutation (no-op cancel_session; disabled run-id guard), so rigor is preserved.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
… waits recv_until discards non-matching frames. In steer_rejected_on_run_id_mismatch the prompt response could arrive before the steer error, be dropped by recv_until(s_id), and make the following recv_until(p_id) hang to the 10s timeout. In cancelled_turn_with_usage the cancel-ack wait could likewise drop the round-1 usage_update or the prompt response racing the ack. Fix: collect frames with recv_until_with_drain across all wait windows (tool_call_update, cancel ack, prompt response), accumulate them, and run the assertions over the accumulated set. The before-response invariant now holds by construction since every collected frame precedes the prompt response. Also assert the cancel ack is a success response (restores the ack check the PR had dropped) and skip the redundant p_id wait when it was already drained. Evidence: cargo fmt --check, clippy -D warnings, cargo metadata --locked, full cargo test -p buzz-agent (all green), 50/50 stress runs of each corrected test.
…cel races The prior correction kept window-split draining: after the cancel-ack wait it unconditionally waited again for the prompt response, hanging to the recv timeout when the response had already been drained into ack_frames. And the accumulated-frames 'usage before response' claim held only when the prompt response ended the last window — a usage_update collected after an early prompt response falsely satisfied ordering. Replace the per-window drain idiom with FrameLog: a single ordered collector whose wait_until matches already-collected frames before reading more, so a prompt response seen in any earlier window is found, never re-read. Ordering is asserted explicitly: assert_usage_strictly_before_ prompt_response requires a usage_update at an index strictly before the FIRST prompt response — a late usage frame, or one before a later turn's response, is rejected. recv_active_run_id's discarding read is bypassed in the rewritten tests (the advert is read through the log). Deterministic regressions (no stochastic stress required): - cancel_after_prompt_response_uses_existing_frames: prompt-before-ack — cancel lands after the turn ended; ack must still be success and the response must be found in the log, not re-read. - steer_rejection_after_prompt_response_uses_existing_frames: prompt-before-rejection — steer at an ended run is invalid_params and the response is served from the log. - usage_ordering_assertion_rejects_usage_after_first_prompt_response: unit proof that usage-after-first-response, usage-before-a-later- response, and missing-response orderings all fail. Preserved: cancel success ack, stopReason cancelled, end_turn on rejected steer, bounded waiting (Harness::recv timeout), gate semantics. Gates: cargo fmt --all -- --check (0), cargo metadata --locked (0), cargo clippy -p buzz-agent --all-targets -D warnings (0), cargo test -p buzz-agent (701 passed / 0 failed), cargo check --workspace (0), stress 30/30 on each racy test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two
crates/buzz-agent/tests/fake_llm.rstests race the wire and fail intermittently on every open PR — including PRs that touch no Rust at all (#20, #25). They are the single largest source of false CI signal in the backlog.Root causes
steer_rejected_on_run_id_mismatch(:1015run-id mismatch was not rejected)The drain loop
breaks as soon as the prompt responsep_idarrives. Under load the turn can finish before the steer rejection is delivered, so the loop exits withsaw_reject = false. Fixed by keying the drain on the steer id viarecv_until— the idiom the sibling steer tests already use.cancelled_turn_with_usage_emits_notification_before_response(:1454stopReason: Null != "cancelled")gate_tx.send(())fired immediately afterh.send("session/cancel"), which only writes to stdin. Round 2 could complete before the cancel was dequeued, ending the turnend_turn. Fixed by awaiting the cancel ack first: the handler awaitscancel_session(..)before replying, so the ack proves cancellation is registered.Verification
cargo fmt --check,cargo clippy --all-targets -- -D warningsclean--test-threads=4buzz-agentsuite greencancel_session;if falserun-id guard), so determinism did not come at the cost of rigorTest-only change; no product code touched.