Skip to content

Fix snapshot recovery across automatic updates - #3782

Open
vigoo wants to merge 3 commits into
mainfrom
fix/gol-182-snapshot-updates
Open

Fix snapshot recovery across automatic updates#3782
vigoo wants to merge 3 commits into
mainfrom
fix/gol-182-snapshot-updates

Conversation

@vigoo

@vigoo vigoo commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • track the component revision that produced each automatic snapshot
  • invalidate pre-update snapshots after successful updates and recover post-update snapshots with matching component metadata
  • add worker-executor regressions for stale snapshots and stale durable context

Testing

  • cargo clippy -p golem-common -p golem-worker-executor --all-targets -- --no-deps -Dwarnings
  • targeted worker-executor integration and status reducer tests
  • release builds and WASM-target clippy for both updated test components

Resolves GOL-182

@vigoo
vigoo requested a review from a team August 28, 2026 12:11
@netlify

netlify Bot commented Aug 28, 2026

Copy link
Copy Markdown

Deploy Preview for golemcloud canceled.

Name Link
🔨 Latest commit eee1633
🔍 Latest deploy log https://app.netlify.com/projects/golemcloud/deploys/6a92db8cd6790b0008d50d6b

@kmatasfp

kmatasfp commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Potential issue 1 — the in-place snapshot fallback now replays pre-update history with post-update metadata

worker/mod.rs:6491 sets component_version_for_replay = snapshot_revision when the automatic snapshot is used. Correct for the snapshot path — but try_load_snapshot has two fallbacks that restart replay in place, without recreating the instance and therefore without recomputing metadata:

  • durable_host/mod.rs:3839 — the entry at last_snapshot_index isn't a Snapshot/PendingUpdate
  • durable_host/mod.rs:3866download_payload fails (transient blob-store error)

Both call restart_replay_without_snapshot()drop_override_and_restart()move_replay_idx(OplogIndex::INITIAL) (replay_state/cursor.rs:1451) and return NotAttempted. resume_replay then walks the whole oplog from INITIAL with state.component_metadata still pinned to the snapshot's revision.

Concretely: create at rev 1 → auto-update to rev 2 → snapshot at rev 2 → restart → payload download times out → the rev-1-era AgentInvocationStarted entries get lowered through rev-2 metadata at durable_host/mod.rs:5580. If the surface changed across the update — the reason you updated — lowering fails or lowers a different shape. That's the mirror image of the bug this PR fixes.

The guest-side failure path is fine, by contrast: SnapshotRecoveryResult::Failed sets snapshot_recovery_disabled and re-creates the instance, so create_instance recomputes the revision correctly.

Two notes on scope:

  • The class already exists for the manual path (last_manual_update_snapshot_index paired with a post-update component_revision_for_replay); this PR extends it to automatic snapshots.
  • The obvious fix — return Failed from those two arms — is only safe for the automatic case. snapshot_recovery_disabled is consulted only for the automatic snapshot (worker/mod.rs:6486), never for last_manual_update_snapshot_index, so a permanently-bad manual snapshot would spin on RetryDecision::Immediate forever. That gating gap is pre-existing and worth its own issue.

Potential issue 2 — missing Desert evolution step

golem-common/src/model/mod.rs:698 carries #[desert(evolution())]; the new field at :737 adds no step. AdtDeserializer::read_field maps an unlisted field to chunk 0, and old blobs also have stored_version == 0, so stored_version < chunk is false and it reads a field the old bytes don't contain.

It degrades safely — reassemble_cached_status (services/worker.rs:216) documents .ok()? as a deliberate cache miss — but every live agent's cached status and checkpoint is discarded on upgrade and recomputed from the full oplog. The repo convention is the one-liner (12 sites, e.g. base_model/oplog/mod.rs:188, model/mod.rs:346):

#[desert(evolution(FieldAdded("last_automatic_snapshot_component_revision", None::<ComponentRevision>)))]

@vigoo

vigoo commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

missing Desert evolution step is intentional - we don't keep backward compatibility between 1.5 and 1.6

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.

2 participants