From 5abe59bc2eadb0dd87a7a6a237b717b0c6bc4978 Mon Sep 17 00:00:00 2001 From: goingforstudying-ctrl Date: Sat, 5 Sep 2026 11:13:47 -0400 Subject: [PATCH 1/2] post: the snapshot that silently ate the routing table --- ...hot-that-silently-ate-the-routing-table.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 _posts/2026-09-05-the-snapshot-that-silently-ate-the-routing-table.md diff --git a/_posts/2026-09-05-the-snapshot-that-silently-ate-the-routing-table.md b/_posts/2026-09-05-the-snapshot-that-silently-ate-the-routing-table.md new file mode 100644 index 0000000..5665453 --- /dev/null +++ b/_posts/2026-09-05-the-snapshot-that-silently-ate-the-routing-table.md @@ -0,0 +1,44 @@ +--- +layout: post +title: "The snapshot that silently ate the routing table" +--- + +I was reading through comqtt's cluster raft code when I spotted a regression that had been hiding in plain sight. An earlier concurrency fix had accidentally broken snapshot restore in both raft backends, and the failure mode was completely silent: a node that installed a snapshot would come back with an empty subscription routing table, and every message that should have been forwarded across the cluster just stopped going anywhere. + +The earlier commit made `KV.GetAll()` return a copy of the routing table. That was the right call for the problem it fixed, because `Persist` iterates the table while other goroutines mutate it, and a concurrent read during snapshot serialization would race. The problem was what the two restore paths did next: + +```go +gob.NewDecoder(ir).Decode(f.GetAll()) +``` + +`GetAll` hands back a pointer to a throwaway copy, so gob dutifully decoded the entire snapshot into a map that was then discarded. The live table never changed. What made this so nasty is that everything runs without a complaint: the code compiles, `Restore` returns no error, and decoding into the wrong map is not something gob warns you about. + +The trigger conditions are more common than you'd expect. A snapshot gets installed whenever a follower falls far enough behind that the leader sends `InstallSnapshot`, and when a partitioned node rejoins. But the worst one is the most mundane: on every graceful restart, since `Peer.Stop()` takes a snapshot right before shutdown and raft replays it on the way back up. So any clustered comqtt deployment that restarts a node ends up with that node's routing table empty until clients happen to re-subscribe. + +And the downstream effects were invisible at the broker level. `Lookup` returns nothing for the affected filters, `pickNodes` finds no remote nodes, and publishes stop being forwarded to subscribers on other nodes. Nothing logs an error, because as far as the cluster is concerned everything worked: the snapshot installed cleanly. For long-lived bridged clients that subscribe once and hold the connection open, those subscriptions are simply gone, and nothing re-establishes them. Messages silently don't route. + +The fix adds a proper `Restore` method on the KV store that decodes into a fresh map and swaps it in under lock: + +```go +func (k *KV) Restore(r io.Reader) error { + var d data + if err := gob.NewDecoder(r).Decode(&d); err != nil { + return err + } + if d == nil { + d = make(data) + } + k.Lock() + defer k.Unlock() + k.data = d + return nil +} +``` + +Both backends, the hashicorp raft FSM and the etcd-based kvstore, now call `KV.Restore` instead of decoding into `GetAll()`'s copy. Replace rather than merge is deliberate here: raft's contract says a restore must discard previous state and replace it with the snapshot, so a node restoring an older snapshot shouldn't keep filters the snapshot doesn't know about. And a corrupt snapshot returns an error while leaving the existing state untouched, which the tests pin down explicitly. + +The tests go through the real Persist and Restore path rather than mocks. Build an FSM, add filters including a shared subscription, snapshot it, feed those bytes into a fresh FSM, and verify `Lookup` sees exactly what the snapshot contained and nothing else. Round-trip tests cover the KV itself and both backends, with corrupt-snapshot cases and a check that the restored map doesn't share slices with the source. + +One thing I deliberately left alone: `notifyReplay` replays every restored filter including ones the local node itself owns. That behavior predates this bug, and changing it felt out of scope, but I flagged it in [the PR](https://github.com/wind-c/comqtt/pull/173) in case it matters down the road. + +For anyone running comqtt, the clustered MQTT broker at [github.com/wind-c/comqtt](https://github.com/wind-c/comqtt), with bridged or otherwise long-lived clients, this one hurts: the node looks healthy, the cluster looks healthy, and messages are being dropped. The fix is merged in [comqtt#173](https://github.com/wind-c/comqtt/pull/173). From 266b791f423a364d385c8f52ee4c46a57220ec4b Mon Sep 17 00:00:00 2001 From: goingforstudying-ctrl Date: Wed, 9 Sep 2026 18:23:19 -0400 Subject: [PATCH 2/2] Clarify technical claims and source evidence Signed-off-by: goingforstudying-ctrl --- ...snapshot-that-silently-ate-the-routing-table.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/_posts/2026-09-05-the-snapshot-that-silently-ate-the-routing-table.md b/_posts/2026-09-05-the-snapshot-that-silently-ate-the-routing-table.md index 5665453..6b4ddd4 100644 --- a/_posts/2026-09-05-the-snapshot-that-silently-ate-the-routing-table.md +++ b/_posts/2026-09-05-the-snapshot-that-silently-ate-the-routing-table.md @@ -3,7 +3,7 @@ layout: post title: "The snapshot that silently ate the routing table" --- -I was reading through comqtt's cluster raft code when I spotted a regression that had been hiding in plain sight. An earlier concurrency fix had accidentally broken snapshot restore in both raft backends, and the failure mode was completely silent: a node that installed a snapshot would come back with an empty subscription routing table, and every message that should have been forwarded across the cluster just stopped going anywhere. +An earlier concurrency fix in comqtt made snapshot restore decode into a copy of the routing table. The decoder could return success while the live table remained unchanged. A fresh node could therefore miss subscriptions stored in its snapshot; a node with existing state could keep stale subscriptions instead. The earlier commit made `KV.GetAll()` return a copy of the routing table. That was the right call for the problem it fixed, because `Persist` iterates the table while other goroutines mutate it, and a concurrent read during snapshot serialization would race. The problem was what the two restore paths did next: @@ -13,9 +13,9 @@ gob.NewDecoder(ir).Decode(f.GetAll()) `GetAll` hands back a pointer to a throwaway copy, so gob dutifully decoded the entire snapshot into a map that was then discarded. The live table never changed. What made this so nasty is that everything runs without a complaint: the code compiles, `Restore` returns no error, and decoding into the wrong map is not something gob warns you about. -The trigger conditions are more common than you'd expect. A snapshot gets installed whenever a follower falls far enough behind that the leader sends `InstallSnapshot`, and when a partitioned node rejoins. But the worst one is the most mundane: on every graceful restart, since `Peer.Stop()` takes a snapshot right before shutdown and raft replays it on the way back up. So any clustered comqtt deployment that restarts a node ends up with that node's routing table empty until clients happen to re-subscribe. +Snapshot installation is used during recovery and when Raft needs to bring a follower up to date. In those paths, successfully decoding bytes is not enough: the restored table must become the state consulted by routing. The observed outcome depends on the node's prior state and any subsequent log replay; this bug does not establish that every graceful restart of every deployment necessarily leaves an empty table. -And the downstream effects were invisible at the broker level. `Lookup` returns nothing for the affected filters, `pickNodes` finds no remote nodes, and publishes stop being forwarded to subscribers on other nodes. Nothing logs an error, because as far as the cluster is concerned everything worked: the snapshot installed cleanly. For long-lived bridged clients that subscribe once and hold the connection open, those subscriptions are simply gone, and nothing re-establishes them. Messages silently don't route. +For remote subscription filters that are missing from the live table, `Lookup` returns no route and `pickNodes` cannot select the subscriber's node. Messages for those subscriptions may stop being forwarded despite a successful restore. Existing routes, later log replay, or a new subscription can change that outcome; the failure is specifically that the snapshot did not update the live routing state. The fix adds a proper `Restore` method on the KV store that decodes into a fresh map and swaps it in under lock: @@ -35,10 +35,12 @@ func (k *KV) Restore(r io.Reader) error { } ``` -Both backends, the hashicorp raft FSM and the etcd-based kvstore, now call `KV.Restore` instead of decoding into `GetAll()`'s copy. Replace rather than merge is deliberate here: raft's contract says a restore must discard previous state and replace it with the snapshot, so a node restoring an older snapshot shouldn't keep filters the snapshot doesn't know about. And a corrupt snapshot returns an error while leaving the existing state untouched, which the tests pin down explicitly. +Both backends, the hashicorp raft FSM and the etcd-based kvstore, now call `KV.Restore` instead of decoding into `GetAll()`'s copy. Replace rather than merge is deliberate here: raft's contract says a restore must discard previous state and replace it with the snapshot, so a restore must not retain stale filters absent from the selected snapshot. Raft decides which snapshot is valid to install. And a corrupt snapshot returns an error while leaving the existing state untouched, which the tests pin down explicitly. -The tests go through the real Persist and Restore path rather than mocks. Build an FSM, add filters including a shared subscription, snapshot it, feed those bytes into a fresh FSM, and verify `Lookup` sees exactly what the snapshot contained and nothing else. Round-trip tests cover the KV itself and both backends, with corrupt-snapshot cases and a check that the restored map doesn't share slices with the source. +The unit tests exercise the serialization and restore code, including a test SnapshotSink for the hashicorp backend. They do not restart a real multi-node cluster. Build an FSM, add filters including a shared subscription, snapshot it, feed those bytes into a fresh FSM, and verify `Lookup` sees exactly what the snapshot contained and nothing else. Round-trip tests cover the KV itself and both backends, with corrupt-snapshot cases and a check that the restored map doesn't share slices with the source. One thing I deliberately left alone: `notifyReplay` replays every restored filter including ones the local node itself owns. That behavior predates this bug, and changing it felt out of scope, but I flagged it in [the PR](https://github.com/wind-c/comqtt/pull/173) in case it matters down the road. -For anyone running comqtt, the clustered MQTT broker at [github.com/wind-c/comqtt](https://github.com/wind-c/comqtt), with bridged or otherwise long-lived clients, this one hurts: the node looks healthy, the cluster looks healthy, and messages are being dropped. The fix is merged in [comqtt#173](https://github.com/wind-c/comqtt/pull/173). +The practical symptom is missing or stale cross-node subscription routing after a snapshot-based recovery, despite the decode reporting success. The fix is merged in [comqtt #173](https://github.com/wind-c/comqtt/pull/173). Round-trip tests through both Raft backends check the live lookup result, which is the state that message forwarding actually uses. + +Implementation reference: [merged commit](https://github.com/wind-c/comqtt/commit/40fdcac5d6811fe9ae5abe169fa1c242584ef9b9) (2026-09-03).