From 5f4d16b9d276f986eef3a84c09f4b1e4c8ecffca Mon Sep 17 00:00:00 2001 From: "Ryan Johnson (ntninja)" Date: Fri, 3 Jul 2026 11:29:55 +0000 Subject: [PATCH] =?UTF-8?q?fix(server):=20batch-2=20backend=20validation?= =?UTF-8?q?=20=E2=80=94=20ScheduleHeat=20guards,=20membership=20pruning,?= =?UTF-8?q?=20DQ=20metrics,=20h2h=20points=20(#335=20#336=20#339=20#341)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Release-review hardening, each fix with tests: - ScheduleHeat validation (#335): reject a duplicate competitor in the lineup; a round tag must name one of the event's rounds; a class tag must be selected by the event and (when a round is also tagged) eligible for it; on a tagged heat, every lineup ref naming a directory pilot must be a member of the tagged round's eligible classes' membership (mirroring the FillRound field / console eligible-members picker). node-{i} timer seats and sim free-text refs still pass, so practice-style heats keep working. - ScheduleHeat duplicate id (#341, VERIFIED by probe): the fold re-seeds a repeated HeatScheduled back to Scheduled, so re-using an id silently reset a finished/Final heat and orphaned its result. Only genuinely-new ids are accepted now (re-runs go through Discard/Restart), on both the event-aware and bare command paths. - Stale membership on shrink (#336): the roster PUT (and the per-pilot DELETE, same hole) now prune classes_membership slots whose pilot left the roster; the classes PUT drops deselected classes' membership entries — so FillRound can no longer seat removed pilots past the #330 membership guard. Surviving members keep their channels; empty entries are dropped. - DQ standings-metric asymmetry (#339): a disqualified placement contributes NO best-lap and NO win-condition metric (lap count / consecutive window) to the standings row — the ranking already excluded it (#331), so the row no longer surfaces values the position ignored. The pilot's clean heats still count; class standings share the fixed best-lap fold. - head_to_head linear points (#341, VERIFIED): the no-table fallback priced a finish by heat.result.places.len(), so a no-show shrank every present pilot's points in that heat. It now prices by the round's group_size (an explicit points table was and is unaffected). Added the missing odd-field tests pinning the [2,2,1] one-pilot trailing heat and the [4,2] short trailing group under both scorings. - heats.contract.ts now builds a real class/roster/membership/round before scheduling a tagged heat (required by the #335 validation) and pins the new rejections over the wire. cargo test --workspace green; cargo fmt + clippy --all-targets -D warnings clean; frontend npm run contract green (9 files, 90 tests) against a rebuilt Director. Co-Authored-By: Claude Fable 5 --- crates/engine/src/head_to_head.rs | 135 +++++++- crates/server/src/control_handler.rs | 499 ++++++++++++++++++++++++++- crates/server/src/events.rs | 146 ++++++++ crates/server/src/round_engine.rs | 88 +++++ frontend/contract/heats.contract.ts | 85 ++++- 5 files changed, 926 insertions(+), 27 deletions(-) diff --git a/crates/engine/src/head_to_head.rs b/crates/engine/src/head_to_head.rs index 29ad18d..9ec129d 100644 --- a/crates/engine/src/head_to_head.rs +++ b/crates/engine/src/head_to_head.rs @@ -176,7 +176,13 @@ impl Generator for HeadToHead { let mut totals: BTreeMap = self.field.iter().map(|c| (c.clone(), 0)).collect(); for heat in completed { - let heat_size = heat.result.places.len(); + // The linear fallback prices a finish against the round's GROUP size, not the + // number of placements the heat happened to produce: a no-show shrinking the + // result must not devalue every present pilot's finish (a win in a 4-up group + // is worth 4 even if only 2 showed). An explicit table is unaffected (it looks + // up by position). A short *trailing* group prices by the same group size, so + // equal finishing positions earn equal points across a round's heats. + let heat_size = self.group_size; for place in &heat.result.places { // A DISQUALIFIED placement earns nothing: the DQ voids the finish that // decided the position, so it must not pay points (mirrors timed_qual's @@ -498,6 +504,133 @@ mod tests { assert_eq!(heat_ids(&g.next(&[])).len(), 6); } + #[test] + fn points_linear_fallback_prices_by_group_size_not_result_size() { + // Two 4-up groups; heat 0's C and D no-show (only two placements land in the result). + // The linear fallback must price by the GROUP size: A's win earns 4 — the same as E's + // win in the full heat — not `places.len() − pos + 1 = 2`. Before the fix the shrunken + // result devalued heat 0 wholesale (E would outrank A off an identical win). + let g = HeadToHead::new( + field(&["A", "B", "C", "D", "E", "F", "G", "H"]), + 4, + 1, + Scoring::Points(None), + ); + let done = vec![ + CompletedHeat::new("h2h-h0", result(&[("A", 1, 5), ("B", 2, 4)])), + CompletedHeat::new( + "h2h-h1", + result(&[("E", 1, 5), ("F", 2, 4), ("G", 3, 3), ("H", 4, 2)]), + ), + ]; + let ranking = g.ranking(&done); + // A and E both earned 4 points and share position 1 (tie → ref order A, E); B and F + // both earned 3 and share position 3; G (2) and H (1) raced and earned theirs; the + // no-shows C and D sit on their seeded 0 points, last. + assert_eq!( + names(&ranking), + vec!["A", "E", "B", "F", "G", "H", "C", "D"] + ); + assert_eq!(ranking[0].position, 1, "A's 2-present win is a full win"); + assert_eq!(ranking[1].position, 1, "E ties A — same finishing position"); + assert_eq!(ranking[2].position, 3); + assert_eq!(ranking[3].position, 3); + } + + #[test] + fn odd_field_lays_out_a_short_trailing_group() { + // 5 pilots, 2-up: chunks() yields [2, 2, 1] — the trailing group is a SINGLE pilot. + // This pins the current layout: the 1-pilot heat IS emitted (a solo time-trial-style + // pass for the odd pilot out), not silently dropped or merged. + let mut g = HeadToHead::new(field(&["A", "B", "C", "D", "E"]), 2, 1, Scoring::Placement); + let step = g.next(&[]); + assert_eq!(heat_ids(&step), vec!["h2h-h0", "h2h-h1", "h2h-h2"]); + let GeneratorStep::Run(heats) = step else { + panic!("expected Run") + }; + assert_eq!(heats[0].lineup, field(&["A", "B"])); + assert_eq!(heats[1].lineup, field(&["C", "D"])); + assert_eq!(heats[2].lineup, field(&["E"]), "the odd pilot flies alone"); + } + + #[test] + fn odd_field_single_pilot_trailing_heat_ranks_under_both_scorings() { + // The [2, 2, 1] layout raced to completion: the solo pilot's unopposed "win" counts + // like any other heat win under BOTH scorings (pinning current behavior, not + // redesigning it — the RD who wants no solo heat picks a different group size). + let done = vec![ + CompletedHeat::new("h2h-h0", result(&[("A", 1, 5), ("B", 2, 4)])), + CompletedHeat::new("h2h-h1", result(&[("C", 1, 6), ("D", 2, 3)])), + CompletedHeat::new("h2h-h2", result(&[("E", 1, 4)])), + ]; + // Placement: E joins the winners band (position 1 in their heat), ordered within the + // band by laps — C (6) > A (5) > E (4) — then the runners-up B (4) > D (3). + let placement = + HeadToHead::new(field(&["A", "B", "C", "D", "E"]), 2, 1, Scoring::Placement); + assert_eq!( + names(&placement.ranking(&done)), + vec!["C", "A", "E", "B", "D"] + ); + // Points (linear, group_size 2): every heat winner earns 2 — including E's solo win — + // so A, C, E tie on 2 points at position 1 (ref order), B and D on 1 point at 4. + let points = HeadToHead::new( + field(&["A", "B", "C", "D", "E"]), + 2, + 1, + Scoring::Points(None), + ); + let ranking = points.ranking(&done); + assert_eq!(names(&ranking), vec!["A", "C", "E", "B", "D"]); + assert_eq!(ranking[0].position, 1); + assert_eq!( + ranking[2].position, 1, + "the solo win pays the same 2 points" + ); + assert_eq!(ranking[3].position, 4); + } + + #[test] + fn odd_field_short_trailing_group_scores_under_both_scorings() { + // 6 pilots, 4-up: chunks() yields [4, 2] — a short (but real) trailing group. + let done = vec![ + CompletedHeat::new( + "h2h-h0", + result(&[("A", 1, 6), ("B", 2, 5), ("C", 3, 4), ("D", 4, 3)]), + ), + CompletedHeat::new("h2h-h1", result(&[("E", 1, 5), ("F", 2, 4)])), + ]; + let laid_out = + |scoring| HeadToHead::new(field(&["A", "B", "C", "D", "E", "F"]), 4, 1, scoring); + let mut g = laid_out(Scoring::Placement); + let step = g.next(&[]); + assert_eq!(heat_ids(&step), vec!["h2h-h0", "h2h-h1"]); + let GeneratorStep::Run(heats) = step else { + panic!("expected Run") + }; + assert_eq!( + heats[1].lineup, + field(&["E", "F"]), + "the trailing group is the leftover 2" + ); + // Placement: winners band A (6 laps) > E (5), then the position-2 band B (5) > F (4), + // then C, D — the short group's finishes band exactly like the full group's. + assert_eq!(names(&g.ranking(&done)), vec!["A", "E", "B", "F", "C", "D"]); + // Points (linear, group_size 4): E's win in the short group earns the full 4 — equal + // finishing positions earn equal points across the round's heats — so A and E tie at + // position 1, B and F (3 points each) at 3, then C (2) and D (1). + let ranking = laid_out(Scoring::Points(None)).ranking(&done); + assert_eq!(names(&ranking), vec!["A", "E", "B", "F", "C", "D"]); + assert_eq!(ranking[0].position, 1); + assert_eq!( + ranking[1].position, 1, + "the short-group win pays the full group_size" + ); + assert_eq!(ranking[2].position, 3); + assert_eq!(ranking[3].position, 3); + assert_eq!(ranking[4].position, 5); + assert_eq!(ranking[5].position, 6); + } + #[test] fn registry_builds_head_to_head_with_points_table() { let mut registry = FormatRegistry::new(); diff --git a/crates/server/src/control_handler.rs b/crates/server/src/control_handler.rs index 23a4c86..bafc76d 100644 --- a/crates/server/src/control_handler.rs +++ b/crates/server/src/control_handler.rs @@ -15,7 +15,7 @@ //! | command group | validation | appended event | //! |---------------|------------|----------------| //! | heat-loop (`Stage`/`Start`/`SkipCountdown`/`ForceEnd`/`Finalize`/`Advance`/`Revert`/`Abort`/`Restart`/`Discard`) | [`heat::heat_state`] folds the heat's current state; [`heat::apply`] checks the transition is legal | [`Event::HeatStateChanged`] with the engine-returned [`HeatTransition`](gridfpv_events::HeatTransition) | -//! | [`Command::ScheduleHeat`] | none (it creates the heat) | [`Event::HeatScheduled`] | +//! | [`Command::ScheduleHeat`] | the id is genuinely **new**, the lineup seats no competitor twice, and a `round`/`class` tag resolves against the event meta (round exists; class selected + round-eligible; pilot refs are eligible members) — #335 | [`Event::HeatScheduled`] | //! | [`Command::SetCurrentHeat`] | the heat exists in the log | [`Event::CurrentHeatSelected`] | //! | [`Command::Register`] | none (the binding is always recordable; last-registration-wins folds downstream) | [`Event::CompetitorRegistered`] | //! | [`Command::VoidDetection`] | the `target` offset exists and is a [`Pass`](gridfpv_events::Pass) | [`Event::DetectionVoided`] | @@ -322,6 +322,20 @@ pub fn apply_command_in_event( /// Handle [`Command::ScheduleHeat`] (race redesign Slice 4a) — create a heat with its lineup, with /// the **channel assignment + heat-size cap** the round-driven path also applies. /// +/// Validated before anything is appended (#335, the #330 hardening pattern — a raw API call must +/// not lay down a heat the UI could never build); every failure is a typed `400`: +/// +/// - the heat id is genuinely **new** ([`require_new_heat_id`] — a duplicate would re-seed an +/// existing, possibly Final, heat back to `Scheduled`); +/// - the lineup seats no competitor twice ([`require_distinct_lineup`]); +/// - a `round` tag names one of the event's rounds; a `class` tag names a class the event +/// selects — and, when both are tagged, one **eligible** for that round; +/// - on a tagged heat, every lineup ref that names a **directory pilot** is a member of the +/// tagged round's eligible classes' membership (the same field FillRound / the console's +/// eligible-members picker resolves) — see [`validate_tagged_lineup`]. Non-pilot refs +/// (`node-{i}` timer seats, sim free-text names) pass through, so practice-style heats keep +/// scheduling. +/// /// The cap (lineup ≤ the event's effective primary timer's node count) is enforced here and an /// oversized lineup is a typed `400` (nothing appended). Channels are assigned from the timer's /// available set unless the caller supplied an explicit `frequencies` set (the caller — a test, a @@ -348,6 +362,15 @@ fn apply_schedule_heat( )); }; + // #335 validation, all-or-nothing before the append: the log-only guards (fresh id, distinct + // lineup) plus the meta-scoped tag/membership checks. + if let Err(err) = require_new_heat_id(state, &heat) + .and_then(|()| require_distinct_lineup(&lineup)) + .and_then(|()| validate_tagged_lineup(registry, &meta, &lineup, &class, &round)) + { + return CommandAck::failed(err); + } + // Caller-supplied frequencies win (manual override / test); otherwise assign from the event's // timer. Either way the heat-size cap is enforced against the event's timer. let frequencies = if frequencies.is_empty() { @@ -391,6 +414,136 @@ fn apply_schedule_heat( } } +/// Require that `heat` names a genuinely **new** heat — one the log never scheduled (#335). +/// +/// The fold ([`heat::heat_state`]) deliberately re-seeds a repeated `HeatScheduled` back to +/// `Scheduled` (robustness on replay), so *accepting* a duplicate id would silently reset an +/// existing — possibly finished/**Final** — heat and orphan its result (#341). A re-run of a raced +/// heat is a `Discard`/`Restart` transition on the existing heat, never a re-schedule; a new heat +/// takes a fresh id (the console mints collision-checked ids for exactly this reason). +fn require_new_heat_id(state: &AppState, heat: &HeatId) -> Result<(), ProtocolError> { + let (events, _cursor) = state.read()?; + if heat::heat_state(&events, heat).is_some() { + return Err(ProtocolError::new( + ErrorCode::BadRequest, + format!( + "a heat with id {:?} already exists; use Discard/Restart to re-run it, or pick a \ + fresh id", + heat.0 + ), + )); + } + Ok(()) +} + +/// Reject a lineup that seats the **same competitor twice** (#335): a lineup ref is the handle +/// passes/channels key on, so a duplicate would merge two seats into one pilot's lap stream. +fn require_distinct_lineup(lineup: &[gridfpv_events::CompetitorRef]) -> Result<(), ProtocolError> { + let mut seen = std::collections::BTreeSet::new(); + for competitor in lineup { + if !seen.insert(competitor.0.as_str()) { + return Err(ProtocolError::new( + ErrorCode::BadRequest, + format!( + "competitor {:?} appears more than once in the lineup", + competitor.0 + ), + )); + } + } + Ok(()) +} + +/// Validate a `ScheduleHeat`'s **round/class tag + lineup** against the event meta (#335). +/// +/// - A `round` tag must name one of the event's rounds; a `class` tag must be one the event +/// selects and — when a round is also tagged — **eligible** for that round (in the round's +/// `classes`). An untagged (free-text / practice) heat skips all of this. +/// - On a tagged heat, every lineup ref that names a **directory pilot** must be an *eligible +/// member*: the union of the tagged round's eligible classes' membership (mirroring how +/// FillRound's `FromRoster` field and the console's eligible-members picker resolve), or the +/// tagged class's own membership when only a class is tagged. Refs that are **not** directory +/// pilots pass through — `node-{i}` timer seats (the open-practice channel lineup) and sim +/// free-text names have no membership to check — so practice-style heats keep scheduling. +fn validate_tagged_lineup( + registry: &EventRegistry, + meta: &crate::events::EventMeta, + lineup: &[gridfpv_events::CompetitorRef], + class: &Option, + round: &Option, +) -> Result<(), ProtocolError> { + // (1) The round tag resolves to this event's round definition. + let round_def = match round { + Some(id) => match meta.rounds.iter().find(|r| &r.id == id) { + Some(def) => Some(def), + None => { + return Err(ProtocolError::new( + ErrorCode::BadRequest, + format!("no round with id {:?} in this event", id.0), + )); + } + }, + None => None, + }; + + // (2) The class tag is selected by the event, and eligible for the tagged round. + if let Some(class_id) = class { + if !meta.classes.contains(class_id) { + return Err(ProtocolError::new( + ErrorCode::BadRequest, + format!("class {:?} is not selected by this event", class_id.0), + )); + } + if let Some(def) = round_def { + if !def.classes.contains(class_id) { + return Err(ProtocolError::new( + ErrorCode::BadRequest, + format!( + "class {:?} is not eligible for round {:?}", + class_id.0, def.id.0 + ), + )); + } + } + } + + // (3) Membership: only for a tagged heat, and only for pilot-shaped refs. + if round_def.is_none() && class.is_none() { + return Ok(()); + } + // The eligible member set — the tagged round's eligible classes (the round wins when both are + // tagged: its `classes` already contain the class per check 2), else the tagged class alone. + let eligible_classes: Vec<&gridfpv_events::ClassId> = match round_def { + Some(def) => def.classes.iter().collect(), + None => class.iter().collect(), + }; + let mut members = std::collections::BTreeSet::new(); + for cls in eligible_classes { + if let Some(membership) = meta.classes_membership.iter().find(|m| &m.class == cls) { + for slot in &membership.pilots { + members.insert(slot.pilot.0.as_str()); + } + } + } + let pilots = registry.pilots(); + for competitor in lineup { + // A ref is "pilot-shaped" when it names a directory pilot — the console's build path + // seats pilot ids verbatim as refs. Anything else (node seats, free-text) passes. + if pilots.exists(&gridfpv_events::PilotId(competitor.0.clone())) + && !members.contains(competitor.0.as_str()) + { + return Err(ProtocolError::new( + ErrorCode::BadRequest, + format!( + "pilot {:?} is not a member of the tagged round/class's eligible classes", + competitor.0 + ), + )); + } + } + Ok(()) +} + /// The defensive cap on `FillMode::All`'s loop (#216): a real deterministic format always /// converges to `Complete` in far fewer than this, so hitting it means the generator never /// reported done — a logic bug, not a request for a 1000th heat. We stop and log rather than @@ -732,9 +885,11 @@ fn command_to_event(state: &AppState, command: Command) -> Result Result Ok(Event::HeatScheduled { - heat, - lineup, - class, - round, - frequencies, - label, - }), + } => { + require_new_heat_id(state, &heat)?; + require_distinct_lineup(&lineup)?; + Ok(Event::HeatScheduled { + heat, + lineup, + class, + round, + frequencies, + label, + }) + } // --- FillRound is intercepted by `apply_command_in_event` (it needs the event // meta, not just the log) and never reaches here on the real control path. The arm @@ -1277,6 +1436,86 @@ mod tests { ))); } + /// A `ScheduleHeat` seating the same competitor twice is rejected with a typed `BadRequest` + /// and appends nothing (#335) — a duplicate ref would merge two seats into one lap stream. + #[test] + fn schedule_heat_rejects_a_duplicate_competitor_in_the_lineup() { + let state = AppState::new(InMemoryLog::default()); + let ack = apply_command( + &state, + Command::ScheduleHeat { + heat: heat(), + lineup: vec![CompetitorRef("A".into()), CompetitorRef("A".into())], + class: None, + round: None, + frequencies: vec![], + label: None, + }, + ); + assert!(!ack.ok, "a duplicate lineup entry must be rejected"); + assert_eq!(ack.error.unwrap().code, ErrorCode::BadRequest); + let (events, _) = state.read().unwrap(); + assert!(events.is_empty(), "a rejected ScheduleHeat appends nothing"); + } + + /// `ScheduleHeat` on an id that already exists is rejected (#335 / #341): the fold re-seeds a + /// repeated `HeatScheduled` back to `Scheduled`, so accepting the duplicate would silently + /// reset a **Final** heat and orphan its result. The heat must stay Final; a re-run goes + /// through `Discard`/`Restart`, never a re-schedule. + #[test] + fn schedule_heat_rejects_an_existing_heat_id() { + use gridfpv_engine::heat::{HeatState, heat_state}; + + // q-1 driven all the way to Final. + let state = drive_current_to(&[ + HeatTransition::Staged, + HeatTransition::Armed, + HeatTransition::Running, + HeatTransition::Finished, + HeatTransition::Finalized, + ]); + let (before, _) = state.read().unwrap(); + assert_eq!(heat_state(&before, &heat()), Some(HeatState::Final)); + + let ack = apply_command( + &state, + Command::ScheduleHeat { + heat: heat(), + lineup: vec![CompetitorRef("A".into())], + class: None, + round: None, + frequencies: vec![], + label: None, + }, + ); + assert!(!ack.ok, "re-scheduling an existing id must be rejected"); + assert_eq!(ack.error.unwrap().code, ErrorCode::BadRequest); + + // Nothing appended; the heat is still Final — NOT re-seeded to Scheduled. + let (after, _) = state.read().unwrap(); + assert_eq!(before.len(), after.len(), "nothing was appended"); + assert_eq!( + heat_state(&after, &heat()), + Some(HeatState::Final), + "the finished heat keeps its state" + ); + + // A merely-Scheduled heat is protected the same way (only genuinely-new ids pass). + let state = scheduled_state(); + let ack = apply_command( + &state, + Command::ScheduleHeat { + heat: heat(), + lineup: vec![CompetitorRef("A".into())], + class: None, + round: None, + frequencies: vec![], + label: None, + }, + ); + assert!(!ack.ok, "a scheduled id is not a fresh id either"); + } + /// `SetCurrentHeat` validates the heat exists, then appends a `CurrentHeatSelected` — and the /// live `current_heat` derivation follows it on replay (event-sourced / deterministic). #[test] @@ -2290,6 +2529,242 @@ mod tests { assert_eq!(before, after, "a rejected FillRound appends nothing"); } + // --- #335: ScheduleHeat tag + membership validation (the event-aware path) --------------- + + /// An 8-node event over a class of `pilots` with a single timed_qual round — the tagged + /// ScheduleHeat fixture. Returns the registry, event id, round id, class id, and the member + /// pilot ids (in membership order). + #[cfg(test)] + fn tagged_schedule_fixture( + pilots: &[&str], + ) -> ( + EventRegistry, + EventId, + gridfpv_events::RoundId, + gridfpv_events::ClassId, + Vec, + ) { + use crate::timers::{ChannelCapability, CreateTimerRequest, TimerKind}; + let timer_req = CreateTimerRequest { + name: "8-node".into(), + kind: TimerKind::Mock { laps: 1, lap_ms: 1 }, + channel_capability: Some(ChannelCapability::Flexible), + node_count: Some(8), + available_channels: Some(crate::channels::RACEBAND_MHZ.to_vec()), + }; + let (registry, event_id, round) = event_with_timer_and_round(timer_req, pilots); + let meta = registry.meta_of(&event_id).unwrap(); + let class = meta.classes[0].clone(); + let members = meta.classes_membership[0] + .pilots + .iter() + .map(|s| s.pilot.clone()) + .collect(); + (registry, event_id, round, class, members) + } + + /// The tagged ScheduleHeat under test, with the fixture's default shape (no explicit + /// frequencies, no label) — each test varies the tag/lineup it cares about. + #[cfg(test)] + fn schedule_tagged( + registry: &EventRegistry, + event_id: &EventId, + state: &AppState, + heat: &str, + lineup: Vec, + class: Option, + round: Option, + ) -> CommandAck { + apply_command_in_event( + registry, + event_id, + state, + Command::ScheduleHeat { + heat: HeatId(heat.into()), + lineup, + class, + round, + frequencies: vec![], + label: None, + }, + ) + } + + /// A `round` tag must name one of the event's rounds (#335) — a dangling tag would create a + /// heat no round view lists and no generator accounts for. + #[test] + fn schedule_heat_rejects_an_unknown_round_tag() { + use gridfpv_events::RoundId; + let (registry, event_id, _round, _class, members) = tagged_schedule_fixture(&["a", "b"]); + let state = registry.resolve(&event_id).unwrap(); + let lineup = vec![CompetitorRef(members[0].0.clone())]; + let ack = schedule_tagged( + ®istry, + &event_id, + &state, + "x-1", + lineup, + None, + Some(RoundId("nope".into())), + ); + assert!(!ack.ok, "a dangling round tag must be rejected"); + assert_eq!(ack.error.unwrap().code, ErrorCode::BadRequest); + let (events, _) = state.read().unwrap(); + assert!(events.is_empty(), "a rejected ScheduleHeat appends nothing"); + } + + /// A `class` tag must be one the event **selects** (#335) — mirroring the membership PUT's + /// class-selection guard (#330). + #[test] + fn schedule_heat_rejects_an_unselected_class_tag() { + use gridfpv_events::ClassId; + let (registry, event_id, _round, _class, members) = tagged_schedule_fixture(&["a", "b"]); + let state = registry.resolve(&event_id).unwrap(); + let lineup = vec![CompetitorRef(members[0].0.clone())]; + let ack = schedule_tagged( + ®istry, + &event_id, + &state, + "x-1", + lineup, + Some(ClassId("nope".into())), + None, + ); + assert!(!ack.ok, "an unselected class tag must be rejected"); + assert_eq!(ack.error.unwrap().code, ErrorCode::BadRequest); + } + + /// With BOTH tags, the class must be **eligible** for the round (in its `classes`) — a + /// selected-but-ineligible class would file the heat under a round that never runs it (#335). + #[test] + fn schedule_heat_rejects_a_class_not_eligible_for_the_round() { + use crate::classes::CreateClassRequest; + let (registry, event_id, round, class, members) = tagged_schedule_fixture(&["a", "b"]); + // A second directory class, selected by the event but NOT eligible for the round. + let spec = registry + .classes() + .create(&CreateClassRequest { + name: "Spec".into(), + source: Default::default(), + reference: None, + description: None, + }) + .unwrap() + .id; + registry + .set_classes(&event_id, vec![class, spec.clone()]) + .unwrap(); + let state = registry.resolve(&event_id).unwrap(); + let lineup = vec![CompetitorRef(members[0].0.clone())]; + let ack = schedule_tagged( + ®istry, + &event_id, + &state, + "x-1", + lineup, + Some(spec), + Some(round), + ); + assert!(!ack.ok, "a round-ineligible class tag must be rejected"); + assert_eq!(ack.error.unwrap().code, ErrorCode::BadRequest); + } + + /// On a tagged heat, a lineup ref naming a **directory pilot** must be an eligible member — + /// a real pilot outside the round's classes' membership must not be seated (#335, closing + /// the manual bypass of FillRound's membership-resolved field). + #[test] + fn schedule_heat_rejects_a_non_member_pilot_on_a_tagged_heat() { + use crate::pilots::CreatePilotRequest; + let (registry, event_id, round, class, members) = tagged_schedule_fixture(&["a", "b"]); + // A directory pilot who is NOT in the round's class membership. + let outsider = registry + .pilots() + .create(&CreatePilotRequest { + callsign: "outsider".into(), + ..Default::default() + }) + .unwrap() + .id; + let state = registry.resolve(&event_id).unwrap(); + let lineup = vec![ + CompetitorRef(members[0].0.clone()), + CompetitorRef(outsider.0.clone()), + ]; + let ack = schedule_tagged( + ®istry, + &event_id, + &state, + "x-1", + lineup, + Some(class), + Some(round), + ); + assert!(!ack.ok, "a non-member directory pilot must be rejected"); + assert_eq!(ack.error.unwrap().code, ErrorCode::BadRequest); + let (events, _) = state.read().unwrap(); + assert!(events.is_empty(), "a rejected ScheduleHeat appends nothing"); + } + + /// The happy paths stay open (#335): eligible members schedule tagged; **non-pilot refs** + /// (`node-{i}` timer seats — the open-practice channel lineup — and sim free-text names) + /// pass the membership check; and an **untagged** heat validates none of it. + #[test] + fn schedule_heat_accepts_members_node_seats_and_untagged_lineups() { + use crate::pilots::CreatePilotRequest; + let (registry, event_id, round, class, members) = tagged_schedule_fixture(&["a", "b"]); + let state = registry.resolve(&event_id).unwrap(); + + // Eligible members, tagged with their round + class — the console's build path. + let lineup: Vec = + members.iter().map(|p| CompetitorRef(p.0.clone())).collect(); + let ack = schedule_tagged( + ®istry, + &event_id, + &state, + "x-1", + lineup, + Some(class), + Some(round.clone()), + ); + assert!(ack.ok, "eligible members schedule tagged: {ack:?}"); + + // A node-seat ref on a tagged heat is NOT membership-checked (the practice-style path). + let ack = schedule_tagged( + ®istry, + &event_id, + &state, + "x-2", + vec![CompetitorRef("node-0".into())], + None, + Some(round), + ); + assert!(ack.ok, "node-seat refs pass the membership check: {ack:?}"); + + // An untagged heat skips tag validation entirely — even for a known non-member pilot + // (the free-text / ad-hoc path stays as permissive as before). + let outsider = registry + .pilots() + .create(&CreatePilotRequest { + callsign: "outsider".into(), + ..Default::default() + }) + .unwrap() + .id; + let ack = schedule_tagged( + ®istry, + &event_id, + &state, + "x-3", + vec![CompetitorRef(outsider.0.clone())], + None, + None, + ); + assert!( + ack.ok, + "an untagged heat is not membership-checked: {ack:?}" + ); + } + /// Build an event with an 8-node Raceband timer over a class of `pilots`, plus a single /// **head_to_head** round (`group_size=heat_size`) — a *deterministic* format whose one /// generator step emits the whole round's heats (the field split into groups). Returns the diff --git a/crates/server/src/events.rs b/crates/server/src/events.rs index ced2fc2..ecc5bdb 100644 --- a/crates/server/src/events.rs +++ b/crates/server/src/events.rs @@ -1290,6 +1290,11 @@ impl EventRegistry { .get_mut(id) .ok_or_else(|| RegistryError::not_found(format!("no event with id {:?}", id.0)))?; event.meta.roster = dedup_preserving_order(pilot_ids); + // Membership is the finer roster × classes join, so a pilot dropped from the roster must + // not linger in any class's membership (#336) — a stale slot would still be seated by + // FillRound/ScheduleHeat, bypassing the roster/membership validation the membership PUT + // enforces. Surviving members keep their slots (and their assigned channels) untouched. + prune_membership_to_roster(&mut event.meta); let meta = event.meta.clone(); let data_dir = reg.data_dir.clone(); persist_meta_change(data_dir.as_deref(), &meta)?; @@ -1310,6 +1315,15 @@ impl EventRegistry { .get_mut(id) .ok_or_else(|| RegistryError::not_found(format!("no event with id {:?}", id.0)))?; event.meta.classes = dedup_preserving_order(ids); + // A deselected class's membership goes with it (#336): membership only means anything for + // a class the event runs, and a stale entry would still field pilots if the class were + // reselected later under different assumptions (or be resolved by a round that still + // names it). Memberships of the surviving selection are untouched. + let selected = event.meta.classes.clone(); + event + .meta + .classes_membership + .retain(|m| selected.contains(&m.class)); let meta = event.meta.clone(); let data_dir = reg.data_dir.clone(); persist_meta_change(data_dir.as_deref(), &meta)?; @@ -1575,6 +1589,9 @@ impl EventRegistry { .get_mut(id) .ok_or_else(|| RegistryError::not_found(format!("no event with id {:?}", id.0)))?; event.meta.roster.retain(|p| p != pilot); + // Same staleness hole as the roster PUT (#336): the removed pilot's membership slots go + // with them, so no class can still seat a pilot who left the event. + prune_membership_to_roster(&mut event.meta); let meta = event.meta.clone(); let data_dir = reg.data_dir.clone(); persist_meta_change(data_dir.as_deref(), &meta)?; @@ -2209,6 +2226,22 @@ fn dedup_preserving_order(items: Vec) -> Vec { out } +/// Drop every [`classes_membership`](EventMeta::classes_membership) slot whose pilot is **not on +/// the roster** (#336) — the shared prune the roster-shrinking mutations (the roster PUT and the +/// per-pilot DELETE) apply so membership never outlives the roster it joins. A membership entry +/// left with no pilots is removed entirely (no empty entries are persisted — the same invariant +/// [`EventRegistry::set_class_membership`] keeps). Surviving slots are untouched, so a remaining +/// member keeps their assigned channel. +fn prune_membership_to_roster(meta: &mut EventMeta) { + let roster = &meta.roster; + for membership in &mut meta.classes_membership { + membership + .pilots + .retain(|slot| roster.contains(&slot.pilot)); + } + meta.classes_membership.retain(|m| !m.pilots.is_empty()); +} + /// The default per-event timer selection (issue #73): just the built-in **Mock** /// ([`MOCK_TIMER_ID`]). New events and Practice select it so they run a sim race out of the box. fn default_timer_selection() -> Vec { @@ -3403,6 +3436,119 @@ mod tests { assert_eq!(meta.roster, vec![p]); } + /// Seed a directory pilot by callsign — the membership-prune tests' shorthand. + fn seed_pilot(reg: &EventRegistry, callsign: &str) -> PilotId { + reg.pilots() + .create(&crate::pilots::CreatePilotRequest { + callsign: callsign.to_string(), + ..Default::default() + }) + .unwrap() + .id + } + + #[test] + fn roster_shrink_prunes_the_departed_pilots_membership() { + // #336: a pilot dropped from the roster must not linger in classes_membership — + // a stale slot would still be seated by FillRound, bypassing the membership PUT's + // roster guard. Surviving members keep their slots AND their assigned channels. + let reg = EventRegistry::new(None).unwrap(); + let event = reg.create(&req("Shrink Event")).unwrap(); + let open = seed_class(®, "Open"); + let (a, b) = (seed_pilot(®, "alpha"), seed_pilot(®, "bravo")); + reg.set_classes(&event.id, vec![open.clone()]).unwrap(); + reg.set_roster(&event.id, vec![a.clone(), b.clone()]) + .unwrap(); + reg.set_class_membership( + &event.id, + open.clone(), + vec![ + MemberSlot { + pilot: a.clone(), + channel: Some(5658), + }, + MemberSlot { + pilot: b.clone(), + channel: Some(5917), + }, + ], + ) + .unwrap(); + + // Shrink the roster to just A: B's membership slot goes with them. + let meta = reg.set_roster(&event.id, vec![a.clone()]).unwrap(); + assert_eq!(meta.classes_membership.len(), 1); + let membership = &meta.classes_membership[0]; + assert_eq!(membership.class, open); + assert_eq!(membership.pilots.len(), 1, "B's slot is pruned"); + assert_eq!(membership.pilots[0].pilot, a); + assert_eq!( + membership.pilots[0].channel, + Some(5658), + "the surviving member keeps their channel" + ); + + // Emptying the roster removes the now-empty membership entry entirely (no empty + // entries are persisted — the set_class_membership invariant). + let meta = reg.set_roster(&event.id, vec![]).unwrap(); + assert!(meta.classes_membership.is_empty()); + } + + #[test] + fn removing_a_roster_pilot_prunes_their_membership() { + // The per-pilot DELETE has the same staleness hole as the roster PUT (#336). + let reg = EventRegistry::new(None).unwrap(); + let event = reg.create(&req("Remove Event")).unwrap(); + let open = seed_class(®, "Open"); + let (a, b) = (seed_pilot(®, "alpha"), seed_pilot(®, "bravo")); + reg.set_classes(&event.id, vec![open.clone()]).unwrap(); + reg.set_roster(&event.id, vec![a.clone(), b.clone()]) + .unwrap(); + reg.set_class_membership(&event.id, open, slots(vec![a.clone(), b.clone()])) + .unwrap(); + + let meta = reg.remove_from_roster(&event.id, &b).unwrap(); + assert_eq!(meta.roster, vec![a.clone()]); + assert_eq!(meta.classes_membership.len(), 1); + assert_eq!(meta.classes_membership[0].pilots.len(), 1); + assert_eq!(meta.classes_membership[0].pilots[0].pilot, a); + } + + #[test] + fn class_deselect_prunes_its_membership() { + // #336: deselecting a class drops its membership entry — a stale entry would still + // field pilots through any round that names the class. The surviving class's + // membership (channels included) is untouched. + let reg = EventRegistry::new(None).unwrap(); + let event = reg.create(&req("Deselect Event")).unwrap(); + let open = seed_class(®, "Open"); + let spec = seed_class(®, "Spec"); + let (a, b) = (seed_pilot(®, "alpha"), seed_pilot(®, "bravo")); + reg.set_classes(&event.id, vec![open.clone(), spec.clone()]) + .unwrap(); + reg.set_roster(&event.id, vec![a.clone(), b.clone()]) + .unwrap(); + reg.set_class_membership( + &event.id, + open.clone(), + vec![MemberSlot { + pilot: a.clone(), + channel: Some(5658), + }], + ) + .unwrap(); + reg.set_class_membership(&event.id, spec.clone(), slots(vec![b.clone()])) + .unwrap(); + + // Deselect Spec: its membership entry goes; Open's survives channel-intact. + let meta = reg.set_classes(&event.id, vec![open.clone()]).unwrap(); + assert_eq!(meta.classes, vec![open.clone()]); + assert_eq!(meta.classes_membership.len(), 1); + assert_eq!(meta.classes_membership[0].class, open); + assert_eq!(meta.classes_membership[0].pilots[0].pilot, a); + assert_eq!(meta.classes_membership[0].pilots[0].channel, Some(5658)); + } + #[test] fn scored_round_requires_an_end_condition() { let reg = EventRegistry::new(None).unwrap(); diff --git a/crates/server/src/round_engine.rs b/crates/server/src/round_engine.rs index fa56e68..2c7699d 100644 --- a/crates/server/src/round_engine.rs +++ b/crates/server/src/round_engine.rs @@ -750,6 +750,13 @@ fn round_best_laps(round: &RoundDef, events: &[Event]) -> BTreeMap = BTreeMap::new(); for heat in completed_heats(round, events) { for place in &heat.result.places { + // A DISQUALIFIED placement contributes nothing: the DQ voids the heat's laps for + // that pilot, so a lap flown in it must not stand as their best (#339 — the ranking + // already excludes the DQ'd placement, #331; the displayed metrics must match). + // The pilot's other, clean heats still count. + if place.disqualified { + continue; + } if let Some(lap) = place.best_lap_micros { best.entry(place.competitor.competitor.clone()) .and_modify(|existing| *existing = (*existing).min(lap)) @@ -895,6 +902,13 @@ pub fn round_standings( let mut best_consec: BTreeMap = BTreeMap::new(); for heat in &completed { for place in &heat.result.places { + // A DISQUALIFIED placement contributes NO metric to the standings row (#339): the + // ranking already excludes it (#331), so the row must not keep surfacing the DQ'd + // heat's lap count / consecutive window next to a position that ignored them. + // The pilot's other, clean heats still aggregate. + if place.disqualified { + continue; + } let competitor = place.competitor.competitor.clone(); most_laps .entry(competitor.clone()) @@ -2459,6 +2473,80 @@ mod tests { assert_eq!(last.competitor.0, "A"); } + #[test] + fn dq_heat_contributes_no_metric_or_best_lap_to_the_standings_row() { + // The #339 asymmetry closed: the ranking excludes a DQ'd placement (#331), so the + // standings row must not keep surfacing that heat's metric/best-lap next to the + // position that ignored them. A, DQ'd in their ONLY heat, ranks last AND their row + // carries no value — exactly like a no-show. B's clean row is untouched. + let round = qual_round("q1", "open"); // BestLap + let meta = meta_with(vec![round.clone()], vec![member("open", &["A", "B"])]); + let mut log = scored_heat( + "q-1", + "q1", + "open", + &[("A", &[0, 1_000_000]), ("B", &[0, 2_000_000])], + ); + log.push(penalty_applied( + "q-1", + "A", + Penalty::Disqualify { reason: None }, + )); + + let standings = round_standings(&meta, &round, &log).unwrap(); + let a = standing(&standings, "A"); + assert_eq!(a.position, 2, "the DQ sinks A below B"); + assert_eq!( + a.best_lap_micros, None, + "the DQ'd heat's lap is no best lap" + ); + assert_eq!(a.laps, 0, "the DQ'd heat's laps do not count"); + assert_eq!(a.metric, RoundMetric::BestLap { micros: None }); + let b = standing(&standings, "B"); + assert_eq!(b.position, 1); + assert_eq!(b.best_lap_micros, Some(2_000_000)); + } + + #[test] + fn dq_in_one_heat_keeps_the_pilots_clean_heats_in_the_standings() { + // Only the DQ'd heat is voided for the pilot: A's clean second heat still feeds the + // row, so their best lap is the CLEAN heat's 3.0s — not the DQ'd heat's faster 1.0s. + let round = qual_round("q1", "open"); // BestLap + let meta = meta_with(vec![round.clone()], vec![member("open", &["A", "B"])]); + let mut log = scored_heat( + "q-1", + "q1", + "open", + &[("A", &[0, 1_000_000]), ("B", &[0, 2_000_000])], + ); + log.extend(scored_heat( + "q-2", + "q1", + "open", + &[("A", &[0, 3_000_000]), ("B", &[0, 2_500_000])], + )); + log.push(penalty_applied( + "q-1", + "A", + Penalty::Disqualify { reason: None }, + )); + + let standings = round_standings(&meta, &round, &log).unwrap(); + let a = standing(&standings, "A"); + assert_eq!( + a.best_lap_micros, + Some(3_000_000), + "the clean heat counts; the DQ'd (faster) lap does not" + ); + assert_eq!(a.laps, 1, "only the clean heat's lap is counted"); + assert_eq!( + a.metric, + RoundMetric::BestLap { + micros: Some(3_000_000) + } + ); + } + #[test] fn ruling_reversed_restores_the_original_ranking() { // RulingReversed un-applies a DQ at its true global LogRef (offsets PRESERVED, not diff --git a/frontend/contract/heats.contract.ts b/frontend/contract/heats.contract.ts index 1321730..176f949 100644 --- a/frontend/contract/heats.contract.ts +++ b/frontend/contract/heats.contract.ts @@ -2,23 +2,32 @@ * Heats listing contract (race redesign Slice 3b): `GET /events/{id}/heats` + the real * `listHeats` client helper. * - * Schedules a **round/class-tagged** heat over the real control path (`Command::ScheduleHeat`), - * then reads it back through the real `@gridfpv/protocol-client`'s `listHeats`. Asserts the served - * `HeatSummary` round-trips the tag (round + class), the lineup, and a derived status — the exact - * shape the Heats UI groups by round. If the new endpoint, the binding, or the tag plumbing were - * wrong, the heat would not come back tagged. + * Sets up a real class + roster + membership + round on the Practice event (ScheduleHeat now + * validates its round/class tag + membership against the event meta, #335), schedules a + * **round/class-tagged** heat over the real control path (`Command::ScheduleHeat`), then reads it + * back through the real `@gridfpv/protocol-client`'s `listHeats`. Asserts the served `HeatSummary` + * round-trips the tag (round + class), the lineup, and a derived status — the exact shape the + * Heats UI groups by round. If the new endpoint, the binding, or the tag plumbing were wrong, the + * heat would not come back tagged. Also pins the #335 rejections: a dangling round tag and a + * non-member pilot on a tagged heat are failed acks (nothing scheduled). */ import { afterAll, beforeAll, describe, expect, it } from 'vitest'; -import { listHeats, PRACTICE_EVENT_ID } from '../packages/protocol-client/dist/index.js'; +import { + createClass, + createPilot, + createRound, + listHeats, + PRACTICE_EVENT_ID, + setClassMembership, + setEventClasses, + setEventRoster +} from '../packages/protocol-client/dist/index.js'; import { type Director } from '../test-harness/director.ts'; import { rdControl, startContractDirector } from './harness.ts'; const TOKEN = 'rd-heats-contract'; const HEAT = 'q-1'; -const LINEUP = ['A', 'B']; -const ROUND = 'r1'; -const CLASS = 'open'; const LABEL = 'Featured Heat'; let director: Director; @@ -33,9 +42,40 @@ afterAll(async () => { describe('GET /heats serves the round-tagged scheduled heats', () => { it('listHeats returns a tagged HeatSummary with lineup and a derived status', async () => { - // Schedule a heat tagged with a round + class + a custom label over the real control path. + // Real directory setup: a class + two pilots, selected + rostered + membered on Practice — + // a tagged ScheduleHeat is validated against exactly this meta (#335). + const klass = await createClass(director.baseUrl, { name: 'Open' }, TOKEN); + const pilotA = await createPilot(director.baseUrl, { callsign: 'alpha' }, TOKEN); + const pilotB = await createPilot(director.baseUrl, { callsign: 'bravo' }, TOKEN); + await setEventClasses(director.baseUrl, PRACTICE_EVENT_ID, [klass.id], TOKEN); + await setEventRoster(director.baseUrl, PRACTICE_EVENT_ID, [pilotA.id, pilotB.id], TOKEN); + await setClassMembership( + director.baseUrl, + PRACTICE_EVENT_ID, + klass.id, + [pilotA.id, pilotB.id], + TOKEN + ); + const round = await createRound( + director.baseUrl, + PRACTICE_EVENT_ID, + { + label: 'Qualifying', + classes: [klass.id], + format: 'timed_qual', + params: { rounds: '1' }, + win_condition: 'BestLap', + // Best Lap only ranks, so a scored round needs a race time to end (server validation). + time_limit_secs: 60, + seeding: 'FromRoster' + }, + TOKEN + ); + const lineup = [pilotA.id, pilotB.id]; + + // Schedule a heat tagged with the round + class + a custom label over the real control path. const ack = await rdControl(director.baseUrl, TOKEN, { - ScheduleHeat: { heat: HEAT, lineup: LINEUP, class: CLASS, round: ROUND, label: LABEL } + ScheduleHeat: { heat: HEAT, lineup, class: klass.id, round: round.id, label: LABEL } }); expect(ack.ok).toBe(true); @@ -43,13 +83,30 @@ describe('GET /heats serves the round-tagged scheduled heats', () => { const heats = await listHeats(director.baseUrl, PRACTICE_EVENT_ID); const summary = heats.find((h) => h.heat === HEAT); expect(summary).toBeDefined(); - expect(summary!.lineup).toEqual(LINEUP); - expect(summary!.round).toBe(ROUND); - expect(summary!.class).toBe(CLASS); + expect(summary!.lineup).toEqual(lineup); + expect(summary!.round).toBe(round.id); + expect(summary!.class).toBe(klass.id); // The custom build-heat label round-trips on the wire (overrides the derived name in the UI). expect(summary!.label).toBe(LABEL); // Freshly scheduled and on the timer: Scheduled phase, marked current. expect(summary!.phase).toBe('Scheduled'); expect(summary!.is_current).toBe(true); + + // #335 rejections over the wire: a dangling round tag is a failed ack … + const dangling = await rdControl(director.baseUrl, TOKEN, { + ScheduleHeat: { heat: 'q-bad-round', lineup, round: 'no-such-round' } + }); + expect(dangling.ok).toBe(false); + + // … and a directory pilot who is NOT a member of the round's classes is rejected too. + const outsider = await createPilot(director.baseUrl, { callsign: 'outsider' }, TOKEN); + const nonMember = await rdControl(director.baseUrl, TOKEN, { + ScheduleHeat: { heat: 'q-bad-member', lineup: [outsider.id], round: round.id } + }); + expect(nonMember.ok).toBe(false); + + // Neither rejected command scheduled anything. + const after = await listHeats(director.baseUrl, PRACTICE_EVENT_ID); + expect(after.some((h) => h.heat === 'q-bad-round' || h.heat === 'q-bad-member')).toBe(false); }); });