fix(service): don't report READY before the endpoint accepts connections - #184
Draft
gonzaloserrano wants to merge 4 commits into
Draft
fix(service): don't report READY before the endpoint accepts connections#184gonzaloserrano wants to merge 4 commits into
gonzaloserrano wants to merge 4 commits into
Conversation
service create/start/fork waited only on the control-plane status field, which flips to READY before Postgres binds its port. ready.go already documents READY as "accepting connections", so callers that connect immediately raced it and got ECONNREFUSED. Add WaitForConnectable, which probes the endpoint until it answers, and run it after the status wait. Any Postgres protocol error counts as serving so the probe works without credentials; 57P03 keeps waiting. Best-effort: an unverified endpoint warns rather than failing, since a VPC-only or allowlisted service is legitimately unreachable from the CLI host while being healthy.
The original comment said READY flips before Postgres binds its port. Tracing savannah-deployer shows binding is already gated: leaderStatus Reconciler requires podIsServing plus passing post-deploy runners. Both are in-cluster checks, and the post-deploy runners connect to the pod (directly or via port-forward), never to the customer endpoint. The ungated gap is the external path: allocated port, DNS, load balancer.
…e-readiness-probe
main gained *config.Config on the connection helpers and moved command wiring to App.GetAll (#183), which lands as a silent break here: the merge is textually clean but the call sites stop compiling. Thread Config through ConnectableWaitArgs, take the client as api.ClientWithResponsesInterface to match WaitForServiceArgs, and read client/projectID from App.GetAll at the three call sites.
Author
|
@nathanjcochran PTAL, in case you think this is a real issue (I’m not familiar with the codebase or API semantics) I’ll mark it as ready for review. |
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
1 participant
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.
service createwaits on the control-planestatusfield. READY gates on in-cluster signals (leader pod serving, post-deploy runners), never on the customer-facing endpoint, so callers connecting immediately getECONNREFUSEDwhile the allocated port, DNS and LB catch up.internal/common/ready.goalready documents READY as "accepting connections".Prod repro in
timescale/migration-smoke-test(run 30977861902): READY 20.1s after create, refused 91ms later. Twice in 40 runs.startandforkshare the pattern.WaitForConnectablepolls the endpoint after the status wait, capped at 2 minutes. Any Postgres protocol error counts as serving:28P01proves the server is up, just refusing our credentials, so no password is needed.57P03is the exception and keeps waiting.Best-effort: unverifiable endpoints warn rather than fail; a VPC-only service is healthy but unreachable from here.
A server-side gate is worth doing separately, but cannot replace this: readiness is a property of the path between a client and the service, not of the service alone.
For review: do
start/forkbelong here, and should the budget be a flag? Call sites covered only by compilation (create tests use--no-wait).