Repository navigation
Drain WebSocket connections instead of dropping them - #12
Merged
Merged
Conversation
connect holds the long-lived connections and could not leave the cluster without damage. Run() blocked in ListenAndServe and never returned, so the signal.Notify in main.go was unreachable for that module and SIGTERM took Go's default path. Three pieces of state outlived the process: the sockets, torn down by the kernel so no client could tell a planned restart from a network failure; the etcd registration, whose TTL is two minutes, during which task kept routing to an instance that no longer existed and dropping the messages; and the Redis routing map. That undercuts the claim the project exists to demonstrate. Being able to add a replica is not worth much if removing one costs a two-minute hole. On a signal, connect now deregisters from etcd and refuses new connections first, then sends every live connection a 1001 close frame, then lets the existing disconnect path clear Redis, bounded by a five second cap split into per-step budgets. The order is the substance: closing connections while task is still routing here, or while new clients are still arriving, does not converge. The buckets could not drive this. Bucket.Put runs only after authentication succeeds, so a connection that has been accepted but not authenticated belongs to no bucket -- and during a restart those are exactly the connections present. connect/registry.go tracks every connection from accept to teardown, sharded the way the buckets are. Every module now starts its work and returns a stopper rather than blocking or handling signals itself, so whether a module reacts to SIGTERM stops depending on whether its Run happened to have returned. That is also what fixes the tracer being shut down at startup: as long as Run() returned while the process kept running, any defer in it fired at the wrong time. Three pre-existing bugs had to be fixed for this to mean anything: - DisConnect never deleted the userId -> serverId key. Not just on shutdown -- an ordinary disconnect left it for its TTL to collect. It now deletes it, in a Lua script that compares first so a departing instance cannot delete the mapping of a user who has already reconnected. The script's three outcomes also guard the room bookkeeping, which was removing users who were online on another instance. - Bucket.DeleteChannel deleted by user rather than by connection, so with two connections for one user the older one's teardown removed the newer one's entry. A drain provokes it: every client reconnects while the old connection is still being torn down. - connect/server_tcp.go dialled etcd from an init(), which made the package impossible to unit test. Measured with make drain-demo, same image both arms, one variable between them. Every connection gets 1001 against none; the etcd registration outlives the stop by 683ms against still being there 20 seconds later. Time to notice is unchanged at about 50ms in both arms, because the TCP close arrives at the same moment either way -- this makes a client able to tell what happened, it does not shorten the outage, and a measurement claiming otherwise is wrong. Evidence in loadtest/reports/drain/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TH3DQ9ADPUsWd5Z3Cn9ckp
PR #13 landed the Kubernetes deployment while this branch was open. The two changes met in eleven files — the six Run() entry points, the connect connection lifecycle, and the metrics server — because both restructure startup and both care about what "ready" means. Three of the resolutions are decisions rather than mechanics: The drain signal moved from /health to /ready. This branch overloaded /health to mean alive-and-ready, which was right when it was the only endpoint. With /ready present it is not, for two reasons. It would not work: the readiness probe in deployments/k8s reads /ready, so a 503 on /health would never remove the pod from the Service endpoints, which is the entire point of draining. And it would do harm: the liveness probe reads /health, and a failing liveness probe asks the kubelet to restart the container in the middle of the shutdown that is trying to close connections cleanly. /health is now unconditional; /ready short-circuits to 503 once draining. Test cases 10 and 30 assert both halves — which is what they were always describing, with one endpoint to say it with. The connection gauge follows the registry. master added gochat_connections_active beside the activeConnections atomic; this branch replaced that atomic with Registry.Len(), which is authoritative rather than a parallel counter. The gauge now moves with Registry.Add and Registry.Remove, so the metric, the admission check and the drain all read one set. The etcd readiness gate got better rather than merged. master called etcdRegistered() from inside the RPC server goroutine because registration happened there; this branch registers synchronously before Serve is spawned, so the gate is a fact rather than a race. Both constructors return nil on a failed registration and their callers skip that address, leaving the gate shut — a connect instance serving but absent from etcd is a black hole for delivery. Also: both branches independently created ADR 0010 and 0011. master's are published, so this branch's became 0014 and 0015, with every reference updated. docs/architecture.md known gaps are merged rather than overwritten — the tracer gap goes because this branch fixes it, the lost-message gap is reworded because graceful shutdown narrows the window without closing it, and master's two new gaps stay. Verified: go build, go vet, gofmt clean, unit suite green, and the integration suite green against a Compose stack running the merged build — including this branch's shutdown cases. A live drain with 30 held connections reported "30 connections answered the close frame, 0 were forced, took 53ms". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QAZ9g4NymZg6BQBPeh6YDH
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.
connectholds the long-lived connections and could not leave the cluster without damage.Run()blocked inListenAndServeand never returned, so thesignal.Notifyinmain.gowas unreachable for that module and SIGTERM took Go's default path.Three pieces of state outlived the process:
taskkept routing to an instance that no longer existed and dropping the messages — the@todoattask/push.go:38;Being able to add a replica is not worth much if removing one costs a two-minute hole.
What changes
On a signal,
connectderegisters from etcd and refuses new connections first, then sends every live connection a 1001 Going Away close frame, then lets the existing disconnect path clear Redis — bounded by a five second cap split into per-step budgets. The order is the substance of it: closing connections whiletaskis still routing here, or while new clients are still arriving, does not converge.The buckets could not drive this.
Bucket.Putruns only after authentication succeeds, so a connection that has been accepted but not authenticated belongs to no bucket — and during a restart those are exactly the connections present.connect/registry.gotracks every connection from accept to teardown, sharded the way the buckets are.Every module now starts its work and returns a stopper rather than blocking or handling signals itself, so whether a module reacts to SIGTERM stops depending on whether its
Runhappened to have returned. That is also what fixes the tracer being shut down at startup: as long asRun()returned while the process kept running, anydeferin it fired at the wrong time.Three pre-existing bugs this had to fix
DisConnectnever deleted theuserId -> serverIdkey. Not just on shutdown — an ordinary disconnect left it for its TTL to collect. It now deletes it, in a Lua script that compares first, so a departing instance cannot delete the mapping of a user who has already reconnected elsewhere. The script's three outcomes also guard the room bookkeeping, which was removing users who were online on another instance.Bucket.DeleteChanneldeleted by user rather than by connection, so with two connections for one user the older one's teardown removed the newer one's entry, leaving that user connected but undeliverable. A drain provokes it, because every client reconnects while the old connection is still being torn down.connect/server_tcp.godialled etcd from aninit(), which made the whole package impossible to unit test.Measured
make drain-demoruns both arms on one image with one environment variable between them, the method ADR 0009 used. Evidence inloadtest/reports/drain/.Time to notice is unchanged, and that is the honest reading. The TCP close arrives at the same moment either way. This makes a client able to tell what happened; it does not shorten the outage, and a measurement claiming otherwise is a measurement error.
The frame counts differ for a reason that checks out exactly. 99 × 25 = 2,475 is the control arm to the frame; the graceful arm's surplus is 625 = 25 × 25, each disconnect republishing room membership to the survivors. In the control arm no disconnect runs, so the remaining clients are never told the room emptied.
What this does not do
task/push.go's@todostays open; this narrows the window from two minutes to the length of a shutdown.site/is a prebuilt bundle with no source here.Tests
37 cases written before the code: 26 unit and 12 integration, including three control-arm cases that fail if
GOCHAT_GRACEFUL_SHUTDOWNever stops switching the behaviour off.docs/architecture.md's Known gaps is also corrected — two entries were fixed in26f9184and never removed, and the tracer entry named the wrong modules.🤖 Generated with Claude Code
https://claude.ai/code/session_01TH3DQ9ADPUsWd5Z3Cn9ckp