fix(downgrade): block during protected array shrink - #2698
fix(downgrade): block during protected array shrink#2698Eli Bosley (elibosley) wants to merge 11 commits into
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
…ected-shrink-downgrade-guard
|
Closing as obsolete: the new Core storage surface replaces the legacy WebGUI path, so this PHP downgrade guard is not part of the rollout and must not be shipped. Core owns storage and OS replacement coordination directly. |
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 retained downgrade marker and blocks downgrade staging while protected recovery is active, incomplete, or uncertain.
Resolution
/var/run/unraid-os-storage-interlockfile lock used by Core before either side checks or publishes its intent/marker./var/run/unraid-os-downgradeexclusively while holding that lock, then inspect the two Core-owned production records:parity-protected-shrink.jsonparity-protected-shrink.recovery.json/bin/sync -f /boot/config(syncfs(2)) while still holding the lock, clear PHP stat caches, and reread both records before deciding./boot/configdirectory chain and contain strict, identity-equivalent version-7 records whose literal stage iscompleted.Reviewer considerations
flockmakes downgrade-marker publication and protected-shrink publication one cross-process decision.lstatfailure without following symlinks.Verification
d74e79c44c28c3e3c5b7648faf365a4d750e7ecd.31cb4ce25b08919b53ea89b7325967a3eba89935; the current-head deltaextracts the existing rejection boundary for direct testing and makes unknown
decisions fail closed before boot mutation.
parity_protected_shrink_guard_test.phpanddowngrade_endpoint_decision_test.phppass.rejection-side boot-mutation suppression, and ok-only continuation.
Risk
Low code volume but safety-critical ordering. The shared lock, checked durability barrier, strict two-copy validation, retained downgrade marker, and fail-closed path classification keep rollback decisions conservative.
Summary by CodeRabbit