fix(storage): Fix full_snapshot RPC deadlock - #1330
Merged
Merged
Conversation
|
🧪 Tempest Identity Compatibility Results (advisory, non-blocking) rust Failed test IDs |
|
🦢 Load Test Results Goose Attack ReportPlan Overview
Request Metrics
Response Time Metrics
Status Code Metrics
Transaction Metrics
Scenario Metrics
Error Metrics
|
|
Bencher Report| Project | keystone |
| Branch | claude/github-issue-1329-hvc1bi |
| Testbed | ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| Command_Serde/apply/remove | 📈 view plot 🚷 view threshold | 126,530.00 ns(-56.58%)Baseline: 291,440.62 ns | 1,448,357.86 ns (8.74%) |
| Command_Serde/apply/set | 📈 view plot 🚷 view threshold | 141,450.00 ns(-69.29%)Baseline: 460,542.28 ns | 3,321,060.90 ns (4.26%) |
| Command_Serde/pack/delete | 📈 view plot 🚷 view threshold | 127.06 ns(+9.15%)Baseline: 116.41 ns | 158.91 ns (79.96%) |
| Command_Serde/pack/delete_index | 📈 view plot 🚷 view threshold | 111.97 ns(+8.13%)Baseline: 103.55 ns | 139.44 ns (80.30%) |
| Command_Serde/pack/set | 📈 view plot 🚷 view threshold | 209.34 ns(+10.51%)Baseline: 189.43 ns | 253.54 ns (82.57%) |
| Command_Serde/pack/set_index | 📈 view plot 🚷 view threshold | 112.03 ns(+8.19%)Baseline: 103.55 ns | 139.99 ns (80.02%) |
| Command_Serde/unpack/delete | 📈 view plot 🚷 view threshold | 188.63 ns(+3.08%)Baseline: 183.00 ns | 270.92 ns (69.63%) |
| Command_Serde/unpack/delete_index | 📈 view plot 🚷 view threshold | 158.05 ns(+3.69%)Baseline: 152.42 ns | 219.60 ns (71.97%) |
| Command_Serde/unpack/set | 📈 view plot 🚷 view threshold | 309.30 ns(+18.14%)Baseline: 261.82 ns | 375.22 ns (82.43%) |
| Command_Serde/unpack/set_index | 📈 view plot 🚷 view threshold | 157.01 ns(+4.37%)Baseline: 150.43 ns | 215.09 ns (73.00%) |
| Payload_encryption/pack/remove_cmd | 📈 view plot 🚷 view threshold | 124.03 ns(+13.48%)Baseline: 109.30 ns | 151.95 ns (81.62%) |
| Payload_encryption/pack/set_cmd | 📈 view plot 🚷 view threshold | 208.27 ns(+5.17%)Baseline: 198.03 ns | 274.32 ns (75.92%) |
| Payload_encryption/unpack/remove_cmd | 📈 view plot 🚷 view threshold | 201.56 ns(+5.32%)Baseline: 191.38 ns | 284.65 ns (70.81%) |
| Payload_encryption/unpack/set_cmd | 📈 view plot 🚷 view threshold | 303.74 ns(+12.24%)Baseline: 270.61 ns | 390.67 ns (77.75%) |
| Raft_1Node_Latency/prefix/1node | 📈 view plot 🚷 view threshold | 3,469,300.00 ns(+12.36%)Baseline: 3,087,777.33 ns | 8,510,044.82 ns (40.77%) |
| Raft_1Node_Latency/read/1node | 📈 view plot 🚷 view threshold | 42,841.00 ns(+14.95%)Baseline: 37,269.47 ns | 53,010.19 ns (80.82%) |
| Raft_1Node_Latency/remove/1node | 📈 view plot 🚷 view threshold | 359,300.00 ns(-70.56%)Baseline: 1,220,650.31 ns | 11,552,389.31 ns (3.11%) |
| Raft_1Node_Latency/write/1node | 📈 view plot 🚷 view threshold | 375,560.00 ns(-62.40%)Baseline: 998,917.03 ns | 7,587,887.21 ns (4.95%) |
| build_snapshot/default | 📈 view plot 🚷 view threshold | 112,550.00 ns(-7.83%)Baseline: 122,106.27 ns | 267,279.45 ns (42.11%) |
| fernet token/project | 📈 view plot 🚷 view threshold | 1,447.30 ns(+3.74%)Baseline: 1,395.15 ns | 1,829.28 ns (79.12%) |
| get_data_keyspace | 📈 view plot 🚷 view threshold | 0.32 ns(+4.23%)Baseline: 0.31 ns | 0.39 ns (83.07%) |
| get_db | 📈 view plot 🚷 view threshold | 0.33 ns(+5.48%)Baseline: 0.31 ns | 0.39 ns (84.06%) |
| get_fernet_token_timestamp/project | 📈 view plot 🚷 view threshold | 155.73 ns(+12.55%)Baseline: 138.37 ns | 183.99 ns (84.64%) |
| get_keyspace | 📈 view plot 🚷 view threshold | 4.36 ns(-24.81%)Baseline: 5.80 ns | 12.70 ns (34.33%) |
full_snapshot() awaited the client-streaming Snapshot RPC's response before sending anything into the channel backing its request stream. The server (raft_service.rs) only replies after it has read the entire request stream, so nothing ever fed that channel and every snapshot install deadlocked silently. A learner added after the leader's log was purged therefore never caught up: RaftMetrics. last_log_index stayed None forever, with no error logged anywhere (GitHub issue #1329). The whole snapshot already lives in memory by this point, so build the full request stream eagerly with stream::iter instead of a channel fed after the await returns. This removes the self-deadlock entirely; no separate feeder task is needed. Add a regression test that snapshots and purges a single-node leader's log, then adds a learner and asserts it catches up via InstallSnapshot, exercising the path GitHub issue #1293's own "purge, join, assert reads" integration test needs and was blocked on. Signed-off-by: Artem Goncharov <artem.goncharov@gmail.com>
gtema
force-pushed
the
claude/github-issue-1329-hvc1bi
branch
from
September 19, 2026 17:48
357a4da to
1a245e5
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 20, 2026
Collaborator
Author
|
The merge-queue run was dequeued for This isn't this PR's diff: it only touches I've kicked off a re-run of the failed job to confirm before asking for a re-queue: https://github.com/openstack-experimental/keystone/actions/runs/35493140847 Generated by Claude Code |
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.
Summary
Fixes #1329: a purged-log leader never resumed replicating to a newly
added learner.
Root cause:
NetworkConnection::full_snapshot(crates/storage/src/network.rs)awaited the client-streaming
SnapshotRPC's response (client.snapshot(rx).await?)before sending anything into the
mpscchannel backing its requeststream (
rx). The server side (RaftServiceImpl::snapshot,crates/storage/src/grpc/raft_service.rs) only replies once it has readthe entire request stream. Since both the "await the response" and
"produce the stream" code lived in the same task, in that order, the
task deadlocked against itself on every attempt to send a snapshot —
silently, since the surrounding retry loop in openraft's
SnapshotTransmitteronly logs onErr, and a hung.awaitproducesneither an
Errnor any further tracing.This is why a learner added after the leader's log was snapshotted and
purged never caught up: openraft correctly detects the need for a
snapshot and issues
Command::ReplicateSnapshot, but the actual RPCcall to transmit it then hung forever, so
RaftMetrics.last_log_indexon the learner stayed
Noneindefinitely. This reproduced identicallywith real, separate OS threads/runtimes per node, ruling out the
test-harness explanation the issue raised as an open question — it's a
deterministic bug in the RPC client code, not a threading/harness
artifact.
The fix: since the whole snapshot already lives in memory
(
Vec<u8>) by the timefull_snapshotruns, build the completerequest stream eagerly with
futures::stream::iter(...)instead offunneling it through a channel fed by code that comes after the
.await. This removes the self-deadlock entirely and needs noseparate feeder task.
Also updates a stale doc comment on
test_join_adopts_cluster_dek_innerthat described this bug (discovered while working on #1298) as an
open, unfixed gap.
Test plan
test_purge_then_join_learner_catches_up_via_snapshottocrates/storage/tests/test_cluster.rs: brings up a single-nodecluster, writes 20 keys, triggers
snapshot()+purge_log(),brings up a fresh node, adds it as a learner via a raw
add_learnercall, and asserts it catches up via
InstallSnapshotwithin 10s.Confirmed this test hangs forever against the pre-fix code and
passes reliably after the fix.
cargo test -p openstack-keystone-distributed-storage --test test_cluster -- --test-threads=1: all 13 tests pass.cargo test -p openstack-keystone-distributed-storage --lib: all 95unit tests pass.
cargo clippy -p openstack-keystone-distributed-storage --lib --testsandcargo fmt -p openstack-keystone-distributed-storage -- --check: clean.pre-commit run --files crates/storage/src/network.rs crates/storage/tests/test_cluster.rsand thecommittedcommit-msghook: both pass.