From 08f7e1cd38b56a893650ddabbd3480cfcc01c78e Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 18 Oct 2026 13:16:41 +0100 Subject: [PATCH 1/3] perf: read a view's reservation tips in one batch A view read each reservation tip in three git processes, its name, its size and its body, so a drain, which views on every move, paid three per reservation per move. The view now reads the ordinary tips, commits within the object bound, in three processes however many there are: one listing of the reservation refs with their objects and types, one `cat-file --batch-check` for the commits' sizes, and one `cat-file --batch` for their bodies, each read as stored and capped. The tips come from origin, written by whoever can push to the reservation namespace, so the batch is read as untrusted and is a shortcut only. A body is taken from it only under the name and size the check read; a tip it does not answer that way (missing, not a commit, past the bound, answered out of shape) is read on its own exactly as before, with every bound and refusal that read applies. The cut stops at the first answer it cannot read, so one bad tip never shifts another's body under its name, and a missing object costs only itself. Nothing outlives the call: each ref is resolved afresh. Tests cut crafted batch answers (a missing object mid-batch, a wrong size or type, an answer out of order, a short read, a body without its newline), hold a batched read of a mixed set (an oversized tip between two readable ones, a malformed one, a blob) equal to each tip read on its own, and pin a view of four tips at two cat-file processes, from eight. One drain test falls from 1,019 git processes to 830, and from 1,107 before the default-branch change it builds on. Resolves: iss-2610180545134593 Assisted-by: Claude:claude-opus-5-5 --- ...-the-implement-loop-package-s-tests-are.md | 6 + internal/core/claim/tipbatch.go | 174 ++++++++++++++++++ internal/core/claim/tipbatch_test.go | 142 ++++++++++++++ internal/core/claim/view.go | 7 +- 4 files changed, 327 insertions(+), 2 deletions(-) rename .abcd/work/issues/{open => resolved}/iss-2610180545134593-the-implement-loop-package-s-tests-are.md (67%) create mode 100644 internal/core/claim/tipbatch.go create mode 100644 internal/core/claim/tipbatch_test.go diff --git a/.abcd/work/issues/open/iss-2610180545134593-the-implement-loop-package-s-tests-are.md b/.abcd/work/issues/resolved/iss-2610180545134593-the-implement-loop-package-s-tests-are.md similarity index 67% rename from .abcd/work/issues/open/iss-2610180545134593-the-implement-loop-package-s-tests-are.md rename to .abcd/work/issues/resolved/iss-2610180545134593-the-implement-loop-package-s-tests-are.md index c1e8c23ddf..03dfab074e 100644 --- a/.abcd/work/issues/open/iss-2610180545134593-the-implement-loop-package-s-tests-are.md +++ b/.abcd/work/issues/resolved/iss-2610180545134593-the-implement-loop-package-s-tests-are.md @@ -10,6 +10,12 @@ origin: researcher-authored production_mode: hand-written found_at: "internal/core/implement/loop" remedy: "Cut the git subprocesses a Drain call makes in the test fixtures, or let the tests that swap no package seam run in parallel, measured against the race step's CPU share" +resolution: "A claim operation names origin's default branch once each side of a fetch, and a view reads its reservation tips in one batch with a per-tip fallback; one drain test fell from 1,107 git processes to 830" +impact: internal --- The implement loop package's tests are bound by git subprocesses: the slowest, TestPollingWithoutAnAnswerNeverHoldsTheIssuePastOneLease, runs Drain 120 times (a 4-minute poll over four 2-hour leases, kept inside the 5-minute take-over margin on purpose) and took 67s plain on a loaded machine, 31s on a quieter one with 3.84s of its own CPU; the next three take about 20s each, and the package is the slowest in both of the ubuntu leg's passes (320-410s plain, 470-650s race on 2026-10-18), with none of its 500 tests parallel. + +## Grounds + +- pursued: the loop package's tests and every drain move spend about a quarter fewer git processes, so the ubuntu leg's race step shortens by a minute or more; a merge-group race step no shorter than before would show it wrong diff --git a/internal/core/claim/tipbatch.go b/internal/core/claim/tipbatch.go new file mode 100644 index 0000000000..7fc692861b --- /dev/null +++ b/internal/core/claim/tipbatch.go @@ -0,0 +1,174 @@ +package claim + +import ( + "context" + "strconv" + "strings" + "time" + + "github.com/intentdriven/abcd/internal/gitutil" +) + +// A view reads every reservation tip it lists, and read one at a time each +// tip cost three git processes: its name, its size and its body +// (iss-2610180545134593). readSeenTips reads the ordinary ones, commits +// within the object bound, in three processes however many there are, and +// every other tip exactly as before. +// +// The tips come from origin, written by whoever can push to the reservation +// namespace, so the batch is read as untrusted. It is a shortcut and nothing +// more: a body is taken from it only under the name and the size the check +// read, and any tip the batch does not answer that way (missing, not a +// commit, past the bound, answered out of shape) is read on its own by +// readSeen, with every bound and refusal that read applies. A tip the batch +// cannot read never shifts another's body under its name, since the cut stops +// at the first answer it cannot read. Nothing it reads outlives the call: a +// ref is resolved to its commit afresh every time. + +// tipRead is one tip as readSeen reads it. +type tipRead struct { + t tip + ok bool + err error +} + +// batchWant is one commit the batch asks for, with the size its check read. +type batchWant struct { + name string + size int +} + +// readSeenTips reads each of refs, reservation refs under TrackingPrefix +// that the caller has already vetted, in order, each as readSeen(root, id, +// ref, tree, fetchedAt, keep) reads it: the same tip, the same ok and the +// same error. +func readSeenTips(root string, refs []string, tree string, fetchedAt time.Time, keep bool) []tipRead { + bodies := batchedTipBodies(root, refs) + out := make([]tipRead, len(refs)) + for i, ref := range refs { + id := strings.TrimPrefix(ref, TrackingPrefix) + if b, ok := bodies[ref]; ok { + // As readTip reads a commit within the bound: parsed, then dated. + h, m := parseTip(b.name, []byte(b.body), tree, reservationParser(id)) + t := tip{head: dated(root, h, fetchedAt, keep), message: m} + out[i] = tipRead{t: seen(root, t, fetchedAt, keep), ok: true} + continue + } + t, ok, err := readSeen(root, id, ref, tree, fetchedAt, keep) + out[i] = tipRead{t: t, ok: ok, err: err} + } + return out +} + +// tipBody is a tip's commit and its object as git stored it. +type tipBody struct { + name string + body string +} + +// batchedTipBodies reads, for each of refs that names a commit within the +// object bound, that commit's object, in three commands: the refs' names and +// types, the commits' types and sizes from their headers (`cat-file +// --batch-check`), then their bodies (`cat-file --batch`), each read as +// stored and capped. A ref it reads no body for is left out, for the caller +// to read on its own; a command that fails leaves out every ref it would have +// read, never refuses the read. +func batchedTipBodies(root string, refs []string) map[string]tipBody { + if len(refs) == 0 { + return nil + } + asked := make(map[string]bool, len(refs)) + for _, r := range refs { + asked[r] = true + } + listing, err := storedGit(root, 1<<20, "for-each-ref", "--format=%(refname) %(objectname) %(objecttype)", TrackingPrefix) + if err != nil { + return nil + } + commitOf := map[string]string{} + var shas []string + inBatch := map[string]bool{} + for _, l := range strings.Split(listing, "\n") { + f := strings.Fields(l) + // Only a ref the caller asked for, named exactly, whose object git + // names in full and calls a commit. + if len(f) != 3 || !asked[f[0]] || f[2] != "commit" || !gitutil.IsFullSHA(f[1]) { + continue + } + commitOf[f[0]] = f[1] + if !inBatch[f[1]] { + inBatch[f[1]] = true + shas = append(shas, f[1]) + } + } + if len(shas) == 0 { + return nil + } + ctx := context.Background() + check, err := storedGitInput(ctx, root, strings.Join(shas, "\n")+"\n", len(shas)*(len(shas[0])+32)+64, + "cat-file", "--batch-check=%(objectname) %(objecttype) %(objectsize)") + if err != nil { + return nil + } + var wants []batchWant + total := 64 + for _, l := range strings.Split(strings.TrimRight(string(check), "\n"), "\n") { + f := strings.Fields(l) + if len(f) != 3 || !inBatch[f[0]] || f[1] != "commit" { + continue + } + n, err := strconv.Atoi(f[2]) + if err != nil || n < 0 || n > maxObjectBytes { + continue // read on its own, which says why it is unreadable + } + wants = append(wants, batchWant{name: f[0], size: n}) + total += n + len(f[0]) + 32 + } + if len(wants) == 0 { + return nil + } + names := make([]string, len(wants)) + for i, w := range wants { + names[i] = w.name + } + objs, err := storedGitInput(ctx, root, strings.Join(names, "\n")+"\n", total, "cat-file", "--batch") + if err != nil { + return nil + } + bodies := splitCommitBatch(string(objs), wants) + out := make(map[string]tipBody, len(bodies)) + for ref, sha := range commitOf { + if b, ok := bodies[sha]; ok { + out[ref] = tipBody{name: sha, body: b} + } + } + return out +} + +// splitCommitBatch cuts git's `cat-file --batch` answer into bodies, keyed by +// object name, for the wants it answers, in order, as commits of exactly the +// size the check read. A want answered as missing costs that want alone. The +// cut stops at the first answer it cannot read as the want it is for (another +// name, type or size, a short read, a body not ended by its newline), so a +// mangled answer never puts a later body under the wrong name: every want +// from there on is left out, for its caller to read on its own. +func splitCommitBatch(out string, wants []batchWant) map[string]string { + got := make(map[string]string, len(wants)) + rest := out + for _, w := range wants { + header, after, ok := strings.Cut(rest, "\n") + if !ok { + break + } + if header == w.name+" missing" { + rest = after + continue + } + if header != w.name+" commit "+strconv.Itoa(w.size) || len(after) < w.size+1 || after[w.size] != '\n' { + break + } + got[w.name] = after[:w.size] + rest = after[w.size+1:] + } + return got +} diff --git a/internal/core/claim/tipbatch_test.go b/internal/core/claim/tipbatch_test.go new file mode 100644 index 0000000000..0ed83bc1e6 --- /dev/null +++ b/internal/core/claim/tipbatch_test.go @@ -0,0 +1,142 @@ +package claim + +import ( + "maps" + "reflect" + "slices" + "strconv" + "strings" + "testing" + "time" +) + +// The reservation tips a view reads come from origin, written by whoever can +// push to the reservation namespace, so reading them in one batch is parsing +// at a trust boundary (iss-2610180545134593). The batch is a shortcut and +// nothing more: a tip it cannot read cleanly is read on its own, as before, +// with every bound and refusal that read applies, and one bad tip never moves +// another's body under its name. + +const ( + shaA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + shaB = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + shaC = "cccccccccccccccccccccccccccccccccccccccc" +) + +// TestTheCommitBatchIsCutOnlyWhereItReadsAsAsked holds splitCommitBatch to +// the answers it may trust: a body is kept only under the name and size the +// check read for it, a missing object costs that object alone, and the first +// answer it cannot read ends the cut, leaving every later object out to be +// read on its own rather than shifted under another's name. +func TestTheCommitBatchIsCutOnlyWhereItReadsAsAsked(t *testing.T) { + wants := []batchWant{{shaA, 3}, {shaB, 4}, {shaC, 2}} + ans := func(name, typ string, body string) string { + return name + " " + typ + " " + strconv.Itoa(len(body)) + "\n" + body + "\n" + } + for _, c := range []struct { + name string + out string + want map[string]string + }{ + {"all three", ans(shaA, "commit", "aaa") + ans(shaB, "commit", "bbbb") + ans(shaC, "commit", "cc"), + map[string]string{shaA: "aaa", shaB: "bbbb", shaC: "cc"}}, + {"one missing in the middle", ans(shaA, "commit", "aaa") + shaB + " missing\n" + ans(shaC, "commit", "cc"), + map[string]string{shaA: "aaa", shaC: "cc"}}, + {"a size other than the check read", ans(shaA, "commit", "aaa") + ans(shaB, "commit", "bbbbbb") + ans(shaC, "commit", "cc"), + map[string]string{shaA: "aaa"}}, + {"a type that is not a commit", ans(shaA, "commit", "aaa") + ans(shaB, "blob", "bbbb") + ans(shaC, "commit", "cc"), + map[string]string{shaA: "aaa"}}, + {"an answer for another object", ans(shaA, "commit", "aaa") + ans(shaC, "commit", "cc") + ans(shaB, "commit", "bbbb"), + map[string]string{shaA: "aaa"}}, + {"a short read", ans(shaA, "commit", "aaa") + shaB + " commit 4\nbb", + map[string]string{shaA: "aaa"}}, + {"a body not ended by a newline", ans(shaA, "commit", "aaa") + shaB + " commit 4\nbbbbX" + ans(shaC, "commit", "cc"), + map[string]string{shaA: "aaa"}}, + {"no answer at all", "", map[string]string{}}, + {"bytes past the last answer", ans(shaA, "commit", "aaa") + ans(shaB, "commit", "bbbb") + ans(shaC, "commit", "cc") + "extra", + map[string]string{shaA: "aaa", shaB: "bbbb", shaC: "cc"}}, + } { + t.Run(c.name, func(t *testing.T) { + if got := splitCommitBatch(c.out, wants); !maps.Equal(got, c.want) { + t.Errorf("splitCommitBatch = %q; want %q", got, c.want) + } + }) + } +} + +// TestABatchedViewReadsEveryTipAsItsOwnReadWould: tips of every shape a writer +// can push, a readable one either side of an oversized one, a malformed one +// and a blob, read in one batch read exactly as each reads on its own. +func TestABatchedViewReadsEveryTipAsItsOwnReadWould(t *testing.T) { + w := newWorld(t) + p := w.clone("1234+octo@users.noreply.github.com", "") + q := w.clone("99+quinn@users.noreply.github.com", "") + if _, err := q.reserve("iss-1", "a"); err != nil { + t.Fatal(err) + } + w.pushHostile("iss-2", "abcd reservation: iss-2\n\n"+strings.Repeat("x", maxObjectBytes+100)+"\n", t0) + w.pushHostile("iss-3", "abcd reservation: iss-3\n\nAbcd-Reservation: iss-3\nAbcd-Claimed-At: now\n", t0) + if _, err := q.reserve("iss-5", "b"); err != nil { + t.Fatal(err) + } + if _, err := Fetch(p.dir, p.opts()); err != nil { + t.Fatal(err) + } + // A blob as a tip: git refuses to push one to a branch, but a copy's own + // remote-tracking ref can name anything. + blob := strings.TrimSpace(p.git("hash-object", "-w", "--", "README.md")) + p.git("update-ref", TrackingPrefix+"iss-4", blob) + tree, err := emptyTree(p.dir) + if err != nil { + t.Fatal(err) + } + var refs []string + for _, id := range []string{"iss-1", "iss-2", "iss-3", "iss-4", "iss-5"} { + refs = append(refs, TrackingPrefix+id) + } + at := t0.Add(time.Minute) + got := readSeenTips(p.dir, refs, tree, at, false) + for i, ref := range refs { + id := strings.TrimPrefix(ref, TrackingPrefix) + tp, ok, err := readSeen(p.dir, id, ref, tree, at, false) + want := tipRead{t: tp, ok: ok, err: err} + if !reflect.DeepEqual(got[i], want) { + t.Errorf("%s batched = %+v; read on its own = %+v", id, got[i], want) + } + } + if got[1].t.Unreadable != reasonObjectTooBig || got[0].t.Unreadable != "" || got[4].t.Unreadable != "" { + t.Errorf("want the oversized tip unreadable and the tips either side of it read: %+v", got) + } +} + +// TestAViewReadsItsTipsInAFixedNumberOfProcesses: a view of several +// reservations reads their tips in one listing, one check and one batch, not +// three git processes per tip. +func TestAViewReadsItsTipsInAFixedNumberOfProcesses(t *testing.T) { + w := newWorld(t) + p := w.clone("1234+octo@users.noreply.github.com", "") + q := w.clone("99+quinn@users.noreply.github.com", "") + for _, id := range []string{"iss-1", "iss-2", "iss-3", "iss-4"} { + if _, err := q.reserve(id, "docs/"+id); err != nil { + t.Fatal(err) + } + } + if _, err := Fetch(p.dir, p.opts()); err != nil { + t.Fatal(err) + } + log := traced(t) + snap, err := View(p.dir, w.clock()) + if err != nil { + t.Fatal(err) + } + if len(snap.Reservations) != 4 { + t.Fatalf("the view lists %d reservations; want 4", len(snap.Reservations)) + } + subs := log.subcommands() + if n := strings.Count(strings.Join(subs, " "), "cat-file"); n != 2 { + t.Errorf("the view ran %d cat-file processes for 4 tips; want 2, one check and one batch: %q", n, subs) + } + if n := len(slices.DeleteFunc(slices.Clone(subs), func(s string) bool { return s != "for-each-ref" })); n > 2 { + t.Errorf("the view ran %d for-each-ref processes; want at most 2, the listing and the tips' names: %q", n, subs) + } +} diff --git a/internal/core/claim/view.go b/internal/core/claim/view.go index c4c88c0a3a..a65ab04792 100644 --- a/internal/core/claim/view.go +++ b/internal/core/claim/view.go @@ -204,9 +204,12 @@ func viewOf(root string, now time.Time, derive bool, only func(subject string) b if dated.IsZero() { dated = now } - for _, ref := range refs { + // The tips in one batch where they read cleanly, each as readSeen reads + // it (iss-2610180545134593). + reads := readSeenTips(root, refs, tree, dated, false) + for i, ref := range refs { id := strings.TrimPrefix(ref, TrackingPrefix) - t, ok, err := readSeen(root, id, ref, tree, dated, false) + t, ok, err := reads[i].t, reads[i].ok, reads[i].err if err != nil { return snap, fmt.Errorf("reading the reservation on %s: %s", id, redacted(err)) } From 3acd3c7f13548154ff562d7384b0fdcb7254aa55 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 18 Oct 2026 13:21:46 +0100 Subject: [PATCH 2/3] fix: read a batched tip trimmed, as a tip read alone is Both reviewers of the batched view found it parted from a tip read on its own: storedGit hands a commit over with trailing whitespace trimmed, and the batch handed parseTip the bytes as git sent them. A writer to the reservation namespace could push a reservation whose message was the message cap plus its newline, or ended in blanks, and the view would read it as unreadable, with no holder or footprint, while every act read it as a live hold. A batched body is now trimmed exactly as storedGit trims it. One batch also reads at most 1 MiB of commit bodies; a tip past that is read on its own, so the memory a view holds stays flat however many reservation refs a writer pushes. A test reads, both ways and held equal, the tips where the two could part: messages either side of the cap, one ending in blanks, commits of exactly the object bound and one past it, two refs on one commit and a tag as a tip, and asserts which of them the batch itself answered. It fails at the cap plus its newline without the trim, and at the blanks with a trim of newlines alone. Refs: iss-2610180545134593 Assisted-by: Claude:claude-opus-5-5 --- internal/core/claim/tipbatch.go | 16 +++- internal/core/claim/tipbatch_test.go | 109 +++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/internal/core/claim/tipbatch.go b/internal/core/claim/tipbatch.go index 7fc692861b..83b423f161 100644 --- a/internal/core/claim/tipbatch.go +++ b/internal/core/claim/tipbatch.go @@ -48,8 +48,11 @@ func readSeenTips(root string, refs []string, tree string, fetchedAt time.Time, for i, ref := range refs { id := strings.TrimPrefix(ref, TrackingPrefix) if b, ok := bodies[ref]; ok { - // As readTip reads a commit within the bound: parsed, then dated. - h, m := parseTip(b.name, []byte(b.body), tree, reservationParser(id)) + // As readTip reads a commit within the bound: the object as + // storedGit hands it over, trailing whitespace trimmed, then + // parsed and dated. An untrimmed body would read a message at the + // cap as past it, and the view would disagree with the act. + h, m := parseTip(b.name, []byte(strings.TrimRight(b.body, " \t\r\n")), tree, reservationParser(id)) t := tip{head: dated(root, h, fetchedAt, keep), message: m} out[i] = tipRead{t: seen(root, t, fetchedAt, keep), ok: true} continue @@ -60,6 +63,12 @@ func readSeenTips(root string, refs []string, tree string, fetchedAt time.Time, return out } +// batchBudget bounds what one batch reads of commit bodies: about a hundred +// and twenty tips at the object bound. A tip past it is read on its own, as +// before, so the memory a view holds stays flat however many reservation +// refs a writer pushes. +const batchBudget = 1 << 20 + // tipBody is a tip's commit and its object as git stored it. type tipBody struct { name string @@ -121,6 +130,9 @@ func batchedTipBodies(root string, refs []string) map[string]tipBody { if err != nil || n < 0 || n > maxObjectBytes { continue // read on its own, which says why it is unreadable } + if total+n+len(f[0])+32 > batchBudget { + continue // past the budget: read on its own + } wants = append(wants, batchWant{name: f[0], size: n}) total += n + len(f[0]) + 32 } diff --git a/internal/core/claim/tipbatch_test.go b/internal/core/claim/tipbatch_test.go index 0ed83bc1e6..7e983b4fa6 100644 --- a/internal/core/claim/tipbatch_test.go +++ b/internal/core/claim/tipbatch_test.go @@ -2,12 +2,17 @@ package claim import ( "maps" + "os" + "os/exec" + "path/filepath" "reflect" "slices" "strconv" "strings" "testing" "time" + + "github.com/intentdriven/abcd/internal/gittest" ) // The reservation tips a view reads come from origin, written by whoever can @@ -107,6 +112,110 @@ func TestABatchedViewReadsEveryTipAsItsOwnReadWould(t *testing.T) { if got[1].t.Unreadable != reasonObjectTooBig || got[0].t.Unreadable != "" || got[4].t.Unreadable != "" { t.Errorf("want the oversized tip unreadable and the tips either side of it read: %+v", got) } + // The batch, not a fallback, read the commits within the bound; the + // oversized commit and the blob it left to be read on their own. + bodies := batchedTipBodies(p.dir, refs) + for i, inBatch := range []bool{true, false, true, false, true} { + if _, ok := bodies[refs[i]]; ok != inBatch { + t.Errorf("%s read in the batch = %v; want %v", refs[i], ok, inBatch) + } + } +} + +// localTip makes a commit on the empty tree with msg, dated t0, in p's own +// object store, and sets id's remote-tracking reservation ref to it: a copy's +// own tracking ref can name anything, which is all a view reads. +func (p *participant) localTip(id, msg string) string { + p.w.t.Helper() + path := filepath.Join(p.w.t.TempDir(), "msg") + if err := os.WriteFile(path, []byte(msg), 0o600); err != nil { + p.w.t.Fatal(err) + } + tree := strings.TrimSpace(p.git("hash-object", "-w", "-t", "tree", "/dev/null")) + date := "@" + strconv.FormatInt(t0.Unix(), 10) + " +0000" + cmd := exec.Command("git", "-C", p.dir, "-c", "user.name=Mal", "-c", "user.email=mal@example.invalid", + "-c", "commit.gpgsign=false", "commit-tree", tree, "-F", path) + cmd.Env = append(gittest.Env(p.w.t), "GIT_AUTHOR_DATE="+date, "GIT_COMMITTER_DATE="+date) + out, err := cmd.Output() + if err != nil { + p.w.t.Fatal(err) + } + sha := strings.TrimSpace(string(out)) + p.git("update-ref", TrackingPrefix+id, sha) + return sha +} + +// TestABatchedReadAgreesWithItsOwnReadAtEveryBound: the tips where a batched +// read could part from a read on its own, each read both ways and held +// equal. Messages either side of the message cap, raw and as git trims +// them (a body of the cap plus its newline reads as at the cap, not past it), +// and one ending in trailing blanks; +// commits of exactly the object bound and one byte past it; two refs on one +// commit; and an annotated tag as a tip. +func TestABatchedReadAgreesWithItsOwnReadAtEveryBound(t *testing.T) { + w := newWorld(t) + p := w.clone("1234+octo@users.noreply.github.com", "") + if _, err := p.reserve("iss-1", "a"); err != nil { + t.Fatal(err) + } + var refs []string + add := func(id string) { refs = append(refs, TrackingPrefix+id) } + add("iss-1") + head := func(id string) string { return "abcd reservation: " + id + "\n\nAbcd-Reservation: " + id + "\n" } + for i, n := range []int{MaxMessageBytes - 1, MaxMessageBytes, MaxMessageBytes + 1, MaxMessageBytes + 2} { + id := "iss-" + strconv.Itoa(10+i) + msg := head(id) + msg += strings.Repeat("x", n-len(msg)-1) + "\n" + p.localTip(id, msg) + add(id) + } + // A reservation whose message ends in trailing blanks: read as stored, the + // blanks are trimmed, and it names its holder; read untrimmed, its last + // trailer would carry them. + p.localTip("iss-15", "abcd reservation: iss-15\n\nAbcd-Reservation: iss-15\nAbcd-Holder: octo\n\n \t\n") + add("iss-15") + // The object bound: a commit of exactly maxObjectBytes, and one past it. + for i, want := range []int{maxObjectBytes, maxObjectBytes + 1} { + id := "iss-" + strconv.Itoa(20+i) + probe := p.localTip(id, head(id)) + size, _ := strconv.Atoi(strings.TrimSpace(p.git("cat-file", "-s", probe))) + p.localTip(id, head(id)+strings.Repeat("y", want-size)) + if got, _ := strconv.Atoi(strings.TrimSpace(p.git("cat-file", "-s", TrackingPrefix+id))); got != want { + t.Fatalf("%s is %d bytes; want %d", id, got, want) + } + add(id) + } + // Two refs on one commit, and an annotated tag as a tip. + p.git("update-ref", TrackingPrefix+"iss-30", strings.TrimSpace(p.git("rev-parse", TrackingPrefix+"iss-1"))) + add("iss-30") + p.git("-c", "user.name=Mal", "-c", "user.email=mal@example.invalid", "tag", "-a", "-m", "a tag", "t31", TrackingPrefix+"iss-1") + p.git("update-ref", TrackingPrefix+"iss-31", strings.TrimSpace(p.git("rev-parse", "t31"))) + add("iss-31") + + tree, err := emptyTree(p.dir) + if err != nil { + t.Fatal(err) + } + at := t0.Add(time.Minute) + got := readSeenTips(p.dir, refs, tree, at, false) + for i, ref := range refs { + id := strings.TrimPrefix(ref, TrackingPrefix) + tp, ok, err := readSeen(p.dir, id, ref, tree, at, false) + if want := (tipRead{t: tp, ok: ok, err: err}); !reflect.DeepEqual(got[i], want) { + t.Errorf("%s batched = %+v\n read on its own = %+v", id, got[i], want) + } + } + bodies := batchedTipBodies(p.dir, refs) + for _, id := range []string{"iss-1", "iss-10", "iss-11", "iss-12", "iss-13", "iss-15", "iss-20", "iss-30"} { + if _, ok := bodies[TrackingPrefix+id]; !ok { + t.Errorf("%s was not read in the batch; the comparison above did not exercise it", id) + } + } + for _, id := range []string{"iss-21", "iss-31"} { + if _, ok := bodies[TrackingPrefix+id]; ok { + t.Errorf("%s was read in the batch; past the bound or not a commit, it is read on its own", id) + } + } } // TestAViewReadsItsTipsInAFixedNumberOfProcesses: a view of several From c73fd9cb0e089231ca306387eb5100215eaae59b Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 18 Oct 2026 13:24:49 +0100 Subject: [PATCH 3/3] test: hold a tip past the batch budget to its own read Both reviewers of the batched view asked for the budget fallback to be pinned, not only probed. The budget is now a parameter below readSeenTips, which passes the 1 MiB constant, and a test with room for two of five tips holds the first two in the batch and every tip, inside the budget or past it, equal to its read on its own. The budget comment names the count it allows at the object bound, about 126. Refs: iss-2610180545134593 Assisted-by: Claude:claude-opus-5-5 --- internal/core/claim/tipbatch.go | 21 ++++++++----- internal/core/claim/tipbatch_test.go | 45 ++++++++++++++++++++++++++-- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/internal/core/claim/tipbatch.go b/internal/core/claim/tipbatch.go index 83b423f161..992e043efb 100644 --- a/internal/core/claim/tipbatch.go +++ b/internal/core/claim/tipbatch.go @@ -43,7 +43,13 @@ type batchWant struct { // ref, tree, fetchedAt, keep) reads it: the same tip, the same ok and the // same error. func readSeenTips(root string, refs []string, tree string, fetchedAt time.Time, keep bool) []tipRead { - bodies := batchedTipBodies(root, refs) + return readSeenTipsWithin(root, refs, tree, fetchedAt, keep, batchBudget) +} + +// readSeenTipsWithin is readSeenTips with the batch's budget named: a test +// holds a small one to prove what falls past it is read on its own. +func readSeenTipsWithin(root string, refs []string, tree string, fetchedAt time.Time, keep bool, budget int) []tipRead { + bodies := batchedTipBodies(root, refs, budget) out := make([]tipRead, len(refs)) for i, ref := range refs { id := strings.TrimPrefix(ref, TrackingPrefix) @@ -63,8 +69,8 @@ func readSeenTips(root string, refs []string, tree string, fetchedAt time.Time, return out } -// batchBudget bounds what one batch reads of commit bodies: about a hundred -// and twenty tips at the object bound. A tip past it is read on its own, as +// batchBudget bounds what one batch reads of commit bodies: about 126 tips at +// the object bound. A tip past it is read on its own, as // before, so the memory a view holds stays flat however many reservation // refs a writer pushes. const batchBudget = 1 << 20 @@ -80,9 +86,10 @@ type tipBody struct { // types, the commits' types and sizes from their headers (`cat-file // --batch-check`), then their bodies (`cat-file --batch`), each read as // stored and capped. A ref it reads no body for is left out, for the caller -// to read on its own; a command that fails leaves out every ref it would have -// read, never refuses the read. -func batchedTipBodies(root string, refs []string) map[string]tipBody { +// to read on its own, as is one past budget, which bounds the bodies the +// batch reads; a command that fails leaves out every ref it would have read, +// never refuses the read. +func batchedTipBodies(root string, refs []string, budget int) map[string]tipBody { if len(refs) == 0 { return nil } @@ -130,7 +137,7 @@ func batchedTipBodies(root string, refs []string) map[string]tipBody { if err != nil || n < 0 || n > maxObjectBytes { continue // read on its own, which says why it is unreadable } - if total+n+len(f[0])+32 > batchBudget { + if total+n+len(f[0])+32 > budget { continue // past the budget: read on its own } wants = append(wants, batchWant{name: f[0], size: n}) diff --git a/internal/core/claim/tipbatch_test.go b/internal/core/claim/tipbatch_test.go index 7e983b4fa6..8b5005af12 100644 --- a/internal/core/claim/tipbatch_test.go +++ b/internal/core/claim/tipbatch_test.go @@ -114,7 +114,7 @@ func TestABatchedViewReadsEveryTipAsItsOwnReadWould(t *testing.T) { } // The batch, not a fallback, read the commits within the bound; the // oversized commit and the blob it left to be read on their own. - bodies := batchedTipBodies(p.dir, refs) + bodies := batchedTipBodies(p.dir, refs, batchBudget) for i, inBatch := range []bool{true, false, true, false, true} { if _, ok := bodies[refs[i]]; ok != inBatch { t.Errorf("%s read in the batch = %v; want %v", refs[i], ok, inBatch) @@ -205,7 +205,7 @@ func TestABatchedReadAgreesWithItsOwnReadAtEveryBound(t *testing.T) { t.Errorf("%s batched = %+v\n read on its own = %+v", id, got[i], want) } } - bodies := batchedTipBodies(p.dir, refs) + bodies := batchedTipBodies(p.dir, refs, batchBudget) for _, id := range []string{"iss-1", "iss-10", "iss-11", "iss-12", "iss-13", "iss-15", "iss-20", "iss-30"} { if _, ok := bodies[TrackingPrefix+id]; !ok { t.Errorf("%s was not read in the batch; the comparison above did not exercise it", id) @@ -249,3 +249,44 @@ func TestAViewReadsItsTipsInAFixedNumberOfProcesses(t *testing.T) { t.Errorf("the view ran %d for-each-ref processes; want at most 2, the listing and the tips' names: %q", n, subs) } } + +// TestATipPastTheBatchBudgetIsReadOnItsOwn: the budget bounds the bodies one +// batch reads, and a tip past it is read on its own and reads the same. With +// room for two of five tips, the first two are read in the batch and the +// rest on their own, all five as each reads alone. +func TestATipPastTheBatchBudgetIsReadOnItsOwn(t *testing.T) { + w := newWorld(t) + p := w.clone("1234+octo@users.noreply.github.com", "") + if _, err := p.reserve("iss-1", "a"); err != nil { + t.Fatal(err) + } + var refs []string + for i := 1; i <= 5; i++ { + id := "iss-" + strconv.Itoa(40+i) + p.localTip(id, "abcd reservation: "+id+"\n\nAbcd-Reservation: "+id+"\n"+strings.Repeat("z", 200)+"\n") + refs = append(refs, TrackingPrefix+id) + } + size, _ := strconv.Atoi(strings.TrimSpace(p.git("cat-file", "-s", refs[0]))) + // The batch's own allowance (64) and two tips' (each its size, its name + // and 32): room for two, not three. + budget := 64 + 2*(size+40+32) + size/2 + bodies := batchedTipBodies(p.dir, refs, budget) + for i, ref := range refs { + if _, ok := bodies[ref]; ok != (i < 2) { + t.Errorf("%s read in the batch = %v; want %v with room for two", ref, ok, i < 2) + } + } + tree, err := emptyTree(p.dir) + if err != nil { + t.Fatal(err) + } + at := t0.Add(time.Minute) + got := readSeenTipsWithin(p.dir, refs, tree, at, false, budget) + for i, ref := range refs { + id := strings.TrimPrefix(ref, TrackingPrefix) + tp, ok, err := readSeen(p.dir, id, ref, tree, at, false) + if want := (tipRead{t: tp, ok: ok, err: err}); !reflect.DeepEqual(got[i], want) { + t.Errorf("%s within the budget = %+v\n read on its own = %+v", id, got[i], want) + } + } +}