fix(rebuild): restart the Hermes gateway after state restore - #8472
fix(rebuild): restart the Hermes gateway after state restore#8472hunglp6d wants to merge 12 commits into
Conversation
Rebuild starts the gateway during recreation and restores durable state about 27 seconds later. An adapter that reads its state once at startup keeps the pre-restore result for the life of the process — Hermes' WhatsApp bridge reads its paired session that way — so the gateway can be alive and healthy while still serving the state the rebuild replaced. The post-restore step proved only liveness, which was never in doubt, so it reported success and left the channel unpaired until an operator ran `gateway restart` by hand. Restart the gateway first and let the existing check report on the process that restart produced. A gateway that stays up through a failed restart is now unverified instead of healthy; one the recovery check replaced is still accepted, because that process is new. `relaunchManagedSupervisorSession` and `restartRestoredSandboxGateway` already restart after their own restore, so rebuild was the outlier. The restart stays outside `runHermesCronRestoreTransaction`, whose drain gate compares the gateway pid and start time on validate and release. Signed-off-by: Hung Le <hple@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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 (8)
📝 WalkthroughWalkthroughHermes rebuild recovery now restarts the gateway after state restoration, checks gateway health, and then completes MCP reconciliation. Tests cover ordering, restart failures, recovery outcomes, non-Hermes rebuilds, harness support, and end-to-end output. ChangesHermes rebuild recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StateRestore
participant HermesPostRestore
participant Gateway
participant RecoveryCheck
participant MCPReconciliation
StateRestore->>HermesPostRestore: complete state restoration
HermesPostRestore->>Gateway: restart gateway quietly
Gateway-->>HermesPostRestore: restart result
HermesPostRestore->>RecoveryCheck: verify or recover gateway
RecoveryCheck-->>HermesPostRestore: gateway status
HermesPostRestore->>MCPReconciliation: reconcile MCP configuration
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8472.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: Blockers
|
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
The two failing CLI shards were caused by this change: the older shared rebuild-flow harness did not stub the newly required Hermes gateway restart, so otherwise successful rebuild cases reached the real restart path and failed post-restore verification. I added the same successful restart stub and typed override/spy already used by the newer harness. Validation now passes for the 111 rebuild-flow cases and the previously failing base-image case; CLI type-check and Biome checks also pass. An independent documentation review confirmed that the existing sandbox recovery page already covers the user-visible behavior and that this test-helper repair needs no further docs change. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer update on the requested change:
The earlier CLI parity failure was PR-related and is fixed. Setup-stage failures from the GitHub Actions outage were infrastructure failures and were not rerun; fresh checks are now queued for the updated branch. The PR remains draft. It still needs contributor ready-state intent, green required checks, and an independent human approval before it can merge. |
|
CI follow-up:
The PR remains draft because contributor ready-state intent and independent human approval are still outstanding. |
|
Maintainer follow-up after the branch's latest main refresh:
No further technical, documentation, compliance, or security change is indicated. The PR remains draft. Remaining blockers are the contributor's decision to mark it ready and independent human approval. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
This PR remains a draft and is behind the current main commit. Refresh it, complete the Hermes post-restore lifecycle evidence, and run the required workflows before marking it ready. The restart-before-health contract correctly avoids accepting the pre-restore gateway process, and the current advisor reports no additional finding.
The stale-base request was addressed. Draft state and required checks remain separate from code-review findings.
3 participants
Summary
rebuildstarts the sandbox gateway during recreation and restores durable workspace state about a minute later, so the gateway keeps serving the state it read before the restore replaced it. The post-restore step proved only that the gateway was alive — which was never in doubt — sorebuildreported success while the restored state was ignored. It now restarts the gateway after the restore and only reports health for a process that restart produced.Before:
rebuildfinishes with a gateway older than the state it serves, and any adapter that reads its durable state once at startup keeps the pre-restore result until an operator runsnemoclaw <sandbox> gateway restartby hand. After: the gateway in a completedrebuildstarted after the restore.Related Issue
Part of #8184, item 4. No closing keyword: #8184 is the umbrella report and its other items are fixed separately.
Changes
src/lib/actions/sandbox/rebuild-hermes-post-restore.ts—ensureHermesGatewayAfterStateRestorerestarts the managed gateway once, then runs the existing bounded recheck loop.healthynow requires both that the restart succeeded and that the gateway is running; a gateway that stayed up through a failed restart isunverified, because it is still serving the state this step exists to replace. A gateway the recovery check replaced is still accepted asrecovered— that process is new regardless of what the restart reported.src/lib/actions/sandbox/rebuild-post-restore-phase.ts— success line now readsHermes gateway restarted and verified after state restore.docs/manage-sandboxes/recover-rebuild-sandboxes.mdx— the page summarizes the printed recovery guidance, so it namesgateway restartfirst andrecoveras the fallback, and states why the restart is needed.restore → restart → health checkordering, an injectablerestartSandboxGatewayin the harness, and a live assertion inrebuild-hermes.test.tsthat rebuild reports a gateway bound to the restored state.The restart stays outside
runHermesCronRestoreTransaction: that gate records the gatewaypidandstart_timewhen it drains dispatch and compares them again on validate and release, so a restart inside it would fail its own identity check.No new abstraction, configuration, fallback, or compatibility path. The restart reuses the exported
restartSandboxGateway.This is not a new pattern.
relaunchManagedSupervisorSession(supervisor-relaunch.ts) andrestartRestoredSandboxGateway(restore-gateway-pairing.ts) already restart the gateway after their own restore, for the same reason.rebuildwas the outlier.maingained a separate fix for the same function while this branch was open — a bounded recheck aroundcheckAndRecoverSandboxProcesses(HERMES_GATEWAY_RECHECK_ATTEMPTS). This branch merges it rather than replacing it: the recheck tolerates a check that comes back inconclusive, and the restart added here is what makes the observed process a fresh one. They address different failures and both are needed.Type of Change
Quality Gates
PR review advisor (Nemotron 3 Ultra),cli-tests,cli-test-shards (5), andchecksare failing on this head and need triage before review.Documentation Writer Review
docs-updateddocs/manage-sandboxes/recover-rebuild-sandboxes.mdx. The review returned one blocking finding — the page summarized the printed recovery guidance and still namedrecoverfor gateway health after the CLI message changed — and it was fixed in this change. The review ran atcc9a3dd250; this PR's own documentation diff is unchanged since then, so the result still describes the current content.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable46940cb2a5; the current head adds one further merge frommainand has not been re-measured.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Live evidence
Measured four times, on three hosts, by three independent operators — my own run plus two verification agents given the same script on two freshly provisioned machines and no knowledge of each other's results. Each sandbox came from a real
onboard --non-interactiverun, not from seeded registry state. A marker file was written into thememoriesstate directory before each rebuild so the restore had something to carry back.ctimeis the signal, notmtime: the restore preservesmtime, so a marker keeps its original value across rebuilds. Onlyctimemoves when the restore writes the file back.gateway-starts.logmainmainmain46940cb2a5, no local patchesThe second
gateway-starts.logentry makes the restart observable inside the sandbox rather than only in CLI output. Onmainthe rebuild printedHermes gateway health verified after state restore; on this branch it printedHermes gateway restarted and verified after state restore, and neither of the other two states. Marker contents survived every rebuild, so the restore itself worked in each run.Environments: Ubuntu 24.04, kernels 6.8 and 6.17, x86_64, Docker 29.7.2, OpenShell 0.0.85, Hermes Agent 0.19.0, Node 22.23.2, NVIDIA hosted inference.
Scope
Reproduced on Hermes. Not tested on OpenClaw, which has no equivalent post-restore gateway step —
ensureHermesGatewayAfterStateRestorereturnsnot-applicablefor every other agent. The comment insupervisor-relaunch.tsnames OpenClaw as retaining pre-restore runtime state on that path, so the same question is open for OpenClaw and is left for a separate change.This change corrects the ordering between the gateway and the state it serves. It is not sufficient on its own to make a Hermes WhatsApp channel deliver messages; other defects in that path are tracked separately under #8184.
Merge-order note for #8423
PR #8423 adds a section to
docs/manage-sandboxes/set-up-whatsapp.mdxinstructing users to rungateway restartafterrebuild, with the rationale this change removes. That text is not onmain, so it cannot be removed here. Whichever of the two PRs merges second must delete the manualgateway restartline and the two sentences explaining it.Signed-off-by: Hung Le hple@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation
Tests