fix(services): make the Redis and Valkey health checks prove a write - #78
Merged
Conversation
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #76.
The defect
Redis answers
PONGwhile refusing every write. When a background save fails andstop-writes-on-bgsave-erroris enabled — Redis's own default — the server serves reads and repliesMISCONFto writes. Valkey has the same shape.Onebox gates rollouts on that answer. So the probe certified the service healthy exactly when the behaviour its callers depend on was gone: dependent workloads started, the deploy converged, and every counter, session, queue and rate-limiter write failed against a dependency the system had just approved.
The fix
SETexercises the path the probe is meant to cover, andMISCONFfails it. The key is namespaced so it cannot collide with an application key, andEXbounds it — one key held, never accumulated.grep -qxrather thangrep -q '^OK$'is deliberate. The anchored form reads better and would have been wrong:escapeDollarsdoubles every$in generated content, so$would have reached Compose as$$and been interpolated. Whole-line matching needs no$. Verified in the rendered output — the credential still renders-a "$$REDIS_PASSWORD", unchanged.Scope, shown by the digests
Six frozen corpus digests move, all of them
redis=. Application digests and every other service digest are byte-identical:That is the evidence this touched only the two drivers it claims to.
Operational note
This is a service-definition change: the first deploy after upgrading recreates the redis and valkey services. Unavoidable — the old definition is the defect.
Tests
Regression tests assert the probe writes, has a TTL, keeps the escaped credential, carries no unescaped
$, and explicitly guard against a return to a connection-only ping. Non-vacuous, verified by restoring the ping probe:Verified
just check,golangci-lint run ./...,go test -race ./....Related
Sequencing note for #75: with AOF disabled on ephemeral Redis, the
stop-writes-on-bgsave-errorpath largely disappears, so this probe's value differs by persistence mode. Landing it first means #75 inherits a write-sensitive probe rather than having to add one.