Fix: snapshot updates for block commit. #2508
Conversation
WalkthroughAdded purge_deleted_snapshots(array &$snaps) and extended refresh_snapshots_database($vm, $delete_used=false) to optionally purge snapshots whose disk backing files are missing; switched snapshot backing keys from device-based to diskid-based and reordered logic so purge runs before NVRAM/metadata cleanup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
🔧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
1980-1989: Key mismatch: backing array uses$diskidbut access uses$disk["device"].The backing array is now keyed by
$diskid(from$disk["@attributes"]["name"]) at line 1980, but line 1985 still accesses it using$disk["device"]. This inconsistency will cause$parentfindto benull, breaking parent snapshot detection.🐛 Proposed fix
- $snaps[$vmsnap]['backing'][$diskid] = $output; - $rev = "r".$diskid; + $snaps[$vmsnap]['backing'][$diskid] = $output; + $rev = "r".$diskid; $reversed = array_reverse($output); $snaps[$vmsnap]['backing'][$rev] = $reversed; } - $parentfind = $snaps[$vmsnap]['backing'][$disk["device"]]; + $parentfind = $snaps[$vmsnap]['backing'][$diskid] ?? []; $parendfileinfo = pathinfo($parentfind[1]);Note: You'll need to preserve
$diskidfrom the last loop iteration or refactor to track the last disk ID explicitly.
🤖 Fix all issues with AI agents
In @emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php:
- Around line 1960-1963: Guard against null $snaps before calling
purge_deleted_snapshots(): when json_decode() can return null, wrap the call so
it only runs if $delete_used is true AND $snaps is an array (e.g. if
($delete_used && is_array($snaps)) purge_deleted_snapshots($snaps);), or
explicitly convert/validate $snaps to an array before calling
purge_deleted_snapshots() so the function's array type hint is never passed
null.
- Around line 1934-1951: The purge_deleted_snapshots function assumes $snaps is
a non-null array and that each $snap contains an iterable 'disks' key; add a
null/type check at the top to return early or cast to an array if $snaps is
null/invalid, and inside the foreach over $snaps ensure isset($snap['disks']) &&
is_array($snap['disks']) before iterating; for malformed entries
(missing/invalid 'disks') skip/unset them, and only access
$disk['source']['@attributes']['file'] after verifying those keys exist before
calling file_exists.
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (2)
2517-2517: Simplify redundant ternary expression.The comparison
$action=="yes"already evaluates to a boolean. The ternary wrapper is unnecessary.♻️ Suggested simplification
- refresh_snapshots_database($vm, $action=="yes" ? true : false); + refresh_snapshots_database($vm, $action === "yes");
2617-2617: Simplify redundant ternary expression (same as blockcommit).♻️ Suggested simplification
- refresh_snapshots_database($vm,$action=="yes" ? true : false); + refresh_snapshots_database($vm, $action === "yes");
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR #2333 by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (3)
get_disk_stats(1225-1284)get_domain_by_name(1493-1496)domain_get_uuid(1691-1696)
| function purge_deleted_snapshots(array &$snaps){ | ||
| foreach ($snaps as $snapname => $snap) { | ||
| $broken = false; | ||
|
|
||
| function refresh_snapshots_database($vm) { | ||
| foreach ($snap['disks'] as $disk) { | ||
| $snapfile = $disk['source']['@attributes']['file']; | ||
|
|
||
| if (!file_exists($snapfile)) { | ||
| $broken = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ($broken) { | ||
| unset($snaps[$snapname]); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null safety checks for snapshot array access.
The function assumes $snap['disks'] always exists and is iterable. If a snapshot entry is malformed or missing the disks key, this will throw a warning/error. Additionally, the caller may pass null if json_decode() fails (e.g., file doesn't exist or contains invalid JSON), which will fail the array type hint.
🛠️ Suggested fix
function purge_deleted_snapshots(array &$snaps){
+ if (!is_array($snaps)) {
+ return;
+ }
foreach ($snaps as $snapname => $snap) {
$broken = false;
+ if (!isset($snap['disks']) || !is_array($snap['disks'])) {
+ continue;
+ }
+
foreach ($snap['disks'] as $disk) {
- $snapfile = $disk['source']['@attributes']['file'];
+ $snapfile = $disk['source']['@attributes']['file'] ?? null;
- if (!file_exists($snapfile)) {
+ if (empty($snapfile) || !file_exists($snapfile)) {
$broken = true;
break;
}
}🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php around lines
1934 - 1951, The purge_deleted_snapshots function assumes $snaps is a non-null
array and that each $snap contains an iterable 'disks' key; add a null/type
check at the top to return early or cast to an array if $snaps is null/invalid,
and inside the foreach over $snaps ensure isset($snap['disks']) &&
is_array($snap['disks']) before iterating; for malformed entries
(missing/invalid 'disks') skip/unset them, and only access
$disk['source']['@attributes']['file'] after verifying those keys exist before
calling file_exists.
| // Only destructive operations may invalidate snapshots | ||
| if ($delete_used) { | ||
| purge_deleted_snapshots($snaps); | ||
| } |
There was a problem hiding this comment.
Guard against null $snaps before calling purge function.
If the snapshots database file doesn't exist or contains invalid JSON, json_decode() returns null. Passing null to purge_deleted_snapshots() will fail the array type hint.
🛠️ Suggested fix
// Only destructive operations may invalidate snapshots
- if ($delete_used) {
+ if ($delete_used && is_array($snaps)) {
purge_deleted_snapshots($snaps);
}🤖 Prompt for AI Agents
In @emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php around lines
1960 - 1963, Guard against null $snaps before calling purge_deleted_snapshots():
when json_decode() can return null, wrap the call so it only runs if
$delete_used is true AND $snaps is an array (e.g. if ($delete_used &&
is_array($snaps)) purge_deleted_snapshots($snaps);), or explicitly
convert/validate $snaps to an array before calling purge_deleted_snapshots() so
the function's array type hint is never passed null.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
1974-1984: Bug: Inconsistent key access for$parentfindlookup.Line 1979 uses
$disk["device"]but the iteration variable$disknow comes from$snap['disks'](line 1963-1964), which uses$disk["@attributes"]["name"]for the disk identifier. The backing array is keyed by$diskid(line 1974), so line 1979 should also use$diskidinstead of$disk["device"].This will cause an undefined index warning and incorrect parent chain resolution.
🐛 Proposed fix
$snaps[$vmsnap]['backing'][$diskid] = $output; $rev = "r".$diskid; $reversed = array_reverse($output); $snaps[$vmsnap]['backing'][$rev] = $reversed; } - $parentfind = $snaps[$vmsnap]['backing'][$disk["device"]]; + $parentfind = $snaps[$vmsnap]['backing'][$diskid] ?? []; $parendfileinfo = pathinfo($parentfind[1]);
♻️ Duplicate comments (2)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (2)
1934-1948: Null safety issues remain unaddressed.The concerns raised in the previous review about null safety in this function are still applicable:
- The function assumes
$snap['disks']always exists and is iterable- No null-coalescing for
$disk['source']['@attributes']['file']- Caller may pass
nullifjson_decode()fails
1957-1960: Guard against null$snapsbefore calling purge function remains unaddressed.The previous review flagged that
json_decode()can returnnull, which would fail thearraytype hint. This concern is still applicable.
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (2)
2511-2511: Consider simplifying the ternary expression.The expression
$action=="yes" ? true : falsecan be simplified to just$action=="yes"since it already evaluates to a boolean.♻️ Suggested simplification
- refresh_snapshots_database($vm, $action=="yes" ? true : false); + refresh_snapshots_database($vm, $action === "yes");
2611-2611: Same simplification applies here.♻️ Suggested simplification
- refresh_snapshots_database($vm,$action=="yes" ? true : false); + refresh_snapshots_database($vm, $action === "yes");
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR `#2333` by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (3)
get_domain_by_name(1493-1496)domain_get_uuid(1691-1696)domain_snapshot_delete(2538-2542)
🔇 Additional comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
2117-2122: LGTM!The reordering of the success response and logging statements is clear and the logic flow is correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.