fix(cli): enforce verified backups and reliable upgrade/rollback outcomes #152

New issue

Open

opened 2026-09-26 23:45:34 +00:00 by gabogg · 0 comments

gabogg commented

2026-09-26 23:45:34 +00:00

Owner

Problem

The release-risk review from 1bb097dd1a through the statistics-deck implementation found existing weaknesses in hikctl update apply and rollback. A larger release involving schema migrations, calibration provenance, and historical counting reconciliation makes these weaknesses more consequential: an upgrade can proceed without a recoverable database backup, and success can be reported without sufficient evidence of readiness or recovery.

These are source-review findings, not a reproduced production outage. The viewer role itself is already supported by CLI argument validation, provisioning, listing, and tests.

Findings

References below are pinned to reviewed master commit d27708c.

Database backup failure does not abort the update. snapshot_engine.py, around lines 110–128, catches checkpoint/backup errors and still returns an archive with database.backed_up=false. cmd_update.py, around lines 163–169, treats that returned archive as successful. A later rollback may therefore have no database to restore. This does not mean every backup is bad; the failure is that a required backup is not enforced.
Snapshot precedes quiescence. The database is backed up before the gateway is stopped (cmd_update.py, lines 163–175). SQLite's backup API provides a consistent snapshot, but events accepted between that snapshot and shutdown will not be present after restoration. Establish and document the recovery boundary rather than describing this as an atomic whole-project snapshot.
Failed service stop and dependency installation only warn. cmd_update.py, around lines 175–176 and 203–204, continues after these errors. Source/dependencies can change while an old worker remains active, or startup can proceed with incomplete dependencies.
Migration/readiness verification is insufficient. The integrity check happens before service startup; schema initialization runs in app/main.py, line 48. The updater then polls /health for eight attempts with two-second sleeps plus request time. /health returns a static success payload and does not exercise statistics or role permissions. Startup work on a production-sized database may exceed that readiness window; this is a risk to measure, not a confirmed timeout.
Rollback can overstate success. rollback_engine.py logs restored-database integrity failure but does not fold it into success. Its success expression also excludes dependency/environment restoration and service health. Database expectation is inferred from archive-file existence, so a missing required DB can be treated as not expected.

Acceptance criteria

Abort before changing source/dependencies unless every required snapshot component is present and verified. Use SQLite's backup API, verify database integrity, and verify recorded checksums. Preserve explicit handling for in-memory targets and optional components.
Confirm workers are stopped before taking the final recovery snapshot and mutating the deployment. Abort safely if quiescence cannot be established; restore the prior running state if snapshot preparation fails after shutdown. Document the snapshot/recovery boundary and any event gap.
Treat dependency-install failure as a failed upgrade, with a verified recovery path and a nonzero exit status.
Validate database integrity after startup migrations complete. Provide a configurable readiness deadline and useful diagnostics for migration/startup failures. Distinguish HTTP liveness from release validation; do not make an unauthenticated health route perform privileged checks.
Preflight rollback manifests and required artifacts before destructive restoration. A missing/corrupt required DB, failed integrity check, failed required component restoration, or unhealthy restarted service must prevent a full-success result. Report partial recovery explicitly and return a nonzero CLI status.
Add deterministic failure-path tests for backup/checkpoint errors, missing/corrupt archives, stop failure, pip failure, slow startup, migration failure, and incomplete rollback. Use real temporary SQLite databases, including WAL-backed data, and mock external network/service-manager operations.
Document and rehearse upgrading and restoring a production-sized database copy. Verify occupancy/calibration outputs and admin/operator/viewer permissions; run the complete suite against the final merged release and smoke-test the deck in a refreshed browser.

Scope and release context

Harden the CLI update/snapshot/rollback contract on both supported service platforms. Frontend merge conflicts and asset-cache versioning are separate release concerns; /health alone must not be represented as proof that the statistics deck works. Existing Windows PATH issues #59 and #60 are separate.

Prioritize a verified database backup and fail-closed upgrade behavior before deploying the statistics-deck release. No production mutation or real upgrade/rollback was performed during this review.

Maintainer triage — 2026-09-27

Accept the existing fail-closed backup, stop, dependency, readiness and rollback requirements. Agent implementation and deterministic failure-path tests may proceed. Document and perform a production-sized database-copy upgrade/rollback rehearsal before release; retain the original platform and release verification requirements. No production upgrade is authorized by this triage.

## Problem The release-risk review from `1bb097dd1a` through the statistics-deck implementation found existing weaknesses in `hikctl update apply` and rollback. A larger release involving schema migrations, calibration provenance, and historical counting reconciliation makes these weaknesses more consequential: an upgrade can proceed without a recoverable database backup, and success can be reported without sufficient evidence of readiness or recovery. These are source-review findings, not a reproduced production outage. The viewer role itself is already supported by CLI argument validation, provisioning, listing, and tests. ## Findings References below are pinned to reviewed master commit `d27708c`. 1. **Database backup failure does not abort the update.** [`snapshot_engine.py`](https://git.gaboggamer.online/gabogg/hikcentral/src/commit/d27708c/app/cli/update/snapshot_engine.py), around lines 110–128, catches checkpoint/backup errors and still returns an archive with `database.backed_up=false`. [`cmd_update.py`](https://git.gaboggamer.online/gabogg/hikcentral/src/commit/d27708c/app/cli/commands/cmd_update.py), around lines 163–169, treats that returned archive as successful. A later rollback may therefore have no database to restore. This does not mean every backup is bad; the failure is that a required backup is not enforced. 2. **Snapshot precedes quiescence.** The database is backed up before the gateway is stopped (cmd_update.py, lines 163–175). SQLite's backup API provides a consistent snapshot, but events accepted between that snapshot and shutdown will not be present after restoration. Establish and document the recovery boundary rather than describing this as an atomic whole-project snapshot. 3. **Failed service stop and dependency installation only warn.** cmd_update.py, around lines 175–176 and 203–204, continues after these errors. Source/dependencies can change while an old worker remains active, or startup can proceed with incomplete dependencies. 4. **Migration/readiness verification is insufficient.** The integrity check happens before service startup; schema initialization runs in [`app/main.py`](https://git.gaboggamer.online/gabogg/hikcentral/src/commit/d27708c/app/main.py), line 48. The updater then polls `/health` for eight attempts with two-second sleeps plus request time. `/health` returns a static success payload and does not exercise statistics or role permissions. Startup work on a production-sized database may exceed that readiness window; this is a risk to measure, not a confirmed timeout. 5. **Rollback can overstate success.** [`rollback_engine.py`](https://git.gaboggamer.online/gabogg/hikcentral/src/commit/d27708c/app/cli/update/rollback_engine.py) logs restored-database integrity failure but does not fold it into success. Its success expression also excludes dependency/environment restoration and service health. Database expectation is inferred from archive-file existence, so a missing required DB can be treated as not expected. ## Acceptance criteria - [ ] Abort before changing source/dependencies unless every required snapshot component is present and verified. Use SQLite's backup API, verify database integrity, and verify recorded checksums. Preserve explicit handling for in-memory targets and optional components. - [ ] Confirm workers are stopped before taking the final recovery snapshot and mutating the deployment. Abort safely if quiescence cannot be established; restore the prior running state if snapshot preparation fails after shutdown. Document the snapshot/recovery boundary and any event gap. - [ ] Treat dependency-install failure as a failed upgrade, with a verified recovery path and a nonzero exit status. - [ ] Validate database integrity after startup migrations complete. Provide a configurable readiness deadline and useful diagnostics for migration/startup failures. Distinguish HTTP liveness from release validation; do not make an unauthenticated health route perform privileged checks. - [ ] Preflight rollback manifests and required artifacts before destructive restoration. A missing/corrupt required DB, failed integrity check, failed required component restoration, or unhealthy restarted service must prevent a full-success result. Report partial recovery explicitly and return a nonzero CLI status. - [ ] Add deterministic failure-path tests for backup/checkpoint errors, missing/corrupt archives, stop failure, pip failure, slow startup, migration failure, and incomplete rollback. Use real temporary SQLite databases, including WAL-backed data, and mock external network/service-manager operations. - [ ] Document and rehearse upgrading and restoring a production-sized database copy. Verify occupancy/calibration outputs and admin/operator/viewer permissions; run the complete suite against the final merged release and smoke-test the deck in a refreshed browser. ## Scope and release context Harden the CLI update/snapshot/rollback contract on both supported service platforms. Frontend merge conflicts and asset-cache versioning are separate release concerns; `/health` alone must not be represented as proof that the statistics deck works. Existing Windows PATH issues #59 and #60 are separate. Prioritize a verified database backup and fail-closed upgrade behavior before deploying the statistics-deck release. No production mutation or real upgrade/rollback was performed during this review. ## Maintainer triage — 2026-09-27 Accept the existing fail-closed backup, stop, dependency, readiness and rollback requirements. Agent implementation and deterministic failure-path tests may proceed. Document and perform a production-sized database-copy upgrade/rollback rehearsal before release; retain the original platform and release verification requirements. No production upgrade is authorized by this triage.

gabogg added the

needs-triage

label

2026-09-26 23:45:34 +00:00

gabogg added

ready-for-agent

and removed

needs-triage

labels

2026-09-27 16:29:15 +00:00