Skip to content

Fix setup wizard runtime spinner and prepare 1.0.2 hotfix - #1254

Merged
malpern merged 3 commits into
masterfrom
fix/wizard-runtime-transient-refresh
Jul 30, 2026
Merged

Fix setup wizard runtime spinner and prepare 1.0.2 hotfix#1254
malpern merged 3 commits into
masterfrom
fix/wizard-runtime-transient-refresh

Conversation

@malpern

@malpern malpern commented Jul 30, 2026

Copy link
Copy Markdown
Owner

What changed

  • Recheck runtime status once per second only while the service page is in a transient startup state.
  • Stop polling immediately when the runtime settles.
  • Cap transient checks at 30 and surface an actionable Restart state instead of an endless spinner.
  • Remove the obsolete disabled timer scaffolding.
  • Bump the hotfix to KeyPath 1.0.2 build 13.

Root cause

The runtime service page performed a single status observation. Its broad auto-refresh timer had previously been disabled to avoid invasive permission checks. If the one observation landed during the brief runtime startup window, the page rendered “Starting KeyPath runtime…” forever even after the runtime became healthy.

User impact

Users who launch KeyPath during that transition can now progress automatically once the already-healthy runtime is observed. The retry is bounded and runtime-only; it does not restore broad permission polling.

Validation

  • Reproduced on the notarized 1.0.1 build while the CLI and validator showed a healthy runtime.
  • Relaunching the UI confirmed the runtime remained continuously operational and the main interface appeared.
  • swift test --filter ServiceStatusEvaluatorActionTests — 7 passed.
  • Pinned SwiftFormat 0.61.1 lint — passed.
  • plutil -lint Sources/KeyPathApp/Info.plist — passed.
  • git diff --check — passed.

@malpern
malpern marked this pull request as ready for review July 30, 2026 21:53
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Bug: timeout path in refreshStatus() only handles .starting, not the transient .stopped case

In WizardKanataServicePage.refreshStatus() (Sources/KeyPathInstallationWizard/UI/Pages/WizardKanataServicePage.swift), the bounded-retry loop calls ServiceStatusEvaluator.shouldRetryTransientStatus, which retries in two situations: runtimeStatus == .starting, and runtimeStatus == .stopped && isInTransientStartupWindow. Once completedAttempts hits transientRefreshAttemptLimit (30), shouldRetry becomes false for both cases and the loop exits.

But the post-loop "did not finish" fallback only fires for the .starting case:

guard shouldRetry else {
    if completedAttempts >= ServiceStatusEvaluator.transientRefreshAttemptLimit,
       runtimeStatus == .starting
    {
        await MainActor.run {
            serviceStatus = .failed(error: "Runtime startup did not finish. Click Restart to retry.")
        }
    }
    return
}

If the runtime stays .stopped with isInTransientStartupWindow == true for all 30 attempts (~30s of polling), the loop exits silently with no serviceStatus update — the wizard is left showing whatever transient/starting UI state applyStatusUpdate last set, with no failure message and no prompt to retry. That's the same "stuck spinner" bug this PR is otherwise trying to fix, just for the stopped/transient branch instead of the starting branch.

Suggested fix: broaden the condition to cover both timeout branches, e.g. reuse the same predicate used for retrying:

if completedAttempts >= ServiceStatusEvaluator.transientRefreshAttemptLimit,
   runtimeStatus == .starting || (runtimeStatus == .stopped && isInTransientStartupWindow)
{
    ...
}

Also note the new tests (ServiceStatusEvaluatorActionTests) only cover shouldRetryTransientStatus itself; they don't exercise this timeout-message logic in WizardKanataServicePage, so this gap wasn't caught.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 04731b0961

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +335 to +336
if completedAttempts >= ServiceStatusEvaluator.transientRefreshAttemptLimit,
runtimeStatus == .starting

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Finish capped retries for transient stopped states

When currentRuntimeStatus() returns .stopped while isInTransientRuntimeStartupWindow() remains true, the 30th observation has already mapped the UI to .starting, but this condition handles only a literal .starting runtime status and then returns with no polling task left. This occurs during the 140-second initial/start grace window and indefinitely for pending SMAppService approval, so a runtime that becomes healthy after the cutoff is never observed and the setup page remains on the spinner without exposing Start or the required approval action. Continue through the authoritative startup window or transition both transient .starting and .stopped states to an explicit terminal state at the cap.

AGENTS.md reference: AGENTS.md:L311-L316

Useful? React with 👍 / 👎.

@malpern

malpern commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

Addressed in 45e0d3c55: retry and exhaustion now share one transient-state predicate, covering both .starting and .stopped inside the startup window. Added a regression assertion proving transient .stopped reaches the actionable timeout state, then reran the focused suite (7/7) and pinned formatting.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review notes on the WizardKanataServicePage/ServiceStatusEvaluator retry-loop change:

  1. Reintroduces the polling behavior the code previously disabled on purpose. The deleted startAutoRefresh() had an explicit comment: "DISABLED to prevent invasive permission checks that cause KeyPath to auto-add to Input Monitoring system preferences." The new refreshStatus() loop calls kanataManager.currentRuntimeStatus() and isInTransientRuntimeStartupWindow() in a while loop, up to 30 times at 1s intervals, whenever the runtime is .starting or transiently .stopped. If those underlying calls are the same ones that triggered the invasive permission-check side effect, this change effectively re-enables repeated polling during the exact window (service startup) where that problem was most likely to occur. Worth confirming currentRuntimeStatus()/isInTransientRuntimeStartupWindow() are safe to call repeatedly (no permission-prompt side effects) before merging, since the prior fix wasn't just inert dead code — it was deliberately neutered for this reason.

  2. Possible overlapping retry loops. refreshTask is only cancelled in onDisappear. If refreshStatus() is invoked again from another call site (e.g. after a lifecycle action) while a previous transient-retry loop is still mid-flight (it can now run for up to 30s), there's no visible guard cancelling the prior task before starting a new one. Two loops writing to serviceStatus/actionStatus concurrently could race and flicker between stale and fresh results. If the call sites outside this diff already cancel refreshTask before reassigning, disregard, but it's not evident from this diff.

  3. Test coverage only exercises the pure shouldRetryTransientStatus/didExhaustTransientStatus functions, which is good, but there's no test around the loop's cancellation/exhaustion integration in WizardKanataServicePage itself (e.g. that a second refreshStatus() call cancels/replaces the first, or that hitting the attempt cap actually surfaces the "Runtime startup did not finish" failure state end-to-end).

Nothing else stood out; the pure-function logic in ServiceStatusEvaluator itself (attempt-limit bookkeeping) looks correct and is well covered by the new tests.

@malpern

malpern commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

Follow-up on the latest review notes:

  1. Confirmed the retry path is runtime-only and does not invoke permission checks. RuntimeCoordinator.currentRuntimeStatus() delegates to ServiceLifecycleCoordinator.currentRuntimeStatus(), which calls KanataDaemonService.refreshStatus() (cached SMAppService.status + PID evidence). isInTransientRuntimeStartupWindow() uses timestamps and cached SMAppService management state. Neither calls PermissionOracle or a TCC mutation path.
  2. Overlapping loops are prevented: WizardKanataServicePage.refreshStatus() begins with refreshTask?.cancel() before assigning the replacement task; onDisappear also cancels it.
  3. Exhaustion is now a shared pure decision (didExhaustTransientStatus) used directly by the page, with regression coverage for the previously missed transient .stopped case.

No additional permission-polling surface is being restored.

@malpern
malpern merged commit 71021e0 into master Jul 30, 2026
3 checks passed
@malpern
malpern deleted the fix/wizard-runtime-transient-refresh branch July 30, 2026 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant