[SimonFair]

Summary

Prevent VM removal from destroying a containing ZFS share dataset, and close the directory handle before removing a VM-owned dataset.

Related to OS-1066.
Fixes #2792.

Why This Exists

Remove VM & Disks checks that the VM folder is empty after deleting its first vdisk. However, zfs list resolves an ordinary directory to its containing dataset. Destroying that dataset can also erase other stopped VMs' disks.

Live QA also found that is_dir_empty() leaves a directory handle open. This makes ZFS report a VM-owned dataset as busy during deletion.

Resolution

Permit zfs destroy only when the canonical directory path matches the dataset mountpoint. Use rmdir() for ordinary directories. Close the emptiness-check handle in finally on both return paths before the caller removes the directory or dataset.

Reviewer Considerations

• Require successful path resolution before comparing paths. Two failed realpath() calls must never authorize destruction.
• Keep dataset destruction non-recursive. Snapshots and child datasets retain ZFS's existing protections.
• The handle fix preserves empty, nonempty, and unreadable-path results.

Behavior Changes

Ordinary VM folders are removed without destroying their containing dataset. Actual dataset mountpoints remain eligible for zfs destroy, after the caller closes its directory handle. Failed lookups stop before deletion. Nonempty folders remain intact.

Implementation Summary

• Guard dataset destruction in Helpers.php.
• Close the directory handle in Libvirt::is_dir_empty().
• Add focused regression tests for the destruction guard and directory-handle lifecycle.

Verification

• Both new regression tests fail against the relevant original implementations and pass with the fixes.
• php tests/helpers_rmdir_test.php — passed.
• php tests/libvirt_directory_test.php — passed.
• PHP syntax checks for both changed production files and test scripts — passed.
• git diff --check — passed.
• Earlier live QAVM validation on Unraid 7.4.0-beta.3.2 used native libvirt and a real mirrored ZFS pool. It confirmed sibling-disk preservation, nonempty-folder preservation, quoted paths, and snapshot protection. It exposed the caller handle leak addressed by the follow-up commit.

Risk

The combined fix passed targeted native PHP/libvirt/ZFS validation. Browser-menu, HTTP-request, and /mnt/user translation coverage remain outside this run. The PR remains in draft pending human review and formal QA approval.

Live QAVM validation of the combined fix

Validated commit 567e326f9a6e9eafcf368a65704aeebd746cbe58 through the published PR plugin 2026.10.21.0841 on Unraid 7.4.0-beta.3.2. Both installed production files exactly matched the PR source hashes.

Using native Libvirt::domain_delete() and a real ZFS mirror on two fresh disposable disks:

• Ordinary-folder removal preserved the containing dataset, the other stopped VM, and its vdisk SHA-256.
• VM-owned dataset removal succeeded in the same PHP process, with zfs destroy returning zero. The handle leak is resolved.
• Nonempty folders and unrelated files remained.
• Snapshots prevented non-recursive dataset destruction.
• Quoted directory paths were removed safely.

The test exited zero with QAVM_ZFS_VALIDATION_PASS. The QA VM was restored and released; live read-back confirmed it was shut off with no data disks attached. No formal Linear QA approval is claimed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9fb6b536-be5e-46ae-9ff7-bbbd56b1a452

📥 Commits

Reviewing files that changed from the base of the PR and between 6f20d25 and 567e326.

📒 Files selected for processing: 2

• emhttp/plugins/dynamix.vm.manager/include/libvirt.php
• tests/libvirt_directory_test.php

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

Walkthrough

my_rmdir now destroys a ZFS dataset only when the requested path matches its resolved mountpoint. Otherwise, it uses rmdir. is_dir_empty() now closes directory handles on all execution paths. Tests cover both behaviors.

Changes

Filesystem safety and cleanup

Layer / File(s)	Summary
Mountpoint-gated ZFS removal emhttp/plugins/dynamix/include/Helpers.php, tests/helpers_rmdir_test.php	my_rmdir queries the dataset and mountpoint together. It destroys the dataset only when resolved paths match. Otherwise, it removes the directory with rmdir. Tests cover lookup and destroy failures, aliases, non-ZFS paths, and cleanup.
Guaranteed directory-handle closure emhttp/plugins/dynamix.vm.manager/include/libvirt.php, tests/libvirt_directory_test.php	is_dir_empty() closes its directory handle in a finally block. Tests cover empty, nonempty, and missing directories.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: High

Possibly related issues

• unraid/webgui#2792 — The change directly addresses destruction of a containing ZFS dataset when the target is a plain directory.

Merge Risk: ⚪ Minimal · up to 567e3

No concrete merge-blocking issue is identified. Complete the planned disposable-pool validation before release.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: preventing VM removal from destroying a containing ZFS dataset.
Linked Issues check		Issue [#2792] requires VM-folder removal to destroy a ZFS dataset only when the folder is that dataset's mountpoint. At the reviewed head, my_rmdir() queries the dataset and mountpoint, compares suc…
Out of Scope Changes check		The changes stay within issue [#2792]. The is_dir_empty() handle closure is required to permit safe removal of a VM-owned dataset after the mountpoint guard authorizes destruction. The added tests d…

Full details: Linked Issues check

Explanation

Issue [#2792] requires VM-folder removal to destroy a ZFS dataset only when the folder is that dataset's mountpoint. At the reviewed head, my_rmdir() queries the dataset and mountpoint, compares successful canonical paths, and uses rmdir() for ordinary directories. Failed lookups do not delete the directory. Dataset destruction remains non-recursive. tests/helpers_rmdir_test.php covers mountpoints, containing datasets, lookup failures, canonicalization failures, nonempty directories, and destroy errors. Libvirt::is_dir_empty() closes its handle before removal, and tests/libvirt_directory_test.php covers empty, nonempty, and missing paths.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue [#2792]. The is_dir_empty() handle closure is required to permit safe removal of a VM-owned dataset after the mountpoint guard authorizes destruction. The added tests directly verify the removal guard and handle lifecycle. No unrelated implementation or test scope is evident.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks the mountpoint bright,
And keeps the parent dataset right.
Empty handles close with care,
Plain directories stay safe there.
Tests hop through each guarded way,
While ZFS keeps the loss at bay.}

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

[github-actions]

🔧 PR Test Plugin Available

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

Version: 2026.10.21.0841
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-2793/webgui-pr-2793.plg

3. Click Install

Alternative: Direct Download

• 📦 Download PLG
• 📦 Download TXZ

⚠️ 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.vm.manager/include/libvirt.php
emhttp/plugins/dynamix/include/Helpers.php

🔄 To Remove:

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

plugin remove webgui-pr-2793

---

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