From 11c4e908d586f3deb4427f6870b61037ad40ff1c Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Sun, 11 Oct 2026 16:21:13 -0400 Subject: [PATCH] fix: hold the deploy lock across git-from-directory and harden the advisory lock git-from-directory updated the deploy branch before taking the deploy lock and rolled it back after releasing it. The lock is now held for the whole update, build, and rollback, and the rollback only moves the branch if it still points at the commit that was created. A failed lock attempt and apps:lock both truncated the lock file, dropping the build id of the in-progress deploy that builds:cancel and builds:output read. Releasing the lock unlocked before removing the file, so a waiter could end up holding a lock on a deleted file while another deploy locked a new one. The file is now removed before unlocking, and acquiring retries when the locked file is no longer the one at the lock path. --- plugins/apps/subcommands.go | 7 ++- plugins/common/functions | 61 ++++++++++++------ plugins/git/functions | 40 +++++++----- plugins/git/git-from-directory | 39 +++++++++--- plugins/git/internal-functions | 2 +- tests/unit/apps_1.bats | 30 +++++++++ tests/unit/deploy-lock.bats | 109 +++++++++++++++++++++++++++++++++ tests/unit/git_4.bats | 80 +++++++++++++++++++++--- 8 files changed, 312 insertions(+), 56 deletions(-) create mode 100644 tests/unit/deploy-lock.bats diff --git a/plugins/apps/subcommands.go b/plugins/apps/subcommands.go index 96942a6e3..72a7946ac 100644 --- a/plugins/apps/subcommands.go +++ b/plugins/apps/subcommands.go @@ -137,8 +137,13 @@ func CommandLock(appName string) error { return err } + // the file is not truncated as it may hold the build id of an in-progress deploy lockPath := getLockPath(appName) - if _, err := os.Create(lockPath); err != nil { + lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_WRONLY, 0644) + if err != nil { + return errors.New("Unable to create deploy lock") + } + if err := lockFile.Close(); err != nil { return errors.New("Unable to create deploy lock") } diff --git a/plugins/common/functions b/plugins/common/functions index bb00b5016..f2645f448 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -980,11 +980,17 @@ dokku_discard_build_capture() { unset DOKKU_BUILD_ID DOKKU_BUILD_LOG_FILE DOKKU_BUILD_SOURCE } +app_deploy_lock_file() { + declare desc="outputs the path to the deploy lock file for an app" + declare APP="$1" + echo "$DOKKU_LIB_ROOT/data/apps/$APP/.deploy.lock" +} + acquire_app_deploy_lock() { declare desc="acquire advisory lock for use in deploys" local APP="$1" local LOCK_TYPE="${2:-waiting}" - local APP_DEPLOY_LOCK_FILE="$DOKKU_LIB_ROOT/data/apps/$APP/.deploy.lock" + local APP_DEPLOY_LOCK_FILE="$(app_deploy_lock_file "$APP")" local LOCK_WAITING_MSG="$APP currently has a deploy lock in place. Waiting..." local LOCK_FAILED_MSG="$APP currently has a deploy lock in place. Exiting..." @@ -1003,7 +1009,7 @@ release_app_deploy_lock() { declare desc="release advisory lock used in deploys" local APP="$1" local EXIT_CODE="${2:-0}" - local APP_DEPLOY_LOCK_FILE="$DOKKU_LIB_ROOT/data/apps/$APP/.deploy.lock" + local APP_DEPLOY_LOCK_FILE="$(app_deploy_lock_file "$APP")" if [[ -n "$DOKKU_BUILD_ID" ]]; then plugn trigger builds-record-finalize "$APP" "$DOKKU_BUILD_ID" "$EXIT_CODE" || true @@ -1018,24 +1024,35 @@ acquire_advisory_lock() { local LOCK_FD="200" local SHOW_MSG=true - eval "exec $LOCK_FD>$LOCK_FILE" - if [[ "$LOCK_TYPE" == "waiting" ]]; then - while [[ $( - flock -n "$LOCK_FD" &>/dev/null - echo $? - ) -ne 0 ]]; do - if [[ "$SHOW_MSG" == "true" ]]; then - echo "$LOCK_WAITING_MSG" - SHOW_MSG=false + while true; do + # opened for appending so a failed attempt does not truncate the build id + # written by the current lock holder + eval "exec $LOCK_FD>>$LOCK_FILE" + if [[ "$LOCK_TYPE" == "waiting" ]]; then + while [[ $( + flock -n "$LOCK_FD" &>/dev/null + echo $? + ) -ne 0 ]]; do + if [[ "$SHOW_MSG" == "true" ]]; then + echo "$LOCK_WAITING_MSG" + SHOW_MSG=false + fi + sleep 1 + done + else + if ! flock -n "$LOCK_FD" &>/dev/null; then + dokku_log_warn "$LOCK_FAILED_MSG" + dokku_log_fail "Run 'apps:unlock' to release the existing deploy lock" fi - sleep 1 - done - else - if ! flock -n "$LOCK_FD" &>/dev/null; then - dokku_log_warn "$LOCK_FAILED_MSG" - dokku_log_fail "Run 'apps:unlock' to release the existing deploy lock" fi - fi + + # the previous holder removes the lock file on release, so the file locked + # above may no longer be the one at the lock path + if [[ "/proc/self/fd/$LOCK_FD" -ef "$LOCK_FILE" ]]; then + break + fi + eval "exec $LOCK_FD>&-" + done echo "${DOKKU_BUILD_ID:-$DOKKU_PID}" >"$LOCK_FILE" } @@ -1044,7 +1061,13 @@ release_advisory_lock() { local LOCK_FILE="$1" local LOCK_FD="200" - flock -u "$LOCK_FD" && rm -f "$LOCK_FILE" &>/dev/null + # the file is removed before unlocking so a waiter that locks it afterwards + # can tell that its lock is stale. It is only removed if it is still the file + # this process locked, as apps:unlock may have handed the path to another deploy + if [[ "/proc/self/fd/$LOCK_FD" -ef "$LOCK_FILE" ]]; then + rm -f "$LOCK_FILE" &>/dev/null + fi + flock -u "$LOCK_FD" } suppress_output() { diff --git a/plugins/git/functions b/plugins/git/functions index 5886822bf..2b23a75e9 100755 --- a/plugins/git/functions +++ b/plugins/git/functions @@ -150,28 +150,36 @@ git_receive_app() { if [[ ! -d "$DOKKU_ROOT/$APP/refs" ]]; then true else - local exit_code=0 has_source_image=false source_image previous_source_image + local exit_code=0 acquire_app_deploy_lock "$APP" "exclusive" - - # DOKKU_GIT_SOURCE_IMAGE is only applied while the deploy lock is held. An - # empty value clears the property, while an unset variable leaves it as-is. - if [[ -n "${DOKKU_GIT_SOURCE_IMAGE+x}" ]]; then - has_source_image=true - source_image="$DOKKU_GIT_SOURCE_IMAGE" - unset DOKKU_GIT_SOURCE_IMAGE - previous_source_image="$(fn-git-source-image "$APP")" - fn-plugin-property-write "git" "$APP" "source-image" "$source_image" - fi - - (git_build "$APP" "$REV") || exit_code="$?" - if [[ "$exit_code" -ne 0 ]] && [[ "$has_source_image" == "true" ]]; then - fn-plugin-property-write "git" "$APP" "source-image" "$previous_source_image" - fi + fn-git-build-with-lock-held "$APP" "$REV" || exit_code="$?" release_app_deploy_lock "$APP" "$exit_code" return "$exit_code" fi } +fn-git-build-with-lock-held() { + declare desc="builds an app at a revision, expecting the caller to hold the app deploy lock" + declare APP="$1" REV="$2" + local exit_code=0 has_source_image=false source_image previous_source_image + + # DOKKU_GIT_SOURCE_IMAGE is only applied while the deploy lock is held. An + # empty value clears the property, while an unset variable leaves it as-is. + if [[ -n "${DOKKU_GIT_SOURCE_IMAGE+x}" ]]; then + has_source_image=true + source_image="$DOKKU_GIT_SOURCE_IMAGE" + unset DOKKU_GIT_SOURCE_IMAGE + previous_source_image="$(fn-git-source-image "$APP")" + fn-plugin-property-write "git" "$APP" "source-image" "$source_image" + fi + + (git_build "$APP" "$REV") || exit_code="$?" + if [[ "$exit_code" -ne 0 ]] && [[ "$has_source_image" == "true" ]]; then + fn-plugin-property-write "git" "$APP" "source-image" "$previous_source_image" + fi + return "$exit_code" +} + cmd-git-upload-pack() { declare desc="executes git-upload-pack" declare cmd="git-upload-pack" diff --git a/plugins/git/git-from-directory b/plugins/git/git-from-directory index 542090c41..25210ff0a 100755 --- a/plugins/git/git-from-directory +++ b/plugins/git/git-from-directory @@ -5,20 +5,19 @@ source "$PLUGIN_AVAILABLE_PATH/git/internal-functions" set -eo pipefail [[ $DOKKU_TRACE ]] && set -x -trigger-git-git-from-directory() { - declare desc="updates a repository from a directory" - declare trigger="git-from-directory" - declare APP="$1" SOURCECODE_WORK_DIR="$2" USER_NAME="${3:-Dokku}" USER_EMAIL="${4:-automated@dokku.sh}" +fn-git-from-directory-with-lock-held() { + declare desc="updates a repository from a directory and builds it, expecting the caller to hold the app deploy lock" + declare APP="$1" SOURCECODE_WORK_DIR="$2" USER_NAME="$3" USER_EMAIL="$4" local APP_ROOT="$DOKKU_ROOT/$APP" local DOKKU_DEPLOY_BRANCH="$(fn-git-computed-deploy-branch "$APP")" - local REV + local REV PREVIOUS_REV local TMP_WORK_DIR=$(mktemp -d "/tmp/dokku-${DOKKU_PID}-${FUNCNAME[0]}.XXXXXX") local TMP_WORK_DIR_2=$(mktemp -d "/tmp/dokku-${DOKKU_PID}-${FUNCNAME[0]}.XXXXXX") trap "rm -rf '$TMP_WORK_DIR' '$TMP_WORK_DIR_2' >/dev/null" RETURN INT TERM EXIT local has_code=false - if fn-git-cmd "$APP_ROOT" rev-parse "$DOKKU_DEPLOY_BRANCH" &>/dev/null; then + if PREVIOUS_REV="$(fn-git-cmd "$APP_ROOT" rev-parse "$DOKKU_DEPLOY_BRANCH" 2>/dev/null)"; then has_code=true fi @@ -60,19 +59,39 @@ trigger-git-git-from-directory() { fi rm -rf "$TMP_WORK_DIR" "$TMP_WORK_DIR_2" >/dev/null - # run in a subshell so a failure to acquire the deploy lock still reaches the rollback - if (git_receive_app "$APP" "$REV"); then + if fn-git-build-with-lock-held "$APP" "$REV"; then return else local exit_code=$? local DOKKU_DEPLOY_BRANCH="$(fn-git-computed-deploy-branch "$APP")" + # only roll back the deploy branch if it still points at the commit created above if [[ "$has_code" == "false" ]]; then - fn-git-cmd "$APP_ROOT" update-ref -d HEAD + fn-git-cmd "$APP_ROOT" update-ref -d "refs/heads/$DOKKU_DEPLOY_BRANCH" "$REV" || dokku_log_warn "Unable to roll back the $DOKKU_DEPLOY_BRANCH branch" else - fn-git-cmd "$APP_ROOT" update-ref "refs/heads/$DOKKU_DEPLOY_BRANCH" "$DOKKU_DEPLOY_BRANCH^" + fn-git-cmd "$APP_ROOT" update-ref "refs/heads/$DOKKU_DEPLOY_BRANCH" "$PREVIOUS_REV" "$REV" || dokku_log_warn "Unable to roll back the $DOKKU_DEPLOY_BRANCH branch" fi return "$exit_code" fi } +trigger-git-git-from-directory() { + declare desc="updates a repository from a directory" + declare trigger="git-from-directory" + declare APP="$1" SOURCECODE_WORK_DIR="$2" USER_NAME="${3:-Dokku}" USER_EMAIL="${4:-automated@dokku.sh}" + local exit_code=0 + + # the lock is held from before the deploy branch is updated until after it is + # rolled back, and the subshell ensures it is released on any failure + acquire_app_deploy_lock "$APP" "exclusive" + set +e + ( + set -e + fn-git-from-directory-with-lock-held "$APP" "$SOURCECODE_WORK_DIR" "$USER_NAME" "$USER_EMAIL" + ) + exit_code="$?" + set -e + release_app_deploy_lock "$APP" "$exit_code" + return "$exit_code" +} + trigger-git-git-from-directory "$@" diff --git a/plugins/git/internal-functions b/plugins/git/internal-functions index f841499e9..41f1f2b4e 100644 --- a/plugins/git/internal-functions +++ b/plugins/git/internal-functions @@ -279,7 +279,7 @@ cmd-git-sync() { fi local APP_ROOT="$DOKKU_ROOT/$APP" - local APP_DEPLOY_LOCK_FILE="$DOKKU_LIB_ROOT/data/apps/$APP/.deploy.lock" + local APP_DEPLOY_LOCK_FILE="$(app_deploy_lock_file "$APP")" local fetch_exit_code=0 dokku_setup_build_capture "$APP" "git:sync" diff --git a/tests/unit/apps_1.bats b/tests/unit/apps_1.bats index 5bbfcda55..6264065bb 100644 --- a/tests/unit/apps_1.bats +++ b/tests/unit/apps_1.bats @@ -402,6 +402,36 @@ teardown() { destroy_app } +@test "(apps) apps:lock does not truncate an existing lock file" { + run create_app + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "echo holder-id >$DOKKU_LIB_ROOT/data/apps/$TEST_APP/.deploy.lock && chown dokku:dokku $DOKKU_LIB_ROOT/data/apps/$TEST_APP/.deploy.lock" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku apps:lock $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "cat $DOKKU_LIB_ROOT/data/apps/$TEST_APP/.deploy.lock" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "holder-id" + + run /bin/bash -c "dokku apps:unlock $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + destroy_app +} + @test "(apps:report) emits new stripped JSON keys alongside legacy" { create_app diff --git a/tests/unit/deploy-lock.bats b/tests/unit/deploy-lock.bats new file mode 100644 index 000000000..4abd87ff3 --- /dev/null +++ b/tests/unit/deploy-lock.bats @@ -0,0 +1,109 @@ +#!/usr/bin/env bats + +load test_helper + +setup() { + global_setup + LOCK_DIR="$(mktemp -d "/tmp/${DOKKU_DOMAIN}.XXXXX")" + LOCK_FILE="$LOCK_DIR/test.lock" + LOCK_PIDS=() +} + +teardown() { + local pid + touch "$LOCK_DIR/release" + for pid in "${LOCK_PIDS[@]}"; do + kill "$pid" &>/dev/null || true + done + rm -rf "$LOCK_DIR" + global_teardown +} + +lock_script() { + declare BUILD_ID="$1" SCRIPT="$2" + echo "source '$PLUGIN_CORE_AVAILABLE_PATH/common/functions'; export DOKKU_BUILD_ID='$BUILD_ID'; $SCRIPT" +} + +start_lock_holder() { + declare BUILD_ID="$1" + /bin/bash -c "$(lock_script "$BUILD_ID" "acquire_advisory_lock '$LOCK_FILE' exclusive 'waiting' 'lock held'; while [[ ! -f '$LOCK_DIR/release' ]]; do sleep 0.2; done; release_advisory_lock '$LOCK_FILE'")" >/dev/null 2>&1 3>&- & + LOCK_PIDS+=("$!") + wait_for "[[ \"\$(cat '$LOCK_FILE' 2>/dev/null)\" == '$BUILD_ID' ]]" +} + +wait_for() { + declare CONDITION="$1" + local i + for i in $(seq 1 50); do + if eval "$CONDITION"; then + return 0 + fi + sleep 0.2 + done + + echo "timed out waiting for: $CONDITION" + return 1 +} + +@test "(deploy-lock) a failed acquire does not truncate the lock holder's build id" { + start_lock_holder "holder-a" + + run /bin/bash -c "$(lock_script "other" "acquire_advisory_lock '$LOCK_FILE' exclusive 'waiting' 'lock held'")" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "lock held" + + run /bin/bash -c "cat '$LOCK_FILE'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "holder-a" +} + +@test "(deploy-lock) a waiter does not keep a stale lock after the holder removes the lock file" { + start_lock_holder "holder-a" + + /bin/bash -c "$(lock_script "holder-b" "acquire_advisory_lock '$LOCK_FILE' waiting 'waiting' 'lock held'; touch '$LOCK_DIR/b.acquired'; exec sleep 60")" >"$LOCK_DIR/b.log" 2>&1 3>&- & + local B_PID="$!" + LOCK_PIDS+=("$B_PID") + wait_for "grep -q waiting '$LOCK_DIR/b.log'" + + touch "$LOCK_DIR/release" + wait_for "[[ -f '$LOCK_DIR/b.acquired' ]]" + + run /bin/bash -c "[[ /proc/$B_PID/fd/200 -ef '$LOCK_FILE' ]]" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "cat '$LOCK_FILE'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "holder-b" + + run /bin/bash -c "$(lock_script "holder-c" "acquire_advisory_lock '$LOCK_FILE' exclusive 'waiting' 'lock held'")" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "lock held" +} + +@test "(deploy-lock) release only removes the lock file it holds" { + start_lock_holder "holder-a" + + run /bin/bash -c "rm -f '$LOCK_FILE' && echo 'replacement' >'$LOCK_FILE'" + echo "output: $output" + echo "status: $status" + assert_success + + touch "$LOCK_DIR/release" + wait_for "! kill -0 ${LOCK_PIDS[0]} 2>/dev/null" + + run /bin/bash -c "cat '$LOCK_FILE'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "replacement" +} diff --git a/tests/unit/git_4.bats b/tests/unit/git_4.bats index e29126095..0b3cd31c1 100644 --- a/tests/unit/git_4.bats +++ b/tests/unit/git_4.bats @@ -51,12 +51,16 @@ deploy_second_procfile_image() { hold_deploy_lock() { DEPLOY_LOCK_FILE="$DOKKU_LIB_ROOT/data/apps/$TEST_APP/.deploy.lock" - sudo -u dokku flock -o "$DEPLOY_LOCK_FILE" sleep 120 >/dev/null 2>&1 3>&- & - DEPLOY_LOCK_HOLDER_PID="$!" + DEPLOY_LOCK_HOLDER_PID_FILE="$(mktemp "/tmp/${DOKKU_DOMAIN}.XXXXX")" + chown dokku:dokku "$DEPLOY_LOCK_HOLDER_PID_FILE" + + # the holder writes a build id like a deploy would, then becomes the sleep + # process so that killing its pid releases the lock + sudo -u dokku bash -c "exec 200>>'$DEPLOY_LOCK_FILE'; flock -n 200 || exit 1; echo test-lock-holder >'$DEPLOY_LOCK_FILE'; echo \$\$ >'$DEPLOY_LOCK_HOLDER_PID_FILE'; exec sleep 120" >/dev/null 2>&1 3>&- & local i for i in $(seq 1 20); do - if ! flock -n "$DEPLOY_LOCK_FILE" true; then + if [[ -s "$DEPLOY_LOCK_HOLDER_PID_FILE" ]] && ! flock -n "$DEPLOY_LOCK_FILE" true; then return 0 fi sleep 0.5 @@ -67,15 +71,25 @@ hold_deploy_lock() { } release_deploy_lock() { - if [[ -z "$DEPLOY_LOCK_HOLDER_PID" ]]; then + if [[ -z "$DEPLOY_LOCK_HOLDER_PID_FILE" ]]; then return 0 fi - kill "$DEPLOY_LOCK_HOLDER_PID" &>/dev/null || true - wait "$DEPLOY_LOCK_HOLDER_PID" &>/dev/null || true - pkill -u dokku -f "flock -o $DEPLOY_LOCK_FILE" &>/dev/null || true - rm -f "$DEPLOY_LOCK_FILE" - DEPLOY_LOCK_HOLDER_PID="" + local holder_pid i + if [[ -s "$DEPLOY_LOCK_HOLDER_PID_FILE" ]]; then + holder_pid="$(cat "$DEPLOY_LOCK_HOLDER_PID_FILE")" + kill "$holder_pid" &>/dev/null || true + for i in $(seq 1 20); do + kill -0 "$holder_pid" &>/dev/null || break + sleep 0.5 + done + fi + rm -f "$DEPLOY_LOCK_FILE" "$DEPLOY_LOCK_HOLDER_PID_FILE" + DEPLOY_LOCK_HOLDER_PID_FILE="" +} + +deploy_branch_rev() { + sudo -u dokku git -C "/home/dokku/$TEST_APP" rev-parse master } @test "(git) git:from-image [missing]" { @@ -113,11 +127,21 @@ release_deploy_lock() { assert_output_contains "Image exists on host, skipping pull" assert_output_contains "No changes detected, skipping git commit" + run /bin/bash -c "dokku apps:locked $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure + run /bin/bash -c "dokku git:from-image --force $TEST_APP linuxserver/foldingathome:7.6.21" echo "output: $output" echo "status: $status" assert_output_contains "Force pulling image" assert_output_contains "No changes detected, skipping git commit" + + run /bin/bash -c "dokku apps:locked $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure } @test "(git) git:from-image [normal-custom-branch]" { @@ -210,6 +234,11 @@ release_deploy_lock() { assert_success assert_output "" + run /bin/bash -c "dokku apps:locked $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure + run /bin/bash -c "dokku config:set --no-restart $TEST_APP FAIL_ON_STARTUP=false" echo "output: $output" echo "status: $status" @@ -236,6 +265,11 @@ release_deploy_lock() { echo "status: $status" assert_failure + run /bin/bash -c "dokku apps:locked $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure + run /bin/bash -c "dokku git:report $TEST_APP --git-source-image" echo "output: $output" echo "status: $status" @@ -500,6 +534,10 @@ EOF echo "status: $status" assert_success + local DEPLOY_REV + DEPLOY_REV="$(deploy_branch_rev)" + echo "deploy branch rev: $DEPLOY_REV" + hold_deploy_lock run /bin/bash -c "dokku git:from-image $TEST_APP dokku-test/$TEST_APP:default-procfile" @@ -514,6 +552,18 @@ EOF assert_success assert_output "dokku-test/$TEST_APP:second-procfile" + run deploy_branch_rev + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$DEPLOY_REV" + + run /bin/bash -c "cat $DEPLOY_LOCK_FILE" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "test-lock-holder" + run /bin/bash -c "dokku git:sync $TEST_APP https://github.com/dokku/smoke-test-app.git 2.0.0" echo "output: $output" echo "status: $status" @@ -526,6 +576,18 @@ EOF assert_success assert_output "dokku-test/$TEST_APP:second-procfile" + run deploy_branch_rev + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$DEPLOY_REV" + + run /bin/bash -c "cat $DEPLOY_LOCK_FILE" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "test-lock-holder" + release_deploy_lock run /bin/bash -c "dokku git:from-image $TEST_APP dokku-test/$TEST_APP:default-procfile"