fix(plugin): serialize host-wide Plugin Manager mutations - #2699
Closed
Pujit Mehrotra (pujitm) wants to merge 13 commits into
Closed
fix(plugin): serialize host-wide Plugin Manager mutations#2699Pujit Mehrotra (pujitm) wants to merge 13 commits into
Pujit Mehrotra (pujitm) wants to merge 13 commits into
Conversation
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds host-wide plugin-operation locking, generation-based update artifacts, scoped snapshots, atomic persistence, integration changes, concurrency tests, and a dedicated GitHub Actions workflow. ChangesPlugin operation coordination
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pujit Mehrotra (pujitm)
force-pushed
the
codex/plugin-manager-host-lock
branch
from
July 18, 2026 04:47
4b096b0 to
a9ecb49
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.plugin.manager/include/PluginOperationLock.php`:
- Around line 407-418: Update plugin_manager_write_plugin_check_state to
preserve the locked handle’s inode while persisting state through a sibling
temporary file: write the complete contents, verify the write, flush and fsync
it, then atomically rename it over the state file. Remove the direct
rewind/ftruncate/write flow and ensure failures clean up the staged file without
leaving empty or partial state.
- Around line 987-1025: Update the rollback cleanup in the Throwable handler
around $backup_path so a backup is not unlinked when restoring it over $target
fails. Preserve $backup_path when the restore rename fails, and include its path
in $rollback_error; only clear and clean up the backup after a successful
restoration and hash validation.
In `@emhttp/plugins/dynamix.plugin.manager/scripts/plugin`:
- Around line 661-662: The check operation is currently serialized for its
entire duration, unnecessarily holding the host-wide lock during network
activity. In emhttp/plugins/dynamix.plugin.manager/scripts/plugin lines 661-662,
replace whole-command plugin_manager_serialize_operation usage for check with
phase-scoped supervised locking; in lines 822-870, perform reservation and
download before acquiring the lock, then lock around revalidation, publication,
hooks, and finalization. In
tests/dynamix.plugin.manager/plugin_operation_lock_test.php lines 473-475,
replace the whole-command lock assertion with a contention test proving download
proceeds while publication waits.
In `@emhttp/plugins/dynamix.plugin.manager/scripts/plugin-operation-lock`:
- Around line 95-98: Update the command execution and cleanup flow around "$@"
and release_scope_after_members_exit to explicitly track and supervise the child
PID. Trap TERM, INT, and HUP, forward each received signal to the child,
continue waiting for it and completing lease cleanup, then release the lock and
exit with the child’s status.
🪄 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: 518e43df-39c3-4962-b85b-2e530c606967
📒 Files selected for processing (9)
.github/workflows/plugin-manager-tests.ymlemhttp/plugins/dynamix.plugin.manager/Plugins.pageemhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.phpemhttp/plugins/dynamix.plugin.manager/include/PluginOperationLock.phpemhttp/plugins/dynamix.plugin.manager/scripts/PluginAPI.phpemhttp/plugins/dynamix.plugin.manager/scripts/pluginemhttp/plugins/dynamix.plugin.manager/scripts/plugin-operation-locktests/dynamix.plugin.manager/plugin_operation_lock_test.phptests/dynamix.plugin.manager/run.sh
Comment on lines
+987
to
+1025
| } catch (Throwable $error) { | ||
| $rollback_error = null; | ||
| if ($target_committed) { | ||
| if ($backup_path !== null) { | ||
| if (!@rename($backup_path, $target)) { | ||
| $rollback_error = 'unable to restore prior update target'; | ||
| } else { | ||
| $backup_path = null; | ||
| $restored_hash = @hash_file('sha256', $target); | ||
| if (!is_string($restored_hash) || | ||
| !hash_equals((string)$previous_hash, $restored_hash)) { | ||
| $rollback_error = 'restored update target hash mismatch'; | ||
| } | ||
| } | ||
| } elseif (!@unlink($target) && file_exists($target)) { | ||
| $rollback_error = 'unable to remove newly created update target'; | ||
| } | ||
| } | ||
|
|
||
| if ($link_committed && $previous_link_target !== null) { | ||
| try { | ||
| $restore_link = plugin_manager_create_sibling_symlink( | ||
| $installed_link, | ||
| $previous_link_target | ||
| ); | ||
| if (!@rename($restore_link, $installed_link)) { | ||
| @unlink($restore_link); | ||
| throw new RuntimeException('unable to restore prior installed link'); | ||
| } | ||
| } catch (Throwable $link_error) { | ||
| $rollback_error = $rollback_error === null | ||
| ? $link_error->getMessage() | ||
| : "$rollback_error; {$link_error->getMessage()}"; | ||
| } | ||
| } | ||
|
|
||
| if ($stage_path !== null) @unlink($stage_path); | ||
| if ($backup_path !== null) @unlink($backup_path); | ||
| if ($temporary_link !== null) @unlink($temporary_link); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not delete the only rollback copy after restoration fails.
When Line 991 cannot rename the backup over the target, $backup_path remains set, and Line 1024 immediately unlinks that verified prior version. Preserve the backup and include its path in the rollback error so recovery remains possible.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 1000-1000: Avoid unsafe call to unlink
Context: unlink($target)
Note: [CWE-73] External Control of File Name or Path.
(avoid-unlink)
[info] 1012-1012: Avoid unsafe call to unlink
Context: unlink($restore_link)
Note: [CWE-73] External Control of File Name or Path.
(avoid-unlink)
[info] 1022-1022: Avoid unsafe call to unlink
Context: unlink($stage_path)
Note: [CWE-73] External Control of File Name or Path.
(avoid-unlink)
[info] 1023-1023: Avoid unsafe call to unlink
Context: unlink($backup_path)
Note: [CWE-73] External Control of File Name or Path.
(avoid-unlink)
[info] 1024-1024: Avoid unsafe call to unlink
Context: unlink($temporary_link)
Note: [CWE-73] External Control of File Name or Path.
(avoid-unlink)
🤖 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.plugin.manager/include/PluginOperationLock.php` around
lines 987 - 1025, Update the rollback cleanup in the Throwable handler around
$backup_path so a backup is not unlinked when restoring it over $target fails.
Preserve $backup_path when the restore rename fails, and include its path in
$rollback_error; only clear and clean up the backup after a successful
restoration and hash validation.
Contributor
There was a problem hiding this comment.
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.plugin.manager/scripts/PluginAPI.php`:
- Line 60: Update the download handling condition around download_url() to
verify that the temporary artifact was fully written before treating the
download as successful. Detect empty, partial, or otherwise invalid output in
addition to download_url() returning false, and preserve the existing failure
path when validation fails.
🪄 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: 761c8b6e-9d0c-4647-bb45-69a70fbe5008
📒 Files selected for processing (9)
.github/workflows/plugin-manager-tests.ymlemhttp/plugins/dynamix.plugin.manager/Plugins.pageemhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.phpemhttp/plugins/dynamix.plugin.manager/include/PluginOperationLock.phpemhttp/plugins/dynamix.plugin.manager/scripts/PluginAPI.phpemhttp/plugins/dynamix.plugin.manager/scripts/pluginemhttp/plugins/dynamix.plugin.manager/scripts/plugin-operation-locktests/dynamix.plugin.manager/plugin_operation_lock_test.phptests/dynamix.plugin.manager/run.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- emhttp/plugins/dynamix.plugin.manager/Plugins.page
- emhttp/plugins/dynamix.plugin.manager/scripts/plugin-operation-lock
- emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php
- .github/workflows/plugin-manager-tests.yml
- emhttp/plugins/dynamix.plugin.manager/scripts/plugin
- tests/dynamix.plugin.manager/run.sh
Pujit Mehrotra (pujitm)
force-pushed
the
codex/plugin-manager-host-lock
branch
from
July 18, 2026 12:25
a9ecb49 to
bf0f09d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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.plugin.manager/include/PluginOperationLock.php`:
- Around line 1653-1678: Update the artifact-policy execution flow around
proc_open, the pipes[1] read loop, and proc_close to enforce a finite deadline
using nonblocking output reads. If the policy exceeds the deadline, terminate
the process, close its pipes, and reap it before releasing the operation lock;
preserve bounded output capture and normal completion behavior.
- Around line 747-759: Update plugin_manager_with_operation_lock() to detect an
already-live inherited supervisor ownership or an active local nesting level
before opening and acquiring another descriptor. Reuse the existing ownership
state for nested calls, increment and decrement the local nesting counter around
the operation, and only unlock and close the descriptor when the outermost owner
exits.
In `@emhttp/plugins/dynamix.plugin.manager/scripts/plugin`:
- Around line 712-725: Move supervised operation-lock acquisition in
emhttp/plugins/dynamix.plugin.manager/scripts/plugin:712-725 out of the initial
branch path so branch-candidate download occurs privately before locking. Update
the branch flow at emhttp/plugins/dynamix.plugin.manager/scripts/plugin:797-802
to acquire the lock only after download, then revalidate source identity, run
hooks, select exact bytes, and publish under the lock. Update
tests/dynamix.plugin.manager/plugin_operation_lock_test.php:4085-4101 to verify
download completes while the lock holder remains active and publication waits.
In `@emhttp/plugins/dynamix.plugin.manager/scripts/plugin-operation-lock`:
- Around line 216-225: Update the ShellCheck suppression comments for both
trap-only callbacks, including record_child_signal and the callback at the other
referenced location, from SC2329 to SC2317 so CI suppresses the diagnostics
actually reported for these functions.
In `@emhttp/plugins/dynamix.plugin.manager/scripts/PluginAPI.php`:
- Around line 109-128: The plugin check flow around
plugin_manager_finalize_plugin_check_artifact must stage the primary artifact,
changes sidecar, and alerts sidecar before making any committed updates, then
commit all outputs as one transaction. On any write or removal failure, roll
back every affected path, including artifacts already committed during this
attempt, so no sidecars remain from a failed check.
In `@emhttp/plugins/dynamix.plugin.manager/scripts/showchanges`:
- Around line 26-33: Update the $valid prefix list used by the foreach check to
terminate the /boot/previous directory entry with a trailing slash, ensuring
paths such as /boot/previous-copy are rejected while descendants of
/boot/previous remain accepted.
In `@tests/dynamix.plugin.manager/plugin_operation_lock_test.php`:
- Around line 4085-4101: Update the branch-check lock test around
test_start_process and test_finish_process to emit a fake-download completion
marker, then assert that marker appears before the holder process exits while
the publication result remains blocked until afterward. Keep the existing
serialization assertion and distinguish download progress from final publication
timing.
In `@tests/dynamix.plugin.manager/production_run_alias_test.php`:
- Around line 78-108: Gate the production-path mutations in
production_run_alias_test.php behind an explicit disposable-container
environment opt-in before touching /etc, /boot, /var/log, or /tmp/plugins.
Update both Docker invocation branches in run.sh—the local-host branch and
Docker-only fallback—to pass the same opt-in variable so the test continues
running in disposable containers.
🪄 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: 6ce75a54-1f4d-4945-a6a6-23144be905ca
📒 Files selected for processing (15)
.github/workflows/plugin-manager-tests.ymlemhttp/plugins/dynamix.plugin.manager/Plugins.pageemhttp/plugins/dynamix.plugin.manager/include/PluginAttributes.phpemhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.phpemhttp/plugins/dynamix.plugin.manager/include/PluginOperationLock.phpemhttp/plugins/dynamix.plugin.manager/include/ShowChanges.phpemhttp/plugins/dynamix.plugin.manager/include/ShowPlugins.phpemhttp/plugins/dynamix.plugin.manager/scripts/PluginAPI.phpemhttp/plugins/dynamix.plugin.manager/scripts/pluginemhttp/plugins/dynamix.plugin.manager/scripts/plugin-operation-lockemhttp/plugins/dynamix.plugin.manager/scripts/plugin_rmemhttp/plugins/dynamix.plugin.manager/scripts/showchangestests/dynamix.plugin.manager/plugin_operation_lock_test.phptests/dynamix.plugin.manager/production_run_alias_test.phptests/dynamix.plugin.manager/run.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/plugin-manager-tests.yml
- emhttp/plugins/dynamix.plugin.manager/Plugins.page
Comment on lines
+747
to
+759
| function plugin_manager_with_operation_lock(callable $operation): mixed { | ||
| $handle = plugin_manager_open_operation_lock(); | ||
| if (!@flock($handle, LOCK_EX)) { | ||
| fclose($handle); | ||
| throw new RuntimeException('Unable to acquire Plugin Manager operation lock'); | ||
| } | ||
|
|
||
| try { | ||
| return $operation(); | ||
| } finally { | ||
| @flock($handle, LOCK_UN); | ||
| fclose($handle); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the in-process mutex wrapper reentrant.
A nested call—or a call from an operation already owned by the supervisor—opens another descriptor and blocks on its own host-wide lock. Check inherited live ownership and track local nesting before reacquiring.
Proposed fix
function plugin_manager_with_operation_lock(callable $operation): mixed {
+ static $depth = 0;
+ if ($depth > 0 || plugin_manager_operation_has_live_owner()) {
+ return $operation();
+ }
+
$handle = plugin_manager_open_operation_lock();
if (!`@flock`($handle, LOCK_EX)) {
fclose($handle);
throw new RuntimeException('Unable to acquire Plugin Manager operation lock');
}
+ $depth++;
try {
return $operation();
} finally {
+ $depth--;
`@flock`($handle, LOCK_UN);
fclose($handle);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function plugin_manager_with_operation_lock(callable $operation): mixed { | |
| $handle = plugin_manager_open_operation_lock(); | |
| if (!@flock($handle, LOCK_EX)) { | |
| fclose($handle); | |
| throw new RuntimeException('Unable to acquire Plugin Manager operation lock'); | |
| } | |
| try { | |
| return $operation(); | |
| } finally { | |
| @flock($handle, LOCK_UN); | |
| fclose($handle); | |
| } | |
| function plugin_manager_with_operation_lock(callable $operation): mixed { | |
| static $depth = 0; | |
| if ($depth > 0 || plugin_manager_operation_has_live_owner()) { | |
| return $operation(); | |
| } | |
| $handle = plugin_manager_open_operation_lock(); | |
| if (!`@flock`($handle, LOCK_EX)) { | |
| fclose($handle); | |
| throw new RuntimeException('Unable to acquire Plugin Manager operation lock'); | |
| } | |
| $depth++; | |
| try { | |
| return $operation(); | |
| } finally { | |
| $depth--; | |
| `@flock`($handle, LOCK_UN); | |
| fclose($handle); | |
| } | |
| } |
🤖 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.plugin.manager/include/PluginOperationLock.php` around
lines 747 - 759, Update plugin_manager_with_operation_lock() to detect an
already-live inherited supervisor ownership or an active local nesting level
before opening and acquiring another descriptor. Reuse the existing ownership
state for nested calls, increment and decrement the local nesting counter around
the operation, and only unlock and close the descriptor when the outermost owner
exits.
Comment on lines
+1653
to
+1678
| $process = @proc_open( | ||
| [$policy, $method, $snapshot, $plugin], | ||
| [ | ||
| 0 => ['file', '/dev/null', 'r'], | ||
| 1 => ['pipe', 'w'], | ||
| 2 => ['redirect', 1] | ||
| ], | ||
| $pipes, | ||
| null, | ||
| null, | ||
| ['bypass_shell' => true] | ||
| ); | ||
| if (!is_resource($process)) { | ||
| throw new RuntimeException('Unable to start Plugin Manager artifact policy'); | ||
| } | ||
|
|
||
| $output = ''; | ||
| while (!feof($pipes[1])) { | ||
| $chunk = fread($pipes[1], 8192); | ||
| if ($chunk === false) break; | ||
| if (strlen($output) < 4096) { | ||
| $output .= substr($chunk, 0, 4096 - strlen($output)); | ||
| } | ||
| } | ||
| fclose($pipes[1]); | ||
| $status = proc_close($process); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound artifact-policy execution time.
The blocking pipe read and proc_close() have no deadline. A hung policy—or a forked child retaining stdout—holds the machine-wide mutation lock indefinitely, blocking every install, update, and removal.
Use nonblocking reads with a deadline, then terminate and reap the policy process on timeout.
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 1653-1664: Dynamic command passed to a shell execution function. Use escapeshellarg() and escapeshellcmd() to sanitize input, or avoid shell execution entirely.
(coderabbit.command-injection.php-shell-exec)
🤖 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.plugin.manager/include/PluginOperationLock.php` around
lines 1653 - 1678, Update the artifact-policy execution flow around proc_open,
the pipes[1] read loop, and proc_close to enforce a finite deadline using
nonblocking output reads. If the policy exceeds the deadline, terminate the
process, close its pipes, and reap it before releasing the operation lock;
preserve bounded output capture and normal completion behavior.
Comment on lines
+712
to
+725
| if ($method != 'check') { | ||
| try { | ||
| $lock_command = plugin_manager_operation_lock_command($method, $script, $argv); | ||
| } catch (Throwable $lock_error) { | ||
| fwrite(STDERR, "plugin: unable to acquire operation lock: {$lock_error->getMessage()}\n"); | ||
| done(1); | ||
| } | ||
| if ($lock_command !== null) { | ||
| if ($nchan) putenv(PLUGIN_MANAGER_NCHAN_CHILD_ENV.'=1'); | ||
| passthru($lock_command, $status); | ||
| if ($nchan) putenv(PLUGIN_MANAGER_NCHAN_CHILD_ENV); | ||
| done($status); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep OS branch-check network I/O outside the host-wide lock. The current branch path serializes before downloading, and its test does not distinguish that behavior from phase-scoped publication.
emhttp/plugins/dynamix.plugin.manager/scripts/plugin#L712-L725: defer supervised lock acquisition until the branch candidate has been downloaded privately.emhttp/plugins/dynamix.plugin.manager/scripts/plugin#L797-L802: download first, then lock to revalidate source identity, run hooks, select exact bytes, and publish.tests/dynamix.plugin.manager/plugin_operation_lock_test.php#L4085-L4101: assert download completion while the holder remains active and publication waits.
📍 Affects 2 files
emhttp/plugins/dynamix.plugin.manager/scripts/plugin#L712-L725(this comment)emhttp/plugins/dynamix.plugin.manager/scripts/plugin#L797-L802tests/dynamix.plugin.manager/plugin_operation_lock_test.php#L4085-L4101
🤖 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.plugin.manager/scripts/plugin` around lines 712 - 725,
Move supervised operation-lock acquisition in
emhttp/plugins/dynamix.plugin.manager/scripts/plugin:712-725 out of the initial
branch path so branch-candidate download occurs privately before locking. Update
the branch flow at emhttp/plugins/dynamix.plugin.manager/scripts/plugin:797-802
to acquire the lock only after download, then revalidate source identity, run
hooks, select exact bytes, and publish under the lock. Update
tests/dynamix.plugin.manager/plugin_operation_lock_test.php:4085-4101 to verify
download completes while the lock holder remains active and publication waits.
Comment on lines
+109
to
+128
| if ( !plugin_manager_finalize_plugin_check_artifact($plugin,$generation,$latest) ) return null; | ||
| $changes_path = "/tmp/plugins/".pathinfo($plugin, PATHINFO_FILENAME).".txt"; | ||
| if ( $changes ) { | ||
| if ( !plugin_manager_write_shared_artifact($changes_path,$changes) ) { | ||
| throw new RuntimeException("Unable to publish plugin changes"); | ||
| } | ||
| } else { | ||
| if ( !plugin_manager_remove_shared_artifact($changes_path) ) { | ||
| throw new RuntimeException("Unable to remove plugin changes"); | ||
| } | ||
| } | ||
| if ( $alerts ) { | ||
| if ( !plugin_manager_write_shared_artifact('/tmp/plugins/my_alerts.txt',$alerts) ) { | ||
| throw new RuntimeException("Unable to publish plugin alerts"); | ||
| } | ||
| } else { | ||
| if ( !plugin_manager_remove_shared_artifact('/tmp/plugins/my_alerts.txt') ) { | ||
| throw new RuntimeException("Unable to remove plugin alerts"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Publish the check artifact and sidecars as one transaction.
The primary artifact is finalized before changes and alerts are committed individually. If a later write/removal fails, the wrapper invalidates only the primary generation, leaving already-updated sidecars from a failed check.
Stage all outputs first, then commit them with rollback of every affected path on failure.
🤖 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.plugin.manager/scripts/PluginAPI.php` around lines 109
- 128, The plugin check flow around
plugin_manager_finalize_plugin_check_artifact must stage the primary artifact,
changes sidecar, and alerts sidecar before making any committed updates, then
commit all outputs as one transaction. On any write or removal failure, roll
back every affected path, including artifacts already committed during this
attempt, so no sidecars remain from a failed check.
Comment on lines
+26
to
33
| $valid = [ | ||
| '/var/tmp/', | ||
| '/tmp/plugins/', | ||
| '/boot/previous' | ||
| ]; | ||
| $good = false; | ||
|
|
||
| foreach ($valid as $check) if (strncmp($file,$check,strlen($check))===0) $good = true; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Terminate the /boot/previous directory prefix.
The current prefix check also accepts sibling paths such as /boot/previous-copy/file.txt. Add the trailing slash so only descendants of the intended directory pass.
Proposed fix
$valid = [
'/var/tmp/',
'/tmp/plugins/',
- '/boot/previous'
+ '/boot/previous/'
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $valid = [ | |
| '/var/tmp/', | |
| '/tmp/plugins/', | |
| '/boot/previous' | |
| ]; | |
| $good = false; | |
| foreach ($valid as $check) if (strncmp($file,$check,strlen($check))===0) $good = true; | |
| $valid = [ | |
| '/var/tmp/', | |
| '/tmp/plugins/', | |
| '/boot/previous/' | |
| ]; | |
| $good = false; | |
| foreach ($valid as $check) if (strncmp($file,$check,strlen($check))===0) $good = true; |
🤖 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.plugin.manager/scripts/showchanges` around lines 26 -
33, Update the $valid prefix list used by the foreach check to terminate the
/boot/previous directory entry with a trailing slash, ensuring paths such as
/boot/previous-copy are rejected while descendants of /boot/previous remain
accepted.
Comment on lines
+4085
to
+4101
| $holder = test_start_process([ | ||
| '--critical', 'install', $directory, 'branch-holder', '300', '0' | ||
| ]); | ||
| test_wait_for( | ||
| fn() => in_array('enter branch-holder', test_events($directory), true), | ||
| 2.0, | ||
| 'Branch-check holder never acquired the host lock' | ||
| ); | ||
| $branch_result = test_finish_process( | ||
| test_start_command([$cli_wrapper, 'branchcheck', $branch_source, 'next']) | ||
| ); | ||
| $holder_result = test_finish_process($holder); | ||
| test_assert($holder_result[0] === 0, "Branch-check holder failed: {$holder_result[2]}"); | ||
| test_assert( | ||
| $branch_result[0] === 0 && $branch_result[3] >= 0.2, | ||
| "Real branch check did not serialize behind the host lock: {$branch_result[2]}" | ||
| ); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Verify branch downloads progress while the lock is busy.
Elapsed time alone passes both whole-command and phase-scoped implementations. Add a fake-download marker and assert download completion before the holder exits, while publication still waits.
🤖 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 `@tests/dynamix.plugin.manager/plugin_operation_lock_test.php` around lines
4085 - 4101, Update the branch-check lock test around test_start_process and
test_finish_process to emit a fake-download completion marker, then assert that
marker appears before the holder process exits while the publication result
remains blocked until afterward. Keep the existing serialization assertion and
distinguish download progress from final publication timing.
Comment on lines
+78
to
+108
| $root = '/tmp/plugin-production-run-alias'; | ||
| $plugin = 'production-run-alias.plg'; | ||
| $installed = "/boot/config/plugins/$plugin"; | ||
| $latest_source = "$root/$plugin"; | ||
| $latest = "/tmp/plugins/$plugin"; | ||
| $wrapper = "$root/plugin"; | ||
| $showchanges_bootstrap = "$root/showchanges-bootstrap.php"; | ||
| $showchanges_script = | ||
| "$repo/emhttp/plugins/dynamix.plugin.manager/scripts/showchanges"; | ||
| $fake_bin = "$root/bin"; | ||
|
|
||
| @mkdir($root, 0700, true); | ||
| @mkdir($fake_bin, 0700, true); | ||
| @mkdir('/boot/config/plugins', 0770, true); | ||
| @mkdir('/var/log/plugins', 0755, true); | ||
| @mkdir('/tmp/plugins', 0755, true); | ||
| file_put_contents('/etc/unraid-version', "version=\"7.3.0\"\n"); | ||
| file_put_contents( | ||
| $installed, | ||
| '<PLUGIN name="production-run-alias" version="1.0.0" '. | ||
| 'pluginURL="file:///tmp/plugin-production-run-alias/'. | ||
| $plugin.'"></PLUGIN>' | ||
| ); | ||
| file_put_contents( | ||
| $latest_source, | ||
| '<PLUGIN name="production-run-alias" version="2.0.0" '. | ||
| 'pluginURL="file:///tmp/plugin-production-run-alias/'. | ||
| $plugin.'"></PLUGIN>' | ||
| ); | ||
| @unlink("/var/log/plugins/$plugin"); | ||
| symlink($installed, "/var/log/plugins/$plugin"); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Gate all production-path test mutations behind a disposable-container capability. The executable integration test can otherwise overwrite real Unraid state when run directly as root.
tests/dynamix.plugin.manager/production_run_alias_test.php#L78-L108: require an explicit environment opt-in before touching/etc,/boot,/var/log, or/tmp/plugins.tests/dynamix.plugin.manager/run.sh#L10-L14: provide the opt-in to the local-host branch’s Docker invocation.tests/dynamix.plugin.manager/run.sh#L26-L34: provide the same opt-in to the Docker-only fallback.
📍 Affects 2 files
tests/dynamix.plugin.manager/production_run_alias_test.php#L78-L108(this comment)tests/dynamix.plugin.manager/run.sh#L10-L14tests/dynamix.plugin.manager/run.sh#L26-L34
🤖 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 `@tests/dynamix.plugin.manager/production_run_alias_test.php` around lines 78 -
108, Gate the production-path mutations in production_run_alias_test.php behind
an explicit disposable-container environment opt-in before touching /etc, /boot,
/var/log, or /tmp/plugins. Update both Docker invocation branches in run.sh—the
local-host branch and Docker-only fallback—to pass the same opt-in variable so
the test continues running in disposable containers.
🧹 PR Test Plugin Cleaned UpThe test plugin and associated files for this PR have been removed from the preview environment. 🤖 This comment is automatically generated when a PR is closed without merging. |
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
No description provided.