fix(custom-model): guard the picker's async open against a slower, superseded probe

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N6eadpRyqpA9PD3i139cSD
This commit is contained in:
Devvyn
2026-09-20 19:27:07 +08:00
co-authored by Claude Sonnet 5
parent 458ca578e7
commit 73607663fd
2 changed files with 60 additions and 9 deletions
+24 -9
View File
@@ -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;
+36
View File
@@ -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'] }],