fix(table): resolve write parent snapshot from target branch head instead of main

[badalprasadsingh]

Description

Fixes #1636

Writes on a non-main branch built snapshots parented on main's head because createSnapshotProducer used MetadataBuilder.currentSnapshot() (only synced with main), ignoring txn.branch. This corrupted branch lineage and could replace branch data with main's on the first commit.

Added currentSnapshotForRef / currentSnapshotIDForRef, and used them for the parent snapshot, the AssertRefSnapshotID requirement, and the overwrite to append downgrade check. main / empty-ref behavior is unchanged; a not-yet-existing branch falls back to the current snapshot.

This PR is intentionally limited to fixing snapshot parent selection, lineage, and OCC behavior for branch writes. Read-side planning paths (such as duplicate checks and replace planning) are not yet branch-aware and will be addressed in a follow-up change.

Testing

Added the necessary tests for it.

[laskoviymishka]

I'd hold this before merge, but the diagnosis is right: currentSnapshot() only ever tracking main was the actual root cause, and splitting the parent lookup from the assertion lookup is the correct shape.

What I'd want fixed first is that rebuildSnapshotUpdates in table/table.go doesn't mirror the new fallback. It resolves the retry parent with freshMeta.SnapshotByName(branch) and stops, so on a first write to a branch that doesn't exist yet it gets nil where currentSnapshotForRef gave attempt 0 main's head. rebuildFn then calls assembleManifests with a nil parent, existingManifests(nil) returns nothing, and the rebuilt manifest list carries only the new files while the attempt-0 list that did inherit main's manifests gets deleted as orphaned. The commit succeeds and the branch quietly comes up missing all of main's data. The asymmetry predates this PR, but the old main-head assertion always failed before any retry could commit, so this is the change that makes it reachable.

Things I'd want settled before merge:

• add the CurrentSnapshot() fallback to rebuildSnapshotUpdates, or move the fork-from-main choice up into createSnapshotProducer so one place owns it
• cover the first write to a new branch when main already has data, since every branch test here starts from an empty table and the parentless assertion passes vacuously
• either extend currentSnapshotForRef to ReplaceDataFiles, ReplaceDataFilesWithDataFiles and AddDataFiles, or file a follow-up so branch support isn't advertised as complete

Once those are addressed, happy to take another pass.

[laskoviymishka]

This fallback isn't mirrored on the OCC retry path, and the gap is silent.

rebuildSnapshotUpdates in table/table.go:694 (outside this diff) resolves the retry parent as freshMeta.SnapshotByName(branch) and stops there, no CurrentSnapshot() fallback. On a first write to a not-yet-existing branch, attempt 0 gets main's head through this helper and inherits main's manifests, but a retry gets freshHead == nil, so rebuildFn calls assembleManifests with a nil parent, existingManifests(nil) returns nothing, and the rebuilt list holds only addedContent. The attempt-0 list that did inherit main's manifests is then deleted as orphaned. The commit succeeds with the branch missing all of main's data.

The asymmetry predates this PR, but pre-fix the main-head assertion failed Validate on every attempt so no retry could ever commit. This change is what makes it reachable. A couple of ways to handle, wdyt?

• give rebuildSnapshotUpdates the same CurrentSnapshot() fallback, matching Java's SnapshotUtil.latestSnapshot(freshBase, branch)
• drop the fallback here and make fork-from-main an explicit choice in createSnapshotProducer, so one place owns the policy

[badalprasadsingh]

Fixed.

You are absolutely right. It was just as you described, moving with option one: latestSnapshotForBranch matches Java's SnapshotUtil.latestSnapshot(freshBase, branch). Added TestBranchCreateForksFromMainHeadAcrossRetry, which reproduces the data loss on the pre-fix code.

[laskoviymishka]

Three sibling call sites in this same file still read main's head, so branch support is only half wired.

ReplaceDataFiles at line 613 and ReplaceDataFilesWithDataFiles at line 999 both do s := meta.currentSnapshot(), bail with ErrInvalidOperation when it's nil, then scan s.dataFiles(fs, nil) to build the delete set. AddDataFiles does the same at line 906 for the duplicate check. On a branch transaction that means a branch with data fails as "cannot replace files in a table without an existing snapshot" when main is empty, and the duplicate check misses files that live only on the branch.

I'd swap those to currentSnapshotForRef(t.branch) in this pass, since it's the same one-line change you've already made here. If you'd rather keep the diff narrow, a follow-up issue works, but then the PR description shouldn't read as though branch writes are fixed end to end.

[badalprasadsingh]

Yes. For now, I am keeping his PR scoped to the snapshot-parent/lineage + retry fix and handling the read-side planning in a dedicated follow-up.

I'll make the PR description explicit that read-side planning is not yet branch-aware so nothing reads as end-to-end.

[laskoviymishka]

This assertion is vacuous as written.

Step 1 builds "feature" on a fresh table, so currentSnapshotForRef("feature") falls through to currentSnapshot(), which is nil, and ParentSnapshotID comes back nil regardless of what the helper does. It'd still pass if we deleted the !ok fallback entirely.

The case that actually exercises the fallback is a first write to a new branch when main already has data: ParentSnapshotID should be main's head and the snapshot's manifests should include main's files. That's the same scenario the retry path gets wrong, so pinning it here earns its keep twice.

[badalprasadsingh]

Done.

It was vacuous. Kept the fresh-table case as said but added TestBranchCreateForksFromMainHead, which starts from a table where main already has data, so ParentSnapshotID must equal main's head and the new branch's manifests must include main's files - it now fails if the !ok fallback is removed. As you noted, this is the same scenario the retry path got wrong, so the retry variant pins it a second time.

[laskoviymishka]

I think this works right now and it isn't a blocker, but I'd really like both doc comments to say why they diverge, not just that they do.

The split is load-bearing: the parent lookup wants main's head so a new branch forks from main, while the assertion wants nil so AssertRefSnapshotID proves the branch is absent. Read cold, the two comments look like an inconsistency. The failure mode for anyone deriving one from the other (currentSnapshotForRef(ref).SnapshotID as the assertion id) is a spurious OCC rejection on every new-branch create. One sentence in each comment naming the caller it serves would head that off.

[badalprasadsingh]

Done.

[laskoviymishka]

Small thing: "unknown refs" here means "not in b.refs", but a ref that is present and points at a snapshot missing from b.snapshotList returns nil rather than falling back.

RemoveSnapshots prunes dangling refs so this shouldn't happen today, and currentSnapshot() at line 388 has the same shape, so I'm not asking for a code change. Worth a line saying the fallback assumes every entry in b.refs resolves, since createSnapshotProducer and mergeOverwrite both read nil as "no snapshot exists".

[badalprasadsingh]

Done, as well.

[laskoviymishka]

Worth the checked form here, for consistency with addSnap1 fourteen lines up. If upM[0] ever comes back as a different update type this panics into a goroutine dump instead of a clean test failure.

[badalprasadsingh]

Done. Now, consistent with addSnap1.

[zeroshade]

Thanks for taking this on — the parent-selection change itself is correct, a single-snapshot retry correctly re-resolves the branch head on every attempt, missing branches correctly fork from main, and default/explicit-main behavior is unchanged. Those pieces are sound.

Branch write planning still reads main

The blocking issue outside the changed lines is that snapshot production now parents on the target branch, while high-level destructive-write planning and validation still inspect meta.currentSnapshot() (main). The resulting snapshot can therefore be parented on the branch while its deletion/replacement plan describes main. A full branch overwrite can retain branch-only files, and replace operations can reject or target the wrong files.

The verified head locations are:

• table/transaction.go:613-643 — ReplaceDataFiles membership and duplicate validation.
• table/transaction.go:905-921 — add-file duplicate validation.
• table/transaction.go:999-1029 — ReplaceDataFilesWithDataFiles membership and duplicate validation.
• table/transaction.go:1133-1185 — ReplaceFiles data/delete/DV validation.
• table/transaction.go:1271-1286 — add-file duplicate validation.
• table/transaction.go:1681-1697 — copy-on-write/delete file classification.
• table/transaction.go:1756-1763 — filtered deletion manifest selection.
• table/transaction.go:2173-2190 — merge-on-read deletion-vector collection.
• table/rewrite_manifests.go:360-367 — branch rewrite no-op detection.

Suggested fix: resolve every planning and validation snapshot through currentSnapshotForRef(t.branch) (or a shared fallible branch-aware lookup), then add semantic tests using branch-only files.

Write-path coverage

Path	Branch HEAD as parent	End-to-end
Append	Correct (snapshot_producers.go:576-605)	Correct except multi-snapshot retry
Fast append	Correct (snapshot_producers.go:92-96)	Correct except multi-snapshot retry
Merge append	Correct (snapshot_producers.go:516-527)	Correct except multi-snapshot retry
Overwrite	Correct (snapshot_producers.go:153-157)	Incorrect: deletion planning reads main
Delete copy-on-write	Correct (transaction.go:61-71,1477-1505)	Incorrect: file classification reads main
Delete merge-on-read	Correct (transaction.go:1524-1552)	Incorrect: classification and DV lookup read main
ReplaceDataFiles	Correct (snapshot_producers.go:658-684)	Incorrect: validation/planning reads main
ReplaceFiles / ReplaceDataFilesWithDataFiles	Correct (snapshot_producers.go:1038-1058,1193-1218)	Incorrect: validation/planning reads main
RewriteFiles / RewriteDataFiles	Correct via ReplaceFiles	Incorrect: inherits main-based validation
RewriteManifests	Correct (rewrite_manifests.go:133-137)	Incomplete: no-op check at :360-367 reads main
RowDelta	Correct (row_delta.go:159-181)	No branch-specific coverage
Multiple staged snapshots	Correctly chained initially	Incorrect on retry: all get the same fresh parent

The two other blockers are inline: retrying multiple staged snapshots has a silent-data-loss path, and a tag-targeted write can advance and convert the tag into a branch. Missing coverage includes public Append/Overwrite/Delete/Replace/Rewrite/AddFiles/RowDelta on divergent branches; actual branch-only overwrite/delete semantics; copy-on-write and merge-on-read deletion; multiple staged snapshots with a forced retry; branch advancement between attempts; a missing branch concurrently created as a tag; existing-tag rejection; full public explicit-main commit; and malformed/dangling refs.

[zeroshade]

freshHead is calculated once and then supplied to every staged addSnapshotUpdate. If a transaction stages snapshots A and B and retries, both are rebuilt as children of the same branch head; they become siblings, and the final ref to B omits A. That is a silent-data-loss path.

Suggested fix: replay staged snapshot updates sequentially, making each rebuilt snapshot the logical parent of the next one, and add a forced-retry test with at least two staged snapshots.

[zeroshade]

This accepts any named ref without checking its type. A transaction targeting a tag will resolve the tag as its parent, then commitManifests emits a BranchRef replacement: the write advances the tag and converts it into a branch. The same bug exists if an initially absent branch name becomes a tag during retry.

Suggested fix: reject non-branch refs both when constructing the transaction and after every metadata refresh before replay/commit, with tests for an existing tag and the absent-name→tag race.

[zeroshade]

A present ref whose snapshot cannot be resolved is silently treated as parentless here. Although loaded metadata should already reject this state, this helper should fail closed rather than turning a dangling ref into a first-snapshot write.

Suggested fix: make the lookup fallible (or validate before reaching it) and propagate ErrInvalidMetadata when a named ref points to a missing snapshot.

[badalprasadsingh]

Sure @zeroshade. I'll address Issue #1638 in this PR itself. Will also revert the PR title and description.

[laskoviymishka]

Almost there. The retry path is fixed and the new test bites, which is what I wanted from last round.

One thing before merge, and it's new with the assertion change rather than something I raised before. Switching AssertRefSnapshotID to the ref's own head removed the accident that was blocking tag writes: pre-fix the requirement asserted main's head against a tag and failed Validate, so tag targeting was unreachable. Now it asserts the tag's own id, passes, and commitManifests advances the ref with BranchRef, so a tag named v1.0 becomes a branch and stops being immutable. Java guards this in SnapshotProducer.targetBranch(). A ref-type check in NewTransactionOnBranchWithError would fail it at construction and cover all three helpers at once.

Everything else I asked for is in:

• latestSnapshotForBranch, taking the option-one route matching SnapshotUtil.latestSnapshot; crediting your call, mirroring it was the right choice over pushing the policy up into createSnapshotProducer
• TestBranchCreateForksFromMainHead starting from a table where main already has data, so it fails if the !ok fallback goes away, and TestBranchCreateForksFromMainHeadAcrossRetry reaching the rebuild path
• the doc comments on both helpers, and the checked assertion on addSnapM
• #1638 for the read-side callers, which settles the scope question

Fix the tag guard and this is good to land. The SnapshotByName simplification and the peer-advances-branch test case are both optional.

[laskoviymishka]

This is new with the assertion change, not something I raised last round. None of the three helpers check SnapshotRefType, so a tag name passed as txn.branch resolves like a branch. Pre-fix that was harmless by accident: the requirement asserted main's head against the tag, so Validate rejected it. Now it asserts the tag's own id, the check passes, and commitManifests calls NewRetainingSnapshotRefUpdate(branch, ..., BranchRef) then b.refs[name] = ref, so the tag advances to a new child snapshot and its immutability is gone. Java rejects this up front in SnapshotProducer.targetBranch(): checkArgument(!refExists || base.ref(branch).isBranch(), "%s is a tag, not a branch...").

I'd put the guard in NewTransactionOnBranchWithError so it fails at construction rather than at commit, which also keeps all three helpers safe without touching them.

[laskoviymishka]

Metadata already exposes SnapshotByName, and its body is exactly this lookup (SnapshotRefs[name] then SnapshotByID) as a direct map index. Refs() yields through cloneSnapshotRef per entry, so this allocates across the whole ref set on every retry attempt to find one name. rewriteRefSnapshotRequirements a few lines up already uses SnapshotByName for the same thing.

if s := meta.SnapshotByName(branch); s != nil {
	return s
}

return meta.CurrentSnapshot()

Same semantics, and it makes the missing ref-type check easier to see.

[laskoviymishka]

flakyCatalog returns the same mainMeta on the retry, so freshMeta still has no feature ref and latestSnapshotForBranch falls back to main's head, same as attempt 0. That pins the nil-parent regression, which is what I asked for. The case it doesn't reach is a peer creating the branch between attempts, where the fallback should be skipped and the peer's head becomes the parent. Not a blocker, but it's the one retry path with no coverage.