From d0e6de6d1253891b56c615922fc77f16e01dd2b1 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Tue, 18 Aug 2026 21:21:09 -0700 Subject: [PATCH 1/3] fix(services): make persistence.mode decide storage and server persistence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit persistence.mode was declared, validated, defaulted, and then ignored by every managed service. A service that said persistence: mode: ephemeral rendered the same durable Docker volume as one that said nothing, and redis and valkey additionally fsynced every write into it through a hardcoded --appendonly yes. Measured across all ten drivers before this change: every one of them owned a durable volume under mode: ephemeral. The field governs volume ownership for workloads through runtime.go; services never consulted it. Two owners now, and the split is what makes an override safe. The mode owns Onebox's storage. An ephemeral service gets no durable volume, neither materialised in the project by the loader nor rendered by the generator, and a driver setting cannot buy one back — a server flag has no business redefining what Onebox claims about data lifetime. The mode supplies server defaults, and authored settings replace them. redis and valkey run appendonly yes under durable, exactly as before, and appendonly no with snapshots off under ephemeral. An author who sets appendonly explicitly gets their value and only their value: options are merged before rendering rather than appended after, so the command carries one value per option. Appending is what produced `--appendonly yes --appendonly no` and made an author compensate for the driver in the first place. Six frozen corpus digests move, all of them redis=. Every application digest and every other service digest is byte-identical, which is the evidence this reached only what it claims to. Durable redis and valkey do render one difference: --appendonly "yes" where the literal command said --appendonly yes. The value now travels the same quoting path as an authored setting, because it is now the same mechanism. redis-server reads both identically. It does move the service digest, so those services recreate on the next deploy — and #76's health-check fix, which this is stacked on, already recreates them in the same release, so the marginal cost is nothing. Not done here, and deliberately: canonical and doctor do not yet report the effective persistence value or name a divergence between an authored setting and the declared mode. That reporting is additive and is what makes an override visible rather than merely permitted; it wants its own change. Refs #75. Co-Authored-By: Claude Opus 5 (1M context) --- internal/app/load.go | 2 +- internal/app/services.go | 69 ++++++++++++++-- internal/app/services_test.go | 87 ++++++++++++++++++++ internal/app/testdata/contract-verdicts.json | 12 +-- 4 files changed, 157 insertions(+), 13 deletions(-) diff --git a/internal/app/load.go b/internal/app/load.go index 332f3384..229115de 100644 --- a/internal/app/load.go +++ b/internal/app/load.go @@ -560,7 +560,7 @@ func crossFieldRules(p *Spec) error { // Materialise the durable volume in the project rather than only in the // generated runtime, so the canonical form, the preflight collision // check and the renderer all name the same volume. - if d.dataPath != "" && len(svc.Volumes) == 0 { + if d.dataPath != "" && len(svc.Volumes) == 0 && !serviceIsEphemeral(svc) { svc.Volumes = []string{"data"} p.Services[name] = svc p.derivedPaths["services."+name+".volumes"] = OriginDefault diff --git a/internal/app/services.go b/internal/app/services.go index 15dc2d70..b4de01cf 100644 --- a/internal/app/services.go +++ b/internal/app/services.go @@ -84,6 +84,14 @@ type driver struct { urlUser string // settings maps declared settings onto the driver's own mechanism. settings settingsForm + // persistenceOptions are the server options this driver derives from + // persistence.mode, keyed by mode. They are defaults: an authored setting of + // the same name replaces the value rather than being appended beside it, so + // the rendered command carries one value per option. + // + // A driver with none leaves its server's own defaults alone; the mode still + // decides whether Onebox gives it a durable volume. + persistenceOptions map[string]map[string]string } type settingsForm int @@ -100,6 +108,20 @@ const ( settingsUnsupported ) +// serviceIsEphemeral reports a service the author declared disposable. +// +// persistence.mode is the declaration Onebox can act on, and until now no +// managed service consulted it: every driver rendered a durable volume whatever +// the mode said, so `mode: ephemeral` changed nothing at all. The mode owns +// this decision and a driver setting cannot override it — a server flag has no +// business redefining what Onebox claims about data lifetime. +// +// Omitted persistence stays durable, which is what defaults.go already fills in, +// so existing projects render exactly as before. +func serviceIsEphemeral(s Service) bool { + return s.Persistence != nil && s.Persistence.Mode == "ephemeral" +} + // drivers is the catalogue. A name that is not here is refused by the loader. var drivers = map[string]driver{ "postgres": { @@ -134,18 +156,31 @@ var drivers = map[string]driver{ // rollout converges onto it. SET proves the write path; EX bounds the // key so the probe cannot accumulate one. health: []string{"CMD-SHELL", "redis-cli -a \"$REDIS_PASSWORD\" set ob:health 1 EX 30 | grep -qx OK"}, - command: []string{"sh", "-c", "exec redis-server --requirepass \"$REDIS_PASSWORD\" --appendonly yes"}, + command: []string{"sh", "-c", "exec redis-server --requirepass \"$REDIS_PASSWORD\""}, secretEnv: []string{"REDIS_PASSWORD"}, urlUser: "default", scheme: "redis", settings: settingsRedisFlag, + // Durable keeps the append-only log this driver has always run with. + // Ephemeral turns off both persistence mechanisms: a service declared + // disposable should not pay an fsync per write, nor grow an AOF in a + // container layer nothing intends to read back. + persistenceOptions: map[string]map[string]string{ + "durable": {"appendonly": "yes"}, + "ephemeral": {"appendonly": "no", "save": ""}, + }, }, "valkey": { majorUpgradeInPlace: true, image: "valkey/valkey", port: 6379, dataPath: "/data", // Same failure mode and the same probe as redis; see the note there. health: []string{"CMD-SHELL", "valkey-cli -a \"$REDIS_PASSWORD\" set ob:health 1 EX 30 | grep -qx OK"}, - command: []string{"sh", "-c", "exec valkey-server --requirepass \"$REDIS_PASSWORD\" --appendonly yes"}, + command: []string{"sh", "-c", "exec valkey-server --requirepass \"$REDIS_PASSWORD\""}, secretEnv: []string{"REDIS_PASSWORD"}, urlUser: "default", scheme: "redis", settings: settingsRedisFlag, + // Same contract as redis; see the note there. + persistenceOptions: map[string]map[string]string{ + "durable": {"appendonly": "yes"}, + "ephemeral": {"appendonly": "no", "save": ""}, + }, }, "mongodb": { database: true, @@ -315,7 +350,7 @@ func (p *Spec) renderService(n Names, name string, s Service, selectedImage stri } command := append([]string(nil), d.command...) - if err := applySettings(name, d, s.Settings, env, &command); err != nil { + if err := applySettings(name, d, s, s.Settings, env, &command); err != nil { return nil, err } if len(env) > 0 { @@ -332,7 +367,7 @@ func (p *Spec) renderService(n Names, name string, s Service, selectedImage stri } volumes := map[string]any{} - if d.dataPath != "" { + if d.dataPath != "" && !serviceIsEphemeral(s) { vol := dataVolume(s) full := n.ServiceVolume(name, vol) svc["volumes"] = []string{full + ":" + d.dataPath} @@ -396,10 +431,32 @@ func identityEnv(key string, d driver, app string) map[string]any { // applySettings puts declared settings where the driver actually reads them. // A driver with no safe mechanism refuses rather than accepting values it // would silently ignore. -func applySettings(name string, d driver, settings map[string]any, env map[string]any, command *[]string) error { - if len(settings) == 0 { +func applySettings(name string, d driver, s Service, settings map[string]any, env map[string]any, command *[]string) error { + // The mode's options first, then the authored ones over them. Merging here + // rather than appending is what keeps one value per option in the rendered + // command: appending produced `--appendonly yes --appendonly no` for an + // author who set the flag the driver had already fixed. + effective := map[string]any{} + mode := "durable" + if s.Persistence != nil && s.Persistence.Mode != "" { + mode = s.Persistence.Mode + } + for k, v := range d.persistenceOptions[mode] { + effective[k] = v + } + for k, v := range settings { + effective[k] = v + } + if len(effective) == 0 { + return nil + } + // A driver with no mechanism refuses only what the author asked for. Mode + // options are Onebox's own and are never declared for such a driver, so + // this cannot refuse a project that set nothing. + if d.settings == settingsUnsupported && len(settings) == 0 { return nil } + settings = effective keys := make([]string, 0, len(settings)) for k := range settings { keys = append(keys, k) diff --git a/internal/app/services_test.go b/internal/app/services_test.go index e1736521..47c07a3d 100644 --- a/internal/app/services_test.go +++ b/internal/app/services_test.go @@ -391,3 +391,90 @@ func renderServices(t *testing.T, src string) map[string][]byte { } return r.Services } + +// persistence.mode was declared, validated, defaulted — and then ignored by +// every managed service. `mode: ephemeral` rendered the same durable volume as +// a service that declared nothing, and redis and valkey additionally fsynced +// every write into it through a hardcoded --appendonly yes. +func TestEphemeralServicesOwnNoDurableVolume(t *testing.T) { + for _, svc := range []string{ + "postgres", "mysql", "mariadb", "mongodb", "clickhouse", + "redis", "valkey", "rabbitmq", "meilisearch", "nats", + } { + version := map[string]string{ + "postgres": "17", "mysql": "8.0", "mariadb": "11.4", "mongodb": "8.0", + "clickhouse": "25.3", "redis": "8-alpine", "valkey": "8-alpine", + "rabbitmq": "4", "meilisearch": "1.10", "nats": "2.10", + }[svc] + rendered := renderServices(t, "api_version: onebox.run/v1\napp: sample\n"+ + "environments: {production: {server: root@h}}\nworkloads:\n web: {role: application, image: x:1}\n"+ + "services:\n "+svc+":\n version: \""+version+"\"\n persistence: {mode: ephemeral}\n") + if strings.Contains(string(rendered[svc]), "_"+svc+"_data") { + t.Fatalf("%s declared ephemeral still owns a durable volume:\n%s", svc, rendered[svc]) + } + } +} + +// Omitted persistence stays durable, so a project that never declared it keeps +// the volume and the append-only log it has always had. +func TestDurableRedisKeepsItsVolumeAndAppendOnlyLog(t *testing.T) { + for _, decl := range []string{ + " redis: {version: 8-alpine}\n", + " redis: {version: 8-alpine, persistence: {mode: durable}}\n", + } { + rendered := renderServices(t, "api_version: onebox.run/v1\napp: sample\n"+ + "environments: {production: {server: root@h}}\nworkloads:\n web: {role: application, image: x:1}\nservices:\n"+decl) + doc := string(rendered["redis"]) + if !strings.Contains(doc, "_redis_data") { + t.Fatalf("durable redis lost its volume:\n%s", doc) + } + if !strings.Contains(doc, `--appendonly "yes"`) { + t.Fatalf("durable redis lost its append-only log:\n%s", doc) + } + } +} + +// A disposable cache should not pay an fsync per write, nor grow a log nothing +// intends to read back. +func TestEphemeralRedisFamilyDisablesBothPersistenceMechanisms(t *testing.T) { + for _, svc := range []string{"redis", "valkey"} { + rendered := renderServices(t, "api_version: onebox.run/v1\napp: sample\n"+ + "environments: {production: {server: root@h}}\nworkloads:\n web: {role: application, image: x:1}\n"+ + "services:\n "+svc+": {version: 8-alpine, persistence: {mode: ephemeral}}\n") + doc := string(rendered[svc]) + if !strings.Contains(doc, `--appendonly "no"`) { + t.Fatalf("ephemeral %s still runs the append-only log:\n%s", svc, doc) + } + if !strings.Contains(doc, `--save ""`) { + t.Fatalf("ephemeral %s still snapshots:\n%s", svc, doc) + } + } +} + +// An authored setting replaces the mode's default rather than being appended +// beside it. Appending produced `--appendonly yes --appendonly no`, which is +// what made an author compensate for the driver in the first place. +func TestAuthoredSettingOverridesTheModeDefaultExactlyOnce(t *testing.T) { + rendered := renderServices(t, `api_version: onebox.run/v1 +app: sample +environments: {production: {server: root@h}} +workloads: + web: {role: application, image: x:1} +services: + redis: + version: 8-alpine + persistence: {mode: ephemeral} + settings: {appendonly: "yes"} +`) + doc := string(rendered["redis"]) + if n := strings.Count(doc, "--appendonly"); n != 1 { + t.Fatalf("--appendonly appears %d times, want exactly 1:\n%s", n, doc) + } + if !strings.Contains(doc, `--appendonly "yes"`) { + t.Fatalf("the authored setting did not win:\n%s", doc) + } + // The mode still owns storage: an explicit server flag does not buy a volume. + if strings.Contains(doc, "_redis_data") { + t.Fatalf("a driver setting overrode the declared data lifetime:\n%s", doc) + } +} diff --git a/internal/app/testdata/contract-verdicts.json b/internal/app/testdata/contract-verdicts.json index 4838e49e..190001c8 100644 --- a/internal/app/testdata/contract-verdicts.json +++ b/internal/app/testdata/contract-verdicts.json @@ -382,7 +382,7 @@ { "case": "conformance/settings key that is a real driver flag", "loads": true, - "digest": "37ef191260abddc6f674ea8bed3bb82970358f8395df2d5a7f5b9b5142268b47 redis=69fece9ab514b04f" + "digest": "37ef191260abddc6f674ea8bed3bb82970358f8395df2d5a7f5b9b5142268b47 redis=f5e4171f39cd0dcb" }, { "case": "conformance/settings key with a shell metacharacter", @@ -517,12 +517,12 @@ { "case": "corpus/authentik.yml", "loads": true, - "digest": "6d3a2518ab077033b35018bb81aa9702a641a2ad7433468afacb6070083d60a8 postgres=dc4f8448b8b82b4a redis=191161a0c85c6c0d" + "digest": "6d3a2518ab077033b35018bb81aa9702a641a2ad7433468afacb6070083d60a8 postgres=dc4f8448b8b82b4a redis=d2660eeb4faa49fe" }, { "case": "corpus/ext-authentik-managed.yml", "loads": true, - "digest": "f6b0b547d0bc7d55b8309e57f4f928a57e2ce44dd4fecc773599c9785580083f postgres=dc4f8448b8b82b4a redis=191161a0c85c6c0d" + "digest": "f6b0b547d0bc7d55b8309e57f4f928a57e2ce44dd4fecc773599c9785580083f postgres=dc4f8448b8b82b4a redis=d2660eeb4faa49fe" }, { "case": "corpus/ext-authentik.yml", @@ -542,7 +542,7 @@ { "case": "corpus/ext-immich-sourced.yml", "loads": true, - "digest": "b852204b8ab417c0b5ca7c162108f95189829c69f8b7284d58120fe1e17c47d9 postgres=76af15324857fa90 redis=0281e909253b33c5" + "digest": "b852204b8ab417c0b5ca7c162108f95189829c69f8b7284d58120fe1e17c47d9 postgres=76af15324857fa90 redis=c7c8bf8044d5342a" }, { "case": "corpus/ext-immich.yml", @@ -552,7 +552,7 @@ { "case": "corpus/ext-n8n.yml", "loads": true, - "digest": "d9b22f801a91ee7c4f6e9d24811e9454374067466263cc08cbc18cb9aefc8241 postgres=809549d286e2dbdc redis=3cef59e7d2733f0a" + "digest": "d9b22f801a91ee7c4f6e9d24811e9454374067466263cc08cbc18cb9aefc8241 postgres=809549d286e2dbdc redis=86933b446609e6d8" }, { "case": "corpus/ext-paperless.yml", @@ -612,7 +612,7 @@ { "case": "corpus/penpot.yml", "loads": true, - "digest": "10d0d86d56452027d9d317f6d77d0a696ed95eb2595e9cf039db6d187ff338e6 postgres=fc584b1b50db23a6 redis=be4534525f623f76" + "digest": "10d0d86d56452027d9d317f6d77d0a696ed95eb2595e9cf039db6d187ff338e6 postgres=fc584b1b50db23a6 redis=fcccb6a023ae5734" }, { "case": "corpus/pursue.yml", From 4bb812bf88aae436e6fb7e5000c898f21e554d9b Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Tue, 18 Aug 2026 21:35:25 -0700 Subject: [PATCH 2/3] fix(services): keep persistence on for every mode except ephemeral The mode table had keys for durable and ephemeral. The grammar allows a third: external, meaning the operator owns this data outside Onebox. A lookup that missed returned nothing, and because the append-only flag had just moved out of the driver's literal command into that table, `mode: external` rendered exec redis-server --requirepass "$REDIS_PASSWORD" with no --appendonly at all, while still mounting the durable volume. Redis falls back to snapshots only. That is a silent durability downgrade on the one mode that exists to say the data matters, and it would have reached anyone whose service Onebox does not itself back up. Any mode added later would have hit the same nil map. So the rule is inverted: ephemeral turns persistence off, and everything else keeps the durable options. A table that does not recognise a mode now errs toward keeping data. The new test also fails if ePersistence grows, so a fourth mode cannot be added without deciding what it means here. Found by review of this branch, not by a test. Refs #75. Co-Authored-By: Claude Opus 5 (1M context) --- internal/app/services.go | 16 +++++++++++----- internal/app/services_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/internal/app/services.go b/internal/app/services.go index b4de01cf..a860f12a 100644 --- a/internal/app/services.go +++ b/internal/app/services.go @@ -437,11 +437,17 @@ func applySettings(name string, d driver, s Service, settings map[string]any, en // command: appending produced `--appendonly yes --appendonly no` for an // author who set the flag the driver had already fixed. effective := map[string]any{} - mode := "durable" - if s.Persistence != nil && s.Persistence.Mode != "" { - mode = s.Persistence.Mode - } - for k, v := range d.persistenceOptions[mode] { + // Only ephemeral turns persistence off. Every other mode — durable, + // external, and any mode added later — keeps the durable options, because a + // mode this table does not know about must not silently disable a server's + // persistence. `external` is exactly that case: it means the operator covers + // this data, and reading it as "no options" rendered a durable volume with + // the append-only log switched off. + options := d.persistenceOptions["durable"] + if serviceIsEphemeral(s) { + options = d.persistenceOptions["ephemeral"] + } + for k, v := range options { effective[k] = v } for k, v := range settings { diff --git a/internal/app/services_test.go b/internal/app/services_test.go index 47c07a3d..7dc50342 100644 --- a/internal/app/services_test.go +++ b/internal/app/services_test.go @@ -478,3 +478,27 @@ services: t.Fatalf("a driver setting overrode the declared data lifetime:\n%s", doc) } } + +// Only ephemeral turns persistence off. `external` means the operator covers +// this data, and a mode table that did not know the word rendered a durable +// volume with the append-only log silently switched off — a durability +// downgrade on the one mode that says the data matters. +func TestOnlyEphemeralDisablesServerPersistence(t *testing.T) { + for _, mode := range []string{"durable", "external"} { + rendered := renderServices(t, "api_version: onebox.run/v1\napp: sample\n"+ + "environments: {production: {server: root@h}}\nworkloads:\n web: {role: application, image: x:1}\n"+ + "services:\n redis: {version: 8-alpine, persistence: {mode: "+mode+"}}\n") + doc := string(rendered["redis"]) + if !strings.Contains(doc, `--appendonly "yes"`) { + t.Fatalf("mode %q disabled the append-only log:\n%s", mode, doc) + } + if !strings.Contains(doc, "_redis_data") { + t.Fatalf("mode %q lost its volume:\n%s", mode, doc) + } + } + // Every value the grammar permits is covered above or by the ephemeral + // tests, so a new mode cannot be added without this failing. + if len(ePersistence) != 3 { + t.Fatalf("persistence modes changed to %v; extend these tests before shipping", ePersistence) + } +} From df880f41f2a97d6c09aebe4e17d6f7f7fa585154 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Tue, 18 Aug 2026 21:47:37 -0700 Subject: [PATCH 3/3] fix(services): refuse ephemeral combinations that cannot mean anything Two shapes the loader accepted and the rest of the system could not honour, both newly reachable now that persistence.mode decides whether a volume exists. An ephemeral service that names its own volumes. Skipping the default volume was not enough: an authored `volumes: [cache]` survived, so the name reached the canonical form, Spec.All's preflight collision check and firstServiceVolume, while renderService created and mounted nothing. The author's declaration was ignored rather than refused, and the collision check reserved a name for a volume that would never exist. An ephemeral service that declares protection. Protection is a contract about recovering durable data, and there is none. It loaded, then failed at apply time in the active-volume seed against a volume that was never created, with the sealed protected identity naming it regardless. Both now refuse at load time and say which declaration to change. Neither is a new rule so much as the loader catching up with what the renderer already does. Found by review of this branch. Not fixed here, and not confused with these: the health-probe budget for a large AOF, and refusing maxmemory without maxmemory-policy, both of which are policy decisions rather than defects. Refs #75. Co-Authored-By: Claude Opus 5 (1M context) --- internal/app/load.go | 17 ++++++++++++++++ internal/app/services_test.go | 38 +++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/internal/app/load.go b/internal/app/load.go index 229115de..bfaacdd3 100644 --- a/internal/app/load.go +++ b/internal/app/load.go @@ -560,6 +560,23 @@ func crossFieldRules(p *Spec) error { // Materialise the durable volume in the project rather than only in the // generated runtime, so the canonical form, the preflight collision // check and the renderer all name the same volume. + // A service declared disposable owns no durable volume, so a volume the + // author named would be carried through the canonical form and the + // preflight collision check while nothing ever created or mounted it. + // Refuse rather than ignore the declaration. + if serviceIsEphemeral(svc) && len(svc.Volumes) > 0 { + return errf("project_invalid", "services."+name+".volumes", "", + "%q declares volumes and persistence.mode: ephemeral; an ephemeral service owns no durable volume, "+ + "so declare durable persistence or remove the volumes", name) + } + // Protection is a contract about recovering durable data. An ephemeral + // service has none: seeding the active volume fails at apply time on a + // volume that was never created, and the sealed identity would name it. + if serviceIsEphemeral(svc) && svc.Protection != nil { + return errf("project_invalid", "services."+name+".protection", "", + "%q declares protection and persistence.mode: ephemeral; there is no durable data to protect, "+ + "so declare durable persistence or remove the protection policy", name) + } if d.dataPath != "" && len(svc.Volumes) == 0 && !serviceIsEphemeral(svc) { svc.Volumes = []string{"data"} p.Services[name] = svc diff --git a/internal/app/services_test.go b/internal/app/services_test.go index 7dc50342..21995ff2 100644 --- a/internal/app/services_test.go +++ b/internal/app/services_test.go @@ -502,3 +502,41 @@ func TestOnlyEphemeralDisablesServerPersistence(t *testing.T) { t.Fatalf("persistence modes changed to %v; extend these tests before shipping", ePersistence) } } + +// An ephemeral service owns no durable volume. A volume the author names would +// still reach the canonical form, Spec.All's preflight collision check, and the +// protected-identity record, while nothing ever created or mounted it — the +// declaration would be silently ignored rather than refused. +func TestEphemeralServiceCannotDeclareVolumes(t *testing.T) { + src := `api_version: onebox.run/v1 +app: sample +environments: {production: {server: root@h}} +workloads: + web: {role: application, image: x:1} +services: + redis: {version: 8-alpine, persistence: {mode: ephemeral}, volumes: [cache]} +` + _, err := LoadBytes([]byte(src), "ob.yml") + if err == nil { + t.Fatal("an ephemeral service declaring volumes was accepted") + } + if !strings.Contains(err.Error(), "owns no durable volume") { + t.Fatalf("refusal does not explain itself: %v", err) + } +} + +// Protection is a contract about recovering durable data. With no volume +// rendered, seeding the active volume fails at apply time against one that was +// never created, and the sealed identity names it anyway. +func TestEphemeralServiceCannotDeclareProtection(t *testing.T) { + src := strings.Replace(validProtectionProject, + " postgres:\n version: 17\n protection:", + " postgres:\n version: 17\n persistence: {mode: ephemeral}\n protection:", 1) + _, err := LoadBytes([]byte(src), "ob.yml") + if err == nil { + t.Fatal("an ephemeral service declaring protection was accepted") + } + if !strings.Contains(err.Error(), "no durable data to protect") { + t.Fatalf("refusal does not explain itself: %v", err) + } +}