fix(tern): bound every outbound call on the drive path#653
Draft
aparajon wants to merge 2 commits into
Draft
Conversation
A drive goroutine runs on an unbounded context while a separate heartbeat goroutine keeps renewing the apply lease. If an outbound call hangs on a black-holed connection, the driver blocks forever, the heartbeat never goes stale, and no peer can reclaim the apply. Bound all three outbound legs: - Tern gRPC: client keepalive params plus a unary interceptor that applies a default per-RPC deadline when the caller's context has none (60s for polling/control RPCs, 5m for RPCs that do synchronous engine or live-schema work). A caller-supplied deadline always wins. - PlanetScale API: construct the SDK client with a timeout-bearing HTTP client, and move the raw-HTTP throttle endpoint off http.DefaultClient onto a bounded client. - MySQL connections: mysqlconn DSN normalization now fills in connect, read, and write timeouts (keeping any explicit DSN values). The read timeout is deliberately long — it must outlast legitimate silent waits like the EnsureSchema advisory-lock wait — but it is no longer infinite. The bounds are generous by design: the invariant is that no outbound call is unbounded, not that calls are fast. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR ensures SchemaBot’s “drive path” cannot hang forever on black-holed network connections by adding explicit, generous bounds to outbound calls across Tern gRPC, PlanetScale HTTP API, and SchemaBot-managed MySQL connections, while keeping the existing lease/heartbeat design intact.
Changes:
- Tern gRPC client: add client keepalive parameters and a unary interceptor that applies per-method default RPC deadlines when the caller provides none.
- PlanetScale client: ensure all SDK and raw-HTTP throttle requests use a timeout-bounded HTTP client (and stop using
http.DefaultClient). - MySQL DSN normalization: fill in connect/read/write timeouts (without overriding non-zero DSN values) and return the normalized DSN string consistently.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/tern/grpc_client.go | Adds keepalive params and a default-deadline unary interceptor for Tern unary RPCs. |
| pkg/tern/grpc_client_test.go | Adds tests proving default deadlines apply only when the caller provides no deadline, and validates keepalive settings. |
| pkg/psclient/client.go | Constructs PlanetScale SDK + throttle HTTP clients with bounded timeouts and pooled transport. |
| pkg/psclient/client_test.go | Adds tests validating bounded HTTP client configuration and throttle timeout behavior. |
| pkg/mysqlconn/mysqlconn.go | Normalizes DSNs to include connect/read/write timeouts (and returns formatted DSNs consistently). |
| pkg/mysqlconn/mysqlconn_test.go | Updates DSN normalization expectations and adds coverage for preserving explicit timeouts. |
| go.mod | Promotes github.com/hashicorp/go-cleanhttp to a direct dependency for pooled HTTP transport construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…fter caller options A caller option could previously displace the auth-wrapping transport and the bounded HTTP client, since caller opts were applied last. Caller options now apply first, with the bounded client and service token installed after them so the auth and timeout invariants always hold. Co-Authored-By: Claude Fable 5 <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.
Why this matters
A drive goroutine runs on an unbounded context while a separate heartbeat goroutine keeps renewing the apply lease. If one outbound call hangs on a black-holed connection (dropped packets, dead NAT entry, half-open TCP), the driver blocks forever — and because the heartbeat keeps the lease fresh, no peer ever sees a stale lease and reclaims the apply. One dead connection wedges the apply until a human intervenes.
What it does
Bounds all three outbound legs of the drive path; the lease/heartbeat design is untouched.
Apply,Plan,PlanDiff,PullSchema,Volume,Revert,SkipRevert). A caller-supplied deadline always wins. ADEADLINE_EXCEEDEDonApplyis handled as an ambiguous dispatch outcome and recovered via the idempotency key.http.DefaultClientonto its own bounded client.Before / after
🤖 Generated with Claude Code