From 73607663fd5ce42dd0fa59255b0c8640b7bbb0e9 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Sun, 20 Sep 2026 19:27:07 +0800 Subject: [PATCH] fix(custom-model): guard the picker's async open against a slower, superseded probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review (high effort) on the previous commit found a real race: making _openCustomModelPickModal async (it now awaits the currently-loaded-model probe before rendering) meant a second, faster call for a different endpoint could render first, only for the first call's slower probe to resolve afterwards and overwrite the modal with the wrong endpoint's model list — while _pendingCustomModelPick (set synchronously, before either await) still named the second, correct endpoint. Picking a model in that state would launch/apply the wrong model on the wrong endpoint. Fixed with the same mutable-generation-counter guard _watchLlamaSwapLoading already uses for an identical async-superseded-by- newer-call shape: every DOM write, including _pendingCustomModelPick itself, is deferred until after the awaited probe, and a call that finds its generation already superseded bails out untouched instead of clobbering whatever a newer call already rendered. Added a regression test driving two overlapping opens with a controlled promise so the earlier, slower probe resolves after the later, faster one renders, asserting the late response is a no-op. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01N6eadpRyqpA9PD3i139cSD --- src/web/public/session-ui.js | 33 +++++++++++++++++------- test/custom-model-run-menu-ui.test.ts | 36 +++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 62c6ca49..5d379e8b 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -678,19 +678,25 @@ Object.assign(CodemanApp.prototype, { } }, - /** Renders the "which model" picker for a (harness, endpoint) pair with more than one discovered model. */ + /** + * Renders the "which model" picker for a (harness, endpoint) pair with more than one + * discovered model. Async since it now awaits the currently-loaded-model probe below, + * so a SECOND call (a different custom-model entry clicked while the first one's probe + * is still in flight — the probe has its own 5s timeout) must not let the first call's + * later-arriving response clobber the second's already-rendered, already-correct modal. + * `_customModelPickGeneration` is the same guard-a-mutable-counter pattern + * `_watchLlamaSwapLoading` uses for the same reason: every DOM write below, including + * `_pendingCustomModelPick` itself, stays deferred until after the await, and a call + * that finds a newer generation already claimed bails out untouched rather than only + * skipping the model-list write and leaving title/hint/`_pendingCustomModelPick` + * inconsistent with what's on screen. + */ async _openCustomModelPickModal(mode, host) { const modal = document.getElementById('customModelPickModal'); const list = document.getElementById('customModelPickList'); if (!modal || !list) return; - this._pendingCustomModelPick = { mode, endpointId: host.id }; - const cliLabel = (window.__codemanCustomModelClis || []).find((c) => c.id === mode)?.label || mode; - // A static title (translatable by i18n.js's exact-string walker) plus a - // dynamic hint carrying the specifics — same split webviewModalTitle uses, - // since the walker cannot i18n a string a variable is already spliced into. - document.getElementById('customModelPickTitle').textContent = 'Choose a model'; - document.getElementById('customModelPickHint').textContent = - `${cliLabel} → ${host.label} — ${(host.models || []).length} models discovered.`; + const generation = (this._customModelPickGeneration = (this._customModelPickGeneration || 0) + 1); + const isCurrent = () => this._customModelPickGeneration === generation; // Whichever model llama-swap actually has loaded right now beats a merely // remembered choice — it's what a launch would attach to with zero wait, while @@ -698,6 +704,7 @@ Object.assign(CodemanApp.prototype, { // reorders past the top: exactly one model is promoted, everything else keeps // its discovery order. const currentlyLoaded = await this._getCustomModelCurrentlyLoaded(host); + if (!isCurrent()) return; // a newer pick opened (and possibly already rendered) while this probe was in flight const lastUsed = currentlyLoaded ? null : this._getCustomModelLastUsed(mode, host.id); const promoted = currentlyLoaded || lastUsed; const models = [...(host.models || [])]; @@ -706,6 +713,14 @@ Object.assign(CodemanApp.prototype, { models.unshift(promoted); } + this._pendingCustomModelPick = { mode, endpointId: host.id }; + const cliLabel = (window.__codemanCustomModelClis || []).find((c) => c.id === mode)?.label || mode; + // A static title (translatable by i18n.js's exact-string walker) plus a + // dynamic hint carrying the specifics — same split webviewModalTitle uses, + // since the walker cannot i18n a string a variable is already spliced into. + document.getElementById('customModelPickTitle').textContent = 'Choose a model'; + document.getElementById('customModelPickHint').textContent = + `${cliLabel} → ${host.label} — ${(host.models || []).length} models discovered.`; list.innerHTML = models .map((m) => { const isDefault = m === host.defaultModelId; diff --git a/test/custom-model-run-menu-ui.test.ts b/test/custom-model-run-menu-ui.test.ts index b7c292dc..ab27a9f6 100644 --- a/test/custom-model-run-menu-ui.test.ts +++ b/test/custom-model-run-menu-ui.test.ts @@ -367,6 +367,42 @@ describe('Custom Model Endpoint Profiles: the "which model" picker', () => { expect(win.localStorage.getItem('codeman:customModelLastUsed:claude:llama-box')).toBe('qwen3'); }); + it('a slower currently-loaded probe for an earlier pick must never clobber a faster, later pick for a different endpoint', async () => { + const { win, app } = bootApp({}); + const hostA = { id: 'host-a', label: 'Host A', baseUrl: 'http://a', models: ['a1', 'a2'] }; + const hostB = { id: 'host-b', label: 'Host B', baseUrl: 'http://b', models: ['b1', 'b2'] }; + let resolveA!: (v: unknown) => void; + const pendingA = new Promise((resolve) => { + resolveA = resolve; + }); + app._apiJson = async (path: string) => { + if (path === '/api/model-endpoints/host-a/running-status') return pendingA; + if (path === '/api/model-endpoints/host-b/running-status') return { isLlamaSwap: false, running: [] }; + return null; + }; + + // Host A's picker opens first but its probe never resolves until we say so below — + // Host B's opens second and resolves immediately, so it renders first. + const openA = app._openCustomModelPickModal('claude', hostA); + await app._openCustomModelPickModal('claude', hostB); + + expect(win.document.getElementById('customModelPickHint')!.textContent).toContain('Host B'); + expect(app._pendingCustomModelPick).toEqual({ mode: 'claude', endpointId: 'host-b' }); + + // Host A's probe finally answers, after Host B has already rendered. + resolveA({ isLlamaSwap: false, running: [] }); + await openA; + + // The late-arriving Host A response must be a no-op: still Host B on screen. + expect(win.document.getElementById('customModelPickHint')!.textContent).toContain('Host B'); + expect(app._pendingCustomModelPick).toEqual({ mode: 'claude', endpointId: 'host-b' }); + const listText = win.document.getElementById('customModelPickList')!.textContent; + expect(listText).toContain('b1'); + expect(listText).toContain('b2'); + expect(listText).not.toContain('a1'); + expect(listText).not.toContain('a2'); + }); + it('picking a row in the modal closes it and launches with that exact model', async () => { const { win, app } = bootApp({ hosts: [{ id: 'llama-box', label: 'llama.cpp', baseUrl: 'http://localhost:8080', models: ['qwen3', 'llama3'] }],