Skip to content

fix(restore): stop containers before restoring, by default - #5553

Open
bettercalln1ck wants to merge 1 commit into
Osmantic:mainfrom
bettercalln1ck:fix/4159-restore-stop-default
Open

bettercalln1ck wants to merge 1 commit into
Osmantic:mainfrom
bettercalln1ck:fix/4159-restore-stop-default

Conversation

@bettercalln1ck

Copy link
Copy Markdown
Contributor

Summary

Fixes #4159.

ods-restore.sh:576 defaulted stop_first="false", so the plain ods-restore.sh <backup-id> path rsynced backup data over data/* while the services that own those files were still running. Postgres, Qdrant and n8n can hold those files open, write over what was just restored, or carry stale in-memory state forward on top of it. -f skips the only confirmation prompt, so the scripted path — the one most likely to be automated — got no warning at all.

Two things in the script say false was never the intent:

  1. The help text already described -s as "(recommended)".
  2. Restore never restarts anything, yet its closing advice is:
Next steps:
  1. Review restored configuration: cat .../.env
  2. Start services: docker compose up -d

That instruction only makes sense if the stack was stopped — which, by default, it was not. The completion message was written for a path nobody took unless they knew to pass -s.

Changes:

  • Default is stop-first. --no-stop-containers restores the old behaviour for anyone who wants it, and warns that running services may hold open or overwrite the restored files.
  • A failed stop aborts. Previously stop_containers logged "Some containers may not have stopped cleanly" and carried on into exactly the situation the stop was meant to prevent. It now returns non-zero, and do_restore refuses rather than restoring underneath live services — naming the two ways forward (stop by hand, or pass --no-stop-containers to accept the risk).
  • ods-cli forwards its arguments verbatim to this script (ods-cli:4727), so it inherits the new default with no change.

AI Assistance

AI-assisted: an AI coding assistant drafted the patch and the regression test and ran the validation recorded below. I reviewed the diff, chose the validation, and am accountable for the change.

Release Lane

  • Stable hotfix targeting release/2.6.x
  • Mainline change targeting main
  • Next-minor work targeting the next feature/minor release
  • Not sure; reviewer should help classify

Stable hotfix reason:

n/a

Changed Surface

  • Docs only
  • Tests only
  • Dashboard UI
  • Dashboard API / host agent
  • Installer / bootstrap / lifecycle
  • Docker Compose / service manifests
  • Model routing / Hermes / capabilities
  • Network exposure / auth / proxy
  • Dependencies / runtime wiring

Risk And Validation

  • Risk level: Medium
  • Validation run:
    • git diff --check
    • Markdown/link sanity for docs
    • Focused tests listed below
    • Dashboard lint/test/build
    • Extension audit / compose validation
    • Release-grade fleet or scoped hardware validation
    • Stable-lane patch validation, if targeting release/2.6.x
    • Not required because: the change is confined to the container-stop decision in one script; the new test covers the default, the opt-out and the failed-stop path, and the neighbouring restore suites are unchanged.

Commands/results:

$ bash tests/test-restore-stops-containers.sh
+ default stops containers first
+ default restore succeeds
+ opt-out skips stopping
+ opt-out warns about running services
+ failed stop aborts the restore
All restore stop-first tests passed

# against main
$ git checkout origin/main -- ods/ods-restore.sh
$ bash tests/test-restore-stops-containers.sh
x containers were NOT stopped by default
exit=1

# neighbouring restore suites
$ bash tests/test-restore-empty-config.sh    -> PASS
$ bash tests/test-backup-restore-cli.sh      -> PASS

$ make lint
All lint checks passed.
$ shellcheck --exclude=SC1091,SC2034 --severity=error \
    ods/ods-restore.sh ods/tests/test-restore-stops-containers.sh
(clean)

Operational Change Check

  • This is not an operational change.
  • This is an operational change and validation is recorded above.
  • This is an operational change and validation is intentionally deferred for:

Notes For Reviewers

  • This changes a CLI default, which is the point of the issue but worth calling out for review: a caller running ods-restore.sh -f <id> in a script will now have its containers stopped where previously they were left running. Given the script already tells the operator to start services afterwards, that seems like the behaviour it always meant to have — but if you would rather keep the default and only add a refusal when containers are detected running, say so and I will flip it round.
  • The test stubs docker so it can assert on whether compose down was invoked, run the opt-out path, and force a failing stop via ODS_TEST_DOWN_EXIT=1. No Docker required.
  • Wired into both ods/Makefile and the integration-smoke job in test-linux.yml. The workflow entry is the one that matters — no workflow invokes make test, so a Makefile-only entry would not gate.
  • Unrelated, but noticed again while working here: tests/test-restore-safety-ux.sh fails identically on main and on this branch (it builds a fake $ODS_DIR without lib/, so ods-restore.sh dies sourcing lib/rsync.sh), and it is referenced from neither the Makefile nor any workflow. I raised this in fix(restore): refuse to replace live config from an empty backup #2952 as well; it is still broken and still invisible.

stop_first defaulted to "false", so `ods-restore.sh <id>` rsynced backup data
over data/* while the services owning those files were still running. Postgres,
Qdrant and n8n can hold the files open, write over what was just restored, or
carry stale state forward — and the operator gets no hint, because -f skips the
only prompt.

Two things say false was never the intent: the help text already called -s
"recommended", and the script never restarts anything, so its own closing
advice ("Start services: docker compose up -d") only makes sense if the stack
was stopped.

Default is now stop-first, with --no-stop-containers to opt out (warning
loudly). A stop that fails aborts the restore instead of proceeding
underneath live services, which also means stop_containers now reports
failure rather than only logging a warning.

ods-cli passes its arguments straight through, so it inherits the new default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Restore rsyncs into live stack by default

1 participant