Skip to content

Commit 5675d9d

Browse files
committed
fix(cold-start): address review findings on the QA-fix work
Six items raised by parallel review pass (silent-failure-hunter, code-reviewer, pr-test-analyzer): - create_class AttributeError handler was catching every AttributeError, masking real user bugs (self.foo.bar where foo is None) behind a generic 'Attribute error while creating class' wrap. Narrowed to the partial-init signature; other AttributeErrors are inlined into the same 'Error creating class. AttributeError(...)' shape the generic handler produces. Original message preserved in the wrap. - Extension-loader cache-hit swallow narrowed from broad except Exception to (ExtensionError, ImportError). At the cache-hit point built-ins already loaded, so an unexpected exception is much more likely a real Extension System bug than a missing optional package; swallowing it would silently shrink the palette. - flow_executor.py split OSError into its own branch with a generic client-facing message. OSError stringifies as '[Errno 2] No such file or directory: /abs/path/...' which leaks server filesystem layout on multi-tenant deploys. ValueError / JSONDecodeError stay in the rich-detail branch. - _LazyImportProxy._resolve pre-prime changed to a first-segment set match so bare 'langchain' (the umbrella package) is covered alongside langchain_classic and langchain_community. Added logger.debug on the success path. - _check_function_body_name_resolution rewritten as recursive _check_fn so nested function bodies are checked against their own locals (the flat ast.walk would false-positive on inner-function parameters). Lambdas get a synthetic-locals scope. Class-body assignments are added to the visible set. Added ast.NamedExpr branch to _function_locals so walrus bindings register. - 11 regression tests for _check_function_body_name_resolution covering QA Fixture A, legacy-lfx symbols, dynamic-runtime globals (no-flag), imported / locally-assigned / param / except-alias / comprehension-iter / walrus / nested-def / with-as scope handling. Pre-prime tests gain a bare-langchain case and a langchain_core skip-case. Generic AttributeError test now asserts the original message survives the wrap.
1 parent 5eb8e8c commit 5675d9d

4 files changed

Lines changed: 408 additions & 47 deletions

File tree

src/backend/base/langflow/agentic/services/flow_executor.py

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,14 +199,26 @@ async def execute_flow_file_streaming(
199199
except CustomComponentValidationError as e:
200200
logger.error(f"Flow preparation error: {e}")
201201
raise HTTPException(status_code=400, detail=str(e)) from e
202-
except (json.JSONDecodeError, OSError, ValueError) as e:
202+
except OSError as e:
203+
# OSError messages tend to embed absolute filesystem paths
204+
# ("[Errno 2] No such file or directory: '/srv/langflow/flows/...'")
205+
# which leak server layout to end users on multi-tenant deploys.
206+
# Keep the rich detail in the server log; return a generic message
207+
# to the client.
208+
logger.error(f"Flow preparation OSError: {e}")
209+
raise HTTPException(
210+
status_code=500,
211+
detail="An error occurred while preparing the flow.",
212+
) from e
213+
except (json.JSONDecodeError, ValueError) as e:
203214
# Include the underlying error message in the HTTP detail so the UI
204215
# surfaces something actionable. The generic "An error occurred while
205216
# preparing the flow." string hid the real failure (e.g. the torch
206217
# partial-init AttributeError that `validate.create_class` re-raises
207218
# as a ValueError), forcing users to dig through server logs to find
208-
# the cause. ValueError messages from validate.py already include
209-
# actionable hints; passing them through is safe.
219+
# the cause. ValueError messages from validate.py and JSONDecodeError
220+
# already include curated, user-safe content (no filesystem paths);
221+
# passing them through is safe.
210222
logger.error(f"Flow preparation error: {e}")
211223
raise HTTPException(
212224
status_code=500,

src/lfx/src/lfx/custom/validate.py

Lines changed: 119 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -164,26 +164,93 @@ def _function_locals(fn: ast.FunctionDef | ast.AsyncFunctionDef | ast.Lambda) ->
164164
elif isinstance(sub, (ast.ListComp, ast.SetComp, ast.GeneratorExp, ast.DictComp)):
165165
for gen in sub.generators:
166166
_collect_target_names(gen.target, locals_)
167+
elif isinstance(sub, ast.NamedExpr):
168+
# Walrus operator (`if (Tool := load_tool()):`) introduces a
169+
# new binding in the enclosing function scope. Without this,
170+
# a walrus-assigned name that happens to live in the hint
171+
# table (e.g. ``Tool``) would false-positive.
172+
_collect_target_names(sub.target, locals_)
167173
return locals_
168174

175+
def _check_fn(fn: ast.FunctionDef | ast.AsyncFunctionDef, outer_visible: set[str]) -> None:
176+
"""Recurse into a function body, checking Name(Load) refs against the visible scope.
177+
178+
``outer_visible`` is the union of names visible from enclosing scopes
179+
(module globals + each enclosing function's locals). The function's own
180+
locals_ extend that set for the body walk. Nested function definitions
181+
recurse with their own (outer | self) visible set so their parameters
182+
and locals don't false-positive against the caller's scope, and the
183+
caller's locals don't shadow the nested function's lookup.
184+
"""
185+
own_locals = _function_locals(fn)
186+
visible = outer_visible | own_locals
187+
188+
# Walk only the function's direct body, descending into nested defs
189+
# via explicit recursion instead of flat-walking — otherwise a Name
190+
# reference inside a nested function would be checked against the
191+
# outer function's locals and miss the nested function's own params.
192+
def _visit(node: ast.AST) -> None:
193+
for child in ast.iter_child_nodes(node):
194+
if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)):
195+
_check_fn(child, visible)
196+
continue
197+
if isinstance(child, ast.Lambda):
198+
# Lambdas have their own scope; the parameters belong to
199+
# the lambda body only. Construct a synthetic FunctionDef
200+
# locals_ from the lambda's args.
201+
lambda_locals: set[str] = set()
202+
for a in (*child.args.posonlyargs, *child.args.args, *child.args.kwonlyargs):
203+
lambda_locals.add(a.arg)
204+
if child.args.vararg:
205+
lambda_locals.add(child.args.vararg.arg)
206+
if child.args.kwarg:
207+
lambda_locals.add(child.args.kwarg.arg)
208+
lambda_visible = visible | lambda_locals
209+
for sub in ast.walk(child.body):
210+
if isinstance(sub, ast.Name) and isinstance(sub.ctx, ast.Load):
211+
_maybe_flag(sub.id, lambda_visible)
212+
continue
213+
if isinstance(child, ast.Name) and isinstance(child.ctx, ast.Load):
214+
_maybe_flag(child.id, visible)
215+
_visit(child)
216+
217+
for stmt in fn.body:
218+
# Top-level nested defs in this function's body. Skip _visit (which
219+
# would treat the def's body items as direct children of fn) and
220+
# recurse with the nested function's own (visible | self) scope.
221+
if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef)):
222+
_check_fn(stmt, visible)
223+
continue
224+
_visit(stmt)
225+
226+
def _maybe_flag(name: str, visible: set[str]) -> None:
227+
if name in visible or name in builtin_names:
228+
return
229+
# Only surface the typed hint when we actually know which module the
230+
# missing name should come from. Names without a known import target
231+
# are deliberately passed through: they may be runtime-injected
232+
# globals (graph-level context, monkey-patched bases, etc.) and we
233+
# don't want to false-positive on those.
234+
if _resolve_import_module_for_name(name) is None:
235+
return
236+
msg = _format_undefined_name_message(name, f"name '{name}' is not defined")
237+
raise ValueError(msg)
238+
239+
module_visible = set(exec_globals)
169240
for class_node in (n for n in module.body if isinstance(n, ast.ClassDef)):
241+
# Names assigned at the class body level (e.g. `outputs = [...]`) are
242+
# visible inside method bodies via the implicit `self.outputs` and
243+
# also via plain name lookup during class-body exec. Add them to the
244+
# visible set so methods referencing them don't false-positive.
245+
class_visible = module_visible | {
246+
tgt.id
247+
for stmt in class_node.body
248+
if isinstance(stmt, ast.Assign)
249+
for tgt in stmt.targets
250+
if isinstance(tgt, ast.Name)
251+
}
170252
for fn in (n for n in class_node.body if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef))):
171-
locals_ = _function_locals(fn)
172-
for sub in ast.walk(fn):
173-
if isinstance(sub, ast.Name) and isinstance(sub.ctx, ast.Load):
174-
name = sub.id
175-
if name in locals_ or name in exec_globals or name in builtin_names:
176-
continue
177-
# Only surface the typed hint when we actually know which
178-
# module the missing name should come from. Names without
179-
# a known import target are deliberately passed through:
180-
# they may be runtime-injected globals (graph-level
181-
# context, monkey-patched bases, etc.) and we don't want
182-
# to false-positive on those.
183-
if _resolve_import_module_for_name(name) is None:
184-
continue
185-
msg = _format_undefined_name_message(name, f"name '{name}' is not defined")
186-
raise ValueError(msg)
253+
_check_fn(fn, class_visible)
187254

188255

189256
def add_type_ignores() -> None:
@@ -487,23 +554,32 @@ def create_class(code, class_name):
487554
msg = f"Import error while creating class: {e!s}.{install_hint}"
488555
raise ValueError(msg) from e
489556
except AttributeError as e:
490-
# Sibling case to the ImportError branch above. The lazy-import refactor
491-
# delays `importlib.import_module(...)` to first attribute access on a
492-
# `_LazyImportProxy`, so a broken transitive import (e.g. torch's
493-
# "partially initialized module 'torch' has no attribute 'library'"
494-
# circular-import bug surfaced via `transformers`) raises AttributeError
495-
# at proxy-resolution time rather than ImportError. Without this branch
496-
# the generic catch-all wraps it as "Error creating class. AttributeError(...)"
497-
# which reads like a code typo even though the fix is environment-level.
498-
torch_partial_init = "partially initialized module" in str(e) or "circular import" in str(e)
499-
hint = (
500-
" This usually means a transitive C-extension import (commonly torch via transformers/"
501-
"langchain) failed to fully initialize. Reinstalling the broken package, pinning a known-"
502-
"good version, or restarting the interpreter typically resolves it."
503-
if torch_partial_init
504-
else ""
505-
)
506-
msg = f"Attribute error while creating class: {e!s}.{hint}"
557+
# Sibling case to the ImportError branch above, scoped narrowly to the
558+
# circular-import / partial-init signature we know about (torch 2.x
559+
# nested under langchain via transformers). The lazy-import refactor
560+
# delays `importlib.import_module(...)` to first attribute access on
561+
# a `_LazyImportProxy`, so a broken transitive import surfaces here
562+
# as AttributeError rather than ImportError; without this branch the
563+
# generic catch-all would wrap it as "Error creating class.
564+
# AttributeError(...)" which reads like a code typo even though the
565+
# fix is environment-level.
566+
#
567+
# Other AttributeErrors (legitimate user bugs like ``self.foo.bar``
568+
# where ``foo`` is None) are wrapped with the same shape as the
569+
# generic-Exception handler below ("Error creating class. AttributeError(...)")
570+
# so the user sees the actual exception type and is not mis-directed
571+
# toward a torch/transformers environment fix. Inlining instead of
572+
# ``raise`` because Python does not re-enter the except chain.
573+
msg_text = str(e)
574+
if "partially initialized module" in msg_text or "circular import" in msg_text:
575+
hint = (
576+
" This usually means a transitive C-extension import (commonly torch via transformers/"
577+
"langchain) failed to fully initialize. Reinstalling the broken package, pinning a known-"
578+
"good version, or restarting the interpreter typically resolves it."
579+
)
580+
msg = f"Attribute error while creating class: {msg_text}.{hint}"
581+
else:
582+
msg = f"Error creating class. {type(e).__name__}({msg_text})."
507583
raise ValueError(msg) from e
508584
except ValueError:
509585
# Static analysis (_check_function_body_name_resolution) and any other
@@ -672,9 +748,18 @@ def _resolve(self):
672748
# to walk a transitive chain that almost always pulls it (langchain
673749
# families known to depend on transformers/torch). Cheap when torch is
674750
# already loaded, no-op when torch is not installed.
675-
if module_name.startswith(("langchain_classic", "langchain.", "langchain_community")):
751+
#
752+
# Scope: first-segment match, so bare ``langchain`` (the umbrella
753+
# package, which re-exports langchain_classic.agents et al) is also
754+
# covered. Direct user imports of transformers / torch from outside
755+
# the langchain family are NOT pre-primed here; they go through the
756+
# standard import path and inherit whatever partial-init risk Python
757+
# gives them.
758+
top_segment = module_name.split(".", 1)[0]
759+
if top_segment in {"langchain", "langchain_classic", "langchain_community"}:
676760
with contextlib.suppress(ImportError):
677761
importlib.import_module("torch")
762+
logger.debug("Pre-primed torch for langchain-family proxy: %s", module_name)
678763

679764
try:
680765
if is_module_binding:

src/lfx/src/lfx/interface/components.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1218,10 +1218,17 @@ async def get_and_cache_all_types_dict(
12181218
# via pip-installed Extensions / seed dirs / dev / inline bundles, all
12191219
# independent of the lfx version stamp). Load them on every cache-hit
12201220
# so manifest-shipping bundles always win on collision, matching the
1221-
# cache-miss path. Failures must never block the cache build.
1221+
# cache-miss path. Narrow exception catch: at the cache-hit point the
1222+
# built-ins are already loaded fine, so a failure here is much more
1223+
# likely a real Extension System bug than a missing optional package.
1224+
# `import_extension_components` already routes typed per-bundle errors
1225+
# through `_emit_extension_diagnostics`; only catch the "expected"
1226+
# absent-deps modes (ExtensionError + ImportError) and let unexpected
1227+
# exceptions propagate so we see them instead of silently shrinking
1228+
# the palette.
12221229
try:
12231230
extension_components = await import_extension_components(settings_service)
1224-
except Exception as exc: # noqa: BLE001
1231+
except (ExtensionError, ImportError) as exc:
12251232
await logger.aerror("Extension System load failed; continuing without it: %s", exc)
12261233
extension_components = {}
12271234
component_cache.all_types_dict = {**merged, **custom_flat, **extension_components}

0 commit comments

Comments
 (0)