fix(web): default RunJob.Delete to true for API-created and persisted jobs

[matheusamendola]

Summary

RunJob.Delete (core/runjob.go) only gets its "true" default through the config.ini decoder's default struct tag. Jobs built directly from a jobRequest (newRunJobFromRequest, web/server.go) or rebuilt from the state file on daemon restart (buildPersistedRunJob, cli/daemon.go) never go through that decoder, so Delete is left at its zero value (""). deleteContainer() (core/runjob.go) treats that as false via strconv.ParseBool(""), so the container from every type=run job created through the web API is left behind after it finishes.

On the next scheduled run, docker create fails because a container with that name already exists:

job run: creating container: create container "<name>": resource conflict

jobRequest doesn't expose a delete field to work around this at the call site, and the two construction paths (live API creation vs. state-file restore on restart) are independent, so both needed the same one-line fix to match the behavior config.ini users already get by default.

Found and reproduced against a real deployment (a daily ETL job created through the web UI started failing every day once its previous container was left behind — confirmed with docker ps -a showing the Exited (0) container days after its last run, and by reading the source to trace why).

Changes

• web/server.go: newRunJobFromRequest now sets j.Delete = "true".
• cli/daemon.go: buildPersistedRunJob now sets rj.Delete = "true".
• Added a regression test for each path asserting the constructed *core.RunJob has Delete == "true".

Test plan

• go build ./...
• go vet ./...
• go test ./web/... ./cli/... (full package suites, not just the new tests)
• gofmt -l . clean
• Manually reproduced end-to-end against a local build: created a type=run job via POST /api/jobs/create, ran it twice in a row (second run previously failed with resource conflict, now succeeds and container is removed both times); also verified across a daemon restart (buildPersistedRunJob path) with OFELIA_STATE_FILE set — same result.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[Copilot]

Pull request overview

This PR fixes a lifecycle bug in Ofelia’s type=run jobs created via the web API and restored from the daemon state file: core.RunJob.Delete was left at its zero value ("") outside the INI decoder path, causing finished containers to be retained and subsequent runs to fail with Docker “resource conflict” errors.

Changes:

• Default RunJob.Delete to "true" when constructing run jobs from API requests (web/server.go).
• Default RunJob.Delete to "true" when reconstructing persisted run jobs on daemon restart (cli/daemon.go).
• Add regression tests covering both construction paths.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
web/server.go	Sets RunJob.Delete = "true" for API-created run jobs to ensure containers are removed after completion.
web/server_run_job_delete_default_test.go	Adds a regression test asserting API-created *core.RunJob defaults Delete to "true".
cli/daemon.go	Sets RunJob.Delete = "true" when rebuilding persisted run jobs after restart.
cli/daemon_persisted_run_job_delete_default_test.go	Adds a regression test asserting persisted *core.RunJob defaults Delete to "true".

[matheusamendola]

@claude review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.57%. Comparing base (60e930f) to head (398f35c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #745      +/-   ##
==========================================
+ Coverage   87.45%   87.57%   +0.11%     
==========================================
  Files          90       90              
  Lines       12010    12012       +2     
==========================================
+ Hits        10503    10519      +16     
+ Misses       1218     1204      -14     
  Partials      289      289              

Flag	Coverage Δ
integration	87.57% <100.00%> (+0.11%)	⬆️
unittests	85.06% <100.00%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[CybotTM]

Thanks for tracking this down — great find, and the reproduction plus root-cause trace in the description made this easy to verify. The diagnosis is correct (Delete gets its "true" default only via the config decoder's default tag, and strconv.ParseBool("") → false in deleteContainer), both construction paths are covered, and the regression tests pin exactly the right thing. Also confirmed persist.Job has no delete field, so forcing "true" on restore can't override an explicit user choice.

One change requested before merge: instead of hand-setting j.Delete = "true", please use the codebase's existing convention — the config path applies all struct-tag defaults via creasty/defaults (_ = defaults.Set(j) in registerAllJobs, cli/config.go). Calling _ = defaults.Set(j) in newRunJobFromRequest and buildPersistedRunJob instead:

• closes the sibling gaps of the same bug class in the same stroke: Pull (default:"true" — API/persisted jobs currently run with pull=false, so a locally-present tag is never refreshed) and HistoryLimit (default:"10" — currently 0, so run history is never trimmed and grows unbounded);
• is future-proof: if a delete field is ever added to jobRequest/persist.Job, an explicit "false" survives while unset still defaults to "true" (defaults.Set only fills zero values);
• needs no new dependency — creasty/defaults is already in go.mod; web just adds the import.

Your regression tests should pass unchanged and then guard the broader fix; the explanatory comments at the two call sites can shrink to a line each ("struct-tag defaults are only applied by the config decoder — apply them here too").

[matheusamendola]

Applied — both call sites now use defaults.Set (398f35c):

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
...

Confirmed defaults.Set only fills zero values, so this also picks up Pull and HistoryLimit for the same two paths, and persist.Job has no delete field so there's nothing for the restore path to override. Existing regression tests pass unchanged; comments trimmed to one line per call site as suggested.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud