refactor(exe_types): thread the engine into the node and parameter types - #5506
Merged
cjkindel merged 35 commits intoSep 21, 2026
Merged
Conversation
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
from
September 8, 2026 17:56
07aebc9 to
9e605eb
Compare
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
from
September 8, 2026 18:24
9e605eb to
9a59b38
Compare
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
from
September 8, 2026 18:37
9a59b38 to
e0274a4
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
from
September 8, 2026 18:47
e0274a4 to
db19575
Compare
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
from
September 9, 2026 15:18
db19575 to
23a2a42
Compare
collindutter
approved these changes
Sep 9, 2026
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
2 times, most recently
from
September 9, 2026 21:04
8ce0edd to
9a77f42
Compare
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
from
September 9, 2026 21:24
9a77f42 to
d9ba18e
Compare
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
from
September 11, 2026 15:50
d9ba18e to
7bf0cc1
Compare
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
from
September 18, 2026 18:37
7bf0cc1 to
cfa2158
Compare
Node machinery reached its collaborators through the process-wide GriptapeNodes facade, which the architecture reserves for callers that cannot be handed a reference: saved workflow files, separately-versioned libraries, and process entry points. exe_types is none of those, and pyproject's TID251 allowlist named it as work to be done. That entry is now deleted. BaseNode takes an optional keyword-only engine and falls back to the ambient one, so a library's `super().__init__(name, metadata=metadata)` is unaffected. Nothing in the engine passes it today -- LibraryRegistry.create_node cannot without breaking every node subclass's two-arg signature -- so nodes resolve the ambient engine; what the migration buys is the classification it forced, not per-node isolation. Calls were split by what they ask for: - questions about workflow state became requests (ListConnectionsForNode, GetConfigValue, GetSecretValue), because the orchestrator is the only process that holds all of it and a request is what reaches it from a worker - DAG machinery and event dispatch became self.engine.*, the sanctioned engine-internal path Internal connection queries pass broadcast_result=False: they run on the hottest read path in the engine, and the default queued a GriptapeNodeEvent carrying the node's full connection list to the GUI per call. Objects that are not nodes reach the engine through the node they hold (TrackedParameterOutputValues, BaseElement). Module-level helpers and classmethods take an engine parameter: query_model_policy, three VariableResolver staticmethods, and two private classmethods on PublicArtifactUrlParameter, which also adopts the ConfigManager-injected storage-driver construction from #5406. ControlFlow's delegation shim landed separately on main (#5388) and is used as-is. Tests that patched the facade now patch or inject the engine, using the `engine` fixture from #5385.
…cross the wire A worker holds no authoritative state: the flow graph, connections, and node registry live in the orchestrator. So while a worker is executing a node, its requests forward there. The question is which ones must not, and the answer has to be derived rather than listed. **The default is forwarding.** It used to be an allowlist, which meant every newly added request type silently resolved against the worker's own managers -- a non-authoritative copy -- and every migration note telling authors to "use a request instead" was wrong for any type nobody remembered to add. Defaulting to forwarding makes that class of mistake impossible: the cost of forgetting is a round trip, not a wrong answer. **What stays local is derived from the cause.** Two independent reasons, each derived so a request added later is covered without anyone remembering this file: Filesystem work. The workspace is shared on disk, so the worker's own answer is already the authoritative one -- and forwarding a write corrupted it, because `content` is `str | bytes` and the wire form base64s bytes into a JSON string that cattrs resolves back to `str`. A worker's write landed corrupted with no error anywhere. Derived from `os_events` itself, so a filesystem request added later is covered by construction. `OpenAssociatedFileRequest` is the one exemption: it hands a path to the OS to open in the user's default application, and that side effect belongs where the user is. Anything that cannot survive serialization. A `MacroPath` wraps a `ParsedMacro` that will not serialize, so the send raises and the worker blocks until the forward times out. A field declared as a bare `type` has no cattrs structure hook, so the orchestrator's ingress raises. And two requests carry a live Python instance -- a ResourceType, a Callable -- which `json.dumps(default=str)` turns into a string, so the orchestrator registers a string and the worker, the process that needs it, registers nothing, with no error on either side. Matched on the declared annotation text rather than a resolved type, because these modules annotate under `from __future__ import annotations`. Per-file project reads stay local for the same reason as filesystem work: a worker has already adopted the orchestrator's project, and a project's base directory is shared on-disk state. Forwarding bought nothing and cost a round trip on a path as hot as writing sidecar metadata for every saved file. It also cost correctness -- `GetCurrentProjectResultSuccess` annotates `project_info` under TYPE_CHECKING, which cattrs cannot resolve from this module, so its NameError fallback passed a raw dict into the constructor and every sidecar write logged "'dict' object has no attribute 'project_base_dir'" and silently wrote nothing. A library reload dispatched by a worker stays local too. The orchestrator decides when libraries reload, and it is mid-reload already -- that is what sent the activation -- so forwarding would have the worker dictate to it. Worse, the orchestrator's pre-reload callback terminates the very worker that asked, mid-node, because `in_node_execution()` is a process-wide refcount rather than task-local. Two things this rests on. `exe_types` takes an injected engine and comes off the facade allowlist, so node base classes reach managers through their engine rather than the process-wide facade -- which is what lets a worker's `exe_types` resolve against its own engine. And a worker's media saves returned URLs on its own ephemeral port; they now point at the orchestrator's static server, so a saved workflow does not reopen with dead links after the worker is evicted. A worker-behavior suite covers the shapes above end to end, because each of them failed silently and a unit test asserting the routing table would have passed throughout. The suite includes a project save through ProjectFileDestination, which composes the situation lookup, the macro resolution, the write, and the map back to macro form. It asserts the macro rather than the bytes alone: losing the mapping leaves the save working and every stored reference an absolute path from whichever machine ran the node. The guard message names requests as the way out, so it states what routing actually promises: a request reaches whichever process owns that state. Saying it is answered by the main engine was wrong for the filesystem requests it names, which this PR makes local-only. The event_converter diagnostic is gone from this PR. The fallback it reported on, and the result structuring that reaches it, both predate this stack -- so warning about them is neither a defect this change introduces nor a regression it causes. The fix that belongs here is keeping those project reads off the wire, which is the routing policy above. A detector for the fallback wants its own PR, with a test, and without a project type name hardcoded into a generic list. The static-server URL handover waits for initialization to decide it rather than sampling it. Spawning and URL resolution are both listeners on AppInitializationComplete, which fan out as unordered concurrent tasks, so sampling was a race with a warning as the consolation prize. The decision is a settled event covering every resolution path, including "no server here" (cloud storage) and a resolution that raised; the wait is bounded by seconds, not the startup grace, because resolution is a socket bind in the same fan-out. The local-only list carries a blurb per request and is grouped by the reason that BINDS each entry, because the reasons expire differently: loops, authoritative-here and live-object carriers are permanent, while wire-format blockers are contingent on serialization and say so. The derivation keeps covering requests added later, but its membership is now pinned by a test, so a new filesystem request fails that test and forces a routing decision rather than being routed silently. Its two module policies -- wholesale versus earned -- are named rather than fused into one boolean. Each entry in the local-only list carries a one-or-two sentence reason, grouped by the cause that binds it: belongs-to-this-process, authoritative-here, undeliverable-today, or carries a live object. Only the third expires, so it says so. The derivation still covers requests added later, but a test pins its membership, and the two module policies behind it are named rather than fused into one boolean.
A MacroPath is a ParsedMacro plus a variables dict, and a ParsedMacro is one string: `template` is its only init field, and `segments` carries `init=False` and is parsed from the template by `__post_init__`. So the template is the whole value and the parsed form is derived. Neither had a hook. cattrs has none for a NamedTuple in the JSON preset, so it handed the MacroPath straight back and never descended into the ParsedMacro inside it -- which put the failure in json.dumps rather than anywhere that names the type. Every request carrying one therefore could not be sent at all. Two hook pairs. ParsedMacro round-trips through its template, which the receiving side reparses. MacroPath goes to a dict of its two members, registered next to the class because event_converter cannot import project_events: project_events -> base_events -> event_converter. Payloads declare `parsed_macro: ParsedMacro` directly as well as via MacroPath, so that pair earns its place in the converter on its own. Routing is deliberately unchanged here. This only removes the reason the wire could not carry these. `test_a_macro_path_will_not_serialize` asserted the defect and is gone. In its place a sweep over every MacroPath carrier in the payload registry round-trips each one through JSON and checks the template, the variables, and that the segments were rebuilt -- so a carrier added later that does not survive the wire fails here rather than at a worker's forward timeout.
Routing is identical. What changes is the stated cause, which was wrong in both directions. `_carries_a_macro_path` and `_registers_a_python_class` read field annotations, so both said "the wire cannot deliver this". For the five artifact_events requests that was never the binding reason: the two registrations put a class into THIS process's provider registry, and the three preview requests resolve a provider back out of that registry while writing into the project's previews directory. Forwarding any of them would act in the orchestrator while the worker, the process that needs the provider to run a node, gets nothing. That holds however well they serialize. Reading annotations also made the routing move when serialization changed. The commit before this one gave MacroPath a hook; on the annotation rule the three preview requests would have started forwarding as a side effect, with no decision recorded anywhere. They are named now, so the set moves only when someone edits it. artifact_events is no longer swept, so the wholesale sweep is os_events alone, which is the one module where the module-wide reason is true throughout. `_annotation_text` and the two annotation readers are gone with it. Three tests replace the two that asserted the old cause: the named set is pinned by name, so dropping an entry fails rather than silently forwarding it; and everything else artifact_events defines is checked to forward, which is 6 of its 11 request types.
One rule, applied to every way a library or node can fail to run, plus the two pieces of state that have to survive a failure for that rule to mean anything. **A broken execution dependency.** It used to fail registration outright: no node types, and placeholder nodes reading "Library not found" in every workflow that used the library -- even though nothing but the edit-time set is needed to define, draw, and edit those nodes. An artist who could not run a node also could not open their workflow. The library now loads with real node classes and the reason recorded on it, so the refusal happens at execution and says why. `DependencyInstallationFailedProblem` already existed for this and was wired to nothing. **An unmet resource requirement.** A library can declare what it needs -- platform, arch, OS version, compute backend -- and an unmet declaration failed registration the same way. SAM3 declares cuda-only, which is why it could not be edited on a laptop at all. Now it is FLAWED rather than UNUSABLE, editable, and refused at execution. That refusal has to survive the worker spawn that follows it. `get_worker_for_library` refuses on the reason before it ever consults a worker, so an exec-dependencies library with a missing capability was resolving and downloading a whole execution environment -- torch, gigabytes -- to serve nothing. The spawn is skipped where the library's nodes already exist locally. A legacy worker-mode library has none, so skipping its spawn would leave it with no node types at all; for that one the spawn still happens, which is why clearing the reason on a fresh attempt is wrong. Clearing is right for an account of a PREVIOUS attempt -- an evicted worker, one that never started -- and wrong for a machine capability, because starting a worker does not give the machine a GPU. So the two conditions are separate: whether to skip the spawn turns on the nodes being local, whether to clear the reason turns on the requirement still being unmet. **A node whose run fails or is cancelled goes back to UNRESOLVED.** `state` was set to RESOLVING on dispatch and to RESOLVED on completion with nothing in between, so a node that raised -- or that was cancelled because a sibling raised -- kept RESOLVING after the run was over, and nothing else moves it. The editor spun on it forever. Given back before the run's maps are cleared, filtered to nodes still mid-flight: one that finished before a sibling failed holds real outputs downstream consumers may already have, and `make_node_unresolved` writes unconditionally. **Parameter structure derives from parameter values.** A node's structure is a function of its values, so it is stated as a contract, enforced on the way in, and honoured on reload rather than serialized alongside them. Without that, a worker rebuilding a node from a wire payload could disagree with the orchestrator about what parameters exist. What an evicted worker leaves behind is pinned per library kind, because the two kinds differ: an exec-dependencies library keeps its real node classes and loses only the ability to run them, while a legacy worker-mode library's classes came from the worker and become stubs.
A library that picks its own compute device duplicates detection the engine can do once, and gets it wrong in a worker: framework probes run against whatever the process can see, which is not necessarily what the orchestrator decided this library should use. GetExecutionDeviceRequest answers it centrally, and BaseNode surfaces the answer, so a node branches on the engine's decision rather than its own probe. Detection failing reports cpu and logs why, because a node silently taking its CPU path on an accelerated machine is the worse outcome. Drops the execution-module declaration that was here. Deferred imports already cover keeping a node module importable on the orchestrator, and the one thing the declaration added -- letting a worker pre-import so a broken execution environment fails at library load rather than mid-run -- is not worth a manifest field, an engine accessor, a boundary check, and an authoring rule to teach. Reaching heavy code with a deferred import of the module holding it is ordinary Python and needs nothing from the engine.
CorridorKey's OCIO path reaches into the OpenEXR library. The orchestrator loaded it, the worker did
not, so selecting an OCIO colour space succeeded on the canvas and failed on run -- the exact class of
divergence the worker boundary exists to eliminate.
A worker loads only the libraries it serves, and that target list was built from the spawn request
alone. It now expands transitively through each target's declared library dependencies. Read from the
discovered manifests rather than from LibraryRegistry, so this is usable before anything has loaded.
The declaration names a REPO while the registry is keyed by library NAME:
`griptape-nodes-library-openexr` publishes itself as `OpenEXR Library`, so matching the repo name
against library names missed every library not named after its repo -- silently, since a miss only
warns and skips. Provisioning installs each download under a repo-name directory, so the path is where
the repo name actually appears.
One normalization for both callers. A declaration URL may carry an `@ref` suffix and a `.git`
extension, and both change the final path segment; the transitive resolver and the target expansion
normalized differently, so a pinned declaration resolved for one and missed for the other. Two
adjacent silences went with it: a manifest that could not be read dropped every dependency it declared
with no log at all, and the install-path fallback answered with whichever entry matched first, so a
FAILURE record beside the copy that loaded reported "not installed here" for a library that is.
The worker-reach-into-orchestrator warning is retired here, because it was about this same boundary and
was measuring it wrong. It fired on every request a worker forwarded during node execution, to warn
that fetching orchestrator-owned state per call costs a round trip and returns a view stale on arrival.
That concern is real; the rule was not measuring it. Almost everything it fired on is engine machinery
the author never wrote and cannot avoid -- `get_parameter_value` issuing `ListConnectionsForNodeRequest`
per parameter is the loudest, and it must keep forwarding, because connections are authoritative
orchestrator state. Its remediation then told you to ignore it, offering a case ("an intentional write,
e.g. publishing a parameter value") that goes through `put_event` and never reaches this code at all. A
rule that fires constantly on unavoidable machinery and hands you an excuse teaches people to ignore the
category, which costs more than the round trips it pointed at.
Requests remain the sanctioned boundary -- the engine forbids manager access during worker execution and
directs authors here -- so the guidance moves to the docs, stated once, where it is not competing with a
warning that cries wolf. Scoping the rule to library call sites was the alternative; it needs a stack
walk on every forwarded request during execution, paying a hot-path cost to warn about hot-path costs.
Forwarding behavior is unchanged.
Author-facing docs stop teaching the pattern the guard refuses. Six places told node authors to read
secrets or connections through a manager accessor, which raises once a node executes in a worker:
three in getting_started, one each in authoring_libraries and error_handling, and the connection
utilities in examples. They use the corresponding request instead. The param_components README keeps
its before/after example, annotated, since the component it advertises already uses the request.
Three sites read a declaration URL, and the registration path still derived its pieces inline. That is the duplication the shared helper was added for: a `@ref` suffix and a `.git` extension both change the final path segment, so a site that normalizes differently resolves a declaration the others miss, and a miss only logs. The helper returned the repo name alone, which the registration path could not use -- it also needs the normalized URL and the ref to download a missing dependency. Widened to return all three, so every caller shares one interpretation instead of two of them sharing part of it. Covers the ref and normalized URL as well as the repo name: the download reads those two, and losing the ref installs the default branch instead of the declared one while still reporting success.
A worker loads its library's declared dependencies, so their execution pins have to be importable here too. They were not: the expanded targets reached the load loop through a local, while the gates that install and splice execution environments read the instance field, so a dependency loaded without the pins its own nodes need. Rather than build each dependency its own .venv-exec and splice them all, the dependency's execution set joins the depending library's single resolution. Splicing several independently resolved environments would reproduce between libraries the disagreement the combined edit/exec resolution already avoids within one -- two resolutions can pick different versions of anything they share, and whichever landed first would win -- and would have one worker writing a venv another library owns. It also leaves the target predicate, and the isolation it enforces, untouched. The combined set drives every decision, not just the install: a library declaring no execution dependencies of its own would otherwise retire the environment its dependency's pins were about to go into, since retirement deliberately runs ahead of every gate. The two walks over dependency declarations are now one generator, so the target expansion and the execution-set collection cannot disagree about the same manifest.
The existing tests patch the collection helper, so a break BETWEEN collection and install passed all of them: reading the wrong library's declarations, failing to resolve a repo name, or dropping the combined set before the install would each have gone unnoticed. Drives the real path instead, with two discovered manifests and nothing patched between the request and the installer. Verified by reverting the install list to the library's own set, which fails it. Roots now arrive at the declaration walk with their manifest already read. `install_library_dependencies_request` holds the schema it just loaded, so collecting from a name meant reading that file a second time for every library, in both processes, on the boot path. Also drops the worker note from the README's "Before" example, which is the pattern the page tells you not to copy, and states it once after the "After" example as the reason the component works under worker execution.
… it changes
A worker only produces the same result as the orchestrator if it resolves everything the same way, and
several separate things decide that: which packages it imports, which project it holds, which workflow
it is inside, and whether it hears about a switch. Each was wrong in a different way, and each failed
silently.
**Execution dependencies reach it as PYTHONPATH.** Splicing `.venv-exec` onto a running interpreter
cannot give a library its own versions: a module already in sys.modules is never reconsidered, and a
package that probed for an optional dependency at import time has cached the answer -- so a library
shipping `safetensors` still hit `NameError` from inside huggingface_hub. PYTHONPATH is on sys.path
before the process imports anything. That requires the directory to exist before the worker starts, so
the ORCHESTRATOR builds it: it never imports from there, and the worker receiving the directory cannot
be the process that creates it. Prepended rather than assigned, because a launcher-set PYTHONPATH is
part of the environment the engine itself booted with.
**The project comes across at registration.** Registration is the one moment both processes are
guaranteed to exist, so the reply carries the project the orchestrator has committed and the worker
adopts it BEFORE loading a library. Libraries resolve against the workspace and the workspace follows
the project, so a worker that loaded first would resolve them against a different tree. Awaiting a
reply the protocol already defines makes that ordering structural rather than a race two messages
usually win.
**The workflow context is lent, not derived from.** `push_workflow` has no worker-side caller, so a
worker answered "there isn't one" to every workflow question: `workflow_dir` raised, the default
`{outputs}` situation marks the reference optional, the raise was swallowed, and the path degraded to
workspace-relative. The worker wrote real files where the orchestrator does not read, with no error --
invisible in every earlier check because the workflow sat at the workspace root, where the two paths
coincide. Lending the CONTEXT rather than any value derived from it is what stops this needing a new
handoff per value. An execution carrying no context clears the mirror, or the last workflow keeps
resolving paths for the next node.
**A switch reaches every worker, in order, before it reports success.** The fan-out was
fire-and-forget, so the switch returned while a worker was still on the old workspace. Waiting serially
and unbounded was worse: adoption runs a full library reload, so one slow worker held the rest from
even receiving the request, and `route_to_worker` argues liveness from the heartbeat, which says
nothing about a worker busy adopting. The asks run concurrently, each bounded by the startup grace.
And because a worker receives activations from two sources on different loops, they carry the
orchestrator's committed generation and only strictly newer ones are adopted -- the newest committed
switch wins regardless of arrival order.
**Routing waits for the worker to be usable.** A worker is routable the moment it registers, but
registration is deliberately early, so forwarding into that window fails node creation with "Library
not found". Execution waits on a per-library event set on every terminal outcome: loaded, spawn
refused, spawn died, worker evicted, no session available. Bounded anyway, because "every" is a claim
about code that keeps changing.
Project switching is also exposed headlessly, which is what makes any of the above testable without a
human clicking. That moves `ProjectID` to a runtime import so cattrs can structure it, which also
changes `ExportProjectRequest.destination_path` from `str` to `Path`, along with the coercion on
`ImportProjectResultSuccess.project_file_path` and two `validation` fields. All four round-trip; the
export path is the one to look at.
Building the execution environment moves behind a gate on the heavy set, so retiring one the
manifest no longer declares moves with it: the removal now sits beside the build rather than inside
_install_dependency_set, which this gate no longer reaches for a library declaring none.
The removal is wrapped: it runs on the registration path, so a directory that will not delete --
held open by a running worker, or read-only on Windows -- would otherwise cost the library its node
types over a leftover. Leaving it costs execution correctness, which is the cheaper failure.
Comment pass over this PR's additions: what survives is the constraint or the reason an obvious alternative is wrong, and what goes is incident history, restatement of the line below, and forward references to code that already explains itself. Eight blocks, none shorter than six lines, now four or fewer. The ProjectID alias keeps its justification, rephrased: pydantic resolves those annotations at runtime, so the import cannot be TYPE_CHECKING-only, and events are the import leaf. SetCurrentProjectRequest, GetCurrentProjectRequest and LoadProjectTemplateRequest come off the MCP server's supported set. They were added for a headless client to drive a project switch, but nothing drives one: no unit coverage, and no e2e harness uses them. Their imports go with them.
A worker could be told which project to run against by two different senders: the registration reply and the switch fan-out. Ordering them meant a generation counter on the wire and a lock on the worker, and the generation had leaked onto CurrentProjectChanged -- protocol bookkeeping in a domain event that clients also consume. Registration now sends the activation instead of answering with the project, down the same channel a mid-session switch uses. One sender, one adoption path, and a switch landing mid-registration is just a second message rather than a second source. The reply carries no project, and the worker waits for the activation to settle before it loads any library. The fan-out reads the committed pair rather than taking the id off the event, so the id and the generation describing it come from one read. Two switches in quick succession therefore both fan out the newest committed state and a worker cannot be told to go backwards. The lock stays, and its comment was wrong twice over: the two adoptions are both on the transport loop, not different ones, and one sender does not remove the need for it -- activation awaits internally, so two that arrive close together interleave regardless of who sent them.
These arrived on the branch below, where the worker installed its own execution environment. Here the orchestrator builds it -- the worker receives that directory as PYTHONPATH and so cannot be the process that creates it -- so they were asserting against a path this branch removes. The behaviour under test is unchanged: a declared dependency's execution pins resolve alongside the depending library's own, in one resolution. Only which process does it, and which call to watch, moved.
The test arrives from the branch below, where routing waited on wait_for_worker_library_load. Here it waits on wait_for_worker_ready, so the mock has to match or the await fails before the case under test is reached.
…an event Backgrounding the `.venv-exec` build to keep a torch install off the startup path bought a coordination layer whose only job was to make an install that had not finished look like one that had: a cancel-and-replace protocol, a task registry keyed by venv directory, a readiness event per build, and a spawn that waited on it. The edit-time install next to it was awaited all along, which is the answer to why only the execution set needed any of this. Awaited now, like its neighbour. That deletes _replace_execution_env_build, _build_execution_env, _execution_env_builds, execution_env_ready and wait_for_execution_env, and the spawn path simply reads the recorded reason instead of waiting first. Holding startup is the honest behaviour; installing less often is how to make it cheap. The invented library name went with it -- a failed build no longer needs a name to key an event on, so nothing falls back to "unknown".
Dropping the worker-side sys.path splice left `test_serializable_outputs_are_unaffected` on a premise this branch removed. It set `_is_worker` BEFORE registering, because that was how the execution venv reached sys.path; with the splice gone, worker mode installs nothing at all, so the node module could not import even its edit-time dependency and the library loaded zero nodes. Registration failed, and the guardrail the test is named for never ran. The flag now flips after registering, as the sibling guardrail test above it already does: loading a library is the orchestrator's job, and the flag only selects the execution path under test. Both dependency sets are declared edit-time so this one process can import both, because a real worker receives the execution set as PYTHONPATH from the spawn and no single-process test has that. The two environments being separate on disk is pinned in test_library_execution_dependency_split.py and is not what this test is about; the execution set stays declared so the library is still worker-routed and the guardrail applies at all. Caught by CI on a PR stacked on this branch. This branch's own Tests runs were cancelled, so no completed e2e run had reached the assertion, and it fails on Linux, Windows and macOS alike.
…ds it Building the environment before returning rather than behind an event left this test asserting the opposite of what now happens: `.venv-exec` exists by the time registration answers, so `assert not exec_venv.exists()` fails, and the comment explaining the absence described a scheduled build that no longer exists. Inverting the assertion is not enough to make it worth having. Nothing anywhere proved the execution environment gets built -- every assertion about it was a negative one, and the test that needs an existing environment mkdir's a fake. So this now asserts the directory exists AND that the heavy dependency is installed inside it, which is the mechanism the whole split rests on: the worker receives that directory as PYTHONPATH and cannot build it for itself. What the test was really pinning is untouched and still asserted: the execution set stays absent from the orchestrator's sys.path, and importing it here still raises. The name said "does not install execution dependencies at all", which was never true of the directory -- only of this process's import path -- so it now says which of the two it means.
`worker_ready` said how a library came to be executable rather than what a caller wants to know, which is whether it may route execution there at all. Now `library_ready` / `wait_for_library_ready`, and the docstring stops assuming a separate process: a library that runs in this one is trivially ready. `project_activation_settled` becomes `worker_settled`. It gates one thing today -- the project the orchestrator sent -- but the name is the barrier rather than its current contents, so a second piece of startup state gates this same event instead of adding another for the caller to wait on. Also records #266 against the generation counters and the adoption lock, which is where a reader meets them.
LibraryInfo carried two fields that answered questions about a PROCESS -- is a worker coming, has it loaded the library, did it die -- and WorkerManager wrote them. A record with one owner had two writers: `_start_workers` cleared `execution_unavailable_reason` before a spawn while `_refuse_spawn` wrote it, and `library_ready` was created by one manager and set by the other. Both move to WorkerManager, keyed by library name, alongside the maps it already keeps. Callers ask rather than reading fields: `wait_until_executable` replaces `wait_for_library_ready`, and `worker_unavailable_reason` replaces the field read. Boot asks `wait_for_libraries` for a collective ceiling instead of gathering events it was handed. The other direction goes too. WorkerManager no longer reads LibraryManager to decide whether to refuse a spawn: a library whose execution environment failed to build is simply never asked for a worker, decided where the build happened and the result is already known. What stays on LibraryManager is what it knows on its own -- a declared resource the machine lacks, an environment that would not build -- and `get_worker_for_library` composes both owners' answers for the one message a caller sees.
The reason and the gate are keyed by a bare library name, so they outlived the record they describe: an evicted worker's reason survived a reload that dropped the library's execution dependencies, and get_worker_for_library read it for every library rather than only worker-backed ones. Both fixed -- forget_library on unload, and the read gated on executes_in_worker. is_execution_available answered "settled", which is not the same as available; renamed has_settled. _refuse_spawn had become a second name for note_worker_unavailable, and the worker_manager None guards documented an optionality the annotations deny -- the None came from a test mock describing a shape the engine cannot be in. Restores the eviction-reason assertion that was deleted rather than moved, covers forget_library, and drops mocks for a method that no longer exists.
A manifest can declare legacy worker mode AND execution dependencies, and nothing rejects the pair:
`_resolve_executes_in_worker` returns True for either reason and no validator looks at the
combination. For that library the orchestrator took the WORKER_DELEGATED branch, which skipped
`install_library_dependencies_request` entirely, and inside the worker the execution install is
gated on `not self._is_worker`. So neither process built `.venv-exec`.
Nothing downstream caught it. No install ran, so no failure was recorded, so
`execution_env_failure_reason` stayed None and the spawn was not refused;
`execution_site_packages` returned None because the directory was absent, and the worker started
with no PYTHONPATH -- the raw ModuleNotFoundError at node-module import that the spawn refusal was
added to prevent, reached from the one direction the refusal cannot see.
Skipping the LOAD is not the same as skipping the environment. The orchestrator has to build
`.venv-exec` whoever loads the library, because the worker receives that directory as PYTHONPATH
and so cannot be the process that creates it.
The call now happens for both branches and only the resulting lifecycle state differs. What the
orchestrator must NOT do for such a library is build the edit-time venv, and
`_this_process_owns_the_edit_venv` is where that belongs: it returns True for every library on the
orchestrator, which its own docstring already contradicts for the worker-mode case ("nobody else
creates `.venv` and the worker must"). It reads the library's `requires_worker` now, so the
edit-time gate declines and the execution gate proceeds. An exec-install failure is recorded rather
than returned, so routing a delegated library through here cannot fail its registration.
A mutation check rather than a reading: reverting the ownership line to `return True` turns two of
the three new tests red.
Both TODOs carried a full URL into griptape-ai/internal, whose visibility is INTERNAL, so for anyone reading this repo from outside the org the link is a 404 dressed up as an explanation. `os_manager.py` and `path_utils.py` already cite the same tracker as `griptape-ai/internal#178`, which reads as a reference rather than promising a page. Matching that: the reasoning those TODOs need is in the prose above them either way.
The execution set used to be spliced after the edit-time one, so it landed at `sys.path[0]` and won for any package in both. It now arrives as PYTHONPATH instead, which Python places at `sys.path[1]`, and `_add_library_edit_venv_to_sys_path` then inserts the edit-time site-packages at 0 -- ahead of it. The precedence inverted. That matters because the two directories can hold different versions of the same package. The edit-time install resolves `pip_dependencies` alone; the execution install resolves `[*pip_dependencies, *execution_dependencies]`, and adding a heavy pin is exactly what makes the combined resolver choose differently for something shared. So `process()` ran against the version the execution resolver rejected -- the disagreement the single combined resolution exists to prevent. The splice is skipped when this library's execution site-packages is already on `sys.path`, which is true only in the worker the library was spawned for. That environment is resolved over both sets, so it provides everything the edit-time one does. A library the worker was NOT spawned for is unaffected: its execution directory is not on the path, so its edit-time venv is still spliced, and so is every library's in the orchestrator. Checked by construction rather than by reading: with the execution directory on the path a later `insert(0, edit_site_packages)` resolves a package in both to the edit-time copy. Nothing covered this function, which is why the inversion was invisible -- the e2e suite uses one edit-time and one execution wheel with no overlap, so no test had a package in both. Two tests now: the execution directory stays first when it is present, and the edit-time venv is still spliced when it is not. The caller's docstring carries the invariant again. It said "goes on FIRST, so it wins for anything present in both" before this stack, and losing that sentence is what let the inversion read as tidying.
cjkindel
force-pushed
the
refactor/thread-the-engine-into-exe-types
branch
from
September 21, 2026 19:20
cfa2158 to
5cfd901
Compare
`spawn_worker` checked `_managed_worker_processes` for a duplicate, then wrote its entry 68 lines later, with the static-URL await and the fork in between. Two spawns for one library both got past the check and both forked; only one could be recorded, leaving the other's process untracked and holding that library's dependencies in memory until its own heartbeat lapsed. Two `StartWorkerRequest`s for one library name overlap whenever `_library_file_path_to_info` carries duplicate entries for it (a FAILURE entry alongside the LOADED copy), or when `_start_workers` runs twice before the first worker registers. The key is now claimed in the same step as the check, and released in a `finally` so a failed fork does not leave the library permanently unspawnable.
`_log_spawn_error` asked `task.exception()` first, which raises `CancelledError` on a cancelled task. Raising from a done-callback surfaces as loop-level "Exception in callback" noise and skips the `note_worker_unavailable` below it, so nothing records that no worker is coming. Cancellation reaches this callback at loop teardown, where nothing is waiting on the worker, so the cost today is the noise rather than a stalled run. Note on A1 of the tracking issue, which covers the same function: the claim that the spawn task can be garbage collected while parked does not hold. `Event.wait()` appends a future to the event's `_waiters`, and the awaiting task installs `Task.__wakeup` as that future's done-callback, so the task stays reachable from the manager for as long as it is parked. No change made there.
`route_to_worker` registers a request then awaits it. Every other exit from that await removes the entry: a response pops it in `_try_match`, eviction in `cancel_requests_by_tag`. Flow cancellation re-raised without removing anything, so the entry stayed in `_pending_requests` for the life of the process -- one per cancelled node execution. Each leaked entry keeps its worker's tag, so a later `cancel_requests_by_tag` walks and re-settles long-dead requests, and `pending_count`, the only visibility into that map, reports a number that never comes down. Nothing fails loudly; it degrades over a long editing session. `_cancel_request` did exactly the removal needed but was async, and awaiting inside an except block while a cancellation is being delivered adds a suspension point in the one place it is least wanted. Its body never awaited, so it becomes the public synchronous `discard_request` and its five internal callers drop their `await`.
`pending_count` and `pending_request_ids` read `_pending_requests` while holding nothing, and it is mutated from more than one loop: `_try_match` on the transport loop, `track_request` and `cancel_requests_by_tag` from whichever loop issued the request. `len()` only risks a stale answer, which is fine for a diagnostic. Building the id list iterates, so a concurrent insert or pop raises "dictionary changed size during iteration" -- a diagnostic that can raise is worse than one that answers a moment out of date. Both are diagnostics with no production caller today, so nothing holds the lock across a call into them.
`_orchestrator_static_server_base_url` picks between a direct read and a thread hop, and between two warnings when no URL arrives. Nothing exercised either choice: inverting the settled check left the suite green, and so did swapping the two warnings. Both choices matter to whoever has to act on them. Taking the hop when the answer is already in parks an uncancellable default-executor thread that teardown joins, and blaming the settle timeout for a resolution that raised points an operator at slow startup instead of the bind failure that actually happened. Five tests: the direct read, the hop with its timeout, each warning, and a cloud backend staying quiet because there its URLs outlive the worker anyway. Each fails if the branch it covers is inverted.
The static-URL tests set `static_server_base_url_settled`, which is a property on the real manager, so pyright rejected the assignment even though the object is a MagicMock. Casting once per test says what the object actually is instead of suppressing each line.
A regression in the spawn claim: `reset_workers` cleared the process registry but not the new claim set, so a spawn suspended across a library reload kept its claim and the reload's own spawn for that library was refused. The refusal records nothing (a worker is normally on its way when a key is claimed) so the next run would wait out the whole startup grace and then blame the library load. Cleared with the registry, with a test that fails without it. `note_worker_unavailable` is deliberately still NOT called from the duplicate guard: it records a reason and settles the library, which would falsely fail a library that already has, or is about to have, a worker. Comments that were wrong or that answered a reviewer are gone rather than reworded again: a claim that `list(dict.keys())` can raise "dictionary changed size during iteration" (it cannot, the whole build is one C loop under the GIL) and a `discard_request` docstring citing a suspension point that never existed. `test_an_undecided_url_is_waited_for_off_the_loop` asserted its own name only loosely; it now records the executing thread and fails if the blocking wait runs on the event loop.
`reset_workers` drops the claims so a reload can spawn its libraries again, which leaves a spawn suspended between its claim and the registry write resuming to find the key claimed by the reload's spawn. Releasing by name alone freed that one, so the library was admitted again while a spawn was genuinely in flight -- and a second fork leaves one of the two processes untracked, which is the leak the claim exists to prevent. Claims are now per attempt: the map holds a token identifying the holder, and the release is conditional on the stored token still being the one this attempt took. The test drives the interleaving: park a spawn, reset, let the reload claim the key, then release the stale spawn with its fork failing so no registry entry shadows a wrongly-freed claim. It fails against an unconditional release. The comment on the claim also drops its comparison with the guard this replaced and states the constraint instead: the registry entry is written only once the subprocess exists, and the work in between suspends.
cjkindel
added a commit
that referenced
this pull request
Sep 21, 2026
Worker v2 landed, and it and this change had overlapping opinions about the same thing. The guardrail goes. on_execute_node_request refused any serializable=False output in a worker, with a message telling the author to cache it library-side and ship a descriptor the consumer trades back -- a description of the workaround this change replaces with an engine-owned cache. It flagged by declaration rather than by value, so it also refused a token string that ships perfectly well. The one case the cache genuinely cannot take is a container, which has no single object to hold and nowhere to attach a release hook, and reaching that needs an attribute the ParameterList constructor refuses to accept. Not worth a guard, so the check and its helper are gone and its two tests now assert what the scenario has become: the object stays and a reference crosses. That is the feature's first coverage through the real worker path rather than a simulated boundary. The facade access was a genuine bug this guard caught, not over-reach. LocalObjectScope reached the store through GriptapeNodes.ResourceManager() with a comment saying BaseNode had no engine reference to hand down. That was true when written and went stale: #5506 added BaseNode.engine in this same stack, and its docstring says node machinery should use it precisely so the facade stays the surface for library code. Engine-internal code going through the facade trips a guard aimed at node authors -- the guard's own docstring names the storage driver doing this and calls it non-hypothetical. The scope now takes the node and reads its engine per call, following the node's own deferred resolution rather than pinning whichever engine was ambient. One choke point, and it fixes every worker execution. Also: this change's rename of _instances to _capability_instances had to be carried into worker v2's new device handler and its test, and the two teardown registrations moved out of register_broadcast_handlers, which they had pushed past its complexity limit.
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
2 participants
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.
Split out of #5409 at review request, same shape as your #5386. It is the
GriptapeNodes→ injected-engine migration forexe_typesand nothing else. Mostly mechanical, but not purely — three call sites change where the answer comes from, listed at the bottom.Why here
pyproject's TID251 allowlist namedexe_typesas work to be done. The facade is reserved for callers that cannot be handed a reference — saved workflow files, separately-versioned libraries, process entry points — and node machinery is none of those. That allowlist entry is now deleted.What it changes
BaseNodetakes an optional keyword-onlyengineand falls back to the ambient one, so a library'ssuper().__init__(name, metadata=metadata)is unaffected. Nothing in the engine passes it yet:LibraryRegistry.create_nodecan't without breaking every node subclass's two-arg signature. So nodes still resolve the ambient engine, and what this buys is the classification it forced, not per-node isolation.Calls were split by what they actually ask for:
ListConnectionsForNode,GetConfigValue,GetSecretValue). The orchestrator is the only process holding all of it, and a request is what reaches it from a worker.self.engine.*, the sanctioned engine-internal path.Objects that aren't nodes reach the engine through the node they hold (
TrackedParameterOutputValues,BaseElement). Module-level helpers and classmethods take an engine parameter:query_model_policy, threeVariableResolverstaticmethods, and two private classmethods onPublicArtifactUrlParameter.Where behaviour actually changes
The two deprecated
BaseNodeconfig helpers (get_config_value/set_config_value) now go throughGetConfigValueRequest/SetConfigValueRequestrather than reading a manager directly. In the engine that is the same answer; in a worker it is the orchestrator's answer instead of the worker's own copy, which is the point — the orchestrator is the process that holds config authoritatively.PublicArtifactUrlParameteradopts the ConfigManager-injected storage-driver construction from #5406.Internal connection queries pass
broadcast_result=False. They run on the hottest read path in the engine, and the default queued aGriptapeNodeEventcarrying the node's full connection list to the GUI on every call.Notes
ControlFlow's delegation shim landed separately on main (#5388) and is used as-is. Tests that patched the facade now patch or inject the engine, using theenginefixture from #5385.Full unit suite passes at this branch's tip (5863 passed).