fix: discard the build record git:sync starts when nothing is built
cmd-git-sync calls dokku_setup_build_capture before the clone/fetch, because it cannot know whether there is anything to deploy until it has fetched. Both paths that then decline to build - a plain `git:sync` with no build flag, and a `--build-if-changes` poll that finds the remote unchanged - returned without ever finalizing that record. The record stayed at status=running forever, which has three consequences on an app polled on a timer: - PruneAppBuilds only runs from builds-record-finalize, so nothing prunes the ticks while the app is idle; they accumulate without bound, invisibly, because builds:list caps output at the retention count. - The next real deploy's ReapAbandonedBuilds finalizes each of them as status=failed, exit_code=-1, with finished_at stamped at the moment of reaping - indistinguishable on disk from a build that really failed, and with a duration that grew to the polling interval. - That same deploy's prune then keeps the newest records by started_at, which after an idle stretch are the ticks, deleting the last real deploy's record and log. Add a builds-record-discard trigger that removes a non-terminal record and its log, and call it from both no-op paths in cmd-git-sync. The trigger leaves missing and already-terminal records alone, so it can never delete real history. Fixes #9030 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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-<build-id>` so `journalctl -t dokku-<build-id>` 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
|
||||
|
||||
1
plugins/builds/.gitignore
vendored
1
plugins/builds/.gitignore
vendored
@@ -3,6 +3,7 @@
|
||||
/triggers/*
|
||||
/triggers
|
||||
/builds-generate-id
|
||||
/builds-record-discard
|
||||
/builds-record-finalize
|
||||
/builds-record-start
|
||||
/install
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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: <app> <build-id>
|
||||
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.
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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]" {
|
||||
|
||||
Reference in New Issue
Block a user