1125 api failures and race conditions with concurrent publish update operations

.github/workflows/ci.yml

@@ -22,23 +22,10 @@ jobs:
   build:
     name: test
     runs-on: ubuntu-20.04
-    continue-on-error: ${{ matrix.allow_failure }}
+    continue-on-error: false
     timeout-minutes: 30
     strategy:
       fail-fast: true
-      matrix:
-        go: [1.17, 1.18, 1.19]
-        run_long_tests: [no]
-        allow_failure: [false]
-        include:
-          # Disable this for now as it's not clear how to select the latest master branch
-          # version of go using actions/setup-go@v2.
-          # - go: master
-          #   run_long_tests: no
-          #   allow_failure: true
-          - go: 1.18
-            run_long_tests: yes
-            allow_failure: false
 
     env:
       NO_FTP_ACCESS: yes
@@ -53,7 +40,7 @@ jobs:
       - name: Setup Go
         uses: actions/setup-go@v3
         with:
-          go-version: ${{ matrix.go }}
+          go-version: '1.20'
 
       - name: Get aptly version
         run: |
@@ -63,6 +50,8 @@ jobs:
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v3
         with:
+          # FIXME: https://github.com/golangci/golangci-lint-action/issues/677
+          skip-pkg-cache: true
           version: v1.51.2
 
       - name: Setup Python
@@ -89,21 +78,19 @@ jobs:
 
       - name: Make
         env:
-          RUN_LONG_TESTS: ${{ matrix.run_long_tests }}
+          RUN_LONG_TESTS: yes
           AZURE_STORAGE_ENDPOINT: "127.0.0.1:10000"
           AZURE_STORAGE_ACCOUNT: "devstoreaccount1"
           AZURE_STORAGE_ACCESS_KEY: "Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw=="
         run: |
           COVERAGE_DIR=${{ runner.temp }} make
 
       - name: Merge code coverage
-        if: matrix.run_long_tests == 'yes'
         run: |
           go install github.com/wadey/gocovmerge@latest
           ~/go/bin/gocovmerge unit.out ${{ runner.temp }}/*.out > coverage.txt
 
       - name: Upload code coverage
-        if: matrix.run_long_tests == 'yes'
         uses: codecov/codecov-action@v2
         with:
           token: ${{ secrets.CODECOV_TOKEN }}

api/api.go

@@ -154,27 +154,50 @@ func truthy(value interface{}) bool {
 func maybeRunTaskInBackground(c *gin.Context, name string, resources []string, proc task.Process) {
 	// Run this task in background if configured globally or per-request
 	background := truthy(c.DefaultQuery("_async", strconv.FormatBool(context.Config().AsyncAPI)))
+
 	if background {
 		log.Info().Msg("Executing task asynchronously")
-		task, conflictErr := runTaskInBackground(name, resources, proc)
-		if conflictErr != nil {
-			AbortWithJSONError(c, 409, conflictErr)
-			return
-		}
-		c.JSON(202, task)
 	} else {
 		log.Info().Msg("Executing task synchronously")
-		out := context.Progress()
-		detail := task.Detail{}
-		retValue, err := proc(out, &detail)
+	}
+
+	// Always run task in background because we need to take
+	// care to not use the same resources twice.
+	// Resource management is currently only implemented for
+	// the async API. However when two API requests are made
+	// concurrently, no reseources are checked and there
+	// might be race conditions.
+	task, conflictErr := runTaskInBackground(name, resources, proc)
+	if conflictErr != nil {
+		AbortWithJSONError(c, 409, conflictErr)
+		return
+	}
+
+	if background {
+		c.JSON(202, task)
+	} else {
+		task, err := context.TaskList().WaitForTaskByID(task.ID)
+		if err != nil {
+			AbortWithJSONError(c, 500, err)
+			return
+		}
+
+		retValue, err := context.TaskList().GetTaskReturnValueByID(task.ID)
 		if err != nil {
-			AbortWithJSONError(c, retValue.Code, err)
+			AbortWithJSONError(c, 500, fmt.Errorf("error: could not get returnvalue of finished task %v: %v", task.ID, err))
 			return
 		}
+
+		detail, err := context.TaskList().GetTaskDetailByID(task.ID)
+		if err != nil {
+			AbortWithJSONError(c, 500, fmt.Errorf("error: could not get details of finished task %v: %v", task.ID, err))
+			return
+		}
+
 		if retValue != nil {
-			c.JSON(retValue.Code, retValue.Value)
+			c.JSON(retValue.Code, detail)
 		} else {
-			c.JSON(http.StatusOK, nil)
+			c.JSON(http.StatusOK, detail)
 		}
 	}
 }

api/mirror.go

@@ -553,4 +553,5 @@ func apiMirrorsUpdate(c *gin.Context) {
 		log.Info().Msgf("%s: Mirror updated successfully!\n", b.Name)
 		return &task.ProcessReturnValue{Code: http.StatusNoContent, Value: nil}, nil
 	})
+	c.JSON(204, gin.H{"task": "OK"})
 }

go.mod

@@ -1,6 +1,6 @@
 module github.com/aptly-dev/aptly
 
-go 1.16
+go 1.20
 
 require (
 	github.com/AlekSi/pointer v1.0.0
@@ -10,16 +10,11 @@ require (
 	github.com/aws/aws-sdk-go v1.34.0
 	github.com/cavaliergopher/grab/v3 v3.0.1
 	github.com/cheggaaa/pb v1.0.10
-	github.com/fatih/color v1.7.0 // indirect
 	github.com/gin-gonic/gin v1.7.7
-	github.com/go-playground/validator/v10 v10.10.1 // indirect
 	github.com/h2non/filetype v1.0.5
-	github.com/jlaffaye/ftp v0.0.0-20180404123514-2403248fa8cc // indirect
 	github.com/kjk/lzma v0.0.0-20161016003348-3fd93898850d
 	github.com/klauspost/compress v1.13.6
 	github.com/klauspost/pgzip v1.2.5
-	github.com/mattn/go-colorable v0.1.13 // indirect
-	github.com/mattn/go-runewidth v0.0.2 // indirect
 	github.com/mattn/go-shellwords v1.0.2
 	github.com/mkrautz/goar v0.0.0-20150919110319-282caa8bd9da
 	github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f
@@ -28,22 +23,57 @@ require (
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.12.1
 	github.com/rs/zerolog v1.28.0
-	github.com/smartystreets/gunit v1.0.4 // indirect
 	github.com/smira/commander v0.0.0-20140515201010-f408b00e68d5
 	github.com/smira/flag v0.0.0-20170926215700-695ea5e84e76
 	github.com/smira/go-aws-auth v0.0.0-20180731211914-8b73995fd8d1
 	github.com/smira/go-ftp-protocol v0.0.0-20140829150050-066b75c2b70d
-	github.com/smira/go-xz v0.1.0
-	github.com/stretchr/testify v1.8.0 // indirect
+	github.com/smira/go-xz v0.0.0-20220607140411-c2a07d4bedda
 	github.com/syndtr/goleveldb v1.0.1-0.20190923125748-758128399b1d
 	github.com/ugorji/go/codec v1.2.7
 	github.com/wsxiaoys/terminal v0.0.0-20160513160801-0940f3fc43a0
 	golang.org/x/crypto v0.0.0-20220331220935-ae2d96664a29
 	golang.org/x/sys v0.3.0
 	golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1
 	golang.org/x/time v0.0.0-20211116232009-f0f3c7e86c11
-	google.golang.org/protobuf v1.28.0 // indirect
 	gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c
+)
+
+require (
+	github.com/Azure/azure-pipeline-go v0.2.3 // indirect
+	github.com/beorn7/perks v1.0.1 // indirect
+	github.com/cespare/xxhash/v2 v2.1.2 // indirect
+	github.com/fatih/color v1.7.0 // indirect
+	github.com/gin-contrib/sse v0.1.0 // indirect
+	github.com/go-playground/locales v0.14.0 // indirect
+	github.com/go-playground/universal-translator v0.18.0 // indirect
+	github.com/go-playground/validator/v10 v10.10.1 // indirect
+	github.com/golang/protobuf v1.5.2 // indirect
+	github.com/golang/snappy v0.0.1 // indirect
+	github.com/google/uuid v1.2.0 // indirect
+	github.com/jlaffaye/ftp v0.0.0-20180404123514-2403248fa8cc // indirect
+	github.com/jmespath/go-jmespath v0.3.0 // indirect
+	github.com/json-iterator/go v1.1.12 // indirect
+	github.com/kr/pretty v0.3.0 // indirect
+	github.com/kr/text v0.2.0 // indirect
+	github.com/leodido/go-urn v1.2.1 // indirect
+	github.com/mattn/go-colorable v0.1.13 // indirect
+	github.com/mattn/go-ieproxy v0.0.1 // indirect
+	github.com/mattn/go-isatty v0.0.16 // indirect
+	github.com/mattn/go-runewidth v0.0.2 // indirect
+	github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
+	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
+	github.com/modern-go/reflect2 v1.0.2 // indirect
+	github.com/prometheus/client_model v0.2.0 // indirect
+	github.com/prometheus/common v0.32.1 // indirect
+	github.com/prometheus/procfs v0.7.3 // indirect
+	github.com/rogpeppe/go-internal v1.8.0 // indirect
+	github.com/smartystreets/assertions v1.0.1 // indirect
+	github.com/smartystreets/gunit v1.0.4 // indirect
+	github.com/stretchr/testify v1.8.0 // indirect
+	golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2 // indirect
+	golang.org/x/text v0.3.7 // indirect
+	google.golang.org/protobuf v1.28.0 // indirect
 	gopkg.in/cheggaaa/pb.v1 v1.0.28 // indirect
 	gopkg.in/h2non/filetype.v1 v1.0.1 // indirect
+	gopkg.in/yaml.v2 v2.4.0 // indirect
 )

go.sum

@@ -327,7 +327,6 @@ github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO
 github.com/syndtr/goleveldb v1.0.1-0.20190923125748-758128399b1d h1:gZZadD8H+fF+n9CmNhYL1Y0dJB+kLOmKd7FbPJLeGHs=
 github.com/syndtr/goleveldb v1.0.1-0.20190923125748-758128399b1d/go.mod h1:9OrXJhf154huy1nPWmuSrkgjPUtUNhA+Zmy+6AESzuA=
 github.com/ugorji/go v1.1.7/go.mod h1:kZn38zHttfInRq0xu/PH0az30d+z6vm202qpg1oXVMw=
-github.com/ugorji/go v1.2.7 h1:qYhyWUUd6WbiM+C6JZAUkIJt/1WrjzNHY9+KCIjVqTo=
 github.com/ugorji/go v1.2.7/go.mod h1:nF9osbDWLy6bDVv/Rtoh6QgnvNDpmCalQV5urGCCS6M=
 github.com/ugorji/go/codec v1.1.7/go.mod h1:Ax+UKWsSmolVDwsd+7N3ZtXu+yMGCf907BLYF3GoBXY=
 github.com/ugorji/go/codec v1.2.7 h1:YPXUKf7fYbp/y8xloBqZOw2qaVggbfwMlI8WM3wZUJ0=

main.go

@@ -1,9 +1,7 @@
 package main
 
 import (
-	"math/rand"
 	"os"
-	"time"
 
 	"github.com/aptly-dev/aptly/aptly"
 	"github.com/aptly-dev/aptly/cmd"
@@ -22,7 +20,5 @@ func main() {
 
 	aptly.Version = Version
 
-	rand.Seed(time.Now().UnixNano())
-
 	os.Exit(cmd.Run(cmd.RootCommand(), os.Args[1:], true))
 }

main_test.go

@@ -6,7 +6,6 @@ package main
 import (
 	"flag"
 	"fmt"
-	"math/rand"
 	"os"
 	"strings"
 	"testing"
@@ -53,8 +52,6 @@ func TestRunMain(t *testing.T) {
 
 	aptly.Version = Version
 
-	rand.Seed(time.Now().UnixNano())
-
 	args := filterOutTestArgs(os.Args[1:])
 	root := cmd.RootCommand()
 	root.UsageLine = "aptly"

system/t12_api/mirrors.py

@@ -74,6 +74,39 @@ def check(self):
         self.check_equal(resp.status_code, 200)
 
 
+class MirrorsAPITestCreateUpdateSync(APITest):
+    """
+    POST /api/mirrors, PUT /api/mirrors/:name, GET /api/mirrors/:name/packages
+    """
+    def check(self):
+        mirror_name = self.random_name()
+        mirror_desc = {'Name': mirror_name,
+                       'ArchiveURL': 'https://packagecloud.io/varnishcache/varnish30/debian/',
+                       'Distribution': 'wheezy',
+                       'Components': ['main']}
+
+        mirror_desc['IgnoreSignatures'] = True
+        resp = self.post("/api/mirrors", json=mirror_desc)
+        self.check_equal(resp.status_code, 201)
+
+        resp = self.get("/api/mirrors/" + mirror_name + "/packages")
+        self.check_equal(resp.status_code, 404)
+
+        mirror_desc["Name"] = self.random_name()
+        resp = self.put("/api/mirrors/" + mirror_name, json=mirror_desc)
+
+        self.check_equal(resp.status_code, 204)
+
+        resp = self.get("/api/mirrors/" + mirror_desc["Name"])
+        self.check_equal(resp.status_code, 200)
+        self.check_subset({'Name': mirror_desc["Name"],
+                           'ArchiveRoot': 'https://packagecloud.io/varnishcache/varnish30/debian/',
+                           'Distribution': 'wheezy'}, resp.json())
+
+        resp = self.get("/api/mirrors/" + mirror_desc["Name"] + "/packages")
+        self.check_equal(resp.status_code, 200)
+
+
 class MirrorsAPITestCreateDelete(APITest):
     """
     POST /api/mirrors, DELETE /api/mirrors/:name