fix(model-provider): guard the Ollama node's config before the run (#1302) - #1347
Merged
Merged
Conversation
…1302) The quarantined test waited 180 s for `div-chat-message` and saw 0 elements 183 times. #1296 read that as a budget too short for CPU inference and #1302's directive asks for a measured replacement. The artifacts refute the budget three independent ways: the retry, same run, same runner 180 445 ms failed -> 5 644 ms passed (07-30) 180 482 ms failed -> 5 559 ms passed (08-05) green dailies, attempt 0 on a 5 408 ms (08-04), 6 503 ms (08-03) FRESHLY created (cold) container => there is no cold-start penalty `div-chat-message` in dev18 wraps chat-message-${sender_name}-${index}, so it counts the USER's bubble too So the cost is 5-6.5 s cold or warm and the failure is binary: either ~5 s, or nothing at all for 180 s. And "0 elements" does not mean the model was slow — it means the message the test typed never rendered. The failing attempt's DOM says why: the Ollama node reverted to its DEFAULTS — `Model Name` back to "Select an option", `Ollama API URL` back to `http://localhost:11434` while the daily injects `http://ollama:11434`. The model had been selected AND asserted one step earlier (that step passed in 1 059 ms). `Model Name` is required, so the run could never 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 `wait-for-flow-save-settled.ts` already 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 (#358/#357/#995). The spec called that barrier already — it proves PATCH quiescence and nothing about what persisted, and the Playground opens after it. `node-config-guard.ts` adds the two halves the barrier cannot give: - `waitForNodeConfigSettled` re-reads the widget after the quiet window and re-applies the selection at most once. Bounded and deliberate: this recovers a write the product lost after it was applied and asserted, it is not a blind retry of a failed interaction. - `assertNodeConfigHeld` runs immediately before `button-send` — never earlier, because the revert was observed with the Playground already open — and fails naming both fields it read. A persistent revert still fails the test; it fails in ~1 s with the cause named instead of as a `toHaveCount` timeout three layers downstream, which is exactly how this got filed as a budget problem. It 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. A guard querying `GET /api/v1/flows/{id}` could pass while the run executes the reverted state. The 180 s budget is deliberately unchanged, and the measurements above are recorded in the spec doc so the question is not reopened. `test.fixme` is lifted so the test runs again. **`@stable` stays off**: the mechanism fired on 2 of 26 dailies and this spec cannot be reproduced locally at all on arm64 (the SSRF allowlist breaks the very hostname it should allow; the amd64 image will not run; the pip script installs the stable release), so only a `manual.yml` dispatch is admissible evidence and #1302 stays open for it. Also recorded in the spec doc: the four earlier occurrences in the window are TWO causes, not one unsettled month — 07-23/24 are the `lfx-ollama` packaging regression already covered by the pre-flight, 07-15/22 are this same mechanism under the pre-#931 assertion shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #1302. Deliberately does not close it — see What stays open.
Verdict: the node loses its configuration before the run. It is not the budget.
#1302 reads the failure as a 180 s budget too short for CPU inference, and its directive asks for a measured replacement. The artifacts refute that three independent ways, so step 2 of the directive should not be actioned:
div-chat-messagein the dev18 bundlechat-message-${sender_name}-${index}— it counts the user's bubble tooSo the cost is 5–6.5 s cold or warm, and the failure is binary: either ~5 s, or nothing at all for 180 s. That is not a distribution with a long tail. And "0 elements 183 times" does not mean the model was slow — it means the message the test typed never rendered.
The failing attempt's
error-context.mdsays why. The Ollama node on the canvas, behind the open Playground:The daily injects
OLLAMA_BASE_URL_FROM_LANGFLOW: http://ollama:11434, and the step that selects the model passed in 1 059 ms, assertingvalue-dropdown-dropdown_str_model_name. So the model was selected, verified, and then lost.Model Nameis required ⇒ the run cannot start. Everything agrees: the token artifact holds one flow trace for two attempts, and the failing attempt logged zero backend errors in 191 s.This also confirms the fork the spec doc named after #931 — "the root cause to chase is whether the run starts at all versus the Ollama node failing to build". It is the first.
The fix
The mechanism is already documented in this repo, in
helpers/flows/wait-for-flow-save-settled.ts:PATCH /api/v1/flows/{id}has no version check and the frontend applies whichever response lands last, so a stale autosave overwrites the selection in both the store and the database (the root of #358, #357, #995). The spec already called that barrier — it proves PATCH quiescence and says nothing about what persisted, and the Playground opens after it.tests/helpers/flows/node-config-guard.tsadds the two halves the barrier cannot give:waitForNodeConfigSettled— after selecting, re-reads the widget past the quiet window and re-applies the selection at most once. Bounded and deliberate: this recovers a write the product lost after it was applied and asserted; it is not a blind retry of a failed interaction.assertNodeConfigHeld— runs immediately beforebutton-send, never earlier (the revert was observed with the Playground already open), and fails naming both fields it read. A persistent revert still fails the test — in ~1 s with the cause named, instead of as atoHaveCounttimeout three layers downstream, which is precisely how this got filed as a budget problem.It reads the WIDGET, not the API, and that is measured rather than conventional: the run is dispatched as
POST /api/v2/workflowswith a 66 801-byte body — the frontend's in-memory graph, not a reference to the persisted flow. A guard queryingGET /api/v1/flows/{id}could pass while the run executes the reverted state.The 180 s budget is unchanged on purpose. The directive asks for a measured budget if the budget is the cause; it is not. The numbers are recorded in the spec doc so the question is not reopened: 5 408 / 5 559 / 5 644 / 6 503 ms across four dailies, and the run request itself took 4 063 ms.
The four earlier occurrences (a deliverable of the issue)
They are two causes, not one unsettled month — the line number separates them, because #931 rebuilt the file:
expect(locator).toBeVisible() failedpage.waitForSelector: Timeout 30000mslfx-ollamaabsent from the image; already covered by theisProviderComponentAvailablepre-flightexpect(locator).toHaveCount(expected) failedValidation
Force-fails — executed, both directions:
classifyConfigOutcomemutated to always return"held"⇒ 2 of the 9 unit tests fail (the reverted states…,a DIFFERENT model…). Reverted, 0 markers left.FF-2 was run against
mcp-server-tab.spec.tsrather than this spec, because this spec cannot run on an arm64 Mac at all — see below. The probe was reverted and the tree verified clean.This spec is CI-only on arm64 (measured on 1.12.0.dev18)
Recorded in the spec doc's Preconditions so the next person does not spend the cycle. A dockerized Langflow could not reach any Ollama in either topology (host instance via
host.docker.internal; a siblingollama/ollamacontainer on a shared network with the CI's exact allowlist). SettingLANGFLOW_SSRF_ALLOWED_HOSTSmakes it worse: without it the layer answersresolves to blocked IP address(es)(so the name resolved), with it the same name answersDNS resolution failed— for a namegetentandsocket.getaddrinforesolve inside that same container. The amd64 image cannot run here (Fatal glibc error: CPU does not support x86-64-v3), andstart-langflow-pip.shinstalls the stable release rather than the nightly line.Worth flagging on its own: inside one container with the allowlist set,
validate_model_provider_key("Ollama", …)called directly validates and connects, whilePOST /api/v1/models/validate-providerwith the same argument returnsDNS resolution failed. The allowlist is honoured by the library and not by the endpoint.What stays open
@stableis NOT restored.test.fixmeis lifted so the test runs again, but the mechanism fired on 2 of 26 dailies and cannot be reproduced locally, so neither a local green nor a single CI green is admissible evidence. The gate is written into the spec doc: several consecutive greenmanual.ymldispatches with-f retries=0. [Daily #1296] ollama-provider — the flow produces no chat message within 180 s (recurrent 2× same-signature, 6 hits in the 30-day window) #1302 stays open for that, which is why this PR saysRefsand notCloses.gh workflow run manual.yml --repo oriontech-me/langflow-e2e \ --ref fix/issue-1302-ollama-node-config-revert \ -f langflow_target=latest -f langflow_image=nightly \ -f test_grep="Ollama" -f retries=0Which write reverts the node is not pinned. A stale autosave and the bulk
DELETE /api/v1/flows/that appears mid-test underactualWorkers: 2are both candidates, and the artifacts do not separate them. The spec doc says so in those words rather than picking a culprit — if it turns out to be the delete, the fix belongs to the wiper, not here (triage-verdicts.mdverdict 5).🤖 Generated with Claude Code