Skip to content
19 changes: 19 additions & 0 deletions src/main/ipc/settings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ type SettingsChangedListener = (
const store = {
getSettings: vi.fn(),
updateSettings: vi.fn(),
updateSettingsAndFlush: vi.fn(),
getGitHubCache: vi.fn(),
setGitHubCache: vi.fn(),
onSettingsChanged: vi.fn(() => () => {})
Expand All @@ -107,6 +108,7 @@ const store = {
describe('registerSettingsHandlers', () => {
beforeEach(() => {
handleMock.mockClear()
store.updateSettingsAndFlush.mockReset()
onMock.mockClear()
applyAppIconMock.mockClear()
applyAgentStatusHooksEnabledMock.mockReset().mockResolvedValue([])
Expand Down Expand Up @@ -239,6 +241,23 @@ describe('registerSettingsHandlers', () => {
)
})

it('propagates a failed durable force-delete preference write through settings:set', async () => {
store.getSettings.mockReturnValue({ alwaysForceDeleteWorktrees: false })
store.updateSettingsAndFlush.mockRejectedValue(new Error('Disk full'))
registerSettingsHandlers(store as never)
const handler = handleMock.mock.calls.find((call) => call[0] === 'settings:set')?.[1]
if (typeof handler !== 'function') {
throw new Error('Missing settings:set handler')
}
await expect(
handler(settingsInvokeEvent, { alwaysForceDeleteWorktrees: true, editorWordWrap: false })
).rejects.toThrow('Disk full')
expect(store.updateSettingsAndFlush).toHaveBeenCalledWith(
{ alwaysForceDeleteWorktrees: true, editorWordWrap: false },
{ notifyListeners: true, originWebContentsId: 1 }
)
})

it('persists Active Server only through the dedicated preference channel', () => {
store.updateSettings.mockReturnValue({ activeRuntimeEnvironmentId: 'windows-2' })
registerSettingsHandlers(store as never)
Expand Down
8 changes: 6 additions & 2 deletions src/main/ipc/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,14 @@ export function registerSettingsHandlers(
// (e.g. blur after a no-op edit), and a `settings_changed` event for a
// no-op flip would inflate the experimental-feature-adoption signal.
const before = store.getSettings()
const result = store.updateSettings(sanitizedArgs, {
const updateOptions = {
notifyListeners: true,
originWebContentsId: event.sender.id
})
}
const result =
'alwaysForceDeleteWorktrees' in sanitizedArgs
? await store.updateSettingsAndFlush(sanitizedArgs, updateOptions)
: store.updateSettings(sanitizedArgs, updateOptions)
const proxySettingsChanged =
('httpProxyUrl' in sanitizedArgs && before.httpProxyUrl !== result.httpProxyUrl) ||
('httpProxyBypassRules' in sanitizedArgs &&
Expand Down
96 changes: 96 additions & 0 deletions src/main/persistence/loading-store/durable-settings-write.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import { expect, it, vi } from 'vitest'
import {
createWorkerMaintenanceFixture,
maintenanceBarrier
} from './profile-state-maintenance-fixture'

vi.mock('../../telemetry/client', () => ({ track: vi.fn() }))
vi.mock('../../telemetry/cohort-classifier', () => ({
getCohortAtEmit: () => ({ nth_repo_added: 2 })
}))
vi.mock('../../ssh/ssh-config-parser', () => ({
loadUserSshConfig: () => ({ hosts: [] }),
sshConfigHostsToTargets: () => []
}))

it('waits for the SQLite write before returning or publishing the preference', async () => {
const { store, authority, readState } = await createWorkerMaintenanceFixture()
const started = maintenanceBarrier()
const release = maintenanceBarrier()
const write = authority.writeSerializedDomains.bind(authority)
vi.spyOn(authority, 'writeSerializedDomains').mockImplementationOnce(async (domains) => {
started.resolve()
await release.promise
await write(domains)
})
const onChanged = vi.fn()
store.onSettingsChanged(onChanged)
let settled = false
const pending = store.updateSettingsAndFlush(
{ alwaysForceDeleteWorktrees: true },
{ notifyListeners: true, originWebContentsId: 7 }
)
void pending.then(() => {
settled = true
})
await started.promise
expect(settled).toBe(false)
expect(onChanged).not.toHaveBeenCalled()
store.updateSettings({ theme: 'dark' })
release.resolve()
expect((await pending).theme).toBe('dark')
expect(readState().settings.alwaysForceDeleteWorktrees).toBe(true)
expect(onChanged).toHaveBeenCalledWith(
{ alwaysForceDeleteWorktrees: true },
expect.objectContaining({ alwaysForceDeleteWorktrees: true }),
7
)
})

it('rejects a failed disk write and rolls back without publishing the preference', async () => {
const { store, authority, readState } = await createWorkerMaintenanceFixture()
const previous = store.getSettings().alwaysForceDeleteWorktrees
vi.spyOn(authority, 'writeSerializedDomains').mockRejectedValueOnce(new Error('Disk full'))
const onChanged = vi.fn()
store.onSettingsChanged(onChanged)
await expect(
store.updateSettingsAndFlush({ alwaysForceDeleteWorktrees: true }, { notifyListeners: true })
).rejects.toThrow('Disk full')
expect(store.getSettings().alwaysForceDeleteWorktrees).toBe(previous)
expect(readState().settings.alwaysForceDeleteWorktrees).toBe(previous)
expect(onChanged).not.toHaveBeenCalled()
})

it('rolls back the preference while retaining unrelated concurrent settings edits', async () => {
const { store, authority } = await createWorkerMaintenanceFixture()
const previous = store.getSettings().alwaysForceDeleteWorktrees
const started = maintenanceBarrier()
const release = maintenanceBarrier()
vi.spyOn(authority, 'writeSerializedDomains').mockImplementationOnce(async () => {
started.resolve()
await release.promise
throw new Error('Disk full')
})
const pending = store.updateSettingsAndFlush({ alwaysForceDeleteWorktrees: true })
const rejected = expect(pending).rejects.toThrow('Disk full')
await started.promise
store.updateSettings({ theme: 'dark' })
release.resolve()
await rejected
expect(store.getSettings()).toMatchObject({
alwaysForceDeleteWorktrees: previous,
theme: 'dark'
})
})

it('removes previously absent optional keys after a failed preference payload write', async () => {
const { store, authority } = await createWorkerMaintenanceFixture()
delete store.getSettings().editorWordWrap
const previous = store.getSettings().alwaysForceDeleteWorktrees
vi.spyOn(authority, 'writeSerializedDomains').mockRejectedValueOnce(new Error('Disk full'))
await expect(
store.updateSettingsAndFlush({ alwaysForceDeleteWorktrees: true, editorWordWrap: false })
).rejects.toThrow('Disk full')
expect(Object.hasOwn(store.getSettings(), 'editorWordWrap')).toBe(false)
expect(store.getSettings().alwaysForceDeleteWorktrees).toBe(previous)
})
47 changes: 47 additions & 0 deletions src/main/persistence/loading-store/profile-preferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type ProfilePreferencesRuntime = Pick<
| 'githubCacheDirty'
| 'githubCacheGeneration'
| 'protectedSecrets'
| 'runDurableMutation'
| 'settingsChangeListeners'
| 'state'
| 'uiChangeListeners'
Expand Down Expand Up @@ -74,6 +75,52 @@ export class ProfilePreferences {
return updateSettingsOperation(getSettingsMutationOperations(this), updates, options)
}

async updateSettingsAndFlush(
updates: Partial<GlobalSettings>,
options: { notifyListeners?: boolean; originWebContentsId?: number } = {}
): Promise<GlobalSettings> {
const { runtime } = this[profilePreferencesContext]
let changedUpdates: Partial<GlobalSettings> = {}
await runtime.runDurableMutation(() => {
const previous = runtime.state.settings
const next = this.updateSettings(updates)
const previousEntries = new Map(Object.entries(previous))
const updateKeys = new Set(Object.keys(updates))
changedUpdates = Object.fromEntries(
Object.entries(next).filter(
([key, value]) => updateKeys.has(key) && !Object.is(previousEntries.get(key), value)
)
)
return {
value: undefined,
rollback: () => {
const currentEntries = new Map(Object.entries(runtime.state.settings))
const nextEntries = new Map(Object.entries(next))
const restoredUpdates = Object.fromEntries(
Object.entries(previous).filter(
([key]) =>
updateKeys.has(key) && Object.is(currentEntries.get(key), nextEntries.get(key))
)
)
const restoredSettings = { ...runtime.state.settings, ...restoredUpdates }
for (const key of updateKeys) {
if (
!previousEntries.has(key) &&
Object.is(currentEntries.get(key), nextEntries.get(key))
) {
Reflect.deleteProperty(restoredSettings, key)
}
}
runtime.state.settings = restoredSettings
}
}
})
if (options.notifyListeners && Object.keys(changedUpdates).length > 0) {
notifySettingsChanged(this, changedUpdates, options.originWebContentsId)
}
return this.getSettings()
}

getUI(): PersistedState['ui'] {
return getPersistedUI(
this[profilePreferencesContext].runtime.state,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ it.each([true, false])(
await waitForPoll()
vi.advanceTimersByTime(TIMEOUT_MS)
await waitForPoll()
await waitForPoll()
expect(worker.terminate).toHaveBeenCalledOnce()
expect(settled).toBe(false)
worker.emit('exit', 1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ import {

const TIMEOUT_MS = 30_000

async function drainTimeoutCheck(): Promise<void> {
await waitForPoll()
await waitForPoll()
}

afterEach(() => {
vi.useRealTimers()
publishSystemResume()
Expand Down Expand Up @@ -48,14 +53,24 @@ function harness() {
}

describe('profile state writer deadline', () => {
it('lets a queued reply clear the deadline after the first poll', async () => {
const { deadline, onTimeout, unsubscribe, run } = harness()
run(TIMEOUT_MS)
setImmediate(() => deadline.clear())
await drainTimeoutCheck()
expect(onTimeout).not.toHaveBeenCalled()
expect(unsubscribe).toHaveBeenCalledOnce()
expect(vi.getTimerCount()).toBe(0)
})

it('times out a request after allowing queued replies to drain', async () => {
const { onTimeout, onGrace, unsubscribe, run } = harness()
run(TIMEOUT_MS - 1)
expect(onTimeout).not.toHaveBeenCalled()
run(1)
expect(onGrace).not.toHaveBeenCalled()
expect(onTimeout).not.toHaveBeenCalled()
await waitForPoll()
await drainTimeoutCheck()
expect(onTimeout).toHaveBeenCalledExactlyOnceWith(
expect.objectContaining({ overdueMs: 0, graces: 0, powerState: 'awake' })
)
Expand All @@ -74,7 +89,7 @@ describe('profile state writer deadline', () => {
run(TIMEOUT_MS - 1)
expect(onTimeout).not.toHaveBeenCalled()
run(1)
await waitForPoll()
await drainTimeoutCheck()
expect(onTimeout).toHaveBeenCalledExactlyOnceWith(
expect.objectContaining({ overdueMs: 0, graces: 1 })
)
Expand All @@ -85,7 +100,7 @@ describe('profile state writer deadline', () => {
stall(PROFILE_STATE_WRITER_OVERDUE_GRACE_MS - 1)
run(TIMEOUT_MS)
expect(onGrace).not.toHaveBeenCalled()
await waitForPoll()
await drainTimeoutCheck()
expect(onTimeout).toHaveBeenCalledOnce()
})

Expand All @@ -99,7 +114,7 @@ describe('profile state writer deadline', () => {
stall(60 * 60_000)
run(TIMEOUT_MS)
expect(onGrace).toHaveBeenCalledTimes(PROFILE_STATE_WRITER_MAX_GRACES)
await waitForPoll()
await drainTimeoutCheck()
expect(onTimeout).toHaveBeenCalledExactlyOnceWith(
expect.objectContaining({ graces: PROFILE_STATE_WRITER_MAX_GRACES })
)
Expand All @@ -114,7 +129,7 @@ describe('profile state writer deadline', () => {
run(TIMEOUT_MS - 1)
expect(onTimeout).not.toHaveBeenCalled()
run(1)
await waitForPoll()
await drainTimeoutCheck()
expect(onGrace).toHaveBeenCalledExactlyOnceWith(expect.objectContaining({ graces: 1 }))
expect(onTimeout).toHaveBeenCalledOnce()
})
Expand All @@ -129,7 +144,7 @@ describe('profile state writer deadline', () => {
expect(onGrace).toHaveBeenCalledTimes(PROFILE_STATE_WRITER_MAX_GRACES)
expect(onTimeout).not.toHaveBeenCalled()
run(TIMEOUT_MS)
await waitForPoll()
await drainTimeoutCheck()
expect(onTimeout).toHaveBeenCalledExactlyOnceWith(
expect.objectContaining({ powerState: 'suspended' })
)
Expand All @@ -148,7 +163,7 @@ describe('profile state writer deadline', () => {
power().onResume()
}
run(1)
await waitForPoll()
await drainTimeoutCheck()
expect(onTimeout).toHaveBeenCalledExactlyOnceWith(
expect.objectContaining({ graces: PROFILE_STATE_WRITER_MAX_GRACES })
)
Expand All @@ -170,7 +185,7 @@ describe('profile state writer deadline', () => {
run(TIMEOUT_MS)
// An exhausted resume must not cancel the final queued timeout check.
power().onResume()
await waitForPoll()
await drainTimeoutCheck()
expect(onGrace).toHaveBeenCalledTimes(PROFILE_STATE_WRITER_MAX_GRACES)
expect(onTimeout).toHaveBeenCalledOnce()
expect(vi.getTimerCount()).toBe(0)
Expand All @@ -183,7 +198,7 @@ describe('profile state writer deadline', () => {
const { deadline, onTimeout, unsubscribe, run } = harness()
run(TIMEOUT_MS)
deadline.clear()
await waitForPoll()
await drainTimeoutCheck()
expect(onTimeout).not.toHaveBeenCalled()
expect(unsubscribe).toHaveBeenCalledOnce()
expect(vi.getTimerCount()).toBe(0)
Expand All @@ -193,10 +208,10 @@ describe('profile state writer deadline', () => {
const { onTimeout, run, power } = harness()
run(TIMEOUT_MS)
power().onResume()
await waitForPoll()
await drainTimeoutCheck()
expect(onTimeout).not.toHaveBeenCalled()
run(TIMEOUT_MS)
await waitForPoll()
await drainTimeoutCheck()
expect(onTimeout).toHaveBeenCalledOnce()
expect(vi.getTimerCount()).toBe(0)
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,15 @@ export function createProfileStateWriterDeadline(
) {
return
}
// Even an on-time timer can run before a reply already queued by the worker.
// Windows can deliver a queued worker reply after the first poll following a stall.
timeoutCheck = setImmediate(() => {
timeoutCheck = undefined
if (active) {
clear()
onTimeout(expiry)
}
timeoutCheck = setImmediate(() => {
timeoutCheck = undefined
if (active) {
clear()
onTimeout(expiry)
}
})
})
}
// Subscription replays the current state synchronously; only later resumes re-arm.
Expand Down
Loading
Loading