mirror of
https://github.com/stablyai/orca.git
synced 2026-10-06 00:02:43 +00:00
fix(speech): coalesce model download progress churn on the Settings/Voice path (#11962)
* fix(speech): coalesce model download progress churn A model download emits one state update per HTTP chunk (thousands for a 500MB model). Each one costs the renderer an IPC round-trip and a forced re-render of the speech-model menu, which stays open by design while a download runs — the Radix portal/Presence tree all four page.settings React #185 reports crashed inside. Emit at whole-percent granularity (what the UI renders) and keep the modelStates array identity stable when a refresh changed nothing, so a no-op refresh no longer forces a commit. Adds a speech_model_state_churn breadcrumb, registered in both coalescing sets, because no bundle in the cluster carried any speech telemetry to confirm a download was in flight. * fix(speech): quantise polled model state so download progress stops churning the renderer Adversarial round 1 found the renderer-side stabilisation was inert during a real download. The progress fan-out already coalesces to whole percent, but the renderer discards the event payload and re-polls getModelStates, and getModelState returned the cached downloading state verbatim - raw sub-percent progress. So every chunk produced a fresh object, resolveModelStates never matched, and all the real benefit came from the main-side coalescing alone. Quantise the polled reply to the precision the UI already renders (Math.round(progress * 100)), keeping the stored cache exact. Also from round 1: - the whole-status dedup swallowed downloadModel's already-downloaded branch, whose lone 'ready' is the only notification the requesting window ever gets - a dead click and a permanently stale pane with two windows open. Gate it on downloading -> downloading so every one-shot transition stays unconditional. - rename the storm test off '.react185.': it counts renders and asserts nothing about #185, and #185 could not be reproduced at IPC-realistic pacing. * fix(speech): stop the churn breadcrumb firing on every healthy download Adversarial round 2. CHURN_REFRESH_THRESHOLD was 60 per 5s window, but main clamps download progress at 0.9 and emits on whole-percent change, so one healthy download is capped at ~91 refreshes — and a 56MB model on a fast link lands all of them inside a single window. The breadcrumb fired on every normal download and burned a slot in the 30-entry ring it was coalesced into to protect. Raise it to 250 so it only fires on the per-chunk shape it was added to detect; noOpRefreshes in the payload still separates the two causes. Round 2 also found that every assertion round 1 added was one-sided (toBeLessThanOrEqual), so each of the three core behaviours could be mutated into "do nothing" with the whole suite still green: - suppress every downloading -> downloading event: progress bar frozen at 0% - toWholePercentState returning 0: every poll reports 0% - resolveModelStates never adopting a same-length change: the Voice pane never updates and a finished model never shows as ready The suite measured that the fix reduces work, never that it still does the work. Convert the two ceilings to exact series, assert the storm test's render floor as well as its ceiling, and add dictation-model-state-stabilisation.test.ts covering adoption per changed field, a full whole-percent download, and both sides of the churn threshold. Ruled out and deliberately not fixed: round 1's request-sequencing MEDIUM on refreshModelStates. 50 concurrent getModelStates() settle strictly FIFO over ipcRenderer.invoke at constant microtask depth, and migrationReady is assigned once in the constructor, so a monotonic request id would be dead code. * test(crash-reporting): cover speech churn breadcrumb name-coalescing The churn breadcrumb's registration in COALESCED_RENDERER_BREADCRUMB_NAMES and NAME_ONLY_COALESCED_BREADCRUMB_NAMES had no test: the storm test only asserted the constant equals its own literal. Removing either registration kept the whole suite green. It carries no message field, so without the name-only entry rendererBreadcrumbCoalesceKey returns undefined and every firing takes its own ring slot — the eviction this breadcrumb exists to avoid. * test(dictation): pin the churn threshold as a rate, not a lifetime total Both existing churn tests stopped at exactly 250 refreshes in one window, so two mutations survived: deleting the per-window reset (the counter degrades into a session total, and three healthy downloads at 91 refreshes each cry wolf), and === to >= (fires on every refresh past the threshold, flooding the ring). Pins Date.now rather than using fake timers so the window rule is measured, not the machine. * test(dictation): pin the churn clock and the window's lower bound The two 250-refresh churn tests measured real elapsed time against a 5s window, so a loaded runner that took longer than one window to run the loop would roll the window and go red. Pin Date.now in both. Pinning alone leaves CHURN_WINDOW_MS unpinned below: shrinking it 5_000 -> 50 kept all 13 tests green, yet a 50ms window can never accumulate 250 refreshes and the detector would be dead. Add a storm spread across most of one window so the constant has to be wide enough to hold a sustained storm, not just an instantaneous burst. Co-authored-by: Orca <help@stably.ai> * refactor(speech): narrow progress churn fix --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -45,4 +45,30 @@ describe('ModelManager progress callbacks', () => {
|
||||
rmSync(dir, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
it('coalesces per-chunk download progress to whole percent', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'orca-model-manager-'))
|
||||
try {
|
||||
const manager = new ModelManager(dir)
|
||||
const internals = manager as unknown as ModelManagerInternals
|
||||
const listener = vi.fn()
|
||||
manager.setProgressCallback(listener)
|
||||
|
||||
// A 500MB model over a 64KB chunk stream reports this many times.
|
||||
for (let chunk = 0; chunk < 8_000; chunk += 1) {
|
||||
internals.updateState('model-a', 'downloading', chunk / 8_000)
|
||||
}
|
||||
|
||||
expect(listener.mock.calls.map(([, progress]) => progress)).toEqual(
|
||||
Array.from({ length: 101 }, (_unused, percent) => percent / 100)
|
||||
)
|
||||
const afterDownload = listener.mock.calls.length
|
||||
internals.updateState('model-a', 'extracting')
|
||||
internals.updateState('model-a', 'ready')
|
||||
internals.updateState('model-a', 'ready')
|
||||
expect(listener.mock.calls.length).toBe(afterDownload + 3)
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
@@ -387,10 +387,24 @@ export class ModelManager {
|
||||
progress?: number,
|
||||
error?: string
|
||||
): void {
|
||||
const state: SpeechModelState = { id: modelId, status, progress, error }
|
||||
const previous = this.modelStates.get(modelId)
|
||||
// Whole-percent state matches the UI and prevents chunk-level IPC/poll churn.
|
||||
const reportedProgress =
|
||||
status === 'downloading' && progress !== undefined
|
||||
? Math.round(progress * 100) / 100
|
||||
: progress
|
||||
if (
|
||||
status === 'downloading' &&
|
||||
previous?.status === 'downloading' &&
|
||||
previous.error === error &&
|
||||
previous.progress === reportedProgress
|
||||
) {
|
||||
return
|
||||
}
|
||||
const state: SpeechModelState = { id: modelId, status, progress: reportedProgress, error }
|
||||
this.modelStates.set(modelId, state)
|
||||
// Why: notify on every state change (not just progress) so extracting/ready/error transitions reach the UI.
|
||||
const progressValue = progress ?? (status === 'extracting' ? 0.95 : -1)
|
||||
// Repeated non-download states can be the requesting window's only resync signal.
|
||||
const progressValue = reportedProgress ?? (status === 'extracting' ? 0.95 : -1)
|
||||
for (const callback of this.progressCallbacks) {
|
||||
callback(modelId, progressValue)
|
||||
}
|
||||
|
||||
@@ -0,0 +1,62 @@
|
||||
/** @vitest-environment happy-dom */
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { create, type StateCreator } from 'zustand'
|
||||
import type { SpeechModelState } from '../../../../shared/speech-types'
|
||||
import type { AppState } from '../types'
|
||||
import { createDictationSlice } from './dictation'
|
||||
|
||||
type DictationTestStore = Pick<AppState, 'modelStates' | 'refreshModelStates' | 'setModelStates'>
|
||||
const dictationSlice = createDictationSlice as unknown as StateCreator<DictationTestStore>
|
||||
|
||||
let reply: SpeechModelState[]
|
||||
|
||||
beforeEach(() => {
|
||||
reply = []
|
||||
Object.assign(window, {
|
||||
api: {
|
||||
speech: { getModelStates: vi.fn(async () => reply.map((state) => ({ ...state }))) }
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe('dictation model-state stabilisation', () => {
|
||||
it('does not publish an unchanged reply', async () => {
|
||||
reply = [{ id: 'whisper-tiny', status: 'downloading', progress: 0.42 }]
|
||||
const store = create<DictationTestStore>(dictationSlice)
|
||||
await store.getState().refreshModelStates()
|
||||
const previous = store.getState()
|
||||
const subscriber = vi.fn()
|
||||
store.subscribe(subscriber)
|
||||
|
||||
await store.getState().refreshModelStates()
|
||||
|
||||
expect(store.getState()).toBe(previous)
|
||||
expect(subscriber).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it.each([
|
||||
{ changed: [{ id: 'whisper-tiny', status: 'downloading', progress: 0.43 }] },
|
||||
{ changed: [{ id: 'whisper-tiny', status: 'ready' }] },
|
||||
{ changed: [{ id: 'whisper-tiny', status: 'error', error: 'boom' }] },
|
||||
{
|
||||
changed: [{ id: 'parakeet-tdt-0.6b-v3-int8', status: 'downloading', progress: 0.42 }]
|
||||
},
|
||||
{
|
||||
changed: [
|
||||
{ id: 'whisper-tiny', status: 'downloading', progress: 0.42 },
|
||||
{ id: 'parakeet-tdt-0.6b-v3-int8', status: 'not-downloaded' }
|
||||
]
|
||||
}
|
||||
] as { changed: SpeechModelState[] }[])('publishes a changed reply', async ({ changed }) => {
|
||||
const store = create<DictationTestStore>(dictationSlice)
|
||||
store.getState().setModelStates([{ id: 'whisper-tiny', status: 'downloading', progress: 0.42 }])
|
||||
const subscriber = vi.fn()
|
||||
store.subscribe(subscriber)
|
||||
|
||||
reply = changed
|
||||
await store.getState().refreshModelStates()
|
||||
|
||||
expect(store.getState().modelStates).toEqual(changed)
|
||||
expect(subscriber).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
})
|
||||
@@ -14,23 +14,49 @@ export type DictationSlice = {
|
||||
refreshModelStates: () => Promise<void>
|
||||
}
|
||||
|
||||
export const createDictationSlice: StateCreator<AppState, [], [], DictationSlice> = (set) => ({
|
||||
dictationState: 'idle',
|
||||
partialTranscript: '',
|
||||
activeModelId: null,
|
||||
modelStates: [],
|
||||
function sameSpeechModelState(a: SpeechModelState, b: SpeechModelState): boolean {
|
||||
return a.id === b.id && a.status === b.status && a.progress === b.progress && a.error === b.error
|
||||
}
|
||||
|
||||
setDictationState: (state) => set({ dictationState: state }),
|
||||
setPartialTranscript: (text) => set({ partialTranscript: text }),
|
||||
setActiveModelId: (id) => set({ activeModelId: id }),
|
||||
setModelStates: (states) => set({ modelStates: states }),
|
||||
// Why: every getModelStates reply is a fresh array, so without this each no-op
|
||||
// refresh re-renders every subscriber — including the open speech-model menu.
|
||||
function resolveModelStates(
|
||||
previous: SpeechModelState[],
|
||||
next: SpeechModelState[]
|
||||
): SpeechModelState[] {
|
||||
if (previous.length !== next.length) {
|
||||
return next
|
||||
}
|
||||
return previous.every((state, index) => sameSpeechModelState(state, next[index]))
|
||||
? previous
|
||||
: next
|
||||
}
|
||||
|
||||
refreshModelStates: async () => {
|
||||
try {
|
||||
const states = await window.api.speech.getModelStates()
|
||||
set({ modelStates: states })
|
||||
} catch (err) {
|
||||
console.error('Failed to fetch model states:', err)
|
||||
export const createDictationSlice: StateCreator<AppState, [], [], DictationSlice> = (set) => {
|
||||
const setModelStates = (states: SpeechModelState[]): void => {
|
||||
set((prev) => {
|
||||
const modelStates = resolveModelStates(prev.modelStates, states)
|
||||
return modelStates === prev.modelStates ? prev : { modelStates }
|
||||
})
|
||||
}
|
||||
|
||||
return {
|
||||
dictationState: 'idle',
|
||||
partialTranscript: '',
|
||||
activeModelId: null,
|
||||
modelStates: [],
|
||||
|
||||
setDictationState: (state) => set({ dictationState: state }),
|
||||
setPartialTranscript: (text) => set({ partialTranscript: text }),
|
||||
setActiveModelId: (id) => set({ activeModelId: id }),
|
||||
setModelStates,
|
||||
|
||||
refreshModelStates: async () => {
|
||||
try {
|
||||
setModelStates(await window.api.speech.getModelStates())
|
||||
} catch (err) {
|
||||
console.error('Failed to fetch model states:', err)
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user