From ffa2991abffe33ff12325ef20842d41807e400ee Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Fri, 18 Sep 2026 02:50:34 -0400 Subject: [PATCH] fix: reject negative process counts when scaling A process count below zero was accepted and stored verbatim, leaving the scheduler to reject the value long after the command reported success. Counts below zero are now rejected, as are process tuples that omit a process type, across `ps:scale`, the `ps-set-scale` trigger and the `formation` key of `app.json`. A negative count persisted by an earlier version stays readable so it can be corrected with `ps:scale`. --- docs/appendices/file-formats/app-json.md | 2 +- docs/development/plugin-triggers.md | 2 +- docs/processes/process-management.md | 3 + plugins/app-json/functions.go | 4 + plugins/ps/functions.go | 23 +++- plugins/ps/functions_test.go | 157 +++++++++++++++++++++++ tests/unit/app-json-3.bats | 33 +++++ tests/unit/ps-general-1.bats | 71 ++++++++++ 8 files changed, 292 insertions(+), 3 deletions(-) diff --git a/docs/appendices/file-formats/app-json.md b/docs/appendices/file-formats/app-json.md index 4146aeb82..dd55093cc 100644 --- a/docs/appendices/file-formats/app-json.md +++ b/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 diff --git a/docs/development/plugin-triggers.md b/docs/development/plugin-triggers.md index 247239ed8..1a15fdee1 100644 --- a/docs/development/plugin-triggers.md +++ b/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: diff --git a/docs/processes/process-management.md b/docs/processes/process-management.md index 1a69b49f9..87526890b 100644 --- a/docs/processes/process-management.md +++ b/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 diff --git a/plugins/app-json/functions.go b/plugins/app-json/functions.go index 5beeb3bda..1422c37f5 100644 --- a/plugins/app-json/functions.go +++ b/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)) } } diff --git a/plugins/ps/functions.go b/plugins/ps/functions.go index 6b48c1a49..03a4567df 100644 --- a/plugins/ps/functions.go +++ b/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 +} diff --git a/plugins/ps/functions_test.go b/plugins/ps/functions_test.go index 793660743..fc2634353 100644 --- a/plugins/ps/functions_test.go +++ b/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) + } +} diff --git a/tests/unit/app-json-3.bats b/tests/unit/app-json-3.bats index efc8de7f8..0f7b79ef9 100644 --- a/tests/unit/app-json-3.bats +++ b/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" </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"