diff --git a/modules/mesh-delivery-forge-bridge/internal/bridge/bridge.go b/modules/mesh-delivery-forge-bridge/internal/bridge/bridge.go index a8669bb..19fd944 100644 --- a/modules/mesh-delivery-forge-bridge/internal/bridge/bridge.go +++ b/modules/mesh-delivery-forge-bridge/internal/bridge/bridge.go @@ -203,6 +203,7 @@ const MaxReadsInFlight = 8 type read struct { dir, path string manifest bool + asked bool raw []byte err error } @@ -216,6 +217,14 @@ type read struct { // order the calls finish in, and the error is that of the first failing file in the order of dirs and LockFiles. After // a failure no further file is asked for, and inputsOf returns only when every call it started has returned. func inputsOf(owner, repo, ref string, dirs []string, files Files) (manifests map[string]string, locks map[string]map[string]string, err error) { + manifests, locks, _, err = inputsRead(owner, repo, ref, dirs, files) + return manifests, locks, err +} + +// inputsRead is inputsOf, and also answers, on a failure too, the directories whose manifest and lock files were all +// read, with their manifests and lock hashes: a merge keeps them by their tree, so its next try reads only the rest +// (issue 1126). +func inputsRead(owner, repo, ref string, dirs []string, files Files) (manifests map[string]string, locks map[string]map[string]string, whole []string, err error) { at := func(dir, name string) string { if dir == "" { return name @@ -244,6 +253,7 @@ func inputsOf(owner, repo, ref string, dirs []string, files Files) (manifests ma <-slots break } + r.asked = true wg.Add(1) go func(r *read) { defer func() { <-slots; wg.Done() }() @@ -258,10 +268,16 @@ func inputsOf(owner, repo, ref string, dirs []string, files Files) (manifests ma wg.Wait() manifests = map[string]string{} locks = map[string]map[string]string{} + broken := map[string]bool{} // a directory with a file not read (failed, or never asked after a failure) for _, r := range reads { switch { case r.err != nil && (r.manifest || !errors.Is(r.err, ErrNotFound)): - return nil, nil, fmt.Errorf("%s: %w", r.path, r.err) + broken[r.dir] = true + if err == nil { + err = fmt.Errorf("%s: %w", r.path, r.err) + } + case r.err == nil && r.raw == nil && !r.asked: + broken[r.dir] = true case r.err != nil: // a lock file the forge does not have case r.manifest: manifests[r.dir] = string(r.raw) @@ -273,13 +289,25 @@ func inputsOf(owner, repo, ref string, dirs []string, files Files) (manifests ma locks[r.dir][path.Base(r.path)] = hex.EncodeToString(sum[:]) } } + for _, dir := range dirs { + if !broken[dir] { + whole = append(whole, dir) + } + } + if err != nil { + for dir := range broken { + delete(manifests, dir) + delete(locks, dir) + } + return manifests, locks, whole, err + } if len(manifests) == 0 { manifests = nil } if len(locks) == 0 { locks = nil } - return manifests, locks, nil + return manifests, locks, whole, nil } // merged is the merged message of a merge into the trunk: the trees of the merge commit, asked once. @@ -301,8 +329,9 @@ func merged(subject string, p pull, trees Trees, files Files, list Listed, kept } slices.Sort(names) manifests, locks, toRead := keptInputs(names, dirs, kept) - read, readLocks, err := inputsOf(p.Owner, p.Repo, p.MergeSHA, toRead, files) + read, readLocks, whole, err := inputsRead(p.Owner, p.Repo, p.MergeSHA, toRead, files) if err != nil { + keep(whole, dirs, read, readLocks, kept) // what was read whole is not asked again at the next try return Message{}, fmt.Errorf("%w: %s: the manifests of %s at the merge commit %s (%d of %d module directories read from the forge, the others kept by their tree): %v", ErrRetry, subject, repository, p.MergeSHA, len(toRead), len(names), err) } keep(toRead, dirs, read, readLocks, kept) diff --git a/modules/mesh-delivery-forge-bridge/internal/bridge/files_test.go b/modules/mesh-delivery-forge-bridge/internal/bridge/files_test.go index a963a19..aaecb3d 100644 --- a/modules/mesh-delivery-forge-bridge/internal/bridge/files_test.go +++ b/modules/mesh-delivery-forge-bridge/internal/bridge/files_test.go @@ -416,13 +416,26 @@ func TestAMergeReadsOnlyTheDirectoriesWhoseTreeChanged(t *testing.T) { if string(first.Data) == string(next.Data) { t.Error("the second merge's message did not change") } - // A failed read keeps nothing of the directories it read, so the next try reads them again. + // A failed read keeps the directories it read whole and nothing of the one that failed, so the next try reads only the + // rest: a merge too large for one try's bound converges over its tries. kept2 := memKept{} failing := &forgeFiles{have: forge.have, errs: map[string]error{"modules/b/module.json": errors.New("context deadline exceeded")}} if _, err := ConvertKept(mergedSubject, mergedBody(nil), treesOf(&calls, "r1", trees, nil), failing.get, nil, kept2); !errors.Is(err, ErrRetry) { t.Fatalf("err = %v, want ErrRetry", err) } - if len(kept2) != 0 { - t.Errorf("a failed read kept %v", kept2) + if _, ok := kept2["t1"]; !ok || len(kept2) != 1 { // modules/a's files are asked first, before the failure is seen + t.Errorf("a failed read kept %v, want modules/a's tree t1 alone", kept2) + } + failing.calls = nil + failing.errs = nil + again, err := ConvertKept(mergedSubject, mergedBody(nil), treesOf(&calls, "r1", trees, nil), failing.get, nil, kept2) + if err != nil { + t.Fatal(err) + } + if string(again.Data) != string(whole.Data) { + t.Errorf("the message of the try after a failure differs from one read whole") + } + if len(failing.calls) != 3 { + t.Errorf("the try after a failure read %d files, want only modules/b's 3", len(failing.calls)) } }