From 7d955bb1121fac55d3f6388825b54c5c8c56d016 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 2 Sep 2026 10:21:02 -0700 Subject: [PATCH 1/3] cat-file: report when object enumeration ends early "git cat-file --batch-all-objects" opens each pack's .idx lazily, as its walk reaches that pack. If a concurrent repack removes a pack first, opening its index fails. The object database already turns this into a non-zero return from odb_for_each_object_ext(), but batch_each_object() discarded it and we reported success -- silently omitting the vanished pack's objects while still exiting 0, which is dangerous for tooling that trusts the listing to be complete. Propagate the error: return it from batch_each_object() and, when set, exit non-zero with a diagnostic instead of pretending a truncated listing was whole. This intentionally changes an existing expectation in t5313, which treats a corrupt pack index as a successful empty enumeration. A rejected index also prevents a complete listing, so reporting success there has the same problem as silently skipping a concurrently removed pack. Instead, expect failure while continuing to check that the corrupt index produces no object output. To reproduce, loop "git repack -adq" in one process while another loops "git cat-file --batch-all-objects --batch-check"; before this change the reader occasionally stopped with status 0 mid-repack, and now it errors instead. Assisted-by: Claude Opus 4.8 Signed-off-by: Elijah Newren --- builtin/cat-file.c | 20 +++++++++++--------- t/t5313-pack-bounds-checks.sh | 7 +++++-- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 8870a210ec2e94..7cb0466aadb0ff 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -1008,10 +1008,10 @@ static int batch_one_object_oi(const struct object_id *oid, return payload->callback(oid, NULL, 0, payload->payload); } -static void batch_each_object(struct batch_options *opt, - for_each_object_fn callback, - unsigned flags, - void *_payload) +static int batch_each_object(struct batch_options *opt, + for_each_object_fn callback, + unsigned flags, + void *_payload) { struct for_each_object_payload payload = { .callback = callback, @@ -1026,8 +1026,8 @@ static void batch_each_object(struct batch_options *opt, .filter = &opt->objects_filter, }; - odb_for_each_object_ext(the_repository->objects, &oi, - batch_one_object_oi, &payload, &opts); + return odb_for_each_object_ext(the_repository->objects, &oi, + batch_one_object_oi, &payload, &opts); } static int batch_objects(struct batch_options *opt) @@ -1084,20 +1084,22 @@ static int batch_objects(struct batch_options *opt) cb.seen = &seen; - batch_each_object(opt, batch_unordered_object, - ODB_FOR_EACH_OBJECT_PACK_ORDER, &cb); + retval = batch_each_object(opt, batch_unordered_object, + ODB_FOR_EACH_OBJECT_PACK_ORDER, &cb); oidset_clear(&seen); } else { struct oid_array sa = OID_ARRAY_INIT; - batch_each_object(opt, collect_object, 0, &sa); + retval = batch_each_object(opt, collect_object, 0, &sa); oid_array_for_each_unique(&sa, batch_object_cb, &cb); oid_array_clear(&sa); } strbuf_release(&output); + if (retval) + return error(_("unable to enumerate all objects")); return 0; } diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh index 5be01260d7760f..3edcc0936a1a47 100755 --- a/t/t5313-pack-bounds-checks.sh +++ b/t/t5313-pack-bounds-checks.sh @@ -100,9 +100,12 @@ test_expect_success 'matched bogus object count' ' clear_base && # Unlike above, we should notice early that the .idx is totally - # bogus, and not even enumerate its contents. - git cat-file --batch-all-objects --batch-check >actual && + # bogus, report failure, and not enumerate its contents. + test_must_fail git cat-file --batch-all-objects \ + --batch-check >actual 2>err && test_must_be_empty actual && + test_grep "non-monotonic index" err && + test_grep "unable to enumerate all objects" err && # But as before, we can do the same object-access checks. test_must_fail git cat-file blob $object && From 5591ca7cb3d2f5292bbb75fc99733c996e3f24ab Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 2 Sep 2026 10:23:41 -0700 Subject: [PATCH 2/3] cat-file: fall back to OID lookup when a pack disappears mid-stream "git cat-file --batch-all-objects --unordered" reads each object from the pack and offset the walk handed it, rather than looking it up again. If a concurrent repack removes that pack in between, packed_object_info() calls use_pack(), which cannot reopen the unlinked pack and dies with "packfile ... cannot be accessed" -- even though the object still lives in the pack repack just wrote. Guard the read with is_pack_valid(): if the pack can no longer be opened, clear the pointer so we fall back to a normal object-database lookup, which finds the object in whatever pack now holds it. is_pack_valid() opens and pins the pack's fd, so once it succeeds the pack stays readable even if it is unlinked an instant later. Assisted-by: Claude Opus 4.8 Signed-off-by: Elijah Newren --- builtin/cat-file.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 7cb0466aadb0ff..d157eb6a8f89b2 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -543,6 +543,15 @@ static void batch_object_write(const char *obj_name, if (opt->objects_filter.choice == LOFC_BLOB_LIMIT) data->info.sizep = &data->size; + /* + * With "--batch-all-objects --unordered" we read straight + * from the pack the walk handed us. If a concurrent repack + * removed it, fall back to a normal lookup; is_pack_valid() + * pins the pack's fd, so a passing check stays valid even if + * the pack is unlinked right after. + */ + if (pack && !is_pack_valid(pack)) + pack = NULL; if (pack) ret = packed_object_info(NULL, pack, offset, &data->info); else From ac181a38c6209d189023cf770ef45736c54d9e4f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Wed, 2 Sep 2026 16:56:43 -0700 Subject: [PATCH 3/3] cat-file: snapshot pack indexes before enumerating all objects The previous commit made "git cat-file --batch-all-objects" error out when a concurrent repack removes a pack mid-walk. That is correct but pessimistic on servers that repack often. Reduce how often it triggers by opening every pack index up front, before the walk. An index mmap survives unlink() of the .idx and pack fd pressure (close_one_pack() closes only the pack fd), so the object set is fixed once the indexes are open. For --batch-all-objects this adds no work: the walk opens every index anyway. Like f6b262581a88 (fsck: snapshot default refs before object walk, 2026-01-09), this narrows rather than closes the race; the residue is still caught by the previous commit rather than silently dropped. Assisted-by: Claude Opus 4.8 Signed-off-by: Elijah Newren --- builtin/cat-file.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index d157eb6a8f89b2..b1f8efb30e671c 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -1017,6 +1017,23 @@ static int batch_one_object_oi(const struct object_id *oid, return payload->callback(oid, NULL, 0, payload->payload); } +/* + * Open every pack index up front so the enumeration's object set is + * fixed: an index mmap survives unlink() of the .idx and pack fd + * pressure (close_one_pack() closes only the pack fd). This narrows + * the race with concurrent repacks, like f6b262581a88 (fsck: snapshot + * default refs before object walk, 2026-01-09). + */ +static int snapshot_pack_indexes(void) +{ + struct packed_git *p; + + repo_for_each_pack(the_repository, p) + if (open_pack_index(p)) + return -1; + return 0; +} + static int batch_each_object(struct batch_options *opt, for_each_object_fn callback, unsigned flags, @@ -1084,6 +1101,11 @@ static int batch_objects(struct batch_options *opt) disable_replace_refs(); + if (snapshot_pack_indexes()) { + strbuf_release(&output); + return error(_("unable to enumerate all objects")); + } + cb.opt = opt; cb.expand = &data; cb.scratch = &output;