From 1543a69cea10a1d0c05a8131b066103163d328e9 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Tue, 18 Aug 2026 21:15:33 -0700 Subject: [PATCH] fix(services): make the Redis and Valkey health checks prove a write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The native probes asked whether the server answers: redis-cli -a "$REDIS_PASSWORD" ping | grep -q PONG Redis answers PONG while refusing every write. When a background save fails and stop-writes-on-bgsave-error is enabled — Redis's own default — the server keeps serving reads and replies MISCONF to writes. Valkey has the same shape. So the probe reported the service healthy exactly when the behaviour its callers depend on was gone. Onebox gates rollouts on that answer, so dependent workloads started, the deploy converged, and every counter, session, queue and rate-limiter write failed against a dependency the system had just certified. An application that fails open on a rate-limit increment does so at the moment it is least supposed to. Both drivers now write: redis-cli -a "$REDIS_PASSWORD" set ob:health 1 EX 30 | grep -qx OK SET exercises the path the probe is meant to cover, and MISCONF fails it. The key is namespaced so it cannot collide with an application's, and EX bounds it, so the probe holds one key rather than accumulating them. `grep -qx` rather than an anchored pattern is deliberate. `grep -q '^OK$'` reads correctly and would have been wrong here: escapeDollars doubles every `$` in generated content, so the anchor would have reached Compose as `$$` and been interpolated. Matching the whole line needs no `$` at all. The generated credential still renders as `-a "$$REDIS_PASSWORD"`, unchanged. Six frozen corpus digests move, all of them `redis=`. The application digests and every other service digest are byte-identical, which is the evidence that this touched only the two drivers it claims to. Operationally this is a service-definition change: the first deploy after upgrading recreates redis and valkey services. That is unavoidable — the old definition is the defect. Tests fail without the fix, checked by restoring the ping probe and watching them go red rather than assumed. Closes #76. Co-Authored-By: Claude Opus 5 (1M context) --- internal/app/services.go | 11 +++- internal/app/services_test.go | 55 ++++++++++++++++++++ internal/app/testdata/contract-verdicts.json | 12 ++--- 3 files changed, 70 insertions(+), 8 deletions(-) diff --git a/internal/app/services.go b/internal/app/services.go index eabee1b1..15dc2d70 100644 --- a/internal/app/services.go +++ b/internal/app/services.go @@ -127,7 +127,13 @@ var drivers = map[string]driver{ "redis": { majorUpgradeInPlace: true, image: "redis", port: 6379, dataPath: "/data", - health: []string{"CMD-SHELL", "redis-cli -a \"$REDIS_PASSWORD\" ping | grep -q PONG"}, + // A write, not a PING. Redis answers PONG while refusing every write when + // a background save has failed and stop-writes-on-bgsave-error is on — + // which is its own default — so a connection-only probe reports healthy + // exactly when the thing callers need is gone, and a health-gated + // 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"}, secretEnv: []string{"REDIS_PASSWORD"}, urlUser: "default", scheme: "redis", settings: settingsRedisFlag, @@ -135,7 +141,8 @@ var drivers = map[string]driver{ "valkey": { majorUpgradeInPlace: true, image: "valkey/valkey", port: 6379, dataPath: "/data", - health: []string{"CMD-SHELL", "valkey-cli -a \"$REDIS_PASSWORD\" ping | grep -q PONG"}, + // 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"}, secretEnv: []string{"REDIS_PASSWORD"}, urlUser: "default", scheme: "redis", settings: settingsRedisFlag, diff --git a/internal/app/services_test.go b/internal/app/services_test.go index ffc137a5..e1736521 100644 --- a/internal/app/services_test.go +++ b/internal/app/services_test.go @@ -336,3 +336,58 @@ func TestCredentialWritesAreAtomic(t *testing.T) { t.Errorf("script still truncates a live file in place:\n%s", script) } } + +// Redis answers PONG while refusing every write when a background save has +// failed and stop-writes-on-bgsave-error is enabled — its own default. A +// connection-only probe therefore reports the service healthy exactly when the +// behaviour callers depend on is unavailable, and a health-gated rollout +// converges onto a dependency that cannot store anything. +func TestRedisFamilyHealthChecksProveAWrite(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} + valkey: {version: 8-alpine} +`) + for _, driver := range []string{"redis", "valkey"} { + doc := string(rendered[driver]) + if !strings.Contains(doc, "set ob:health") { + t.Fatalf("%s health check does not write:\n%s", driver, doc) + } + // A PING-only probe is the regression this guards. + if strings.Contains(doc, "ping | grep") { + t.Fatalf("%s health check regressed to a connection-only ping:\n%s", driver, doc) + } + // Bounded, so the probe cannot accumulate keys. + if !strings.Contains(doc, "EX 30") { + t.Fatalf("%s health-check key has no TTL:\n%s", driver, doc) + } + // The generated credential still reaches the container unexpanded on the + // host: Compose reads `$$` and passes `$`. + if !strings.Contains(doc, `-a "$$REDIS_PASSWORD"`) { + t.Fatalf("%s health check lost its escaped credential reference:\n%s", driver, doc) + } + // No bare `$` of our own, which Compose would interpolate away. + probe := doc[strings.Index(doc, driver+"-cli"):] + probe = probe[:strings.Index(probe, "\n")] + if strings.Count(probe, "$")-strings.Count(probe, "$$")*2 != 0 { + t.Fatalf("%s health check carries an unescaped dollar: %s", driver, probe) + } + } +} + +func renderServices(t *testing.T, src string) map[string][]byte { + t.Helper() + spec, err := LoadBytes([]byte(src), "ob.yml") + if err != nil { + t.Fatal(err) + } + r, err := spec.Render("production", "rel", nil) + if err != nil { + t.Fatal(err) + } + return r.Services +} diff --git a/internal/app/testdata/contract-verdicts.json b/internal/app/testdata/contract-verdicts.json index 952853df..4838e49e 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=bc9b8e8616909750" + "digest": "37ef191260abddc6f674ea8bed3bb82970358f8395df2d5a7f5b9b5142268b47 redis=69fece9ab514b04f" }, { "case": "conformance/settings key with a shell metacharacter", @@ -517,12 +517,12 @@ { "case": "corpus/authentik.yml", "loads": true, - "digest": "6d3a2518ab077033b35018bb81aa9702a641a2ad7433468afacb6070083d60a8 postgres=dc4f8448b8b82b4a redis=2efe6d6e03c3fd6f" + "digest": "6d3a2518ab077033b35018bb81aa9702a641a2ad7433468afacb6070083d60a8 postgres=dc4f8448b8b82b4a redis=191161a0c85c6c0d" }, { "case": "corpus/ext-authentik-managed.yml", "loads": true, - "digest": "f6b0b547d0bc7d55b8309e57f4f928a57e2ce44dd4fecc773599c9785580083f postgres=dc4f8448b8b82b4a redis=2efe6d6e03c3fd6f" + "digest": "f6b0b547d0bc7d55b8309e57f4f928a57e2ce44dd4fecc773599c9785580083f postgres=dc4f8448b8b82b4a redis=191161a0c85c6c0d" }, { "case": "corpus/ext-authentik.yml", @@ -542,7 +542,7 @@ { "case": "corpus/ext-immich-sourced.yml", "loads": true, - "digest": "b852204b8ab417c0b5ca7c162108f95189829c69f8b7284d58120fe1e17c47d9 postgres=76af15324857fa90 redis=365ab102d3f39431" + "digest": "b852204b8ab417c0b5ca7c162108f95189829c69f8b7284d58120fe1e17c47d9 postgres=76af15324857fa90 redis=0281e909253b33c5" }, { "case": "corpus/ext-immich.yml", @@ -552,7 +552,7 @@ { "case": "corpus/ext-n8n.yml", "loads": true, - "digest": "d9b22f801a91ee7c4f6e9d24811e9454374067466263cc08cbc18cb9aefc8241 postgres=809549d286e2dbdc redis=f88af25243b8688b" + "digest": "d9b22f801a91ee7c4f6e9d24811e9454374067466263cc08cbc18cb9aefc8241 postgres=809549d286e2dbdc redis=3cef59e7d2733f0a" }, { "case": "corpus/ext-paperless.yml", @@ -612,7 +612,7 @@ { "case": "corpus/penpot.yml", "loads": true, - "digest": "10d0d86d56452027d9d317f6d77d0a696ed95eb2595e9cf039db6d187ff338e6 postgres=fc584b1b50db23a6 redis=5e6b9f3ca2de9a2f" + "digest": "10d0d86d56452027d9d317f6d77d0a696ed95eb2595e9cf039db6d187ff338e6 postgres=fc584b1b50db23a6 redis=be4534525f623f76" }, { "case": "corpus/pursue.yml",