Report a delete whose reply was lost as having succeeded

[kmatasfp]

Chaos scenario S6 (GOL-372) killed an executor mid-deletion on golem-dev, twice, against v1.5.10. Both runs reported deletes that had in fact succeeded as AGENT_NOT_FOUND.

Run	Deletes in flight at the kill	Reported AGENT_NOT_FOUND despite succeeding
32795611004	8	4
32798585154	10	7

delete_worker_internal opens with get_latest_metadata(...).ok_or(worker_not_found), so deleting is not idempotent in its response. The first attempt lands, its executor dies before replying, worker-service invalidates its routing table and retries, and the new owner correctly reports nothing there. All seven took 11s, matching the executor client's ~10s connect timeout before the retry; a healthy delete p50s at 42ms.

The effect was always correct — the resurrection oracle was clean across 138,900 rounds. Only the answer was wrong. Same family as #3752: the routing layer re-sends a request whose meaning depends on work the first attempt already did.

The fix

Settle existence with a read. A retried delete cannot tell "I already deleted it" from "it was never here". A read can, because repeating it does not change the answer. WorkerService::delete now does one get_metadata before anything is dispatched. Absent → AGENT_NOT_FOUND, nothing dispatched, nothing to retry. This is what keeps #3133's answer (which resolved #2404, a user told a delete worked when no agent existed) intact rather than trading one wrong answer for another.

Count dispatches, not routing attempts. Given the agent existed, a not-found from a retried dispatch means our own delete landed, so it is reported as success. The counter lives in the remote-call closure rather than reading call_worker_executor's attempt number, because there are two retry layers: MultiTargetGrpcClient::call has its own retries_on_unavailable loop that reconnects and re-sends, and the routing layer never sees those. A counted dispatch is one handed to a connection, not one known to have arrived — attempt races the call against connection retirement, so a request that never left looks identical to a reply that was lost. That ambiguity is the thing being counted.

Discount dispatches the executor turned away. delete_worker_internal calls ensure_worker_belongs_to_this_executor before the metadata lookup, so an InvalidShardId or ShardingNotReady reply proves that dispatch deleted nothing. It still gets retried. Counting it would mean an ordinary rebalance reported success for a delete that never happened — no crash required.

The request is also built once above the retry closure and cloned per attempt, as #3752 left invoke_agent.

What remains rounding the other way is narrow: the agent existed at the read and a third party deleted it before one of our dispatches landed. That needs a concurrent deleter on the same id, and the agent is gone either way.

Tests

Eight. handle_delete_reply is shared between the retry closure and the tests, so the sequence tests drive the real arithmetic rather than a copy of it. Every guard was verified by disabling it:

Disabled	Fails
pre-delete read	deleting_an_agent_that_never_existed_still_reports_it_missing
not-found guard	a_delete_whose_reply_was_lost_is_not_reported_as_a_missing_agent
not-found guard widened to > 0	the_only_dispatch_of_a_delete_still_reports_a_missing_agent
turned-away discount	a_dispatch_the_executor_turned_away_is_taken_back_out_of_the_count

The pre-check test asserts that nothing was dispatched, not merely that the right error came back — a dispatched delete is one that can be retried, and the retry is what would turn the answer into a success. a_dispatch_that_was_never_answered_still_makes_the_next_answer_ambiguous is the positive control: a transport failure never reaches handle_delete_reply, which is why dispatches are counted rather than replies. The turned-away test covers ShardingNotReady as well as InvalidShardId, exercising the proto round-trip both take.

Notes for review

• That every attempt shares one counter is structural, not covered. RecordingWorkerClient sits above call_worker_executor, so no test at that seam sees a second attempt — the same gap Answer callers whose agent moved, and stop retries duplicating their work #3752 documented. Closing it needs a fake WorkerExecutor gRPC server, which the repo does not have.
• The recreate case is untouched. Delete carries no identity tying an attempt to an incarnation of an agent id, so a retry can still delete a newly recreated X. That is the real exactly-once violation; this PR fixes the cosmetic one. S6 cannot produce it — each slot's emitter is sequential and never recreates an id with a delete outstanding. Reasoned from the code, not observed.
• The S6 driver's max_retries: 0 on delete stays necessary. A driver-level retry is a fresh request with its own dispatch count.
• Revert is the remaining operation in this family. S7 (GOL-373) shows it has the same property with a worse consequence: retrying "the last two invocations" takes back four.
• One commit is an unrelated CI fix. build-golem-moonbit and it-cli are both red on 1.5.x because the pinned 0.10.1+a46be2066 toolchain 404s out of the CDN. Update test-r to 3.0.12 #3760 fixed this on main as part of a test-r update; this is the one line of it that matters, applied by hand. Both jobs now pass. main also dropped moon install from these jobs — deliberately not backported, since it does not fix this and came with a MoonBit workspace restructure 1.5.x does not have.

[github-actions]

✅ All contributors have signed the CLA.
_{Posted by the CLA Assistant Lite bot.}

[kmatasfp]

recheck

[vigoo]

I'm not sure about this. The not-found message was intentionally introduced in #3133

Now with this, I understand we 'survive' a scenario where the first request deletes the agent but the reply is lost, and a retried deletion sees no agent anymore and returns with a failure.
On the other hand we introduce another kind of error where the first request does not reach the executor or in any way does not end up deleting the worker, then we retry, and notice that it was originally already deleted - but we don't return with not-found.

[kmatasfp]

You're right, and that trade wasn't the right one to make. #3133 resolved #2404, where the complaint was precisely "delete reports success when there was no agent", and counting dispatches hands that back whenever a retry happened.

Fixed in cdf0895 by settling existence with a read rather than inferring it from a retry.

A retried delete can't distinguish "I already deleted it" from "it was never here" — delete_worker_internal opens with a metadata lookup and answers identically either way. A read can, because repeating it doesn't change the answer. So WorkerService::delete now does one get_metadata before anything is dispatched:

• absent at that read → AGENT_NOT_FOUND, nothing dispatched, nothing to retry. Fail on deleting non-existing agents #3133's behaviour, now holding during churn rather than only when nothing goes wrong.
• present → a not-found from a retried dispatch means our own delete landed, and is reported as success.

Cost is one extra RPC on the delete path; a healthy delete p50s at 42ms.

What's left rounding the other way is narrower than the case you described: the agent existed at that read, and a third party deleted it before one of our dispatches landed. That needs a concurrent deleter on the same id, and the agent is gone either way.

If the extra round trip isn't worth it, the alternative is dropping the routing-layer retry for delete entirely — then not-found is always first-dispatch and always honest, but a lost reply surfaces as a transport error and the caller is left to decide what it means. Happy to go that way instead.

The test is deleting_an_agent_that_never_existed_still_reports_it_missing. It asserts that nothing was dispatched, not just that the right error came back — returning the error isn't enough, since a dispatched delete is one that can be retried, and the retry is what would turn it into a success.