Suppress transient startup TCP errors

[malpern]

Summary

• classify TCP connection loss during an intentional Kanata runtime transition as pending instead of a reload failure
• retry the persisted configuration once the replacement runtime is healthy
• show Starting… / Restarting… during the canonical startup grace window, while preserving No TCP for settled failures
• add focused lifecycle and overlay-state regression coverage plus a durable bug note

Root cause

Startup bootstrap could begin a TCP reload against the old Kanata process just before stale-runtime recovery replaced it. The old process closed the acknowledged reload connection during termination, and KeyPath surfaced that expected transport churn as a configuration error.

User impact

Normal runtime replacement no longer produces the false Reload delayed: Connection failed: Connection closed toast, error sound, or transient No TCP state. Real validation errors and network failures outside an intentional transition remain visible.

Validation

• swift build --jobs 4
• TEST_FILTER=ConfigReloadCoordinatorTests ./Scripts/run-tests-safe.sh
• TEST_FILTER=OverlayTCPStatusPresentationTests ./Scripts/run-tests-safe.sh
• python3 Scripts/check-accessibility.py
• SwiftFormat 0.61.1 and git diff --check
• ./Scripts/review-gate.sh selected the enforced remote claude-review gate

A broad local safe-suite run also completed; unrelated existing failures were three Home Row snapshot height mismatches and one timing-sensitive canonical capture-timeout assertion. The new parallel notification test issue found in that run was fixed and its full coordinator suite was rerun successfully.

[claude]

Reviewed the diff. Two actionable points in ConfigReloadCoordinator.swift:

1. Uncoordinated retry tasks can double-fire a reload. deferredReloadTask (existing cooldown-based retry) and the new transitionReloadTask (runtime-transition retry) are independent Task properties, and neither cancels the other when scheduled. If a cooldown retry is already pending and a transition-related network error also occurs (or vice versa) close in time, both loops can independently resolve and trigger a reload, causing a duplicate/redundant reload call. Consider cancelling the sibling task in scheduleDeferredReloadAfterCooldown()/scheduleDeferredReloadAfterRuntimeTransition() before scheduling.

2. The new retry loop itself has no test coverage. scheduleDeferredReloadAfterRuntimeTransition() returns immediately when TestEnvironment.isRunningTests is true, so the two new tests (transitionNetworkFailureStaysPending, settledNetworkFailureStillNotifies) only verify the immediate .pending disposition and notification suppression — they never exercise the actual polling loop (waiting for isRuntimeTransitioning() to clear, checking health, calling the reload retry, or the grace-period-exceeded warning path). Given this is the core new behavior described in the bug doc, it'd be worth adding coverage for it (e.g., by extracting the loop's dependencies so they're injectable/fake-clock-driven instead of being fully short-circuited in tests).

Everything else (the OverlayTCPStatusPresentation state machine, the TCPReloadResult: Sendable conformance, and the header grace-period UI logic) looks correct and is reasonably well covered by the new OverlayTCPStatusPresentationTests.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: af3e712720

ℹ️ 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".

[chatgpt-codex-connector]

Anchor TCP grace to the runtime transition

When the overlay window is first created or recreated after the app has already been running, both fallback timestamps describe header/UI lifecycle rather than Kanata startup. If Kanata's process is running but its event TCP connection is in a settled failed state, opening the overlay starts a fresh 140-second grace period and displays Starting… instead of No TCP; dismissing the health indicator similarly restarts that grace. Base the deadline on actual runtime startup/transition state or its timestamp so creating or dismissing UI cannot mask an established disconnection.

Useful? React with 👍 / 👎.

[claude]

Reviewed the diff. One correctness/reliability concern and one minor UX nit:

Silent failure when the runtime never recovers (ConfigReloadCoordinator.swift, scheduleDeferredReloadAfterRuntimeTransition/retryAfterRuntimeTransition)

When a TCP .networkError occurs while isRuntimeTransitioning() is true, triggerConfigReload() now returns .pending and suppresses .configReloadFailed — correct for the transient "Kanata is being replaced" case. But if the runtime never comes back healthy (e.g. a genuine permissions/start failure that keeps isStartingKanata true, or Kanata crash-loops), retryAfterRuntimeTransition() exhausts transitionRetryMaximumPolls and scheduleDeferredReloadAfterRuntimeTransition only logs a warning — no .configReloadFailed (or any other user-facing) notification is ever posted:

let didRetry = await retryAfterRuntimeTransition()
if !didRetry, !Task.isCancelled {
    AppLogger.shared.warn(...)  // only a log, nothing user-visible
}

Previously, any TCP network error surfaced immediately via .configReloadFailed. Now a persistent failure that merely happens to start with a network error during a transition window silently drops the pending config write with no toast/notification — only the transient "Restarting…" pill, which fades once the startup grace period elapses. Worth posting .configReloadFailed (or similar) when didRetry == false so genuine failures aren't swallowed.

Minor: kanataTransitionPill (OverlayDragHeader+LayerPicker.swift) hardcodes .help("Kanata is starting") for both the "Starting…" and "Restarting…" labels — the tooltip won't match the visible text in the restart case.

[claude]

Reviewed the diff (runtime-transition-aware reload handling + overlay TCP status). Overall solid — the transition/cooldown distinction is well tested. Two smaller points worth a look:

1. ConfigReloadCoordinator.retryAfterRuntimeTransition() — possible off-by-one on the last poll (Sources/KeyPathAppKit/Managers/ConfigReloadCoordinator.swift): each loop iteration only checks health when isRuntimeTransitioning() is already false at the start of that iteration, then unconditionally calls transitionRetryWait(). If the transition flag flips to false exactly during the final wait (i.e., right after the last iteration's check but before the loop exits), the function falls through to the failure branch and posts .configReloadFailed even though the runtime may now be healthy. Consider one more health check immediately after the loop (before giving up) to close this race, since the whole point of the retry is to avoid a false failure notification during expected transport churn.

2. NotificationCenter posts switched from object: nil to object: self for .configReloadRecovered and .configReloadFailed. This looks motivated by test isolation (avoiding cross-talk between ConfigReloadCoordinator instances in parallel tests), and is harmless for observers registered with object: nil (the common pattern). Worth a quick grep across the app for any observer that filters on a specific non-nil object for these two notification names — if one exists, this change would silently stop delivering to it.

Test coverage for the new transition-retry paths and the OverlayTCPStatusPresentation.resolve state machine looks thorough. No other correctness/security issues spotted.

[claude]

Review of the reload/runtime-transition changes

1. deferredReloadTask can be clobbered by a stale, already-cancelled task (ConfigReloadCoordinator.swift) — scheduleDeferredReloadAfterRuntimeTransition()'s spawned task unconditionally does deferredReloadTask = nil at the end of its closure, with no check that it's still the task stored in the property. Sequence: task A starts → a new deferred-reload request arrives (cooldown or another transition) and calls deferredReloadTask?.cancel() then assigns task B → task A observes Task.isCancelled at its next loop-top check, bails out via return false, but still runs deferredReloadTask = nil on the way out, wiping out the reference to task B. Task B keeps running but is now untracked, so the "de-duped via a single outstanding task" invariant the comment above the property describes is broken — a later scheduleDeferredReloadAfterCooldown()/scheduleDeferredReloadAfterRuntimeTransition() call won't cancel it, and overlapping reload attempts (and possibly duplicate notifications) become possible. Worth guarding the reset with an identity check (e.g. capture the task locally and only nil the property if it still equals self/still points at the same task).

2. retryReloadIfRuntimeReady() discards the outcome of the retried reload (ConfigReloadCoordinator.swift) — once health looks good it calls await triggerReload() and unconditionally returns true, regardless of whether that reload actually succeeded. retryAfterRuntimeTransition() treats true as "handled" and skips the "runtime did not become ready" warning/second attempt. If the retry itself hits another transient failure right as the runtime flips healthy, the persisted config edit can silently fail to reach kanata with no further retry and no visible warning. Consider checking the reload result before treating the retry as resolved.

Nothing else jumped out — the notification object: change (nil → self) and the new OverlayTCPStatusPresentation state machine look correct and are reasonably covered by the added tests.

[claude]

Review: potential self-cancellation race in the deferred-retry mechanism (Sources/KeyPathAppKit/Managers/ConfigReloadCoordinator.swift)

scheduleDeferredReloadAfterRuntimeTransition() / scheduleDeferredReloadAfterCooldown() both do deferredReloadTask?.cancel(); deferredReloadTask = Task { ... }, and the body of that task (retryAfterRuntimeTransition() → retryReloadIfRuntimeReady() → triggerConfigReload(notifyOnFailure: false)) can itself hit the networkError && isRuntimeTransitioning() branch and call scheduleDeferredReloadAfterRuntimeTransition() again (or the cooldown variant, if isCooldownBlockMessage matches).

Since retryReloadIfRuntimeReady() only checks isRuntimeTransitioning() once before the awaits (health check + TCP round trip), the runtime can flip back into a transitioning state during that window. If the retry's own TCP call then fails, the coordinator cancels the currently-executing deferredReloadTask (itself) and replaces it with a brand-new task that restarts polling from 0. Net effect: the poll counter resets, the "bounded by grace period" guarantee described in docs/bugs/2026-07-28-startup-reload-runtime-replacement.md can be extended past transitionRetryMaximumPolls, and duplicate AppLogger lines/notifications can be emitted for what is logically one retry sequence. It's a narrow window (needs a second transition to start mid-retry), but worth either guarding re-entrant scheduling (e.g. skip cancel-and-replace if the call originates from the task's own execution) or documenting it as accepted behavior.

No other blocking issues found — the pending/disposition handling, Sendable fix on TCPReloadResult, and the new overlay starting/restarting/disconnected presentation logic all look correct and are well covered by the added tests.

[claude]

Reviewed the diff — one correctness issue worth a look before merge.

In ConfigReloadCoordinator.swift, retryReloadIfRuntimeReady() calls triggerConfigReload(notifyOnFailure: false, scheduleRetryOnPending: false) while polling after a runtime transition. But triggerConfigReload's failure branch only handles the "silent" case for cooldown-blocked and network-error-during-transition failures:

if isCooldownBlockMessage(errorMessage) {
    if scheduleRetryOnPending { scheduleDeferredReloadAfterCooldown() }
} else if notifyOnFailure {
    NotificationCenter.default.post(name: .configReloadFailed, ...)
}

If the retry attempt fails for a persistent reason — e.g. a genuine config validation error surfaced as .failure(error:, response:), not .networkError — this branch does nothing at all: no notification, no reschedule. The failure is silently swallowed, and retryReloadIfRuntimeReady just returns false, so retryAfterRuntimeTransition's outer loop keeps polling for the full transitionRetryMaximumPolls window as if Kanata simply hasn't become healthy yet. Only after the grace period expires does the user see a notification, and it's the generic "Kanata did not become ready after restarting; config reload was not applied" — not the actual validation error that caused the failure.

Net effect: a real, permanent config problem hit during the post-transition retry window gets misreported as a transient startup/restart timing issue, and the specific error message is lost. Given the whole point of this PR is to make TCP-churn-during-restart distinguishable from real failures, it'd be good to have retryReloadIfRuntimeReady (or triggerConfigReload) distinguish "still not ready" (network error / cooldown, keep polling) from "reload attempted and failed for a real reason" (surface immediately with the actual message) rather than treating all non-success results the same way during the retry loop.

No tests currently cover a persistent/validation failure occurring inside the transition-retry window — worth adding one alongside the fix.

Everything else (the Sendable conformance on TCPReloadResult, the object: self notification changes, the overlay Starting…/Restarting… presentation logic) looks correct and is well covered by the new tests.

[claude]

Reviewed the diff. The runtime-transition-aware reload retry logic in ConfigReloadCoordinator is well covered by the new tests, and the overlay TCP status presentation logic (OverlayTCPStatusPresentation.resolve) is cleanly factored and unit-tested. No blocking correctness/security issues found. Two minor, non-blocking observations:

1. Misleading terminal error message (ConfigReloadCoordinator.retryAfterRuntimeTransition): when all polls are exhausted, the failure is always reported as "Kanata did not become ready after restarting; config reload was not applied". But retryReloadIfRuntimeReady() also returns .pending when the runtime is healthy and not transitioning but the reload itself keeps hitting the cooldown throttle (case .pending, .failed: return .pending). In that scenario the message misattributes the failure to Kanata not restarting rather than persistent cooldown throttling, which could confuse debugging.

2. Stale deferredReloadTask reference (scheduleDeferredReloadAfterCooldown): the deferredReloadTask = nil reset after firing was removed in this diff, so the property now keeps holding a reference to an already-completed Task until the next schedule call overwrites it. Harmless (.cancel() on a finished task is a no-op) but was presumably intentional cleanup previously — worth confirming the removal was deliberate and not incidental.

Nice test coverage overall, including the shared-task coalescing behavior between cooldown and runtime-transition retries.

[claude]

Reviewed the diff. A few findings on the new runtime-transition reload logic:

1. Race in transition-failure classification (ConfigReloadCoordinator.triggerConfigReload): isRuntimeTransitioning() is sampled after the TCP round trip fails, not at the time the request was issued. If the intentional transition (isStartingKanata/isIntentionalTransitionInProgress) completes in the window between the network error and this check (plausible on a fast restart), a reload failure that really was caused by the runtime swap gets misclassified as a genuine failure and surfaces the visible configReloadFailed toast — the exact symptom this PR sets out to suppress. Consider also snapshotting the transition flag when the TCP call is initiated (treat as pending if it was true at either point), since the current fix only closes the common case, not this narrower race.

2. Dropped task cleanup in scheduleDeferredReloadAfterCooldown: the old code cleared deferredReloadTask = nil after await triggerReload() completed; that line was removed and not reintroduced in either the cooldown path or the new scheduleDeferredReloadAfterRuntimeTransition path. deferredReloadTask now keeps referencing a finished Task indefinitely until the next schedule call overwrites it. Harmless today since nothing else reads the property, but it's a latent footgun if future code checks it as an "is a retry in flight" signal.

3. Minor/style — kanataTransitionPill(_:indicatorCornerRadius:) in OverlayDragHeader+LayerPicker.swift derives its .help() tooltip by string-comparing the label against the literal \"Restarting…\". Any future caller passing a different string (or i18n of these labels) will silently get the wrong tooltip. Prefer passing the transition kind/tooltip explicitly rather than deriving it from the display text.

Test coverage for the new retry state machine (ConfigReloadCoordinatorTests) looks solid — pending classification, bounded retry-then-success, permanent rejection, and expiry are all exercised.

[claude]

Reviewed the diff. Overall solid — the runtime-transition/cooldown coalescing via deferredReloadGeneration is a nice fix for the prior race where a newer deferred task could be nulled out by an older one's completion, and the new tests exercise the retry state machine well. Two things worth a look:

1. retryAfterRuntimeTransition can mask genuine (non-restart) failures for the full grace window. In retryReloadIfRuntimeReady, both .pending and .failed dispositions are treated as "keep retrying" (ConfigReloadCoordinator.swift, the switch result.disposition block). Only .networkError occurring while isRuntimeTransitioning() is true gets the "restarting" pending classification; any other .failed disposition (e.g. an unrelated but persistent network error that happens to occur while automaticDeferredRetriesEnabled retries are running, with notifyOnFailure: false) is silently retried too, and after transitionRetryMaximumPolls is exhausted it surfaces as the generic "Kanata did not become ready after restarting; config reload was not applied" message — which mischaracterizes an unrelated failure as a restart timeout and delays any user-visible signal until the whole grace period elapses. Consider only treating .failed as retryable when the underlying error is actually network/transport-related, not any non-applied/non-rejected disposition.

2. Notification object changed from nil to self/coordinator for .configReloadFailed / .configReloadRecovered. Existing production observers registered with object: nil still fire (matches any poster), but it's worth double-checking no call site elsewhere filters on a specific object for these notification names, since that would now silently stop receiving them.

Nothing blocking — just flagging the retry-classification edge case as worth confirming is intentional, and the notification-object change as worth a quick grep across observers.