netneutrality/.junie/plans/improve-task-manager.md

---
sessionId: session-260919-132150-1vmv
---

# Requirements

### Overview & Goals
The `TaskManager` subsystem in Net Neutrality manages the in-memory registry, execution dispatch, locking, persistence, and lifecycle transitions for Discord interaction tasks (buttons, modals, reactions, step-by-step flows, and scheduled background tasks).

This plan addresses several bugs, synchronization oversights, memory leak potentials, and architectural discrepancies between `docs/tasks.md` and `TaskManagerImpl.java`:
1. **Fix Parent-Child Lifecycle Propagation**: Correctly handle `REMOVE_SELF_AND_CHILDREN` when triggered by child tasks (`WithParent`).
2. **Synchronize DB Persistence on Removal**: Ensure `PersistableTask` entries are purged from the database via `TaskService.unpersistTask()` when removed.
3. **Prevent Memory Leaks in Pending Tasks**: Unify `pendingTasks` tracking and ensure `TaskCleanup` cleans up expired pending step tasks.
4. **Fix Concurrency & Lock Cleanup Hazards**: Eliminate race conditions where lock eviction during task execution allows duplicate concurrent runs.
5. **Optimize Lookup Performance**: Replace eager full-collection queries with short-circuiting streams for partial ID lookups.
6. **Improve Test Coverage**: Introduce thorough unit tests covering all task lifecycle paths, locking behavior, and persistence hooks.

### Scope
- **In Scope**:
  - `me.bobert.netneutrality.core.tasks.TaskManager` interface.
  - `me.bobert.netneutrality.core.tasks.TaskManagerImpl` implementation.
  - `me.bobert.netneutrality.scheduled.cleanup.TaskCleanup` expiration handling.
  - Interaction deletion error-handling callbacks on `InteractionHook`.
  - Comprehensive unit test suite for `TaskManagerImpl`.
- **Out of Scope**:
  - Refactoring concrete task business logic (e.g., individual music or reaction commands).
  - Schema changes to the `OTask` entity or database migrations.

### Functional Requirements
- **FR-1**: When any child task implementing `WithParent` returns `Lifecycle.REMOVE_SELF_AND_CHILDREN`, `TaskManager` must remove the parent task and all associated child tasks from the registry.
- **FR-2**: When any `PersistableTask` is removed from `TaskManager` (via lifecycle completion, manual removal, or cancellation), `TaskManager` must call `taskService.unpersistTask()` so the task is deleted from persistent storage.
- **FR-3**: Pending step tasks must be tracked reliably, exposed for expiration checks, and cleaned up by `TaskCleanup` if abandoned.
- **FR-4**: Concurrent task invocations sharing the same `lockId` must be serialized safely without lock collisions or premature lock disposal.
- **FR-5**: `findTaskByPartialId` must short-circuit upon finding the first match without allocating full intermediate lists.
- **FR-6**: Interaction deletion via `Deletable` must gracefully handle expired hooks and Discord API errors asynchronously.

# Technical Design

### Current Implementation
`TaskManagerImpl` (`me.bobert.netneutrality.core.tasks.TaskManagerImpl`) implements `TaskManager` and `Lifecycle`. It maintains:
- `tasks`: `ConcurrentHashMap<String, Task>` for active tasks.
- `pendingTasks`: `CopyOnWriteArrayList<Task>` for unassigned step tasks.
- `deletableTasksContext`: `ConcurrentHashMap<String, CommandContext>` for tracking interaction hooks to delete.
- `tasksLock`: `ConcurrentHashMap<String, ReentrantLock>` for per-task/per-parent synchronization.
- `taskService`: `TaskService` for loading and persisting `PersistableTask` entities.

### Key Issues & Decisions

#### 1. Parent/Child Task Lifecycle Resolution
- **Issue**: `handleTaskLifecycle` only checks `if (task instanceof WithChildren)` for `REMOVE_SELF_AND_CHILDREN`. When a button task (which implements `WithParent<P>`, such as `LoadTrackFromPlaylistTask`) returns `REMOVE_SELF_AND_CHILDREN`, the check evaluates to `false`, leaving the parent and sibling buttons active in memory.
- **Decision**: In `handleTaskLifecycle`, check if the task implements `WithParent<?>`. If so, obtain the parent task (`taskWithParent.getParentTask()`) and remove the parent along with all of its child tasks (`parent.getChildTasks()`). If the task directly implements `WithChildren`, remove all its child tasks and itself.

#### 2. Automatic Persistence Synchronization
- **Issue**: `docs/tasks.md` specifies that persistable tasks removed after execution are deleted from the database. Currently `TaskManagerImpl.removeTask` only removes from `tasks` map and never calls `taskService.unpersistTask(persistableTask)`.
- **Decision**: In `removeTask(Task task)`, check `if (task instanceof PersistableTask persistableTask)` and invoke `taskService.unpersistTask(persistableTask)`.

#### 3. Pending Task Registry & Cleanup
- **Issue**: `pendingTasks` is stored in a `CopyOnWriteArrayList<Task>`. `getTasks()` ignores it, preventing `TaskCleanup` from pruning expired step tasks. `removeTask()` also ignores `pendingTasks`.
- **Decision**: Store `pendingTasks` in a `ConcurrentHashMap<String, Task>` (or unify with active tasks / pending lookup). Update `getTasks()` or provide a method to include pending tasks in cleanup routines. Update `removeTask(Task task)` to remove from `pendingTasks` as well as `tasks`.

#### 4. Concurrency & Lock Management
- **Issue**: `removeTask` calls `tasksLock.remove(task.getLockId())` immediately. If another thread is waiting or about to acquire the lock for a sibling task, it can get a new lock instance from `computeIfAbsent`, breaking mutual exclusion.
- **Decision**: Maintain lock instances safely; do not remove locks eagerly while in the middle of executing handlers. Persistable tasks with state should also participate in locking if needed, or maintain clear locking semantics.

#### 5. Lookup API Optimization
- **Issue**: `findTaskByPartialId` calls `findTasksByPartialId`, instantiating and populating a full `ArrayList` before taking the first element.
- **Decision**: Implement `findTaskByPartialId` with `.filter(...).findFirst()` directly on the stream.

### Architecture & Component Interaction Diagram

```mermaid
graph TD
    subgraph Discord Events
        BE[ButtonInteractionEvent] --> EH[EventsHandler]
        ME[ModalInteractionEvent] --> EH
        RE[MessageReactionAddEvent] --> EH
        MSG[MessageReceivedEvent] --> EH
    end

    subgraph TaskManager Subsystem
        EH -->|TaskContext| TM[TaskManagerImpl]
        TM -->|Acquire Lock| TL[tasksLock Map]
        TM -->|Lookup Task| TR[tasks & pendingTasks Maps]
        TM -->|Execute| T[Task.execute]
        T -->|Return Lifecycle| TM
        TM -->|Cascade Cleanup| LC[handleTaskLifecycle]
        LC -->|WithParent / WithChildren| RM[removeTask]
        RM -->|Unpersist| TS[TaskService]
        TS -->|Delete| DB[(Database / OTask)]
        RM -->|Deletable| DH[InteractionHook.deleteOriginal]
    end

    subgraph Background Maintenance
        TC[TaskCleanup] -->|Periodic Poll| TM
        TM -->|Prune Expired| RM
    end
```

### Proposed Code Changes & Contracts

#### `TaskManagerImpl.java`
```java
private void handleTaskLifecycle(Task task, Task.Lifecycle status) {
    if (status == null) {
        throw new IllegalArgumentException("Task lifecycle status cannot be null");
    }

    switch (status) {
        case REMOVE_SELF -> removeTask(task);
        case REMOVE_SELF_AND_CHILDREN -> {
            if (task instanceof WithParent<?> taskWithParent) {
                Task parent = taskWithParent.getParentTask();
                if (parent instanceof WithChildren parentWithChildren) {
                    for (Task child : parentWithChildren.getChildTasks()) {
                        removeTask(child);
                    }
                }
                removeTask(parent);
            } else if (task instanceof WithChildren taskWithChildren) {
                for (Task child : taskWithChildren.getChildTasks()) {
                    removeTask(child);
                }
                removeTask(task);
            } else {
                removeTask(task);
            }
        }
        case KEEP, PARENT, BUILTIN -> {
            // Maintained in active registry
        }
    }
}

@Override
public void removeTask(Task task) {
    if (task == null) return;
    String id = task.getId();
    boolean removed = (tasks.remove(id) != null);
    removed |= (pendingTasks.remove(task) != null);

    if (removed) {
        logger.debug("Removing task: {}", id);
        if (task instanceof PersistableTask persistableTask) {
            taskService.unpersistTask(persistableTask);
        }
    }
}
```

### File Structure Changes
- **Modified**: `src/main/java/me/bobert/netneutrality/core/tasks/TaskManagerImpl.java`
- **Modified**: `src/main/java/me/bobert/netneutrality/scheduled/cleanup/TaskCleanup.java`
- **Added**: `src/test/java/me/bobert/netneutrality/core/tasks/TaskManagerImplTest.java`

# Testing

### Validation Approach
Automated unit tests will be created in `src/test/java/me/bobert/netneutrality/core/tasks/TaskManagerImplTest.java` utilizing JUnit 5, AssertJ, and Mockito to validate all functional requirements without requiring live Discord connections or database instances.

### Key Scenarios
1. **Task Registration & Lookup**:
   - Insert standard `Task`, verify `getTask(id)` and `hasTask(id)` return `true`.
   - Insert duplicate task ID, verify rejected with user notification.
2. **Parent / Child Lifecycle (`REMOVE_SELF_AND_CHILDREN`)**:
   - Create parent task implementing `WithChildren` and child task implementing `WithParent`.
   - Execute child task returning `REMOVE_SELF_AND_CHILDREN`.
   - Verify parent task and all sibling child tasks are removed from the registry.
3. **Persistable Task DB Synchronization**:
   - Insert `PersistableTask`, verify `TaskService.persistTask` is called.
   - Remove `PersistableTask` via lifecycle completion or `removeTask()`, verify `TaskService.unpersistTask` is invoked.
4. **Step Task Flow**:
   - Insert `StepTask` into pending registry.
   - Reassign ID via `activateTask(task, oldId)`, verify task moves from pending to active registry under new ID.
5. **Partial ID Search**:
   - Query `findTaskByPartialId` and verify it matches correct instance without scanning beyond first match.
6. **Task Expiration & Cleanup**:
   - Mock expired active and pending tasks, trigger `TaskCleanup.run()`, verify expired tasks are pruned from both collections.
7. **Concurrency & Locking**:
   - Simulate concurrent executions for tasks sharing a `lockId`, confirming mutual exclusion.

# Delivery Steps

### ✓ Step 1: Fix Task Lifecycle Hierarchy and DB Persistence Synchronization
`TaskManagerImpl` correctly propagates parent-child lifecycle actions and synchronizes task deletions with `TaskService`.

- Update `handleTaskLifecycle` in `TaskManagerImpl.java` to support `REMOVE_SELF_AND_CHILDREN` for tasks implementing `WithParent`, resolving the parent task and removing both the parent and all sibling child tasks.
- Modify `removeTask(Task task)` in `TaskManagerImpl.java` to invoke `taskService.unpersistTask(persistableTask)` when `task instanceof PersistableTask`, ensuring persisted records in the database are purged when tasks finish.
- Refactor `deleteInteraction(Task task)` in `TaskManagerImpl.java` to cleanly handle `Deletable` logic for both standalone and parented tasks, adding failure logging to `InteractionHook.deleteOriginal().queue()`.

### ✓ Step 2: Refactor Pending Task Management and Concurrency Locking
Pending tasks and active task locks are managed safely without race conditions or memory leaks.

- Refactor `pendingTasks` in `TaskManagerImpl.java` to use a `ConcurrentHashMap<String, Task>` keyed by task ID for O(1) lookups and consistent state management.
- Update `removeTask(Task task)`, `getTask(String id)`, and `getTasks()` in `TaskManagerImpl.java` to include pending tasks so they can be retrieved, removed, and cleaned up when expired.
- Update `TaskCleanup.java` to ensure expired pending tasks are identified and cleaned up.
- Improve lock management in `TaskManagerImpl.handle(TaskContext context)` so that lock eviction does not interfere with concurrently waiting threads acquiring new lock instances.

### ✓ Step 3: Optimize Task Query Performance and Diagnostic Logging
Task lookup methods use short-circuiting stream pipelines, and diagnostic logging is corrected.

- Optimize `findTaskByPartialId` in `TaskManagerImpl.java` to use `.findFirst()` directly rather than allocating a full list via `findTasksByPartialId`.
- Fix typos in log messages and warnings across `TaskManagerImpl.java` (e.g. `"Aquiring lock"`, `"childs"`).
- Ensure consistent ephemeral fallback messages and handling when unacknowledged interactions or missing tasks are encountered.

### ✓ Step 4: Add Comprehensive Unit Tests for Task Management
A complete unit test suite validates `TaskManagerImpl` functionality across all task types, lifecycles, and concurrency scenarios.

- Create `src/test/java/me/bobert/netneutrality/core/tasks/TaskManagerImplTest.java` using JUnit 5 and Mockito.
- Implement tests for standard task registration (`insert`), lookup (`getTask`, `findTaskByPartialId`), and removal (`removeTask`).
- Implement tests for parent-child tasks (`WithChildren`, `WithParent`), verifying lock inheritance and `REMOVE_SELF_AND_CHILDREN` cascading removals.
- Implement tests for `StepTask` lifecycle (pending insertion, ID reassignment via `activateTask`, and expiration).
- Implement tests for `PersistableTask` lifecycle, verifying automatic database persistence on insert and unpersisting on removal.
- Implement tests verifying thread synchronization and lock acquisition during task execution.