Skip to content

fix(middleware): announce shared middleware once per session and stop copying it in couple - #1758

Open
CoronRing wants to merge 4 commits into
mainfrom
fix/1675-middleware-shared-lifecycle
Open

CoronRing wants to merge 4 commits into
mainfrom
fix/1675-middleware-shared-lifecycle

Conversation

@CoronRing

Copy link
Copy Markdown
Collaborator

Summary

Finishes #1675 on top of #1695. Stacked on #1695: merge that first. Until then this diff also shows its commits; the change here is only the last commit, fix(middleware): announce shared middleware once per session ....

#1695 stops deep-copying model middleware, so one instance is now shared across every run. That exposed two things:

  • Guardrail traces lose their name from the second run on. Middleware emitted its middleware.creation event once per instance (a _has_registered flag). Session documents resolve middleware names from that session's creation events only (observability/node_internals.py), so a shared guard's rail_name came out as "" in every run after the first. Node-level middleware already had this problem on main; fix: shallow-copy flow context and model_middleware instead of deepcopy #1695 extends it to model_middleware, where guardrails live.
  • rt.couple still deep-copied the existing middleware (Node.extend_middleware / extend_model_middleware). A coupled agent got its own MaxCalls budget and its own Lock, and middleware holding a threading.Lock or DB connection raised TypeError.

Changes:

  • Creation events once per session. The registry of announced middleware now lives on SessionContext and is passed by reference to every frame derived from it (with_scope_pushed, restore_scope), so it lives exactly as long as the session. claim_middleware_registration in context/central.py returns False outside a session, where emit has nowhere to publish, so a call outside a run no longer uses up the first run's announcement.
  • rt.couple shares instances. extend_middleware / extend_model_middleware build a new list but keep the instances.
  • Removed ConversationMemory.__deepcopy__. It only existed to work around the copy in extend_middleware.
  • Docs: couple docstring and the MaxCalls sharing note.

Fixes #1675

Type of change

  • Bug fix
  • Feature
  • Breaking change
  • Docs
  • Refactor / chore / build / tests

Checklist

  • Lint & format pass (ruff check . && ruff format .)
  • Tests added/updated and pass locally (pytest tests)
  • Docs updated if user-facing behavior changed
  • Breaking changes include migration notes

Notes

Tests. All 17 new tests fail with this commit's source changes reverted and pass with them:

Area Tests
Session registry new contexts start empty; pushed scopes and restore_scope share it; once per session; new session resets; nothing claimed outside a session
Creation events, end to end shared node and model middleware announce once in every run, however often they run; concurrent runs each get one; original + coupled node share one per run; an announcement outside a session doesn't use up the first run
Guardrail trace rail_name is set in both of two runs of the same flow (integration)
rt.couple keeps node and model middleware instances (agents and function_node); middleware holding a threading.Lock or sqlite3.Connection; coupled agent spends the original MaxCalls budget
ConversationMemory a coupled agent still fills the caller's memory handle

Full unit + integration suite: 4076 passed. The only failures were the 2 MCP integration tests, which pip install an MCP server and couldn't in my local env. mkdocs build --strict passes.

Behaviour change. A coupled node now shares its middleware with the node it came from, as agent_node / function_node already do (see the comment in node_builder.py). For MaxCalls and Lock that's the point. For a custom stateful middleware it means state is shared across the original and coupled node rather than starting fresh.

Not changed. Node.safe_copy still deep-copies node attributes. I checked it with a non-copyable object passed as a node argument and the run, to_vertices() and graph_serialization() all work, since node instances don't keep their call arguments. So I left it out of scope.

Christian-Sidak and others added 4 commits October 22, 2026 20:24
deepcopy in two hot paths crashed on non-copyable objects (locks, DB
connections, sockets) and silently cloned stateful objects so writes
never reached the caller.

- connection.py: dict(flow._context) gives per-run isolation without
  deep-copying user values
- flow.py update_context: copy.copy(self) + shallow dict merge avoids
  deep-copying the whole Flow including callbacks
- model_invoker.py create_with_llm_observe: list(middleware) passes
  instances through by reference, matching MiddlewareChain behavior,
  so stateful middleware (rate limiters, counters) accumulates state
  correctly across LLM calls

Fixes #1675

Signed-off-by: Christian-Sidak <61099993+Christian-Sidak@users.noreply.github.com>
…ions

- Replace the closure-based model middleware tests, which passed without the
  fix, with ones that keep state on the middleware instance and build one
  invoker per agent invocation.
- Add end-to-end MaxCalls tests for the model_middleware slot, shared across
  invocations and across agents, plus a prebuilt Lock smoke test.
- Cover the Flow context contract both ways: values shared by reference
  (Lock, sqlite3 connection, nested dict writes) and top-level put/delete
  isolated per run, including concurrent runs and update_context.
- Update the Flow docs and the ModelInvoker docstring to match.

Signed-off-by: Guan Zheng Huang <guanzheng.huang@hotmail.com>
… copying it in couple

Middleware instances are shared policy objects, so tracking the creation
event with a per-instance flag meant a shared middleware only announced
itself in the first run of the process. Session documents resolve middleware
names per session, so from the second run on a guardrail trace lost its
rail_name. The registry now lives on the session and is shared by every
frame derived from it.

Node.extend_middleware and extend_model_middleware (used by rt.couple) no
longer deep-copy the existing middleware, so a coupled node shares the
original's MaxCalls budget, Lock or ConversationMemory, and middleware
holding a non-copyable resource no longer raises. ConversationMemory's
__deepcopy__ only existed to work around that copy and is removed.

Fixes #1675
@CoronRing CoronRing self-assigned this Oct 29, 2026
@CoronRing
CoronRing requested a review from Amir-R25 October 29, 2026 19:55
CoronRing added a commit that referenced this pull request Oct 29, 2026
…py (#1695)

## Summary

- connection.py: replace deepcopy(flow._context) with
dict(flow._context) to give per-run isolation without deep-copying
user-supplied values
- flow.py update_context: replace deepcopy(self) with copy.copy(self)
and a shallow dict merge, avoiding a full deep-copy of the Flow
including callbacks on every call
- model_invoker.py create_with_llm_observe: replace deepcopy(middleware)
with list(middleware), passing instances through by reference so
stateful middleware accumulates state correctly across LLM calls

The dict(...) shallow copy preserves the existing contract:
rt.context.put(...) inside a run writes to a per-run copy of the
top-level dict, so it cannot leak into flow._context or the next run.

## Test plan

- [x] test_flow_invoke_passes_non_copyable_context_object:
threading.Lock in context no longer raises TypeError
- [x] test_flow_invoke_context_value_is_same_object_not_clone: run sees
the callers exact object (is check)
- [x] test_flow_update_context_non_copyable_object: update_context with
a lock in context no longer raises
- [x] test_flow_update_context_does_not_modify_original: original Flow
is unchanged after update_context
- [x] test_flow_context_mutations_isolated_between_runs: rt.context.put
inside a run does not bleed into flow._context
- [x] test_create_with_llm_observe_preserves_stateful_middleware:
counting middleware accumulates state across two invoke calls (reads 2,
not 0)
- [x]
test_create_with_llm_observe_non_copyable_middleware_does_not_crash:
middleware holding a threading.Lock no longer raises TypeError
- [x] Existing test suite passes: all 30 unit tests and 21 integration
flow tests green locally

Part of #1675 (finished in #1758)

---------

Signed-off-by: Christian-Sidak <61099993+Christian-Sidak@users.noreply.github.com>
Signed-off-by: Guan Zheng Huang <guanzheng.huang@hotmail.com>
Co-authored-by: Guan Zheng Huang <guanzheng.huang@hotmail.com>
Co-authored-by: Guan Huang <89324736+CoronRing@users.noreply.github.com>
@Amir-R25
Amir-R25 requested a review from Pooria90 October 29, 2026 21:23

@Pooria90 Pooria90 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CoronRing.
The PR looks clean to me and nothing is blocking. I have two comments:

  1. Please edit the comments that refer to the old process-wide middleware. My Claude flagged these:
  • utils/logging/run_view.py:62
  • cli/viz_api/queries/_common.py:208-212
  • cli/viz_api/queries/middleware.py:139-146
  • cli/viz_api/queries/guardrails.py:46-49
  1. The fact that middleware are shared now, make the concurrent sessions fail esp if the user does not properly handle stateful middleware. We need to document this behavior, refer them to proper implementations like MaxCalls and likely provide some tools for them so they can make stateful middleware more easily.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flow context and agent model_middleware are deep-copied, crashing on non-copyable objects and silently dropping state

3 participants