fix(model-provider): guard the Ollama node's config before the run (#1302)

docs/core-functionality/model-provider/ollama-provider.md

@@ -68,7 +68,8 @@ a selected local model no longer executes.
 ## Tags *(required)*
 
 Test 1: `@stable` `@model-provider` `@settings`
-Test 2: `@stable` `@regression` `@model-provider` `@components` `@playground`
+Test 2: `@regression` `@model-provider` `@components` `@playground`
+— **`@stable` withheld again, see the #1302 gate below.**
 
 `@stable` added after 4 clean `--retries=0` runs against the local Ollama
 (issue #498's "Done when"). In environments without a local Ollama, both
@@ -98,7 +99,19 @@ the shard, is simply far slower than the ~13 s this spec takes locally. So a
 local green says nothing about the daily, and `@stable` restored on local
 evidence alone would predictably redden it again for an unrelated reason.
 
-**Restoration gate — SATISFIED.** The bar was a green sequence in the real CI
+**Restoration gate for #1302 (the CURRENT one — the block below it is the
+satisfied #931 gate, kept for the record).** `@stable` was removed again and
+`test.fixme` added at triage on 2026-08-06 (#1296 → #1302). It is restored only
+on evidence from the **real CI environment**, because the failure mode is a
+flow-state race that a dev box cannot reproduce at all — worse than in #931's
+case, since this spec now cannot even RUN locally on an arm64 Mac (see
+*Preconditions → local reproduction*). The bar: a `manual.yml` dispatch on the
+branch, `-f test_grep="Ollama"`, `-f retries=0`, green across several
+consecutive runs, with the guard in place. A local green is not admissible
+evidence here and neither is a single CI green — the mechanism fired on 2 of 26
+dailies, so one run proves nothing about it.
+
+**Restoration gate — SATISFIED (#931, historical).** The bar was a green sequence in the real CI
 environment, not on a dev box, via `manual.yml` dispatched on the branch (it
 carries the same `ollama` service container and SSRF allowlist):
 
@@ -122,11 +135,15 @@ dropped: `daily-stable.yml` runs `--grep @stable`, and `nightly.yml` (the only
 full-suite workflow) is disabled — so while untagged, this test ran in **no**
 recurring workflow at all.
 
-**Residual known flake:** on a dev box the execute test still failed ~1 in 8
-runs with no playground reply in 180 s (measured while authoring this). It did
-not reproduce across the 4 CI runs. Watch it in the daily; if it returns, the
-root cause to chase is whether the run starts at all (`POST /api/v2/workflows`,
-SSE) versus the Ollama node failing to build.
+**Residual known flake — it returned, and the fork this note named was the
+right one (#1302).** The prediction stood: *"the root cause to chase is whether
+the run starts at all versus the Ollama node failing to build"*. **The run never
+starts.** It recurred on the 2026-07-30 and 2026-08-05 dailies with the same
+signature, the spec was quarantined at triage (#1296), and the artifacts settle
+it — see the *Why the run never starts* section below. What is NOT the cause is
+the 180 s budget, which the issue's preliminary read proposed raising: the
+budget is untouched here on purpose, and the measurements that justify leaving
+it are recorded in that section.
 
 **Model resolution — the image is the source of truth (#931).** The model the
 CI exercises is BAKED into a dedicated image by
@@ -154,6 +171,60 @@ wait for the reply — which is exactly how a slow-starting CI run produces the
 hidden AND `button-send` visible**, with a window sized for CPU inference, so
 a slow run is waited out instead of being mistaken for a finished one.
 
+**Why the run never starts, and what guards it (#1302).** The failing attempt
+waits 180 s for `div-chat-message` and sees 0 elements 183 times. Three
+independent readings of the artifacts show that is not slowness:
+
+| Evidence | Measurement |
+|---|---|
+| The retry, same run, same runner (07-30 / 08-05) | attempt 0 **180 445 / 180 482 ms failed**, attempt 1 **5 644 / 5 559 ms passed** |
+| Green dailies, attempt 0 on a **freshly created** (therefore cold) Ollama container (08-04 / 08-03) | **5 408 / 6 503 ms passed** — there is no cold-start penalty |
+| `div-chat-message` in the dev18 bundle | wraps `chat-message-${sender_name}-${index}` — it counts the **user's** bubble too, so 0 means the typed message never rendered |
+
+The failure DOM says why: the Ollama node on the canvas has reverted to its
+defaults — `Model Name` reads *"Select an option"* and `Ollama API URL` reads
+`http://localhost:11434`, while the daily injects `http://ollama:11434`. The
+model **was** selected and asserted one step earlier (that step passed in
+1 059 ms). `Model Name` is required, so the run cannot start; consistently, the
+token artifact holds one flow trace for two attempts and the failing attempt
+logged zero backend errors in 191 s.
+
+The mechanism is the one `helpers/flows/wait-for-flow-save-settled.ts`
+documents: `PATCH /api/v1/flows/{id}` has no version check and the frontend
+applies whichever response lands LAST, so a stale autosave overwrites the store
+and the database (the root of #358, #357, #995). The spec already calls that
+barrier; it guarantees PATCH quiescence for 700 ms and nothing about what
+persisted. **Which write reverts it is not pinned** — a stale autosave and the
+bulk `DELETE /api/v1/flows/` that appears mid-test under `actualWorkers: 2` are
+both candidates, and the artifacts do not separate them.
+
+**The guard reads the WIDGET, not the API, and that is measured rather than
+conventional:** the run is dispatched as `POST /api/v2/workflows` with a
+**66 801-byte body** — the frontend's in-memory graph, not a reference to the
+persisted flow. A guard that queried `GET /api/v1/flows/{id}` could therefore
+pass while the run executes the reverted state.
+
+It does two things, in this order:
+
+1. **Converge** — after selecting the model, wait for the node's configuration
+   to hold (widget value stable, no flow-save PATCH in flight), re-applying the
+   selection at most once. This is condition-based waiting on a known product
+   race, not a blind retry of a failed interaction.
+2. **Attribute** — immediately before `button-send`, assert the node still
+   carries the model. If it does not, fail **there**, naming the revert and the
+   two fields observed, in ~1 s instead of 180 s.
+
+Step 2 does not mask the defect: a persistent revert still fails the test, just
+quickly and with the cause named instead of as a bare `toHaveCount` timeout on a
+locator three layers downstream.
+
+**The 180 s budget is deliberately unchanged.** #1302's directive asks for a
+measured replacement *if the budget is the cause*; it is not. Recorded so the
+question is not reopened: the playground step costs **5 408 / 5 559 / 5 644 /
+6 503 ms** across four dailies, cold and warm, and the run request itself
+(`POST /api/v2/workflows`) took **4 063 ms**. Any budget above ~10 s is
+equivalent for the healthy path, and for the broken path no budget works.
+
 ---
 
 ## Preconditions *(optional)*
@@ -177,6 +248,22 @@ a slow run is waited out instead of being mistaken for a finished one.
   model-list fetch — start the Langflow container with
   `-e LANGFLOW_SSRF_ALLOWED_HOSTS=host.docker.internal` (discovered live on
   1.11.0.dev36 while authoring this spec).
+- **Local reproduction is NOT possible on an arm64 Mac (measured 2026-08-06 on
+  1.12.0.dev18)** — treat this spec as CI-only there and do not spend the cycle.
+  A dockerized Langflow could not reach any Ollama in either topology (host
+  instance via `host.docker.internal`; a sibling `ollama/ollama` container on a
+  shared network with the CI's exact allowlist). Setting
+  `LANGFLOW_SSRF_ALLOWED_HOSTS` makes it worse rather than better: **without**
+  it the layer answers `resolves to blocked IP address(es)` (so the name
+  resolved), **with** it the same name answers `DNS resolution failed` — for a
+  name `getent` and `socket.getaddrinfo` resolve inside that same container.
+  Independently: `validate_model_provider_key("Ollama", …)` called directly in
+  that container validates and connects, while `POST
+  /api/v1/models/validate-provider` with the same argument does not — the
+  allowlist is honoured by the library and not by the endpoint. The escapes are
+  closed too: the amd64 image dies with `Fatal glibc error: CPU does not support
+  x86-64-v3`, and `start-langflow-pip.sh` installs the stable release, not the
+  nightly line.
 - If the probe fails, both tests skip with the reason — no false red.
 - No collect-models / cloud key needed (local provider).
 - **How CI satisfies all of the above** (`daily-stable.yml`): an `ollama`
@@ -215,14 +302,20 @@ a slow run is waited out instead of being mistaken for a finished one.
    refresh/open the `model_name` dropdown.
 4. **Assert (configure/connectivity):** the dropdown lists
    `OLLAMA_TEST_MODEL` — the component genuinely enumerated the local
-   instance's models. Select it.
-5. Open the Playground, send a per-run sentinel prompt, and wait for the run
+   instance's models. Select it, then **wait for the selection to converge**
+   (#1302): the widget still shows it with no flow-save PATCH in flight,
+   re-applying at most once.
+5. Open the Playground. **Immediately before sending, assert the Ollama node
+   still carries the model** (#1302) — the run ships the frontend's in-memory
+   graph, so a reverted node produces no message at all and the 180 s wait
+   below would otherwise absorb it unattributed.
+6. Send a per-run sentinel prompt, and wait for the run
    to COMPLETE on the deterministic signal — `button-stop` hidden **and**
    `button-send` visible — never on a short "did Stop appear?" probe.
-6. **Assert (execute):** the AI reply is non-empty (hard); log whether the
+7. **Assert (execute):** the AI reply is non-empty (hard); log whether the
    sentinel round-tripped (soft, family pattern — model obedience is not the
    contract).
-7. No `allowFlowErrors`.
+8. No `allowFlowErrors`.
 
 ---
 
@@ -253,12 +346,22 @@ reply presence — never model wording.
   distribution is an image-packaging regression, so it must stay red and
   attributed (#931). Contrast with Groq/Mistral, absent by design ⇒ skip
   (#1039).
+- **Pre-run configuration guard (#1302)** — a node that reverted to its
+  defaults cannot produce any message, so without this the spec spends its
+  whole 180 s budget on a locator that will never resolve and reports a
+  `toHaveCount` timeout three layers away from the cause. The guard reads the
+  widget, not the API, because the run ships the in-memory graph.
 - **Force-failure checks** (CONTRIBUTING §2): M1 — test 1 asserts the
   validate-provider body reports `valid === false` (inverted) ⇒ must fail;
   M2 — test 2 expects a never-pulled model name in the live dropdown ⇒ must
   fail; M3 — test 2 asserts the reply is empty (inverted) ⇒ must fail; M4 —
   the pre-flight probe token is changed to a family absent from the build
-  (e.g. `groq`) ⇒ must fail on the attributed pre-flight message.
+  (e.g. `groq`) ⇒ must fail on the attributed pre-flight message; **M5 (#1302)
+  — the model selection is cleared right before the pre-run guard ⇒ the guard
+  must fail there, naming the revert, NOT 180 s later on
+  `div-chat-message`.** M5 is the one that proves the guard is load-bearing:
+  without it the same mutation still fails the test, but as the unattributed
+  timeout this issue was filed under.
 
   M1 was previously documented as "test 1 expects a **4xx** validate-provider
   (inverted)". That mutation could not fail as described: the endpoint answers

tests/helpers/flows/node-config-guard.test.ts

@@ -0,0 +1,116 @@
+// Unit tests for the node-configuration guard (issue #1302).
+// Run with: npm run test:units
+//
+// What rides on these: whether a node that silently reverted reports itself as
+// such, or as the thing #1302 was filed under — `expect(locator).toHaveCount`
+// timing out after 180 s on `div-chat-message`, three layers downstream of the
+// cause, which read as "the model was slow" and sent the issue after the wait
+// budget. The budget was never the cause: the same step costs 5 408-6 503 ms on
+// the four dailies measured, cold container or warm.
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { classifyInfraError } from "../../../scripts/lib/infra-signatures";
+import {
+  classifyConfigOutcome,
+  revertedConfigMessage,
+} from "./node-config-guard";
+
+const DETAIL = {
+  field: "Model Name",
+  expected: "llama3.2:1b",
+  observed: "Select an option",
+  valueTestId: "value-dropdown-dropdown_str_model_name",
+};
+
+test("a widget still showing the value HELD, even inside a larger label", () => {
+  // These widgets render the selection among other text — the spec's own
+  // assertion uses toContainText for the same reason. Equality here would call
+  // a perfectly good selection a revert.
+  assert.equal(classifyConfigOutcome("llama3.2:1b", "llama3.2:1b"), "held");
+  assert.equal(
+    classifyConfigOutcome("llama3.2:1b", "Model: llama3.2:1b (local)"),
+    "held",
+  );
+});
+
+test("the reverted states are the default label, empty, and a missing widget", () => {
+  // "Select an option" is what the #1302 DOM actually showed; the other two are
+  // the states the same revert can leave behind while the canvas re-renders.
+  assert.equal(
+    classifyConfigOutcome("llama3.2:1b", "Select an option"),
+    "reverted",
+  );
+  assert.equal(classifyConfigOutcome("llama3.2:1b", ""), "reverted");
+  assert.equal(classifyConfigOutcome("llama3.2:1b", null), "reverted");
+});
+
+test("a DIFFERENT model is reverted, not held — the run would use the wrong one", () => {
+  // Not hypothetical: the drop-to-workspace-default class (#491/#596) replaces
+  // the selection rather than clearing it, and that run executes green against
+  // a model the test never chose.
+  assert.equal(
+    classifyConfigOutcome("llama3.2:1b", "qwen2.5:0.5b"),
+    "reverted",
+  );
+});
+
+test("the message names the field, both values, and the testid it read", () => {
+  const msg = revertedConfigMessage(DETAIL);
+
+  assert.match(msg, /Model Name/);
+  assert.match(msg, /llama3\.2:1b/);
+  assert.match(msg, /Select an option/);
+  assert.match(msg, /value-dropdown-dropdown_str_model_name/);
+});
+
+test("an empty widget is reported as empty, not as a missing one", () => {
+  // Same distinction the sidebar-add message draws: `""` is a real observation
+  // (the field was reset) and must not read the same as "the widget is gone",
+  // which points at a re-render instead.
+  const empty = revertedConfigMessage({ ...DETAIL, observed: "" });
+  const gone = revertedConfigMessage({ ...DETAIL, observed: null });
+
+  assert.match(empty, /EMPTY/);
+  assert.doesNotMatch(empty, /GONE/);
+  assert.match(gone, /GONE/);
+  assert.doesNotMatch(gone, /EMPTY/);
+});
+
+test("the companion field is what distinguishes one widget from a whole-node reset", () => {
+  // In #1302 BOTH the model and the base URL were back at their defaults. A
+  // message about the dropdown alone would understate it as a selection glitch.
+  const msg = revertedConfigMessage({
+    ...DETAIL,
+    companion: {
+      field: "Ollama API URL",
+      expected: "http://ollama:11434",
+      observed: "http://localhost:11434",
+    },
+  });
+
+  assert.match(msg, /Ollama API URL/);
+  assert.match(msg, /http:\/\/ollama:11434/);
+  assert.match(msg, /http:\/\/localhost:11434/);
+  assert.match(msg, /the whole node was reset/);
+});
+
+test("without a companion the message makes no claim about other fields", () => {
+  assert.doesNotMatch(revertedConfigMessage(DETAIL), /whole node was reset/);
+});
+
+test("the message points at the cause and forbids the wrong reading", () => {
+  // The two sentences that exist to stop #1302 being re-diagnosed as a budget:
+  // it names the flow-save race, and it says outright that this is not slowness.
+  const msg = revertedConfigMessage(DETAIL);
+
+  assert.match(msg, /wait-for-flow-save-settled/);
+  assert.match(msg, /no version check/);
+  assert.match(msg, /in-memory graph/);
+  assert.match(msg, /NOT read this as a slow model or a short timeout/);
+});
+
+test("the reverted-config message is NOT classifiable as an infra failure", () => {
+  // #1262's rule: claiming infra would exempt this from @stable auto-removal
+  // and hide a node that silently drops its configuration.
+  assert.equal(classifyInfraError(revertedConfigMessage(DETAIL)), null);
+});