refactor monitoring heartbeat loop & task.claim handler & swarm attachment missing use

[fblgit]

This PR:

• fix: creation attachment on swarm create_project under swarm.context
• refactor: monitoringworker duplication with instancemanager

{
  "taskText": "Consolidate health monitoring into MonitoringWorker by removing duplicate monitoring from InstanceManager. InstanceManager should only handle registration and heartbeat, while MonitoringWorker handles all health checking and failure detection. Use consistent configuration values from environment/config. Ensure no race conditions and maintain backward compatibility with existing tests.",
  "strategy": "mixed",
  "totalComplexity": 28,
  "reasoning": "The decomposition follows the natural refactoring flow: first understand the current state (st-1), then remove duplication from InstanceManager (st-2), enhance MonitoringWorker to handle all monitoring (st-3), update initialization (st-4), ensure proper handler integration (st-5), and finally verify with comprehensive tests (st-6, st-7). The strategy is mixed because some tasks can run in parallel (st-2 and st-3 after st-1), while others have strict dependencies.",
  "subtaskCount": 7,
  "subtasks": [
    {
      "id": "st-1",
      "description": "Analyze and document current health monitoring implementation in both InstanceManager and MonitoringWorker to identify exact duplication and dependencies",
      "specialist": "backend",
      "complexity": 3,
      "estimatedMinutes": 20,
      "dependencies": [],
      "context": {
        "files": [
          "apps/server/src/core/instance-manager.ts",
          "apps/server/src/core/jobs.ts",
          "apps/server/src/config.ts"
        ],
        "patterns": [
          "Code analysis",
          "Documentation",
          "Dependency mapping"
        ],
        "constraints": [
          "Must identify all test dependencies",
          "Document configuration usage"
        ]
      },
      "status": "pending"
    },
    {
      "id": "st-2",
      "description": "Remove health monitoring methods from InstanceManager: stopHealthMonitoring, monitorInstances, handleFailedInstance, and healthCheckInterval property",
      "specialist": "backend",
      "complexity": 5,
      "estimatedMinutes": 30,
      "dependencies": [
        "st-1"
      ],
      "context": {
        "files": [
          "apps/server/src/core/instance-manager.ts"
        ],
        "patterns": [
          "Clean refactoring",
          "Method removal",
          "State cleanup"
        ],
        "constraints": [
          "Preserve registration and heartbeat functionality",
          "Keep checkHealth method for other uses",
          "Add comments explaining removed monitoring"
        ]
      },
      "status": "pending"
    },
    {
      "id": "st-3",
      "description": "Update MonitoringWorker health-check job to use consistent configuration from environment variables and handle all instance monitoring logic",
      "specialist": "backend",
      "complexity": 6,
      "estimatedMinutes": 35,
      "dependencies": [
        "st-1"
      ],
      "context": {
        "files": [
          "apps/server/src/core/jobs.ts",
          "apps/server/src/config.ts"
        ],
        "patterns": [
          "Configuration injection",
          "Job scheduling",
          "BullMQ patterns"
        ],
        "constraints": [
          "Use healthMonitoring config values",
          "Maintain 3-second interval",
          "Preserve existing behavior for marking instances OFFLINE"
        ]
      },
      "status": "pending"
    },
    {
      "id": "st-4",
      "description": "Update server.ts initialization to remove instanceManager.startHealthMonitoring() call since monitoring is now handled by jobScheduler",
      "specialist": "backend",
      "complexity": 2,
      "estimatedMinutes": 15,
      "dependencies": [
        "st-2"
      ],
      "context": {
        "files": [
          "apps/server/src/server.ts"
        ],
        "patterns": [
          "Initialization sequence",
          "Service startup"
        ],
        "constraints": [
          "Keep instanceManager for registration",
          "Ensure jobScheduler starts monitoring"
        ]
      },
      "status": "pending"
    },
    {
      "id": "st-5",
      "description": "Ensure MonitoringWorker failure-detection job properly uses system.check_health handler and prevent any circular dependencies",
      "specialist": "backend",
      "complexity": 4,
      "estimatedMinutes": 25,
      "dependencies": [
        "st-3"
      ],
      "context": {
        "files": [
          "apps/server/src/core/jobs.ts",
          "apps/server/src/handlers/system/system.check_health.handler.ts"
        ],
        "patterns": [
          "Handler execution",
          "Error handling",
          "Async patterns"
        ],
        "constraints": [
          "Prevent circular imports",
          "Handle handler failures gracefully",
          "Use consistent timeout values"
        ]
      },
      "status": "pending"
    },
    {
      "id": "st-6",
      "description": "Write integration tests to verify health monitoring consolidation works correctly with no race conditions",
      "specialist": "testing",
      "complexity": 5,
      "estimatedMinutes": 40,
      "dependencies": [
        "st-2",
        "st-3",
        "st-4",
        "st-5"
      ],
      "context": {
        "files": [
          "apps/server/tests/integration/health-monitoring.test.ts"
        ],
        "patterns": [
          "Integration testing",
          "Async testing",
          "Redis mocking"
        ],
        "constraints": [
          "Test instance failure detection",
          "Test task redistribution",
          "Verify no duplicate monitoring",
          "Check configuration usage"
        ]
      },
      "status": "pending"
    },
    {
      "id": "st-7",
      "description": "Update any existing tests that depend on instanceManager.startHealthMonitoring or related removed methods",
      "specialist": "testing",
      "complexity": 3,
      "estimatedMinutes": 25,
      "dependencies": [
        "st-2",
        "st-6"
      ],
      "context": {
        "files": [
          "apps/server/tests/**/*.test.ts"
        ],
        "patterns": [
          "Test maintenance",
          "Mock updates",
          "Test refactoring"
        ],
        "constraints": [
          "Maintain test coverage",
          "Update mocks appropriately",
          "Ensure backward compatibility"
        ]
      },
      "status": "pending"
    }
  ],
  "decomposedAt": "2025-09-21T05:07:24.594Z",
  "decomposedBy": "worker-1"
}

• refactor: task.claim to be handler native without redis voodo

[claude]

Claude finished @fblgit's task —— View job

---

Code Review Complete ✅

I've completed my review of PR #13. The refactoring successfully consolidates health monitoring and improves the task.claim handler to follow the project's handler-centric architecture.

Summary

Strengths:

• Clean consolidation of health monitoring into MonitoringWorker
• task.claim refactored to use handler composition instead of Redis voodoo
• Proper swarm attachment implementation
• Good adherence to CLAUDE.md principles

Key Concerns:

• Potential race condition in task.claim between assign and update operations
• Missing test updates for the refactored components
• Configuration timing mismatch for health check intervals

Recommendation: Address the race condition and add tests before merging.

The full detailed review has been posted as a PR comment.

[claude]

Code Review for PR #13: Refactor monitoring heartbeat loop & task.claim handler

Overview

This PR successfully consolidates health monitoring into MonitoringWorker and refactors the task.claim handler to follow the handler-centric design pattern. The changes align well with the repository's CLAUDE.md principles of Event Democracy and Type Uniformity.

✅ Positive Changes

1. Monitoring Consolidation

   • Successfully removed duplicate health monitoring logic from InstanceManager (70 lines removed)
   • MonitoringWorker now handles all health checking through the job scheduler
   • Clean separation of concerns: InstanceManager focuses on registration/heartbeat, MonitoringWorker handles failure detection

2. task.claim Handler Refactoring

   • Properly converted from Redis Lua script approach to handler-native implementation
   • Now uses existing handlers (task.list, task.assign, task.update) following the event democracy principle
   • Added proper attachment fetching using task.list_attachments and task.get_attachments_batch
   • Maintains atomicity through sequential handler calls with proper error handling

3. Swarm Attachment Fix (line 383-400 in jobs.ts)

   • Correctly stores context as attachments using task.create_attachment instead of metadata updates
   • Follows the established pattern for attachment management

4. Configuration Management

   • Good use of environment variable overrides for health monitoring intervals
   • Consistent configuration access pattern throughout

🔍 Areas for Improvement

1. Race Condition Concern in task.claim
   Between task.assign and task.update (lines 61-85), another worker could potentially interfere. Consider adding a transactional pattern or using a single handler that performs both operations atomically.

2. Error Handling in MonitoringWorker
   Line 181-186 silently catches all errors and returns empty results. Consider propagating certain critical errors or implementing alerting for health check failures.

3. Performance Optimization Opportunity
   Lines 109-134 fetch all attachments even if not needed. Consider lazy loading attachments or adding a flag to optionally skip attachment fetching if the worker doesn't need them.

4. Missing Tests

   • The PR removes monitoring from InstanceManager but doesn't show updated tests for the new MonitoringWorker health checking
   • task.claim handler refactoring should have corresponding test updates

🚨 Potential Issues

1. Backward Compatibility

   • Line 286 in instance-manager.ts mentions "Health monitoring cleanup is now handled by MonitoringWorker" but cleanup() is empty
   • Tests expecting instanceManager.startHealthMonitoring() will fail

2. Configuration Timing

   • MonitoringWorker reads config values at job execution time (good for flexibility)
   • But JobScheduler sets intervals at startup time (lines 517-519)
   • This mismatch means changing HEALTH_CHECK_INTERVAL env var requires restart

Recommendations

1. Add Integration Tests: Verify the new monitoring flow works correctly and task claiming handles concurrent workers properly

2. Document Breaking Changes: Update documentation to reflect that health monitoring is now handled by JobScheduler, not InstanceManager

3. Consider Atomic Operations: For task.claim, consider creating a dedicated atomic Lua script or transaction pattern to prevent race conditions

4. Add Metrics: Track claim success/failure rates and monitoring job execution times

Adherence to CLAUDE.md Principles

✅ Event Democracy: Properly uses event handlers instead of direct Redis operations
✅ Localhost Reality: Maintains single-user focus without distributed complexity
✅ Type Uniformity: Uses established schemas and handlers
⚠️ Pragmatic Testing: Tests appear to be missing for the refactored components

Verdict

The refactoring is well-executed and follows the project's architectural patterns. However, please address:

1. The potential race condition in task.claim
2. Add/update tests for the refactored components
3. Document the breaking changes for users upgrading

Once these items are addressed, this PR will be ready for merge.