Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lfs/transfer_queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ func (q *TransferQueue) individualApiRoutine(apiWaiter chan interface{}) {
for t := range q.apic {
obj, err := t.LegacyCheck()
if err != nil {
if q.canRetryObject(obj.Oid, err) {
if q.canRetryObject(t.Oid(), err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that this fixes the issue, and that the new test-legacy-retries would catch it if it still was present. Thanks!

q.retry(t)
} else {
q.errorc <- err
Expand Down
44 changes: 43 additions & 1 deletion test/cmd/lfstest-gitserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ var (
"status-storage-403", "status-storage-404", "status-storage-410", "status-storage-422", "status-storage-500", "status-storage-503",
"status-legacy-404", "status-legacy-410", "status-legacy-422", "status-legacy-403", "status-legacy-500",
"status-batch-resume-206", "batch-resume-fail-fallback", "return-expired-action", "return-invalid-size",
"object-authenticated",
"object-authenticated", "legacy-download-check-retry", "legacy-upload-check-retry",
}
)

Expand Down Expand Up @@ -182,6 +182,12 @@ func lfsPostHandler(w http.ResponseWriter, r *http.Request, id, repo string) {
io.Copy(ioutil.Discard, r.Body)
r.Body.Close()

if retries, ok := incrementRetriesFor("upload", repo, obj.Oid, true); ok && retries < 3 {
w.WriteHeader(502)
w.Write([]byte("malformed contents"))
return
}

debug(id, "REQUEST")
debug(id, buf.String())
debug(id, "OID: %s", obj.Oid)
Expand Down Expand Up @@ -236,10 +242,46 @@ func lfsPostHandler(w http.ResponseWriter, r *http.Request, id, repo string) {
w.Write(by)
}

var (
legacyRetries = make(map[string]uint32)
legacyRetriesMu sync.Mutex
)

func incrementRetriesFor(direction, repo, oid string, check bool) (after uint32, ok bool) {
var fmtStr string
if check {
fmtStr = "legacy-%s-check-retry"
} else {
fmtStr = "legacy-%s-retry"
}

if oidHandlers[oid] != fmt.Sprintf(fmtStr, direction) {
return 0, false
}

legacyRetriesMu.Lock()
defer legacyRetriesMu.Unlock()

retryKey := strings.Join([]string{direction, repo, oid}, ":")

legacyRetries[retryKey]++
retries := legacyRetries[retryKey]

return retries, true
}

func lfsGetHandler(w http.ResponseWriter, r *http.Request, id, repo string) {
parts := strings.Split(r.URL.Path, "/")
oid := parts[len(parts)-1]

if r.Header.Get("X-Ignore-Retries") != "true" {
if retries, ok := incrementRetriesFor("download", repo, oid, true); ok && retries < 3 {
w.WriteHeader(502)
w.Write([]byte("malformed contents"))
return
}
}

// Support delete for testing
if len(parts) > 1 && parts[len(parts)-2] == "delete" {
largeObjects.Delete(repo, oid)
Expand Down
63 changes: 63 additions & 0 deletions test/test-legacy-retries.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#!/usr/bin/env bash

. "test/testlib.sh"

begin_test "legacy upload check causes retries"
(
set -e

reponame="legacy-upload-check-retry"
setup_remote_repo "$reponame"
clone_repo "$reponame" repo-upload
git config --local lfs.batch false

contents="legacy-upload-check-retry"
oid="$(calc_oid "$contents")"
printf "$contents" > a.dat

git lfs track "*.dat"
git add .gitattributes a.dat
git commit -m "initial commit"

git config --local lfs.transfer.maxretries 3
git push origin master

assert_server_object "$reponame" "$oid"
)
end_test

begin_test "legacy download check causes retries"
(
set -e

reponame="legacy-download-check-retry"
setup_remote_repo "$reponame"
clone_repo "$reponame" repo-download

contents="legacy-download-check-retry"
oid="$(calc_oid "$contents")"
printf "$contents" > a.dat

git lfs track "*.dat"
git add .gitattributes a.dat
git commit -m "initial commit"

git push origin master
assert_server_object "$reponame" "$oid"

pushd ..
git \
-c "filter.lfs.smudge=cat" \
-c "filter.lfs.required=false" \
clone "$GITSERVER/$reponame" "$reponame-assert"

cd "$reponame-assert"

git config credential.helper lfstest
git config --local lfs.batch false
git config --local lfs.transfer.maxretries 2

git lfs pull origin
popd
)
end_test
6 changes: 4 additions & 2 deletions test/testhelpers.sh
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ refute_server_object() {
curl -v "$GITSERVER/$reponame.git/info/lfs/objects/$oid" \
-u "user:pass" \
-o http.json \
-H "Accept: application/vnd.git-lfs+json" 2>&1 |
-H "Accept: application/vnd.git-lfs+json" \
-H "X-Ignore-Retries: true" 2>&1 |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

X-Ignore-Retries is added as a header value here so as to not fail these assert_server_object when the objects that are being looked for have a failure count assosciated with them.

tee http.log

grep "404 Not Found" http.log
Expand Down Expand Up @@ -100,7 +101,8 @@ assert_server_object() {
curl -v "$GITSERVER/$reponame.git/info/lfs/objects/$oid" \
-u "user:pass" \
-o http.json \
-H "Accept: application/vnd.git-lfs+json" 2>&1 |
-H "Accept: application/vnd.git-lfs+json" \
-H "X-Ignore-Retries: true" 2>&1 |
tee http.log
grep "200 OK" http.log

Expand Down