Skip to content

fix(orchestrator): recover lost ancestor entries when persisting headers - #3489

Closed
ValentaTomas wants to merge 5 commits into
mainfrom
fix/header-gap-write-path
Closed

ValentaTomas wants to merge 5 commits into
mainfrom
fix/header-gap-write-path

Conversation

@ValentaTomas

@ValentaTomas ValentaTomas commented Jul 31, 2026 •

Copy link
Copy Markdown
Member

Two complementary fixes for headers whose mapping references builds their Builds map lost. #3447 stopped load-time fabrication of zero entries for such gaps; this PR closes the two paths it left open.

Write path: stop persisting inherited gaps

A pause carries its source header's Builds map forward, so an entry lost once (e.g. the live header was peer-served/incomplete when an earlier pause persisted it) propagates to every descendant header, and each descendant diff pays a proactive header refresh per gap forever. Now, when appendAncestorBuilds resolves nothing locally and the entry is still missing, it recovers the entry from the referenced build's own stored header before the diff header is persisted — gaps heal on the next pause instead of being inherited, at the cost of one header GET per still-missing entry (normally zero). Builds with no stored header (legacy uncompressed) stay absent and keep resolving on the read path. The heal lives in appendAncestorBuilds rather than StoreHeader because that's where the ancestor barrier, storage handle, and file type already are.

Read path: recover stale zero entries

Before #3447, the fabricated zero entries were also serialized whenever a pause ran on a header loaded from storage — baked into descendant headers as real entries, on the wire indistinguishable from the legit V3 uncompressed sentinel. For a compressed ancestor the claimed suffix-less object does not exist, and createDiff failed the size lookup permanently; #3447 only helps when the entry is absent. Now a zero entry whose basic-name object is missing falls back to the build's own header (same recovery as the no-entry branch, telemetry cause zero_entry_miss). Legit V3 sentinels are unaffected — their object exists, so no extra roundtrip.

Stored zero entries are re-persisted by later pauses (indistinguishable from V3 sentinels without a per-pause probe), but reads now always recover; rewriting them is fleet hygiene, not orchestrator logic.

orchestrator.storage.diff.frame_table_refresh (cause=proactive) should trend down as affected lineages pause past the write-path fix; cause=zero_entry_miss counts the baked-zero recoveries.

Tests

  • gap recovered from the build's stored header at upload; missing header file leaves the gap absent without failing the upload; present entries cause no storage round-trip
  • stale zero entry: size-lookup 404 falls back to the build's own header and serves the compressed object; the healthy-sentinel latch test still passes unchanged
  • negative controls: reverting either production change fails the corresponding test

A pause clones its source header's Builds map, so a mapping-referenced
build whose entry was lost (e.g. the header was peer-served/incomplete
when an earlier pause persisted it) stays absent in every descendant
header. Since #3447 the read path recovers from such gaps, but each
descendant then pays a header refresh per gap forever.

Close the write path: when appendAncestorBuilds resolves nothing locally
and the entry is still missing, load the referenced build's own header
and copy its self entry before persisting. Builds without a header file
(legacy uncompressed) stay absent and keep resolving on the read path.
@cla-bot cla-bot Bot added the cla-signed label Jul 31, 2026
@cursor

cursor Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes sandbox header persistence and diff open logic on the pause and fault-read paths; behavior is guarded by tests and degrades to the prior gap/absent behavior when recovery fails.

Overview
Headers that reference ancestor builds without matching Builds entries used to keep that gap on every pause and could strand reads behind stale zero sentinels. On pause, when an ancestor isn’t available from the upload wait path and the outgoing header still lacks that build, the upload path now loads the ancestor’s stored header to fill Builds before persistence (best-effort on transient errors; legacy builds with no header stay absent). On read, when a zero sentinel points at a missing basic object or a peer transition during size lookup, createDiff refreshes from the build’s own header and opens the real compressed data instead of failing permanently.

Reviewed by Cursor Bugbot for commit a198284. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 7 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ges/orchestrator/pkg/sandbox/build/storage_diff.go 75.86% 6 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

ValentaTomas and others added 2 commits July 31, 2026 16:11
Older releases fabricated a zero BuildData at load time for every build a
header's mapping referenced but Builds lacked, and a pause serialized those
into descendant headers as real entries — on the wire indistinguishable
from the legit V3 uncompressed sentinel. For a compressed ancestor the
claimed suffix-less object does not exist, so every fault failed
permanently, even after the load-time backfill fix.

Instead of failing the createDiff size lookup, treat a zero entry whose
basic-name object is missing as a persisted gap and resolve the build's
own header, mirroring the no-entry branch. Legit V3 sentinels are
unaffected: their object exists and no extra roundtrip happens.
When the zero-entry fallback cannot load the build's own header, the load
error was dropped and only the original object miss surfaced, so a pruned
genuine V3 build was indistinguishable from an unreadable header. Join both
into the returned size-lookup error.
@arkamar
arkamar marked this pull request as ready for review August 3, 2026 13:08
@arkamar
arkamar requested review from dobrac and jakubno as code owners August 3, 2026 13:08

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Zero-entry recovery skips peer transition
    • Added PeerTransitionedError handling to the zero-entry recovery logic so it recovers via refreshHeader like the no-entry branch does.

Create PR

Or push these changes by commenting:

@cursor push d39bdce632
Preview (d39bdce632)
diff --git a/packages/orchestrator/pkg/sandbox/build/storage_diff.go b/packages/orchestrator/pkg/sandbox/build/storage_diff.go
--- a/packages/orchestrator/pkg/sandbox/build/storage_diff.go
+++ b/packages/orchestrator/pkg/sandbox/build/storage_diff.go
@@ -209,7 +209,15 @@
 
 	if size == 0 {
 		size, err = upstream.Size(ctx)
-		if hasEntry && initialFT == storage.UncompressedFullFrameTable && errors.Is(err, storage.ErrObjectNotExist) {
+		var shouldRecover bool
+		if hasEntry && initialFT == storage.UncompressedFullFrameTable {
+			shouldRecover = errors.Is(err, storage.ErrObjectNotExist)
+			if !shouldRecover {
+				var transErr *storage.PeerTransitionedError
+				shouldRecover = errors.As(err, &transErr)
+			}
+		}
+		if shouldRecover {
 			// The zero entry claimed an uncompressed V3-era ancestor, but the
 			// suffix-less object does not exist: the entry is a gap persisted
 			// as zero by older releases. Resolve the build's own header like

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit f26f8a6. Configure here.

Comment thread packages/orchestrator/pkg/sandbox/build/storage_diff.go
@blacksmith-sh

This comment has been minimized.

Comment thread packages/orchestrator/pkg/sandbox/build/storage_diff.go
arkamar added 2 commits August 3, 2026 18:16
The zero-entry fallback only matched ErrObjectNotExist, but the ancestor is
opened through the peer-routing provider, whose Size reports a peer miss as
PeerTransitionedError. A stale zero entry on a peer-served build therefore
failed the read for the length of the transition window instead of refreshing
the build's own header, which is what the no-entry branch already does for the
same signal. Refresh on either error, attributing the peer case to the existing
peer_transitioned cause so zero_entry_miss keeps counting only baked-zero
recoveries.
Recovering a lost ancestor entry is an optimization: an absent entry is what
every release before the recovery persisted, and createDiff still resolves the
build's own header per fault-in. Aborting the header store when that load fails
turns an unreadable ancestor header into a failed snapshot — fatal on the
Checkpoint path, which has no retry and tears the sandbox down, and permanent
for a deterministic read error, which burns the whole retry budget. Log and
leave the gap instead, keeping the failure fatal only when the context is
already done, where continuing would bury the cause under a store-header
error.
@arkamar arkamar closed this Aug 4, 2026
@ValentaTomas
ValentaTomas deleted the fix/header-gap-write-path branch August 15, 2026 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants