-
Notifications
You must be signed in to change notification settings - Fork 75
fix: add cooldown mechanism for event triggers with 5 min default value #436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add cooldown_seconds field to EventTrigger config (default: 300s/5min) - Implement cooldown logic to prevent repeated triggers when conditions remain true - Extract cooldown check and update methods for testability - Add comprehensive unit tests covering all cooldown scenarios - Define DefaultEventTriggerCooldownSeconds constant - Update registry and node_types to support cooldown configuration This prevents event triggers from firing constantly when conditions are met (e.g., price > threshold fires every block). The cooldown ensures tasks only trigger once per configured period, even if matching events continue to occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a cooldown mechanism to event triggers to prevent repeated task firing when matching conditions remain true (e.g., a price exceeding a threshold continues to fire every block). The implementation introduces a configurable cooldown period with a default of 5 minutes.
Key changes:
- Added
cooldown_secondsoptional field to EventTrigger protobuf configuration with default value of 300 seconds - Implemented cooldown tracking using a timestamp map with mutex-protected access
- Added comprehensive unit tests covering default, disabled, custom, and multi-task cooldown scenarios
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| protobuf/avs.proto | Added optional cooldown_seconds field to EventTrigger.Config with documentation |
| protobuf/avs.pb.go | Generated code for the new cooldown_seconds field with getter method |
| core/taskengine/trigger/registry.go | Updated EventTaskData struct to include CooldownSeconds field |
| core/taskengine/trigger/event.go | Implemented cooldown logic with isInCooldown/updateCooldownTimestamp methods and integrated into event processing |
| core/taskengine/trigger/event_test.go | Added comprehensive test suite covering all cooldown scenarios |
| core/taskengine/node_types.go | Added cooldown_seconds serialization to trigger configuration |
Comments suppressed due to low confidence (1)
core/taskengine/trigger/event.go:862
- The cooldown timestamp is only updated when the trigger is successfully sent to the channel (line 850). However, when the channel is full and the trigger is dropped (line 861), the cooldown timestamp is not updated. This creates an inconsistent behavior where a dropped trigger won't be counted toward the cooldown period, potentially allowing another trigger attempt immediately if another matching event occurs.
Consider updating the cooldown timestamp before attempting to send to the channel, or add the timestamp update in the default case as well to ensure consistent cooldown behavior regardless of whether the trigger was successfully delivered.
select {
case t.triggerCh <- triggerMeta:
// Update last trigger time after successfully sending to channel
if entry.EventData != nil {
t.updateCooldownTimestamp(taskID, entry.EventData.CooldownSeconds, now)
}
hasEnrichedData := marker.EnrichedData != nil
t.logger.Info("🎯 Task triggered with enriched data",
"task_id", taskID,
"block", log.BlockNumber,
"tx", log.TxHash.Hex(),
"log_index", log.Index,
"has_enriched_data", hasEnrichedData)
default:
t.logger.Warn("⚠️ Trigger channel full, dropping enriched trigger", "task_id", taskID)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Update cooldown extraction: use default (300s) only when not set (nil), respect explicit 0 - Remove unused legacy functions: logMatchesTask and evaluateEventConditions - Update legacy conversion to use default cooldown for old tasks - Update comments to reflect new cooldown behavior
- Add checkAndUpdateCooldown method that atomically checks cooldown and updates timestamp - Prevents TOCTOU vulnerability where concurrent events could both pass cooldown check - Follows same atomic pattern as deduplication logic in processLog - Initialize cooldownMutex explicitly in test setup for clarity
- Replace httpbin.org endpoints with MockAPIEndpoint in TestRunNodeImmediately_MalformedTemplateDetection - Replace httpbin.org endpoints with MockAPIEndpoint in TestRunNodeImmediately_ValidTemplateAfterFix - Prevents network timeout failures in CI environments
…ue (#436) * feat: add cooldown mechanism for event triggers - Add cooldown_seconds field to EventTrigger config (default: 300s/5min) - Implement cooldown logic to prevent repeated triggers when conditions remain true - Extract cooldown check and update methods for testability - Add comprehensive unit tests covering all cooldown scenarios - Define DefaultEventTriggerCooldownSeconds constant - Update registry and node_types to support cooldown configuration This prevents event triggers from firing constantly when conditions are met (e.g., price > threshold fires every block). The cooldown ensures tasks only trigger once per configured period, even if matching events continue to occur. * fix: update cooldown logic and remove unused legacy functions - Update cooldown extraction: use default (300s) only when not set (nil), respect explicit 0 - Remove unused legacy functions: logMatchesTask and evaluateEventConditions - Update legacy conversion to use default cooldown for old tasks - Update comments to reflect new cooldown behavior * fix: atomic cooldown check-and-update to prevent race condition - Add checkAndUpdateCooldown method that atomically checks cooldown and updates timestamp - Prevents TOCTOU vulnerability where concurrent events could both pass cooldown check - Follows same atomic pattern as deduplication logic in processLog - Initialize cooldownMutex explicitly in test setup for clarity * fix: use MockAPIEndpoint in template tests to prevent CI timeouts - Replace httpbin.org endpoints with MockAPIEndpoint in TestRunNodeImmediately_MalformedTemplateDetection - Replace httpbin.org endpoints with MockAPIEndpoint in TestRunNodeImmediately_ValidTemplateAfterFix - Prevents network timeout failures in CI environments
This prevents event triggers from firing constantly when conditions are met (e.g., price > threshold fires every block). The cooldown ensures tasks only trigger once per configured period, even if matching events continue to occur.
Note
Introduce per-task cooldown for event triggers (default 300s), plumbed via new
cooldown_secondsin proto and enforced in processing with unit tests.lastTriggerTimetracking,isInCooldownandupdateCooldownTimestamphelpers.processLogInternal; update timestamp on successful trigger; initialize tracking in constructor.DefaultEventTriggerCooldownSeconds = 300and enhance logs to includecooldown_seconds.EventTrigger.Configadds optionalcooldown_seconds; generatedavs.pb.goupdated (getter/oneof/raw desc).CooldownSecondsinEventTaskData; set from config or default; legacy conversion uses default.cooldown_secondsinTaskTriggerToConfigforeventTrigger.TestEventTriggerCooldowncovering default, disabled, custom, multi-task independence, prevention, and timestamp update.Written by Cursor Bugbot for commit db345bf. Configure here.