From 194e7dc5cdfd9315cd66bd0dd125b6830ce43049 Mon Sep 17 00:00:00 2001 From: Matheus Amendola Date: Mon, 27 Jul 2026 09:33:01 -0300 Subject: [PATCH 1/2] fix(web): default RunJob.Delete to true for API-created and persisted jobs RunJob.Delete only receives its "true" default (core/runjob.go) through the config.ini decoder's default-tag handling. Jobs built directly from a jobRequest (web/server.go newRunJobFromRequest) or restored from the state file on daemon restart (cli/daemon.go buildPersistedRunJob) never go through that decoder, so Delete was left at its zero value (""), and deleteContainer() treats that as false. Every job-run created via the API left its container behind, colliding with the next scheduled run's `docker create` on the same name ("resource conflict"). jobRequest has no "delete" field to work around this at the call site, and the two construction paths are independent of each other, so both needed the same one-line fix to match the config.ini default behavior. Signed-off-by: Matheus Amendola --- cli/daemon.go | 4 ++ ...n_persisted_run_job_delete_default_test.go | 40 ++++++++++++++++ web/server.go | 6 +++ web/server_run_job_delete_default_test.go | 46 +++++++++++++++++++ 4 files changed, 96 insertions(+) create mode 100644 cli/daemon_persisted_run_job_delete_default_test.go create mode 100644 web/server_run_job_delete_default_test.go diff --git a/cli/daemon.go b/cli/daemon.go index 36b39ac560..b0ae102225 100644 --- a/cli/daemon.go +++ b/cli/daemon.go @@ -614,6 +614,10 @@ func (c *DaemonCommand) buildPersistedRunJob(name string, j *persist.Job, provid rj.Command = j.Command rj.Image = j.Image rj.Container = j.Container + // Same gap as newRunJobFromRequest (web/server.go): Delete's "true" default only + // applies through the config.ini decoder, and persisted API jobs are rebuilt here on + // every daemon restart without going through it. + rj.Delete = "true" return rj, nil } diff --git a/cli/daemon_persisted_run_job_delete_default_test.go b/cli/daemon_persisted_run_job_delete_default_test.go new file mode 100644 index 0000000000..760818a26a --- /dev/null +++ b/cli/daemon_persisted_run_job_delete_default_test.go @@ -0,0 +1,40 @@ +// Copyright (c) 2025-2026 Netresearch DTT GmbH +// SPDX-License-Identifier: MIT + +package cli + +import ( + "testing" + + "github.com/netresearch/ofelia/core" + "github.com/netresearch/ofelia/core/persist" +) + +// TestBuildPersistedRunJob_DefaultsDelete pins the restore-path half of +// the container-leak fix. buildPersistedRunJob rebuilds a persisted +// job-run on every daemon restart (initPersistStore -> newDaemonForPersistTest +// et al.) through a path entirely separate from newRunJobFromRequest +// (web/server.go) — a job created before this fix, or reloaded across +// any restart, would keep leaking containers even after the API path +// was patched if this path is not fixed too. +func TestBuildPersistedRunJob_DefaultsDelete(t *testing.T) { + t.Parallel() + + c := newDaemonForPersistTest(t) + job, err := c.buildPersistedRunJob("persisted-run-job", &persist.Job{ + Type: persist.JobTypeRun, + Schedule: "@hourly", + Image: "busybox", + }, &mockDockerProvider{}) + if err != nil { + t.Fatalf("buildPersistedRunJob: %v", err) + } + + rj, ok := job.(*core.RunJob) + if !ok { + t.Fatalf("expected *core.RunJob, got %T", job) + } + if rj.Delete != "true" { + t.Fatalf("Delete = %q, want \"true\" — persisted run jobs must clean up their container on every restart", rj.Delete) + } +} diff --git a/web/server.go b/web/server.go index 0aea7df1ae..1ed71aba2b 100644 --- a/web/server.go +++ b/web/server.go @@ -759,6 +759,12 @@ func (s *Server) newRunJobFromRequest(req *jobRequest) (core.Job, error) { j.Command = req.Command j.Image = req.Image j.Container = req.Container + // RunJob.Delete only gets its "true" default (core/runjob.go) via the config.ini + // decoder's default-tag handling; jobs built here from a jobRequest never go through + // that decoder, so Delete was left at its zero value ("") and deleteContainer() treated + // that as false — the container from every API-created run job was left behind, + // colliding with the next run's `docker create` on the same name. + j.Delete = "true" return j, nil } diff --git a/web/server_run_job_delete_default_test.go b/web/server_run_job_delete_default_test.go new file mode 100644 index 0000000000..9ea53c1aed --- /dev/null +++ b/web/server_run_job_delete_default_test.go @@ -0,0 +1,46 @@ +// Copyright (c) 2025-2026 Netresearch DTT GmbH +// SPDX-License-Identifier: MIT + +package web + +import ( + "io" + "log/slog" + "testing" + + "github.com/netresearch/ofelia/core" +) + +// TestNewRunJobFromRequest_DefaultsDelete pins the fix for the +// container-leak bug: jobRequest has no "delete" field, and unlike +// config.ini-sourced jobs (whose RunJob.Delete defaults to "true" via +// the decoder's default-tag handling), a job built here from the API +// request used to leave Delete at its zero value (""). deleteContainer +// treats that as false, so every API-created run job's container was +// left behind and collided with the next run's `docker create` on the +// same name ("resource conflict"). Delete must default to "true" here +// to match the config.ini behavior. +func TestNewRunJobFromRequest_DefaultsDelete(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + s := NewServer("", core.NewScheduler(logger), nil, newHangingDockerProvider()) + + job, err := s.newRunJobFromRequest(&jobRequest{ + Name: "api-run-job", + Type: "run", + Schedule: "@hourly", + Image: "busybox", + }) + if err != nil { + t.Fatalf("newRunJobFromRequest: %v", err) + } + + rj, ok := job.(*core.RunJob) + if !ok { + t.Fatalf("expected *core.RunJob, got %T", job) + } + if rj.Delete != "true" { + t.Fatalf("Delete = %q, want \"true\" — API-created run jobs must clean up their container like config.ini ones do", rj.Delete) + } +} From 398f35cf1e90a5781b3be9f9da8a95868c7a0f5d Mon Sep 17 00:00:00 2001 From: Matheus Amendola Date: Mon, 27 Jul 2026 13:45:42 -0300 Subject: [PATCH 2/2] fix(web,cli): use defaults.Set instead of hand-setting RunJob.Delete Per review on #745: reuse the codebase's existing struct-tag-default convention (creasty/defaults, already used in registerAllJobs) instead of hand-setting Delete = "true". This also fixes the sibling gaps of the same bug class for API-created and persisted run jobs: Pull (default:"true", was silently false) and HistoryLimit (default:"10", was 0, unbounded run history). Future-proof: defaults.Set only fills zero values, so an explicit false survives if delete is ever exposed in jobRequest/persist.Job. Signed-off-by: Matheus Amendola --- cli/daemon.go | 7 +++---- web/server.go | 9 +++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/cli/daemon.go b/cli/daemon.go index b0ae102225..e6ed479de3 100644 --- a/cli/daemon.go +++ b/cli/daemon.go @@ -14,6 +14,7 @@ import ( "sync" "time" + "github.com/creasty/defaults" "github.com/gobs/args" cfgvalidator "github.com/netresearch/ofelia/config" @@ -609,15 +610,13 @@ func (c *DaemonCommand) buildPersistedRunJob(name string, j *persist.Job, provid return nil, fmt.Errorf("docker provider unavailable for run job") } rj := core.NewRunJob(provider) + // struct-tag defaults are only applied by the config decoder — apply them here too. + _ = defaults.Set(rj) rj.Name = name rj.Schedule = j.Schedule rj.Command = j.Command rj.Image = j.Image rj.Container = j.Container - // Same gap as newRunJobFromRequest (web/server.go): Delete's "true" default only - // applies through the config.ini decoder, and persisted API jobs are rebuilt here on - // every daemon restart without going through it. - rj.Delete = "true" return rj, nil } diff --git a/web/server.go b/web/server.go index 1ed71aba2b..643e62c4c5 100644 --- a/web/server.go +++ b/web/server.go @@ -16,6 +16,7 @@ import ( "sync" "time" + "github.com/creasty/defaults" "github.com/gobs/args" cron "github.com/netresearch/go-cron" @@ -754,17 +755,13 @@ func (s *Server) newRunJobFromRequest(req *jobRequest) (core.Job, error) { return nil, fmt.Errorf("docker provider unavailable for run job") } j := core.NewRunJob(s.provider) + // struct-tag defaults are only applied by the config decoder — apply them here too. + _ = defaults.Set(j) j.Name = req.Name j.Schedule = req.Schedule j.Command = req.Command j.Image = req.Image j.Container = req.Container - // RunJob.Delete only gets its "true" default (core/runjob.go) via the config.ini - // decoder's default-tag handling; jobs built here from a jobRequest never go through - // that decoder, so Delete was left at its zero value ("") and deleteContainer() treated - // that as false — the container from every API-created run job was left behind, - // colliding with the next run's `docker create` on the same name. - j.Delete = "true" return j, nil }