docs(openspec): define OSS production baseline - #21
Conversation
vishr
left a comment
There was a problem hiding this comment.
Architecture and specification review — planning artifacts only, no implementation requested.
Verified clean first: openspec validate complete-oss-production-baseline --strict passes. The tasks file contains exactly 129 checkboxes, matching the PR description. The eleven-driver list matches the live catalogue exactly (internal/app/services.go:104-201: postgres, mysql, mariadb, redis, valkey, mongodb, rabbitmq, minio, meilisearch, clickhouse, nats). The RENAMED and MODIFIED requirement headers resolve against real requirements in the current specs. Restic is correctly fenced to artifact transport, with task 5.3 explicitly testing that a database engine cannot select it. Retention safety, foreign-content refusal, replay continuity, and no-remote-deletion-on-policy-removal are all covered. No premium or control-plane scope has leaked in.
The strongest part of the change is the graduation discipline: every driver gate ends with "Only after task X.N passes", and task 22.5 adds a consistency test comparing published claims against the runtime capability catalogue. That control should survive review unchanged.
Twenty findings inline, ordered by severity within each file. Three are critical and block the design as written:
- pgBackRest must be installed on the PostgreSQL host and invoked by
archive_commandfrom inside the container; the officialpostgresimage has no such binary and no task resolves how it gets there. - The Redis/Valkey restore path produces an empty dataset, because the shipped driver command enables AOF and Redis loads the AOF in preference to a restored RDB.
- Enabling PITR requires a service restart that the interruption contract does not model, and MariaDB binary logging is off by default with no author-reachable way to enable it.
Remaining implementation risks beyond the findings: the scope is genuinely large at 129 tasks across eleven independent gates, each needing local-Docker end-to-end runs with injected crashes; section 15 (MinIO) is the only gate with no backup-generation path at all, so the manifest model in 5.5 must degrade cleanly for replicated rather than inventing generations; and nine external helper toolchains enter the recovery dependency chain, which makes the 22.4 direct-recovery runbooks gate-blocking work rather than documentation cleanup.
| ClickHouse, Redis, Valkey, RabbitMQ, MinIO, Meilisearch, and NATS JetStream. | ||
| Their initial contracts are deliberately different: | ||
|
|
||
| - **PostgreSQL:** pgBackRest owns encrypted full/differential/incremental base |
There was a problem hiding this comment.
Critical — pgBackRest cannot run against the pinned upstream postgres image.
pgBackRest's user guide states it must be installed on the PostgreSQL host, run as the postgres OS user, and be invoked by archive_command = 'pgbackrest --stanza=... archive-push %p'. That command executes inside the PostgreSQL container, and the catalogue pins the official image (internal/app/services.go:107), which ships no pgbackrest binary. A sidecar cannot supply it. Decision 3 only says a "database-native archive hook ... remains part of that service runtime" without naming the mechanism.
Consequence: the flagship PITR gate — the one every later gate is patterned on — has no implementation path. Resolving it forces a Onebox-built postgres+pgbackrest image, which is a supply-chain and image-provenance scope item absent from proposal.md Impact and from decision 1's pinned-image model.
Recommendation: add a decision naming the mechanism (derived image over a pinned upstream base digest, vs. pgBackRest TLS server mode — which still needs the binary at both ends), add tasks under 7.1, and list the derived-image class in proposal Impact. The same question applies to XtraBackup/MariaDB Backup datadir access and ClickHouse named-collection configuration.
| parts, counts, and declared queries. ClickHouse documents native S3, | ||
| incremental, asynchronous, and password-protected backup behavior: | ||
| https://clickhouse.com/docs/concepts/features/backup-restore/overview | ||
| - **Redis:** the driver requests and waits for an immutable RDB generation, |
There was a problem hiding this comment.
Critical — the Redis/Valkey restore procedure produces an empty dataset under the shipped driver command.
The driver runs redis-server --requirepass ... --appendonly yes (internal/app/services.go:131; Valkey :139). Upstream Redis persistence docs: "In the case both AOF and RDB persistence are enabled and Redis restarts the AOF file will be used to reconstruct the original dataset since it is guaranteed to be the most complete." Restoring dump.rdb into an empty volume and starting the driver as configured loads the empty AOF and ignores the RDB.
Consequence: the documented restore path yields zero keys. Tasks 12.3/13.3 verify key counts so the gate would eventually fail, but only after the whole contract is built on a broken procedure — and any operator following the 22.4 direct-recovery runbook loses data.
Recommendation: require an explicit RDB load mechanism in the driver contract — preload-file rdb:<path> (Redis >= 8.10, which skips appenddirname and dump.rdb loading entirely), or a documented appendonly-off first boot followed by CONFIG SET appendonly yes plus a rewrite — and pin the qualified version range to whichever is chosen.
| local durability, not the backup artifact. Restore boots an exact-compatible | ||
| empty service from the RDB and verifies load and key counts. Redis documents | ||
| the atomic RDB publication and safe live copying contract: | ||
| https://redis.io/docs/latest/operate/oss_and_stack/management/persistence/ |
There was a problem hiding this comment.
Suggestion — the Redis contract predates the modern native backup mechanism.
Redis >= 8.10 ships a BACKUP START / LIST / SEAL / CLEANUP family producing a sealed BASE + INCR + manifest artifact set, restored with preload-file aof:<manifest>. It is online, does not require disabling AOF rewrites during the copy, and its recovery point is the seal boundary rather than the BASE fork — a strictly better observed RPO than a bare RDB, and it resolves the AOF-precedence hazard noted above directly.
Worth evaluating as the qualified Redis contract, with plain RDB retained as the pre-8.10 fallback.
| - **WHEN** its policy requires point-in-time recovery | ||
| - **THEN** validation fails with code `recovery_objective_unsupported` and reports the strongest executable objective without silently weakening the declaration | ||
|
|
||
| #### Scenario: Required interruption is not authorized |
There was a problem hiding this comment.
Critical — enabling PITR requires a restart that the interruption contract does not model, and MariaDB binary logging cannot be enabled at all.
backup_interruption_not_authorized fires only for a driver "whose complete backup contract requires a stopped-service window" — i.e. RabbitMQ cold. But PostgreSQL archive_mode=on requires a server restart; ClickHouse XML named collections require a restart; and MariaDB log_bin is off by default while both mysql and mariadb carry settings: settingsUnsupported (internal/app/services.go:118,125), so no author-reachable mechanism enables it.
Consequence: turning protection on silently restarts a live production database with no authorization gate. On a single-host product there is no standby to absorb that. And MariaDB PITR — a full six-task gate in section 9 — is unimplementable without a driver-level change no task lists.
Recommendation: add a distinct one-time enablement interruption requirement and error code, separate from ongoing backup interruption, and add tasks under 9.1 and 7.1 for driver-level log_bin / archive_mode enablement including the restart plan and its approval.
| - **THEN** validation fails with code `backup_interruption_not_authorized` and the service remains `Run` | ||
|
|
||
| ### Requirement: Backup creation is consistent, encrypted, and retry-safe | ||
|
|
There was a problem hiding this comment.
High — the uniform encryption-at-rest obligation is not satisfiable across recovery kinds.
"Onebox SHALL encrypt backup data at rest in the off-host destination" cannot hold uniformly. ClickHouse BACKUP ... TO S3(...) is not encrypted by ClickHouse — its password setting applies to archive-format backups, not plain S3 destinations. MinIO replication stores whatever the source held. design.md cites "password-protected backup behavior" without stating which form qualifies.
Consequence: ClickHouse and MinIO graduate under an encryption claim they do not meet, or the requirement is waived without record — the exact overclaim this capability exists to prevent.
Recommendation: make the encryption obligation per-recovery-kind in the capability record and manifest (client-side / archive password / server-side SSE / inherited for replicated), and have status publish which one is in effect.
| it would duplicate mature encryption, resume, locking, and retention behavior. | ||
|
|
||
| ### 3. Run schedules through a short-lived canonical runner | ||
|
|
There was a problem hiding this comment.
Medium — the target-resident runner is a new artifact class with unaddressed version skew.
The runner is the canonical Go service plus operation graph, installed on the target with secret-file access. The current model has no Onebox executable on the target at all, yet proposal.md Impact never lists it as an artifact class. Skew is covered by one sentence — a runner whose supported schema excludes the envelope refuses — which leaves open what happens when the CLI is older than the runner, how runner provenance is verified beyond a digest comparison, and whether ob destroy revokes it.
Recommendation: add explicit requirements for bidirectional version skew, provenance verification, and removal on destroy, and list the runner in proposal Impact.
| isolation. RabbitMQ explicitly discourages copying message data from a live | ||
| node and binds disk restore to node identity: | ||
| https://www.rabbitmq.com/docs/backup | ||
| - **MinIO:** ordinary volume copying and mirroring are insufficient. The |
There was a problem hiding this comment.
Medium — MinIO is the only driver whose Managed tier requires the operator to run a second server.
Every other driver reaches Managed with user-owned S3 storage. MinIO replicated requires "an independently identified MinIO deployment" that Onebox explicitly does not provision — on a single-host OSS baseline that is a second box the user must build, secure, and keep alive. Section 15 is also one of the largest gates (five tasks covering replication setup, lag, outage/resync, version and delete recovery, and metadata export).
That places it closer to the multi-host scope the proposal lists as a non-goal than the rest of the change.
Recommendation: either state the second-deployment prerequisite plainly in proposal.md so nobody reads MinIO as protected by default, or defer section 15 and leave MinIO honestly Run with an exported-metadata-only claim — dropping five tasks and the whole replication-lag and resync surface from the baseline.
| - daily backup at 02:00 UTC only when protection is explicitly enabled without | ||
| a schedule; | ||
| - seven daily, four weekly, and six monthly recoverable base generations, with | ||
| replay logs retained for every still-declared continuous recovery window; |
There was a problem hiding this comment.
High — the default retention grid contradicts the mandated driver-native retention semantics.
managed-data-protection requires Onebox to "invoke the qualified driver's repository-aware retention semantics", but this default is a calendar grid: seven daily, four weekly, six monthly. pgBackRest expresses retention as repo1-retention-full / -diff / -archive counts; PBM as count or age. Neither has a calendar grid.
Consequence: either the default cannot be expressed, or Onebox deletes objects itself inside a native repository — which the same requirement forbids and which breaks pgBackRest's own continuity bookkeeping.
Recommendation: state the default as generations-per-contract with an explicit per-driver mapping table, or drop the calendar grid entirely.
| replay logs retained for every still-declared continuous recovery window; | ||
| - restore proof expires after seven days; | ||
| - generated container logs use the Docker `local` driver with 20 MiB per file | ||
| and five files where no author policy exists; |
There was a problem hiding this comment.
Medium — no default restore-drill schedule against a seven-day proof expiry.
The policy selects a drill maximum age, the defaults set proof expiry at seven days, and no default drill cadence exists anywhere. Every graduated service therefore demotes to Run weekly unless the author separately declares a schedule.
Recommendation: default the drill schedule from the proof lifetime, and have validation refuse a drill maximum age shorter than the installed drill cadence.
| MinIO replica require future explicit destructive contracts and are not part | ||
| of rollback. | ||
|
|
||
| ## Open Questions |
There was a problem hiding this comment.
Note — Open Questions understates what is actually unresolved.
Only the OCI publication location and the reminder cadence are listed, both of which are packaging details. Genuinely open design decisions remain: how the pgBackRest binary reaches the PostgreSQL container; how one-time enablement restarts are authorized; which encryption form qualifies per recovery kind; and how the calendar retention default maps onto driver-native retention.
Recommendation: move those here or resolve them before approval.
vishr
left a comment
There was a problem hiding this comment.
Re-review of 0731e06 against the previously reviewed 8c1d84b.
All 21 findings from the first review are resolved. Strict OpenSpec validation still passes. Highlights:
- The pgBackRest blocker is answered with a service-image delivery class (
upstream-digest/derived-image/external-helper), a reproducible derived PostgreSQL image over the pinned upstream base with SBOM and provenance, and matching updates to proposal What Changes, Impact, the risk register, and task 7.1. - The Redis AOF hazard is answered with two separately qualified matrices — the sealed BASE/INCR/manifest contract with
preload-file, and a gated RDB fallback whose restore boots with AOF loading disabled, verifies the dataset, then enables AOF and waits for a completed rewrite. Tests for empty-AOF precedence and a pre-existing append directory are explicit, and Valkey is adapted rather than assumed compatible. - The restart problem gets its own requirement, its own operation kind
protection_enable, theprotection_enablement_restart_not_authorizedcode, and is explicitly unreachable from both the scheduled runner and recurring interruption permission. MariaDB gets driver-ownedlog_binconfiguration. - The calendar retention grid is replaced by minimum-generations plus recovery-window intent with a declared per-driver native mapping,
backup_retention_unsupported, and an explicit prohibition on deleting objects inside a native repository. - Drill capacity is handled honestly:
drill_deferred_capacityis distinct from backup corruption, an optional separate staging filesystem is permitted, and deferral correctly does not extend proof expiry, so the tier still degrades truthfully. - Promoting direct-recovery runbooks from post-graduation documentation to gate-blocking work, with 22.4 failing consistency when a graduated envelope lacks a passing runbook, was a note rather than a finding — good to see it folded in anyway.
Seven new findings, all introduced by this revision. Three change work:
- Digest resolution now applies to every run service, protected or not, which contradicts the additive-compatibility claim and adds a registry dependency to apply.
- The derived PostgreSQL image makes Onebox the delivery path for PostgreSQL patches, and the declared-version to derived-image mapping is undefined.
- Task 18.3's pruning root set omits manifest-referenced service image digests, so a service version upgrade silently voids every backup taken before it — the design already states this rule, only the task is stale.
The remaining four are scoping and honesty refinements: shared default drill slots with per-driver-only caps, tasks 7.1 and 17.1 overloaded to hold the count at 129, enablement evidence treated as a stored fact rather than an observed prerequisite, and the NATS conversion lacking an interruption declaration.
|
|
||
| Generation SHALL emit a separate, stable runtime document for each supported | ||
| Onebox-run service and SHALL keep it outside application releases. The document | ||
| SHALL resolve the driver's versioned image to an immutable registry digest at |
There was a problem hiding this comment.
Digest resolution now applies to every run service, protected or not — and it contradicts the compatibility claim.
This MODIFIED requirement makes every run-service runtime resolve its versioned image to a registry-confirmed digest at apply, but proposal.md:75-76 still states that existing projects "continue to load and keep their present behavior".
Consequences: apply now requires registry reachability where it previously rendered from a cached tag; the rendered service document digest changes whenever upstream moves a tag, so ordinary applies start surfacing drift; and the change lands on unprotected services that gain nothing from it.
Recommendation: gate digest resolution on a protection policy existing, or state the behavior change explicitly in the compatibility paragraph and add a cached/offline digest path with a typed failure for an unreachable registry.
| workflow above remain proposed until this change is implemented, documented, | ||
| strict-validated, and archived. | ||
|
|
||
| Compatibility is additive within `onebox.run/v1`. Existing projects continue |
There was a problem hiding this comment.
This paragraph is now inaccurate for existing unprotected services.
specs/runtime-generation/spec.md:12 makes every run-service runtime resolve to an immutable registry digest at apply, which changes rendered output, adds a registry dependency, and produces tag-moved drift for projects that declare no backup policy at all.
Recommendation: either scope digest resolution to protected services, or amend this paragraph to name the one behavior change existing projects will see. Both options are fine; silently leaving the claim as-is is not.
| a runnable service cheap while making the stronger `Managed` claim deliberately | ||
| expensive and independently testable. | ||
|
|
||
| Tools invoked by a process inside a service container are delivered in that |
There was a problem hiding this comment.
The derived PostgreSQL image makes Onebox the PostgreSQL patch path, and the version mapping is undefined.
internal/app/services.go:277 builds the image from the author's declared version, and postgres: 17 floats across upstream patch releases. This paragraph builds the derived image "from the catalogue's pinned upstream PostgreSQL digest". Nothing states how a declared version resolves to a published derived image, or what happens when an operator declares a PostgreSQL version for which no derived image has been built.
Consequence: either postgres: 17 stops tracking upstream patches — PostgreSQL security updates then arrive at Onebox's release cadence rather than upstream's — or the mapping is simply undefined at apply time. The new supply-chain risk entry covers build provenance but not patch latency.
Recommendation: state the declared-version to derived-image mapping, add a typed refusal for an unpublished version, and add patch latency to the risk entry with its intended response time.
|
|
||
| - [ ] 18.1 Apply the documented Docker `local` logging defaults of 20 MiB and five files to every generated workload, service, helper, and proxy container while preserving authored policies; add rendering and effective-origin golden tests. | ||
| - [ ] 18.2 Verify effective runtime log policies and report unsupported custom drivers as externally managed instead of protected; add status/doctor tests for default, authored, drifted, and unverifiable policies. | ||
| - [ ] 18.3 Build the image ownership and reachability graph from current and retained releases, services, proxy, scheduled jobs, helpers, and active/incomplete restore state; add graph tests proving every rollback and recovery root is retained. |
There was a problem hiding this comment.
This root set omits manifest-referenced service image digests, which silently voids older backups.
design.md:411-415 and the runtime-generation digest scenario both require the recorded digest to remain a pruning root "for as long as any manifest or staged restore depends on it". This task's root list is unchanged from the previous revision: current and retained releases, services, proxy, scheduled jobs, helpers, and active or incomplete restore state.
A service upgraded from 17.4 to 17.6 leaves the 17.4 digest referenced only by older protection manifests. Nothing in that list retains it, so housekeeping prunes it and every backup taken before the upgrade becomes unrestorable — discovered at restore time, which is the worst possible moment.
This is the sharpest of the new findings: the design already states the rule, only the task is stale.
Recommendation: add manifest-referenced service image digests to the root set here, and extend the "every rollback and recovery root is retained" test to cover a post-upgrade restore of a pre-upgrade backup.
| cannot preserve the stated minimum and never emulates native retention by | ||
| deleting objects inside a native repository; | ||
| - restore proof expires after seven days; | ||
| - restore drills run by default at 03:00 UTC every Sunday and Wednesday, with |
There was a problem hiding this comment.
Concurrent drills can jointly exhaust headroom that each per-driver cap individually approved.
The default places every protected service in the same 03:00 UTC Sunday and Wednesday window. Footprint caps are per-driver (design.md:495-497, task 18.5), and the protection lock is per-service, so nothing serializes drills across services. On a single host with PostgreSQL, ClickHouse, and Redis protected, three drills each pass their own headroom check and then contend for the same filesystem — and the result surfaces as drill_deferred_capacity or a failed drill for whichever loses, not as the scheduling problem it is.
Recommendation: add an aggregate host staging budget checked before admission, or serialize drills across services under the app lock, and spread the default slot per service rather than sharing one.
|
|
||
| ## 7. PostgreSQL qualification gate | ||
|
|
||
| - [ ] 7.1 Implement the exact PostgreSQL/pgBackRest matrix and reproducible Onebox PostgreSQL-plus-pgBackRest image from a pinned upstream digest, publishing immutable digest, SBOM, base/source provenance, repository encryption, S3 settings, and least-privilege runtime configuration; generate a state-bound archive-mode enablement/restart plan and add catalogue, supply-chain, rendering, approval-refusal, rollback, and provenance tests. |
There was a problem hiding this comment.
Task count held at exactly 129 while scope grew materially, and this task absorbed most of it.
The revision is 49 insertions against 49 deletions — every addition was folded into an existing checkbox. 7.1 now covers the compatibility matrix, a reproducible derived-image build, SBOM and base/source provenance publication, repository encryption, S3 settings, least-privilege runtime configuration, a state-bound archive-mode enablement and restart plan, and five distinct test classes. Task 17.1 is similarly overloaded with credentials, a health probe, a conversion contract, and the stream matrix.
Consequence: these are no longer independently verifiable units, which is the property the task list is organized around.
Recommendation: split 7.1 into image supply chain, pgBackRest configuration, and enablement plan; split 17.1 into runtime credentials/probe and legacy conversion. Let the total move — the round number is not a design goal.
| health verification. It SHALL NOT apply that delta or restart the service from | ||
| ordinary policy convergence, a recurring interruption window, or a scheduled | ||
| operation. Execution SHALL require a fresh strong approval delivered | ||
| independently of model-authored text. Passing enablement evidence SHALL remain |
There was a problem hiding this comment.
Enablement evidence reads as a stored historical fact rather than an observed prerequisite.
The approved operation "records prerequisite evidence", and passing evidence "SHALL remain a prerequisite" for backup execution and graduation. If PostgreSQL later restarts with archive_mode off, or a generated config mount drifts, the recorded evidence still passes. WAL continuity checks would eventually catch it, but the tier overclaims for the interval between.
Recommendation: require the effective prerequisite to be re-observed at backup and status time, with the enablement record serving as provenance for how it was established rather than as the check itself.
| ambiguously. RabbitMQ SHALL use a stable generated node name recorded as | ||
| protected identity. NATS SHALL use generated least-privilege account | ||
| credentials and a digest-pinned external CLI health probe when the upstream | ||
| service image cannot provide an in-container check. Existing unauthenticated |
There was a problem hiding this comment.
The NATS credential conversion is disruptive but declares no interruption.
Adding account credentials to a running NATS server drops existing client connections and requires workloads to pick up regenerated env-file credentials — in practice a workload redeploy. The conversion is correctly gated behind a state-bound strong approval, but it carries none of the expected-outage, rollback, and post-change health fields that the restart-bound enablement contract in managed-data-protection requires for an equivalent event.
Recommendation: give this conversion the same expected-interruption, rollback, and health-verification structure as the enablement plan.
vishr
left a comment
There was a problem hiding this comment.
Third review, of 21198ae against 0731e06.
All 7 findings from the second review are resolved, several more thoroughly than recommended. Strict validation passes, and the task count correctly moved 129 to 133 rather than being held at a round number — section 7 split 6 to 8, section 17 split 5 to 7.
Worth calling out:
- The PostgreSQL capability release record answers the version-mapping gap completely: authored selector plus exact upstream patch/base digest maps to pgBackRest version, derived digest, publication time, and support state. The added rule that enabling protection on an existing service maps only to its observed current base and "never smuggles a PostgreSQL patch upgrade into protection enablement" is a better answer than the one I suggested, and it closes a hazard I had not raised.
- Digest resolution is now correctly scoped to protection enablement, with a verified local-cache path for registry outage, and the compatibility paragraph in the proposal is accurate again.
- Manifest-referenced service-image digests became pruning roots in the design, the hygiene requirement, a new scenario, and task 18.3 with a post-patch restore test.
- Drill contention is answered with a host-wide atomic per-filesystem reservation ledger across all applications, plus stable per-service offsets inside separate six-hour windows, so distinct filesystems admit concurrently while shared ones serialize.
- Enablement evidence is now explicitly provenance rather than continuing proof, with
protection_prerequisite_driftedre-observed by preflight, status, doctor, and assurance.
Four new findings. One changes work.
The serious one is that protection disablement is now dangerous in a way enablement is not. Because tag-versus-digest rendering keys off whether a policy currently exists, removing a backup policy reverts PostgreSQL from the derived pgBackRest image to the plain upstream image while archive_mode remains on. PostgreSQL then retains WAL in pg_wal indefinitely because archive_command can no longer run, the volume fills, and the database stops. The enablement contract in this change is careful and well specified; it simply has no mirror. Removing a backup policy is a low-ceremony action an operator would reasonably expect to be safe.
The other three are a missing onboarding path (enabling protection on an existing PostgreSQL will usually require patching first), a published publication SLO with no pipeline task behind it, and two small clarifications.
| authorized convergence; remote backups, application | ||
| data, service volumes, and foreign units SHALL remain untouched. | ||
|
|
||
| #### Scenario: Protection is disabled |
There was a problem hiding this comment.
Disabling protection reverts PostgreSQL to an image without pgbackrest while archive_mode stays on — the WAL then fills the volume and the database stops.
This revision makes tag-versus-digest rendering conditional on a protection policy existing (specs/runtime-generation/spec.md:11-13). A service whose policy is removed becomes "a service without a protection policy" and reverts to version-tag rendering — for PostgreSQL, from the derived pgBackRest image back to plain postgres:17. Nothing in the disable path reverts archive_mode.
PostgreSQL with archive_mode=on and a failing archive_command retains WAL segments in pg_wal indefinitely until archiving succeeds. The data volume fills, and the database stops. Removing a backup policy is a low-ceremony action that an operator would reasonably expect to be safe.
This scenario covers only timer removal, and the handback paragraph at design.md:662-668 covers only repositories, replicas, and volumes. Neither covers the runtime prerequisites the enablement contract installed. The enablement contract is careful and well specified in one direction only.
Recommendation: add a symmetric disablement contract. Reverting a restart-bound prerequisite is itself restart-bound and needs its own plan and approval; it must be ordered before the image reverts; and until it runs, the service keeps its recorded digest rather than falling back to the tag. Add scenarios for disable-with-prerequisite-active and for refusing an image revert that would strand a live archive_command.
There was a problem hiding this comment.
Addressed in 9bb9793. Policy removal now enters durable disable-pending state and keeps the derived image plus archive support effective. The approved disable plan disables and verifies archive_mode and WAL recycling before image or unit removal, with unsafe-revert errors and phase-crash coverage.
|
|
||
| Generation SHALL emit a separate, stable runtime document for each supported | ||
| Onebox-run service and SHALL keep it outside application releases. A service | ||
| without a protection policy SHALL retain the existing version-tag image |
There was a problem hiding this comment.
Companion to the disablement finding above.
This is the mechanism: the tag-versus-digest choice keys off whether a protection policy currently exists, so removing a policy silently changes which image a durable service runs on its next apply. For PostgreSQL that swap also removes the pgbackrest binary that the still-configured archive_command depends on.
Recommendation: key the rule on whether the service has ever been protected and still holds retained manifests or installed prerequisites, not on the current presence of a policy. A previously protected service should keep its recorded digest until an explicit, approved disablement reverts its prerequisites.
There was a problem hiding this comment.
Addressed in 9bb9793. Runtime selection now keys on durable never-enabled, enabled, disable-pending, or disabled state rather than current policy presence. A previously protected service retains its digest and hooks through approved disablement, and retained manifests keep the exact restore digest.
| ownership. ClickHouse configuration is a generated, digest-bound mount read by | ||
| the service rather than an untracked host edit. | ||
|
|
||
| The PostgreSQL capability release record is the closed mapping from an authored |
There was a problem hiding this comment.
Enabling protection on an existing PostgreSQL will usually require a patch upgrade first, and that path is not written down.
Existing services map only to a derived image built over their observed current upstream base — a good rule, since it prevents protection enablement from smuggling in a patch upgrade. But an install that resolved postgres: 17 months ago sits on whatever patch was current then, and Onebox will not have published a derived image for every historical upstream patch digest.
The two scenarios then fire correctly: protection_service_image_unpublished, or a refusal requiring a separate patch plan. The practical result is that the common onboarding case is "patch first, then protect." That is the right safety behavior; it just is not stated anywhere an operator would encounter it before hitting the refusal.
Recommendation: state the patch-then-protect onboarding path in proposal.md and in the 22.3 documentation task, and decide explicitly whether the release record publishes derived images for a supported window of past patches or only forward from qualification.
There was a problem hiding this comment.
Addressed in 9bb9793. The plan now chooses forward-only publication from qualification, with no arbitrary historical backfill. Older existing PostgreSQL bases receive protection_service_patch_required and a separate same-major ob service apply --refresh-image <service> plan before protection enablement.
|
|
||
| ## 7. PostgreSQL qualification gate | ||
|
|
||
| - [ ] 7.1 Implement the closed PostgreSQL capability release record mapping an authored selector plus exact upstream patch/base digest to pgBackRest version, derived digest, publication time, and support state; build and publish reproducibly with SBOM/provenance, map existing services only to their observed current base while new services select the latest qualified base, poll upstream daily, report missed 72-hour security/seven-day routine targets, and test no-implicit-patch, moved tags, unpublished mappings, verified cache, provenance, and overdue status. |
There was a problem hiding this comment.
The derived-image publication SLO is now a public commitment with no owning task.
design.md:580-589 targets a 24-hour upstream poll, qualified publication within 72 hours of a supported security patch and seven days of another supported patch, and protection_image_update_overdue makes a missed target visible in every operator's status output.
This task covers polling upstream and reporting the miss. Nothing in the repository builds, tests, or schedules the actual derived-image republication, and section 22 (release evidence) has no entry for it. A published SLO with no pipeline behind it becomes a status code that is permanently red.
Recommendation: add a release-engineering task for the publication pipeline itself — trigger, reproducibility check, provenance signing, and a test that a newly observed upstream digest produces a qualified mapping — and name where it runs.
There was a problem hiding this comment.
Addressed in 9bb9793. Task 7.2 now owns the repository pipeline: scheduled and manual triggers, upstream digest detection, reproducibility builds, compatibility and restore smoke tests, SBOM, provenance, image push, atomic mapping publication, failure retention, and end-to-end SLO evidence.
| - **WHEN** an existing protected service is applied or restored while the registry is unreachable and its recorded digest is present locally | ||
| - **THEN** Onebox verifies and uses the exact local digest without resolving the mutable tag | ||
|
|
||
| #### Scenario: Derived image publication is overdue |
There was a problem hiding this comment.
Nit — state the deliberate choice that an overdue rebuild does not demote the tier.
This scenario reports protection_image_update_overdue "without silently upgrading the service" and correctly does not drop the service to Run. That is the right call: an overdue Onebox rebuild does not invalidate the operator's backups, and conflating base-image CVE status with data-protection status would make Managed mean two things at once.
Recommendation: say so explicitly in the service-tier requirement, so nobody reads Managed as a claim that the base image is currently patched.
There was a problem hiding this comment.
Clarified in 9bb9793. protection_image_update_overdue is separate security maintenance; it does not demote otherwise valid Managed recovery evidence, and Managed explicitly makes no claim of current patch currency.
| deleting objects inside a native repository; | ||
| - restore proof expires after seven days; | ||
| - restore drills run twice weekly by default. Canonicalization derives a stable | ||
| per-service UTC offset inside separate Sunday and Wednesday six-hour windows |
There was a problem hiding this comment.
Nit — specify which components make up the identity the drill offset derives from.
"A stable per-service UTC offset ... derived from the protected service identity" does not say whether that identity includes application and environment. Two applications on one host each running a service named postgres would otherwise derive the same slot and permanently serialize against each other.
The host-wide reservation ledger makes that safe rather than incorrect, so this is a predictable hot spot rather than a defect. Naming the identity components removes the ambiguity.
There was a problem hiding this comment.
Clarified in 9bb9793. The default drill offset now derives from canonical application identity, environment, declared service name, and driver. Task 19.3 includes cross-application same-name spread coverage.
vishr
left a comment
There was a problem hiding this comment.
Fourth review, of 9bb9793 against 21198ae.
All 6 findings from the third review are resolved, and the disablement fix is thorough. Strict validation passes; task count moved 133 to 137 (new 3.7, section 7 split 8 to 11).
The disablement contract is now genuinely symmetric with enablement, and in the right order: an explicit never-enabled / enabled / disable-pending / disabled runtime state that the image and prerequisites derive from instead of from whether the project text currently has a policy; a protection_disable operation kind; and a PostgreSQL sequence that stays on the derived image, disables archive_mode, restarts, and verifies WAL recycling before anything removes hooks or reverts the image. protection_image_revert_unsafe blocks the stranded-archive_command case directly, a crash between phases safely leaves the derived image installed, and disablement is explicitly barred from deleting manifests or the image digests those manifests need for restore. Task 7.6 tests full-volume prevention by name.
The other five also landed well. The forward-only publication window is now stated rather than implied, with protection_service_patch_required and a named resolving command. Task 7.2 gives the SLO a real owning pipeline — scheduled and manually dispatchable, double-build reproducibility check, smoke suite, SBOM, signed provenance, atomic mapping publication, and failure preserving the prior mapping — which is more than I asked for. The tier requirement now says outright that Managed is not a claim of patch currency. The drill offset derives from application identity, environment, service name, and driver, with a cross-application same-name spread test.
Four new findings. One matters.
The change now has an operational loop that does not close: there is no defined path to patch an already-protected PostgreSQL. design.md:108 asserts one exists, but task 7.3's --refresh-image plan is explicitly pre-protection and stops without enabling protection. Everything built in this revision — the daily upstream poll, the 72-hour security target, protection_image_update_overdue — exists to tell an operator that a patched derived image is available, and once protection is enabled they have no supported way to take it. The implied disable-patch-re-enable route costs four approved restarts and breaks WAL continuity, so in practice protected services will stay unpatched.
The other three concern the new disable-pending state: it is unbounded with undefined schedule behavior and an unbounded remote-storage growth path, it will make the watchdog alert on a service the operator deliberately turned off, and the patch plan that resolves protection_service_patch_required has a refusal scenario but no success scenario.
| enablement. A newly created protected service selects the latest qualified | ||
| mapping for its selector. The selected mapping binds into the plan, and | ||
| ordinary apply does not chase a moved upstream tag. Existing protected services | ||
| keep their recorded mapping until a separately planned patch update. A first |
There was a problem hiding this comment.
There is no path to patch an already-protected PostgreSQL, and the overdue SLO exists to tell operators to take one.
This line asserts that existing protected services "keep their recorded mapping until a separately planned patch update." That update is never defined and never tasked. Task 7.3's ob service apply --refresh-image plan is explicitly pre-protection onboarding — it "restart[s] and verif[ies] without enabling protection" and returns the subsequent protection command. Nothing covers patching a service that is already enabled.
Consequence: the protection_image_update_overdue machinery, the daily upstream poll, and the 72-hour security publication target all exist to tell an operator that a patched derived image is available. Once protection is on, they have no supported way to act on it. The only route implied by the current text is disable, patch, re-enable — which under the new disablement contract means an approved archive-mode reversal, a restart, a patch restart, an approved re-enablement restart, and a repository that loses its continuous WAL chain in between. That is not a security-patch path anybody will use, so protected services will simply stay unpatched.
A protected patch also has real content beyond the unprotected one: the new derived image must carry a pgBackRest version compatible with the existing stanza and repository, archive_mode and the archive command must remain effective across the restart, WAL continuity must not break, and manifests referencing the prior digest must stay retained as restore roots.
Recommendation: define a protected same-major patch plan as its own contract — pgBackRest/repository compatibility check against the recorded stanza, prerequisite preserved across the restart, WAL continuity asserted before and after, prior digest retained as a manifest root — and add a task for it in section 7 plus a qualification-matrix entry in 7.9. This is the loop that makes the publication SLO meaningful.
There was a problem hiding this comment.
Addressed in 18b99af. ob service apply --refresh-image is now context-sensitive: an enabled service gets a strongly approved same-major service_image_patch plan binding old/new PostgreSQL and pgBackRest digests, stanza/repository and volume/config identities, a fresh recovery point, and WAL position. One restart keeps archive prerequisites and schedules effective, verifies health, stanza, archive round trip, and WAL continuity, and retains prior manifest-root digests. Dedicated task 7.4 and qualification task 7.10 cover the path.
| Protection runtime state is explicit: `never-enabled`, `enabled`, | ||
| `disable-pending`, or `disabled`. The live image and installed prerequisites | ||
| derive from that state, not merely from whether the current project text has a | ||
| policy. Removing a policy moves an enabled service to `disable-pending`, reports |
There was a problem hiding this comment.
disable-pending is unbounded, and what happens to the installed schedules while a service sits in it is undefined.
Removing a policy moves the service to disable-pending and keeps "the recorded service digest, configuration, archive hook, credentials, and schedules required to prevent data-volume growth or a broken engine contract." Keeping WAL archiving effective is correct — stopping it is exactly the failure this revision fixed. But the state persists until an approval arrives, and nothing bounds how long that is or says what the base-backup and drill timers do meanwhile.
Both branches have a cost. If base backups keep firing, the operator continues paying for storage in a repository they believe they have disconnected. If they stop, WAL keeps accumulating with no new verified generation to anchor retention — and retention only acts "after a new recoverable generation verifies" — so the archive grows without bound in remote storage. An operator who removes a policy and never runs the approval gets a quietly growing bill either way, on a change whose stated position is that backup bytes are paid for by the user.
Recommendation: state explicitly which schedules survive disable-pending and why; bound the state with a reported age and a resolving command; and either keep base backups running so native retention still has an anchor, or cap archive accumulation with a typed warning when a pending disablement exceeds its bound.
There was a problem hiding this comment.
Addressed in 18b99af. disable-pending now retains the last effective target/retention contract and continues base-backup, continuous-archive, and native-prune schedules so growth stays retention-bounded; restore drills stop. State records request time and a 24-hour action deadline, status exposes continued storage plus the exact command, and overdue state is typed without unsafe automatic disablement.
| - **WHEN** backup preflight, status, doctor, or assurance observes the service | ||
| - **THEN** it reports `protection_prerequisite_drifted`, blocks dependent backup work, and reports the service `Run` until a newly approved enablement restores and verifies the prerequisite | ||
|
|
||
| ### Requirement: Protection disablement preserves service safety and recovery assets |
There was a problem hiding this comment.
disable-pending reports Run, which will make the watchdog alert on a service the operator deliberately disabled.
Assurance checks backup freshness and restore-proof freshness, and emits transition notifications through the webhook contract with a 24-hour reminder. A service parked in disable-pending reports Run, its drill proof expires on schedule, and its backups go stale by design. Nothing here or in local-continuous-assurance says those checks are suppressed or reframed for a pending disablement.
Consequence: removing a backup policy starts a recurring alert stream about the service you just turned off, which is precisely the kind of noise that trains operators to ignore the watchdog.
Recommendation: make disable-pending a distinct assurance state that reports the pending disablement and its resolving command rather than stale-backup and stale-drill failures, and exclude it from freshness transitions.
There was a problem hiding this comment.
Addressed in 18b99af. Assurance treats disable-pending as its own action state: service/unit/deadline/storage checks continue, while backup and drill freshness transitions are suppressed. Entry emits one pending transition; crossing the deadline emits protection_disablement_overdue with bounded reminders and the resolving command.
|
|
||
| #### Scenario: Existing service has only a newer derived mapping | ||
| - **WHEN** an existing service enables protection but the only published derived mapping would change its observed upstream patch/base digest | ||
| - **THEN** planning refuses with `protection_service_patch_required` and names a separate same-major `ob service apply --refresh-image` patch plan rather than upgrading as a side effect of protection enablement |
There was a problem hiding this comment.
The patch-required refusal has a scenario; the patch plan that resolves it does not.
The MODIFIED requirement states that Onebox "SHALL require a separate state-bound, same-major service patch plan before protection," and this scenario exercises the refusal. No scenario exercises the plan succeeding, so the behavior an operator actually runs — volume and rollback identity preserved, exact qualified upstream digest resolved, restart verified, protection deliberately not enabled, major upgrade refused — is normative only in design.md:113-120 and task 7.3.
That is the same shape as the MongoDB replica-set gap from the first review: real behavior specified in design and tasks with no requirement scenario behind it, which task 22.5's claims-versus-catalogue consistency test cannot check.
Recommendation: add a success scenario for the patch plan and a refusal scenario for a cross-major attempt.
There was a problem hiding this comment.
Addressed in 18b99af. The runtime spec now has normative scenarios for a successful pre-protection same-major refresh and cross-major refusal with service_major_upgrade_unsupported, covering exact digest resolution, preserved volume/rollback identity, verified restart, protection remaining disabled, and the subsequent protection command. It also adds protected-patch success and compatibility-refusal scenarios.
vishr
left a comment
There was a problem hiding this comment.
Fifth review, of 18b99af against 9bb9793.
All 4 findings from the fourth review are resolved. Strict validation passes; 138 tasks (new 7.4, section 7 now 12).
The protected-patch loop now closes properly, and the design carries the content that makes it real rather than nominal: a service_image_patch operation kind, a plan binding old and new service and pgBackRest digests, observed stanza and repository identity, a fresh pre-patch recovery point and WAL position, a preflight that refuses unless the new pgBackRest is compatible with the existing repository, one approved restart with archive mode, command, credentials, and schedules held effective throughout, and completion gated on health, unchanged archive configuration, a stanza check, an archive round trip, and continuous WAL evidence spanning the restart. The line that matters most is explicit: this path "never disables or re-enables protection and never starts a new WAL chain."
disable-pending is now bounded and honest. Base-backup, archive, and prune schedules continue under the last effective target and retention contract so remote growth stays retention-bounded, drills stop because their proof no longer qualifies the service, the state records a request time and 24-hour deadline, status makes the continued storage activity explicit, and protection_disablement_overdue reports the miss — with an explicit statement that no timeout may bypass approval or revert a live prerequisite. That was the right branch of the choice.
The assurance interaction is handled at the right layer: disable-pending keeps health, unit-safety, deadline, and storage checks but drops backup and drill freshness from transitions, emits one transition on entry, suppresses freshness transitions while pending, and reminds on the overdue state instead. And the pre-protection patch plan now has both a success scenario and a service_major_upgrade_unsupported refusal.
Three new findings, all narrower than the last round.
The one worth acting on is scope: the new "Protected service image maintenance preserves recovery continuity" requirement is written for every enabled service, but only PostgreSQL implements it. Sections 8 through 17 have no refresh task. That matters most for MySQL, MariaDB, and MongoDB, where helper compatibility is bound to the exact server version — patching a protected image can move the server out of its qualified matrix while protection still reports Managed.
The other two are local: artifacts deliberately retained during disable-pending are not excluded from drift evaluation and could fail closed on unrelated applies, and --refresh-image has no defined behavior in the disable-pending state.
|
|
||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Protected service image maintenance preserves recovery continuity |
There was a problem hiding this comment.
This requirement is normative for every enabled service, but only PostgreSQL implements it.
"Refreshing an enabled service image SHALL use a state-bound, strongly approved, same-major service_image_patch plan" applies to all eleven drivers. Grepping the task list, service_image_patch and --refresh-image appear only in 2.3 (operation-graph registration), 7.3, 7.4, and 22.3. Sections 8 through 17 have no refresh task and no qualification-matrix entry.
This is not merely missing coverage — for several drivers the hazard is the same class the PostgreSQL path exists to prevent. XtraBackup compatibility is bound to the exact MySQL server version and edition (task 8.1's own matrix), and PBM compatibility is bound to the mongod version (10.3). Patching a protected MySQL or MongoDB image can therefore move the server out of its qualified helper matrix while protection still reports Managed, with the break surfacing at the next backup or, worse, at restore.
The verification content also differs per driver: PostgreSQL proves stanza and repository compatibility plus WAL continuity; MySQL/MariaDB would need binlog continuity and helper-matrix revalidation; MongoDB would need PBM sidecar compatibility and oplog continuity; the snapshot drivers need much less.
Recommendation: decide explicitly whether this requirement covers all delivery classes or only derived-image. If it covers all, add a refresh entry to each driver gate with that driver's continuity and helper-compatibility checks, plus a matrix line in each X.N E2E task. If it covers only derived images, scope the requirement text accordingly and add a separate, simpler upstream-digest patch contract — protected services on upstream-digest drivers still need some supported way to take a patch.
Related: the protection_service_patch_available scenario at line 93 is written for a "newer qualified same-major derived mapping", so upstream-digest drivers get no patch-available signal at all.
There was a problem hiding this comment.
Addressed in bafade4. The contract now covers derived-image, upstream-digest, and external-helper delivery with no default transition. Every protected patch must be an exact driver-qualified non-major current-to-candidate service/applicable-helper transition with its own recovery, compatibility, continuity, and rollback evidence. Sections 8 through 17 each gained a dedicated patch task and an E2E matrix entry; an unqualified transition refuses without demoting current recovery evidence.
|
|
||
| ## 8. MySQL qualification gate | ||
|
|
||
| - [ ] 8.1 Implement the exact MySQL/Percona XtraBackup/server-edition/storage-engine compatibility matrix with a pinned helper whose UID/GID, exact data-volume mounts, socket/network scope, credentials, privileges, locks, and resources are contract-bound; add supported-version, mount isolation, mixed-engine, incompatible-edition, unreadable-metadata, and conflict tests. |
There was a problem hiding this comment.
No refresh-image task in this gate, though XtraBackup compatibility is version-bound to the server.
specs/runtime-generation/spec.md:111 makes the protected service_image_patch plan normative for every enabled service, but only section 7 implements one. This task defines an exact "MySQL/Percona XtraBackup/server-edition/storage-engine compatibility matrix" — which is precisely what a MySQL image patch can move the server out of, while protection continues to report Managed.
The same applies to MariaDB (9.1) and MongoDB, where PBM compatibility is bound to the mongod version (10.3).
Recommendation: add a refresh-image task to each gate covering that driver's helper-matrix revalidation and continuity proof (binlog position for MySQL/MariaDB, oplog for MongoDB), and add the patch case to each gate's E2E matrix. See the requirement comment for the scoping decision this depends on.
There was a problem hiding this comment.
Addressed in bafade4. MySQL task 8.4 now revalidates the exact server/edition/engine/XtraBackup matrix and binlog continuity, with protected patching in matrix 8.5. MariaDB tasks 9.4/9.5 do the same for mariadb-backup and binlogs; MongoDB tasks 10.6/10.7 bind mongod/PBM compatibility and oplog continuity. Equivalent driver-specific tasks were added through section 17.
| - **WHEN** a protected service runtime is previewed | ||
| - **THEN** output includes the redacted generated schedule, target reference, retention, helper provenance, and artifact digest | ||
|
|
||
| #### Scenario: Generated artifact drifts |
There was a problem hiding this comment.
Artifacts deliberately retained during disable-pending will read as drift.
The disablement contract keeps the recorded digest, configuration, archive hook, credentials, and the base-backup, archive, and prune units installed after the policy is removed from the project. But the policy is gone from the project text, so the plan no longer declares those artifacts — and this scenario requires execution to fail before mutation when a target-side protection unit differs from the plan-bound artifact.
Consequence: after removing a backup policy, ordinary applies could fail closed on exactly the artifacts the disablement contract preserves on purpose, blocking unrelated deploys until the operator approves the disablement. That converts a deliberate safety hold into a deployment outage.
Recommendation: state that artifacts retained by disable-pending are plan-bound from durable lifecycle state rather than from current project text, so they are not drift; and add a drift-versus-pending test to 3.5 and 3.7.
There was a problem hiding this comment.
Addressed in bafade4. Plan generation now derives desired artifacts from current project intent plus durable lifecycle state. During disable-pending, the last-effective image, hooks, configuration, target/retention, and required schedules remain plan-bound desired state, so matching retained artifacts do not block unrelated applies; divergence from that retained projection is still real drift. Tasks 3.5 and 3.7 include both paths.
|
|
||
| #### Scenario: Protection policy is removed while prerequisites remain active | ||
| - **WHEN** an apply removes the policy from a service with installed protection prerequisites | ||
| - **THEN** the service enters `disable-pending`, retains its recorded digest and working hooks, reports `Run`, and requires a state-bound approved disablement before ordinary tag rendering |
There was a problem hiding this comment.
--refresh-image behavior on a disable-pending service is undefined.
Command dispatch is now defined for two of the four lifecycle states: never-enabled and disabled get the pre-protection patch plan, enabled gets the protected derived-to-derived plan. disable-pending has no defined behavior, yet it is the state in which a service can sit indefinitely with archive prerequisites still effective — the worst state in which to guess.
Recommendation: refuse the refresh while a disablement is pending, naming the pending state, its deadline, and the disablement command as the resolving action.
There was a problem hiding this comment.
Addressed in bafade4. ob service apply --refresh-image now refuses a disable-pending service with service_image_patch_disable_pending, reports pending age and deadline, and names the exact disablement command without changing image or schedules. The lifecycle and integration tasks include this refusal.
Establish shared schema, capability, fencing, journaling, active-volume, and scheduled-runner contracts before adding the first PostgreSQL backend. Unqualified drivers remain Run and mutations fail closed.
Project external credentials through least-privilege release files, bind protected service rendering to observed lifecycle state, and make recurring operations occurrence-specific. Fail closed on foreign units, unavailable images, stale timing, and credential cleanup errors.
feat(backup): add S3 target adapter
feat(protection): add lifecycle foundation
docs(openspec): define OSS production baseline
Summary
MongoDB may run as a one-member replica set because its native transaction and PBM paths require that mode; Onebox does not add a second MongoDB node.
This PR contains planning artifacts only; it does not implement or deploy the proposed behavior.
Validation