Merge pull request #9050 from dokku/9048-ps-scale-accepts-a-negative-process-count

Reject negative process counts when scaling

docs/appendices/file-formats/app-json.md

@@ -158,7 +158,7 @@ On subsequent deploys:
 
 - `autoscaling` (map of string to object, optional) autoscaling rules. See the autoscaling section for more details
 - `max_parallel`: (int, optional) number of instances to deploy in parallel at a given time
-- `quantity`: (int, optional) number of processes to maintain. Default 1 for web processes, 0 for all others.
+- `quantity`: (int, optional) number of processes to maintain. Must be zero or greater. Default 1 for web processes, 0 for all others.
 - `service`: (map of string to oject, optional) governs how non-web processes are exposed as services on the network
 
 ### Autoscaling

docs/development/plugin-triggers.md

@@ -2396,7 +2396,7 @@ set -eo pipefail; [[ $DOKKU_TRACE ]] && set -x
 
 ### `ps-set-scale`
 
-- Description: Sets the scale for an app based on a specified formation (process-type=quantity). Any unspecified process types will be left as is, or have their process count set to zero when `$CLEAR_EXISTING` is `true`.
+- Description: Sets the scale for an app based on a specified formation (process-type=quantity). Any unspecified process types will be left as is, or have their process count set to zero when `$CLEAR_EXISTING` is `true`. Each process tuple must name a process type, and process counts must be zero or greater.
 - Invoked by:
 - Arguments: `$APP $SKIP_DEPLOY $CLEAR_EXISTING [$PROCESS_TUPLE...]`
 - Example:

docs/processes/process-management.md

@@ -248,6 +248,9 @@ Multiple process types can be scaled at once:
 dokku ps:scale node-js-app web=1 worker=1
 ```
 
+> [!NOTE]
+> Each argument must name a process type, and process counts must be zero or greater. Scaling a process type to `0` stops all of its containers, while a negative count is rejected.
+
 If desired, the corresponding deploy will be skipped by using the `--skip-deploy` flag:
 
 ```shell

plugins/app-json/functions.go

@@ -431,6 +431,10 @@ func setScale(appName string) error {
 	args := []string{appName, strconv.FormatBool(skipDeploy), strconv.FormatBool(clearExisting)}
 	for processType, formation := range appJSON.Formation {
 		if formation.Quantity != nil {
+			if *formation.Quantity < 0 {
+				return fmt.Errorf("Invalid quantity for process type %s in app.json: value must be zero or greater", processType)
+			}
+
 			args = append(args, fmt.Sprintf("%s=%d", processType, *formation.Quantity))
 		}
 	}

plugins/ps/functions.go

@@ -196,7 +196,7 @@ func parseProcessTuples(processTuples []string) (FormationSlice, error) {
 		processType := strings.TrimSpace(s[0])
 		quantity, err := strconv.Atoi(strings.TrimSpace(s[1]))
 		if err != nil {
-			return formations, fmt.Errorf("Invalid count for process type %s", s[0])
+			return formations, fmt.Errorf("Invalid count for process type %s", processType)
 		}
 
 		if foundFormations[processType] {
@@ -351,6 +351,10 @@ func scaleSet(input scaleSetInput) error {
 		return err
 	}
 
+	if err := validateFormations(formations); err != nil {
+		return err
+	}
+
 	if input.clearExisting {
 		formations = appendClearedFormations(formations, existingFormations)
 	}
@@ -484,3 +488,20 @@ func updateScale(appName string, clearExisting bool, formationUpdates FormationS
 
 	return common.PropertyListWrite("ps", appName, "scale", values)
 }
+
+// validateFormations returns an error when a user specified formation cannot be scaled to.
+// Formations read back from the scale property are deliberately not validated so that a value
+// persisted by an older dokku version remains visible and repairable via ps:scale.
+func validateFormations(formations FormationSlice) error {
+	for _, formation := range formations {
+		if formation.ProcessType == "" {
+			return fmt.Errorf("Missing process type for count %d", formation.Quantity)
+		}
+
+		if formation.Quantity < 0 {
+			return fmt.Errorf("Invalid count for process type %s: value must be zero or greater", formation.ProcessType)
+		}
+	}
+
+	return nil
+}

plugins/ps/functions_test.go

@@ -90,3 +90,160 @@ func TestDefaultProcessTuplesWithoutProcfile(t *testing.T) {
 		t.Fatalf("expected [web=1], got %v", processTuples)
 	}
 }
+
+func TestParseProcessTuples(t *testing.T) {
+	testCases := []struct {
+		name          string
+		processTuples []string
+		expected      []Formation
+		expectedError string
+	}{
+		{
+			name:          "no process tuples",
+			processTuples: []string{},
+			expected:      []Formation{},
+		},
+		{
+			name:          "single process type",
+			processTuples: []string{"web=1"},
+			expected:      []Formation{{ProcessType: "web", Quantity: 1}},
+		},
+		{
+			name:          "multiple process types",
+			processTuples: []string{"web=1", "worker=2"},
+			expected:      []Formation{{ProcessType: "web", Quantity: 1}, {ProcessType: "worker", Quantity: 2}},
+		},
+		{
+			name:          "trims surrounding whitespace",
+			processTuples: []string{" web = 2 "},
+			expected:      []Formation{{ProcessType: "web", Quantity: 2}},
+		},
+		{
+			name:          "first specified quantity wins",
+			processTuples: []string{"web=1", "web=3"},
+			expected:      []Formation{{ProcessType: "web", Quantity: 1}},
+		},
+		{
+			// persisted formations are parsed without validation so that a negative
+			// quantity written by an older dokku version remains readable
+			name:          "negative quantities are parsed",
+			processTuples: []string{"web=-1"},
+			expected:      []Formation{{ProcessType: "web", Quantity: -1}},
+		},
+		{
+			name:          "missing count",
+			processTuples: []string{"web"},
+			expectedError: "Missing count for process type web",
+		},
+		{
+			name:          "non-numeric count",
+			processTuples: []string{"web=one"},
+			expectedError: "Invalid count for process type web",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			formations, err := parseProcessTuples(testCase.processTuples)
+			if testCase.expectedError != "" {
+				if err == nil {
+					t.Fatalf("expected error %q, got none", testCase.expectedError)
+				}
+
+				if err.Error() != testCase.expectedError {
+					t.Fatalf("expected error %q, got %q", testCase.expectedError, err.Error())
+				}
+
+				return
+			}
+
+			if err != nil {
+				t.Fatalf("unexpected error parsing process tuples: %v", err)
+			}
+
+			assertFormations(t, formations, testCase.expected)
+		})
+	}
+}
+
+func TestValidateFormations(t *testing.T) {
+	testCases := []struct {
+		name          string
+		processTuples []string
+		expectedError string
+	}{
+		{
+			name:          "zero is a valid quantity",
+			processTuples: []string{"web=0"},
+		},
+		{
+			name:          "positive quantities are valid",
+			processTuples: []string{"web=1", "worker=10"},
+		},
+		{
+			name:          "negative quantity",
+			processTuples: []string{"web=-1"},
+			expectedError: "Invalid count for process type web: value must be zero or greater",
+		},
+		{
+			name:          "names the offending process type",
+			processTuples: []string{"web=1", "worker=-2"},
+			expectedError: "Invalid count for process type worker: value must be zero or greater",
+		},
+		{
+			name:          "missing process type",
+			processTuples: []string{"=1"},
+			expectedError: "Missing process type for count 1",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			formations, err := parseProcessTuples(testCase.processTuples)
+			if err != nil {
+				t.Fatalf("unexpected error parsing process tuples: %v", err)
+			}
+
+			err = validateFormations(formations)
+			if testCase.expectedError == "" {
+				if err != nil {
+					t.Fatalf("unexpected error: %v", err)
+				}
+
+				return
+			}
+
+			if err == nil {
+				t.Fatalf("expected error %q, got none", testCase.expectedError)
+			}
+
+			if err.Error() != testCase.expectedError {
+				t.Fatalf("expected error %q, got %q", testCase.expectedError, err.Error())
+			}
+		})
+	}
+}
+
+func TestScaleSetRejectsNegativeQuantity(t *testing.T) {
+	tmpDir, err := os.MkdirTemp("", "ps-test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	t.Setenv("DOKKU_LIB_ROOT", tmpDir)
+	t.Setenv("DOKKU_PID", "")
+
+	err = scaleSet(scaleSetInput{
+		appName:       "test-app",
+		skipDeploy:    true,
+		processTuples: []string{"web=-1"},
+	})
+	if err == nil {
+		t.Fatal("expected an error scaling to a negative quantity, got none")
+	}
+
+	if err.Error() != "Invalid count for process type web: value must be zero or greater" {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}

tests/unit/app-json-3.bats

@@ -58,6 +58,23 @@ teardown() {
   assert_output_contains "cron: 1" 1
 }
 
+@test "(app-json) negative formation quantity" {
+  mkdir -p /var/lib/dokku/data/app-json/$TEST_APP
+  negative_formation_callback "$TEST_APP" /var/lib/dokku/data/app-json/$TEST_APP
+
+  run_plugn_trigger post-release-builder herokuish $TEST_APP dokku/$TEST_APP:latest
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "Invalid quantity for process type web in app.json: value must be zero or greater"
+
+  run /bin/bash -c "dokku --quiet ps:scale $TEST_APP"
+  output=$(echo "$output" | tr -s " ")
+  echo "output: ($output)"
+  echo "status: $status"
+  assert_output "web: 1"
+}
+
 persist_scale_callback_a() {
   local APP="$1"
   local APP_REPO_DIR="$2"
@@ -81,3 +98,19 @@ persist_scale_callback_b() {
   echo "cron: python3 worker.py" >>"$APP_REPO_DIR/Procfile"
   mv "$APP_REPO_DIR/app-5205b.json" "$APP_REPO_DIR/app.json"
 }
+
+negative_formation_callback() {
+  local APP="$1"
+  local APP_REPO_DIR="$2"
+  [[ -z "$APP" ]] && local APP="$TEST_APP"
+
+  cat >"$APP_REPO_DIR/app.json" <<EOF
+{
+  "formation": {
+    "web": {
+      "quantity": -1
+    }
+  }
+}
+EOF
+}

tests/unit/ps-general-1.bats

@@ -357,6 +357,77 @@ worker=1" >/var/lib/dokku/config/ps/$TEST_APP/scale
   assert_output_contains "The --clear and --replace flags cannot be specified together"
 }
 
+@test "(ps:scale) invalid process counts" {
+  run /bin/bash -c "dokku ps:scale $TEST_APP web=-1"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "Invalid count for process type web: value must be zero or greater"
+
+  run /bin/bash -c "dokku ps:scale --replace $TEST_APP web=1 worker=-2"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "Invalid count for process type worker: value must be zero or greater"
+
+  run /bin/bash -c "dokku ps:scale $TEST_APP =1"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "Missing process type for count 1"
+
+  run_plugn_trigger ps-set-scale $TEST_APP true false web=-1
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "Invalid count for process type web: value must be zero or greater"
+
+  run /bin/bash -c "dokku --quiet ps:scale $TEST_APP"
+  output=$(echo "$output" | tr -s " ")
+  echo "output: ($output)"
+  echo "status: $status"
+  assert_output "web: 1"
+
+  run /bin/bash -c "dokku ps:scale --skip-deploy $TEST_APP web=0"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku --quiet ps:scale $TEST_APP"
+  output=$(echo "$output" | tr -s " ")
+  echo "output: ($output)"
+  echo "status: $status"
+  assert_output "web: 0"
+}
+
+@test "(ps:scale) repairing a negative process count" {
+  echo "web=-1
+worker=2" >/var/lib/dokku/config/ps/$TEST_APP/scale
+
+  run /bin/bash -c "dokku --quiet ps:scale $TEST_APP"
+  output=$(echo "$output" | tr -s " ")
+  echo "output: ($output)"
+  echo "status: $status"
+  assert_success
+  assert_output $'web: -1\nworker: 2'
+
+  run_plugn_trigger ps-current-scale $TEST_APP
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku ps:scale --skip-deploy $TEST_APP web=1"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku --quiet ps:scale $TEST_APP"
+  output=$(echo "$output" | tr -s " ")
+  echo "output: ($output)"
+  echo "status: $status"
+  assert_output $'web: 1\nworker: 2'
+}
+
 @test "(ps) handle windows newlines in procfile" {
   run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP procfile_line_endings_to_windows
   echo "output: $output"