fix(rules): don't consume cooldown when a fire dispatched nothing#178
Merged
Conversation
Engine.Handle bumped lastFire and fires for a matched rule BEFORE dispatching its actions. When the rule's only action was an ActionTool dropped because the global in-flight cap (maxToolActions=8) was saturated, nothing actually ran — yet the rule was recorded as fired and its cooldown clock started. The real damage is the cooldown: the NEXT genuinely-matching event inside the window is then wrongly suppressed, so a saturation blip silently swallows a real trigger. /rules list also over-reported Fires/LastFire. Handle now records the fire optimistically under the lock (so a concurrent Handle for the same rule is still cooldown-suppressed) and rolls it back when fire() reports that nothing dispatched: fires-- and lastFire restored to its prior value (or deleted). The restore is guarded by an Equal(bump) check so a concurrent successful fire that legitimately advanced lastFire — possible only for Cooldown==0, where lastFire is display-only; a real cooldown prevents two same-rule fires from both passing the check — is never clobbered. fire() now returns whether at least one action dispatched (a webhook/log ran, or a tool slot was reserved); a missing dispatcher, unknown kind, or saturation-drop counts as not-dispatched. Also clamps a negative Rule.Cooldown to 0 at Register (with a warn) so an operator sign typo can't slip past the `Cooldown > 0` suppression guard and silently disable rate-limiting. Found via the internal audit sweep of the automation engines (v0.795.0 fixed the campaign fail-open from the same sweep; this clears the two banked rules items). Tests (-race): - TestEngine_SaturationDropDoesNotConsumeCooldown: saturate the cap, fire a tool-only rule, assert Fires==0 and LastFire unset; free the cap and assert the same event now fires (proving the cooldown wasn't consumed). Proven to fail pre-fix — the drop consumed the cooldown and suppressed the next event. - TestEngine_NegativeCooldownClampedToZero. Verified: task ci green (lint 0 / vet / build / test:full -race 0 fail / govulncheck clean); task eval 17/17.
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.
Engine.Handle bumped lastFire+fires BEFORE dispatch. When a rule's only action was an ActionTool dropped due to the in-flight cap (maxToolActions=8) being saturated, nothing ran yet the cooldown clock started — so the NEXT genuinely-matching event was wrongly suppressed (a saturation blip silently swallows a real trigger). Handle now records optimistically under the lock (concurrent same-rule fires still cooldown-suppressed) and rolls back (fires--, restore lastFire guarded by Equal(bump) so a concurrent successful fire isn't clobbered) when fire() reports nothing dispatched. Also clamps a negative Cooldown to 0 at Register. Found via the automation-engine sweep (clears the 2 banked rules items). -race tests; the core test is proven to fail pre-fix. task ci green; eval 17/17.