diff --git a/docs/advanced-usage/builds.md b/docs/advanced-usage/builds.md index 1e80bca43..c7a431876 100644 --- a/docs/advanced-usage/builds.md +++ b/docs/advanced-usage/builds.md @@ -25,6 +25,8 @@ Every build is identified by a sortable base36 ULID-style id (`DOKKU_BUILD_ID`) Output is also tagged into syslog as `dokku-` so `journalctl -t dokku-` continues to work. The on-disk log file is the durable source of truth and is read for `builds:output` even when journald has rotated old entries away. +A run that builds nothing leaves no record behind. `git:sync` has to fetch before it can tell whether there is anything to deploy, so it opens a record up front and discards it again when it turns out there is not - a `git:sync` with no `--build` flag, or a `--build-if-changes` poll that finds the remote unchanged. Without that, an app polled on a timer would accumulate `running` records that are later reaped as spurious failures, pushing real deploys out of the retention window. + ### Record schema ```json diff --git a/plugins/builds/.gitignore b/plugins/builds/.gitignore index ec242ac8a..f245b5926 100644 --- a/plugins/builds/.gitignore +++ b/plugins/builds/.gitignore @@ -3,6 +3,7 @@ /triggers/* /triggers /builds-generate-id +/builds-record-discard /builds-record-finalize /builds-record-start /install diff --git a/plugins/builds/Makefile b/plugins/builds/Makefile index f1ce6dc1f..6d47f2e6e 100644 --- a/plugins/builds/Makefile +++ b/plugins/builds/Makefile @@ -1,5 +1,5 @@ SUBCOMMANDS = subcommands/cancel subcommands/info subcommands/list subcommands/output subcommands/prune subcommands/report subcommands/set -TRIGGERS = triggers/builds-generate-id triggers/builds-record-finalize triggers/builds-record-start triggers/install triggers/post-app-rename-setup triggers/post-delete +TRIGGERS = triggers/builds-generate-id triggers/builds-record-discard triggers/builds-record-finalize triggers/builds-record-start triggers/install triggers/post-app-rename-setup triggers/post-delete BUILD = commands subcommands triggers PLUGIN_NAME = builds diff --git a/plugins/builds/builds.go b/plugins/builds/builds.go index 80b0bed4b..826c82a6a 100644 --- a/plugins/builds/builds.go +++ b/plugins/builds/builds.go @@ -436,6 +436,30 @@ func PruneAppBuilds(appName string) error { return nil } +// DiscardBuild removes a build record and its log outright. It exists for +// callers that start a record and then discover there was nothing to build: +// leaving such a record at status=running means it is later reaped as a +// spurious failure, and counts against retention in the meantime. +// +// It is a no-op for a record that is missing or already terminal - discarding +// a finalized build would delete real history. +func DiscardBuild(appName, buildID string) error { + b, err := ReadBuild(appName, buildID) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + + if b.Status.IsTerminal() { + return nil + } + + removeBuildFiles(appName, buildID) + return nil +} + func removeBuildFiles(appName, buildID string) { if err := os.Remove(RecordPath(appName, buildID)); err != nil && !os.IsNotExist(err) { common.LogWarn(fmt.Sprintf("Could not remove build record %s/%s: %s", appName, buildID, err)) diff --git a/plugins/builds/builds_test.go b/plugins/builds/builds_test.go index 235a1315c..dd16e8539 100644 --- a/plugins/builds/builds_test.go +++ b/plugins/builds/builds_test.go @@ -553,6 +553,72 @@ func TestRecordStartFinalizeIdempotency(t *testing.T) { } } +func TestRecordDiscardRemovesRunningRecord(t *testing.T) { + setupTestRoot(t) + app := "discard" + id := GenerateBuildID() + + if err := TriggerBuildsRecordStart(app, id, strconv.Itoa(os.Getpid()), string(BuildSourceGitSync)); err != nil { + t.Fatalf("record-start: %v", err) + } + if err := os.WriteFile(LogPathFor(app, id), []byte("fetch chatter\n"), 0644); err != nil { + t.Fatalf("write log: %v", err) + } + + if err := TriggerBuildsRecordDiscard(app, id); err != nil { + t.Fatalf("record-discard: %v", err) + } + + if _, err := os.Stat(RecordPath(app, id)); !os.IsNotExist(err) { + t.Errorf("record still present after discard, stat err = %v", err) + } + if _, err := os.Stat(LogPathFor(app, id)); !os.IsNotExist(err) { + t.Errorf("log still present after discard, stat err = %v", err) + } +} + +func TestRecordDiscardLeavesTerminalRecordsAlone(t *testing.T) { + setupTestRoot(t) + app := "discard-terminal" + id := GenerateBuildID() + + if err := TriggerBuildsRecordStart(app, id, strconv.Itoa(os.Getpid()), string(BuildSourceGitSync)); err != nil { + t.Fatalf("record-start: %v", err) + } + if err := TriggerBuildsRecordFinalize(app, id, "0"); err != nil { + t.Fatalf("finalize: %v", err) + } + + if err := TriggerBuildsRecordDiscard(app, id); err != nil { + t.Fatalf("record-discard: %v", err) + } + + got, err := ReadBuild(app, id) + if err != nil { + t.Fatalf("read after discard: %v", err) + } + if got.Status != BuildStatusSucceeded { + t.Errorf("Status = %v, want succeeded (discard must not touch finalized history)", got.Status) + } +} + +func TestRecordDiscardMissingRecordIsNoop(t *testing.T) { + setupTestRoot(t) + if err := TriggerBuildsRecordDiscard("discard-missing", GenerateBuildID()); err != nil { + t.Errorf("discard of missing record = %v, want nil", err) + } +} + +func TestRecordDiscardValidatesArguments(t *testing.T) { + setupTestRoot(t) + if err := TriggerBuildsRecordDiscard("", "abc"); err == nil { + t.Error("discard with empty app = nil, want error") + } + if err := TriggerBuildsRecordDiscard("app", ""); err == nil { + t.Error("discard with empty build id = nil, want error") + } +} + func TestRecordStartUnknownSourceCoercedToUnknown(t *testing.T) { setupTestRoot(t) app := "src" diff --git a/plugins/builds/src/triggers/triggers.go b/plugins/builds/src/triggers/triggers.go index 9f283bd4e..fcdf2343d 100644 --- a/plugins/builds/src/triggers/triggers.go +++ b/plugins/builds/src/triggers/triggers.go @@ -19,6 +19,10 @@ func main() { switch trigger { case "builds-generate-id": err = builds.TriggerBuildsGenerateID() + case "builds-record-discard": + appName := flag.Arg(0) + buildID := flag.Arg(1) + err = builds.TriggerBuildsRecordDiscard(appName, buildID) case "builds-record-finalize": appName := flag.Arg(0) buildID := flag.Arg(1) diff --git a/plugins/builds/triggers.go b/plugins/builds/triggers.go index 2b44a16bf..54e2a7e6f 100644 --- a/plugins/builds/triggers.go +++ b/plugins/builds/triggers.go @@ -96,6 +96,23 @@ func TriggerBuildsRecordStart(appName, buildID, pidStr, sourceStr string) error return WriteBuild(b) } +// TriggerBuildsRecordDiscard drops a record that was started before the caller +// knew whether it would build anything - the git:sync no-op paths, which fetch +// first and only then discover there is nothing to deploy. Records that are +// missing or already terminal are left alone. +// +// Args: +func TriggerBuildsRecordDiscard(appName, buildID string) error { + if appName == "" { + return errors.New("builds-record-discard: missing app name") + } + if buildID == "" { + return errors.New("builds-record-discard: missing build id") + } + + return DiscardBuild(appName, buildID) +} + // TriggerBuildsRecordFinalize writes the terminal status onto an existing // build record. It is idempotent: records that are already terminal (or have // been overwritten by an earlier finalize) are left untouched. diff --git a/plugins/common/functions b/plugins/common/functions index aec97aeb4..74750073f 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -910,6 +910,20 @@ dokku_setup_build_capture() { exec &> >(tee -a "$LOG" >(logger -i -t "dokku-${DOKKU_BUILD_ID}")) } +dokku_discard_build_capture() { + declare desc="drop the build record started by dokku_setup_build_capture when nothing ended up being built" + declare APP="$1" + if [[ -z "$APP" ]] || [[ -z "$DOKKU_BUILD_ID" ]]; then + return 0 + fi + + plugn trigger builds-record-discard "$APP" "$DOKKU_BUILD_ID" || true + # forget the id so a later finalize does not warn about the missing record. + # output stays redirected: the tee keeps the now-unlinked log open until the + # process exits, and stdout still reaches the operator. + unset DOKKU_BUILD_ID DOKKU_BUILD_LOG_FILE DOKKU_BUILD_SOURCE +} + acquire_app_deploy_lock() { declare desc="acquire advisory lock for use in deploys" local APP="$1" diff --git a/plugins/git/internal-functions b/plugins/git/internal-functions index 50857cf34..fa6455c4a 100644 --- a/plugins/git/internal-functions +++ b/plugins/git/internal-functions @@ -299,6 +299,7 @@ cmd-git-sync() { elif [[ "$FLAG" == "--build-if-changes" ]]; then if [[ "$CURRENT_REF" == "$UPDATED_REF" ]]; then dokku_log_verbose "Skipping build as no changes were detected" + dokku_discard_build_capture "$APP" return fi @@ -314,6 +315,11 @@ cmd-git-sync() { plugn trigger receive-app "$APP" "$GIT_REF" plugn trigger deploy-source-set "$APP" "git-sync" "${GIT_REMOTE}#${GIT_REF}" + else + # nothing was built, so the record opened before the fetch has no build to + # describe. Left in place it stays at status=running, is later reaped as a + # spurious failure, and evicts real history from the retention window. + dokku_discard_build_capture "$APP" fi } diff --git a/tests/unit/git_3.bats b/tests/unit/git_3.bats index 049cbda6b..eca165091 100644 --- a/tests/unit/git_3.bats +++ b/tests/unit/git_3.bats @@ -395,6 +395,59 @@ teardown() { echo "status: $status" assert_success assert_output_contains "$SMOKE_TEST_APP_MASTER_SHA" + + # the no-op tick must not add a record of its own, and must leave the + # succeeded record of the build above untouched + run /bin/bash -c "ls $DOKKU_LIB_ROOT/data/builds/$TEST_APP/*.json 2>/dev/null | wc -l" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "1" + + run /bin/bash -c "dokku builds:list $TEST_APP --format json" + echo "output: $output" + echo "status: $status" + assert_success + assert_output_contains '"status":"succeeded"' + assert_output_not_contains '"display_status":"abandoned"' +} + +@test "(git) git:sync new [no build leaves no build record]" { + run /bin/bash -c "rm -rf $DOKKU_LIB_ROOT/data/builds/$TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + # a sync with no build flag builds nothing, so it should record nothing + run /bin/bash -c "dokku git:sync $TEST_APP https://github.com/dokku/smoke-test-app.git" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "ls $DOKKU_LIB_ROOT/data/builds/$TEST_APP/*.json 2>/dev/null | wc -l" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "0" + + # and neither should a --build-if-changes poll that finds no changes + run /bin/bash -c "dokku git:sync --build-if-changes $TEST_APP https://github.com/dokku/smoke-test-app.git" + echo "output: $output" + echo "status: $status" + assert_success + assert_output_contains "Skipping build as no changes were detected" + + run /bin/bash -c "ls $DOKKU_LIB_ROOT/data/builds/$TEST_APP/*.json 2>/dev/null | wc -l" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "0" + + run /bin/bash -c "ls $DOKKU_LIB_ROOT/data/builds/$TEST_APP/*.log 2>/dev/null | wc -l" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "0" } @test "(git) git:sync new [--build branch]" {