[Vamsi-klu]

Closes #10572

Change Description

Background

Import is a merge with an empty base, where the destination iterator is wrapped in SkipPrefixIterator so that everything under the import prefixes is dropped ("prefixes completely overridden", graveler.go). The merge main loop only copies a whole destination range when validWritingRange(m.dest) says the range does not touch an import prefix. handleAll, which drains whichever side is left after the other runs out, had no such check.

Bug Fix

1. Problem - Re-importing into a prefix whose old objects span more than one range keeps some of the old objects. The issue's own repro (2000 files, default settings) leaves 222 objects under p/ instead of 1. The leaked ranges keep their original range IDs, so nothing reports them as changed.
2. Root cause - When the import source runs out before the destination, handleAll(dest) copies each remaining destination range with WriteRange. With an empty base, getNextOverlappingFromBase is always nil, so every remaining range is copied whole, including ranges that straddle the prefix or lie entirely inside it. SkipPrefixIterator.NextRange() skips ranges entirely inside the prefix, but SkipPrefixIterator.Next() can stop on such a range's head after skipping records across a range boundary. handleAll then copies that range too. That is the path the issue's repro takes.
3. Solution - In handleAll, only copy a range whole when validWritingRange(iter) is true, the same guard the two main-loop copy sites already use. Otherwise enter the range so SkipPrefixIterator filters its records. For plain merges validWritingRange is always true (their iterators are not ImportIterators), so merge behavior is unchanged.

Testing Details

New Test_import cases (pkg/graveler/committed/import_test.go). Each lists the exact WriteRange (range copied whole) and WriteRecord (record written after filtering) calls expected:

• source exhausted before a dest range straddling the prefix (fails on master)
• small source before left-straddling, bounded and right-straddling dest ranges, the issue's layout (fails on master)
• empty source, i.e. re-import of an empty directory, where every dest range goes through handleAll; ranges outside the prefix are still copied whole (fails on master)
• prefixes equal to a range MinKey and MaxKey, plus keys equal to the prefix string (fails on master)
• "" prefix replaces the whole branch (guard, passes before and after)
• no prefixes, so dest ranges left after the source are still copied whole (guard, passes before and after)

Commands:

go test -count=1 -run 'Test_import|Test_merge' ./pkg/graveler/committed/   # 4 new cases fail without the merge.go change, all pass with it
go test -count=1 -race ./pkg/graveler/...                                  # pass; full Test_merge unchanged
go vet ./pkg/graveler/...                                                  # clean
go tool golangci-lint run ./pkg/graveler/...                               # 0 issues

Why synthetic unit cases: the bug depends on where range boundaries fall relative to the prefix. Range breaks are FNV-64a(key) % range_raggedness_entries == 0, so a natural repro depends on the hashes of specific key names and on the default settings. The unit cases spell out each range layout directly, so they do not depend on hashes or settings.

Manual check with a local lakefs run --quickstart (local blockstore import), unfixed master vs this branch:

• Issue repro as written (2000 files, defaults): master leaves 222 objects under p/; this branch leaves 1 (p/f).
• 300 files with committed.permanent.range_raggedness_entries: 20, delete all but 10 source files, re-import into the same p/: master leaves 19 objects (p/f0029..p/f0037 leaked); this branch leaves 10.
• Then delete the remaining source files and re-import the empty directory: master leaves 10 objects; this branch leaves 0.

Objects outside the prefix (a, q) survive in every run. An esti test is possible because the issue's 2000-file layout splits deterministically, but it would depend on default settings and key-name hashes. I can add one if you want it.

Breaking Change?

No. Plain merges are unchanged. Import now removes old objects under the prefix, as the docs already say it does. One cost difference: when the source runs out before a large old prefix, those ranges are now read record by record instead of being copied wrongly.

Additional info

• Existing behavior, not changed here: import prefixes are raw strings (strings.HasPrefix), and object-type import paths are passed as prefixes too. Importing an object to x/file1 therefore also removes x/file10 from the destination. This comes from reading the code; I did not run it.
• Possible follow-up for performance: SkipPrefixIterator.Next() does not check IsCurrentRangeBoundedByPrefix on a range head it reaches while skipping records (NextRange() does). With this fix that no longer affects correctness, since every copy site checks validWritingRange. But those ranges are scanned record by record instead of skipped, which already happens in the merge main loop today.

Contact Details

Via this PR.

Drafted-by: Claude Opus 5.5; reviewed by @Vamsi-klu before posting

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-01T20:31:14.941044Z	580b637	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.