Troubleshoot operator on Base Sepolia and redo TaskRegistry for three triggrers #338
Conversation
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
The codebase has two distinct notification systems that were incorrectly overlapping: StreamCheckToOperator (Lines ~980-1050) - The "Primary Channel" Purpose: Send MonitorTaskTrigger messages with complete task metadata Triggers: When operators connect/reconnect via the streaming connection Data: Full TaskMetadata with TaskId, Remain, ExpiredAt, and complete Trigger object Delivery: Real-time via active gRPC streams Batched Notifications (sendBatchedNotifications) - The "Cleanup Channel" Purpose: Send CancelTask/DeleteTask messages with minimal data Triggers: Periodic batching (every 3 seconds) for efficiency Data: Just task ID and operation type (TaskMetadata = nil intentionally) Delivery: Batched for performance
- Remove BadgerDB runtime files (*.vlog, DISCARD, KEYREGISTRY, MANIFEST) from git tracking - Add BadgerDB patterns to .gitignore to prevent future commits of runtime data - Fix nil pointer dereference in integration tests by properly initializing User.SmartAccountAddress - Resolves panic: runtime error: invalid memory address or nil pointer dereference in ValidWalletOwner
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the task engine by migrating legacy data handling (using schedule maps and sync.Map) to a unified TaskRegistry, improving logging, error handling, and backward compatibility. Key changes include updating BlockTrigger, EventTrigger, and TimeTrigger implementations, modifying task notification handling in the Engine, and enhancing test cases to reflect these structural upgrades.
Reviewed Changes
Copilot reviewed 13 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| operator/worker_loop.go | Updated streaming message processing with additional debug logging and panic recovery strategies. |
| integration_test/operator_reconnection_test.go | Refactored user initialization to prevent nil pointer dereferences. |
| core/taskengine/trigger/time_test.go | Added tests for new time trigger functionality including legacy migration. |
| core/taskengine/trigger/time.go | Migrated legacy job management to the unified TaskRegistry with improved legacy conversion methods. |
| core/taskengine/trigger/registry_test.go | Added tests covering TaskRegistry conversion, basic operations, and thread safety. |
| core/taskengine/trigger/registry.go | Introduced a unified TaskRegistry to manage tasks across trigger types. |
| core/taskengine/trigger/event_test.go | Updated tests for event triggers to utilize the new TaskRegistry. |
| core/taskengine/trigger/event_conditional_test.go | Modified conditional filtering tests to align with the new event data API. |
| core/taskengine/trigger/event.go | Refactored event trigger logic to integrate TaskRegistry and legacy conversion from sync.Map. |
| core/taskengine/trigger/block_test.go | Adjusted tests to validate block trigger updates using TaskRegistry and legacy data conversion. |
| core/taskengine/trigger/block.go | Migrated block trigger scheduling from a legacy schedule map to the new TaskRegistry. |
| core/taskengine/engine.go | Updated task notification logic to warn against using batched notifications for MonitorTaskTrigger. |
| cmd/data/badger/KEYREGISTRY | Removed legacy key registry data. |
Comments suppressed due to low confidence (1)
core/taskengine/engine.go:1067
- [nitpick] Clarify in inline comments or documentation why MonitorTaskTrigger events are excluded from batched notifications and must be handled by StreamCheckToOperator to ensure complete task metadata.
if operation == avsproto.MessageOp_MonitorTaskTrigger {
- Remove all SEGFAULT_FIX debug logs and troubleshooting messages - Simplify MonitorTaskTrigger processing by removing verbose debug logging - Reduce file size by 190 lines (19% reduction) while maintaining functionality - Keep essential error handling and panic recovery for production safety - Improve performance by eliminating unnecessary debug log overhead - Clean up code for production readiness
- Add ensureLegacyConversion() helper function to EventTrigger, BlockTrigger, and TimeTrigger - Eliminate code duplication by replacing repeated detectLegacyData() + ensureNewFormat() calls - Improve maintainability and reduce redundancy in trigger implementations - All tests pass, maintaining backward compatibility
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request introduces significant updates to the
taskenginepackage, focusing on transitioning from legacy data structures to a unifiedTaskRegistryfor improved task management and backward compatibility. The changes streamline code, enhance logging, and ensure smooth migration from older formats. Key updates include modifications toBlockTriggerandEventTriggerimplementations, adjustments to task notification handling, and updates to test cases for compatibility.Transition to Unified
TaskRegistry:BlockTrigger Updates (
core/taskengine/trigger/block.go):TaskRegistryto replace the legacyschedulemap, enabling unified task management. Added methods for automatic migration from the old format, includingdetectLegacyDataandconvertFromScheduleMap. [1] [2]TaskRegistry, simplifying operations and improving logging. [1] [2] [3]EventTrigger Updates (
core/taskengine/trigger/event.go):sync.MapwithTaskRegistryfor managing event-based tasks. Added migration methods similar toBlockTrigger. [1] [2]Task Notification Handling:
core/taskengine/engine.go):MonitorTaskTriggeroperations, as these now rely onStreamCheckToOperatorfor sending complete task metadata. Added warnings to ensure correct usage. [1] [2]Test Case Updates:
core/taskengine/trigger/block_test.go):TaskRegistryinstead of the legacyschedulemap.These changes improve maintainability, ensure compatibility during migration, and enhance task management efficiency across the codebase.