Repository navigation
Conversation
Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
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 detailsDo 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:
Likely related people:
Codex review notes: model internal, reasoning medium; reviewed against 5e3fb40bcc89. |
|
Global triage: NEEDS_HUMAN_TEST. Take confidence 72%; recommendation confidence 88%; effort small; risk medium. Reviewed exact head |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5bd9a14b-aa47-42bb-ba42-9adf83300c8b
|
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>
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>
|
Global triage: NEEDS_HUMAN_TEST. Take confidence 55%; recommendation confidence 97%; effort small; risk low. Reviewed exact head 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. |
|
Thanks for working through this. I reverified head |
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
03bed856f2b000382cd101b9aed9b7f7158d8cd0the 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.ps1exit 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
Validation
Maintainer current-head validation
Validated on native Windows ARM64 with a process-local .NET SDK 10.0.400 at
57c8fb2595b0e6340a6c7d0895720f16fb5eaa4bon 2026-09-23 UTC. This is a normal merge of contributor head302ce2a18006e74f02230289b69452dd9f6dd6bdand main42c562f91a42ae0251ac971c44f6ce0f8028e88d, pushed non-force to the original contributor branch. No maintainer source changes. All three changed production blobs andSetupCloseCleanupTests.csare byte-identical to the contributor head; the complete five-file patch has the same stable patch ID,95a1a169f47719a612733a1794c13507966f5711.OPENCLAW_REPO_ROOTpointed to the isolated worktree. Tray data/AppData roots were task-private;OPENCLAW_STATE_DIR,OPENCLAW_TRAY_LOCALAPPDATA_DIR, andOPENCLAW_TRAY_LOCAL_DATA_DIRwere cleared. AllOPENCLAW_RUN_*flags were cleared. Each fresh test project was built before its--no-restoretest run.The focused
SetupCloseCleanupTestsboth passed within the full SetupEngine run (217 ms and 231 ms);SetupWindowOwnership_WaitsForCleanupBeforeAllowingAnotherRunalso 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 Gateway2026.9.6 (eb377ac)and arun-wizardrestart refusal:GATEWAY_RESTART_PREPARATION_REFUSED, unable to verify the live serving Gateway owner; the Gateway was not signaled. The comparedSetupPipeline,SetupWizardRunner,Program, andE2ESetupFixtureblobs are unchanged from main42c562f91a42ae0251ac971c44f6ce0f8028e88d. 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
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.
Real behavior proof
Current-head ENGINE FIXTURE ONLY
A session-only harness referenced the compiled production
SetupPipeline,SetupRunLock, andSetupCloseCleanupat57c8fb2595b0e6340a6c7d0895720f16fb5eaa4b. It used harmless synthetic steps and task-owned absolute marker/journal roots. All external command methods were fail-closed. Each scenario ran once:Observed in both scenarios: active-step cancellation, interrupted-step cleanup, completed-step
RollbackAsyncstart, owned-marker deletion and rollback completion, production cleanup-helper termination, lock release, then successful reacquisition. The journal recorded bothrollback_okentries beforepipeline_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, plusvalidation/{Shared,Tray,SetupEngine}.trx. These local artifacts are not uploaded/public links.On head
03bed856f2b000382cd101b9aed9b7f7158d8cd0the window does not register a second pipeline task. Progress-page disposal is the lifecycle already on main. This is not a nativeSetupWindow.Closedrecording.Not verified / blocked: no actual
ProgressPagepipeline cancellation by nativeSetupWindow.Closed, noWindowManagernext-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 andCleanupCompletedfinish, 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_ReleasesSetupLockOnlyAfterProgressRollbackFinishespassed 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_StillWaitsForPipelineWhenContextFaultspassed 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-createrollback_ok, distro removed). That is the setup-engine rollback, not this window'ssetup.lockpath.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.