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..992e043efb --- /dev/null +++ b/internal/core/claim/tipbatch.go @@ -0,0 +1,193 @@ +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 { + 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) + if b, ok := bodies[ref]; ok { + // 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 + } + t, ok, err := readSeen(root, id, ref, tree, fetchedAt, keep) + out[i] = tipRead{t: t, ok: ok, err: err} + } + return out +} + +// 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 + +// 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, 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 + } + 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 + } + 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}) + 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..8b5005af12 --- /dev/null +++ b/internal/core/claim/tipbatch_test.go @@ -0,0 +1,292 @@ +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 +// 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) + } + // 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, 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) + } + } +} + +// 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, 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) + } + } + 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 +// 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) + } +} + +// 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) + } + } +} 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)) }