From fcdf14c0dc8cab514b953046cf8a47b994b34d53 Mon Sep 17 00:00:00 2001 From: grom Date: Sun, 25 Oct 2026 00:30:46 -0600 Subject: [PATCH] app: a held press cannot time out while something still guards it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The queue stamped each entry's deadline when the press was made. That reads right and has a hole in it: a cast pushed back twice (`SMSG_SPELL_DELAYED`) ends after the window a press made at its start was given, so the entry times out while the thing it is waiting for is still running — and the press vanishes with no packet, no error line and no cast. That is the one failure mode a player cannot describe, and it is exactly the class of bug the press queue was supposed to remove rather than introduce. The same hole swallows a press held through a cast whose global cooldown then outlasts the window, which is the ordinary case rather than an edge one: a 500 ms window, a cast that ends, and 1.5 s of global cooldown behind it. So a slot now keeps the window and the base it is measured from, and the drain ratchets that base forward — only ever forward — past every live guard: the cast's reopening, the channel's trimmed end, and the global cooldown's. An entry gives up when nothing is holding it back any more and the window has run out since, which is what "how long past the guard's end" meant all along. Scripts keep a fixed deadline. Their contract is "after queued spells", and a script whose spells never fired has nothing left to run after. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/spell/queue.rs | 108 +++++++++++++++++++++---- crates/benilla-app/src/ui_spellbook.rs | 34 +++++--- docs/ADDON_COMPAT.md | 8 ++ 3 files changed, 121 insertions(+), 29 deletions(-) diff --git a/crates/benilla-app/src/spell/queue.rs b/crates/benilla-app/src/spell/queue.rs index 5003ed79..5db0337e 100644 --- a/crates/benilla-app/src/spell/queue.rs +++ b/crates/benilla-app/src/spell/queue.rs @@ -81,14 +81,32 @@ impl Kind { #[derive(Clone, Copy, Debug, PartialEq, Eq)] struct Slot { spell_id: u32, - /// When this entry gives up. See [`SpellQueue::queue`] for where it comes from. - expires: Instant, - /// The earliest [`SpellQueue::pop`] will take it. `now` for an ordinary entry, which is to - /// say no constraint at all; later only for a retry (`NP_RetryServerRejectedSpells`), where - /// the point is to not re-send inside the same millisecond the server just refused. + /// Where the window is measured from: the latest guard end this entry has been told about. + /// + /// **It ratchets forward**, in [`SpellQueue::expire`], while something is still guarding. + /// A fixed `expires` stamped at queue time looks right and has a hole in it: a cast pushed + /// back twice (`SMSG_SPELL_DELAYED`) ends after the window a press made at its start was + /// given, so the entry would time out while the thing it is waiting for is still running and + /// the press would vanish with no packet and no error. The same hole swallows a press held + /// through a cast whose global cooldown then outlasts the window. + base: Instant, + /// How long past [`Self::base`] this entry survives: one of nampower's four windows, picked + /// by what guarded the press (`np_press.rs`'s `window_for`). + window: Duration, + /// The earliest [`SpellQueue::pop`] will take it. The moment it was queued for an ordinary + /// entry, which is to say no constraint at all; later only for a retry + /// (`NP_RetryServerRejectedSpells`), where the point is to not re-send inside the same + /// millisecond the server just refused. not_before: Instant, } +impl Slot { + /// Whether the window has run out, `base` having already been brought up to date. + fn expired(&self, now: Instant) -> bool { + now >= self.base + self.window + } +} + /// One queued script, with the priority that orders it. #[derive(Clone, Debug, PartialEq, Eq)] struct Script { @@ -172,10 +190,10 @@ impl SpellQueue { in_flight_until: Option, window_ms: u32, ) { - let until = in_flight_until.unwrap_or(now).max(now); let slot = Slot { spell_id, - expires: until + Duration::from_millis(u64::from(window_ms)), + base: in_flight_until.unwrap_or(now).max(now), + window: Duration::from_millis(u64::from(window_ms)), not_before: now, }; *self.slot_mut(kind) = Some(slot); @@ -208,7 +226,8 @@ impl SpellQueue { let not_before = now + Duration::from_millis(u64::from(delay_ms)); *self.slot_mut(kind) = Some(Slot { spell_id, - expires: not_before + Duration::from_millis(u64::from(window_ms)), + base: not_before, + window: Duration::from_millis(u64::from(window_ms)), not_before, }); self.events.push((kind.queued_code(), spell_id)); @@ -273,9 +292,18 @@ impl SpellQueue { /// **No popped event for an expiry**, and that is the point of the distinction: a consumer /// reads `*_POPPED` as "this spell was sent". An entry that timed out was not. Upstream has /// no code for it either. - pub(crate) fn expire(&mut self, now: Instant) { + pub(crate) fn expire(&mut self, now: Instant, guard_until: Option) { for kind in [Kind::OnSwing, Kind::NonGcd, Kind::Normal] { - if self.slot_mut(kind).is_some_and(|s| now >= s.expires) { + let Some(slot) = self.slot_mut(kind) else { + continue; + }; + // Ratchet the base forward while something still guards the press: the window is + // "this long past the guard's end", and the guard's end can move — pushback stretches + // a cast, and a global cooldown outlives the cast that armed it. See [`Slot::base`]. + if let Some(guard) = guard_until { + slot.base = slot.base.max(guard); + } + if slot.expired(now) { let dropped = self.slot_mut(kind).take(); if let Some(s) = dropped { debug!( @@ -285,6 +313,8 @@ impl SpellQueue { } } } + // The scripts keep a fixed deadline: their contract is "after queued spells", and a + // script whose spells never fired has nothing left to run after. self.scripts.retain(|s| now < s.expires); } @@ -440,9 +470,9 @@ mod tests { ); // An expiry raises nothing: `*_POPPED` means "this was sent", and this was not. - q.expire(t0 + Duration::from_millis(499)); + q.expire(t0 + Duration::from_millis(499), None); assert_eq!(q.peek(Kind::Normal), Some(133), "inside the window"); - q.expire(t0 + Duration::from_millis(500)); + q.expire(t0 + Duration::from_millis(500), None); assert!(q.is_empty(), "every slot gave up at the window"); assert!( q.take_events().is_empty(), @@ -462,24 +492,68 @@ mod tests { let cast_ends = t0 + Duration::from_secs(3); q.queue(133, Kind::Normal, t0, Some(cast_ends)); - q.expire(t0 + Duration::from_millis(600)); + q.expire(t0 + Duration::from_millis(600), None); assert_eq!( q.peek(Kind::Normal), Some(133), "600 ms in, the cast is still running — the window has not started" ); - q.expire(cast_ends + Duration::from_millis(499)); + q.expire(cast_ends + Duration::from_millis(499), None); assert_eq!(q.peek(Kind::Normal), Some(133)); - q.expire(cast_ends + Duration::from_millis(500)); + q.expire(cast_ends + Duration::from_millis(500), None); assert_eq!(q.peek(Kind::Normal), None); // A deadline already in the past is not a shorter window than a fresh queue's: the `max` // is what stops a lapsed cast making the entry expire instantly. q.queue(133, Kind::Normal, t0, Some(t0 - Duration::from_secs(10))); - q.expire(t0 + Duration::from_millis(499)); + q.expire(t0 + Duration::from_millis(499), None); assert_eq!(q.peek(Kind::Normal), Some(133)); } + /// The window is measured from the **live** guard end, not the one stamped at queue time. + /// + /// A fixed deadline has a hole in it: a cast pushed back twice ends after the window a press + /// made at its start was given, so the entry would time out while the thing it is waiting for + /// is still running, and the press would vanish with no packet and no error line. The same + /// hole swallows a press held through a cast whose global cooldown then outlasts the window. + #[test] + fn a_held_entry_cannot_time_out_while_something_still_guards_it() { + let t0 = Instant::now(); + let mut q = SpellQueue { + window_ms: 500, + ..Default::default() + }; + let cast_ends = t0 + Duration::from_secs(3); + q.queue(133, Kind::Normal, t0, Some(cast_ends)); + + // Two pushbacks of 500 ms each: the cast now ends at t0 + 4 s, well past the entry's + // original t0 + 3.5 s deadline. + let pushed = cast_ends + Duration::from_secs(1); + q.expire(t0 + Duration::from_millis(3_600), Some(pushed)); + assert_eq!( + q.peek(Kind::Normal), + Some(133), + "past where the window would have ended, but the cast is still running" + ); + + // And once it really ends, the window runs from there. + q.expire(pushed + Duration::from_millis(499), None); + assert_eq!(q.peek(Kind::Normal), Some(133)); + q.expire(pushed + Duration::from_millis(500), None); + assert_eq!(q.peek(Kind::Normal), None, "500 ms past the real end"); + + // The base only ever moves forward: a guard that reads *earlier* than the one already + // recorded cannot shorten an entry's life. + q.queue(133, Kind::Normal, t0, Some(cast_ends)); + q.expire(t0, Some(t0)); + q.expire(cast_ends + Duration::from_millis(499), None); + assert_eq!( + q.peek(Kind::Normal), + Some(133), + "a shorter guard end does not pull the deadline in" + ); + } + /// `NP_RetryServerRejectedSpells`: a refused cast goes back in its slot, held for the delay, /// and **once** — a retry that could itself be retried is a packet a second forever. #[test] @@ -545,7 +619,7 @@ mod tests { // And they expire on the same window as a spell does. q.queue_script("gone".into(), 1, t0); - q.expire(t0 + Duration::from_millis(500)); + q.expire(t0 + Duration::from_millis(500), None); assert!(q.take_ready_scripts().is_empty()); } } diff --git a/crates/benilla-app/src/ui_spellbook.rs b/crates/benilla-app/src/ui_spellbook.rs index 8521be07..95bb9a0f 100644 --- a/crates/benilla-app/src/ui_spellbook.rs +++ b/crates/benilla-app/src/ui_spellbook.rs @@ -593,8 +593,6 @@ fn drain_spell_queue( } } - ladder.queue.expire(now); - // The pop. `in_flight` is the ladder's own guard test, so a queued spell waits exactly as // long as a press would have been refused for. // @@ -603,20 +601,32 @@ fn drain_spell_queue( // channel by a macro that asked for it, where a held *press* made mid-channel is the ordinary // case, and sending it before the channel ends would cancel the channel server-side — the // player's own press interrupting their own cast, which is not what holding it meant. + // // The channel's trimmed end, not `current(now)`: that one carries two seconds of slack for a // lost `MSG_CHANNEL_UPDATE(0)`, so popping on it would fire a held press a round trip — and // then some — after the channel really finished. `NP_ChannelLatencyReductionPercentage` is // read here for the same reason the admission reads it, and from the same place. - let channel_guards = ladder - .np - .channel_guard_until( - &ladder.channel, - crate::net::measured_latency_ms(&ladder.ping), - now, - ) - .is_some(); - let guarded = ladder.pending.in_flight(now) || channel_guards; - let gcd_running = ladder.cooldowns.gcd(now).is_some(); + let channel_until = ladder.np.channel_guard_until( + &ladder.channel, + crate::net::measured_latency_ms(&ladder.ping), + now, + ); + let gcd_until = ladder.cooldowns.gcd(now).map(|(start, dur)| start + dur); + let guarded = ladder.pending.in_flight(now) || channel_until.is_some(); + let gcd_running = gcd_until.is_some(); + + // The expiry, with the guard end the entries' windows are measured from. Every guard, not + // just the cast: a press held through a cast whose global cooldown then outlasts the window + // must not be dropped a moment before the cooldown lets it fire. + let guard_until = [ + ladder.pending.guard_ends().filter(|_| guarded), + channel_until, + gcd_until, + ] + .into_iter() + .flatten() + .max(); + ladder.queue.expire(now, guard_until); // Decided first and sent after, because a send borrows the whole ladder where a slot read // borrows one field of it. let mut popped: Vec = Vec::new(); diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index 1f2325a4..0cbd757a 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -1040,6 +1040,14 @@ a narrower set is the safe direction to be wrong in. | `NP_OnSwingBufferCooldownMs` | stored | A rate limit on re-queueing an on-swing strike. The slot is live; what is missing is anything to rate-limit, since a re-press of the queued strike already takes the reference's own silent bail (`6e4d43`) and so cannot queue twice | | `NP_ChannelLatencyReductionPercentage` | **LIVE** | How much of the measured round trip comes off a channel's named end before the queue stops treating it as a guard, so a press held through a channel is not sent a round trip late. The arithmetic is the cast buffer's again, and worth writing down because "send early" sounds reckless: the server starts the channel at `T` and ends it at `T + duration`, we learn of it half a trip later, so our own end is already half a trip *after* the server is free — trimming 75 % of `L` puts a packet at the server a quarter of a trip after its own end. The named end, not the slack-inflated one, for the same reason. With no pong measured yet nothing is trimmed | +**A held entry's window is measured from the guard's *live* end, not the one stamped when the +press was made.** A fixed deadline reads right and has a hole in it: a cast pushed back twice +(`SMSG_SPELL_DELAYED`) ends after the window a press made at its start was given, so the entry +would time out while the thing it is waiting for is still running — and the press would vanish with +no packet and no error line, which is the one failure mode a player cannot describe. The same hole +swallows a press held through a cast whose global cooldown then outlasts the window. So the base +ratchets forward while anything still guards, and only ever forward. + **One behaviour worth knowing before testing.** Pressing a spell during a long cast holds it for the whole cast and fires it at the end — the last press wins, so pressing three different spells during a Pyroblast fires the third. That is upstream's `Replacing queued nonGcd spell X with Y` applied to