Skip to content

Never forward an agent command to the node it is already on, and split the bool - #4184

Merged
jeremydmiller merged 1 commit into
mainfrom
CW-1171-no-self-forward
Aug 29, 2026
Merged

Never forward an agent command to the node it is already on, and split the bool#4184
jeremydmiller merged 1 commit into
mainfrom
CW-1171-no-self-forward

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

The defect

InvokeOnAgentOrForwardAsync asks two different sources about the same fact:

question source
"run it here?" the in-process NodeController.Agents dictionary, via AllRunningAgentUris()
"forward it where?" the durable wolverine_nodes table

Nothing handled them disagreeing. During a startup window the table is already populated while the
dictionary is still empty — so on a single-node service the envelope was sent to
node.ControlUri, which is this node, where it took the same branch again. The method returned
true for that, and every caller reads true as "done".

FanOutToAllNodes, ten lines below in the same file, has always excluded self.

How it was found

Reported downstream as CritterWatch#1171:
four intermittent tests across two suites shared one sentence — a projection command reaches the
monitored service, is accepted, and the daemon does not act
.

It read as a daemon defect for weeks, because the agent-URI resolution that precedes this call
goes through the store's shard registry — so it returns a URI happily either way and emits no
failure ack. The command was acked as successful and nothing happened.

Proven by sampling both sources every 250 ms across 10 isolated runs: 2 failed, and exactly one
variable separated them — the agent was running on all 8 passes and on neither failure. An
end-state dump could not see it: a rewind that never reset progress and one that reset and caught
back up land on the same number, so only a trajectory tells them apart.

The change

1. A node that is THIS node is not a forwarding destination. Log it and report it, so the caller
can fall back or fail honestly instead of being told the work is done.

2. One bool cannot mean both "executed" and "forwarded". The caller cannot tell whether the work
is finished or merely handed off, and the two failure modes are not the same failure. New
InvokeOnAgentAsync returns an AgentInvocationOutcome:

outcome meaning
ExecutedLocally the action ran here; the work is done
Forwarded another node owns it and was sent the message; not done yet
NoOwner no node claims the agent
NotRunningLocally the table says this node owns it, but it is not running here

InvokeOnAgentOrForwardAsync keeps its bool signature and is now defined over the new method,
so no existing caller has to change. It answers false for the self case, which is the behavioural
fix — downstream, that turns a rebuild that silently skipped its working fallback into one that takes
it, and a rewind that acked success into one that acks a failure naming the cause.

Also replaces the null-forgiving node!.ControlUri! with a real check: an owner with no control
endpoint reports NoOwner rather than throwing a NullReferenceException from inside the routing.

Verification

Five new tests in InvokeOnAgentOrForwardAsyncTests.cs. Falsified rather than merely green
reverting the self check reds 2 of the 5. CoreTests agent suite is 358/358 on net9.0 and
net10.0
.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EVQ4LcDmK6cmvnY792tUEm

…t the bool

`InvokeOnAgentOrForwardAsync` asks two different sources about the same fact:

  * "run it here?"      -> the in-process NodeController.Agents dictionary,
                           via AllRunningAgentUris()
  * "forward it where?" -> the durable wolverine_nodes table

Nothing handled them disagreeing. During a startup window the table is already
populated while the dictionary is still empty, so on a single-node service the
envelope was sent to node.ControlUri — this node — where it took the same branch
again. The method returned `true` for that, and every caller reads `true` as
"done". `FanOutToAllNodes`, ten lines below, has always excluded self.

Reported downstream as CritterWatch#1171: four intermittent tests across two
suites shared one sentence — a projection command reaches the monitored service,
is accepted, and the daemon does not act. It read as a daemon defect for weeks,
because the agent-URI resolution that precedes this goes through the store's
shard REGISTRY and answers happily either way, emitting no failure.

Proven by sampling both sources every 250 ms across 10 isolated runs: 2 failed,
and exactly one variable separated them — the agent was running on all 8 passes
and on neither failure.

Two changes:

  * a node that is THIS node is not a forwarding destination. Log and report it
    instead, so a caller can fall back or fail honestly.
  * one bool cannot mean both "executed" and "forwarded" — the caller cannot tell
    whether the work is done. `InvokeOnAgentAsync` returns an
    `AgentInvocationOutcome`: ExecutedLocally, Forwarded, NoOwner, or
    NotRunningLocally. `InvokeOnAgentOrForwardAsync` keeps its bool signature and
    is now defined over it, so no caller has to change; it answers `false` for
    the self case, which is the behavioural fix.

Also replaces the null-forgiving `node!.ControlUri!` with a real check: an owner
with no control endpoint reports NoOwner rather than throwing a
NullReferenceException from inside the routing.

Falsified: reverting the self check reds 2 of the 5 new tests. CoreTests agent
suite is 358/358 on net9.0 and net10.0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EVQ4LcDmK6cmvnY792tUEm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant