fix(portable): unify Hermes lifecycle lock roots - #11482
Conversation
Use the receipt-owned state directory for lifecycle locks and roll back partial starts. Signed-off-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughHermes Portable lifecycle lock options are centralized and applied to connect, start, stop, CLI, and Docker lifecycle flows. OpenShell startup failures now trigger reconciliation and rollback handling. Tests cover Portable lock usage and startup failure recovery. ChangesHermes Portable lifecycle
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Connect
participant hermesPortableLifecycleLockOptions
participant LifecycleLock
participant HermesPortableLifecycle
participant OpenShell
Connect->>hermesPortableLifecycleLockOptions: resolve sandbox lock options
hermesPortableLifecycleLockOptions-->>Connect: state directory or undefined
Connect->>LifecycleLock: run preflight and child preparation
HermesPortableLifecycle->>OpenShell: start sandbox
OpenShell-->>HermesPortableLifecycle: status and error output
HermesPortableLifecycle->>HermesPortableLifecycle: reconcile and rollback on failure
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change centralizes how Hermes Portable sandboxes acquire their lifecycle lock and improves failure handling when a Portable container fails to start, ensuring partial starts are rolled back correctly. The changes are well covered by targeted tests for lock behavior, non-default port stop, interactive connect locking, and partial-start rollback, and no material risk was found in the affected CLI, sandbox action, or Docker runtime paths. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 03803aa in the TypeScript / code-coverage/cliThe overall line coverage in commit 03803aa in the Show a line coverage summary of the most impacted files.
Updated |
rsliter
left a comment
There was a problem hiding this comment.
Reviewed commit 03803aa8da7d341e40a013f69c6aefcf0d1403be. The shared lock-option helper now keeps Hermes Portable lifecycle roots consistent across connect, start, stop, CLI, and Docker flows. Startup failures also enter the existing reconciliation and rollback path. The complete diff and adjacent lock and recovery code show no blocking correctness or security issue across the nine security categories. Required checks pass, CodeRabbit found no actionable issue, and exact-commit CI source tests pass. Focused local validation passed 174 cases; two additional local cases were limited by the macOS sandbox environment rather than candidate behavior.
Outcome
Hermes portable lifecycle operations now use the receipt-owned host lock root across CLI dispatch, start, stop, provider recovery, and connect. A failed OpenShell start is reconciled and rolled back even when it changed container state before reporting failure.
Reason
Follow-up review after #11336 merged found that non-default gateway ports could give start and stop opposite lock ordering, while interactive connect still selected the gateway-scoped lock. The same review found that a partially successful OpenShell start could return a failure without entering rollback.
Related issues
Changes
Verification
npm ci— passed, including the CLI build.npm run typecheck— passed.npm run test:titles:check— passed.npm run test-size:check— 45 tests passed.NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr— passed with no skipped applicable validator.git diff --check— passed.Review notes
The first canonical validation attempt reached
tsc-cliand exhausted Node’s default approximately 4 GB heap. The identical gate passed after raising only the Node heap ceiling to 8 GB; no check was bypassed.Signed-off-by: Prekshi Vyas 34834085+prekshivyas@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests