Skip to content

GH-4407: owner-scoped assignment deletes, claim-if-absent, and reconciliation-sweep hardening - #4408

Merged
jeremydmiller merged 1 commit into
mainfrom
gh-4407-assignment-divergence-followups
Sep 10, 2026
Merged

jeremydmiller merged 1 commit into
mainfrom
gh-4407-assignment-divergence-followups

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Closes #4407. These are the follow-ups from reviewing #4404, the node-side reconciliation sweep for #3987.

1. RavenDB and Cosmos DB no longer delete other nodes' assignment rows

On the SQL providers, RemoveAssignmentAsync only deletes a row the calling node owns (where id = @id and node_id = @node). RavenDB and Cosmos DB deleted by agent id alone. #4404's orphan-stop branch calls StopAgentAsync exactly when another node owns the row, so on those stores it deleted the owner's row. The leader then saw the agent as unassigned and placed it again, creating a new duplicate that the next sweep would stop, deleting that row too.

  • The orphan stop leaves assignment rows alone. StopAgentAsync(uri) keeps its behaviour; the sweep calls a private stopAgentAsync(uri, removeAssignment: false). This fixes the problem for every provider.
  • RavenDB and Cosmos DB deletes are now owner-scoped. RavenDB loads the row and deletes it only if the NodeId matches, with optimistic concurrency. Cosmos reads the row and deletes it only if the node matches, conditional on the ETag. A row that changes hands in between survives.

2. Restoring a claim no longer overwrites a newer one

Adds INodeAgentPersistence.TryClaimAssignmentAsync(nodeId, agentUri). It inserts a row only if no node owns the agent, and returns whether the row belongs to the caller afterwards. The sweep uses it instead of the last-writer-wins AddAssignmentAsync when it restores a claim for a running copy, so a peer that claimed the agent after the snapshot keeps it.

  • SQL providers: a race-safe insert-if-absent for each dialect: Postgres on conflict do nothing, SQL Server not exists under updlock, holdlock, MySQL on duplicate key as a no-op, Oracle merge ... when not matched (tolerating ORA-00001), SQLite insert or ignore. Each is followed by a count(*) on id and node_id to check ownership.
  • RavenDB: a create-only store (empty change vector), re-reading to see who won if it loses the race.
  • Cosmos DB: CreateItemAsync, reading on conflict.
  • Wrappers: MultiTenantedMessageStore forwards the new member explicitly, and NullMessageStore returns true.
  • Deliberately no default implementation. The multi-tenant store implements the interface explicitly and forwards every member. With a default, a wrapper that forwarded everything but this one would silently fall back to an upsert. Anyone implementing INodeAgentPersistence outside this repo will need to add the method.

3. A cap on reconciliation actions per health check

New DurabilitySettings.MaxLocalAgentReconciliationsPerTick, default 50; 0 or less means no cap. The sweep runs inside the serialized health check, before the leader's assignment pass on the leader node, so after something like a leader crash on a large fleet an unbounded sweep could hold that pass back. Deferred divergences keep their streak count and are handled on later ticks, oldest first.

4. Sweep starts respect stop revocations and starts still in progress

  • Revocation: the command sequence is captured before the health check reads its snapshot. The sweep starts agents through StartAgentGuardedAsync(uri, snapshotSequence), and skips restoring a claim if a stop has arrived since. A stop the snapshot couldn't have known about always wins.
  • Starts in progress: a _startingAgents set tracks them, the way _stoppingAgents tracks stops. The marker is set before the wedged-restart path deregisters the agent and released only after registration, so the sweep never sees a slow start as "assigned here but not running" and starts it again.

5. Compliance coverage for assignment ownership and divergence

NodePersistenceCompliance, run for every provider:

  • removing_an_assignment_owned_by_another_node_is_a_no_op, the regression test for item 1
  • add_assignment_moves_the_row_to_the_new_node, documenting last-writer-wins
  • claiming_an_unowned_assignment_takes_it
  • claiming_an_assignment_a_peer_owns_leaves_it_alone
  • claiming_an_assignment_this_node_already_owns_succeeds

LeadershipElectionCompliance, run against real hosts and real storage on every leadership suite. Each scenario fakes a divergence, then requires exactly one copy of every agent, and for the agent under test a row naming the node that runs it:

  • a duplicate copy on a node the table doesn't name
  • an agent assigned to a node but not running there
  • a running agent whose row was deleted
  • the leader dying while its start commands are still in flight

The scenarios deliberately don't assert which node ends up with the agent. The leader may legitimately re-place or rebalance it while the divergence heals, and in practice it does.

Also fixed: persist_and_load_node_records never advanced its retry counter, so on the SharedMemory variant it spun forever. That was a 40-minute hang locally; SlowTests only runs on manual dispatch, so CI never saw it. It now makes 10 attempts with a delay between them, and on SharedMemory it now fails fast rather than hanging (details under Verification).

Verification

  • dotnet build wolverine.slnx -c Release -f net9.0: 0 errors, 0 warnings
  • CoreTests Runtime.Agents: 386 of 386. New unit tests cover: the orphan stop never calls RemoveAssignmentAsync, restoring goes through the claim and never the upsert, a peer winning the claim race keeps the row, a start in progress is never started again, and the per-tick cap works through a backlog over later ticks.
  • NodePersistenceCompliance, all passing:
    • Postgres 18/18
    • SQL Server 18/18
    • MySQL 17/17
    • Oracle 17/17
    • SQLite 17/17
    • RavenDB 19/19
  • RavenDB negative control: with only the old delete-by-id RemoveAssignmentAsync swapped back in, removing_an_assignment_owned_by_another_node_is_a_no_op fails (1 of 1), and passes again once the fix is restored.
  • Leadership suites: all four new scenarios pass on MySQL, Oracle, Postgres, RavenDB (both variants), SQL Server, RabbitMQ and SharedMemory. The only failures were singular_agent_is_only_running_on_one (below) and SharedMemory's persist_and_load_node_records, which predates this PR and now fails fast instead of hanging.
  • singular_agent_is_only_running_on_one: a known flake in the leadership suites (see Register the event-subscription family aliases from the ancillary Marten path (GH-3438) #3451 for the CosmosDB occurrence). It failed with two copies in three suite runs on this branch, and in 1 of 5 isolated Oracle runs against 0 of 5 on the GH-3987: node-side reconciliation sweep for assigned-vs-running #4404 baseline. In every failing run the reconciliation sweep took no action at all, and every behaviour this PR changes on the SQL providers goes through a sweep action, so it isn't involved. Each failing log instead shows the leader re-placing a start that hadn't been confirmed yet (Node 1 confirmed 0 of 5 requested agents; ... remain unconfirmed), then simple:/// starting on two nodes. A larger interleaved A/B is running; its numbers will follow as a comment.
  • Not run locally: Cosmos DB (needs the emulator) and Azure Service Bus. CI covers both.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VDUrBeB4tTnKj4AExCS1nj

…ening

Follow-ups from reviewing #4404's node-side reconciliation sweep.

- RavenDB and Cosmos DB RemoveAssignmentAsync deleted by agent id alone,
  so the sweep's orphan stop -- which fires exactly when another node owns
  the row -- deleted the owner's row, the leader re-placed the agent, and
  the next sweep did it again. The orphan stop now leaves assignment rows
  alone (all providers), and both stores' deletes are owner-scoped
  (RavenDB optimistic concurrency, Cosmos ETag).
- New INodeAgentPersistence.TryClaimAssignmentAsync: insert only if
  unowned, report whether the row is ours. The sweep restores claims
  through it instead of the last-writer-wins upsert. No default
  implementation on purpose -- MultiTenantedMessageStore forwards the
  interface explicitly and a default would silently skip the real store.
- MaxLocalAgentReconciliationsPerTick (default 50) caps sweep actions per
  health check, longest-standing divergence first.
- Sweep starts go through StartAgentGuardedAsync with the command sequence
  captured before the snapshot read; starts in flight are tracked
  (_startingAgents) so a slow start is never started twice.
- Compliance: five ownership tests in NodePersistenceCompliance (the
  owner-scoped delete test fails against RavenDB's old delete-by-id) and
  four divergence scenarios in LeadershipElectionCompliance. Also bounds
  persist_and_load_node_records, whose retry counter never advanced and
  hung the SharedMemory variant for 40 minutes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VDUrBeB4tTnKj4AExCS1nj
@jeremydmiller

Copy link
Copy Markdown
Member Author

Singular-agent A/B result: in 15 interleaved rounds on Oracle, singular_agent_is_only_running_on_one passed 15 of 15 on the #4404 baseline and 15 of 15 on this branch. That means no measurable difference. Together with the logs, where the reconciliation sweep took no action in any failing run and the duplicate comes from the leader re-placing a start that hadn't been confirmed yet, it confirms that flake predates this PR and isn't affected by it.

@jeremydmiller
jeremydmiller merged commit cd776ae into main Sep 10, 2026
42 checks passed
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.

Follow-ups from #4404: owner-scoped assignment deletes on RavenDB/Cosmos, reconciliation-sweep hardening, compliance coverage for assignment divergence

1 participant