Fail mirroring more gracefully:

* reuse recoverable error checks across mirror_pull
* add new cases for 'cannot lock ref/not our ref' (race condition in fetch) and 'Unable to create/lock"
* move lfs sync right after commit graph write, and before other maintenance which may fail
* try a prune for 'broken reference' as well as 'not our ref'
* always sync LFS right after commit graph write, and before other maintenance which may fail

Author: rremer

services/mirror/mirror_pull.go

@@ -235,6 +235,24 @@ func pruneBrokenReferences(ctx context.Context,
 	return pruneErr
 }
 
+// checkRecoverableSyncError takes an error message from a git fetch command and returns false if it should be a fatal/blocking error
+func checkRecoverableSyncError(stderrMessage string) bool {
+	switch {
+	case strings.Contains(stderrMessage, "unable to resolve reference") && strings.Contains(stderrMessage, "reference broken"):
+		return true
+	case strings.Contains(stderrMessage, "remote error") && strings.Contains(stderrMessage, "not our ref"):
+		return true
+	case strings.Contains(stderrMessage, "cannot lock ref") && strings.Contains(stderrMessage, "but expected"):
+		return true
+	case strings.Contains(stderrMessage, "cannot lock ref") && strings.Contains(stderrMessage, "unable to resolve reference"):
+		return true
+	case strings.Contains(stderrMessage, "Unable to create") && strings.Contains(stderrMessage, ".lock"):
+		return true
+	default:
+		return false
+	}
+}
+
 // runSync returns true if sync finished without error.
 func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bool) {
 	repoPath := m.Repo.RepoPath()
@@ -275,7 +293,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 		stdoutMessage := util.SanitizeCredentialURLs(stdout)
 
 		// Now check if the error is a resolve reference due to broken reference
-		if strings.Contains(stderr, "unable to resolve reference") && strings.Contains(stderr, "reference broken") {
+		if checkRecoverableSyncError(stderr) {
 			log.Warn("SyncMirrors [repo: %-v]: failed to update mirror repository due to broken references:\nStdout: %s\nStderr: %s\nErr: %v\nAttempting Prune", m.Repo, stdoutMessage, stderrMessage, err)
 			err = nil
 
@@ -324,6 +342,15 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 		return nil, false
 	}
 
+	if m.LFS && setting.LFS.StartServer {
+		log.Trace("SyncMirrors [repo: %-v]: syncing LFS objects...", m.Repo)
+		endpoint := lfs.DetermineEndpoint(remoteURL.String(), m.LFSEndpoint)
+		lfsClient := lfs.NewClient(endpoint, nil)
+		if err = repo_module.StoreMissingLfsObjectsInRepository(ctx, m.Repo, gitRepo, lfsClient); err != nil {
+			log.Error("SyncMirrors [repo: %-v]: failed to synchronize LFS objects for repository: %v", m.Repo, err)
+		}
+	}
+
 	log.Trace("SyncMirrors [repo: %-v]: syncing branches...", m.Repo)
 	if _, err = repo_module.SyncRepoBranchesWithRepo(ctx, m.Repo, gitRepo, 0); err != nil {
 		log.Error("SyncMirrors [repo: %-v]: failed to synchronize branches: %v", m.Repo, err)
@@ -333,15 +360,6 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 	if err = repo_module.SyncReleasesWithTags(ctx, m.Repo, gitRepo); err != nil {
 		log.Error("SyncMirrors [repo: %-v]: failed to synchronize tags to releases: %v", m.Repo, err)
 	}
-
-	if m.LFS && setting.LFS.StartServer {
-		log.Trace("SyncMirrors [repo: %-v]: syncing LFS objects...", m.Repo)
-		endpoint := lfs.DetermineEndpoint(remoteURL.String(), m.LFSEndpoint)
-		lfsClient := lfs.NewClient(endpoint, nil)
-		if err = repo_module.StoreMissingLfsObjectsInRepository(ctx, m.Repo, gitRepo, lfsClient); err != nil {
-			log.Error("SyncMirrors [repo: %-v]: failed to synchronize LFS objects for repository: %v", m.Repo, err)
-		}
-	}
 	gitRepo.Close()
 
 	log.Trace("SyncMirrors [repo: %-v]: updating size of repository", m.Repo)
@@ -368,7 +386,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 			stdoutMessage := util.SanitizeCredentialURLs(stdout)
 
 			// Now check if the error is a resolve reference due to broken reference
-			if strings.Contains(stderrMessage, "unable to resolve reference") && strings.Contains(stderrMessage, "reference broken") {
+			if checkRecoverableSyncError(stderrMessage) {
 				log.Warn("SyncMirrors [repo: %-v Wiki]: failed to update mirror wiki repository due to broken references:\nStdout: %s\nStderr: %s\nErr: %v\nAttempting Prune", m.Repo, stdoutMessage, stderrMessage, err)
 				err = nil
 

services/mirror/mirror_test.go renamed to services/mirror/mirror_pull_test.go

@@ -64,3 +64,48 @@ func Test_parseRemoteUpdateOutput(t *testing.T) {
 	assert.EqualValues(t, "1c97ebc746", results[9].oldCommitID)
 	assert.EqualValues(t, "976d27d52f", results[9].newCommitID)
 }
+
+func Test_checkRecoverableSyncError(t *testing.T) {
+
+	recoverable_description := " should be marked as recoverable."
+	fatal_description := " should be considered fatal and not recoverable."
+
+	// would exit status 128
+	description := "A race condition in http git-fetch where certain refs were listed on the remote and are no longer there"
+	stderr := "fatal: remote error: upload-pack: not our ref 988881adc9fc3655077dc2d4d757d480b5ea0e11"
+	assert.True(t, checkRecoverableSyncError(stderr), description+recoverable_description)
+
+	// would exit status 1
+	description = "A race condition where a local gc/prune removes a named ref during a git-fetch"
+	stderr = "cannot lock ref 'refs/pull/123456/merge': unable to resolve reference 'refs/pull/134153/merge'"
+	assert.True(t, checkRecoverableSyncError(stderr), description+recoverable_description)
+
+	description = "A race condition in http git-fetch where named refs were listed on the remote and are no longer there"
+	stderr = "error: cannot lock ref 'refs/remotes/origin/foo': unable to resolve reference 'refs/remotes/origin/foo': reference broken"
+	assert.True(t, checkRecoverableSyncError(stderr), description+recoverable_description)
+
+	// would exit status 128
+	description = "A race condition in http git-fetch where named refs were force-pushed during the update"
+	stderr = "error: cannot lock ref 'refs/pull/123456/merge': is at 988881adc9fc3655077dc2d4d757d480b5ea0e11 but expected 7f894307ffc9553edbd0b671cab829786866f7b2"
+	assert.True(t, checkRecoverableSyncError(stderr), description+recoverable_description)
+
+	// would exit status 128
+	description = "A race condition with other local git operations, such as git-maintenance,"
+	stderr = "fatal: Unable to create '/data/gitea-repositories/foo-org/bar-repo.git/./objects/info/commit-graphs/commit-graph-chain.lock': File exists."
+	assert.True(t, checkRecoverableSyncError(stderr), description+recoverable_description)
+
+	// would exit status 128
+	description = "Missing or unauthorized credentials"
+	stderr = "fatal: Authentication failed for 'https://example.com/foo-does-not-exist/bar.git/'"
+	assert.False(t, checkRecoverableSyncError(stderr), description+fatal_description)
+
+	// would exit status 128
+	description = "A non-existent remote repository"
+	stderr = "fatal: Could not read from remote repository."
+	assert.False(t, checkRecoverableSyncError(stderr), description+fatal_description)
+
+	// would exit status 128
+	description = "A non-functioning proxy"
+	stderr = "fatal: unable to access 'https://example.com/foo-does-not-exist/bar.git/': Failed to connect to configured-https-proxy port 1080 after 0 ms: Couldn't connect to server"
+	assert.False(t, checkRecoverableSyncError(stderr), description+fatal_description)
+}