diff --git a/apps/desktop/src/app/DesktopStartupRecovery.ts b/apps/desktop/src/app/DesktopStartupRecovery.ts index c8228183aad91..e6c210043cf26 100644 --- a/apps/desktop/src/app/DesktopStartupRecovery.ts +++ b/apps/desktop/src/app/DesktopStartupRecovery.ts @@ -217,7 +217,6 @@ export const listen: Effect.Effect< const download = yield* updates.download; if (!download.completed) return; } - // Waits for a running background check; plain install would refuse. yield* updates.installPrepared(current.version); return; } diff --git a/apps/desktop/src/updates/DesktopRemoteUpdates.test.ts b/apps/desktop/src/updates/DesktopRemoteUpdates.test.ts index 1f5321148b77b..04c1fac8ca6aa 100644 --- a/apps/desktop/src/updates/DesktopRemoteUpdates.test.ts +++ b/apps/desktop/src/updates/DesktopRemoteUpdates.test.ts @@ -691,7 +691,7 @@ describe("DesktopRemoteUpdates", () => { const backgroundCheck = yield* updates.check("poll").pipe(Effect.forkChild); yield* Deferred.await(backgroundCheckStarted); - assert.equal((yield* updates.getState).status, "checking"); + assert.isTrue(yield* updates.isActionActive); yield* Queue.offer(commits, { version: 1, @@ -701,7 +701,6 @@ describe("DesktopRemoteUpdates", () => { yield* settle; assert.equal(harness.quitAndInstalls(), 0); - harness.emit("update-downloaded", { version: "1.2.4" }); yield* Deferred.succeed(releaseBackgroundCheck, undefined); yield* Fiber.join(backgroundCheck); yield* settle; diff --git a/apps/desktop/src/updates/DesktopUpdates.test.ts b/apps/desktop/src/updates/DesktopUpdates.test.ts index 3b45d80bbc6e2..b890f04960eb1 100644 --- a/apps/desktop/src/updates/DesktopUpdates.test.ts +++ b/apps/desktop/src/updates/DesktopUpdates.test.ts @@ -65,8 +65,12 @@ describe("DesktopUpdates", () => { ).pipe(Effect.provide(harness.layer)); }); - it.effect("checks for newer releases after an update has been downloaded", () => { + it.effect("re-checks an offered update without announcing it again", () => { const harness = makeHarness(); + const announcements: unknown[] = []; + const logger = Logger.make(({ message }) => { + if ([message].flat().includes("update available")) announcements.push(message); + }); return Effect.scoped( Effect.gen(function* () { @@ -78,14 +82,24 @@ describe("DesktopUpdates", () => { releaseNotes: "## What's changed\n- fix: queued update", }); yield* flushCallbacks; + const offeredStates = harness.sentStates.length; + + yield* updates.check("poll"); + harness.emit("update-available", { version: "1.2.4" }); + yield* flushCallbacks; + assert.equal(harness.sentStates.length, offeredStates); + harness.emit("update-downloaded", { version: "1.2.4" }); yield* flushCallbacks; + const downloadedStates = harness.sentStates.length; const result = yield* updates.check("poll"); assert.isTrue(result.checked); harness.emit("update-available", { version: "1.2.4" }); yield* flushCallbacks; + assert.equal(harness.sentStates.length, downloadedStates); + assert.lengthOf(announcements, 1); const unchangedState = yield* updates.getState; assert.equal(unchangedState.status, "downloaded"); @@ -105,8 +119,17 @@ describe("DesktopUpdates", () => { assert.equal(state.status, "available"); assert.equal(state.availableVersion, "1.2.5"); assert.isNull(state.downloadedVersion); + assert.lengthOf(announcements, 2); }), - ).pipe(Effect.provide(Layer.merge(TestClock.layer(), harness.layer))); + ).pipe( + Effect.provide( + Layer.mergeAll( + TestClock.layer(), + harness.layer, + Logger.layer([logger], { mergeWithExisting: false }), + ), + ), + ); }); it.effect("preserves a queued installer when the feed has no update", () => { @@ -142,15 +165,26 @@ describe("DesktopUpdates", () => { ).pipe(Effect.provide(Layer.merge(TestClock.layer(), harness.layer))); }); - it.effect( - "rejects install while a refresh check is in progress and releases the reservation", - () => + it.effect.each([ + { action: "download", offer: "update-available", found: null, accepted: true }, + { action: "install", offer: "update-downloaded", found: null, accepted: true }, + { action: "download", offer: "update-available", found: "1.2.5", accepted: true }, + { action: "install", offer: "update-downloaded", found: "1.2.5", accepted: false }, + ] as const)( + "a $action waiting out a refresh check acts on what it found ($found)", + ({ action, offer, found, accepted }) => Effect.gen(function* () { const checkStarted = yield* Deferred.make(); const releaseCheck = yield* Deferred.make(); const harness = makeHarness({ checkForUpdates: Deferred.succeed(checkStarted, undefined).pipe( Effect.andThen(Deferred.await(releaseCheck)), + // electron-updater emits the check's outcome before its promise resolves. + Effect.andThen( + Effect.sync(() => { + if (found !== null) harness.emit("update-available", { version: found }); + }), + ), ), }); @@ -158,22 +192,20 @@ describe("DesktopUpdates", () => { Effect.gen(function* () { const updates = yield* DesktopUpdates.DesktopUpdates; yield* updates.configure; - harness.emit("update-downloaded", { version: "1.2.4" }); + harness.emit(offer, { version: "1.2.4" }); yield* flushCallbacks; - const checkFiber = yield* updates.check("manual").pipe(Effect.forkScoped); + const checkFiber = yield* updates.check("poll").pipe(Effect.forkScoped); yield* Deferred.await(checkStarted); - - const installResult = yield* updates.install; - assert.isFalse(installResult.accepted); + const actionFiber = yield* updates[action].pipe(Effect.forkScoped); + yield* flushCallbacks; yield* Deferred.succeed(releaseCheck, undefined); - const checkResult = yield* Fiber.join(checkFiber); - assert.isTrue(checkResult.checked); - - const followUpCheck = yield* updates.check("manual"); - assert.isTrue(followUpCheck.checked); - assert.equal(harness.checkCount(), 2); + assert.isTrue((yield* Fiber.join(checkFiber)).checked); + assert.equal((yield* Fiber.join(actionFiber)).accepted, accepted); + const state = yield* updates.getState; + assert.equal(state.availableVersion, found ?? "1.2.4"); + if (action === "download") assert.equal(state.status, "downloading"); }), ).pipe(Effect.provide(Layer.merge(TestClock.layer(), harness.layer))); }), diff --git a/apps/desktop/src/updates/DesktopUpdates.ts b/apps/desktop/src/updates/DesktopUpdates.ts index 6279e8b6141a8..abc88a1ccd2a8 100644 --- a/apps/desktop/src/updates/DesktopUpdates.ts +++ b/apps/desktop/src/updates/DesktopUpdates.ts @@ -56,7 +56,7 @@ const CHANNEL_CHANGE_RETRY_DELAY = Duration.seconds(1); const CHANNEL_CHANGE_RETRIES = 30; const AUTO_UPDATE_STARTUP_DELAY = "15 seconds"; const AUTO_UPDATE_POLL_INTERVAL = "4 minutes"; -const PREPARED_INSTALL_CHECK_WAIT = Duration.seconds(90); +const UPDATE_CHECK_WAIT = Duration.seconds(90); type UpdateAction = "check" | "download" | "install" | "install-recovery" | "channel"; @@ -310,6 +310,10 @@ function getCanRetryFromState(state: DesktopUpdateState): boolean { return state.availableVersion !== null || state.downloadedVersion !== null; } +function isOffered(state: DesktopUpdateState): boolean { + return state.status === "available" || state.status === "downloaded"; +} + function shouldBroadcastDownloadProgress( currentState: DesktopUpdateState, nextPercent: number, @@ -509,6 +513,23 @@ export const make = Effect.gen(function* () { ), ); + // Re-runs admit after each finished action, so a caller waiting on a check decides on its result. + const admitAfterCheck = Effect.fn("desktop.updates.admitAfterCheck")(function* ( + admit: Effect.Effect<"admitted" | "refused" | "wait-for-check">, + ) { + const actionCompletions = yield* PubSub.subscribe(finishedUpdateActions); + let admission = yield* stateMutex.withPermits(1)(admit); + while (admission === "wait-for-check") { + const finishedAction = yield* PubSub.take(actionCompletions).pipe( + Effect.timeoutOption(UPDATE_CHECK_WAIT), + ); + admission = Option.isNone(finishedAction) + ? "refused" + : yield* stateMutex.withPermits(1)(admit); + } + return admission; + }); + const applyAutoUpdaterChannel = Effect.fn("desktop.updates.applyAutoUpdaterChannel")(function* ( channel: DesktopUpdateChannel, ) { @@ -550,8 +571,10 @@ export const make = Effect.gen(function* () { if (actionReservation === "acquire" && !(yield* tryStartUpdateAction("check"))) return false; const check = Effect.gen(function* () { - const checkedAt = yield* currentIsoTimestamp; - yield* setState(reduceDesktopUpdateStateOnCheckStart(state, checkedAt)); + // An offer stays on screen while re-checking, so finding it again changes nothing. + if (!isOffered(state)) { + yield* setState(reduceDesktopUpdateStateOnCheckStart(state, yield* currentIsoTimestamp)); + } yield* logUpdaterInfo("checking for updates", { reason }); // The flag is shared updater state, so it flips only while this check holds the reservation. @@ -588,15 +611,30 @@ export const make = Effect.gen(function* () { }); const downloadAvailableUpdate = Effect.gen(function* () { - const state = yield* Ref.get(updateStateRef); - if (!(yield* Ref.get(updaterConfiguredRef)) || state.status !== "available") { - return { accepted: false, completed: false }; - } - - if (!(yield* tryStartUpdateAction("download"))) { + const admission = yield* Effect.scoped( + admitAfterCheck( + Effect.gen(function* () { + const state = yield* Ref.get(updateStateRef); + if (!(yield* Ref.get(updaterConfiguredRef)) || state.status !== "available") { + return "refused" as const; + } + const activeAction = yield* Ref.get(activeUpdateActionRef); + if (Option.isSome(activeAction)) { + return activeAction.value === "check" + ? ("wait-for-check" as const) + : ("refused" as const); + } + return (yield* tryStartUpdateAction("download")) + ? ("admitted" as const) + : ("refused" as const); + }), + ), + ); + if (admission === "refused") { return { accepted: false, completed: false }; } + const state = yield* Ref.get(updateStateRef); return yield* Effect.gen(function* () { yield* setState(reduceDesktopUpdateStateOnDownloadStart(state)); yield* electronUpdater.setDisableDifferentialDownload( @@ -758,47 +796,35 @@ export const make = Effect.gen(function* () { const installDownloadedUpdate = (expectedVersion?: string) => Effect.scoped( Effect.gen(function* () { - const actionCompletions = yield* PubSub.subscribe(finishedUpdateActions); - let admission: "admitted" | "refused" | "wait-for-check" = "wait-for-check"; - while (admission === "wait-for-check") { - admission = yield* stateMutex.withPermits(1)( - Effect.gen(function* () { - const state = yield* Ref.get(updateStateRef); - const activeAction = yield* Ref.get(activeUpdateActionRef); - const hasExpectedDownload = - state.downloadedVersion !== null && - (expectedVersion === undefined || state.downloadedVersion === expectedVersion); - if ( - (yield* Ref.get(desktopState.quitting)) || - !(yield* Ref.get(updaterConfiguredRef)) || - !hasExpectedDownload - ) { - return "refused" as const; - } - if (Option.isSome(activeAction)) { - return activeAction.value === "check" && expectedVersion !== undefined - ? ("wait-for-check" as const) - : ("refused" as const); - } - const hasInstallableDownload = - state.status === "downloaded" || - (state.status === "error" && - (state.errorContext === null || state.errorContext === "install")); - if (!hasInstallableDownload) return "refused" as const; - return (yield* tryStartUpdateAction("install")) - ? ("admitted" as const) + const admission = yield* admitAfterCheck( + Effect.gen(function* () { + const state = yield* Ref.get(updateStateRef); + const activeAction = yield* Ref.get(activeUpdateActionRef); + const hasExpectedDownload = + state.downloadedVersion !== null && + (expectedVersion === undefined || state.downloadedVersion === expectedVersion); + if ( + (yield* Ref.get(desktopState.quitting)) || + !(yield* Ref.get(updaterConfiguredRef)) || + !hasExpectedDownload + ) { + return "refused" as const; + } + if (Option.isSome(activeAction)) { + return activeAction.value === "check" + ? ("wait-for-check" as const) : ("refused" as const); - }), - ); - if (admission === "wait-for-check") { - const finishedAction = yield* PubSub.take(actionCompletions).pipe( - Effect.timeoutOption(PREPARED_INSTALL_CHECK_WAIT), - ); - if (Option.isNone(finishedAction)) { - admission = "refused"; } - } - } + const hasInstallableDownload = + state.status === "downloaded" || + (state.status === "error" && + (state.errorContext === null || state.errorContext === "install")); + if (!hasInstallableDownload) return "refused" as const; + return (yield* tryStartUpdateAction("install")) + ? ("admitted" as const) + : ("refused" as const); + }), + ); if (admission === "refused") { return { accepted: false, completed: false, failed: false }; } @@ -934,6 +960,7 @@ export const make = Effect.gen(function* () { yield* Ref.set(lastLoggedDownloadMilestoneRef, -1); return; } + if (isOffered(state) && state.availableVersion === info.version) return; const checkedAt = yield* currentIsoTimestamp; const { releaseNotes, omittedReleaseCount } = normalizeDesktopUpdateReleaseNotes( @@ -1113,6 +1140,8 @@ export const make = Effect.gen(function* () { ), configure: Effect.gen(function* () { const context = yield* Effect.context(); + // Runs synchronously up to the first await, so a check's outcome is applied before the + // check's promise resolves and wakes the actions waiting on it. const runEffect = (effect: Effect.Effect) => { void Effect.runPromiseWith(context)(effect); };