Implement task queue management system

[guzus]

• Add TaskQueue service with queue management logic
• Add QueuedTask and QueueStatus types
• Integrate queue into TaskHandlers for automatic task queueing
• Add /queue command for viewing and managing queue (list, clear, cancel)
• Add queue callback handlers for inline keyboard buttons
• Update StatusHandlers to mention queue command
• Tasks are automatically queued when there's already an active task
• Queue processes tasks automatically when previous tasks complete

[claude]

Pull Request Review: Implement task queue management system

Overview

This PR adds a comprehensive task queue management system that allows tasks to be automatically queued when there's already an active task running. The implementation is well-structured and follows good design patterns.

---

✅ Strengths

Architecture & Design

• Clean separation of concerns: TaskQueue is a standalone service that properly uses EventEmitter for async communication
• Good dependency injection: TaskQueue receives dependencies through constructor, making it testable
• Smart event-driven design: Using task_ready event to decouple queue processing from task execution

Code Quality

• Comprehensive JSDoc comments throughout the codebase
• Proper error handling: Try-catch blocks around message edits with graceful degradation
• Good logging: Appropriate use of logger for debugging and monitoring
• Type safety: Proper TypeScript types defined in src/types/index.ts

User Experience

• Inline keyboards for queue management (Clear, Refresh, Cancel buttons)
• Status messages that update as tasks move through the queue
• Position tracking so users know where they are in the queue
• Integration with existing /status command to mention queue functionality

---

⚠️ Issues & Concerns

1. Critical: Race Condition in Task Processing (High Priority)

Location: src/services/TaskQueue.ts:276-278

setTimeout(() => {
  this.processingUsers.delete(userId);
}, 3000);

Problem: Fixed 3-second timeout may cause race conditions:

• If task execution takes < 3s, multiple tasks could start simultaneously
• If task takes > 3s to register with executor, queue might start another task
• No verification that task actually started before releasing the lock

Recommendation: Instead of setTimeout, listen to task lifecycle events:

// When task actually starts in executor
this.executor.on('task_started', (userId) => {
  this.processingUsers.delete(userId);
});

Or wait for the task to be registered in the executor's active tasks map before releasing the lock.

---

2. Memory Leak: Uncleaned Empty Queues (Medium Priority)

Location: src/services/TaskQueue.ts:190

Problem: The Map<number, QueuedTask[]> grows indefinitely. Even after clearing a user's queue, the empty array remains in the Map.

Recommendation: Clean up empty queues:

this.queue.set(userId, []);
if (tasks.length > 0) {
  this.queue.delete(userId);  // Remove empty queues
}

Also apply this cleanup in dequeue() after the last task is removed.

---

3. Memory Leak: setInterval Never Cleared (High Priority)

Location: src/services/TaskQueue.ts:217-221

private startProcessor(): void {
  setInterval(async () => {
    await this.processQueue();
  }, 2000);
}

Problem: No way to stop the interval when the bot shuts down. This could cause:

• Tests that instantiate TaskQueue to hang
• Graceful shutdown issues
• Resource leaks in long-running processes

Recommendation:

private processorInterval?: NodeJS.Timeout;

private startProcessor(): void {
  this.processorInterval = setInterval(async () => {
    await this.processQueue();
  }, 2000);
}

public destroy(): void {
  if (this.processorInterval) {
    clearInterval(this.processorInterval);
  }
}

---

4. Unused Field: QueueStatus (Low Priority)

Location: src/services/TaskQueue.ts:10

private status: QueueStatus = QueueStatus.IDLE;

Problem:

• Field is set to IDLE but never updated to PROCESSING
• Only checked for PAUSED, which is never set
• The getStatus() method returns a value that doesn't accurately reflect queue state

Recommendation: Either:

1. Remove the field and getStatus() method if not needed
2. Properly maintain the status (set to PROCESSING when active, etc.)
3. Add a /queue pause command to actually use the PAUSED status

---

5. Potential Bug: Callback Data Length Limit (Medium Priority)

Location: src/services/TaskQueue.ts:74

callback_data: `queue_cancel:${task.id}`

Problem: Telegram callback data has a 64-byte limit. UUIDs are 36 chars, prefix is 13 chars = 49 bytes, which is safe but close to the limit.

Current status: ✅ Safe for now, but worth noting for future modifications.

---

6. Missing Error Handling in Event Listener (Medium Priority)

Location: src/handlers/TaskHandlers.ts:37-39

this.taskQueue.on('task_ready', (task: QueuedTask) => {
  this.executeQueuedTask(task);
});

Problem: If executeQueuedTask() throws an error, it could crash the application or leave the queue in a bad state.

Recommendation:

this.taskQueue.on('task_ready', async (task: QueuedTask) => {
  try {
    await this.executeQueuedTask(task);
  } catch (error) {
    logger.error('Failed to execute queued task', { taskId: task.id, error });
    // Optionally notify user
    await this.bot.sendMessage(task.chatId, `❌ Failed to start queued task: ${error.message}`);
  }
});

---

7. Code Duplication: formatDuration (Low Priority)

Location: src/services/TaskQueue.ts:325-329

Problem: Similar duration formatting logic likely exists in UIHelpers.formatDuration() (referenced in StatusHandlers.ts:23).

Recommendation: Reuse existing utility or extract to shared location per CLAUDE.md rule #1 (focused, clean codebase).

---

🔒 Security Review

✅ No Major Security Issues

• Input validation is appropriate (taskId prefix matching)
• No SQL injection risks (in-memory data structures)
• No XSS risks (Telegram Bot API handles escaping)
• User isolation is maintained (userId-based queue separation)

Minor Concerns:

• DoS potential: Users could queue unlimited tasks. Consider adding a max queue size per user (e.g., 10 tasks).

---

🧪 Test Coverage

❌ No Tests Provided (Critical)

Problem: This is a complex feature with multiple edge cases and NO test files exist in the repository (checked **/*.test.ts).

Impact:

• Race conditions and timing issues are hard to catch without tests
• Queue state management bugs could go unnoticed
• Refactoring becomes risky

Recommendation: Add unit tests for:

// Essential test cases:
- Task enqueuing when user has active task
- Task execution when queue becomes available  
- Cancelling queued tasks
- Clearing queue
- Position updates after removal
- Empty queue cleanup
- Concurrent queue operations
- Processing lock behavior

Per CLAUDE.md rule #5 (Run and Debug yourself PROACTIVELY), you should test this feature thoroughly before merging.

---

🎯 Performance Considerations

Concerns:

1. 2-second polling interval: Could be optimized to event-driven approach (listen to task completion events)
2. Iterating all users every 2 seconds: Scales poorly with many users. Consider indexed queue of "ready" users.
3. Multiple bot.editMessageText() calls: Could hit Telegram rate limits with many queued tasks

Recommendations:

• Consider making the queue processor event-driven rather than polling
• Add rate limiting awareness around message edits

---

📋 Additional Observations

Good Practices Followed:

• ✅ No new config variables (follows CLAUDE.md rule Add Claude Code GitHub Workflow #4)
• ✅ Clean, focused implementation (follows CLAUDE.md rule Fix Telegram Bot Repository Management Issues #1)
• ✅ Proper TypeScript typing
• ✅ Graceful degradation (ignoring message edit errors)

Minor Improvements:

1. Consider adding metrics/stats (e.g., average wait time, queue depth over time)
2. Add queue persistence for bot restarts (tasks currently lost on restart)
3. Consider priority queue for different task types

---

🎬 Final Verdict

Summary:

This is a well-designed feature with good architecture, but has some critical issues that should be addressed before merging:

Must Fix (Blocking):

• Fix race condition in task processing lock (issue Fix Telegram Bot Repository Management Issues #1)
• Fix memory leak from setInterval (issue Enable Other model (DeepSeek) / MCP / Skills integration #3)
• Add error handling to event listener (issue Add GitHub workflow for auto deployment via SSH #6)
• Add basic test coverage

Should Fix (Recommended):

• Clean up empty queues from Map (issue feat: run bots with mothership #2)
• Fix or remove unused QueueStatus field (issue Add Claude Code GitHub Workflow #4)
• Reuse existing formatDuration utility (issue Implement task queue management system #7)
• Add max queue size limit per user

Nice to Have:

• Event-driven processing instead of polling
• Queue persistence across restarts
• Add queue metrics/monitoring

Recommendation: Request changes to address the critical issues, particularly the race condition and memory leaks. The feature is solid but needs these fixes for production readiness.

---

Great work on this feature! The overall architecture is sound and the UX is well thought out. Once the critical issues are addressed, this will be a valuable addition to the bot. 🚀