fix(downgrade): block during protected array shrink#2698
Conversation
WalkthroughThe downgrade flow now checks interlock availability, concurrent downgrade markers, and parity-protected shrink activity before continuing. A reusable guard module handles filesystem coordination, with tests covering status transitions, marker cleanup, and lock handoff. ChangesDowngrade protection
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Sequence Diagram(s)sequenceDiagram
participant Downgrade
participant ShrinkGuard
participant Filesystem
Downgrade->>ShrinkGuard: begin downgrade with proof paths
ShrinkGuard->>Filesystem: acquire interlock and create marker
Filesystem-->>ShrinkGuard: return marker and lock status
ShrinkGuard->>Filesystem: inspect protected shrink files
Filesystem-->>ShrinkGuard: return active or idle state
ShrinkGuard-->>Downgrade: return guard status
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🔧 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.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.plugin.manager/tests/parity_protected_shrink_guard_test.php (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse double quotes for newline escape sequences.
The string
'stage=prepared\n'uses single quotes, which means\nis interpreted as literal characters\andnrather than a newline character. While this doesn't impact theis_file()check in this test, it's best to use double quotes if a true newline was intended.🧽 Proposed nitpick
-file_put_contents($testFiles[2], 'stage=prepared\n'); +file_put_contents($testFiles[2], "stage=prepared\n");🤖 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/tests/parity_protected_shrink_guard_test.php` at line 28, Update the file_put_contents call in parity_protected_shrink_guard_test to use a double-quoted string so \n is written as an actual newline, preserving the existing content and test behavior.
🤖 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.
Nitpick comments:
In
`@emhttp/plugins/dynamix.plugin.manager/tests/parity_protected_shrink_guard_test.php`:
- Line 28: Update the file_put_contents call in
parity_protected_shrink_guard_test to use a double-quoted string so \n is
written as an actual newline, preserving the existing content and test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8e288f21-1229-4663-b6f8-e20811c64a73
📒 Files selected for processing (3)
emhttp/plugins/dynamix.plugin.manager/include/Downgrade.phpemhttp/plugins/dynamix.plugin.manager/include/ParityProtectedShrinkGuard.phpemhttp/plugins/dynamix.plugin.manager/tests/parity_protected_shrink_guard_test.php
There was a problem hiding this comment.
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.plugin.manager/tests/parity_protected_shrink_guard_test.php (2)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest the
downgrade_pendingstatus.The test correctly validates the success state but misses an opportunity to verify the
downgrade_pendingresponse that occurs when the marker already exists. Testing this state improves coverage of the guard's contract.🧪 Proposed refactor to add the test case
assert_same(true, is_file($testMarker), 'A successful downgrade decision must retain its marker.'); +assert_same( + 'downgrade_pending', + parity_protected_shrink_begin_downgrade($testFiles, $testMarker, $testInterlock), + 'A concurrent or subsequent downgrade attempt must report downgrade_pending.' +); unlink($testMarker);🤖 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/tests/parity_protected_shrink_guard_test.php` around lines 37 - 38, Extend the test around the existing $testMarker assertions to cover the downgrade_pending response when the marker already exists. After establishing the marker, invoke the guard again and assert the returned status is downgrade_pending before cleanup, while preserving the existing success-state and marker-retention checks.
64-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up temporary directories to prevent resource leaks.
The test creates temporary nested directories (
$testRoot/unraid/storageand$testRoot/storage) but only cleans up specific files at the end. This leaves dangling directories in the system temp directory after every test run.Consider replacing the individual file
unlinkcalls with a recursive removal of the entire test root to ensure all temporary artifacts are thoroughly cleaned up.♻️ Proposed refactor for complete cleanup
-unlink($testFiles[2]); -unlink($testInterlock); +exec('rm -rf ' . escapeshellarg($testRoot));🤖 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/tests/parity_protected_shrink_guard_test.php` around lines 64 - 65, Update the cleanup at the end of the test to recursively remove the entire temporary test root instead of only unlinking $testFiles[2] and $testInterlock. Ensure the removal covers all nested directories and files created under $testRoot, leaving no temporary artifacts after the test.
🤖 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.
Nitpick comments:
In
`@emhttp/plugins/dynamix.plugin.manager/tests/parity_protected_shrink_guard_test.php`:
- Around line 37-38: Extend the test around the existing $testMarker assertions
to cover the downgrade_pending response when the marker already exists. After
establishing the marker, invoke the guard again and assert the returned status
is downgrade_pending before cleanup, while preserving the existing success-state
and marker-retention checks.
- Around line 64-65: Update the cleanup at the end of the test to recursively
remove the entire temporary test root instead of only unlinking $testFiles[2]
and $testInterlock. Ensure the removal covers all nested directories and files
created under $testRoot, leaving no temporary artifacts after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43dc4d10-502c-47f3-9f75-f1bdc0fb15f2
📒 Files selected for processing (3)
emhttp/plugins/dynamix.plugin.manager/include/Downgrade.phpemhttp/plugins/dynamix.plugin.manager/include/ParityProtectedShrinkGuard.phpemhttp/plugins/dynamix.plugin.manager/tests/parity_protected_shrink_guard_test.php
1 participant
Summary
An OS downgrade can strand a parity-protected array shrink on a release that cannot recover it. This adds a shared OS-level interlock plus a durable downgrade marker and blocks downgrade staging while any Core intent or daemon proof exists.
Resolution
/var/run/unraid-os-storage-interlockfile lock used by Core before either side checks or publishes its durable intent/marker./var/run/unraid-os-downgradeexclusively while holding that lock, then inspect persistent state.Reviewer considerations
flockmakes downgrade-marker publication and protected-shrink authorization one cross-process decision; the durable marker/intent keeps the loser fenced afterward./boot/config/unraid/storage/parity-protected-shrink.json; daemon proofs live under/boot/config/storage.Verification
59b2cc06bcf9bfc8206584eaa9de02ca23fbe5b9contains latest fetched upstreammasterand is mergeable.parity_protected_shrink_guard_test.phppassed, including lock handoff behavior.465) passed the final staged-topology hard-power scenario in 9.2 minutes; daemon-restart transaction fencing (509) passed in 3.0 minutes.Risk
Low code volume but safety-critical ordering. The shared lock and marker fail closed; deployment must follow the daemon-first sequence.