Summary
Follow-up to PR #454, which migrated the executor's run-scoped state from a threading.local slot to a per-instance contextvars.ContextVar (_run_scope) to isolate state per asyncio task. The migration is correct and shipped with a regression test; this issue tracks four minor, non-blocking polish items surfaced by the audit pass. None affects correctness — all current tests pass.
Items
-
Per-instance ContextVar is never garbage-collected (chainweaver/executor.py). The run-state var is created per FlowExecutor with id(self) in its name. Python never GCs ContextVar objects, so an app that spawns thousands of short-lived executors slowly grows the contextvar registry. Fine for the common long-lived-executor case (already noted in a code comment). Decide whether to (a) keep + keep the documented trade-off, or (b) switch to a single module-level ContextVar holding per-executor state keyed by a WeakKeyDictionary.
-
Redundant composition save/restore (chainweaver/executor.py, ~the sub-flow recursion in _invoke_subflow/composition path). The manual saved_version = self._local.active_flow_version / restore around the recursive execute_flow call is now redundant: execute_flow runs the sub-flow in its own _run_scope, which already restores the parent's active_flow_version on exit. It is currently kept as documented belt-and-suspenders; it can be removed for clarity if preferred.
-
apply_output_mapping read-only contract (chainweaver/_execution/context.py). The no-mapping fast path now returns the input dict by reference (no defensive copy) for hot-path efficiency. The read-only contract is documented and every internal caller complies (feeds straight into context.update / iteration). Consider a small test asserting object identity on the no-mapping path to lock the contract in.
-
Flow.dynamic_params is declarative-only (chainweaver/flow.py). The executor accepts any dynamic_params keys at runtime regardless of what Flow.dynamic_params / DAGFlow.dynamic_params declare (intentional metadata for hosts/export adapters, per the field docstring). Consider an optional strict mode that warns/errors on undeclared injected params, or emphasise the declarative-only semantics in the docs.
Acceptance criteria
- Each item is either actioned or explicitly closed as "won't do" with a one-line rationale.
- No behavior change to the deterministic execution path unless item 1(b) or item 4 is deliberately chosen; in that case it ships with tests.
- All four validation commands stay green (
ruff check, ruff format --check, mypy, pytest).
Notes
Pure tech-debt/polish — safe to defer. Relates to PR #454 and issue #336 (run-scoped state).
Summary
Follow-up to PR #454, which migrated the executor's run-scoped state from a
threading.localslot to a per-instancecontextvars.ContextVar(_run_scope) to isolate state per asyncio task. The migration is correct and shipped with a regression test; this issue tracks four minor, non-blocking polish items surfaced by the audit pass. None affects correctness — all current tests pass.Items
Per-instance
ContextVaris never garbage-collected (chainweaver/executor.py). The run-state var is created perFlowExecutorwithid(self)in its name. Python never GCsContextVarobjects, so an app that spawns thousands of short-lived executors slowly grows the contextvar registry. Fine for the common long-lived-executor case (already noted in a code comment). Decide whether to (a) keep + keep the documented trade-off, or (b) switch to a single module-levelContextVarholding per-executor state keyed by aWeakKeyDictionary.Redundant composition save/restore (
chainweaver/executor.py, ~the sub-flow recursion in_invoke_subflow/composition path). The manualsaved_version = self._local.active_flow_version/ restore around the recursiveexecute_flowcall is now redundant:execute_flowruns the sub-flow in its own_run_scope, which already restores the parent'sactive_flow_versionon exit. It is currently kept as documented belt-and-suspenders; it can be removed for clarity if preferred.apply_output_mappingread-only contract (chainweaver/_execution/context.py). The no-mapping fast path now returns the inputdictby reference (no defensive copy) for hot-path efficiency. The read-only contract is documented and every internal caller complies (feeds straight intocontext.update/ iteration). Consider a small test asserting object identity on the no-mapping path to lock the contract in.Flow.dynamic_paramsis declarative-only (chainweaver/flow.py). The executor accepts anydynamic_paramskeys at runtime regardless of whatFlow.dynamic_params/DAGFlow.dynamic_paramsdeclare (intentional metadata for hosts/export adapters, per the field docstring). Consider an optional strict mode that warns/errors on undeclared injected params, or emphasise the declarative-only semantics in the docs.Acceptance criteria
ruff check,ruff format --check,mypy,pytest).Notes
Pure tech-debt/polish — safe to defer. Relates to PR #454 and issue #336 (run-scoped state).