feat(simulator): deterministic seed based workload fuzz harness#3272
feat(simulator): deterministic seed based workload fuzz harness#3272krishvishal wants to merge 4 commits into
Conversation
|
Thanks for the pull request. It is now waiting for review, labeled You can update that label as the review goes back and forth, with slash commands - each on its own line, in a regular PR comment (not an inline review reply):
Commands take up to ~90s to apply. If no reaction (👍 or 😕) appears on your comment, the apply step likely failed - check the repo's Actions tab for the See CONTRIBUTING.md for details. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3272 +/- ##
=============================================
- Coverage 73.78% 24.11% -49.68%
Complexity 943 943
=============================================
Files 1200 1229 +29
Lines 109116 96425 -12691
Branches 86007 73333 -12674
=============================================
- Hits 80515 23251 -57264
- Misses 25874 72587 +46713
+ Partials 2727 587 -2140
🚀 New features to boost your workflow:
|
|
LGTM +1 binding |
| // submitted to. A mismatch means the reply landed in the wrong | ||
| // VSR group's bookkeeping; refuse to apply effects against the | ||
| // wrong shadow bucket. | ||
| if entry.request_namespace != header.namespace { |
There was a problem hiding this comment.
ns-mismatch path removes the in_flight entry then returns None; the caller at mod.rs:152-156 skips the in_flight_per_client decrement on that branch. unreachable today (server echoes request namespace verbatim, request ids are monotonic globally per SimClient), but a future routing or dedup bug would manifest as a silent single-client wedge at CLIENT_REQUEST_QUEUE_MAX=1 instead of a clean replies_unknown count. cleanest fix is to return an enum Match(InFlight) | NsMismatch | Unknown so the caller decrements on NsMismatch without applying effects.
| match outcome { | ||
| Outcome::Success => { | ||
| let user = shadow.pick_user_name(prng)?; | ||
| let current_password = format!("pw-{user}"); |
There was a problem hiding this comment.
format!("pw-{user}") is correct only pre-first-commit. once the first ChangePassword succeeds server-side it stores new_password, but the next sample() rebuilds pw-{user} again, so a second ChangePassword on the same user mismatches the server. dormant under all-Success classification + weight=0 default. fix: shadow tracks per-user current pw; seed pw-{user} on AddUser, update on ChangePassword effect and on RenameUser.
| new: String, | ||
| }, | ||
| RenameUser { | ||
| old: String, |
There was a problem hiding this comment.
RenameUser { old, new } carries no pw field; combined with the pw-{user} reconstruction at change_password.rs:52, a ChangePassword issued after RenameUser derives the wrong password. add a pw payload to the RenameUser effect and propagate it through shadow.rs:227-231. bundle with the change_password fix.
| } => { | ||
| if self.stream_names.contains(&stream) { | ||
| self.topic_names.insert((stream, name)); | ||
| } |
There was a problem hiding this comment.
AddTopic silently no-ops when the parent stream is gone; same pattern at AddConsumerGroup (lines 197-205). auditor.note_committed at mod.rs:174 runs unconditionally, so commits_per_action diverges from the shadow under multi-client interleave. same root cause as the multi-client rename collision below. single fix: Shadow::apply returns ApplyResult { applied: bool } and note_committed only fires on applied = true.
|
|
||
| fn rename_stream(&mut self, old: &str, new: &str) { | ||
| if !self.stream_names.shift_remove(old) { | ||
| return; |
There was a problem hiding this comment.
rename_stream's if !shift_remove(old) { return; } silently drops the second concurrent rename when two clients picked the same old. same pattern at rename_topic (lines 271-277). CLIENT_REQUEST_QUEUE_MAX=1 is per-client; the second client's pick_*_name reads the shadow before the first rename's reply lands, so collisions are possible across clients. dormant under default weights but closes via the same ApplyResult change as the AddTopic finding above.
| self.stream_names.insert(new.to_string()); | ||
| // Rename in (stream, topic) and (stream, topic, group): | ||
| // collect-then-rebuild keeps the loop borrow simple. | ||
| let old_topics: Vec<(String, String)> = self |
There was a problem hiding this comment.
rename_stream calls shift_remove inside the topic loop. each shift_remove is O(N), total O(N*M). cold path today (UpdateStream weight=0 default). single rebuild pass collapses it to O(N).
| pub namespaces_live: IndexSet<IggyNamespace>, | ||
|
|
||
| /// Live streams by name. `CreateStream` inserts; `DeleteStream` removes. | ||
| pub stream_names: IndexSet<String>, |
There was a problem hiding this comment.
IndexSet<String> for entity names grows linearly with creates; fresh_name allocates a new String each time. bounded today by tick_budget per run. flag for Arc<str> interning when long-running harness lands.
|
|
||
| self.auditor.note_committed(entry.action); | ||
|
|
||
| if let Some(count) = self.in_flight_per_client.get_mut(&header.client) { |
There was a problem hiding this comment.
count.saturating_sub(1) will mask any future double-decrement once the auditor on_reply consumed-but-rejected hardening lands. switch to checked_sub(1).expect("in_flight underflow") so the invariant fails loud.
| } | ||
|
|
||
| #[must_use] | ||
| pub const fn predicted_effect(_input: &Input, outcome: Outcome) -> Effect { |
There was a problem hiding this comment.
predicted_effect = Effect::None is acceptable today because shadow.sends_committed is keyed by IggyNamespace (packed stream/topic/partition) and the purge request carries only a name-based WireIdentifier. the shadow has no name -> ns reverse index, so it cannot zero the right keys. add a TODO so this surfaces when that reverse index lands and the offset clamp at store_consumer_offset.rs:63-64 starts validating against post-purge state.
| Outcome::Success | ||
| } | ||
|
|
||
| #[must_use] |
There was a problem hiding this comment.
same as purge_stream.rs:62. add a matching TODO.
github-actions
Bot
added
S-waiting-on-author
3 participants
Shadowpredicts server entity state; anAuditormatches replies to in-flight requests and tracks per-(client, namespace) bookkeeping.sample/build_message/classify_reply/predicted_effect). Theop_dispatch!macro generates the dispatch table from the op list so forgetting an op is a compile error.header.namespaceis cross-checked against the in-flightrequest_namespace.