execution_graph: stop a node's own writes from re-triggering it#90
Conversation
forest-rs#85 made host writes mark the written key dirty so prior readers are invalidated. But a node that both reads and writes the same key then invalidated *itself*: the read made its output depend on the key, the write re-dirtied it, and the node re-ran on every run_all() forever (observed run_count 1, 2, 3, ... with no external change). Track the keys a node writes during a run and exclude them from its own dependency set (reads ∖ writes). The write still marks the key dirty, so other nodes that read it are still invalidated; only the writer's self-trigger is removed, so a read-modify-write node now reaches a fixpoint. Also ignore host writes to graph-owned Input keys: they intern to the same id as a node's input binding, so honoring them would let the self-write filter strip a real graph-input dependency and leave the node stale after invalidate_input. Hosts only own the HostState/OpaqueHost namespaces. Adds regression tests for both cases.
| @@ -98,14 +101,14 @@ impl AccessSink for CollectingAccessSink<'_> { | |||
|
|
|||
| fn write(&mut self, key: ResourceKeyRef<'_>) { | |||
| self.counter.set(self.counter.get().saturating_add(1)); | |||
There was a problem hiding this comment.
An ignored ResourceKeyRef::Input writes still increment the strict-deps access counter before mark_tape_key_dirty(...) returns None.
This is also true of line 265 below.
That means a host call that only does ctx.record_write(ResourceKeyRef::Input("x")) is treated as having reported an access in strict-deps mode, even though the graph records no dependency, no invalidation, and no access-log entry for it. This weakens strict-deps: an unusable/ignored key can satisfy “this host call recorded an access.”
Fix direction: only increment the strict-deps counter for accepted writes, or make ignored input writes explicitly visible as a strict-deps violation. If non-strict mode should silently ignore these writes, strict mode should still reject “only ignored writes.”
I think just moving the increment inside the if statement below is fine and it preserves strict-reps semantics: ignored Input writes don't count as real access events.
Host writes to graph-owned Input keys are ignored (mark_tape_key_dirty returns None), but the access sinks still bumped the strict-deps counter before that rejection. As a result a host call whose only action was ctx.record_write(ResourceKeyRef::Input(..)) advanced the counter and passed strict-deps, even though the graph recorded no dependency, no invalidation, and no access-log entry — weakening the mode's guarantee that every host call reports something usable. Move the counter increment inside the accepted-write branch in both CollectingAccessSink::write and DepsOnlyAccessSink::write, so only accepted (HostState/OpaqueHost) writes count as access events. Reads are unchanged; non-strict mode still silently ignores Input writes (it never reads the counter), while strict mode now rejects a call whose only event is an ignored write. Adds a regression test asserting StrictDepsViolation for a host call that only writes an Input key.
2 participants
What
Fixes a non-termination bug introduced by #85 (host writes invalidate prior readers).
A node whose tape both reads and writes the same host-state key never reached a
fixpoint: the read made the node's output depend on the key, while the write
marked that same key dirty during the run — after the planning drain — so the
key stayed dirty and re-scheduled the node on the next pass.
run_all()with noexternal change re-ran it forever (run_count climbed 1, 2, 3, …).
How
After each run, a node's dependency set is now
reads ∖ writes: we collect thekeys the node wrote (a new
write_idsscratch buffer, symmetric with theexisting
read_ids) and exclude them from its own dependencies. The write stillcalls
mark_dirty, so other nodes that read the key are invalidated exactlyas before — only the writer's self-trigger is removed. A read-modify-write node
now converges after one run.
While here, host writes to graph-owned
Inputkeys are ignored: anInputwrite interns to the same id as a node's input-binding dependency, so honoring
it would let the self-write filter strip that binding and leave the node stale
after a later
invalidate_input. Inputs are graph-owned (set viaset_input_value/invalidate_input); hosts should only write theHostState/OpaqueHostnamespaces.Behavior note
A key a node both reads and writes is treated as an output it owns, not an
input — so an external invalidation of such a key won't re-run that node
either. This is documented in the
## Semanticsrustdoc. It's the onlynon-looping resolution: keeping the dependency to honor external invalidation
would re-introduce the infinite self-trigger.
Tests
node_that_reads_and_writes_same_key_reaches_fixpoint— run_count stays 1across repeated
run_all().host_write_to_input_key_does_not_drop_graph_input_dependency— a hostInput-key write no longer strips the node's
"x"binding;invalidate_inputstill reruns it.
host_write_invalidates_prior_readers_of_same_keystill passes(cross-node invalidation intact).
Follow-up (not in this PR)
The
Input-write guard is enforced locally in the graph's access sink. Thedeeper, compile-time fix is to give writes their own key type in
execution_tape(e.g. aWritableKeyRefwith onlyHostState/OpaqueHost)so writing an
Inputkey is unrepresentable inrecord_write/AccessSink::write. That's a cross-crate public-API change, better suited tothe pre-release API hardening tracked separately.