From 8b0a050427dd5d67027f083966b94dbc0a237f0e Mon Sep 17 00:00:00 2001 From: Rafael Date: Tue, 1 Sep 2026 18:47:11 -0300 Subject: [PATCH] fix(provider-setup): name the stalled enable write instead of blaming the model picker (#1649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The flush gate added in #1651 gave up eight times on the 2026-09-01 daily, every one reading "30 toggle(s) clicked, 1 write(s) started, 0 finished" — the batched POST /api/v1/models/enabled_models was issued and never answered within 90 s. The gate warns rather than throws, so the panel closes anyway, the close-path flush never refreshes the picker, and the picker keeps the pre-toggle set. The picker is correct there: aria-checked is the optimistic client cache and flips before any request, so the panel is the source that lies. The failure 90 s later nonetheless reported MODEL_PICKER_DEFECT and named two hypotheses nobody had measured. Carry the batch's own observation forward. writeStallReason() and modelTriggerStallMessage() are pure; resolveModelOption gains a write-stalled verdict consulted inside the existing checked-includes-requested branch, after empty/match/unmatchable; the three provider setups capture the batch result they were discarding and open the picker through one shared helper that re-throws a model_model timeout as the same named stall, keeping Playwright's original message. No assertion weakened, no budget raised, nothing became a skip. A picker miss after a settled batch is still MODEL_PICKER_DEFECT, and an unobserved batch leaves every existing verdict byte-identical. This does not make a saturated day green — those specs still fail, naming the instance instead of the picker. The run-level cause that puts a spec on this path (collect-models confirming 0 of 74 enable writes) is #1666. Closes #1649 Co-Authored-By: Claude Opus 5 --- .../llm-agents/agent-context-id-continuity.md | 11 +- .../provider-setup/model-option.test.ts | 101 ++++++++++++++++++ tests/helpers/provider-setup/model-option.ts | 92 ++++++++++++++++ .../provider-setup/model-toggle-batch.test.ts | 101 +++++++++++++++++- .../provider-setup/model-toggle-batch.ts | 99 ++++++++++++++++- .../helpers/provider-setup/setup-anthropic.ts | 35 +++--- tests/helpers/provider-setup/setup-google.ts | 35 +++--- tests/helpers/provider-setup/setup-openai.ts | 39 +++---- .../core-functionality/llm-agents/CLAUDE.md | 52 ++++++++- 9 files changed, 505 insertions(+), 60 deletions(-) diff --git a/docs/core-functionality/llm-agents/agent-context-id-continuity.md b/docs/core-functionality/llm-agents/agent-context-id-continuity.md index 3d89d840..ae84c1ff 100644 --- a/docs/core-functionality/llm-agents/agent-context-id-continuity.md +++ b/docs/core-functionality/llm-agents/agent-context-id-continuity.md @@ -1,6 +1,6 @@ # Agent context_id — continuity between session messages -**Last validated:** Langflow 1.12.x (nightly `1.12.0.dev44`) +**Last validated:** Langflow 1.12.x (nightly `1.12.0.dev45`) --- @@ -72,8 +72,13 @@ surface; `@components` — Message History node drives the retrieval assert. unflushed batch leaves the model picker on the pre-toggle set — the `MODEL_PICKER_DEFECT` this spec reported five times on 2026-08-31 (#1649). The wait and its ~30 s refresh budget live in `tests/helpers/provider-setup/`; the - measurement is in the agent-area `CLAUDE.md` § 5. **This does not change what - this test validates** — it is a precondition of reaching the assertions at all. + measurement is in the agent-area `CLAUDE.md` § 5. When the enable write is + issued and never answers inside that wait's budget — the 2026-09-01 shape, and + what this spec hit again on run 33511210195 — the failure is + `MODEL_TOGGLE_WRITE_STALLED` instead: an INSTANCE stall naming the write, not a + picker defect, because the picker is then correctly showing the five models the + server actually has (agent-area `CLAUDE.md` § 5.1). **Neither changes what this + test validates** — both are preconditions of reaching the assertions at all. --- diff --git a/tests/helpers/provider-setup/model-option.test.ts b/tests/helpers/provider-setup/model-option.test.ts index 2d84f858..31d103ef 100644 --- a/tests/helpers/provider-setup/model-option.test.ts +++ b/tests/helpers/provider-setup/model-option.test.ts @@ -366,3 +366,104 @@ test("with only listedModels observed the message says so instead of claiming EN assert.match(verdict.message, /listed by the provider panel/); assert.ok(!/is ENABLED in the provider panel/.test(verdict.message)); }); + +// --- #1649 (reopened): a stalled write is not a picker defect --- +// +// The loud "is ENABLED in the provider panel" verdict reads `aria-checked`, which is +// `useModelToggleQueue`'s OPTIMISTIC cache: it flips at click time, before any +// request. So when the batched write never answers, the panel claims the model is on +// while the server still has only its `MIN_DEFAULT_MODELS` five — and the picker, +// rendering the server's truth, is the honest source. On the 2026-09-01 daily that +// state produced four `MODEL_PICKER_DEFECT` failures naming two hypotheses nobody +// had measured, while the gate had printed the actual cause 90 s earlier. +// +// The stall verdict is consulted INSIDE the checked-includes-requested branch, after +// `empty`/`match`/`unmatchable`: a picker that DID offer the model is never a stall, +// and a stall must not rewrite a verdict about one. + +const STALLED = { + clicked: 30, + verdict: "gave-up" as const, + writesStarted: 1, + writesFinished: 0, +}; + +test("a picker miss after a STALLED write blames the write, not the picker", () => { + const verdict = resolveModelOption("gemini-3.5-flash", [option("OpenAI", "gpt-4o-mini")], { + listedModels: ["gemini-3.5-flash", "gemini-3.1-flash-lite"], + checkedModels: ["gemini-3.5-flash"], + providerLabel: "Google Generative AI", + toggleWrite: STALLED, + }); + assert.equal(verdict.kind, "write-stalled"); + // Loud, and never a skip: the whole point of #1461's assertion survives. + assert.ok(!verdict.message.startsWith("MODEL_NOT_AVAILABLE")); + assert.match(verdict.message, /^MODEL_TOGGLE_WRITE_STALLED:/); + // It must NOT keep asserting the panel's claim as a fact about the server. + assert.ok(!/is ENABLED in the provider panel/.test(verdict.message)); + assert.match(verdict.message, /OPTIMISTIC/); + assert.match(verdict.message, /1 write\(s\) started, 0 finished/); + // The picker's own counts stay in the message: they are what shows the server had + // the five defaults, which is the reading that makes the picker correct. + assert.match(verdict.message, /1 option\(s\)/); + assert.match(verdict.message, /MIN_DEFAULT_MODELS/); + assert.match(verdict.message, /INSTANCE stall/); +}); + +test("a picker miss after a SETTLED write is still MODEL_PICKER_DEFECT", () => { + // The load-bearing negative. If the stall branch widened to every give-up-shaped + // context, the genuine picker/panel disagreement #1461 exists to catch would be + // relabelled as an environment problem and stop being investigated. + const verdict = resolveModelOption("gemini-3.5-flash", [option("OpenAI", "gpt-4o-mini")], { + listedModels: ["gemini-3.5-flash"], + checkedModels: ["gemini-3.5-flash"], + providerLabel: "Google Generative AI", + toggleWrite: { clicked: 30, verdict: "settled", writesStarted: 1, writesFinished: 1 }, + }); + assert.equal(verdict.kind, "unmatchable"); + assert.match(verdict.message, /^MODEL_PICKER_DEFECT:/); + assert.match(verdict.message, /is ENABLED in the provider panel/); +}); + +test("an unobserved batch leaves every existing verdict byte-identical", () => { + // Callers that never ran the gate (and every caller before this change) pass no + // `toggleWrite`. An unobserved source must not be read as a negative one (#1012), + // so the message they get is the one they got before. + const withoutContext = resolveModelOption( + "gemini-3.5-flash", + [option("OpenAI", "gpt-4o-mini")], + { + listedModels: ["gemini-3.5-flash"], + checkedModels: ["gemini-3.5-flash"], + providerLabel: "Google Generative AI", + }, + ); + assert.equal(withoutContext.kind, "unmatchable"); + assert.match(withoutContext.message, /^MODEL_PICKER_DEFECT:/); +}); + +test("a stall never rewrites a verdict about a picker that DOES offer the model", () => { + // `match` and `unmatchable`-by-identity are decided before the panel sources are + // consulted at all. A stalled write says nothing about a model the picker is + // offering, and reporting an instance stall there would hide the #1459 class of + // suite defect (identity no longer resolving). + const matched = resolveModelOption("gpt-4o-mini", [option("OpenAI", "gpt-4o-mini")], { + checkedModels: ["gpt-4o-mini"], + providerLabel: "OpenAI", + toggleWrite: STALLED, + }); + assert.equal(matched.kind, "match"); +}); + +test("a listed model with its toggle OFF stays a setup failure even under a stall", () => { + // The optimistic cache is what makes a stall look enabled; a toggle reading OFF is + // therefore NOT the stall's signature, and `MODEL_NOT_ENABLED` — which already + // names the debounce cause — remains the right verdict. + const verdict = resolveModelOption("gemini-3.5-flash", [option("OpenAI", "gpt-4o-mini")], { + listedModels: ["gemini-3.5-flash", "gemini-3.1-flash-lite"], + checkedModels: ["gemini-3.1-flash-lite"], + providerLabel: "Google Generative AI", + toggleWrite: STALLED, + }); + assert.equal(verdict.kind, "not-enabled"); +}); diff --git a/tests/helpers/provider-setup/model-option.ts b/tests/helpers/provider-setup/model-option.ts index 74921121..fcfc26a5 100644 --- a/tests/helpers/provider-setup/model-option.ts +++ b/tests/helpers/provider-setup/model-option.ts @@ -1,4 +1,10 @@ import type { Locator, Page } from "@playwright/test"; +import { + MODEL_TOGGLE_WRITE_STALLED, + modelTriggerStallMessage, + writeStallReason, + type ToggleBatchOutcome, +} from "./model-toggle-batch"; /** * One entry of the unified ModelInput picker, read straight from the DOM. @@ -52,6 +58,7 @@ export type ModelOptionVerdict = | { kind: "unmatchable"; message: string; evidence: string[] } | { kind: "empty"; message: string } | { kind: "not-enabled"; message: string } + | { kind: "write-stalled"; message: string } | { kind: "absent"; message: string }; export type ResolveContext = { @@ -80,6 +87,19 @@ export type ResolveContext = { checkedModels?: string[]; /** Provider the caller is configuring, for the message only. */ providerLabel?: string; + /** + * What the panel's own toggle batch did before the panel was closed — + * `enableAndSettleModelToggles`' return value. + * + * It is the THIRD source, and the only one that can tell a picker/panel + * disagreement with a known cause from one without: `checkedModels` reads + * `aria-checked`, which is `useModelToggleQueue`'s OPTIMISTIC cache and flips at + * click time before any request. So when the batched write never answers, the + * panel claims the model is on while the server still holds its + * `MIN_DEFAULT_MODELS` five, and the picker — rendering the server — is the + * honest one. `undefined` means "not observed" and changes no verdict (#1012). + */ + toggleWrite?: ToggleBatchOutcome; }; const MODEL_NOT_AVAILABLE = "MODEL_NOT_AVAILABLE"; @@ -223,6 +243,27 @@ export function resolveModelOption( const checked = context.checkedModels; if (checked?.includes(requested)) { + // The panel says ON — but `aria-checked` is the optimistic cache, so before + // that claim may be turned into a picker defect, ask whether the write behind + // it ever landed. Consulted HERE and not earlier on purpose: `empty`, `match` + // and `unmatchable`-by-identity are already decided above, and a stalled write + // says nothing about a model the picker IS offering — reporting a stall there + // would hide the #1459 class of defect (identity no longer resolving). + const stalled = writeStallReason(context.toggleWrite); + if (stalled !== null) { + return { + kind: "write-stalled", + message: + `${MODEL_TOGGLE_WRITE_STALLED}: "${requested}" reads as enabled in the provider ` + + `panel (llm-toggle-${requested})${provider}, but that is the OPTIMISTIC client ` + + `cache — ${stalled}. The model was therefore never enabled server-side, and the ` + + `picker is CORRECT to offer ${options.length} option(s) ` + + `(${providerCounts(options)}): a freshly configured provider's ` + + `${"`"}MIN_DEFAULT_MODELS${"`"} default. This is an INSTANCE stall — not a picker ` + + `defect and not a missing model. Do not raise the flush budget to make it pass ` + + `(#1649).`, + }; + } return { kind: "unmatchable", evidence: [`llm-toggle-${requested} in the provider panel`], @@ -448,6 +489,49 @@ export async function clickModelOption(page: Page, option: ModelOption): Promise await locator.first().click(); } +/** + * Opens the model picker after the provider panel was closed, attributing a + * failure to the toggle batch when the batch is what explains it. + * + * Shared by the three provider setups because the block was copy-pasted three + * times and had already drifted: #1651 landed the same 60 s budgets in each with + * three differently-worded comments, and this is the second change to touch all + * three. Both budgets stay 60 s and are NOT a retry: taking the correct flush path + * means the product genuinely re-fetches, measured at 30 020 ms and 29 640 ms + * against the 4 327 ms the broken path returned in. The click carries its own + * budget because it otherwise falls back to the 20 s `actionTimeout` while the + * trigger re-enters `ModelInputLoadingButton` between "visible" and the click. + * + * What is new is the catch. On a batch that never settled, the post-close refresh + * runs in a write's `onSettled` that never fired, so the trigger can stay + * unusable for the whole budget — measured twice on the 2026-09-01 daily as a bare + * `locator.waitFor: Timeout 60000ms exceeded ... getByTestId('model_model')` with + * nothing naming a cause. The batch's own observation is re-thrown instead, and + * Playwright's original message is kept inside it. + */ +export async function openModelPickerAfterPanelClose( + page: Page, + context: { providerLabel: string; toggleWrite?: ToggleBatchOutcome }, +): Promise { + const trigger = page.getByTestId("model_model"); + try { + await trigger.waitFor({ state: "visible", timeout: 60000 }); + // The locator is re-resolved on every actionability retry, so this survives the + // element being replaced, and nothing about the assertion that follows is + // weakened. + await trigger.click({ timeout: 60000 }); + } catch (error) { + const attributed = modelTriggerStallMessage(context.toggleWrite, { + providerLabel: context.providerLabel, + original: (error as Error).message, + }); + if (attributed !== null) throw new Error(attributed); + // No stall to blame: a trigger that never returns on a healthy flush is a real + // defect and must keep surfacing as Playwright's own error, call log included. + throw error; + } +} + export type PinnedSelection = | { status: "selected"; model: string } | { status: "absent"; message: string }; @@ -468,6 +552,13 @@ export async function selectPinnedModelOption( listedModels?: string[]; checkedModels?: string[]; providerLabel?: string; + /** + * `enableAndSettleModelToggles`' result. `write-stalled` is deliberately NOT + * returnable through `absentBehavior: "return"`: that hatch exists for a stale + * pin from `models.json` (#606), and degrading on an instance that could not + * accept the write would hide exactly the state #1649 was reopened for. + */ + toggleWrite?: ToggleBatchOutcome; absentBehavior?: "throw" | "return"; timeout?: number; }, @@ -477,6 +568,7 @@ export async function selectPinnedModelOption( listedModels: opts.listedModels, checkedModels: opts.checkedModels, providerLabel: opts.providerLabel, + toggleWrite: opts.toggleWrite, }); if (verdict.kind === "match") { diff --git a/tests/helpers/provider-setup/model-toggle-batch.test.ts b/tests/helpers/provider-setup/model-toggle-batch.test.ts index 4c0b0956..c2b133b8 100644 --- a/tests/helpers/provider-setup/model-toggle-batch.test.ts +++ b/tests/helpers/provider-setup/model-toggle-batch.test.ts @@ -17,7 +17,12 @@ // reason `resolveModelOption` and `censusForTarget` are. import { test } from "node:test"; import assert from "node:assert/strict"; -import { flushVerdict, type ToggleBatchObservation } from "./model-toggle-batch"; +import { + flushVerdict, + modelTriggerStallMessage, + writeStallReason, + type ToggleBatchObservation, +} from "./model-toggle-batch"; const OPTS = { quietMs: 1500, deadlineAt: 100_000 }; @@ -106,3 +111,97 @@ test("the deadline never overrides nothing-to-flush", () => { ); assert.equal(v.kind, "nothing-to-flush"); }); + +// --- #1649 (reopened): a give-up is an OBSERVED cause, and it must be carried --- +// +// The gate above already prints what it saw. What it did NOT do was hand that +// observation to the picker read that follows, so 90 s later the failure named a +// cause nobody had measured ("the picker did not refresh, or the option list is +// filtered") while the real one — the write never answered — sat in a log line no +// failure message, no `error_signature` and no triage dataset correlates. All eight +// give-ups on the 2026-09-01 daily read `1 write(s) started, 0 finished`. +// +// `writeStallReason` is the carrier, and it is pure for the same reason +// `flushVerdict` is. Three properties ride on it: an UNOBSERVED batch is not a +// negative one (#1012), a SETTLED batch must leave the existing verdict alone, and +// an unchanged panel is never a stall. + +test("a gave-up batch yields a reason naming the write that never answered", () => { + const reason = writeStallReason({ + clicked: 30, + verdict: "gave-up", + writesStarted: 1, + writesFinished: 0, + }); + assert.ok(reason !== null); + assert.match(reason!, /30 toggle\(s\) clicked/); + assert.match(reason!, /1 write\(s\) started/); + assert.match(reason!, /0 finished/); + // The endpoint is named, because "the write" is not actionable on its own. + assert.match(reason!, /enabled_models/); +}); + +test("an UNOBSERVED batch is not a stalled one", () => { + // The three provider helpers pass what they measured; anything else (a caller + // that never ran the gate) must produce no claim at all rather than a negative. + assert.equal(writeStallReason(undefined), null); +}); + +test("a settled batch is never a stall, whatever the counts say", () => { + // This is the branch that keeps MODEL_PICKER_DEFECT alive: a picker that + // disagrees AFTER a clean flush is the genuine, unexplained disagreement #1461 + // wrote its assertion for, and re-labelling it as an instance stall would blind + // the suite to it. + assert.equal( + writeStallReason({ clicked: 36, verdict: "settled", writesStarted: 1, writesFinished: 1 }), + null, + ); + assert.equal( + writeStallReason({ clicked: 0, verdict: "nothing-to-flush", writesStarted: 0, writesFinished: 0 }), + null, + ); +}); + +test("a panel nobody changed is never a stall, even past the deadline", () => { + // `flushVerdict` cannot return gave-up with clicked === 0 today, but the guard is + // cheap and the alternative is a scary instance-stall verdict on a healthy run + // the moment that ordering changes. + assert.equal( + writeStallReason({ clicked: 0, verdict: "gave-up", writesStarted: 0, writesFinished: 0 }), + null, + ); +}); + +test("the model_model message blames the instance, keeps the original error, and cannot skip", () => { + const message = modelTriggerStallMessage( + { clicked: 30, verdict: "gave-up", writesStarted: 1, writesFinished: 0 }, + { + providerLabel: "Google Generative AI", + original: "locator.waitFor: Timeout 60000ms exceeded.", + }, + ); + assert.ok(message !== null); + // Two of #1649's six occurrences were this timeout, 60 s each, with nothing in + // the message naming a cause. The prefix must NOT be the skip prefix. + assert.ok(!message!.startsWith("MODEL_NOT_AVAILABLE")); + assert.match(message!, /^MODEL_TOGGLE_WRITE_STALLED:/); + assert.match(message!, /Google Generative AI/); + assert.match(message!, /1 write\(s\) started, 0 finished/); + assert.match(message!, /locator\.waitFor: Timeout 60000ms exceeded\./); + // The refresh runs in the batch's own onSettled — saying so is what separates + // this from a trigger/testid defect. + assert.match(message!, /onSettled/); + assert.match(message!, /#1649/); +}); + +test("with no stall the model_model failure is left exactly as it was", () => { + // A trigger that never appears on a HEALTHY flush is a real defect and must keep + // surfacing as Playwright's own locator error, not be re-labelled. + assert.equal( + modelTriggerStallMessage( + { clicked: 36, verdict: "settled", writesStarted: 1, writesFinished: 1 }, + { providerLabel: "OpenAI", original: "locator.click: Timeout 60000ms exceeded." }, + ), + null, + ); +}); diff --git a/tests/helpers/provider-setup/model-toggle-batch.ts b/tests/helpers/provider-setup/model-toggle-batch.ts index 5026059a..5ab408d1 100644 --- a/tests/helpers/provider-setup/model-toggle-batch.ts +++ b/tests/helpers/provider-setup/model-toggle-batch.ts @@ -102,8 +102,9 @@ export function flushVerdict( `${clicked} toggle(s) clicked, ${postsStarted} write(s) started, ` + `${postsFinished} finished. Closing the panel now takes the flush path that ` + `does NOT refresh the model picker, so the picker may still show the ` + - `pre-toggle set. Not failing here: the picker read that follows is the real ` + - `gate and names the disagreement itself (#1649).`, + `pre-toggle set. Not failing here: the read that follows is the real gate, and ` + + `it attributes the failure to THIS write — MODEL_TOGGLE_WRITE_STALLED — rather ` + + `than to the picker (#1649).`, }); if (lastClickAt !== null && now - lastClickAt < options.quietMs) { @@ -141,8 +142,91 @@ export type ToggleBatchResult = { checked: number; /** How the flush ended — `gave-up` is logged, never thrown. */ verdict: FlushVerdict["kind"]; + /** `POST /models/enabled_models` requests seen starting during the pass. */ + writesStarted: number; + /** …and answering. `started > finished` on a give-up is THE stall signature. */ + writesFinished: number; }; +/** + * The subset a caller must carry forward so a later failure can name this batch. + * + * Deliberately narrow: `visible`/`checked` describe the panel, and the panel is + * the source that LIES under a stall (`aria-checked` is the optimistic cache). + */ +export type ToggleBatchOutcome = Pick< + ToggleBatchResult, + "clicked" | "verdict" | "writesStarted" | "writesFinished" +>; + +/** + * Prefixed `MODEL_` like its four siblings in `model-option.ts`, and deliberately + * NOT `MODEL_NOT_AVAILABLE`: every caller turns that prefix into a `test.skip`, + * and an instance that cannot accept a write must never be reported as a model + * the product does not have. + */ +export const MODEL_TOGGLE_WRITE_STALLED = "MODEL_TOGGLE_WRITE_STALLED"; + +/** + * Why a later failure is this batch's fault rather than the picker's — or `null` + * when this batch cannot explain anything. + * + * PURE, for the same reason `flushVerdict` is. It exists because #1651's gate + * already OBSERVED the cause and printed it, and then dropped it: 90 s later the + * picker read failed naming two hypotheses nobody had measured ("the picker did + * not refresh, or the option list is filtered"), while the measured cause — the + * write was issued and never answered — sat in a log line no failure message, no + * `error_signature` and no triage dataset correlates. That is why #1649 was + * verdicted twice and reopened. + * + * Three properties are load-bearing, each pinned by a unit test: + * + * - an UNOBSERVED batch (`undefined`) yields `null` — a source nobody read must + * never be reported as a negative one (#1012); + * - a SETTLED batch yields `null`, which is what keeps `MODEL_PICKER_DEFECT` + * alive for the genuine, unexplained disagreement #1461 wrote it for; + * - a panel nobody changed yields `null`, so a healthy run can never print an + * instance-stall verdict. + */ +export function writeStallReason(batch?: ToggleBatchOutcome): string | null { + if (!batch) return null; + if (batch.verdict !== "gave-up") return null; + if (batch.clicked === 0) return null; + return ( + `the enable write never answered — ${batch.clicked} toggle(s) clicked, ` + + `${batch.writesStarted} write(s) started, ${batch.writesFinished} finished before the ` + + `flush budget expired, so POST /api/v1/models/enabled_models did not land` + ); +} + +/** + * The message for a `model_model` trigger that never became usable after the + * panel closed on a stalled batch — or `null` when the batch cannot explain it. + * + * Two of #1649's six occurrences were exactly this, 60 s each + * (`locator.waitFor: Timeout 60000ms exceeded ... getByTestId('model_model')`), + * with nothing in the message naming a cause. The post-close refresh runs in the + * batch's own `onSettled`, which never fired — so a trigger that never returns is + * the same instance stall, not a trigger or testid defect. `original` is kept + * verbatim: a re-labelled failure that discards Playwright's own call log is + * harder to triage, not easier. + */ +export function modelTriggerStallMessage( + batch: ToggleBatchOutcome | undefined, + context: { providerLabel: string; original: string }, +): string | null { + const reason = writeStallReason(batch); + if (reason === null) return null; + return ( + `${MODEL_TOGGLE_WRITE_STALLED}: the model picker's trigger never became usable after ` + + `the provider panel closed for ${context.providerLabel} — ${reason}. The panel's ` + + `post-close refresh runs in that write's own onSettled, which never fired, so this is ` + + `an INSTANCE stall — not a picker defect, not a missing testid and not a model that is ` + + `gone. Do not raise this budget to make it pass (#1649). Original error: ` + + `${context.original}` + ); +} + /** * Enables every visible model toggle in the OPEN provider panel, then waits for * the product's own write to settle so the caller can close the panel safely. @@ -219,7 +303,16 @@ export async function enableAndSettleModelToggles( const checked = await page .locator('[data-testid^="llm-toggle"]:visible[aria-checked="true"]') .count(); - return { visible, clicked: observation.clicked, checked, verdict: verdict.kind }; + return { + visible, + clicked: observation.clicked, + checked, + verdict: verdict.kind, + // Returned, not merely printed: the give-up message already carried these + // and the caller could not read them, which is the whole of #1649's reopen. + writesStarted: observation.postsStarted, + writesFinished: observation.postsFinished, + }; } finally { page.off("request", onRequest); page.off("requestfinished", onFinished); diff --git a/tests/helpers/provider-setup/setup-anthropic.ts b/tests/helpers/provider-setup/setup-anthropic.ts index 7c707e28..90036f8a 100644 --- a/tests/helpers/provider-setup/setup-anthropic.ts +++ b/tests/helpers/provider-setup/setup-anthropic.ts @@ -5,6 +5,7 @@ import { enumerateCheckedModels, enumerateEnabledModels, enumerateModelOptions, + openModelPickerAfterPanelClose, selectPinnedModelOption, } from "./model-option"; import { enableAndSettleModelToggles } from "./model-toggle-batch"; @@ -58,7 +59,10 @@ export async function setupAnthropic( // refreshes the model picker (#1649). The helper clicks and then waits for the // product's own write to go quiet, so Step 6 below cannot close on top of it. // Costs nothing when nothing was clicked, which is the normal CI path. - await enableAndSettleModelToggles(page); + // The result is CAPTURED, not discarded: when this batch does not settle it is + // the only source that can explain the picker read below, and #1651 printed it + // to a log nothing correlates instead of carrying it forward (#1649). + const toggleWrite = await enableAndSettleModelToggles(page); // Read the panel's toggles BEFORE closing it: they are the second, independent // source the picker can be contradicted by, and a picker miss that they @@ -73,20 +77,17 @@ export async function setupAnthropic( // Step 7: Select model — uses modelTestId if provided, otherwise selects the first available await hideInspectorPanel(page); - // Closing the panel (Step 6) puts the model dropdown into a post-close refresh - // state where the `model_model` trigger is briefly replaced by a (testid-less) - // "Loading models…" button while providers and enabled models refetch. Both - // budgets below are 60 s, and NOT to make a stall pass: taking the correct - // flush path in Step 5 means the product genuinely re-fetches, measured at - // 30 020 ms and 29 640 ms against the 4 327 ms the broken path returned in. - // The click carries its own budget because it otherwise falls back to the 20 s - // `actionTimeout` and the trigger can re-enter the loading state between - // "visible" and the click — measured failing exactly there - // (`locator.click: Timeout 20000ms exceeded ... getByTestId('model_model')`) - // on the cold path with the provider on its MIN_DEFAULT_MODELS default (#1649). - const modelTrigger = page.getByTestId("model_model"); - await modelTrigger.waitFor({ state: "visible", timeout: 60000 }); - await modelTrigger.click({ timeout: 60000 }); + // Opening the picker is shared with the other two provider setups: closing the + // panel (Step 6) puts the dropdown into a post-close refresh state where + // `model_model` is briefly replaced by a (testid-less) "Loading models…" button — + // and when the batch above never settled, that refresh never runs at all, so the + // trigger can stay unusable for the whole budget. Both 60 s budgets, their + // measurements, and the attribution of that case live in + // `openModelPickerAfterPanelClose` (#1649). + await openModelPickerAfterPanelClose(page, { + providerLabel: "Anthropic", + toggleWrite, + }); if (modelTestId) { // Resolved by option IDENTITY (data-value / data-testid), never by the option's // text: 1.12.0.dev26 renders a `sr-only` "N of M" counter inside each option, so @@ -97,6 +98,10 @@ export async function setupAnthropic( listedModels, checkedModels, providerLabel: "Anthropic", + // The third source: `checkedModels` reads the OPTIMISTIC cache, so a picker + // miss it contradicts is only a picker defect when the write behind it landed + // (#1649). + toggleWrite, }); } else { const options = await enumerateModelOptions(page); diff --git a/tests/helpers/provider-setup/setup-google.ts b/tests/helpers/provider-setup/setup-google.ts index 69805a9b..2cb3cf04 100644 --- a/tests/helpers/provider-setup/setup-google.ts +++ b/tests/helpers/provider-setup/setup-google.ts @@ -5,6 +5,7 @@ import { enumerateCheckedModels, enumerateEnabledModels, enumerateModelOptions, + openModelPickerAfterPanelClose, selectPinnedModelOption, } from "./model-option"; import { enableAndSettleModelToggles } from "./model-toggle-batch"; @@ -99,7 +100,10 @@ export async function setupGoogle( // refreshes the model picker (#1649). The helper clicks and then waits for the // product's own write to go quiet, so Step 6 below cannot close on top of it. // Costs nothing when nothing was clicked, which is the normal CI path. - await enableAndSettleModelToggles(page); + // The result is CAPTURED, not discarded: when this batch does not settle it is + // the only source that can explain the picker read below, and #1651 printed it + // to a log nothing correlates instead of carrying it forward (#1649). + const toggleWrite = await enableAndSettleModelToggles(page); // Read the panel's toggles BEFORE closing it: they are the second, independent // source the picker can be contradicted by, and a picker miss that they @@ -114,20 +118,17 @@ export async function setupGoogle( // Step 7: Select model — uses modelTestId if provided, otherwise selects the first available await hideInspectorPanel(page); - // Closing the panel (Step 6) puts the model dropdown into a post-close refresh - // state where the `model_model` trigger is briefly replaced by a (testid-less) - // "Loading models…" button while providers and enabled models refetch. Both - // budgets below are 60 s, and NOT to make a stall pass: taking the correct - // flush path in Step 5 means the product genuinely re-fetches, measured at - // 30 020 ms and 29 640 ms against the 4 327 ms the broken path returned in. - // The click carries its own budget because it otherwise falls back to the 20 s - // `actionTimeout` and the trigger can re-enter the loading state between - // "visible" and the click — measured failing exactly there - // (`locator.click: Timeout 20000ms exceeded ... getByTestId('model_model')`) - // on the cold path with the provider on its MIN_DEFAULT_MODELS default (#1649). - const modelTrigger = page.getByTestId("model_model"); - await modelTrigger.waitFor({ state: "visible", timeout: 60000 }); - await modelTrigger.click({ timeout: 60000 }); + // Opening the picker is shared with the other two provider setups: closing the + // panel (Step 6) puts the dropdown into a post-close refresh state where + // `model_model` is briefly replaced by a (testid-less) "Loading models…" button — + // and when the batch above never settled, that refresh never runs at all, so the + // trigger can stay unusable for the whole budget. Both 60 s budgets, their + // measurements, and the attribution of that case live in + // `openModelPickerAfterPanelClose` (#1649). + await openModelPickerAfterPanelClose(page, { + providerLabel: "Google Generative AI", + toggleWrite, + }); if (modelTestId) { // Resolved by option IDENTITY (data-value / data-testid), never by the option's // text: 1.12.0.dev26 renders a `sr-only` "N of M" counter inside each option, so @@ -139,6 +140,10 @@ export async function setupGoogle( listedModels, checkedModels, providerLabel: "Google Generative AI", + // The third source: `checkedModels` reads the OPTIMISTIC cache, so a picker + // miss it contradicts is only a picker defect when the write behind it landed + // (#1649). + toggleWrite, }); } else { const options = await enumerateModelOptions(page); diff --git a/tests/helpers/provider-setup/setup-openai.ts b/tests/helpers/provider-setup/setup-openai.ts index d53a4749..bb8ceec7 100644 --- a/tests/helpers/provider-setup/setup-openai.ts +++ b/tests/helpers/provider-setup/setup-openai.ts @@ -5,6 +5,7 @@ import { enumerateCheckedModels, enumerateEnabledModels, enumerateModelOptions, + openModelPickerAfterPanelClose, selectPinnedModelOption, } from "./model-option"; import { enableAndSettleModelToggles } from "./model-toggle-batch"; @@ -73,7 +74,10 @@ export async function setupOpenAI( // refreshes the model picker (#1649). The helper clicks and then waits for the // product's own write to go quiet, so Step 6 below cannot close on top of it. // Costs nothing when nothing was clicked, which is the normal CI path. - await enableAndSettleModelToggles(page); + // The result is CAPTURED, not discarded: when this batch does not settle it is + // the only source that can explain the picker read below, and #1651 printed it + // to a log nothing correlates instead of carrying it forward (#1649). + const toggleWrite = await enableAndSettleModelToggles(page); // Read the panel's toggles BEFORE closing it: they are the second, independent // source the picker can be contradicted by, and a picker miss that they @@ -87,29 +91,16 @@ export async function setupOpenAI( await page.getByRole("button", { name: "Close" }).click(); // Step 7: Select model — uses modelTestId if provided, otherwise selects the first available. - // Closing the management panel (Step 6) puts the model dropdown into a - // post-close refresh state where the `model_model` trigger is briefly - // replaced by a (testid-less) "Loading models…" button while providers and - // enabled models refetch. Wait for the trigger to be VISIBLE — not merely - // attached — so the click does not race that loading swap. + // Opening the picker is shared with the other two provider setups: closing the + // management panel (Step 6) puts the dropdown into a post-close refresh state + // where `model_model` is briefly replaced by a (testid-less) "Loading models…" + // button — and when the batch above never settled, that refresh never runs at + // all, so the trigger can stay unusable for the whole budget. Both 60 s budgets, + // their measurements, and the attribution of that case live in + // `openModelPickerAfterPanelClose` (#1649). await hideInspectorPanel(page); const modelTrigger = page.getByTestId("model_model"); - // 60 s, not the 15 s this used to allow, and NOT to make a stall pass: taking the - // correct flush path above means the product genuinely re-fetches, measured at - // 30 020 ms and 29 640 ms against the 4 327 ms the broken path returned in - // (#1649). The old budget would turn the fix into a model_model timeout. - await modelTrigger.waitFor({ state: "visible", timeout: 60000 }); - // The click carries its own 60 s budget for the same measured reason as the - // waitFor above, and NOT as a retry bolted on to make a red pass: `click()` - // otherwise falls back to the 20 s `actionTimeout`, and the trigger re-enters the - // loading state between "visible" and the click while the refresh this helper - // correctly triggered is still running. Measured failing exactly there — - // `locator.click: Timeout 20000ms exceeded ... waiting for - // getByTestId('model_model')` — on the cold path with the provider on its - // MIN_DEFAULT_MODELS default (#1649). The locator is re-resolved on every - // actionability retry, so this survives the element being replaced, and nothing - // about the assertion that follows is weakened. - await modelTrigger.click({ timeout: 60000 }); + await openModelPickerAfterPanelClose(page, { providerLabel: "OpenAI", toggleWrite }); let pickByRanking = !modelTestId; if (modelTestId) { // Resolved by option IDENTITY (data-value / data-testid), never by the option's @@ -125,6 +116,10 @@ export async function setupOpenAI( listedModels, checkedModels, providerLabel: "OpenAI", + // The third source: `checkedModels` reads the OPTIMISTIC cache, so a picker + // miss it contradicts is only a picker defect when the write behind it landed + // (#1649). `write-stalled` throws through `absentBehavior` on purpose. + toggleWrite, absentBehavior: opts?.fallbackToRanking ? "return" : "throw", }); if (selection.status === "absent") { diff --git a/tests/tests-automations/regression/core-functionality/llm-agents/CLAUDE.md b/tests/tests-automations/regression/core-functionality/llm-agents/CLAUDE.md index a993bea5..66007908 100644 --- a/tests/tests-automations/regression/core-functionality/llm-agents/CLAUDE.md +++ b/tests/tests-automations/regression/core-functionality/llm-agents/CLAUDE.md @@ -106,7 +106,9 @@ So closing the panel **within** the debounce window takes the path that never refreshes the picker, and the picker then renders the **pre-toggle** enabled set — which on a freshly configured provider is the `MIN_DEFAULT_MODELS = 5` default (`lfx/base/models/model_utils.py`). That is a genuine picker/panel disagreement, -so `MODEL_PICKER_DEFECT` fires, correctly (§4) — and the cause is ours. +so `MODEL_PICKER_DEFECT` fires, correctly (§4) — and the cause is ours. That +verdict holds when the batch **settled**; when it never did, the disagreement has +a different and already-observed cause, and § 5.1 below is the one that applies. Measured on `1.12.0.dev44`, one clean container, three runs of the identical sequence differing **only** in the pause between the last toggle click and Close, @@ -136,6 +138,54 @@ times out while the write has in fact landed — and polling `GET enabled_models This is handled for you inside `tests/helpers/provider-setup/` — do not re-implement the toggle loop in a spec. +#### 5.1 …and the write it waits for can fail to answer at all + +Waiting for the batch is necessary but not sufficient, because the write the wait +is watching can simply never come back. On the 2026-09-01 daily +([33511210195](https://github.com/oriontech-me/langflow-e2e/actions/runs/33511210195)) +the gate gave up **eight** times across three shards, every one reading the same +shape: + +``` +⚠️ provider panel: the model-toggle batch did not settle in time — + 30 toggle(s) clicked, 1 write(s) started, 0 finished. +``` + +`POST /api/v1/models/enabled_models` was issued and had not answered 90 s later. +The listener counts `requestfailed` too, so that is a hang, not a network error. +The gate then closes the panel anyway — by design: it warns, it does not throw — +so the close-path flush runs and the picker keeps the pre-toggle set. + +**The picker is RIGHT there, and the panel is what lies.** `aria-checked` is +`useModelToggleQueue`'s optimistic cache: it flips at click time, before any +request, and it is the source the loud message calls ENABLED. Corroborated on the +same run by `collect-models`' own server-side read — `0 of 30/36/8` enable writes +confirmed, on all four shards — and by `5` being `MIN_DEFAULT_MODELS` +(`lfx/base/models/model_utils.py`), i.e. the set the server genuinely had. + +So the outcome is split by **what the batch did**, not by what the picker shows: + +| Batch verdict | Picker omits the pinned model ⇒ | +|---|---| +| `settled` / `nothing-to-flush` | `MODEL_PICKER_DEFECT` — a real, unexplained disagreement | +| `gave-up` | `MODEL_TOGGLE_WRITE_STALLED` — the write never answered; an INSTANCE stall | + +and the `model_model` wait that follows a give-up raises the same named stall +instead of an anonymous `locator.waitFor` timeout (two of #1649's six occurrences +were exactly that timeout, 60 s each, with nothing in the message naming a cause). + +Reproduced causally rather than by re-running (re-running cannot falsify a +timing defect): on a healthy `1.12.0.dev45` container a scout held that POST for +120 s with `page.route`, everything else unchanged, and produced the daily's +evidence byte for byte — `36 toggle(s) clicked, 1 write(s) started, 0 finished` +followed by `MODEL_PICKER_DEFECT … 5 option(s) enumerated (OpenAI: 5)`. + +Neither budget was raised and nothing became a skip: on such a day the specs +still fail — they name the instance instead of accusing the picker. The +**run-level** cause that puts a spec on this path at all (`collect-models` +confirming 0 of 74 enable writes, so every provider sits on its five defaults) +is #1666, not this. + ### 6. Run with --workers=1 ```bash