fix: make lifecycle lock acquisition atomic - #529
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds persistent OS-level lifecycle locking, coordinates sidecar startup and shutdown through shared state, preserves lock ownership during failures, stops asynchronous work during shutdown, and expands the Windows smoke test to validate lock reuse across two launches. ChangesLifecycle lock coordination
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DesktopLauncher
participant LifecycleLock
participant Sidecar
participant SidecarState
DesktopLauncher->>LifecycleLock: acquire_lock
LifecycleLock-->>DesktopLauncher: LockGuard
DesktopLauncher->>Sidecar: start and probe
Sidecar-->>DesktopLauncher: readiness
DesktopLauncher->>SidecarState: publish child and instance_id
DesktopLauncher->>Sidecar: terminate published child
Sidecar-->>DesktopLauncher: successful termination
DesktopLauncher->>LifecycleLock: clear owner record
Possibly related PRs
🚥 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 |
|
独立审计补充：hosted Windows run |
|
审计更正：run |
|
Implemented and independently re-audited the startup/shutdown race fix in 0a13383. Local evidence: 25/25 Rust tests (including real child-process locking and deterministic transaction tests), clippy -D warnings, fmt, PowerShell AST, and diff-check all pass. The earlier hosted smoke failure was a stale assertion requiring lock-file deletion; the updated contract intentionally retains an empty inode and now verifies two packaged launch/shutdown cycles. Unix/macOS native behavior remains unverified and is not claimed. Keeping this draft until the current CI run is green, then formal audit will be triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@deploy/windows-webui/smoke-desktop-ui.ps1`:
- Around line 58-75: In the record validation flow, move the
[string]::IsNullOrWhiteSpace($instanceId) check before
[Guid]::Parse($instanceId), preserving the existing dedicated error message for
missing or empty instance_id values and validating the GUID only afterward.
In `@ui/src-tauri/src/lifecycle.rs`:
- Around line 22-23: Add a rust-version = "1.89" declaration for the
ui/src-tauri crate or its workspace so File::try_lock and TryLockError require
the supported toolchain. Update the static Rust toolchain pin SHA to the
corresponding CI stable version, preserving the existing toolchain
configuration.
- Around line 466-480: Update startup_lock_serializes_concurrent_claims to
acquire the lock from a separate child process rather than using two descriptors
in the same process, while preserving the WouldBlock assertion and
reclaimability check after the child exits. Reuse the existing dedicated
child-process locking test or its established process-spawn mechanism instead of
relying on same-process acquire_lock calls.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 968bbb7d-7ea7-4d93-aedb-5bf7ca448321
📒 Files selected for processing (3)
deploy/windows-webui/smoke-desktop-ui.ps1ui/src-tauri/src/lifecycle.rsui/src-tauri/src/main.rs
1 participant
Summary
engine-instance.lockbefore lifecycle read/probe/spawn/write work, and retain the lock inode when clearing its owner record.JoinHandle, so exit cannot race a half-published child/owner pair or leave a newly spawned engine orphaned.The original read → probe → spawn → write sequence allowed concurrent shells to make the same decision. The follow-up audit also found that detached startup could overlap
ExitRequested, exposing spawn/publish/cleanup windows. Both paths are now serialized without aborting a spawned-but-unpublished child.Refs #478
Validation
cargo test --manifest-path ui/src-tauri/Cargo.toml --locked— 25 passed, including a real child-process lock handshake and deterministic spawn/shutdown transaction tests.cargo clippy --manifest-path ui/src-tauri/Cargo.toml --all-targets --locked -- -D warnings— passed.cargo fmt --all -- --check— passed.deploy/windows-webui/smoke-desktop-ui.ps1— passed.git diff --check— passed.build.rsprerequisite requiresui/src-tauri/binaries/airp-core-x86_64-pc-windows-gnu.exe. Local Rust validation used an empty temporary placeholder that was removed afterward; no binary asset is committed.Audit status
Independent audit was performed before the lifecycle fix and repeated after each blocking test correction. The final audit found no remaining blocking issue and approved the change for CI. Formal repository audit remains required after the PR leaves draft; this PR must not merge until that audit and every required check pass.
Summary by CodeRabbit
Bug Fixes
Enhancements
Tests