diff --git a/studio/frontend/src/features/api-monitor/new-traffic.ts b/studio/frontend/src/features/api-monitor/new-traffic.ts index 526e6ead42..b430565ae9 100644 --- a/studio/frontend/src/features/api-monitor/new-traffic.ts +++ b/studio/frontend/src/features/api-monitor/new-traffic.ts @@ -26,10 +26,17 @@ export interface ApiMonitorWatch { /** performance.now() when this watch began; monotonic, so a client clock step * mid-session cannot move it. */ watchStartedAt: number; + /** This seed follows a stay on the full page rather than starting a session. */ + resumed: boolean; } export function createWatch(nowMs: number): ApiMonitorWatch { - return { seeded: false, seenIds: new Set(), watchStartedAt: nowMs }; + return { + seeded: false, + seenIds: new Set(), + watchStartedAt: nowMs, + resumed: false, + }; } /** @@ -47,6 +54,7 @@ export function startWatching(watch: ApiMonitorWatch, nowMs: number): void { /** The full page took over; what it showed is not new traffic on the way back. */ export function rearmWatch(watch: ApiMonitorWatch): void { watch.seeded = false; + watch.resumed = true; } /** @@ -96,9 +104,19 @@ export function observeResponse( const { entries } = response; if (!watch.seeded) { watch.seeded = true; + const { resumed } = watch; + watch.resumed = false; const cutoff = historyCutoff(watch, response, nowMs); + // A rearm is not a fresh watch. isHistory holds a running row back on + // purpose: at a session's first snapshot it started while Studio was still + // loading and nobody has seen it. On the way off the full page the opposite + // is true -- that page was showing this same feed, running rows included -- + // so seeding from isHistory alone reopens the overlay on the request the + // user was reading when they left. Everything the page could show is read. watch.seenIds = new Set( - entries.filter((entry) => isHistory(entry, cutoff)).map((e) => e.id), + entries + .filter((entry) => resumed || isHistory(entry, cutoff)) + .map((e) => e.id), ); } const seen = watch.seenIds; diff --git a/studio/frontend/src/features/hub/hub-page.tsx b/studio/frontend/src/features/hub/hub-page.tsx index 4a5c8fc3bc..83032b9f02 100644 --- a/studio/frontend/src/features/hub/hub-page.tsx +++ b/studio/frontend/src/features/hub/hub-page.tsx @@ -387,9 +387,11 @@ export function ModelsPage() { // Drops a response that lands after a newer read started, or after unmount. const residentStatusSeq = useRef(0); - const refreshResidentModelStatus = useCallback(() => { + // Returns the read so a caller that needs the answer before it decides + // anything can wait for it; fire-and-forget callers just drop it. + const refreshResidentModelStatus = useCallback((): Promise => { const seq = ++residentStatusSeq.current; - void getInferenceStatus() + return getInferenceStatus() .then((status) => { if (seq !== residentStatusSeq.current) return; const store = useChatRuntimeStore.getState(); @@ -437,7 +439,7 @@ export function ModelsPage() { // external selection and for a load this tab started, so a refresh never fights // the model the user is switching to. useEffect(() => { - refreshResidentModelStatus(); + void refreshResidentModelStatus(); const unsubscribe = subscribeResidentStatusRefresh( refreshResidentModelStatus, ); @@ -1305,8 +1307,12 @@ export function ModelsPage() { // is seeded once, from saved/default values when the store says this model is // not the resident one, and Apply reloads with them. Reading status here covers // a switch that landed while this window kept focus the whole time. + // + // It cannot cover the resolution of the target itself, though: a target has to + // exist before this runs. openModelSettings therefore does its own read; this + // one is for the handlers that hand the target over ready-made. useEffect(() => { - if (settingsTarget) refreshResidentModelStatus(); + if (settingsTarget) void refreshResidentModelStatus(); }, [settingsTarget, refreshResidentModelStatus]); // Bumped per open so a slow variant lookup for an abandoned row cannot land // on top of the row actually chosen. @@ -1332,17 +1338,27 @@ export function ModelsPage() { const repoId = row.kind === "cache" ? row.repoId : (row.repoId ?? null); if (repoId) { try { - const res = await listGgufVariants(repoId, hfApiToken(hfToken), { - preferLocalCache: true, - localPath: - row.kind === "local" ? row.path : (row.cachePath ?? null), - }); + const [res] = await Promise.all([ + listGgufVariants(repoId, hfApiToken(hfToken), { + preferLocalCache: true, + localPath: + row.kind === "local" ? row.path : (row.cachePath ?? null), + }), + // Read status as part of this click. The effect above cannot help + // here, because it does not run until the target it watches exists, + // and the Hub has no polling timer: it re-reads on focus and + // visibility only. So a window that has kept focus since the last + // read holds a checkpoint from before any API-driven switch, for + // however long the user has been sitting on the page. Alongside the + // variant lookup rather than before it, since that one usually hits + // the network while this is a loopback call. + refreshResidentModelStatus(), + ]); const downloaded = res.variants.filter((v) => v.downloaded); - // Re-read residency after the await. Opening settings also kicks a - // status refresh, and this request usually hits the network, so the - // refresh routinely lands first; the values closed over above then - // describe whichever model was resident BEFORE an API-driven switch, - // and the loaded-quant branch would silently pick the wrong one. + // Read residency after the awaits, never from the values closed over + // above: the status read that just settled is what knows which model + // is resident now, and the loaded-quant branch below would otherwise + // silently pick the quant of whichever model it displaced. const settled = useChatRuntimeStore.getState(); const settledCheckpoint = settled.params.checkpoint && @@ -1413,7 +1429,7 @@ export function ModelsPage() { }, }); }, - [activeCheckpoint, activeGgufVariant, hfToken], + [activeCheckpoint, activeGgufVariant, hfToken, refreshResidentModelStatus], ); // Applying loads the model with exactly these settings. ModelConfigPage has // already persisted them locally and, when "remember" is on, to the server, so diff --git a/studio/frontend/tests/api-monitor-new-traffic.test.ts b/studio/frontend/tests/api-monitor-new-traffic.test.ts index 19ba42d5d0..fba8a002ff 100644 --- a/studio/frontend/tests/api-monitor-new-traffic.test.ts +++ b/studio/frontend/tests/api-monitor-new-traffic.test.ts @@ -138,3 +138,50 @@ test("coming back from the full page does not replay the rows it showed", () => ); assert.equal(opened, false); }); + +test("a request still running when the full page is left does not reopen the overlay", () => { + // The user opened /api-monitor to watch a long generation, then went back to + // chat while it was still running. That row was on screen the whole time. + const watch = watchFrom(WATCH_AT); + const live = entry("apireq_live", "running", SERVER_NOW - 5); + observeResponse(watch, snapshot([live]), WATCH_AT + 10); + rearmWatch(watch); + startWatching(watch, WATCH_AT + 60_000); + const opened = observeResponse( + watch, + snapshot([live], SERVER_NOW + 60), + WATCH_AT + 60_010, + ); + assert.equal(opened, false); +}); + +test("a rearm writes off only the snapshot it comes back to", () => { + // The write-off is one seed, not a mode: a call that arrives after the return + // is still new traffic. + const watch = watchFrom(WATCH_AT); + const live = entry("apireq_live", "running", SERVER_NOW - 5); + observeResponse(watch, snapshot([live]), WATCH_AT + 10); + rearmWatch(watch); + startWatching(watch, WATCH_AT + 60_000); + observeResponse(watch, snapshot([live], SERVER_NOW + 60), WATCH_AT + 60_010); + const opened = observeResponse( + watch, + snapshot( + [entry("apireq_next", "running", SERVER_NOW + 61), live], + SERVER_NOW + 62, + ), + WATCH_AT + 62_010, + ); + assert.equal(opened, true); +}); + +test("a fresh watch still reports a request that was already running", () => { + // The rearm write-off must not become the default seed for a session that + // never saw the full page. + const opened = observeResponse( + watchFrom(WATCH_AT), + snapshot([entry("apireq_live", "running", SERVER_NOW - 90)]), + WATCH_AT + 10, + ); + assert.equal(opened, true); +}); diff --git a/tests/studio/test_model_picker_contracts.py b/tests/studio/test_model_picker_contracts.py index 7108a9ae03..1655b79081 100644 --- a/tests/studio/test_model_picker_contracts.py +++ b/tests/studio/test_model_picker_contracts.py @@ -1541,3 +1541,20 @@ def test_monitor_unload_clears_only_the_model_it_freed(): assert "!isExternalModelId(selected)" in page assert "unloadedAliases.some((alias) => modelIdsMatch(selected, alias))" in page assert "store.clearCheckpoint();" in page + + +def test_settings_open_reads_status_before_resolving_the_quant(): + """A cache row carries no quant, so opening its settings resolves one from + the store's active variant. The effect that re-reads /status watches + settingsTarget and so cannot run until the target already exists, and the + Hub has no polling timer: it re-reads on focus and visibility only. Without + a read of its own the resolution therefore sees a checkpoint from before any + API-driven switch, for as long as the window has kept focus, and opens the + editor on the quant of whichever model that switch displaced.""" + page = " ".join(_read("features/hub/hub-page.tsx").split()) + assert ( + "const refreshResidentModelStatus = useCallback((): Promise => {" + in page + ) + assert "const [res] = await Promise.all([ listGgufVariants(" in page + assert "refreshResidentModelStatus(), ]);" in page