fix(server): dispatch pipeline lifecycle in topological order, including control sub-pipelines

[asclearuc]

Summary

• Per-object lifecycle (open/closing/close) was dispatched in .pipe canvas /
  input[] order, not dependency order, so a join node could be flushed before an
  upstream branch emitted its flush-time data — silently dropping it (the admin
  chat.pipe incident: embeddings never reached the vector store).
• Lifecycle is now bound flat onto each region's root in topological order:
  closing/close upstream-first (a node flushes only after ALL its upstream parents),
  open reversed. The pipe head owns the source region; each control/invoke node (e.g.
  tool_pipe) owns the sub-pipeline it reaches and drives it the same way, to any
  nesting depth. Orders are computed once by computeLifecycleOrder (reachability + Kahn
  topo sort) and partitioned by computeLifecycleRegions.
• tool_pipe now drives closing() before close(), so buffer-and-flush nodes in its
  sub-pipeline emit before the tool reads its result. A diamond sub-pipeline returns the
  merged output of all branches instead of dropping one.
• Wirings that cannot honour one-owner-per-node are rejected when the pipeline opens:
  a sub-pipeline shared with the main flow or a second start; a node reached by two
  control nodes; an invoke node that is itself data-fed.

Behaviour notes for reviewers:

• Policy: a node is flushed and closed only after every node that feeds it, and
  opened before them. In the diamond A -> J, B -> J:

  • closing/close: A, B, then J — the join is last
  • open: J, then B, A — the join is first, so it is ready before anyone writes to it

  This matches today's chain semantics, so existing linear pipelines are unchanged.

• Control/invoke sub-pipelines are no longer left on per-parent bindings — this is the
  scope change versus the first review. Their nodes move onto the invoke node's own lanes.

• The three rejections above are new hard failures at beginEndpoint / client.use().
  Normal agent -> tool_pipe -> sub-pipeline wiring is unaffected; invoking the same
  tool from two agents stays valid.

• Rejection messages name nodes by service title and component id, e.g.
  Control node "Pipeline Tool" (pipe_tool_1) reaches node "Prompt" (sub_mid) ....

• An error in a native node's closing now aborts the remaining flush walk (Python nodes
  already did; unifies semantics).

• Ec::PreventDefault from a lifecycle callback no longer prunes a subtree (no production
  node uses it there).

• Trace apaevt_flow: lifecycle frames appear flat under the pipe head, and under each
  control node for its sub-pipeline (pairs stay balanced; no protocol change).

• buildConnections now clears the controls table (stale rows could survive a retried
  beginEndpoint).

Type

fix

Testing

• Tests added or updated
  • store::lifecycleOrder — 22 sections / 31 assertions (per-region ordering)
  • store::lifecycleRegions — 12 sections / 20 assertions (partition + all three
    ownership rejections, nested invoke chains, multi-start topologies)
  • nodes/test/test_lifecycle_order.py — end-to-end guard for the closing order: a
    diamond of buffer-and-flush nodes whose join is declared before its branches, so
    declaration-order dispatch would flush it early and drop a branch. No LLM or mock.
• Tested locally — store::linkages (38) and PipelineConfig (42) green, no
  regressions. Runtime verified against a live engine with instrumented sub-pipeline
  nodes: each node receives open/closing/close exactly once per invocation and
  the join flushes last, both single-level and two-level nested.
• ./builder test passes

Scope of the new E2E: it asserts the closing order. open/close order and the
tool_pipe region are not covered by CI (driving a tool needs an agent + LLM); those are
covered by the unit tests, the manual runtime verification above, and the rejection
examples in examples/incorrect/.

Docs & examples

• docs/agents/ROCKETRIDE_PIPELINE_RULES.md — lifecycle guarantee extended to control
  sub-pipelines, plus the three wiring constraints
• docs/agents/ROCKETRIDE_COMMON_MISTAKES.md — new entry for sharing a sub-pipeline
• nodes/src/nodes/tool_pipe/README.md — exclusive-ownership rule
• packages/server/docs/observability.md — how sub-pipeline lifecycle frames appear
• examples/tool-pipe-diamond.pipe, examples/tool-pipe-nested.pipe, and
  examples/incorrect/ (five rejected wirings + README)

Checklist

• Commit messages follow conventional commits
• No secrets or credentials included
• Wiki updated (if applicable)
• Breaking changes documented (if applicable)

Linked Issue

Fixes #1650

Summary by CodeRabbit

• New Features

  • Improved pipeline lifecycle handling for deterministic, dependency-aware execution.
  • Ensured merge and join components receive all upstream outputs before flushing.
  • Added validation for invalid sub-pipeline wiring, including shared or conflicting ownership.
  • Added support and examples for nested and diamond-shaped sub-pipelines.

• Bug Fixes

  • Fixed buffered sub-pipeline results being read before flush completion.
  • Ensured sub-pipelines close reliably even when flushing encounters an error.

• Documentation

  • Expanded lifecycle, observability, troubleshooting, and invalid-pipeline guidance.

[github-actions]

🤖 Internal: Discord sync marker

Auto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete.

[coderabbitai]

📝 Walkthrough

Walkthrough

The engine now computes dependency-ordered lifecycle regions, binds open/closing/close accordingly, validates control-node ownership, and flushes tool sub-pipelines before reading responses. Tests and examples cover joins, nested pipelines, invalid wiring, and lifecycle tracing.

Changes

Pipeline lifecycle ordering

Layer / File(s)	Summary
Lifecycle order computation and validation packages/server/engine-lib/engLib/store/headers/endpoint.hpp, packages/server/engine-lib/engLib/store/stack.cpp, packages/server/engine-lib/test/store/pipeline/lifecycle_order.cpp	Adds deterministic topological lifecycle ordering, head/control region partitioning, cycle detection, ownership validation, stored endpoint state, and unit coverage.
Lifecycle lane binding packages/server/engine-lib/engLib/store/core/endpoint/endpoint.pipes.cpp	Binds region-owned lifecycle lanes in dependency order while retaining deduplicated parent binding for unowned targets.
Sub-pipeline lifecycle execution and examples nodes/src/nodes/tool_pipe/*, examples/tool-pipe-*.pipe, examples/incorrect/*	Flushes tool sub-pipelines before response reads, guarantees close after flush, and adds valid nested/diamond plus rejected ownership and wiring examples.
Lifecycle and ownership contracts docs/agents/*, packages/server/docs/observability.md, examples/incorrect/README.md	Documents lifecycle ordering, flush guarantees, trace-frame structure, and control-node sub-pipeline ownership rules.
Flush-order end-to-end validation nodes/test/test_lifecycle_order.py	Verifies a diamond join receives both branches’ flush-time outputs.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: jmaionchi, rod-christensen, stepmikhaylov, kwit75

Sequence Diagram(s)

sequenceDiagram
  participant PipelineInput
  participant IServiceEndpoint
  participant LifecycleRegions
  participant ToolPipe
  participant ResponseLane
  PipelineInput->>IServiceEndpoint: buildConnections
  IServiceEndpoint->>LifecycleRegions: compute dependency-ordered regions
  LifecycleRegions-->>IServiceEndpoint: return head and control orders
  IServiceEndpoint->>ToolPipe: dispatch open, closing, and close
  ToolPipe-->>ResponseLane: write flushed response
  IServiceEndpoint->>ResponseLane: read response after flush

Loading

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR fixes source lifecycle order, but also changes control-sub-pipeline lifecycle and ownership rules that #1650 says belong in follow-up scope.	Limit #1650 to source-connected data-flow ordering and move control-sub-pipeline lifecycle, ownership, docs, and examples to a separate PR.
Out of Scope Changes check	⚠️ Warning	Docs, example pipes, and control-sub-pipeline region logic extend beyond the source-connected lifecycle fix requested by #1650.	Trim those additions or split them into a follow-up so this PR only changes the dependency-ordered lifecycle dispatch.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: lifecycle dispatch is made topological, including control sub-pipelines.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/1650-open-closing-close-order

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[joshuadarron]

Overview

Solid fix for a real bug. Lifecycle (open/closing/close) was dispatched in .pipe declaration order via per-parent binder chains, so a join node could flush before an upstream branch emitted its flush-time data. This binds the source-reachable nodes' lifecycle flat onto the pipe head in topological order — closing/close upstream-first, open reversed — leaving control/invoke sub-pipelines on their per-parent bindings.

I verified the two claims the whole design rests on, and both hold:

• open downstream-first matches existing semantics. IPythonInstanceBase::open calls Parent::open (→ binder.open → children) before invoking the Python node's own open(), so children already opened first under the old chain. Reversing lifecycleOrder preserves that.
• closing/close upstream-first matches existing semantics. IPythonInstanceBase::closing/close run the Python method first, then Parent:: → binder → children. Parent-before-child, same as the new flat order.

Also checked:

• tool_pipe is the only production consumer of self.instance.open()/close() (nodes/src/nodes/tool_pipe/IInstance.py:98,106). It is wired to agents through control/invoke, never as a data input target, so neither it nor its sub-pipeline nodes are source-reachable — the exclusion works, and its per-parent bindings survive. packages/ai/src/ai/modules/data/data_conn.py:504 is the pipe head, unaffected.
• The Ec::PreventDefault-no-longer-prunes note is accurate: every preventDefault() call in nodes/ is in a data lane (writeText, writeImage, writeDocuments, renderObject), none in open/closing/close.
• pipeStack[0] is always the pipe filter (buildPipeStack pushes it first) and instanceStack is 1:1 with pipeStack, so lifecycleOrder indices can't collide with the head. bindFilters has exactly one caller, so the rewritten doc comment ("called when we create the initial filter stack") is correct.
• No double-binding: the skip is keyed on toIndex, so a head-managed target is bound exactly once, on the head.
• computeLifecycleOrder is deterministic and allocation-safe — unordered_map element references survive rehash, so for (int to : adj[node]) is fine even though adj[node] may insert.

Docs are updated per the co-located rule, CI is green on all three platforms, and the reasoning for skipping the E2E (a naive diamond with a response join is a false-green) is sound.

Two things I'd like addressed before merge — both small.

---

Requested changes

1. Binder::getBound() is dead code

packages/server/engine-lib/engLib/store/core/binder.cpp:59 + headers/binder.hpp:178

The accessor is added to the public Binder API, documented as "Used by tests to assert lifecycle lane contents/order" — and has zero callers. The only new test (lifecycle_order.cpp) exercises the pure computeLifecycleOrder function and never touches a Binder.

This is public surface added for a consumer that doesn't exist, which .claude/rules/architecture.md explicitly rules out ("Every file and folder exists because something depends on it right now"). Either use it (see #2) or drop it.

2. Nothing tests the actual bind order

All 10 new tests cover the ordering algorithm. Nothing covers the part of bindFilters that consumes it, which is where a silent regression would live:

• closing/close bound on filters[0] in forward order, open in reverse
• head-managed targets skip the per-parent lifecycle bind
• non-reachable (control/invoke) targets keep it

That last one is the tool_pipe contract. If someone later drops the headManaged guard, computeLifecycleOrder still passes all 10 tests and tool_pipe breaks. getBound() is exactly the hook needed to assert this — please add the test it was written for, so the accessor earns its place.

---

Non-blocking nits

• Bounds-check asymmetry (endpoint.pipes.cpp:145-159): the closing/close loop validates toIndex against filters.size(); the reversed open loop doesn't. It's safe only because loop 1 validates every entry before loop 2 runs — an implicit coupling that breaks the moment someone reorders or early-exits. Validate the whole lifecycleOrder once up front (or in buildConnections, against pipeStack.size()) and drop the in-loop check.
• toIndex >= filters.size() compares int to size_t. Consistent with the pre-existing checks in this file and builds are clean, so noting only for consistency if you touch it per the point above.
• headManaged allocation: the comment at stack.cpp says the order is precomputed "so bindFilters stays allocation-free per pipe", but bindFilters now builds an unordered_set per pipe stack. lifecycleOrder is small — a linear std::find would beat the hash set here anyway. Either way, the comment overstates it.
• Kahn loop is O(N²) (stack.cpp): the inner scan restarts from the head of nodeOrder after each emit. Irrelevant at pipeline sizes and it runs once per endpoint, so fine — just flagging it's quadratic by construction, not by accident. A rolling start index would make it linear-ish if you ever care.
• if (inDegree[to] > 0) inDegree[to]--;: with deduped edges and seeded in-degrees this guard can't fire. It converts a would-be logic error into a silently wrong order. An assert (or nothing) says more.
• edges duplicates the information already in adj; the in-degree pass could walk adj directly and drop the extra vector.

---

Question

The mixed reachability node included after reachable parent test locks in a behavior change worth confirming: a node fed by both a source-reachable parent and a control/invoke node becomes head-managed, so its invoke node's self.instance.open()/close() no longer reaches it.

Concretely — a response node shared between the main pipeline and a tool_pipe sub-branch would stop being opened/closed per tool invocation. The tool_pipe README says to cap each branch with its own response node, so this is probably out of contract, but it's a wiring a user can produce on the canvas. Is that the intended trade-off? If so, worth a line in ROCKETRIDE_PIPELINE_RULES.md next to the new lifecycle guarantee.

Also: the ./builder test checkbox is still unchecked in the description.

[asclearuc]

Overview

Solid fix for a real bug. Lifecycle (open/closing/close) was dispatched in .pipe declaration order via per-parent binder chains, so a join node could flush before an upstream branch emitted its flush-time data. This binds the source-reachable nodes' lifecycle flat onto the pipe head in topological order — closing/close upstream-first, open reversed — leaving control/invoke sub-pipelines on their per-parent bindings.

I verified the two claims the whole design rests on, and both hold:

• open downstream-first matches existing semantics. IPythonInstanceBase::open calls Parent::open (→ binder.open → children) before invoking the Python node's own open(), so children already opened first under the old chain. Reversing lifecycleOrder preserves that.
• closing/close upstream-first matches existing semantics. IPythonInstanceBase::closing/close run the Python method first, then Parent:: → binder → children. Parent-before-child, same as the new flat order.

Also checked:

• tool_pipe is the only production consumer of self.instance.open()/close() (nodes/src/nodes/tool_pipe/IInstance.py:98,106). It is wired to agents through control/invoke, never as a data input target, so neither it nor its sub-pipeline nodes are source-reachable — the exclusion works, and its per-parent bindings survive. packages/ai/src/ai/modules/data/data_conn.py:504 is the pipe head, unaffected.
• The Ec::PreventDefault-no-longer-prunes note is accurate: every preventDefault() call in nodes/ is in a data lane (writeText, writeImage, writeDocuments, renderObject), none in open/closing/close.
• pipeStack[0] is always the pipe filter (buildPipeStack pushes it first) and instanceStack is 1:1 with pipeStack, so lifecycleOrder indices can't collide with the head. bindFilters has exactly one caller, so the rewritten doc comment ("called when we create the initial filter stack") is correct.
• No double-binding: the skip is keyed on toIndex, so a head-managed target is bound exactly once, on the head.
• computeLifecycleOrder is deterministic and allocation-safe — unordered_map element references survive rehash, so for (int to : adj[node]) is fine even though adj[node] may insert.

Docs are updated per the co-located rule, CI is green on all three platforms, and the reasoning for skipping the E2E (a naive diamond with a response join is a false-green) is sound.

Two things I'd like addressed before merge — both small.

Requested changes

1. Binder::getBound() is dead code

packages/server/engine-lib/engLib/store/core/binder.cpp:59 + headers/binder.hpp:178

The accessor is added to the public Binder API, documented as "Used by tests to assert lifecycle lane contents/order" — and has zero callers. The only new test (lifecycle_order.cpp) exercises the pure computeLifecycleOrder function and never touches a Binder.

This is public surface added for a consumer that doesn't exist, which .claude/rules/architecture.md explicitly rules out ("Every file and folder exists because something depends on it right now"). Either use it (see #2) or drop it.

2. Nothing tests the actual bind order

All 10 new tests cover the ordering algorithm. Nothing covers the part of bindFilters that consumes it, which is where a silent regression would live:

• closing/close bound on filters[0] in forward order, open in reverse
• head-managed targets skip the per-parent lifecycle bind
• non-reachable (control/invoke) targets keep it

That last one is the tool_pipe contract. If someone later drops the headManaged guard, computeLifecycleOrder still passes all 10 tests and tool_pipe breaks. getBound() is exactly the hook needed to assert this — please add the test it was written for, so the accessor earns its place.

Non-blocking nits

• Bounds-check asymmetry (endpoint.pipes.cpp:145-159): the closing/close loop validates toIndex against filters.size(); the reversed open loop doesn't. It's safe only because loop 1 validates every entry before loop 2 runs — an implicit coupling that breaks the moment someone reorders or early-exits. Validate the whole lifecycleOrder once up front (or in buildConnections, against pipeStack.size()) and drop the in-loop check.
• toIndex >= filters.size() compares int to size_t. Consistent with the pre-existing checks in this file and builds are clean, so noting only for consistency if you touch it per the point above.
• headManaged allocation: the comment at stack.cpp says the order is precomputed "so bindFilters stays allocation-free per pipe", but bindFilters now builds an unordered_set per pipe stack. lifecycleOrder is small — a linear std::find would beat the hash set here anyway. Either way, the comment overstates it.
• Kahn loop is O(N²) (stack.cpp): the inner scan restarts from the head of nodeOrder after each emit. Irrelevant at pipeline sizes and it runs once per endpoint, so fine — just flagging it's quadratic by construction, not by accident. A rolling start index would make it linear-ish if you ever care.
• if (inDegree[to] > 0) inDegree[to]--;: with deduped edges and seeded in-degrees this guard can't fire. It converts a would-be logic error into a silently wrong order. An assert (or nothing) says more.
• edges duplicates the information already in adj; the in-degree pass could walk adj directly and drop the extra vector.

Question

The mixed reachability node included after reachable parent test locks in a behavior change worth confirming: a node fed by both a source-reachable parent and a control/invoke node becomes head-managed, so its invoke node's self.instance.open()/close() no longer reaches it.

Concretely — a response node shared between the main pipeline and a tool_pipe sub-branch would stop being opened/closed per tool invocation. The tool_pipe README says to cap each branch with its own response node, so this is probably out of contract, but it's a wiring a user can produce on the canvas. Is that the intended trade-off? If so, worth a line in ROCKETRIDE_PIPELINE_RULES.md next to the new lifecycle guarantee.

Also: the ./builder test checkbox is still unchecked in the description.

Thanks for the thorough review. The two semantics checks you did (open
downstream-first, closing/close upstream-first) were exactly the right things
to verify.

Please note the PR grew after your review. You reviewed 411ec05d. Three
more commits are on the branch now:

• de913be1 — extend the topological lifecycle to control sub-pipelines
• 8eb14ffb — name nodes in the ownership error messages
• 4f9f7e4d — the fixes below, plus a bind-order test

This invalidates one of your conclusions. You wrote that tool_pipe and its
sub-pipeline keep their per-parent bindings. That is no longer true: extending
the same ordering guarantee into a control node's sub-pipeline is what the
new commits do. A re-review with that in mind would be appreciated.

Requested changes

1. Binder::getBound() is dead code — removed

You were right, and it was worse than a leftover: it was added by this PR
(7c9c9da8) with no caller, for a test that was never written. Declaration and
definition are both deleted.

2. Nothing tests the actual bind order — test added

New file: nodes/test/test_lifecycle_order.py. It runs a real pipeline — a
diamond of prompt nodes, which accumulate on write and emit on closing:

webhook -> branch_a -\
webhook -> branch_b --> join -> response

The join and its response are declared before the branches on purpose. A
dispatch that follows declaration order flushes the join first — the exact
failure you described.

Each branch reaches the join twice: once forwarded when it is written, once when
it flushes. So the join's merged question must hold 4 entries. If the join
flushes early, the branch flushes land on a closed node and the count drops.

No LLM, agent or mock is involved, so it is deterministic and runs in ~7s.

Three limitations, stated plainly:

• It covers the closing order only. open and close order produce no
  observable data here, so they are not asserted.
• It does not cover a control node's sub-pipeline (the tool_pipe region).
  Driving that needs an agent, which needs an LLM — too unstable for CI. That
  path is covered by manual runs and by the rejection examples in
  examples/incorrect/.
• I have not yet demonstrated the test failing against a deliberately broken
  build. The counts above come from measured behaviour, not from a red run.

Nits applied

• Bounds-check asymmetry — every lifecycle index is now range-checked once,
  up front, instead of inside one of the bind loops. This matters more than when
  you flagged it: the bind helper now runs once per region.
• int vs size_t — fixed as part of that change.
• "allocation-free per pipe" comment — corrected. It now says the order is
  computed once per endpoint, so errors surface at beginEndpoint.

Nits not applied, with reasons

• Kahn loop is O(N²) — agreed it is quadratic. It runs once per endpoint on
  graphs of a few dozen nodes. Restructuring a correct topological sort for no
  measurable gain seems like the wrong trade.
• edges duplicates adj — same reasoning. The algorithm works and is well
  covered; the tidy-up carries more risk than value.
• if (inDegree[to] > 0) inDegree[to]--; — kept. You are right that it
  cannot fire today. It costs nothing and prevents an int underflow if the
  dedup invariant ever changes.

Happy to revisit any of these three if you disagree.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/server/engine-lib/engLib/store/stack.cpp (1)

332-345: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject every data-fed control root with a sub-pipeline.

Lines 337-345 use ownerOf to infer whether the control root is data-fed, but ownerOf only records nodes already claimed by lifecycle regions. A sub-pipeline root fed by an unowned node is accepted, then receives a per-parent lifecycle binding in bindFilters while its descendants are also flat-bound from the invocation root, allowing duplicate lifecycle execution and dropped/duplicated flush output.

Track incoming data targets directly and reject region.first whenever it appears as a connection target.

The PR objective explicitly requires rejecting control roots that own a region and are also data-fed.

Proposed fix

+    std::unordered_set<int> dataFed;
+    for (const auto &conn : connections)
+        dataFed.insert(std::get<1>(conn));
+
     for (const auto &region : regions.control) {
-        auto it = ownerOf.find(region.first);
-        if (it != ownerOf.end())
+        if (dataFed.count(region.first))
             return APERR(Ec::InvalidParam, "Control node", nameOf(region.first),
-                         "drives a sub-pipeline and is also data-fed (owned by",
-                         nameOf(it->second),
-                         "); an invoked node with a sub-pipeline must not take a "
-                         "data input");
+                         "drives a sub-pipeline and is also data-fed; an "
+                         "invoked node with a sub-pipeline must not take a "
+                         "data input");
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/server/engine-lib/engLib/store/stack.cpp` around lines 332 - 345,
Update the control-root validation in the region-checking loop to use the
incoming data-connection targets rather than ownerOf, since unowned data-fed
nodes are currently missed. Track or query those targets directly and reject
every region.first that is a data connection target, while preserving the
existing InvalidParam error behavior and message context.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@packages/server/engine-lib/engLib/store/stack.cpp`:
- Around line 332-345: Update the control-root validation in the region-checking
loop to use the incoming data-connection targets rather than ownerOf, since
unowned data-fed nodes are currently missed. Track or query those targets
directly and reject every region.first that is a data connection target, while
preserving the existing InvalidParam error behavior and message context.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 34b3ff4e-5fa8-4ea3-bf6d-752a3be6d2e4

📥 Commits

Reviewing files that changed from the base of the PR and between 8eb14ff and 4f9f7e4.

📒 Files selected for processing (3)

• nodes/test/test_lifecycle_order.py
• packages/server/engine-lib/engLib/store/core/endpoint/endpoint.pipes.cpp
• packages/server/engine-lib/engLib/store/stack.cpp

[dsapandora]

solid work — the flat-per-region binding is the right model, and I verified it end to end: Binder::callMethods dispatches methodMap[lane] in bind order, and IServiceFilterInstance::open/closing/close are pre-order, so with region-owned nodes no longer bound per-parent there's no double dispatch. The controls.clear() in buildConnections and the try/finally in tool_pipe are nice catches too.