Skip to content

fix(updater): ignore stale check results #330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ThunderTr77 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(updater): ignore stale check results #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54f738e
fix(updater): ignore stale check results
ThunderTr77 May 31, 2026
9ba57af
fix(updater): preserve checks when channel write fails
ThunderTr77 Jun 7, 2026
9a64db7
Merge branch 'main' into fix/updater-stale-check-race
ThunderTr77 Jun 7, 2026
2d277c6
Merge branch 'main' into fix/updater-stale-check-race
hiqiancheng Jun 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions apps/desktop/src/services/AppUpdateService/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export class AppUpdateController {
private readonly now: () => string;
private initialized = false;
private unlistenProgress: UnlistenFn | null = null;
private checkRequestVersion = 0;

constructor(deps: AppUpdateControllerDeps) {
this.native = deps.native;
Expand Down Expand Up @@ -112,6 +113,7 @@ export class AppUpdateController {
async setChannel(channel: AppUpdateChannel): Promise<void> {
await this.initialize();
await this.settings.updateAppUpdateChannel(channel);
this.checkRequestVersion += 1;

@hiqiancheng hiqiancheng Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Potential issue: checkRequestVersion is advanced before updateAppUpdateLastCheckedAt(null) succeeds. If the channel write succeeds but clearing lastCheckedAt fails while a check is in flight, the old check is now treated as stale, while channel-updated is never committed. That can leave the controller stuck in the previous channel's checking state even though the persisted channel has already changed.

Could we cover the updateAppUpdateLastCheckedAt(null) rejection case and either move invalidation until the channel switch is fully committed, or explicitly handle that partial-failure path by committing/rolling back state so the in-flight check does not get stranded?

await this.settings.updateAppUpdateLastCheckedAt(null);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
this.commit({ type: 'channel-updated', channel });
}
Expand All @@ -125,15 +127,24 @@ export class AppUpdateController {

const previousState = this.state;
const channel = this.state.channel;
const requestVersion = ++this.checkRequestVersion;
this.commit({ type: 'check-started', channel });

try {
const result = await this.native.checkForUpdates(channel);
const checkedAt = this.now();
if (!this.isCurrentCheckRequest(requestVersion, channel)) {
return false;
}

this.commit({ type: 'check-completed', channel, result, checkedAt });
await this.settings.updateAppUpdateLastCheckedAt(checkedAt);
return true;
} catch (error) {
if (!this.isCurrentCheckRequest(requestVersion, channel)) {
return false;
}

if (source === 'automatic') {
this.replaceState(previousState);
return false;
Expand Down Expand Up @@ -171,6 +182,10 @@ export class AppUpdateController {
}
}

private isCurrentCheckRequest(requestVersion: number, channel: AppUpdateChannel): boolean {
return requestVersion === this.checkRequestVersion && this.state.channel === channel;
}

private shouldRunAutomaticCheck(): boolean {
if (!this.state.autoCheckEnabled) {
return false;
Expand Down
137 changes: 136 additions & 1 deletion apps/desktop/tests/services/AppUpdateService/service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ function createController(
lastCheckedAt?: string | null;
checkResult?: AppUpdateCheckResult;
checkError?: Error;
updateChannelError?: Error;
} = {}
) {
const checkForUpdates = options.checkError
Expand All @@ -66,7 +67,9 @@ function createController(
);
const downloadUpdate = vi.fn().mockResolvedValue(availableUpdate);
const installUpdate = vi.fn().mockResolvedValue(true);
const updateAppUpdateChannel = vi.fn().mockResolvedValue(undefined);
const updateAppUpdateChannel = options.updateChannelError
? vi.fn().mockRejectedValue(options.updateChannelError)
: vi.fn().mockResolvedValue(undefined);
const updateAppUpdateAutoCheck = vi.fn().mockResolvedValue(undefined);
const updateAppUpdateLastCheckedAt = vi.fn().mockResolvedValue(undefined);

Expand Down Expand Up @@ -170,6 +173,33 @@ describe('AppUpdateController', () => {
});
});

it('does not restore an old channel after a stale automatic check fails', async () => {
const deferredCheck = createDeferred<AppUpdateCheckResult>();
const { controller, checkForUpdates, updateAppUpdateLastCheckedAt } = createController();
checkForUpdates.mockReturnValueOnce(deferredCheck.promise);

await controller.initialize();
const checkPromise = controller.checkNow('automatic');
await Promise.resolve();

expect(controller.getState()).toMatchObject({
status: 'checking',
channel: 'stable',
});

await controller.setChannel('nightly');
deferredCheck.reject(new Error('network unavailable'));

await expect(checkPromise).resolves.toBe(false);
expect(controller.getState()).toMatchObject({
status: 'idle',
channel: 'nightly',
error: null,
});
expect(updateAppUpdateLastCheckedAt).toHaveBeenCalledTimes(1);
expect(updateAppUpdateLastCheckedAt).toHaveBeenCalledWith(null);
});

it('surfaces manual check failures', async () => {
const { controller } = createController({
checkError: new Error('network unavailable'),
Expand Down Expand Up @@ -248,6 +278,111 @@ describe('AppUpdateController', () => {
});
});

it('does not persist stale check timestamps after the user switches channels', async () => {
const deferredCheck = createDeferred<AppUpdateCheckResult>();
const { controller, checkForUpdates, updateAppUpdateLastCheckedAt } = createController();
checkForUpdates.mockReturnValueOnce(deferredCheck.promise);

await controller.initialize();
const checkPromise = controller.checkNow('manual');
await Promise.resolve();

await controller.setChannel('nightly');
deferredCheck.resolve({
status: 'available',
channel: 'stable',
currentVersion: '0.1.0',
latest: latestUpdate,
update: availableUpdate,
requirement: neutralRequirement,
});

await expect(checkPromise).resolves.toBe(false);
expect(controller.getState()).toMatchObject({
status: 'idle',
channel: 'nightly',
availableUpdate: null,
lastCheckedAt: null,
});
expect(updateAppUpdateLastCheckedAt).toHaveBeenCalledTimes(1);
expect(updateAppUpdateLastCheckedAt).toHaveBeenCalledWith(null);
});

it('keeps in-flight checks current when channel persistence fails', async () => {
const deferredCheck = createDeferred<AppUpdateCheckResult>();
const { controller, checkForUpdates, updateAppUpdateLastCheckedAt } = createController({
updateChannelError: new Error('database unavailable'),
});
checkForUpdates.mockReturnValueOnce(deferredCheck.promise);

await controller.initialize();
const checkPromise = controller.checkNow('manual');
await Promise.resolve();

await expect(controller.setChannel('nightly')).rejects.toThrow('database unavailable');
deferredCheck.resolve({
status: 'available',
channel: 'stable',
currentVersion: '0.1.0',
latest: latestUpdate,
update: availableUpdate,
requirement: neutralRequirement,
});

await expect(checkPromise).resolves.toBe(true);
expect(controller.getState()).toMatchObject({
status: 'available',
channel: 'stable',
availableUpdate,
lastCheckedAt: '2026-05-22T10:00:00.000Z',
});
expect(updateAppUpdateLastCheckedAt).toHaveBeenCalledTimes(1);
expect(updateAppUpdateLastCheckedAt).toHaveBeenCalledWith('2026-05-22T10:00:00.000Z');
});

it('ignores an older check result when a newer same-channel check finishes first', async () => {
const firstCheck = createDeferred<AppUpdateCheckResult>();
const secondCheck = createDeferred<AppUpdateCheckResult>();
const { controller, checkForUpdates, updateAppUpdateLastCheckedAt } = createController();
checkForUpdates
.mockReturnValueOnce(firstCheck.promise)
.mockReturnValueOnce(secondCheck.promise);

await controller.initialize();
const firstPromise = controller.checkNow('manual');
await Promise.resolve();
const secondPromise = controller.checkNow('manual');
await Promise.resolve();

secondCheck.resolve({
status: 'available',
channel: 'stable',
currentVersion: '0.1.0',
latest: latestUpdate,
update: availableUpdate,
requirement: neutralRequirement,
});
await expect(secondPromise).resolves.toBe(true);

firstCheck.resolve({
status: 'not_available',
channel: 'stable',
currentVersion: '0.2.0',
latest: latestUpdate,
requirement: neutralRequirement,
});
await expect(firstPromise).resolves.toBe(false);

expect(controller.getState()).toMatchObject({
status: 'available',
channel: 'stable',
availableUpdate,
currentVersion: '0.1.0',
});
expect(updateAppUpdateLastCheckedAt).toHaveBeenCalledTimes(1);
expect(updateAppUpdateLastCheckedAt).toHaveBeenCalledWith('2026-05-22T10:00:00.000Z');
});

it('persists auto-check changes', async () => {
const { controller, updateAppUpdateAutoCheck } = createController();

Expand Down
Loading