Fix the second snapshot-based update on an agent

[kmatasfp]

What is broken

The second manual (snapshot-based) update on an agent fails, leaving it unable to start:

snapshot-based pending update expected replay state to already be live

Direction is irrelevant. Two updates moving forward, to two revisions carrying the same build, fail identically, which is what manual_update_on_idle_twice pins down. An agent's first manual update works, so this only appears once an agent has been manually updated before.

Why

calculate_skipped_regions already does the right thing. When it sees a pending SnapshotBased update it installs a replay override covering everything up to that update's own oplog entry, because the snapshot carries the agent's whole state:

OplogEntry::PendingUpdate { description: UpdateDescription::SnapshotBased { .. }, .. } => {
    skipped_override = Some(/* INITIAL.next()..=*idx */)
}

PrivateDurableWorkerState::new then overwrites it via set_override, substituting the region for last_manual_update_snapshot_index, which is the previous manual update's snapshot. set_override replaces rather than merges; it drops whatever override is already in place:

pub fn set_override(&mut self, other: DeletedRegions) {
    if self.is_overridden() {
        self.drop_override();
    }

Everything the agent did between the two updates is therefore left to replay, and prepare_instance refuses exactly that while a snapshot update is pending:

// If a snapshot based update is pending, no replay should be necessary
if !store.as_context().data().durable_ctx().is_live() {
    return Err(WorkerExecutorError::runtime(
        "snapshot-based pending update expected replay state to already be live",
    ));
}

That is the whole asymmetry: on an agent's first manual update last_snapshot_index is None, nothing clobbers the override, and it works.

Fixed by

Deleting the override. PrivateDurableWorkerState::new passes the status record's skipped_regions straight to ReplayState::new.

The branch was a second copy of an invariant calculate_skipped_regions already maintains. On SuccessfulUpdate the reducer folds the pending override into the regions proper, and calculate_updates sets last_manual_update_snapshot_index to the very index whose region it just folded, in the same iteration over the same entries. One region, computed twice. On a second manual update the two copies disagree, and set_override wins.

last_snapshot_index stays on AgentConfig: try_load_snapshot reads the snapshot payload back through it. It just no longer decides which regions replay skips.

The parameter is renamed with it. It is fed from last_known_status.skipped_regions, and AgentStatusRecord carries both skipped_regions and deleted_regions with different meanings, so the field held the first under the second's name. ReplayState::new already called its parameter skipped_regions; now the hops in between agree. AgentConfig's field is renamed too, since it is the same value one step earlier.

One nearby set_override stays: the automatic-snapshot one in worker/mod.rs. calculate_skipped_regions never matches on OplogEntry::Snapshot, so there is no reducer-side equivalent to defer to. The two looked symmetric; only the manual one was redundant.

Tests

Three in hot_update.rs, using the existing SnapshotCounter agent (already snapshot-capable in both builds) with agent-counters-v2 as the update target, following the it_agent_update_v3_release precedent already in that file.

Test	Pins
manual_update_on_idle_twice	the bug, with direction ruled out, since both updates move forward to the same build
manual_update_on_idle_to_earlier_component	a manual update to a revision carrying an earlier build, state intact across both hops
auto_update_on_idle_after_manual_update	a later automatic update replaying only the suffix after the manual snapshot, not the pre-migration history the migration existed to get past

Against 1.5.x the first two fail with the runtime error above; with the fix they pass. On the head of this branch the module is 18 of 18 green, alongside 497 executor unit tests, with clippy and fmt clean.

The third test covers ground that had no coverage either way, and needs an agent that is snapshot-capable and distinguishable between builds. So SnapshotCounter gains a component_version returning 1 in agent-counters and 2 in agent-counters-v2, the same single-constant difference the Counter agent in the same crate already carries, where diff agent-counters/src/lib.rs agent-counters-v2/src/lib.rs is one line.

One reducer test comes with it. two_successful_manual_updates in worker/status.rs builds two snapshot-based updates that both succeed, which is the sequence the runtime got wrong and which the existing multiple_manual_updates_with_jump_and_revert cannot reach: that one fails its first update, and FailedUpdate drops the override without folding it, so nothing ever accumulates.

On mutation coverage

Mutant	Result
restore the unguarded set_override, as on 1.5.x	killed by manual_update_on_idle_twice, manual_update_on_idle_to_earlier_component
drop the reducer's SuccessfulUpdate fold	killed by auto_update_on_idle_after_manual_update, agent_can_be_invoked_after_manual_snapshot_update_and_restart, worker::status::single_manual_update

An earlier revision of this PR guarded the set_override rather than deleting it, because mutation testing showed it surviving removal. That reasoning was backwards. The fold and the set_override were each masking the other: alone, either could be removed and every test stayed green; only the pair going together turned anything red. A surviving mutant meant the branch was untestable, not that it was load-bearing.

Deleting one makes the other observable. Both rows above were run, not reasoned about: the first against the guarded revision, where removing the guard is byte-for-byte the 1.5.x behaviour, and the second against this one.

Also in this diff

MOONBIT_INSTALL_VERSION moves from 0.10.1+a46be2066 to 0.10.2+1bb3e16cf in ci.yaml. Nothing to do with the fix: the pinned toolchain stopped resolving and build-golem-moonbit cannot pass without it. Same bump as #3764, which shares this base.

How it was found

The S9 chaos scenario (executor crash during a component rollback) needs snapshot-based updates, because the documented model makes automatic update viable only for changes no recorded invocation can tell apart, and a rollback is usually the opposite. Building that leg ran into this immediately.

Related

• Fix missing load snapshot after manual snapshot update #2955 fixed adjacent snapshot-update state-machine bugs (duplicate enqueued updates, missing load-snapshot on replay start)
• GOL-182 tracks separate snapshot/update boundary issues, including stale periodic snapshots after an automatic update. Not addressed here.

[vigoo]

While I was checking this to make sure I understand I realized there is a simpler fix, this is just a mix of old and new code that has not been properly updated in the past.

Actually the deleted_regions parameter here is coming from the latest calculated AgentStatusRecord's skipped_regions so it should be renamed to skipped_region. And the status record already applies the same calculation based on pending/successful updates and so on, so this whole override does not seem to be necessary, we could just pass this skipped_regions (currently deleted_regions) to the ReplayState::new directly.

[kmatasfp]

Right on both counts, thanks. Done in 65d3e51.

PrivateDurableWorkerState::new now passes the status record's regions straight to ReplayState::new. The rename goes one hop further back than the parameter, to AgentConfig, which held the same value under the same wrong name. AgentStatusRecord carries both skipped_regions and deleted_regions with different meanings, so the misnaming was doing real damage to readability; ReplayState::new already called its parameter skipped_regions, and now everything between agrees.

On the override being unnecessary: confirmed. calculate_updates sets last_manual_update_snapshot_index to applied_update_oplog_index in the same SuccessfulUpdate iteration where calculate_skipped_regions folds in the override covering INITIAL.next()..= that same index. Same region, computed twice from the same entries.

One nearby set_override I deliberately left: the automatic-snapshot one in worker/mod.rs. calculate_skipped_regions never matches on OplogEntry::Snapshot, so there is no reducer-side equivalent to defer to there. The two looked symmetric, but only the manual one was redundant.

The part worth recording is that your version is not just smaller, it is better covered. Mutation testing on the guarded version showed the reducer fold and this set_override each survived removal on their own and only died as a pair, so each was masking the other. With one gone, dropping the fold now kills auto_update_on_idle_after_manual_update and agent_can_be_invoked_after_manual_snapshot_update_and_restart on top of the reducer unit test that already caught it. I ran that mutant to check rather than assume.

18/18 hot_update, 497 executor unit tests, clippy and fmt clean. The description's "On mutation coverage" section argued for keeping the branch, so I have rewritten it.