feat: add self-update foundations#281
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR establishes the self-update subsystem foundation with manifest parsing, binary release management, and concurrent-update protection. It adds the ChangesSelf-update foundation implementation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AppUpdateManifest
participant Validation
participant Conversion
Caller->>AppUpdateManifest: parse(json)
AppUpdateManifest->>AppUpdateManifest: deserialize raw
AppUpdateManifest->>Validation: enforce schema_version
Validation->>Validation: enforce channel="stable"
Validation->>Validation: parse and bounds-check versions
Validation->>Conversion: parse assets
Conversion->>Conversion: validate URLs (https, host, filename)
Conversion->>Conversion: enforce asset size > 0
Conversion->>Conversion: reject duplicate platforms
Conversion-->>AppUpdateManifest: return typed manifest
AppUpdateManifest-->>Caller: Result<AppUpdateManifest>
sequenceDiagram
participant AppReleaseLayout
participant FileSystem
participant Symlink
AppReleaseLayout->>AppReleaseLayout: validate version format
AppReleaseLayout->>FileSystem: verify release binary exists
AppReleaseLayout->>Symlink: create temporary symlink to new version
Symlink->>FileSystem: atomic rename to "active"
FileSystem->>FileSystem: sync bin directory
FileSystem-->>AppReleaseLayout: sync complete
AppReleaseLayout-->>AppReleaseLayout: success or InvalidAppReleasePointer
sequenceDiagram
participant Client
participant ReconciliationQueue
participant DaemonJobs
participant UpdateLock
Client->>DaemonJobs: run_job(scope)
DaemonJobs->>ReconciliationQueue: enqueue_mutating_with_abandon(scope)
alt No existing lock
ReconciliationQueue->>UpdateLock: acquire(paths)
UpdateLock-->>ReconciliationQueue: UpdateLock guard
ReconciliationQueue->>ReconciliationQueue: store in QueueState
ReconciliationQueue-->>DaemonJobs: EnqueueResult
DaemonJobs-->>Client: Ok(job_id)
else Lock already held
UpdateLock-->>ReconciliationQueue: Err(UpdateInProgress)
ReconciliationQueue-->>DaemonJobs: Err(UpdateInProgress)
DaemonJobs->>DaemonJobs: write error over transport
DaemonJobs-->>Client: error response
end
alt On queue idle
ReconciliationQueue->>UpdateLock: drop guard (release_update_lock_if_idle)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
This PR leaves two self-update foundations unsafe for merge: daemon reconciliation is not mutually excluded for the update window, and installed release binaries may not be executable.
Reviewed changes — This PR adds the stable app update manifest parser, PV app release layout helpers, and daemon update-lock checks needed for later pv update work.
- Document the v1 app update contract —
DESIGN.mdnow scopes the manifest to stable-channel PV app releases with exact macOS platform assets and explicit out-of-scope update behavior. - Add
self-updatemanifest parsing —crates/self-updatevalidates schema, channel, versions, timestamps, URLs, checksums, sizes, duplicate platforms, and platform selection with snapshot coverage. - Add release layout and locking primitives —
statenow exposesAppReleaseLayout, active PV binary symlink helpers, app release paths, filesystem helpers, andUpdateLockat~/.pv/run/update.lock. - Gate daemon reconciliation during updates — foreground and background reconciliation paths now reject requests when the update lock is already held while keeping daemon health available.
GPT ｜ 𝕏
Contributor
There was a problem hiding this comment.
Important
This incremental update fixes the prior lock lifetime issue, but it introduces a concurrency regression for normal daemon reconciliation requests.
Reviewed changes — This run reviewed the updates since the prior Pullfrog review, focused on lock lifetime and release binary executability fixes.
- Held daemon mutation locks longer —
run_joband background reconciliation now acquireUpdateLockguards and hold them across queued/running reconciliation work. - Expanded daemon lock coverage — new daemon tests cover lock-held request rejection and queued background reconciliation reserving the update lock.
- Made installed app binaries executable —
AppReleaseLayout::install_release_binarynow secures copied release binaries with executable permissions, and state tests assert mode700.
GPT ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/daemon/tests/daemon_foundation.rs (1)
111-149: ⚡ Quick winAdd a same-daemon contention regression.
This only proves that an externally held update lock rejects
run_job. It will still pass if one daemon reconciliation holdsUpdateLockand a second daemon request starts failing early instead of reaching the queue/coalescing path, which is the more fragile contract here. Please add a two-request case that keeps the first reconcile alive and asserts the second is queued/coalesced rather than rejected.🤖 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 `@crates/daemon/tests/daemon_foundation.rs` around lines 111 - 149, Extend the test update_lock_rejects_mutating_jobs_but_keeps_health_available to add a same-daemon contention scenario: start a RunningDaemon, acquire an UpdateLock in test harness, then send two "run_job" reconcile requests via request_lines where the first request is kept alive (simulate a long-running reconcile—e.g., send a reconcile that blocks or uses a job ID you can inspect) and the second request is issued while the first is active; assert that the second is queued or coalesced (not rejected) by checking daemon's job queue/state (use database.recent_jobs() and the existing run_job_lines/normalize_update_lock_path helpers) and adjust assertions to expect a queued/coalesced result for the second request instead of an immediate rejection while preserving the existing health check assertions.crates/state/src/update_lock.rs (1)
33-38: Limit exposure ofUpdateLock::check_available(or return the guard)
check_available()immediately drops the lock guard after checking, and it’s only declared incrates/state/src/update_lock.rs(no other call sites found). Consider making itpub(crate)/test-only, or have it returnUpdateLockso callers must hold the reservation through the mutation window.🤖 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 `@crates/state/src/update_lock.rs` around lines 33 - 38, The check_available function currently acquires then immediately drops the UpdateLock guard (in UpdateLock::check_available using PvPaths), which either should be restricted or should return the guard so callers must hold the reservation; fix by either making check_available non-public (change pub fn check_available to pub(crate) or annotate as #[cfg(test)] for test-only) if it’s only used internally, or change its signature to return Result<UpdateLock, StateError> (i.e., return the acquired lock instead of dropping it) so callers retain the UpdateLock guard during the mutation window; update call sites/tests accordingly to use the returned UpdateLock when chosen.
🤖 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 `@crates/daemon/src/server.rs`:
- Around line 146-167: handle_connection currently acquires UpdateLock
(UpdateLock::acquire) before calling run_job, which causes concurrent in-process
requests to fail with StateError::UpdateInProgress instead of allowing
enqueue_reconciliation_job to return EnqueueResult::Coalesced; fix by deferring
the UpdateLock acquisition until after the reconciliation job has been
enqueued/coalesced (i.e., remove the upfront UpdateLock::acquire in
handle_connection and instead acquire the lock inside run_job or just before the
actual update work after enqueue_reconciliation_job returns a non-coalesced
result), or make UpdateLock reuse a single process-wide file descriptor so
multiple acquires in the same process do not trigger EWOULDBLOCK; update
run_job/enqueue_reconciliation_job logic accordingly to ensure coalescing occurs
before taking the exclusive filesystem lock.
---
Nitpick comments:
In `@crates/daemon/tests/daemon_foundation.rs`:
- Around line 111-149: Extend the test
update_lock_rejects_mutating_jobs_but_keeps_health_available to add a
same-daemon contention scenario: start a RunningDaemon, acquire an UpdateLock in
test harness, then send two "run_job" reconcile requests via request_lines where
the first request is kept alive (simulate a long-running reconcile—e.g., send a
reconcile that blocks or uses a job ID you can inspect) and the second request
is issued while the first is active; assert that the second is queued or
coalesced (not rejected) by checking daemon's job queue/state (use
database.recent_jobs() and the existing run_job_lines/normalize_update_lock_path
helpers) and adjust assertions to expect a queued/coalesced result for the
second request instead of an immediate rejection while preserving the existing
health check assertions.
In `@crates/state/src/update_lock.rs`:
- Around line 33-38: The check_available function currently acquires then
immediately drops the UpdateLock guard (in UpdateLock::check_available using
PvPaths), which either should be restricted or should return the guard so
callers must hold the reservation; fix by either making check_available
non-public (change pub fn check_available to pub(crate) or annotate as
#[cfg(test)] for test-only) if it’s only used internally, or change its
signature to return Result<UpdateLock, StateError> (i.e., return the acquired
lock instead of dropping it) so callers retain the UpdateLock guard during the
mutation window; update call sites/tests accordingly to use the returned
UpdateLock when chosen.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba216143-50f4-4725-a1c6-1267d35bd277
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.lockcrates/daemon/tests/snapshots/daemon_foundation__update_lock_rejects_mutating_jobs_but_keeps_health_available.snapis excluded by!**/*.snapcrates/self-update/tests/snapshots/app_update_manifest__app_update_manifest_parses_stable_release_and_selects_platform.snapis excluded by!**/*.snapcrates/self-update/tests/snapshots/app_update_manifest__app_update_manifest_rejects_invalid_asset_fields.snapis excluded by!**/*.snapcrates/self-update/tests/snapshots/app_update_manifest__app_update_manifest_rejects_schema_channel_version_and_compatibility_errors.snapis excluded by!**/*.snapcrates/self-update/tests/snapshots/app_update_manifest__app_update_manifest_selection_reports_missing_platform.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
Cargo.tomlDESIGN.mdcrates/daemon/src/jobs.rscrates/daemon/src/server.rscrates/daemon/tests/daemon_foundation.rscrates/self-update/Cargo.tomlcrates/self-update/src/lib.rscrates/self-update/tests/app_update_manifest.rscrates/state/src/app_release.rscrates/state/src/error.rscrates/state/src/fs.rscrates/state/src/lib.rscrates/state/src/paths.rscrates/state/src/update_lock.rscrates/state/tests/state_foundation.rs
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — This run reviewed the daemon queue-lock update added since the prior Pullfrog review, focused on preserving reconciliation coalescing while maintaining self-update exclusion.
- Moved daemon mutation locking into
ReconciliationQueue—enqueue_mutating_with_abandonnow keeps a single daemon-ownedUpdateLockacross queued and running reconciliation work instead of requiring each same-daemon request to acquire the OS lock before queueing. - Preserved same-daemon reconciliation behavior — foreground and background reconciliation can now continue queueing or coalescing while an existing daemon reconciliation owns the update lock.
- Added a coalescing regression test —
background_reconciliation_coalesces_under_daemon_update_lockcovers a queued background reconciliation receiving a duplicate request while the daemon-held update lock is active.
GPT ｜ 𝕏
Contributor
|
Fixed the RustFS CI failure by making Task list (8/8 completed)
|
This was referenced Jun 12, 2026
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Out of scope
pv update,pv update --check, binary download/swap, daemon restart coordination, rollback, installer/release metadata generation, Managed Resource update orchestration, artifact recipe changes, or runtime channel selection.Verification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo nextest run -p self-update -p state -p daemon --all-features --lockedcargo insta pending-snapshots --workspacegit diff --cached --checkSummary by CodeRabbit
New Features
Chores