fix(setup): retry guarded restart intent contention - #1507
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 28, 2026, 3:06 PM ET / 19:06 UTC (Revision 9). ClawSweeper reviewWhat this changesThe setup engine adds one guarded Gateway CLI restart retry after exact coordinator contention, while the disposable WSL test fixture records bounded service diagnostics before rollback. Regression provenancePossible regression — suspected (failure trace). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 5 items remain Current main still lacks recovery for the typed restart-intent contention that blocks WSL setup. The earlier proc-path logging finding is fixed, but the prior approval of a guarded retry explicitly excluded typed contention. This branch still needs approval for that expanded boundary and a real setup result showing the new retry path. Priority: P0 Review scores
Verification
How this fits togetherWindows setup completes the Gateway wizard inside an app-owned WSL distro, restores its reload setting, and asks the Gateway CLI to restart the service. Endpoint checks and Gateway health determine whether setup continues or rolls back. flowchart LR
A[Wizard completes] --> B[Restore reload setting]
B --> C[Gateway CLI restart]
C --> D{Classified refusal?}
D -->|Yes| E[Check managed endpoint]
E --> F[One guarded CLI retry]
D -->|No| G[Health or rollback]
F --> G
Decision needed
Why: The new attempt stays behind the CLI guard, but accepting an additional restart after admission contention is a security-boundary policy choice that the earlier decision did not cover. Before merge
Findings
Agent review detailsSecurityNeeds attention: The retry still uses the guarded CLI and the proc-path logging fix is present, but the newly retried admission case needs an explicit boundary decision. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the Gateway CLI as the final restart authority and provide an in-product Fix or retry path with redacted diagnostics when contention persists. Do we have a high-confidence way to reproduce the issue? Yes. Hosted setup on main with Gateway 2026.9.6 recorded the exact typed contention and restart-intent refusal, and current main still lacks this recovery branch. Is this the best way to solve the issue? Unclear. Repeating the same guarded CLI once is a narrow local repair, but its exact real-world recovery and expanded boundary have not yet been established. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5e0b31ab830a. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (8 earlier review cycles)
|
|
CI run 35993290413 on official Gateway 2026.9.6 produced three pre-rollback allowlisted diagnostics. Setup/connect job 107612506610 recorded typed Every fixture retained fail-closed cleanup: setup journal Review: rubber-duck found no blocking issues. Structured |
|
Global triage: NEEDS_HUMAN_TEST. Take confidence 80%; recommendation confidence 92%; effort small; risk low. Reviewed exact head Owner: maintainer. Inspect the three |
|
Current-head local behavior proof (reviewed head Setup and uninstall completed their owned cleanup (19 and 34 |
|
Exact-head hosted closeout for #1507 at
This contrasts with the separate task-owned local generic refusal where the selected unit was |
|
Public #1498 (Gateway 2026.9.6 blocks WSL setup at guarded post-wizard restart) and this diagnostic PR remain open; this diagnostic is not a compatibility fix and should not be merged to claim closure. The exact-main run already covered all three lanes. No rerun or raw artifact disclosure is needed. |
…back Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1533edb3-7b37-405a-9829-959466a93b54
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
60bc21e to
a2ce88e
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 504f71d1-79b6-4d69-abd3-cbb098ca9e0b
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1cae2b28-e7ec-42b3-96d8-cddf8565243e
…start-fixture-diagnostics
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1cae2b28-e7ec-42b3-96d8-cddf8565243e
|
@clawsweeper re-review |
|
🦞👀 I queued a lightweight read-only assist pass. It will post a separate answer comment and will not edit the durable ClawSweeper review comment or trigger close, merge, repair, label, or branch changes. Request: re-review |
|
@clawsweeper re-review Maintainer decision: approve the expanded bounded retry for the exact typed coordinator-contention plus restart-intent refusal. The accepted boundary is narrow: both exact markers are required, setup waits 500 ms, managed endpoint provenance must pass, and the same guarded Gateway CLI restart is attempted once. Persistent contention, either marker alone, unrelated failures, and unknown/conflicting listeners remain terminal. There is no direct systemd fallback, ownership bypass, or lock deletion. The timing-dependent typed contention did not recur in current-head hosted artifacts, so I am explicitly accepting deterministic exact-path tests plus current-head real Gateway boundary proof, strict MXC 17/17, all three hosted WSL lanes, security checks, clean autoreview, and the resolved proc-path finding as sufficient release evidence. ClawSweeper is advisory rather than final merge authority; no actionable code finding remains. |
|
🦞👀 Re-review progress:
|
Summary
For #1498, this PR adds a fixture-only, pre-rollback diagnostic for guarded post-wizard Gateway restart failures. The allowlisted artifact records coarse selected user-service state, PID liveness/equality, restart count, and bounded service result/exit facts without writing raw PIDs, paths, process arguments, config, environment, credentials, or raw command output.
The diagnostic identified a distinct hosted failure: the Gateway CLI refused restart-intent recording because another OpenClaw process owned
state-lifecycle. The setup engine recognizes only the combination of the typed coordinator-contention marker and the exact restart-intent refusal. It waits 500 ms, re-verifies expected managed endpoint provenance, and retries the same guardedopenclaw gateway restartcommand exactly once.The #1515 serving-owner recovery remains intact. Persistent contention, unrelated restart failures, and unknown or conflicting listeners still fail closed. There is no direct
systemctl restart, ownership bypass, lock deletion, or relaxed Gateway CLI admission.The remaining ClawSweeper security finding is fixed. The fixture now redirects Bash stderr before opening the racy
/proc/<pid>/statinput, so a disappearing process cannot leak a raw proc path into setup command logs or uploaded artifacts.Required proof pools
windows-wsl-gateway-e2e: setup, pairing, post-wizard restart, revocation recovery, and network recovery cross the real WSL Gateway boundary.windows-wsl-mxc: strict real Gateway to Windows nodesystem.runcontainment proof is required by repository policy for setup/connect changes.Validation
Current head:
f6fab60a5cc7049981b7df1cbe99348fa162179b.$env:OPENCLAW_REPO_ROOT=(Get-Location).Path; .\build.ps1: passed. All five projects and documentation validation succeeded.dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore: 4,115 passed, 32 skipped, 0 failed.dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore: 3,154 passed, 0 failed.dotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --no-restore: 1,206 passed, 1 skipped, 0 failed. Coverage includes exact two-marker classification, one successful guarded retry, repeated-failure boundedness, and rejection of unknown/conflicting listeners.dotnet test .\tests\OpenClaw.E2ETests\OpenClaw.E2ETests.csproj --no-restore --filter FullyQualifiedName~GatewayRestartFailureDiagnosticTests: 12 passed, 0 failed..\scripts\validate-mxc-e2e.ps1with process-onlyTEMP/TMPset to a task-ownedD:\directory: 17 passed, 0 failed. Strict proof did not use-AllowSkip..\scripts\Invoke-CiE2e.ps1 -Name revocation-recovery -Filter FullyQualifiedName~OpenClaw.E2ETests.Setup.RevocationAndRecoveryTests: 1 passed, 0 failed..\scripts\Invoke-CiE2e.ps1 -Name network-recovery -Filter FullyQualifiedName~OpenClaw.E2ETests.Setup.NetworkRecoveryTests: 2 passed, 0 failed.FullSetup_TrayConnects_OperatorAndNode: 1 passed, 0 failed. The narrowed wrapper then intentionally returned nonzero because its filter omitted the separate required MXC sentinel; strict MXC passed independently above.python .agents\skills\autoreview\scripts\autoreview --mode branch --base origin/main ...: clean, no accepted/actionable findings, overall confidence 0.96.Real behavior proof
read -r stat 2>/dev/null < /proc/999999999/statreturned stdoutsuppressed, empty stderr, and exit code 0. The regression test requires this redirection order and rejects the prior unsafe order.Restarted systemd service: openclaw-gateway.service, reached HTTP 200, and completed the wizard step successfully. No direct systemd fallback was used by setup.system.runexecuted through the Windows node MXC sandbox, tray-data writes were blocked, and the mirrored WSL-safe Gateway port was listening and recorded. All 17 strict tests passed.The task-owned
D:\validation temp directory was removed after all disposable distros were unregistered. No secret-bearing settings or gateway artifacts are attached.