Skip to content

feat(webgui): backend-tracked task queue with multi-task tray + foreground recall#2665

Open
elibosley wants to merge 13 commits into
masterfrom
feat/backend-task-queue
Open

feat(webgui): backend-tracked task queue with multi-task tray + foreground recall#2665
elibosley wants to merge 13 commits into
masterfrom
feat/backend-task-queue

Conversation

@elibosley

@elibosley elibosley commented Jun 17, 2026

Copy link
Copy Markdown
Member

Summary

Replaces the single-backgrounded-task / single-banner model with a backend-owned task queue shared across subsystems (plugins / Docker / VM) and all browser clients. Operations can now be queued, brought back to the foreground from any tab (with output replayed from a server-side log), and tracked in a tray — instead of only one untracked background task with no way to reopen it.

This touches core OS webgui plumbing loaded on every page (DefaultPageLayout, the nchan subscribers, and publish.php), so it warrants careful review.

How it works

  • One running op per channel-type. Because at most one plugin/docker/vmaction op streams at a time, the existing shared /sub/{plugins,docker,vmaction} channels never interleave — so no backend op-script or nginx changes are needed. Extra same-type ops are queued and auto-advanced on completion.
  • The full task list is broadcast on a new /sub/tasks channel; any client renders the same tray and can foreground/abort/dismiss.
  • Per-task output is captured to a server-side log so foregrounding replays history, then continues live.

Changes

New

  • TaskQueue.php — per-task JSON store under /var/local/emhttp/tasks + scheduler helpers (reuses file_put_contents_atomic; mirrors the File Manager queue).
  • TaskCommand.php — endpoint: create / abort / dismiss / log / list; every mutation rebroadcasts the list. POST mutations are CSRF-protected by local_prepend.php.
  • nchan/tasks — scheduler daemon modeled on nchan/file_manager: watches pids, marks done/error, advances the queue, exits when idle.

Edited

  • publish.php — env-var-gated (NCHAN_TASK) per-task log tee for replay. No-op on every other publish path — worth a reviewer's eye since this function is on every streaming path.
  • DefaultPageLayout.php — keeps the daemon alive while tasks exist.
  • DefaultPageLayout/HeadInlineJS.phpopenPlugin/openDocker/openVMAction keep their exact signatures but now enqueue + foreground a task; abortOperation repurposed; bannerAlert retired to a stub.
  • DefaultPageLayout/BodyInlineJS.php/sub/tasks subscriber, router/renderer split of the three live handlers, foregroundTask (replay-then-live), tray render; replaces the old addAlert cookie restore.
  • MiscElementsBottom.php + styles/default-base.css#opTray element + theme-aware styles.

Testing status

  • php -l (short tags on) on all PHP, node --check on both JS bodies.
  • ✅ Sandboxed store-logic test: serialization, cross-type concurrency, dedupe, auto-advance, invalid-type rejection, TTL prune.
  • ⚠️ Not yet runtime-tested on a live Unraid box — nchan pub/sub, real pgrep --ns/process launch, and the foreground-replay UX still need on-device verification (two-tab shared tray, same-type queueing, Docker replay incl. show_Wait dots, abort/dismiss).

Review notes

  • Abort sends kill to the launched shell pid — identical best-effort behavior to the previous StartCommand path (no regression, but same caveat).
  • New files live in plugins/dynamix/include/; the frontend references /plugins/dynamix/include/TaskCommand.php (same web-served dir as Control.php).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added a fixed background-operation task tray with real-time queued/running/done/error updates.
    • Introduced a server-side task queue with per-type sequencing, automatic pruning, and persistent logs.
  • UI / Styling
    • Implemented tray layout and state-specific row styling, with responsive placement above the fixed footer.
  • Behavior Changes
    • Updated background-operation rendering to use centralized live task channels with foreground task progress and modal updates.
    • Improved task controls (abort/dismiss/clear finished/confirm abort) with optimistic task entry and a safer abort fallback.
    • Kept the background scheduler active when task files exist, and captured publish payloads for later replay.

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR replaces the legacy banner-based background operation flow with a persistent multi-task queue system. A new TaskQueue.php backend persists task records to filesystem JSON files, TaskCommand.php exposes HTTP task actions, and an nchan polling daemon monitors running tasks. The client gains a centralized task-state model, shared nchan subscribers per operation type, and a fixed-position operation tray.

Changes

Multi-task background operation system

Layer / File(s) Summary
Backend task queue and publish log capture
emhttp/plugins/dynamix/include/TaskQueue.php, emhttp/plugins/dynamix/include/publish.php
TaskQueue.php introduces filesystem-persisted task records under /var/local/emhttp/tasks, all core functions for task CRUD, FIFO listing, nchan broadcast, command resolution, async launch, per-type scheduling, daemon management, deduplication, creation, and TTL pruning. publish.php appends nchan messages to a per-task log file when NCHAN_TASK is set, enabling client-side log replay.
TaskCommand HTTP endpoint, nchan daemon, and nchan registration
emhttp/plugins/dynamix/include/TaskCommand.php, emhttp/plugins/dynamix/nchan/tasks, emhttp/plugins/dynamix/include/DefaultPageLayout.php
TaskCommand.php routes action requests (create/abort/dismiss/clear/log/list) to TaskQueue functions. The nchan/tasks daemon polls running task PIDs, transitions statuses to done/error, advances queued work, and exits via removeNChanScript() when idle. DefaultPageLayout.php conditionally registers the tasks nchan script when task JSON files exist.
Client-side task creation and operation entry points
emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php
Removes legacy addAlert cookie setup. bannerAlert becomes a no-op stub. openPlugin, openDocker, and openVMAction delegate to a new createTask() helper posting to TaskCommand.php and forwarding the returned task ID to foregroundTask. abortOperation(pid) resolves via taskByPid/confirmAbortTask with a kill-POST fallback.
Client-side task state, nchan subscribers, message rendering, and tray
emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php, emhttp/plugins/dynamix/include/DefaultPageLayout/MiscElementsBottom.php, emhttp/plugins/dynamix/styles/default-base.css
Introduces per-type nchan subscribers mapped by nchanByType, task state variables (taskList, foregroundTaskId, foregroundType), and helpers. renderMessage centralizes modal log/progress updates; routeMessage gates live messages to the foregrounded task and handles _DONE_/_ERROR_. foregroundTask opens a modal, replays the server log, and starts/finalizes live streaming. onTaskListUpdate reacts to server-pushed status transitions via taskChannel. trayRender builds #opTray rows; page init starts taskChannel so the tray reflects server state. MiscElementsBottom.php adds the hidden #opTray container; default-base.css styles the fixed-position tray with color-coded task borders and responsive footer avoidance.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant HeadInlineJS as openPlugin/openDocker/openVMAction
  participant TaskCommand.php
  participant TaskQueue.php
  participant nchanDaemon as nchan/tasks daemon
  participant taskChannel as taskChannel (/sub/tasks)
  participant BodyInlineJS as onTaskListUpdate / foregroundTask

  User->>HeadInlineJS: trigger operation
  HeadInlineJS->>TaskCommand.php: POST action=create
  TaskCommand.php->>TaskQueue.php: task_create() → task_launch()
  TaskQueue.php-->>TaskCommand.php: {id, status}
  TaskCommand.php-->>HeadInlineJS: JSON {id, status}
  HeadInlineJS->>BodyInlineJS: foregroundTask(id) — open modal
  TaskQueue.php->>taskChannel: task_publish() via nchan
  taskChannel->>BodyInlineJS: onTaskListUpdate()
  BodyInlineJS->>BodyInlineJS: start nchanByType[type] live stream
  nchanDaemon->>TaskQueue.php: poll PID liveness
  nchanDaemon->>TaskQueue.php: task_write() status=done/error
  TaskQueue.php->>taskChannel: task_publish()
  taskChannel->>BodyInlineJS: onTaskListUpdate() → fireTaskCallback / trayRender
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 A bunny hops through queues today,
Tasks lined up in JSON array.
No banners wave—the tray gleams bright,
Each job runs tracked through darkest night.
_DONE_ arrives, the daemon sleeps,
While nchan hums and order keeps! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly reflects the main changes: a backend-tracked task queue system with multi-task support and the ability to bring tasks to the foreground. It accurately summarizes the core innovation across all modified files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/backend-task-queue

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown

🔧 PR Test Plugin Available

A test plugin has been generated for this PR that includes the modified files.

Version: 2026.06.22.2332
Build: View Workflow Run

📥 Installation Instructions:

Install via Unraid Web UI:

  1. Go to Plugins → Install Plugin
  2. Copy and paste this URL:
https://preview.dl.unraid.net/pr-plugins/pr-2665/webgui-pr-2665.plg
  1. Click Install

Alternative: Direct Download

⚠️ Important Notes:

  • Testing only: This plugin is for testing PR changes
  • Backup included: Original files are automatically backed up
  • Easy removal: Files are restored when plugin is removed
  • Conflicts: Remove this plugin before installing production updates
  • Post-merge behavior: This preview stays available after merge until preview storage expires or it is manually cleaned up

📝 Modified Files:

Click to expand file list
emhttp/plugins/dynamix.docker.manager/javascript/docker.js
emhttp/plugins/dynamix.plugin.manager/Plugins.page
emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php
emhttp/plugins/dynamix/Apps.page
emhttp/plugins/dynamix/Language.page
emhttp/plugins/dynamix/Tailscale.page
emhttp/plugins/dynamix/include/DefaultPageLayout.php
emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php
emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php
emhttp/plugins/dynamix/include/DefaultPageLayout/MiscElementsBottom.php
emhttp/plugins/dynamix/include/TaskCommand.php
emhttp/plugins/dynamix/include/TaskQueue.php
emhttp/plugins/dynamix/include/publish.php
emhttp/plugins/dynamix/include/task_complete
emhttp/plugins/dynamix/nchan/tasks
emhttp/plugins/dynamix/styles/default-base.css

🔄 To Remove:

Navigate to Plugins → Installed Plugins and remove webgui-pr-2665, or run:

plugin remove webgui-pr-2665

🤖 This comment is automatically generated and will be updated with each new push to this PR.

@elibosley elibosley marked this pull request as ready for review June 18, 2026 14:08

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (5)
emhttp/plugins/dynamix/include/TaskQueue.php (1)

147-177: ⚖️ Poor tradeoff

Potential race condition in task creation and launch.

Between the deduplication check (lines 151-156), the task_running_type() check (line 173), and task_launch(), another concurrent request could create or launch a task of the same type. This could violate the "at most one running task per type" invariant if two requests race.

Consider using file locking around the create-and-launch sequence to ensure atomicity:

♻️ Suggested approach using flock
function task_create($type,$cmd,$title,$plg,$func,$start,$button) {
  if (!in_array($type, TASK_TYPES)) return null;
  
  $lockfile = task_dir() . "/.lock.$type";
  $fp = fopen($lockfile, 'c');
  if (!flock($fp, LOCK_EX)) { fclose($fp); return null; }
  
  try {
    // dedupe check and task creation logic here...
    // (existing code)
  } finally {
    flock($fp, LOCK_UN);
    fclose($fp);
  }
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@emhttp/plugins/dynamix/include/TaskQueue.php` around lines 147 - 177, The
task_create function has a race condition where concurrent requests could
violate the "at most one running task per type" invariant between the
deduplication check (inside the loop checking task_list) and the task_launch
call. To fix this, wrap the entire deduplication check, task creation, and task
launch logic (from the initial in_array check through the task_launch call and
task_publish call) in a file lock using flock on a lock file specific to the
task type stored in the task directory. Acquire an exclusive lock before
entering the critical section and release it in a finally block to ensure the
lock is always released, maintaining atomicity for the create-and-launch
sequence.
emhttp/plugins/dynamix/include/TaskCommand.php (2)

35-51: 💤 Low value

Consider returning a JSON response from abort/dismiss for consistency.

The abort and dismiss actions call die() without a response body, while create and list return JSON. While the client code (BodyInlineJS.php) doesn't appear to use the response, returning a consistent JSON acknowledgment (e.g., {"ok":true}) would improve API consistency and debuggability.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@emhttp/plugins/dynamix/include/TaskCommand.php` around lines 35 - 51, The
abort case (and similarly the dismiss case) currently calls die() without
returning any response body, while other actions like create and list return
JSON responses. Replace the die() call at the end of the abort case with a JSON
response using die(json_encode(['ok' => true])) to provide a consistent JSON
acknowledgment across all task command actions. Apply the same change to the
dismiss case for consistency throughout the TaskCommand.php file.

38-44: 💤 Low value

Verify PID validity more robustly before kill.

The check $task['pid'] > 1 guards against killing PID 0 or 1, but $task['pid'] is stored as a string (from exec() output). The comparison works due to PHP's type juggling, but consider:

  1. The PID could be empty string or non-numeric if exec failed
  2. Validating the process still belongs to the expected task (though this may be overkill)

Current implementation is acceptable since escapeshellarg() handles edge cases safely, but explicit validation would be clearer:

♻️ Optional: explicit numeric validation
-    if ($task['status']==='running' && $task['pid'] > 1) {
+    if ($task['status']==='running' && is_numeric($task['pid']) && (int)$task['pid'] > 1) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@emhttp/plugins/dynamix/include/TaskCommand.php` around lines 38 - 44, The PID
validation in the condition `if ($task['status']==='running' && $task['pid'] >
1)` relies on PHP's implicit type juggling since $task['pid'] is a string from
exec() output. To make this more explicit and robust, add explicit numeric
validation before the kill operation. Replace or enhance the condition to
explicitly check that $task['pid'] is numeric using is_numeric() or
ctype_digit() before proceeding with the exec('kill
'.escapeshellarg($task['pid'])) call, ensuring the PID is a valid numeric value
and not an empty string or non-numeric value from a failed exec.
emhttp/plugins/dynamix/nchan/tasks (1)

32-37: 💤 Low value

_ERROR_ marker detection may have false positives.

The error detection searches for the literal string _ERROR_ anywhere in the last 64KB of the log. If a task's legitimate output happens to contain this string (e.g., in a log message or user data), it would incorrectly mark the task as failed.

Consider making the marker more unique or checking for it only at specific positions (e.g., as a standalone message in the RS-delimited format).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@emhttp/plugins/dynamix/nchan/tasks` around lines 32 - 37, The
task_log_has_error function uses a simple strpos search for the literal string
_ERROR_ which can cause false positives if this string appears in legitimate
task output. To fix this, make the error marker detection more robust by either
creating a more unique/distinctive error marker string (e.g., wrapping it with
special delimiters) or by checking for it only in specific positions or formats
such as standalone messages in RS-delimited format. Update the strpos check in
task_log_has_error to use this more specific detection logic instead of
searching for the bare _ERROR_ string anywhere in the log tail output.
emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php (1)

126-177: ⚖️ Poor tradeoff

Unescaped HTML in nchan message rendering.

Several locations render nchan message data directly into innerHTML/html() without escaping (Lines 142, 155, 160, 173). While these messages originate from server-side processes, if a process outputs content containing <script> tags or event handlers, it could execute in the browser.

This appears to be preserved behavior from the legacy handlers, but worth noting for a security-conscious codebase. Consider escaping output or using textContent where HTML formatting isn't required.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php` around
lines 126 - 177, The renderMessage function renders nchan message data directly
into DOM elements using innerHTML and html() without escaping, which could allow
script injection if processes output malicious content. In the 'plugins',
'addLog', and 'addToID' cases within the switch statement, replace direct HTML
concatenation with properly escaped content or use textContent for plain text
output. Create a helper function to escape HTML entities (replacing &, <, >, ",
') and apply it to all data values (data[1], data[2], label, etc.) before
inserting them into the DOM, or use textContent instead of innerHTML where HTML
formatting is not required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php`:
- Around line 200-239: The `foregroundTask` function passes `task.title`
directly into the swal dialog title without escaping, and since `html:true` is
enabled, any HTML content in the title will be rendered as markup. Escape the
`task.title` value using a JavaScript HTML escaping function (such as a utility
function that converts characters like <, >, &, and quotes to their HTML entity
equivalents) before inserting it into the swal title string, or use a DOM
manipulation method that treats the content as text rather than HTML to prevent
malicious scripts from executing.
- Around line 271-296: In the trayRender function, the task ID variable t.id is
being directly embedded into onclick handler attribute strings without escaping
on multiple lines (in foregroundTask, confirmAbortTask, cancelTask, and
dismissTask calls). This creates a potential XSS vulnerability if t.id contains
special characters or quotes. Apply the existing escapeTaskHtml function
(already used for t.title in the same function) to escape t.id wherever it
appears in the onclick handler strings to prevent the ID from breaking the
handler or enabling injection attacks.

In `@emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php`:
- Around line 183-192: The createTask function uses $.post() to submit a request
but lacks error handling for network or server failures. When the POST request
fails, the spinner gets hidden in the success callback but the user receives no
error feedback. Add error handling by chaining a .fail() method to the $.post()
call to display an error notification to the user when the request fails, and
consider whether the spinner should be hidden or kept visible during error
states.

In `@emhttp/plugins/dynamix/include/TaskQueue.php`:
- Around line 119-128: The exec() call in the task execution block contains a
command injection vulnerability where the $args variable from task_resolve() is
interpolated directly into a single-quoted bash command without proper escaping.
Although $name is safely handled and $task['id'] uses escapeshellarg(), the
$args variable can contain shell metacharacters including single quotes that
break out of the quoted context. Apply escapeshellarg() to the $args variable
just like you do for $task['id'] before interpolating it into the bash command
string to ensure all shell metacharacters are properly escaped.

In `@emhttp/plugins/dynamix/nchan/tasks`:
- Around line 27-30: The task_pid_alive function does not validate that the PID
is numeric before checking if the /proc directory exists. When $pid is an empty
string, file_exists("/proc/") returns true because /proc is a directory, causing
tasks to be stuck in running state. Fix this by adding a numeric validation
check for $pid (such as is_numeric() or ensuring it's a positive integer) before
the file_exists() call to ensure only valid numeric PIDs are checked against
/proc/$pid.

In `@emhttp/plugins/dynamix/styles/default-base.css`:
- Around line 1919-1924: Add the CSS property `min-width: 0` to the `.op-tray
.op-title` rule. This property is required for flex items with `overflow:
hidden` and `text-overflow: ellipsis` to properly shrink and enable reliable
ellipsis behavior. Without it, long titles may not shrink appropriately and can
overflow, pushing action icons out of view. Insert this property among the
existing declarations in the `.op-tray .op-title` selector.

---

Nitpick comments:
In `@emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php`:
- Around line 126-177: The renderMessage function renders nchan message data
directly into DOM elements using innerHTML and html() without escaping, which
could allow script injection if processes output malicious content. In the
'plugins', 'addLog', and 'addToID' cases within the switch statement, replace
direct HTML concatenation with properly escaped content or use textContent for
plain text output. Create a helper function to escape HTML entities (replacing
&, <, >, ", ') and apply it to all data values (data[1], data[2], label, etc.)
before inserting them into the DOM, or use textContent instead of innerHTML
where HTML formatting is not required.

In `@emhttp/plugins/dynamix/include/TaskCommand.php`:
- Around line 35-51: The abort case (and similarly the dismiss case) currently
calls die() without returning any response body, while other actions like create
and list return JSON responses. Replace the die() call at the end of the abort
case with a JSON response using die(json_encode(['ok' => true])) to provide a
consistent JSON acknowledgment across all task command actions. Apply the same
change to the dismiss case for consistency throughout the TaskCommand.php file.
- Around line 38-44: The PID validation in the condition `if
($task['status']==='running' && $task['pid'] > 1)` relies on PHP's implicit type
juggling since $task['pid'] is a string from exec() output. To make this more
explicit and robust, add explicit numeric validation before the kill operation.
Replace or enhance the condition to explicitly check that $task['pid'] is
numeric using is_numeric() or ctype_digit() before proceeding with the
exec('kill '.escapeshellarg($task['pid'])) call, ensuring the PID is a valid
numeric value and not an empty string or non-numeric value from a failed exec.

In `@emhttp/plugins/dynamix/include/TaskQueue.php`:
- Around line 147-177: The task_create function has a race condition where
concurrent requests could violate the "at most one running task per type"
invariant between the deduplication check (inside the loop checking task_list)
and the task_launch call. To fix this, wrap the entire deduplication check, task
creation, and task launch logic (from the initial in_array check through the
task_launch call and task_publish call) in a file lock using flock on a lock
file specific to the task type stored in the task directory. Acquire an
exclusive lock before entering the critical section and release it in a finally
block to ensure the lock is always released, maintaining atomicity for the
create-and-launch sequence.

In `@emhttp/plugins/dynamix/nchan/tasks`:
- Around line 32-37: The task_log_has_error function uses a simple strpos search
for the literal string _ERROR_ which can cause false positives if this string
appears in legitimate task output. To fix this, make the error marker detection
more robust by either creating a more unique/distinctive error marker string
(e.g., wrapping it with special delimiters) or by checking for it only in
specific positions or formats such as standalone messages in RS-delimited
format. Update the strpos check in task_log_has_error to use this more specific
detection logic instead of searching for the bare _ERROR_ string anywhere in the
log tail output.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a46bcf23-fb41-4180-a1a2-3ff7d615eee1

📥 Commits

Reviewing files that changed from the base of the PR and between cc6a800 and 0f86cab.

📒 Files selected for processing (9)
  • emhttp/plugins/dynamix/include/DefaultPageLayout.php
  • emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php
  • emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php
  • emhttp/plugins/dynamix/include/DefaultPageLayout/MiscElementsBottom.php
  • emhttp/plugins/dynamix/include/TaskCommand.php
  • emhttp/plugins/dynamix/include/TaskQueue.php
  • emhttp/plugins/dynamix/include/publish.php
  • emhttp/plugins/dynamix/nchan/tasks
  • emhttp/plugins/dynamix/styles/default-base.css

Comment thread emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php
Comment thread emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php
Comment thread emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php
Comment thread emhttp/plugins/dynamix/include/TaskQueue.php
Comment thread emhttp/plugins/dynamix/nchan/tasks Outdated
Comment thread emhttp/plugins/dynamix/styles/default-base.css

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@emhttp/plugins/dynamix/nchan/tasks`:
- Around line 62-67: The active check in the loop counts both running and queued
tasks as work, but the actual task processing only advances `$freed` types,
meaning queued-only tasks never get launched. Add a recovery pass before the `if
(!$active) break;` statement that detects when queued tasks exist without
corresponding running tasks, and triggers processing of those queued tasks to
ensure the daemon does not sleep indefinitely when restarted with pending queue
work.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 742983b2-c4a7-447b-a0e8-cfa64f81f836

📥 Commits

Reviewing files that changed from the base of the PR and between ed431a5 and b6a1e9f.

📒 Files selected for processing (2)
  • emhttp/plugins/dynamix/include/TaskQueue.php
  • emhttp/plugins/dynamix/nchan/tasks

Comment thread emhttp/plugins/dynamix/nchan/tasks
@elibosley

Copy link
Copy Markdown
Member Author

CodeRabbit fix loop — two nitpicks intentionally not applied

The other 10 review items (6 inline + 1 follow-up thread + 3 nitpicks) are addressed in the incoming commit. Two review-body nitpicks are declined, with reasons:

1. TaskCommand.php (35-51) — return JSON {"ok":true} from abort/dismiss.
Declined. The client never reads these responses (as the review itself notes), and the canonical success signal is the authoritative tasks list broadcast on the tasks nchan channel — every mutation calls task_publish(). Adding an unused JSON acknowledgment introduces a second, speculative success contract with no consumer. Keeping one canonical signal is intentional.

2. BodyInlineJS.php (126-177) — escape nchan message data in renderMessage().
Declined. renderMessage() implements a deliberate HTML/span rendering protocol: the addToID path injects structural markup (<span id="...">...<span class="content">...</span><span class="progress-...">) that the live progress/addToID updates then target by id/class. Blanket-escaping data[1]/data[2] would render that markup as text and break the live progress display. The data originates from trusted server-side processes (docker pull / plugin install / vm action output) published over a server-controlled channel — not untrusted client input — so this preserved legacy behavior is the correct contract here. The genuinely user/plugin-controlled value, task.title, is now escaped (separate finding, fixed).

elibosley and others added 9 commits June 19, 2026 10:37
…round recall

Replace the single-backgrounded-task / single-banner model with a backend-owned
task queue shared across subsystems (plugins/Docker/VM) and all clients.

- New TaskQueue.php store + scheduler: one JSON file per task under
  /var/local/emhttp/tasks, one-running-per-type invariant so the shared
  /sub/{plugins,docker,vmaction} channels never interleave; extra same-type
  ops are queued and auto-advanced.
- New TaskCommand.php endpoint (create/abort/dismiss/log/list); every mutation
  rebroadcasts the list on /sub/tasks.
- New nchan/tasks daemon (mirrors nchan/file_manager): watches pids, marks
  done/error, advances the queue, exits when idle.
- publish.php: env-var-gated (NCHAN_TASK) per-task log tee for foreground replay;
  no-op on every other publish path.
- DefaultPageLayout/Head/BodyInlineJS: /sub/tasks subscriber, router/renderer
  split of the live handlers, task tray; openPlugin/openDocker/openVMAction keep
  their signatures.
- Tray DOM (#opTray) + theme-aware CSS.

Validated with php -l, node --check, and a sandboxed store-logic test. Not yet
runtime-tested on a live Unraid box (nchan pub/sub, real process launch).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The background-operation task tray was fixed at bottom:1rem with z-index
999, but #footer is position:fixed bottom:0 with z-index 10000, so the
tray was painted behind and clipped by the footer in the bottom-right
corner. Raise the tray's bottom offset above the footer using the same
media queries that make the footer fixed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
createTask() called foregroundTask(res.id) immediately on the AJAX
response, but foregroundTask() looks the task up in taskList, which is
only populated asynchronously by the /sub/tasks websocket broadcast.
When the broadcast lands after the AJAX response, taskById() returns null
and foregroundTask() bails, so no modal opens even though the command
runs and its output is captured server-side. This made actions like the
plugin 'Check For Updates' appear to do nothing.

Seed an optimistic taskList entry from the data we already have so the
modal foregrounds immediately; onTaskListUpdate() reconciles it when the
authoritative broadcast arrives.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
NchanSubscriber.start()/stop() throw if called when already running / not
running ('Can't stop NchanSubscriber, it's not running.'). stopAllTypeChannels()
called .stop() on all three type channels unconditionally, and foregroundTask()
runs it at the top before any channel is started, so the uncaught throw aborted
the function before the modal opened. Wrap start/stop in nchanStart()/nchanStop()
helpers that check the .running flag (with a try/catch safety net) and route the
type-channel transitions through them.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion

Finished tasks previously only left the tray on manual dismissal or a
1-day prune that ran solely at daemon startup, so completed tasks piled
up and users cleared them one by one.

- Successful tasks now auto-expire 30s after they finish (TASK_DONE_TTL);
  the scheduler daemon lingers (1s cadence) while any success is pending
  expiry so it can prune and re-broadcast, then exits as before.
- Failures persist (TASK_ERROR_TTL, ~1 day) so errors aren't swept away
  before they're noticed.
- Add a 'Clear finished' tray header action (TaskCommand 'clear' ->
  task_clear_finished) shown when more than one finished task is present.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per review, the manual 'Clear finished' tray action is enough; the 30s
server-side auto-expiry and the daemon lingering to prune/re-broadcast
add complexity we don't need. Restore the scheduler daemon and the
done/error prune TTL to their original behavior (1-day prune on daemon
startup) and keep only task_clear_finished() backing the tray button.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The bomb icon read as overly aggressive for aborting a running task;
fa-stop-circle conveys 'stop' more calmly while keeping the meaning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A running task only showed a small spinning icon. Add a CSS-only
indeterminate bar sweeping along the bottom of the running card so it
clearly reads as still active; respects prefers-reduced-motion.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the indeterminate progress bar in favor of a simple loader: running
tasks now use the fa-circle-o-notch spinner (clearer 'loading' read than
the refresh arrows). Keeps the running indicator minimal.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@elibosley elibosley force-pushed the feat/backend-task-queue branch from d54a2d6 to 9d016d8 Compare June 19, 2026 14:37
elibosley added a commit that referenced this pull request Jun 19, 2026
Generic progress headings ("Updating the container", "Install Plugin")
don't say what is being acted on, which is ambiguous when several
operations run at once. The target is already in scope at each call
site, so fold it into the title that openDocker/openPlugin render:

- docker update:          "Update container: <name>"
- docker update all:      "Updating all Containers (N)"
- plugin install:         "Install Plugin: <plugin>"   (Apps, Tailscale)
- language install:       "Install Language: <pack>"

Names that aren't charset-constrained (plugin/language files) are reduced
to a sanitized basename slug before display, since the swal heading is
rendered with html:true. Container names rely on Docker's naming charset;
counts are numeric. Stays out of HeadInlineJS so it does not conflict with
the backend task-queue work in #2665.

OS-460

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
elibosley added a commit that referenced this pull request Jun 19, 2026
The foreground task modal drove its single button off an inverted per-type
`button` flag. For docker/vmaction (default button=0) this hid the Close
button for the entire running phase AND left the confirm button disabled,
which surfaced SweetAlert's `.la-ball-fall` loader (the "bouncing green
dots") with no way to background the operation.

Drive the modal by task status instead, uniformly for all types:

- running  -> a top-corner minimize control backgrounds the task (it keeps
              running under the daemon and stays in the tray); no disabled
              button, so the bouncing-dots loader never appears. The
              spinning title icon remains the in-progress indicator.
- done/err -> a single primary "Dismiss" button that fires the reload
              callback and clears the task from the tray.

Esc while running now backgrounds (instead of deleting) since the
done-callback checks live status. No caller passes the `button` flag, so
no real behavior contract changes; `task.button` is now inert (follow-up:
drop it from the task record + open* signatures).

Stacked on the backend task queue (#2665).

OS-461

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
elibosley and others added 4 commits June 22, 2026 19:07
Apply robot review feedback for the backend task queue:

- TaskQueue.php: escape the full bash -c payload with escapeshellarg() to
  close a single-quote shell-injection vector while preserving multi-arg
  word splitting; serialize create->launch per type with flock to keep the
  one-running-task-per-type invariant under concurrent requests.
- nchan/tasks: require numeric pid before the /proc liveness check;
  recover queued-only tasks so the daemon never sleeps with pending work;
  match _ERROR_ as a discrete RS-delimited record (read tail in PHP).
- TaskCommand.php: validate pid is numeric before kill.
- BodyInlineJS.php: escape task.title in the swal title and task ids in the
  tray onclick handlers.
- HeadInlineJS.php: surface a createTask() failure via .fail().
- default-base.css: add min-width:0 so .op-title ellipsizes reliably.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Completion was registered only by the `tasks` scheduler daemon noticing
the launched PID disappear, which it can miss on PID reuse or a
daemon-restart race (TOCTOU in task_daemon_start) — leaving a finished
task stuck "in progress" until the next page-load nchan sweep.

Have the launched command stamp its own result on exit: task_launch wraps
the payload to capture the command's exit code and invoke task_complete,
which marks the task done/error, advances the queue and broadcasts. This
makes completion authoritative at the source. task_log_has_error moves
into TaskQueue.php so the stamp and the daemon share it, and task_advance
now takes the per-type lock so two advancers can't double-launch the next
queued task. The daemon stays a fallback for hard-killed processes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lds #2669, OS-461)

Squash of feat/task-modal-buttons (PR #2669, 31 iteration commits) into the
consolidated task-queue PR. Adds backgroundable running operations with a
corrected modal button set, drives the plugin "Upgrading" button state from
the server-side task queue, and a round of task-modal/tray styling
(state strip, dismiss placement, mobile grouped-notification carousel).

Touches only the tray/modal UI (BodyInlineJS, HeadInlineJS, default-base.css)
and the Plugins page (Plugins.page, PluginHelpers.php); no changes to the
core queue/daemon files.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Generic progress headings ("Updating the container", "Install Plugin")
don't say what is being acted on, which is ambiguous when several
operations run at once. The target is already in scope at each call
site, so fold it into the title that openDocker/openPlugin render:

- docker update:          "Update container: <name>"
- docker update all:      "Updating all Containers (N)"
- plugin install:         "Install Plugin: <plugin>"   (Apps, Tailscale)
- language install:       "Install Language: <pack>"

Names that aren't charset-constrained (plugin/language files) are reduced
to a sanitized basename slug before display, since the swal heading is
rendered with html:true. Container names rely on Docker's naming charset;
counts are numeric. Stays out of HeadInlineJS so it does not conflict with
the backend task-queue work in #2665.

OS-460

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant