Skip to content

fix(setup): retry guarded restart after reload owner handoff - #1515

Open
karkarl wants to merge 1 commit into
mainfrom
karkarl-guarded-restart-recovery
Open

karkarl wants to merge 1 commit into
mainfrom
karkarl-guarded-restart-recovery

Conversation

@karkarl

@karkarl karkarl commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Related to #1498. This is a user-authorized, focused extraction from a6190ba0935d20c97ecaf69bdc4f0871ebb63589 in #1447 (feat(setup): guide native Gateway installation and onboarding), based directly on main 273b0182745a3093c0e09f306ca8a1fff6ef3c5a. It does not include the native onboarding feature history. #1447 and its branch are unchanged. Do not close #1498: restart-intent coordinator contention remains unresolved.

After reload-mode restoration, SetupWizardRunner.RestoreReloadModeAsync recognizes only the exact diagnostic:

GATEWAY_RESTART_PREPARATION_REFUSED: Cannot verify a live serving Gateway owner for the selected service. Gateway was not signaled.

It reuses VerifyExpectedManagedGatewayAsync, then makes one recovery call to the same guarded Gateway CLI restart. The existing provenance wait allows up to 30 one-second retry delays, plus probe duration, for NoListener or UnknownListener specifically tagged ListenerSnapshotChanged. Other unknown/conflicting listeners, other restart errors, and repeated refusal fail closed.

There is no direct systemd restart fallback, lock deletion, arbitrary signaling, silent downgrade, or ownership bypass. SetupWizardRunner remains the orchestration owner; the existing provenance policy and upstream Gateway admission retain their responsibilities. Listener provenance is not proof of owner-lease or coordinator readiness.

Extraction provenance and deviations

Head: 77b7e4bb295e2c32df587c17e4e1a35ab0a2f428. Three files, 122 added lines.

  • Production recovery: all 18 added lines are unchanged from a6190ba0.
  • Tests: retain all 88 original restart-test lines and add one explicit negative case for StateDatabaseCoordinatorContentionError / restart-intent-recording refusal. Seven restart regression cases pass.
  • Documentation: retain the directly relevant setup-restart paragraph, adapting the main-only context and clarifying the typed snapshot-change exception, probe-duration budget, provenance limits, and untreated contention.
  • Exclude all 40 unrelated native-package lines in docs/TEST_COVERAGE.md and native onboarding context from the setup-document conflict.
  • Do not duplicate fixture-isolation changes in E2ESetupFixture.cs, SetupAndConnectTestsUninstallIsolation.cs, or AppRefactorContractTests.cs: they are already on main through fb8b9e736f7473705ea14d8e97f6117bcbaddf68.

Required proof pools

  • windows-wsl-gateway-e2e: post-wizard managed Gateway restart and setup recovery. Fresh local strict fixture exercised the changed recovery on official Gateway 2026.9.6.
  • windows-wsl-mxc: required strict gateway setup/connect closeout, including real Gateway -> Windows node -> system.run containment. All 17 tests passed without skips.

No new UI surface, node capability, or MCP command contract is introduced.

Validation

Fresh validation of this extracted source on local Windows ARM64, build 26694. OPENCLAW_REPO_ROOT pointed to this worktree; tray data and runtime TEMP/TMP were task-owned and isolated. $proof below is the session-owned results directory. Fresh-worktree tests intentionally allowed restore rather than risking a --no-restore no-op.

Command Result
.\build.ps1 Passed all five build targets: Shared, Cli, WinNodeCli, SetupEngine, WinUI
dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --logger "trx;LogFileName=Shared.trx" --results-directory $proof 4,106 passed, 33 skipped, 0 failed
dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --logger "trx;LogFileName=Tray.trx" --results-directory $proof 3,072 passed, 0 skipped, 0 failed
dotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --logger "trx;LogFileName=SetupEngine.trx" --results-directory $proof 1,200 passed, 1 skipped, 0 failed; all 7 focused restart cases executed
$env:OPENCLAW_E2E_GATEWAY_VERSION='2026.9.6'; pwsh -NoProfile -File .\scripts\validate-mxc-e2e.ps1 -NoBuild -ResultsDirectory "$proof\mxc-results" 17 passed, 0 skipped, 0 failed, exit 0. -NoBuild reused the completed full ARM64 app build; the script always rebuilt the E2E project. No -AllowSkip.
git diff --cached --check before commit Passed

Real behavior proof

Current-source strict local fixture, head 77b7e4bb:

  • Official Gateway 2026.9.6 selected; the classified recovery warning occurred exactly once: Gateway restart owner was unavailable after restoring reload. Rechecking managed ownership before one restart retry.
  • Setup completed, the isolated tray connected through its MCP fixture, and all strict tests passed. Passing required proofs include MirroredWslSafeGatewayPort_IsListeningAndRecorded, RealGateway_SystemRun_ExecutesThroughWindowsNodeMxcSandbox, and RealGateway_SystemRun_BlocksWritesToTrayDataDirectoryInMxcSandbox.
  • No typed coordinator-contention marker occurred. This run proves the exercised serving-owner recovery, not a remedy for coordinator contention or every historical owner refusal.
  • Fixture uninstall exited 0 and teardown completed. The uniquely named task distro was verified unregistered, and both fixture data directories were verified absent. No existing user distro/profile, MSIX registration, auth terminal, or autostart entry was used for proof.
  • Only allowlisted counts, diagnostic categories, version, test names, and cleanup outcomes are published. No raw runtime logs, configurations, environments, command lines, tokens, or identity data are attached.

Historical source evidence, not validation of this extracted head

Run 36055236206 at original a6190ba0 exercised the classified retry once in each Gateway 2026.9.6 shard: setup/connect 40 passed and 6 MXC skips, revocation 1 passed, network 2 passed. SetupEngine was 1,353 passed / 1 skipped; Tray was 3,097 passed. The overall run failed: Shared had 4,167 passed, 2 failed, 1 skipped. The failures were BoundedProcessWaitTests.WaitAsync_CancellationDoesNotWaitForInheritedPipeHandles (3,053 ms against a <3-second limit) and PiperVoiceExtractionTests.ExtractTarBz2Async_CancellationIsBoundedAndKillsExtractor (extractor still running). They are not attributed to #1498 or declared harmless flakes. These historical counts do not replace the fresh results above.

Review and remaining gates

Draft pending structured autoreview and security-sensitive maintainer review. No merge is authorized.

  • Required rubber-duck review completed with no blocking findings. Nonblocking follow-ups were stdout-only future refusal diagnostics and retention of the first diagnostic if provenance re-verification fails. Neither expands this extraction: fresh runtime proof exercised the current classifier, and an untrusted listener still fails closed. The original production patch is preserved.
  • Structured review attempted with python .agents\skills\autoreview\scripts\autoreview --mode local --prompt <focused-extraction-context> --output <session-report> --json-output <session-json>. Not verified / blocked: its secret-like-content guard rejected expanded diff context before the model ran. The scanner was not bypassed or weakened. This is not a clean autoreview result.
  • Keep the issue's clawsweeper:needs-security-review requirement visible and preserve clawsweeper:no-new-fix-pr; the separate focused PR was explicitly authorized by the user, not spawned as an automated repair.
  • StateDatabaseCoordinatorContentionError / approximately 5,011 ms state-lifecycle admission failure occurs before owner resolution. It remains a separate, untreated intent-recording failure.
  • The old-owner-still-listening / successor-readiness timing case remains unproven. Provenance cannot establish full upstream readiness; a repeated guarded refusal still fails setup. No broader retry policy is proposed here.

Extract the guarded serving-owner recovery from a6190ba in PR #1447 (feat(setup): guide native Gateway installation and onboarding), onto main 273b018. Production recovery is unchanged. Omit native-package documentation and fixture isolation already present on main via fb8b9e7. Retain focused tests and setup documentation; add a coordinator-contention non-retry case and clarify provenance limitations.

Related to #1498. Restart-intent coordinator contention remains unresolved. No guard bypass or broader retry policy.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b57fee8a-96bb-4dd3-8352-588f901fffa8
@karkarl karkarl added the clawsweeper:needs-security-review ClawSweeper marked this issue as needing security-sensitive review. label Sep 25, 2026
@clawsweeper clawsweeper Bot added P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 25, 2026
@clawsweeper

clawsweeper Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed September 25, 2026, 1:33 AM ET / 05:33 UTC (Revision 3).

ClawSweeper review

What this changes

After the WSL setup wizard restores Gateway reload mode, the branch rechecks managed endpoint ownership and retries the guarded Gateway CLI restart once for a specific serving-owner refusal, with tests and setup documentation.

Regression provenance

Possible regression — probable (reproduction; reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Needs changes before merge - 2 items remain

Keep this PR open. Current main still lacks the recovery, and the PR reports a successful current-head run against Gateway 2026.9.6. The prior documentation finding remains: it presents an unobserved owner-lease condition as fact. A maintainer has accepted the narrow retry policy, but the PR body still withholds merge authorization while GitHub marks it ready.

Priority: P0
Reviewed head: 77b7e4bb295e2c32df587c17e4e1a35ab0a2f428

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The focused patch has positive current-head runtime evidence and negative-case tests, but an unverified causal statement still blocks readiness.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): At this head, the PR body reports an isolated Windows ARM64 setup fixture using official Gateway 2026.9.6: the changed recovery warning appeared once, setup completed, and strict Gateway/MXC checks passed 17/17 without skips. The added fake-command cases supplement that real-path observation; this reviewer did not rerun the fixture. No stored-data contract changes.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): At this head, the PR body reports an isolated Windows ARM64 setup fixture using official Gateway 2026.9.6: the changed recovery warning appeared once, setup completed, and strict Gateway/MXC checks passed 17/17 without skips. The added fake-command cases supplement that real-path observation; this reviewer did not rerun the fixture. No stored-data contract changes.
Evidence reviewed 10 items Current-main gap: The pinned main revision returns a failed post-wizard restart without the proposed owner-refusal retry.
Introduced recovery: The pinned PR delta recognizes one refusal, checks the managed endpoint, and calls the existing restart helper once more.
Final-effect dependency: The restart helper executes the official openclaw gateway restart command in WSL and waits for health only after that command succeeds. This direct invocation makes the Gateway CLI admission contract relevant.
Findings 1 actionable finding [P2] Qualify the unobserved owner-lease explanation
Security None None.

How this fits together

The Windows Setup Engine runs the Gateway wizard inside an app-managed WSL installation. After the wizard, it restores reload settings, asks the Gateway CLI to restart the service, verifies the endpoint, and then lets setup continue or fail.

flowchart LR
A[Wizard completes] --> B[Restore reload mode]
B --> C{Guarded restart result}
C -->|Specific owner refusal| D[Check managed endpoint]
D --> E[Retry guarded restart once]
C -->|Other result| F[Setup outcome]
E --> F
Loading

Before merge

  • Qualify the unobserved owner-lease explanation (P2) - The linked diagnostics observed service states but did not expose the Gateway's rejected owner-lease predicate. Saying the lease “is absent” assigns an unverified cause and could misdirect investigation of lasting mismatches. Describe the reload handoff as a possible explanation. This previously raised finding remains on the unchanged reviewed head.
  • Complete next step (P2) - Correct the unverified lease claim in the setup documentation, then reconcile the PR body's draft/no-merge statement with its GitHub ready state before maintainer reassessment.

Findings

  • [P2] Qualify the unobserved owner-lease explanation — docs/SETUP_ENGINE_REDESIGN.md:14-17
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed lines production +18, tests +89, docs +15 The production change is narrow and has focused negative-case coverage; the documentation still needs one causal claim corrected.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #1498
Summary: This PR is a candidate repair for one refusal category in the canonical setup issue; typed coordinator contention remains separate.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Technical review

Best possible solution:

Keep the bounded guarded CLI recovery, qualify the unobserved lease explanation in the documentation, and track the separate coordinator-contention failure in #1498.

Do we have a high-confidence way to reproduce the issue?

Yes. Exact-main Windows/WSL setup runs and a disposable local fixture reproduced the specific post-wizard serving-owner refusal with Gateway 2026.9.6; the PR reports one successful current-head recovery of that refusal.

Is this the best way to solve the issue?

Yes for the code path: one endpoint-checked repeat through the existing guarded CLI is a narrow recovery for the observed refusal. The documentation must stop presenting the unexposed lease predicate as an established cause.

Full review comments:

  • [P2] Qualify the unobserved owner-lease explanation — docs/SETUP_ENGINE_REDESIGN.md:14-17
    The linked diagnostics observed service states but did not expose the Gateway's rejected owner-lease predicate. Saying the lease “is absent” assigns an unverified cause and could misdirect investigation of lasting mismatches. Describe the reload handoff as a possible explanation. This previously raised finding remains on the unchanged reviewed head.
    Confidence: 0.94
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 273b0182745a.

Labels

Label changes:

No label changes.

Label justifications:

  • P0: The linked official-Gateway failure blocks completion of first-time WSL setup after the wizard.
  • merge-risk: 🚨 security-boundary: The accepted extra restart attempt crosses the Gateway owner-admission boundary, so the existing final CLI guard remains important merge context.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): At this head, the PR body reports an isolated Windows ARM64 setup fixture using official Gateway 2026.9.6: the changed recovery warning appeared once, setup completed, and strict Gateway/MXC checks passed 17/17 without skips. The added fake-command cases supplement that real-path observation; this reviewer did not rerun the fixture. No stored-data contract changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. At this head, the PR body reports an isolated Windows ARM64 setup fixture using official Gateway 2026.9.6: the changed recovery warning appeared once, setup completed, and strict Gateway/MXC checks passed 17/17 without skips. The added fake-command cases supplement that real-path observation; this reviewer did not rerun the fixture. No stored-data contract changes.

Evidence

What I checked:

Likely related people:

  • shanselman: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • karkarl: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Revise the setup documentation to distinguish the observed refusal from the unexposed owner-lease predicate.
  • Align the PR body's draft and no-merge wording with the GitHub ready state before maintainer reassessment.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-25T00:40:15.087Z sha 77b7e4b :: blocked before merge. :: none
  • reviewed 2026-09-25T00:45:08.837Z sha 77b7e4b :: blocked before merge. :: [P2] Describe the owner-lease gap as a hypothesis

@karkarl
karkarl marked this pull request as ready for review September 25, 2026 00:41
@clawsweeper

clawsweeper Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@karkarl
karkarl requested a review from RomneyDa September 25, 2026 00:42
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 25, 2026
@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 25, 2026
@shanselman

Copy link
Copy Markdown
Collaborator

Hi Karen, Copilot here. Thanks for extracting this narrow recovery and for the current-head official Gateway 2026.9.6 proof: the changed warning appeared once, the guarded CLI retry completed setup, and strict Gateway/MXC ran 17 passed, 0 skipped. The separate StateDatabaseCoordinatorContentionError remains correctly outside this retry; the endpoint check is not being treated as proof of the upstream owner lease.

One concrete wording fix before readiness: docs/SETUP_ENGINE_REDESIGN.md says the live-owner lease is absent during a reload-triggered systemd restart. That causal predicate has not been observed. Our local #1498 diagnostic captured activating/auto-restart with no MainPID, while hosted generic refusals captured an active/running unit and live PID; the public Gateway CLI does not expose the rejected lease predicate. Please phrase reload-induced owner handoff as a possible timing explanation, not the established cause, and keep the distinction from intent-recording contention. For example: “Gateway 2026.9.6 may refuse the guarded restart when it cannot verify a live serving owner; the rejected lease predicate is not exposed. A reload-triggered supervisor transition is one possible explanation, but observed service states differ.”

Scott explicitly accepts one repeat through the normal guarded Gateway CLI after the exact serving-owner refusal and managed-endpoint recheck, provided both attempts keep Gateway owner/intent admission and other refusals (especially typed contention) fail closed. This is a policy disposition, not merge approval: your PR body currently says “Draft pending” / “No merge is authorized,” although GitHub marks the PR non-draft. Do you intend #1515 to remain a draft while you finish the documentation/review, or is it ready for a maintainer to reassess once those are aligned? I stopped without committing or pushing any changes.

Local supplemental check, not new-head proof: the full ARM64 build passed. One Shared-suite run failed the unrelated intermittent MCP disposal test; its exact focused rerun passed. I did not call the full required floor green or waive it after stopping at your draft/no-merge wording.

@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 25, 2026
@karkarl

karkarl commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator Author

Global triage: HOLD_FOR_AUTHOR. Take confidence 65%; recommendation confidence 98%; effort extra-small; risk medium.

The production retry remains narrowly bounded and guarded, and current-head proof is strong: build, Shared, Tray, SetupEngine, strict Gateway setup, and windows-wsl-mxc all passed, including 17 MXC tests with no skips. No significant code defect was found.

One documentation finding remains. docs/SETUP_ENGINE_REDESIGN.md states that the live-owner lease is absent, but the observed diagnostics do not expose that rejected lease predicate and showed differing service states. Describe reload-induced owner handoff as a possible timing explanation rather than an established cause.

Also reconcile the PR body's draft/no-merge wording with GitHub's non-draft ready state before maintainer reassessment. This is a policy and author-intent hold, not a request to broaden the accepted one-retry behavior. Restart-intent coordinator contention correctly remains outside this fix.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clawsweeper:needs-security-review ClawSweeper marked this issue as needing security-sensitive review. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gateway 2026.9.6 blocks WSL setup at guarded post-wizard restart

2 participants