Skip to content

Retry transient Sparkle download failures - #6992

Closed
austinywang wants to merge 34 commits into
mainfrom
issue-5632-sparkle-update-fails-with-sudownloaderror-200
Closed

austinywang wants to merge 34 commits into
mainfrom
issue-5632-sparkle-update-fails-with-sudownloaderror-200

Conversation

@austinywang

@austinywang austinywang commented Jun 26, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #5632

Summary

  • add a regression test for Sparkle SUDownloadError 2001 caused by transient HTTP 504s
  • retry transient Sparkle download failures with bounded backoff before showing the error
  • preserve install/attempt intent across automatic retries so DMG download retries continue silently

Validation

  • swift test --package-path Packages/macOS/CmuxUpdater
  • python3 scripts/check-package-resolved-policy.py
  • python3 scripts/check-workspace-package-groups.py --check
  • ./scripts/lint-pbxproj-test-wiring.sh
  • git diff --check HEAD

Summary by cubic

Retries transient Sparkle download failures with bounded, cancellable backoff and preserves install intent so retried installs stay silent. Also fixes readiness-wait and cancel edge cases to avoid stranding installs or silently auto-confirming after a user cancel. Fixes #5632.

  • Bug Fixes

    • UpdateRetryPolicy: classify transient download errors (HTTP 408/429/5xx, timeouts/DNS/connection loss), handle nested/shadowed statuses; retry at 1s/3s/8s.
    • UpdateDriver: auto-retry with cancellable backoff via injected clock; preserve/restore install intent for download/extract/install retries; surface non‑transient 404s; keep bounded retry count on readiness‑delayed plain retries; user Cancel returns the pill to idle and clears any pending preserve; preserve failure count only during the synchronous internal retry restart; reset retry state on success.
    • UpdateController: retryAfterTransientFailure(preservingInstallIntent:) uses pure TransientRetryPlan to restart a monitored check, re‑arm to auto‑confirm when not yet monitoring, or run a plain check; readiness wait keeps polling for any pending state and stops only on .idle; preserved‑intent and synthetic .checking retries bypass Sparkle teardown delay; DEV/staging gating via BundleReleaseChannel.
    • AttemptUpdateCoordinator/host: armForConfirmedRetryCheck() to avoid stranding when a pre‑restart Cancel occurs.
    • UpdateActionDelegate: now @MainActor and passes preservingInstallIntent; AppDelegate forwards to UpdateController.retryAfterTransientFailure(...).
  • Tests/CI

    • Added tests for retry backoff, preserved‑intent re‑arm, cancel‑before‑restart disarm, readiness‑wait behavior, 404 no‑retry, and nested status parsing; retry tests wait on causality via RecordingUpdateActionDelegate; added ImmediateUpdateClock and NullUpdateLog.
    • CI forces a short app‑host TMPDIR (/tmp/) and verifies it; updated the app‑host retry wrapper/test; decision helpers are value‑type enums (ReadinessWaitDecision, BundleReleaseChannel) in one‑type‑per‑file.

Written for commit 2efdd21. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Added automatic retry for transient update download failures using a bounded backoff schedule.
    • When an update was interrupted during install/download, retries can preserve the prior install intent for a smoother continuation.
  • Bug Fixes

    • Non-transient failures now bypass retry and surface immediately.
    • Retry behavior is consistent whether update checks begin immediately or after readiness gating.
  • Tests / Chores

    • Added/expanded retry decision and driver retry unit tests with clock/delegate test helpers.
    • Improved CI app-host XCTest handling by enforcing a short TMPDIR.

@vercel

vercel Bot commented Jun 26, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Jul 5, 2026 5:13am
cmux-staging Building Building Preview, Comment Jul 5, 2026 5:13am

@coderabbitai

coderabbitai Bot commented Jun 26, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds transient Sparkle-download retry handling with preserved install intent across the updater protocol, controller, driver, and tests. Also updates the app-host CI wrapper to force a short TMPDIR and adjusts the corresponding retry test.

Changes

Transient Download Failure Retry

Layer / File(s) Summary
UpdateRetryPolicy and retry classification
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift
UpdateRetryPolicy now classifies transient download failures and maps failure counts to bounded retry delays.
UpdateActionDelegate retry callback
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift
The retry callback now carries a preservingInstallIntent flag.
UpdateController retry plan helpers
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+Decisions.swift, Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift, Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift
UpdateController adds retry-plan selection, readiness gating, and preserved-intent retry paths.
UpdateDriver transient retry scheduling
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift
UpdateDriver schedules bounded transient retries, resets retry state on other transitions, and routes manual fallback errors through the new retry callback shape.
AppDelegate retry delegate wiring
Sources/AppDelegate.swift
AppDelegate forwards the new retry flag into UpdateController.
Retry tests and helpers
Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/*
Adds test helpers and coverage for retry planning, transient retry scheduling, non-transient errors, and retry delay computation.

CI App-Host TMPDIR Shortening

Layer / File(s) Summary
run-app-host-xcodebuild.sh TMPDIR handling
scripts/ci/run-app-host-xcodebuild.sh
The app-host wrapper now normalizes and exports a shorter TMPDIR and updates its lock-file path.
CI retry test updates
tests/test_ci_app_host_xcodebuild_retry.sh
The shell test now checks the exported TMPDIR and the updated idle-timeout timing.
Estimated code review effort: 4 (Complex) ~60 minutes

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (3 errors)

Check name Status Explanation Resolution
Cmux Swift Blocking Runtime ❌ Error FAIL: non-test Swift adds a new Task.sleep-backed transient retry in UpdateDriver, and UpdateController expands its readiness poll loop—both are timing sync in shipped code. Use a signal/callback/timer source to trigger retries instead of sleeping a task, and avoid polling readiness on a loop in shipped code; let Sparkle or an async event/state change drive it.
Cmux User-Facing Error Privacy ❌ Error FAIL: The new user-visible update error popover copies raw SUSparkleErrorDomain, SUDownloadError, failing URL/feed URL, and localizedDescription into Details. Sanitize the popover: keep generic cmux-facing copy only, and move domain/code/URL/feed/raw upstream text to logs or a hidden debug-only details view.
Cmux Full Internationalization ❌ Error UpdateController adds update.error.notReady, but Resources/Localizable.xcstrings only has en/ja, leaving the catalog’s other locales untranslated. Add translated entries for the other locales already supported by Resources/Localizable.xcstrings.
✅ Passed checks (22 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code and tests implement transient Sparkle retry/backoff and intent preservation, matching #5632's requirements.
Out of Scope Changes check ✅ Passed The CI and test-support changes appear ancillary to validating the retry flow and do not introduce obvious unrelated behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Cmux Swift Actor Isolation ✅ Passed PASS: New production APIs stay within @MainActor boundaries; the added pure helpers are explicitly nonisolated, and no new shared mutable Sendable refs or background UI-store access appears.
Cmux Browser Automation Off-Main ✅ Passed PR changes are confined to updater/CI/test files; no browser.* commands or scoped browser-automation files were modified.
Cmux Expensive Synchronous Load ✅ Passed Changed files are updater retry/CI code; no RestorableAgentSessionIndex.load(), transcript/trajectory JSON, or other agent-history loads appear on MainActor paths.
Cmux Cache Substitution Correctness ✅ Passed PASS: The diff only adds transient-retry and intent-preservation logic; it doesn’t replace any fresh authoritative read with cached/opportunistic data in persistence/history/undo/snapshot paths.
Cmux No Hacky Sleeps ✅ Passed PASS: the only sleep is in test scaffolding; the production shell wrapper uses event-driven retries/timeout handling and adds no fixed synchronization delay.
Cmux Algorithmic Complexity ✅ Passed PASS: New retry logic only scans bounded error chains and fixed delay lists; readiness polling is capped at 20, and no scalable collection hot path was added.
Cmux Swift Concurrency ✅ Passed No new legacy async anti-patterns were added; retry work uses stored/cancelled Tasks, and callback/continuation uses are boundary or test-only.
Cmux Swift @Concurrent ✅ Passed No changed async helper needs @concurrent; all new async work stays on @MainActor, and the test clock is a no-op, so the diff respects the rule.
Cmux Swift File And Package Boundaries ✅ Passed New production logic is split into small CmuxUpdater package files; AppDelegate is thin glue, and no touched production Swift file is a new oversized or mixed-responsibility file.
Cmux Swiftpm Lockfiles ✅ Passed The diff adds sources/tests and a pbxproj source-list update only; no Package.swift, Package.resolved, .gitignore, or SwiftPM package-reference changes appear.
Cmux Swift Logging ✅ Passed No relevant Swift diff touches logging; production updater code uses the existing UpdateLogging sink and adds no print/debugPrint/NSLog/Logger changes.
Cmux Swiftui State Layout ✅ Passed PASS: the PR only updates the ghostty submodule, and that diff touches .zig/.zon files only—no SwiftUI state/layout patterns were introduced.
Cmux Architecture Rethink ✅ Passed The retry/backoff and readiness polling stay within UpdateDriver/UpdateController ownership, are bounded/testable, and are documented as required Sparkle bridges.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR only changes updater retry logic, tests, and CI scripts; no new standalone NSWindow/NSPanel/NSWindowController/WindowGroup or cmux.* window-identifier changes appear.
Cmux Source Artifacts ✅ Passed Only changed path is a submodule pointer update (ghostty); no temp/build/cache/log artifacts or broad scratch dirs appear in the diff.
Cmux No Test Or Debug Seam In Production Source ✅ Passed Touched production Sources add retry logic and pure helpers; no new *ForTesting/TestHook/debug-only seam appeared in those changed APIs.
Cmux No Ambient Global State ✅ Passed New retry logic is injected value/state on UpdateController/UpdateDriver; no new file-scope APIs, mutable globals, or singleton-style namespaces were added.
Title check ✅ Passed The title clearly and concisely summarizes the main change: retrying transient Sparkle download failures.
Description check ✅ Passed The description covers summary and validation, but it omits the demo video, review trigger block, and checklist sections from the template.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-5632-sparkle-update-fails-with-sudownloaderror-200

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jun 26, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds bounded, cancellable automatic retries for transient Sparkle download failures (HTTP 408/429/5xx, timeouts, DNS/connection loss) and preserves install intent across retries so DMG downloads that fail mid-progress silently resume rather than surfacing a fresh "Update Available" prompt.

  • UpdateRetryPolicy classifies transient errors via structured NSURLError codes and a regex fallback over Sparkle's localized HTTP error copy; retries occur at 1 s/3 s/8 s with a hard cap.
  • UpdateDriver schedules cancellable backoff tasks, captures install intent (downloading/extracting/installing state), and gates the synchronous retry-restart window with isRestartingTransientRetry so a subsequent user Cancel idles the pill correctly.
  • UpdateController introduces retryAfterTransientFailure(preservingInstallIntent:), driven by the pure TransientRetryPlan enum, and ReadinessWaitDecision, which keeps the readiness-wait loop alive for any non-idle state so attemptUpdate()'s in-progress install is not stranded when Sparkle is briefly unready.

Confidence Score: 5/5

Safe to merge — the retry logic is well-isolated behind pure value-type decision enums, all timing uses the injected UpdateClock (cancellable, testable), and the two previously identified regressions (shadow-status parser and readiness-wait decision) are confirmed fixed at HEAD.

The retry backoff is bounded (1/3/8 s, hard cap at 3 attempts), cancellable (Task cancel + isRestartingTransientRetry scope), and correctly preserves the install-intent state machine through the AttemptUpdateCoordinator. The transient error classifier correctly scans all regex matches before moving to the next pattern, the ReadinessWaitDecision fix stops the wait only on .idle, and the rearmConfirmedInstall path disarms safely on user cancel. Test coverage includes the key scenarios: 504 retry, download-phase intent preservation, cancel-to-idle, 404 no-retry, and the shadow-status parser case.

No files require special attention.

Important Files Changed

Filename Overview
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift New file; classifies transient errors via NSURLError codes + regex fallback, respects depth limit on the error chain, and correctly scans all matches per pattern before moving on so a non-transient code cannot shadow a later transient one.
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift Adds bounded retry backoff with cancellable Task, preserves install intent from downloading/extracting/installing state, and correctly scopes the isRestartingTransientRetry flag to the synchronous delegate call window so user cancels are honored afterwards.
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift Adds retryAfterTransientFailure(preservingInstallIntent:) routed through TransientRetryPlan; refactors performCheckForUpdates to bypass cancelActiveStateForNewCheck for synthetic checking pills; updates readiness wait to keep polling on any non-idle state.
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+TransientRetry.swift New file; pure value-type TransientRetryPlan enum is nonisolated and unit-testable; correctly handles the three retry paths (restartMonitoredCheck, rearmConfirmedInstall, plainCheck).
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/AttemptUpdateCoordinator.swift Adds armForConfirmedRetryCheck() entering awaitingResult directly; correctly disarms on idle in awaitingResult so a user cancel does not strand the coordinator armed to auto-confirm a later unrelated update.
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift Adds preservingInstallIntent parameter to updaterRequestsRetryCheckForUpdates; both conformers (AppDelegate, RecordingUpdateActionDelegate) updated in this PR.
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+ReadinessWaitDecision.swift New file; pure ReadinessWaitDecision enum stops the readiness poll only on .idle, fixing the stranded-install regression from issue #5632.
Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController+BundleReleaseChannel.swift New file; refactors inline dev/staging detection into a testable pure enum, mirroring TransientRetryPlan; removes the static isDevLikeBundleIdentifier function from UpdateController.
Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift Comprehensive retry tests: 504 triggers retry, downloading state preserves intent, user cancel returns to idle, 404 surfaces error without retry, and non-transient shadowing test.
scripts/ci/run-app-host-xcodebuild.sh Forces short TMPDIR (/tmp/) for app-host XCTest to avoid AF_UNIX socket path length limit; verified by the updated test_ci_app_host_xcodebuild_retry.sh test.
tests/test_ci_app_host_xcodebuild_retry.sh Adds TMPDIR verification to the retry test; deterministic scaffolding with sleep is appropriate for this CI harness test.
Sources/AppDelegate.swift Forwards updaterRequestsRetryCheckForUpdates(preservingInstallIntent:) to UpdateController.retryAfterTransientFailure(preservingInstallIntent:) correctly.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Sparkle
    participant UpdateDriver
    participant UpdateController
    participant AttemptCoordinator

    Sparkle->>UpdateDriver: showUpdaterError(504 error)
    UpdateDriver->>UpdateDriver: scheduleTransientErrorRetryIfNeeded()
    UpdateDriver->>UpdateDriver: setState(.checking) [synthetic retry pill]
    UpdateDriver->>Sparkle: acknowledgement()
    Note over UpdateDriver: backoff sleep (1s/3s/8s)
    UpdateDriver->>UpdateController: updaterRequestsRetryCheckForUpdates(preservingInstallIntent:)
    alt "coordinatorIsMonitoring == true"
        UpdateController->>UpdateController: checkForUpdatesWhenReady(preservingInstallIntent: true)
    else "preservingInstallIntent == true"
        UpdateController->>AttemptCoordinator: armForConfirmedRetryCheck()
        UpdateController->>UpdateController: checkForUpdatesWhenReady(preservingInstallIntent: true)
    else plain retry
        UpdateController->>UpdateController: checkForUpdatesWhenReady(preservingInstallIntent: false)
    end
    UpdateController->>Sparkle: updater.checkForUpdates()
    Sparkle->>UpdateDriver: showUserInitiatedUpdateCheck()
    UpdateDriver->>UpdateDriver: beginChecking() [preserves failure count]
    Sparkle->>UpdateDriver: showUpdateFound()
    UpdateDriver->>UpdateController: state → .updateAvailable
    UpdateController->>AttemptCoordinator: handleStateChange(.updateAvailable)
    AttemptCoordinator-->>UpdateController: .confirmInstall
    UpdateController->>UpdateController: model.state.confirm()
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Sparkle
    participant UpdateDriver
    participant UpdateController
    participant AttemptCoordinator

    Sparkle->>UpdateDriver: showUpdaterError(504 error)
    UpdateDriver->>UpdateDriver: scheduleTransientErrorRetryIfNeeded()
    UpdateDriver->>UpdateDriver: setState(.checking) [synthetic retry pill]
    UpdateDriver->>Sparkle: acknowledgement()
    Note over UpdateDriver: backoff sleep (1s/3s/8s)
    UpdateDriver->>UpdateController: updaterRequestsRetryCheckForUpdates(preservingInstallIntent:)
    alt "coordinatorIsMonitoring == true"
        UpdateController->>UpdateController: checkForUpdatesWhenReady(preservingInstallIntent: true)
    else "preservingInstallIntent == true"
        UpdateController->>AttemptCoordinator: armForConfirmedRetryCheck()
        UpdateController->>UpdateController: checkForUpdatesWhenReady(preservingInstallIntent: true)
    else plain retry
        UpdateController->>UpdateController: checkForUpdatesWhenReady(preservingInstallIntent: false)
    end
    UpdateController->>Sparkle: updater.checkForUpdates()
    Sparkle->>UpdateDriver: showUserInitiatedUpdateCheck()
    UpdateDriver->>UpdateDriver: beginChecking() [preserves failure count]
    Sparkle->>UpdateDriver: showUpdateFound()
    UpdateDriver->>UpdateController: state → .updateAvailable
    UpdateController->>AttemptCoordinator: handleStateChange(.updateAvailable)
    AttemptCoordinator-->>UpdateController: .confirmInstall
    UpdateController->>UpdateController: model.state.confirm()
Loading

Reviews (20): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

Comment thread Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift Outdated
Comment thread Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift Outdated
Comment thread Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift Outdated
…derror-200

Reconcile the transient-Sparkle-download-retry feature with origin/main's
AttemptUpdateCoordinator refactor (#6366) and DEV/staging update gating (#6817):

- UpdateController.checkForUpdatesWhenReady: keep the branch's
  preservingInstallIntent parameter AND main's dev/staging suppression block.
- UpdateController.retryAfterTransientFailure: main replaced the
  isForceInstalling/isAttemptingUpdate booleans with AttemptUpdateCoordinator,
  so derive the secondary install-intent guard from attemptCoordinator.isMonitoring
  (the download/extract/install phase is already covered by the driver's
  preservingInstallIntent parameter; isMonitoring covers the re-resolve check phase).
- UpdateDriver: combine the branch's retryPolicy and main's isDevLikeBundle
  init parameters and stored properties.
- UpdatePopoverView "Install and Relaunch": adopt main's actions.attemptUpdate()
  (re-resolve to latest at install time, #6366); the branch's actions.installUpdate()
  scaffolding is superseded. Remove the now-orphaned installUpdate() from the
  UpdateActionsHost protocol and AppDelegate.
- Retighten AppDelegate.swift file-length budget after the installUpdate() removal.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift`:
- Around line 24-26: The retry tests are relying on fixed Task.yield() loops in
UpdateDriverRetryTests instead of a real completion signal, which makes them
scheduler-dependent. Update the test flow around the retry assertions to wait on
a causality-based predicate or an async signal from
RecordingUpdateActionDelegate, and use that signal in place of the yield loops
so the tests only proceed once the retry action has actually occurred.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e3032e2d-40df-46dc-99d0-cc5729a13bf7

📥 Commits

Reviewing files that changed from the base of the PR and between 1e9025f and 3be1ee3.

⛔ Files ignored due to path filters (1)
  • .github/swift-file-length-budget.tsv is excluded by !**/*.tsv
📒 Files selected for processing (11)
  • Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swift
  • Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift
  • Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swift
  • Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateRetryPolicy.swift
  • Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/ImmediateUpdateClock.swift
  • Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/NullUpdateLog.swift
  • Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/RecordingUpdateActionDelegate.swift
  • Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift
  • Sources/AppDelegate.swift
  • scripts/ci/run-app-host-xcodebuild.sh
  • tests/test_ci_app_host_xcodebuild_retry.sh

Comment thread Packages/macOS/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateDriverRetryTests.swift Outdated
CodeRabbit flagged the `for _ in 0..<5 { await Task.yield() }` waits in
UpdateDriverRetryTests as scheduler-dependent (they can race the retry task on
slower CI), violating the repo guideline "assert on causality, not latency."

Add an async completion signal to RecordingUpdateActionDelegate
(`waitForRetryRequests(atLeast:)`) that resumes the moment
`updaterRequestsRetryCheckForUpdates` fires, and await it in the two transient
tests instead of yielding a fixed number of times. Both the mock and the driver
are @mainactor, so registering a waiter and resuming it are serialized and
race-free. The non-transient 404 test stays synchronous (it asserts no retry is
scheduled).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
austinywang and others added 2 commits July 1, 2026 21:28
…date-fails-with-sudownloaderror-200

# Conflicts:
#	.github/swift-file-length-budget.tsv
A Sparkle transient download failure during a download/extract/install
phase asks the host to retry with preservingInstallIntent=true. When the
attempt coordinator (issue #6366) is not yet monitoring, the controller
must re-arm it so the retried check auto-confirms whatever update it
resolves — otherwise the retry surfaces a fresh "Update Available" prompt
and the interrupted install is silently stranded (the very failure mode
issue #5632's retry work is meant to recover from).

Factor the controller's retry decision into a pure
UpdateController.transientRetryPlan(...) in its own file, mirroring
AttemptUpdateCoordinator's pure-policy split so the decision is testable
without the live SPUUpdater the controller owns, and dispatch
retryAfterTransientFailure through it. This commit deliberately keeps
today's buggy mapping (a preserved-intent retry that is not yet monitored
still collapses into the monitored-restart path and never re-arms the
coordinator), so the new UpdateControllerTransientRetryPlanTests
regression fails. The one-line mapping fix follows in the next commit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai 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.

1 issue found across 14 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Flip the one remaining branch of transientRetryPlan: a transient retry
carrying install intent that arrives before the attempt coordinator is
monitoring now returns .rearmConfirmedInstall, which routes through the
shared attempt path (requestInstallLatest) so the coordinator is armed
and the retried check auto-confirms the update it resolves. Previously
this case fell through to a bare fresh check that only surfaced an
"Update Available" prompt, silently stranding the interrupted install.

Turns UpdateControllerTransientRetryPlanTests green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
austinywang and others added 6 commits July 2, 2026 02:05
…date-fails-with-sudownloaderror-200

# Conflicts:
#	.github/swift-file-length-budget.tsv
The preserved-intent branch in `performCheckForUpdates` returns early and calls
`updater.checkForUpdates()` immediately, bypassing the non-idle
`cancelActiveStateForNewCheck()` + 100ms delay used elsewhere. That looks like it
could let Sparkle coalesce/drop the re-check, but it is safe and intentional:

- This path is only reached from `retryAfterTransientFailure` after the driver's
  multi-second retry backoff, and the failed Sparkle session was already
  acknowledged/torn down at error time (`showUpdaterError` → `acknowledgement()`).
  There is no just-dismissed live session to coalesce with — the 100ms delay only
  guards an *immediate* re-check against a session Sparkle is still aborting.
- Routing it through the teardown path would be wrong: the current non-idle state
  is the driver's synthetic `.checking` backoff placeholder whose `cancel` aborts
  the retry, so `cancelActiveStateForNewCheck()` would kill the retry and flicker
  the pill to idle.

Comment-only; no behavior change. Documents the invariant an autoreview pass
flagged so it is legible to future readers.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion

The guard in cancelPendingTransientErrorRetry preserves the escalating
failure count across the synchronous internal restart teardown, and does
not strand a user-initiated cancel: at retry time Sparkle has already
acknowledged/ended the failed session and the backoff has elapsed, so the
controller performs the check immediately rather than parking in the
uncancellable readiness-wait loop. Documents the invariant behind
autoreview's low-confidence UpdateDriver.swift:266 finding (no behavior change).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reproduces the readiness-wait cancel regression (UpdateDriver.swift): after a
transient download failure schedules a retry and the backoff fires, the pill
stays in .checking; pressing Cancel then hits the
shouldPreserveRetryStateForNextCheck guard and no-ops, so the pill is stuck
instead of returning to idle. This commit adds the test only (CLAUDE.md
two-commit regression policy) — it fails until the follow-up scopes the
preserve guard to the synchronous internal restart.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Scope the transient-retry preserve-state suppression to the synchronous
internal restart so a user cancel during the controller's readiness wait idles
the pill instead of no-oping.

- UpdateDriver: add isRestartingTransientRetry, set true only around the
  synchronous updaterRequestsRetryCheckForUpdates delegate call (the window
  where the controller may re-enter the cancel closure via
  cancelActiveStateForNewCheck). The cancel guard now keys off this flag
  instead of shouldPreserveRetryStateForNextCheck, which stays true across the
  async readiness wait; also clear the pending preserve on user cancel.
- UpdateController: waitForReadinessThenCheck bails when the model has left
  .checking, so an idled pill isn't resurrected once readiness arrives and the
  orphaned readiness task stops promptly.

Fixes the readiness-wait regression covered by
userCancelWhileRetryPillAwaitsReadinessReturnsToIdle (commit 2031c15).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@austinywang
austinywang force-pushed the issue-5632-sparkle-update-fails-with-sudownloaderror-200 branch from e624abd to 2031c15 Compare July 2, 2026 10:26
austinywang and others added 2 commits July 2, 2026 03:48
Convert the pure `UpdateController.transientRetryPlan(preservingInstallIntent:
coordinatorIsMonitoring:)` static factory into a `nonisolated init` on the nested
`TransientRetryPlan` value type. Behavior-preserving: the decision logic
(monitored -> restart, preserved-intent-before-monitoring -> re-arm, else ->
plain check) is byte-identical and stays covered by the existing
UpdateControllerTransientRetryPlanTests.

Constructing the plan by value (`TransientRetryPlan(...)`) rather than calling a
static method on the stateful `@MainActor UpdateController` removes the
static-as-namespace shape flagged by package-design review, without adding any
new type or cross-object reference.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…date-fails-with-sudownloaderror-200

# Conflicts:
#	.github/swift-file-length-budget.tsv

@cubic-dev-ai cubic-dev-ai 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.

1 issue found across 5 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift Outdated
austinywang and others added 5 commits July 2, 2026 04:14
The download-phase transient-retry re-arm (`.rearmConfirmedInstall`) currently
arms the attempt coordinator via `requestInstallLatest`, which enters
`awaitingCheckRestart`. That phase exists to ignore a stale on-screen prompt
until a real check restarts, so a state change to `.idle` there is read as
"check restarted" and advances to `awaitingResult` (still armed).

But a transient-retry re-arm has no stale prompt: the model is the retry's own
synthetic `.checking` pill. If the user cancels it during the controller's
readiness wait, the model idles directly (no intervening `.checking`) and the
coordinator is stranded armed — later silently auto-confirming an unrelated
update the user never asked to install (autoreview P1).

Introduce an `armForConfirmedRetryCheck()` seam for this re-arm and wire the
controller to it (routing the check through the preserving path so the synthetic
pill is not torn down and the bounded-retry count is preserved). This commit
gives the seam the status-quo `awaitingCheckRestart` body so the new
`retryRearmDisarmsWhenUserCancelsBeforeCheckRestarts` regression test fails,
proving it catches the strand before the fix lands in the next commit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Enter `awaitingResult` directly in `armForConfirmedRetryCheck()` instead of
`awaitingCheckRestart`. The retry re-arm has no stale on-screen prompt to gate
against (the model is the retry's own synthetic `.checking` pill), so the
`awaitingCheckRestart` stage is unnecessary and actively harmful here: a user
cancel during the readiness wait idles the model directly, and in
`awaitingCheckRestart` that idle is misread as check-restart progress
(→ `awaitingResult`, still armed), later silently auto-confirming an unrelated
update. Arming straight into `awaitingResult` routes that idle through the
`awaitingResult` idle → inactive path, disarming the coordinator (autoreview P1).

`retryRearmDisarmsWhenUserCancelsBeforeCheckRestarts` now passes; the #6366
user-initiated install flow is untouched (still goes through
`requestInstallLatest`/`awaitingCheckRestart`).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`performCheckForUpdates` already bypasses the non-idle teardown +
`cancelActiveStateForNewCheck()` for preserved-intent retries and for the idle
case, precisely because tearing down the driver's synthetic `.checking` backoff
placeholder fires its `cancel` — which aborts the retry and resets the
`[1, 3, 8]` bounded-retry failure count. But the guard keyed on
`preservingInstallIntent`, so a *plain* transient retry (no install intent)
whose model is that same `.checking` pill fell through to the teardown.

On the readiness-delayed path this runs from `readyCheckTask`, after the
synchronous `isRestartingTransientRetry` window has closed, so
`cancelPendingTransientErrorRetry` resets the failure count to 0 every attempt —
turning the bounded policy into an unbounded ~1s retry loop while a transient
appcast/download error persists (autoreview P2).

Extend the bypass to any synthetic `.checking` state. Every caller reaches
`performCheckForUpdates` only after observing `canCheckForUpdates == true`, so no
live Sparkle session exists and a `.checking` here is always the synthetic
placeholder — safe to start the fresh check directly and keep the count.

Not unit-tested: the repro requires toggling `SPUUpdater.canCheckForUpdates`,
which `UpdateController` owns as a concrete, non-injectable updater; the fix
mirrors the two adjacent (likewise unit-untested) bypasses with an explicit
rationale and is covered at compile time by CI.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…kle not ready

Extract the readiness-wait continuation guard into a pure static predicate
(readinessWaitShouldContinue) so it can be unit-tested, preserving the current
(too-broad) `.checking`-only behavior for now. Add UpdateControllerReadinessWaitTests
asserting the wait must keep polling while the model is the `.updateAvailable`
prompt that attemptUpdate() re-resolves (issue #6366).

That case fails today: the guard bails on any non-`.checking` state, so an Install
triggered while Sparkle is briefly not ready (canCheckForUpdates == false) exits the
readiness wait before it can run the fresh re-resolution check — stranding the
coordinator armed so the user's Install does nothing until some unrelated state
change occurs (autoreview follow-up to issue #5632).

Red half of the two-commit regression proof; the fix follows.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The transient-retry work guarded the shared readiness wait to stop unless the
model was `.checking`. But `attemptUpdate()` (install from an on-screen prompt)
enters the readiness wait while the model is still `.updateAvailable` — the #6366
re-resolve keeps the prompt visible, it does not switch to `.checking`. When
Sparkle was briefly not ready (canCheckForUpdates == false), the guard bailed
immediately, no fresh check ran, and the armed coordinator left the user's Install
doing nothing until an unrelated state change.

Scope the guard to the actual cancellation signal: stop only when the pending
check has returned to `.idle` (the retry/checking pill's Cancel), and keep polling
for every other pending state — `.checking` placeholders and the `.updateAvailable`
install re-resolve alike. Turns UpdateControllerReadinessWaitTests green.

Green half of the two-commit regression proof (red: prior commit).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread Packages/macOS/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift Outdated
austinywang and others added 4 commits July 2, 2026 04:39
… budget

The transient-retry and readiness fixes grew UpdateController.swift past the
500-line budget (workflow-guard-tests / scripts/swift_file_length_budget.py, which
fails any untracked cmux-owned Swift file at or over the threshold). Per the
swift-file-package-boundaries review rule, restore the budget by splitting a
responsibility rather than expanding the TSV: move the two pure, self-free static
decision helpers — readinessWaitShouldContinue (readiness-wait continuation) and
isDevLikeBundleIdentifier (dev/staging bundle classification) — into a new
UpdateController+Decisions.swift extension, mirroring UpdateController+TransientRetry.

Both remain reachable at their unchanged UpdateController.<static> call sites and
their existing tests; UpdateController.swift returns to 494 lines.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…date-fails-with-sudownloaderror-200

# Conflicts:
#	.github/swift-file-length-budget.tsv
…policy)

The two pure decision helpers extracted into `UpdateController+Decisions.swift`
were `static func`s called as `UpdateController.method(...)`, which the package
design policy flags as a "static-as-namespace" anti-pattern (a caseless
namespace on a type that holds no relevant stored state).

Re-express both as value-type enums constructed via a `nonisolated init`,
mirroring the sibling `TransientRetryPlan` that the same policy already accepts:

- `ReadinessWaitDecision(modelState:)` → `.keepPolling` / `.stop`
- `BundleReleaseChannel(bundleIdentifier:)` → `.devLike` / `.release`

These stay unit-testable without the live `SPUUpdater` the controller owns (the
reason they were extracted), while replacing the `Type.staticMethod()` call
shape with instance construction. No behavior change; the readiness-wait
continuation and dev/staging gating logic are identical. Existing unit tests are
retargeted to the new types, and the two call sites plus doc links updated.
UpdateController.swift stays within the 500-line file-length budget.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The prior commit re-expressed the two pure decision helpers as value-type
enums (ReadinessWaitDecision, BundleReleaseChannel) to satisfy Aziz's
static-as-namespace policy, but placed both in a single
UpdateController+Decisions.swift. Aziz's file-organization policy wants one
major type per file (precedent: UpdateController+TransientRetry.swift holds
only TransientRetryPlan).

Split into UpdateController+ReadinessWaitDecision.swift and
UpdateController+BundleReleaseChannel.swift, one nested enum each. Pure file
reorganization — no type, signature, or behavior change; all call sites and
tests already reference the types unqualified / via UpdateController. and are
untouched. BundleReleaseChannel keeps `import Foundation` for `hasPrefix`;
ReadinessWaitDecision needs no import (matches +TransientRetry.swift).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@austinywang
austinywang force-pushed the issue-5632-sparkle-update-fails-with-sudownloaderror-200 branch from 5494234 to 372b5a4 Compare July 2, 2026 13:22
…date-fails-with-sudownloaderror-200

# Conflicts:
#	.github/swift-file-length-budget.tsv
@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026

This branch was successfully deployed

1 active deployment
Preview – cmux — 2efdd21a Deployed Jul 5, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sparkle update fails with SUDownloadError (2001) on transient GitHub release CDN 504s - no retry/fallback

3 participants