From dfac3d60791c57a7b7af5c21902374354d88399f Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Thu, 23 Jul 2026 14:53:44 -0700 Subject: [PATCH 1/5] Add integration test for read-only activation with one downstairs Add integration_test_just_read_one_downstairs, which creates three read-only downstairs, stops two of them, and confirms that a read-only upstairs can still activate and read from the remaining single downstairs. Also add a stop() helper to TestDownstairs so a test can bring an individual downstairs down while keeping the rest of the set running. --- integration_tests/src/lib.rs | 54 ++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/integration_tests/src/lib.rs b/integration_tests/src/lib.rs index 7f29b0e2c..487d0a4cc 100644 --- a/integration_tests/src/lib.rs +++ b/integration_tests/src/lib.rs @@ -487,6 +487,17 @@ mod integration_tests { downstairs.clone_region(source).await } + // Stop this downstairs, freeing the port it was listening on. Used + // to simulate a downstairs that is not running. The address that was + // assigned during spawn is still recorded in any CrucibleOpts we + // handed out, so the upstairs will try (and fail) to connect to it. + pub async fn stop(&mut self) -> Result<()> { + if let Some(downstairs) = self.downstairs.take() { + downstairs.stop().await?; + } + Ok(()) + } + pub fn address(&self) -> SocketAddr { // If start_downstairs returned Ok, then address will be populated self.downstairs.as_ref().unwrap().address() @@ -1160,6 +1171,49 @@ mod integration_tests { Ok(()) } + #[tokio::test] + async fn integration_test_just_read_one_downstairs() -> Result<()> { + // Create three read-only downstairs, but only leave one of them + // running. A single read-only downstairs is enough to activate a + // read-only upstairs, so both activation and a read should succeed. + const BLOCK_SIZE: usize = 512; + + // small(true) creates and starts all three downstairs read only. We + // capture the opts (which record all three addresses) before stopping + // two of them, leaving only downstairs1 running. + let mut tds = DefaultTestDownstairsSet::small(true).await?; + let opts = tds.opts(); + + tds.downstairs2.stop().await?; + tds.downstairs3.stop().await?; + + let vcr = VolumeConstructionRequest::Volume { + id: Uuid::new_v4(), + block_size: BLOCK_SIZE as u64, + sub_volumes: vec![], + read_only_parent: Some(Box::new( + VolumeConstructionRequest::Region { + block_size: BLOCK_SIZE as u64, + blocks_per_extent: tds.blocks_per_extent(), + extent_count: tds.extent_count(), + opts, + generation: 1, + }, + )), + }; + + let volume = Volume::construct(vcr, None, csl()).await?; + volume.activate().await?; + + // Read one block: should be all 0x00 + let mut buffer = Buffer::new(1, BLOCK_SIZE); + volume.read(BlockIndex(0), &mut buffer).await?; + + assert_eq!(vec![0x00; BLOCK_SIZE], &buffer[..]); + + Ok(()) + } + #[tokio::test] async fn integration_test_volume_write_unwritten_1() -> Result<()> { // Test a simple single layer volume, verify write_unwritten From bdae83151995b86efde506e238ed52c89abf17d6 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Thu, 23 Jul 2026 15:18:34 -0700 Subject: [PATCH 2/5] Flush and re-read in read-only one-downstairs test Augment integration_test_just_read_one_downstairs to manually send a flush after the first read, then confirm a second read completes successfully. With only one downstairs running, the flush still acks because the two stopped downstairs have their jobs moved to Skipped, so the flush is complete on all clients rather than hanging. --- integration_tests/src/lib.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/integration_tests/src/lib.rs b/integration_tests/src/lib.rs index 487d0a4cc..8212028aa 100644 --- a/integration_tests/src/lib.rs +++ b/integration_tests/src/lib.rs @@ -1211,6 +1211,18 @@ mod integration_tests { assert_eq!(vec![0x00; BLOCK_SIZE], &buffer[..]); + // Manually send a flush. With only one downstairs running, the flush + // still completes: the two stopped downstairs have their jobs moved to + // Skipped, so the flush is complete on all clients and acks back to us. + // This should not hang. + volume.flush(None).await?; + + // A second read after the flush should also complete successfully. + let mut buffer = Buffer::new(1, BLOCK_SIZE); + volume.read(BlockIndex(0), &mut buffer).await?; + + assert_eq!(vec![0x00; BLOCK_SIZE], &buffer[..]); + Ok(()) } From 9a2b923cab1dbd3866dd42e6fa46d7d88efe8d88 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Fri, 24 Jul 2026 14:30:57 -0700 Subject: [PATCH 3/5] Make RO volume not have a ROP --- integration_tests/src/lib.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/integration_tests/src/lib.rs b/integration_tests/src/lib.rs index 8212028aa..310d45e1a 100644 --- a/integration_tests/src/lib.rs +++ b/integration_tests/src/lib.rs @@ -1187,19 +1187,19 @@ mod integration_tests { tds.downstairs2.stop().await?; tds.downstairs3.stop().await?; + // Put the region under sub_volumes (not as a read_only_parent) so that + // flushes are actually sent to it. let vcr = VolumeConstructionRequest::Volume { id: Uuid::new_v4(), block_size: BLOCK_SIZE as u64, - sub_volumes: vec![], - read_only_parent: Some(Box::new( - VolumeConstructionRequest::Region { - block_size: BLOCK_SIZE as u64, - blocks_per_extent: tds.blocks_per_extent(), - extent_count: tds.extent_count(), - opts, - generation: 1, - }, - )), + sub_volumes: vec![VolumeConstructionRequest::Region { + block_size: BLOCK_SIZE as u64, + blocks_per_extent: tds.blocks_per_extent(), + extent_count: tds.extent_count(), + opts, + generation: 1, + }], + read_only_parent: None, }; let volume = Volume::construct(vcr, None, csl()).await?; From 3eb3eda6e22df24e135646c23f4bf0be5ea9ca93 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Mon, 27 Jul 2026 08:16:02 -0700 Subject: [PATCH 4/5] Ack flushes on read-only volumes without sending to downstairs A flush on a read-only volume has no data to persist, so ack it immediately in the guest Flush path instead of sending it to the downstairs. This broke a test, test_no_read_only_live_repair, which relied on a guest flush reaching the downstairs to retire skipped jobs after a fault. Retiring completed jobs still must happen, but in the production code the auto-flush will come around and clear these jobs out. The internal flush timer for the test framework is pinned to 24 hours, so to enable the internal flush we add a test-only BlockOp::FlushCheck and guest.flush_check() that runs the same work the automatic flush timer performs. Unlike a guest flush, this path still sends the flush to the downstairs on a read-only volume. Update the test to use it. This appears to be a simpler solution and does not require the heavier modification that enabling the auto flush on all tests would involve. Enabling auto-flush for all tests would add non-determinism to many tests that don't expect it. --- upstairs/src/dummy_downstairs_tests.rs | 20 ++++++-------------- upstairs/src/guest.rs | 10 ++++++++++ upstairs/src/lib.rs | 5 +++++ upstairs/src/upstairs.rs | 19 ++++++++++++++++++- 4 files changed, 39 insertions(+), 15 deletions(-) diff --git a/upstairs/src/dummy_downstairs_tests.rs b/upstairs/src/dummy_downstairs_tests.rs index 7547f83de..78befc8fe 100644 --- a/upstairs/src/dummy_downstairs_tests.rs +++ b/upstairs/src/dummy_downstairs_tests.rs @@ -2654,20 +2654,12 @@ async fn test_no_read_only_live_repair() { assert!(matches!(harness.ds2.try_recv(), Err(TryRecvError::Empty))); assert!(matches!(harness.ds3.try_recv(), Err(TryRecvError::Empty))); - // Flush to clean out skipped jobs - { - // We must `spawn` here because `flush` will wait for the - // response to come back before returning - let jh = harness.spawn(|guest| async move { - guest.flush(None).await.unwrap(); - }); - - harness.ds2.ack_flush().await; - harness.ds3.ack_flush().await; - - // Wait for the flush to come back - jh.await.unwrap(); - } + // Flush to clean out skipped jobs. A read-only guest flush is acked + // locally and never sent to the downstairs, so we trigger the internal + // (automatic) flush instead, which does reach the downstairs. + harness.guest.flush_check().await.unwrap(); + harness.ds2.ack_flush().await; + harness.ds3.ack_flush().await; // Confirm that DS1 has been disconnected (and cannot reply to jobs) { diff --git a/upstairs/src/guest.rs b/upstairs/src/guest.rs index b8d24d684..9695eaccd 100644 --- a/upstairs/src/guest.rs +++ b/upstairs/src/guest.rs @@ -235,6 +235,16 @@ impl Guest { self.send_and_wait(|done| BlockOp::FaultDownstairs { client_id, done }) .await } + + /// Run the work that the automatic flush timer performs + /// + /// This is used in tests to deterministically trigger the internal flush + /// that a read-only guest flush intentionally skips. + #[cfg(test)] + pub async fn flush_check(&self) -> Result<(), CrucibleError> { + self.send_and_wait(|done| BlockOp::FlushCheck { done }) + .await + } } #[async_trait] diff --git a/upstairs/src/lib.rs b/upstairs/src/lib.rs index c2cea6a3f..38e129127 100644 --- a/upstairs/src/lib.rs +++ b/upstairs/src/lib.rs @@ -1576,6 +1576,11 @@ pub(crate) enum BlockOp { client_id: ClientId, done: BlockRes<()>, }, + + #[cfg(test)] + FlushCheck { + done: BlockRes<()>, + }, } /** diff --git a/upstairs/src/upstairs.rs b/upstairs/src/upstairs.rs index 9832cfe96..b4f5e75df 100644 --- a/upstairs/src/upstairs.rs +++ b/upstairs/src/upstairs.rs @@ -1172,7 +1172,10 @@ impl Upstairs { done.send_err(CrucibleError::UpstairsInactive); return; } - + if self.cfg.read_only { + done.send_ok(()); + return; + } let n = self.downstairs.active_client_count(); let required = if snapshot_details.is_some() { 3 } else { 2 }; if n < required { @@ -1205,6 +1208,20 @@ impl Upstairs { ); done.send_ok(()); } + + #[cfg(test)] + BlockOp::FlushCheck { done } => { + // Deterministically run the work the automatic flush timer + // does, so a test can trigger the internal flush that a + // read-only guest flush intentionally skips. We omit the + // timer's has_jobs guard because the test is forcing this + // explicitly. + if self.need_flush { + let io_guard = self.try_acquire_io(0); + self.submit_flush(None, None, io_guard); + } + done.send_ok(()); + } } } From 810bc819bfbfe43d9067f8b000b1765e3cf5b63d Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Mon, 27 Jul 2026 10:23:45 -0700 Subject: [PATCH 5/5] Add a comment --- upstairs/src/upstairs.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/upstairs/src/upstairs.rs b/upstairs/src/upstairs.rs index b4f5e75df..85e0092a6 100644 --- a/upstairs/src/upstairs.rs +++ b/upstairs/src/upstairs.rs @@ -1173,6 +1173,10 @@ impl Upstairs { return; } if self.cfg.read_only { + // While we ACK a guest sent flush here, The upstairs + // internally will still send a flush to all connected RO + // downstairs, which they are expected to handle. This + // internal flush serves to clean out completed jobs. done.send_ok(()); return; }