Skip to content

fix(setup): release the setup lock only after rollback finishes - #1490

Closed
SebTardif wants to merge 6 commits into
openclaw:mainfrom
SebTardif:fix/setup-lock-after-rollback
Closed

SebTardif wants to merge 6 commits into
openclaw:mainfrom
SebTardif:fix/setup-lock-after-rollback

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Closing setup cancelled the progress page, and that cancel started rollback with CancellationToken.None. The window then disposed setup.lock and completed CleanupCompleted after only the Windows-node context task.

Why

A new setup could take the lock while the old run was still unregistering the distro.

User Impact

Window close uses ProgressPage.DisposeAsync as the only pipeline wait. That method cancels and awaits the pipeline, including settlement, before setup.lock is released. There is no second raw RunAsync registration.

Evidence

Red: SetupCloseCleanupTests failed 2 because cleanup finished while rollback was still running. Green: those 2 passed, plus 2 setup-window contract tests. .\build.ps1 passed.

On 03bed856f2b000382cd101b9aed9b7f7158d8cd0 the raw RunAsync registration and the extra close wait are removed. The close handler matches the merged main: it awaits page disposal, and that disposal awaits the pipeline, before releasing setup.lock. A Task.Run given the setup token stayed incomplete for 300 ms after cancel and finished only when its delegate finished. ./build.ps1 exit 0. Shared 4263 passed, 33 skipped. Tray 3996 passed. Native SetupWindow close was not recorded. These files have no remaining difference from the merged main.

Required proof pools

  • windows-wsl-gateway-e2e: setup close and rollback ordering changed.
  • windows-winui-interactive: close setup mid-pipeline and reopen it.

Validation

Maintainer current-head validation

Validated on native Windows ARM64 with a process-local .NET SDK 10.0.400 at 57c8fb2595b0e6340a6c7d0895720f16fb5eaa4b on 2026-09-23 UTC. This is a normal merge of contributor head 302ce2a18006e74f02230289b69452dd9f6dd6bd and main 42c562f91a42ae0251ac971c44f6ce0f8028e88d, pushed non-force to the original contributor branch. No maintainer source changes. All three changed production blobs and SetupCloseCleanupTests.cs are byte-identical to the contributor head; the complete five-file patch has the same stable patch ID, 95a1a169f47719a612733a1794c13507966f5711.

OPENCLAW_REPO_ROOT pointed to the isolated worktree. Tray data/AppData roots were task-private; OPENCLAW_STATE_DIR, OPENCLAW_TRAY_LOCALAPPDATA_DIR, and OPENCLAW_TRAY_LOCAL_DATA_DIR were cleared. All OPENCLAW_RUN_* flags were cleared. Each fresh test project was built before its --no-restore test run.

.\build.ps1
dotnet build .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --nologo -v:q
dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --logger "trx;LogFileName=Shared.trx" --results-directory $validationArtifacts
dotnet build .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --nologo -v:q
dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --logger "trx;LogFileName=Tray.trx" --results-directory $validationArtifacts
dotnet build .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --nologo -v:q
dotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --no-restore --logger "trx;LogFileName=SetupEngine.trx" --results-directory $validationArtifacts
Validation Passed Failed Skipped Total
Full repository build exit 0 0 N/A N/A
Shared 4096 0 35 4131
Tray 3069 0 0 3069
SetupEngine 1194 0 1 1195

The focused SetupCloseCleanupTests both passed within the full SetupEngine run (217 ms and 231 ms); SetupWindowOwnership_WaitsForCleanupBeforeAllowingAnotherRun also passed. These remain automated helper/source-contract evidence, not native window proof. The original five Tray failures below did not reproduce on this integrated head.

Current-head hosted CI: Build and Test run 35932360781 completed failed at 57c8fb2595b0e6340a6c7d0895720f16fb5eaa4b. The coordinator's bounded classification found that Setup and connect E2E job 107421976994 reported 23 failed tests from shared-fixture initialization, before their test bodies ran. The log identifies actual Gateway 2026.9.6 (eb377ac) and a run-wizard restart refusal: GATEWAY_RESTART_PREPARATION_REFUSED, unable to verify the live serving Gateway owner; the Gateway was not signaled. The compared SetupPipeline, SetupWizardRunner, Program, and E2ESetupFixture blobs are unchanged from main 42c562f91a42ae0251ac971c44f6ce0f8028e88d. That exact main's E2E job 107402954693 passed at 22:21:30 UTC on 2026-09-23. Root cause and attribution to this PR are not established. Local passes above remain valid; hosted CI is not green and the native proof gates below remain open. No CI rerun, local rerun, or source change was made for this classification. The existing authoritative source review was reused because the contributor patch is unchanged; no duplicate full review was launched.

Contributor validation at original head

  • .\build.ps1 passed.
  • SetupCloseCleanupTests: 2 passed.
  • Setup window contract tests: 2 passed.

Closeout

Original contributor closeout, retained for provenance. The current-head maintainer results above supersede this baseline.

Run on 2026-09-23 on this Windows host, head 302ce2a.

  • Shared: Passed 4093, Failed 0, Skipped 32, Total 4125.
  • Tray: Passed 3059, Failed 5, Total 3064. The five failures are the same source-contract checks on four unrelated heads (1488, 1489, 1490, 1491), so they are not this lock-ordering change:
    • ChatVoiceDialogs_RouteDisabledTtsCapabilityToPermissions_AndPreserveFallbackForMissingSetup
    • DebugPage_CopySpecificCards_HaveCopyGlyph_NotChevron_AndFeedback
    • SetupCodeEntry_ClearsStaleSshTunnelFields
    • CapabilitiesReview_PendingAvailabilityIsEscapedByToggleNotByBypassingContinue
    • CapabilitiesReview_SeparatesReasonActionFromDisabledOptions

Real behavior proof

Current-head ENGINE FIXTURE ONLY

A session-only harness referenced the compiled production SetupPipeline, SetupRunLock, and SetupCloseCleanup at 57c8fb2595b0e6340a6c7d0895720f16fb5eaa4b. It used harmless synthetic steps and task-owned absolute marker/journal roots. All external command methods were fail-closed. Each scenario ran once:

Scenario Result Lock denials sampled during rollback Cancel-to-lock-reacquire External command attempts
Healthy context Passed 25 424.10 ms 0
Faulted context Passed 27 430.67 ms 0

Observed in both scenarios: active-step cancellation, interrupted-step cleanup, completed-step RollbackAsync start, owned-marker deletion and rollback completion, production cleanup-helper termination, lock release, then successful reacquisition. The journal recorded both rollback_ok entries before pipeline_cancelled. The faulted context exception was observed only after rollback finished. The 400 ms synthetic rollback delay was intentional; these timings are not native setup close/reopen latency.

The loaded engine SHA256 matched the worktree build: 572DD75BFBC94B32674811B7AB6F465C96507314028F7B05A4D37FA0F0005E14. Session-local evidence: engine-proof/run-eca0f752e69f4e07bc40c111e70545ab/result.json, journal and pipeline logs, harness source, plus validation/{Shared,Tray,SetupEngine}.trx. These local artifacts are not uploaded/public links.

On head 03bed856f2b000382cd101b9aed9b7f7158d8cd0 the window does not register a second pipeline task. Progress-page disposal is the lifecycle already on main. This is not a native SetupWindow.Closed recording.

Not verified / blocked: no actual ProgressPage pipeline cancellation by native SetupWindow.Closed, no WindowManager next-window observation, no real distro rollback, and no native close-to-reopen measurement. Shared-host native WSL/GUI execution is paused pending host-safety disposition. No WSL, GUI, probe, or actual setup was launched by this maintainer validation lane. Both declared proof pools remain open.

The existing setup E2E fixture provisions headlessly before launching the tray; setup preview bypasses the pipeline. Neither proves this native lifecycle. Remaining proof needs a separately approved disposable WSL/WinUI host: start a task-owned real setup, close while rollback is active, request another setup through WindowManager, confirm lock rejection/no replacement window until rollback and CleanupCompleted finish, then observe reopening and measure elapsed time. A safe synthetic native fixture would require a separately agreed test-only pipeline-injection seam; none was added.

Contributor evidence at original/related heads

Host: WSL 2.7.14 with Ubuntu 24.04.5. Head 302ce2a, 2026-09-23.

  • Close_ReleasesSetupLockOnlyAfterProgressRollbackFinishes passed in 220 ms. Cleanup was still waiting at the 200 ms check, the second acquire failed while rollback was in flight, and the lock could be acquired again after rollback completed.
  • WaitForRunningWork_StillWaitsForPipelineWhenContextFaults passed in 211 ms. A faulted context task does not release the wait before rollback finishes.

A live setup on the related head f2f7c33 created OpenClawE2E-782a01ca, then rolled it back after the wizard restart step failed (wsl-create rollback_ok, distro removed). That is the setup-engine rollback, not this window's setup.lock path.

Not measured: WinUI close of a visible setup window, and the wall time of that window's full rollback. The unit wait above is 200 ms by construction, then about 20 ms to release.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 23, 2026
@clawsweeper

clawsweeper Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close this PR: GitHub reports no changed files against the current base branch.

So I’m closing this here because the remaining work is already tracked in the canonical issue.

Review details

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

No current-main failure was reproduced. Source inspection shows cancellation and rollback are awaited before lock release; no native execution was performed in this read-only review.

Is this the best way to solve the issue?

Yes, the existing disposal lifecycle is the narrow maintainable solution. The current branch adds no competing implementation or remaining repair.

Security review:

Security review cleared: The empty introduced patch changes no authority, credentials, persistence, dependencies, or execution paths.

AGENTS.md: found and applied where relevant.

What I checked:

  • live no-diff PR: GitHub reports changed_files: 0 for this open PR, so there is no remaining branch diff to merge.

Likely related people:

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

Codex review notes: model internal, reasoning medium; reviewed against 5e3fb40bcc89.

@karkarl

karkarl commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Global triage: NEEDS_HUMAN_TEST. Take confidence 72%; recommendation confidence 88%; effort small; risk medium.

Reviewed exact head 302ce2a18006. The main ordering defect is real and regressions enforce cancellation, rollback completion, then lock release. Blocking proof is windows-winui-interactive plus windows-wsl-gateway-e2e: close setup mid-pipeline, show rollback finishing, lock release afterward, and immediate setup reopen. Record close latency because WindowManager now awaits full rollback without an outer timeout; bounded steps can total minutes. Add required Shared results and clean Validation formatting.

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. label Sep 23, 2026
@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 23, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5bd9a14b-aa47-42bb-ba42-9adf83300c8b
@shanselman shanselman added status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. and removed status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. labels Sep 23, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 24, 2026
@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 24, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

Head 57c8fb2 keeps the lock until rollback finishes. Local SetupCloseCleanupTests: 2 passed (514 ms). Close_ReleasesSetupLockOnlyAfterProgressRollbackFinishes still holds the lock at the 200 ms check, rejects a second acquire while rollback is in flight, and allows acquire again after rollback completes.

The PR body engine fixture on this head recorded the same order, then reacquire at 424.10 ms and 430.67 ms. That timing includes a 400 ms synthetic rollback delay, so it is not native window close latency. Shared on the maintainer pass: 4096 passed, 0 failed. Tray: 3069 passed, 0 failed.

A visible WinUI close mid-pipeline, and a live distro rollback measured from that window, were not run.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
A faulted context or pipeline task used to leave the close handler
before page disposal, secret clearing, and native-session release.
The wait now sits in a finally, so those steps still run, and
setup.lock is released only after they finish.

SetupCloseCleanupTests 3 passed. build.ps1 exit 0. Shared 4263
passed, 33 skipped. Tray 3996 passed.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. label Oct 7, 2026
Window close already awaits ProgressPage.DisposeAsync before it
releases setup.lock. That method cancels and awaits the pipeline,
including settlement. The extra raw RunAsync registration waited
on a different task, so it is removed.

A Task.Run started with the setup token stayed incomplete for 300 ms
after that token was cancelled, then finished when the delegate
finished. These files now match the merged main.

build.ps1 exit 0. Shared 4263 passed, 33 skipped. Tray 3996 passed.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
Core failed one timing check. The post-exit output drain took 1605 ms
and the bound is 1 second. The setup files on this head match main.
Rerun of the failed job needs admin rights.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Oct 7, 2026
@karkarl

karkarl commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

Global triage: NEEDS_HUMAN_TEST. Take confidence 55%; recommendation confidence 97%; effort small; risk low.

Reviewed exact head d2d1294da99d. GitHub currently reports CLEAN with 24 passing, 0 failing, and 0 pending checks. needs-proof label is present. Durable automated signal: No durable ClawSweeper verdict found.

Maintainer or proof-pool owner: run every declared Windows proof pool at this exact head and replace blocked or historical evidence with current-head results.

This is a triage classification, not a merge action. No branch changes were made.

@shanselman

Copy link
Copy Markdown
Collaborator

Thanks for working through this. I reverified head d2d1294da99d against current main cc20426854e3: GitHub reports zero changed files, and the local merge-base-to-head diff is also empty. Current main already awaits ProgressPage.DisposeAsync (which cancels and awaits pipeline settlement) before disposing setup.lock, so there is no remaining unique patch to merge. Closing this as no-diff/superseded.

@shanselman shanselman closed this Oct 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants