Merge branch 'bomb' into 'main'

tor_memtrack: Soup up DropBomb. make into its own module, and add more tests

Closes #1381

See merge request tpo/core/arti!2100

6 files changed, 654 insertions, 76 deletions

diff --git a/crates/tor-memtrack/src/drop_bomb.rs b/crates/tor-memtrack/src/drop_bomb.rs
new file mode 100644
index 000000000..6a7b55fa6
--- /dev/null
+++ b/crates/tor-memtrack/src/drop_bomb.rs
@@ -0,0 +1,515 @@
+//! Drop bombs, for assurance of postconditions when types are dropped
+//!
+//! Provides two drop bomb types: [`DropBomb`] and [`DropBombCondition`].
+//!
+//! These help assure that our algorithms are correct,
+//! by detecting when types that contain the bomb are dropped inappropriately.
+//!
+//! # No-op outside `#[cfg(test)]`
+//!
+//! When used outside test code, these types are unit ZSTs,
+//! and are completely inert.
+//! They won't cause panics or detect bugs, in production.
+//!
+//! # Panics (in tests), and simulation
+//!
+//! These types work by panicking in drop, when a bug is detected.
+//! This will then cause a test failure.
+//! Such panics are described as "explodes (panics)" in the documentation.
+//!
+//! There are also simulated drop bombs, whose explosions do not actually panic.
+//! Instead, they record that a panic would have occurred,
+//! and print a message to stderr.
+//! The constructors provide a handle to allow the caller to enquire about explosions.
+//! This allows for testing a containing type's drop bomb logic.
+//!
+//! Certain misuses result in actual panics, even with simulated bombs.
+//! This is described as "panics (actually)".
+//!
+//! # Choosing a bomb
+//!
+//! [`DropBomb`] is for assuring the runtime context or appropriate timing of drops
+//! (and could be used for implementing general conditions).
+//!
+//! [`DropBombCondition`] is for assuring the properties of a value that is being dropped.
+
+use crate::internal_prelude::*;
+
+#[cfg(test)]
+use std::sync::atomic::{AtomicBool, Ordering};
+
+//---------- macros used in this module, and supporting trait ----------
+
+define_derive_deftly! {
+    /// Helper for common impls on bombs
+    ///
+    ///  * Provides `fn new_armed`
+    ///  * Provides `fn new_simulated`
+    ///  * Implements `Drop`, using `TestableDrop::drop_impl`
+    BombImpls =
+
+    impl $ttype {
+        /// Create a new drop bomb, which must be properly disposed of
+        pub(crate) const fn new_armed() -> Self {
+            let status = Status::ARMED_IN_TESTS;
+            $ttype { status }
+        }
+    }
+
+    #[cfg(test)]
+    impl $ttype {
+        /// Create a simulated drop bomb
+        pub(crate) fn new_simulated() -> (Self, SimulationHandle) {
+            let handle = SimulationHandle::new();
+            let status = S::ArmedSimulated(handle.clone());
+            ($ttype { status }, handle)
+        }
+
+        /// Turn an existing armed drop bomb into a simulated one
+        ///
+        /// This is useful for writing test cases, without having to make a `new_simulated`
+        /// constructor for whatever type contains the drop bomb.
+        /// Instead, construct it normally, and then reach in and call this on the bomb.
+        ///
+        /// # Panics
+        ///
+        /// `self` must be armed.  Otherwise, (actually) panics.
+        pub(crate) fn make_simulated(&mut self) -> SimulationHandle {
+            let handle = SimulationHandle::new();
+            let new_status = S::ArmedSimulated(handle.clone());
+            let old_status = mem::replace(&mut self.status, new_status);
+            assert!(matches!(old_status, S::Armed));
+            handle
+        }
+
+        /// Implemnetation of `Drop::drop`, split out for testability.
+        ///
+        /// Calls `drop_status`, and replaces `self.status` with `S::Disarmed`,
+        /// so that `self` can be actually dropped (if we didn't panic).
+        fn drop_impl(&mut self) {
+            // Do the replacement first, so that if drop_status unwinds, we don't panic in panic.
+            let status = mem::replace(&mut self.status, S::Disarmed);
+            <$ttype as DropStatus>::drop_status(status);
+        }
+    }
+
+
+    #[cfg(test)]
+    impl Drop for $ttype {
+        fn drop(&mut self) {
+            // We don't check for unwinding.
+            // We shouldn't drop a nonzero one of these even if we're panicking.
+            // If we do, it'll be a double panic => abort.
+            self.drop_impl();
+        }
+    }
+}
+
+/// Core of `Drop`, that can be called separately, for testing
+///
+/// To use: implement this, and derive deftly
+/// [`BombImpls`](derive_deftly_template_BombImpls).
+trait DropStatus {
+    /// Handles dropping of a `Self` with this `status` field value
+    fn drop_status(status: Status);
+}
+
+//---------- public types ----------
+
+/// Drop bomb: for assuring that drops happen only when expected
+///
+/// Obtained from [`DropBomb::new_armed()`].
+///
+/// # Explosions
+///
+/// Explodes (panicking) if dropped,
+/// unless [`.disarm()`](DropBomb::disarm) is called first.
+#[derive(Deftly, Debug)]
+#[derive_deftly(BombImpls)]
+pub(crate) struct DropBomb {
+    /// What state are we in
+    status: Status,
+}
+
+/// Drop condition: for ensuring that a condition is true, on drop
+///
+/// Obtained from [`DropBombCondition::new_armed()`].
+///
+/// Instead of dropping this, you must call
+/// `drop_bomb_disarm_assert!`
+/// (or its internal function `disarm_assert()`.
+// rustdoc can't manage to make a link to this crate-private macro or cfg-test item.
+///
+/// It will often be necessary to add `#[allow(dead_code)]`
+/// on the `DropBombCondition` field of a containing type,
+/// since outside tests, the `Drop` impl will usually be configured out,
+/// and that's the only place this field is actually read.
+///
+/// # Panics
+///
+/// Panics (actually) if it is simply dropped.
+#[derive(Deftly, Debug)]
+#[derive_deftly(BombImpls)]
+pub(crate) struct DropBombCondition {
+    /// What state are we in
+    #[allow(dead_code)] // not read outside tests
+    status: Status,
+}
+
+/// Handle onto a simulated [`DropBomb`] or [`DropCondition`]
+///
+/// Can be used to tell whether the bomb "exploded"
+/// (ie, whether `drop` would have panicked, if this had been a non-simulated bomb).
+#[cfg(test)]
+#[derive(Debug)]
+pub(crate) struct SimulationHandle {
+    exploded: Arc<AtomicBool>,
+}
+
+/// Unit token indicating that a simulated drop bomb did explode, and would have panicked
+#[cfg(test)]
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)]
+pub(crate) struct SimulationExploded;
+
+//---------- internal types ----------
+
+/// State of some kind of drop bomb
+///
+/// This type is inert; the caller is responsible for exploding or panicking.
+#[derive(Debug)]
+enum Status {
+    /// This bomb is disarmed and will not panic.
+    ///
+    /// This is always the case outside `#[cfg(test)]`
+    Disarmed,
+
+    /// This bomb is armed.  It will (or may) panic on drop.
+    #[cfg(test)]
+    Armed,
+
+    /// This bomb is armed, but we're running in simulation.
+    #[cfg(test)]
+    ArmedSimulated(SimulationHandle),
+}
+
+use Status as S;
+
+//---------- DropBomb impls ----------
+
+impl DropBomb {
+    /// Disarm this bomb.
+    ///
+    /// It will no longer explode (panic) when dropped.
+    pub(crate) fn disarm(&mut self) {
+        self.status = S::Disarmed;
+    }
+}
+
+#[cfg(test)]
+impl DropStatus for DropBomb {
+    fn drop_status(status: Status) {
+        match status {
+            S::Disarmed => {}
+            S::Armed => panic!("DropBomb dropped without a previous call to .disarm()"),
+            S::ArmedSimulated(handle) => handle.set_exploded(),
+        }
+    }
+}
+
+//---------- DropCondition impls ----------
+
+/// Check the condition, and disarm the bomb
+///
+/// If `CONDITION` is true, disarms the bomb; otherwise, explodes (panics).
+///
+/// # Syntax
+///
+/// ```
+/// drop_bomb_disarm_assert!(BOMB, CONDITION);
+/// drop_bomb_disarm_assert!(BOMB, CONDITION, "FORMAT", FORMAT_ARGS..);
+/// ```
+///
+/// where
+///
+///  * `BOMB: &mut DropCondition` (or something that derefs to that).
+///  * `CONDITION: bool`
+///
+/// # Example
+///
+/// ```
+/// # struct S { drop_bomb: DropCondition };
+/// # impl S { fn f(&mut self) {
+/// drop_bomb_disarm_assert!(self.drop_bomb, self.raw, Qty(0));
+/// # } }
+/// ```
+///
+/// # Explodes
+///
+/// Explodes unless the condition is satisfied.
+//
+// This macro has this long name because we can't do scoping of macro-rules macros.
+#[cfg(test)] // Should not be used outside tests, since the drop impls should be conditional
+macro_rules! drop_bomb_disarm_assert {
+    { $bomb:expr, $condition:expr $(,)? } => {
+        $bomb.disarm_assert(
+            || $condition,
+            format_args!(concat!("condition = ", stringify!($condition))),
+        )
+    };
+    { $bomb:expr, $condition:expr, $fmt:literal $($rest:tt)* } => {
+        $bomb.disarm_assert(
+            || $condition,
+            format_args!(concat!("condition = ", stringify!($condition), ": ", $fmt),
+                         $($rest)*),
+        )
+    };
+}
+
+impl DropBombCondition {
+    /// Check a condition, and disarm the bomb
+    ///
+    /// If `call()` returns true, disarms the bomb; otherwise, explodes (panics).
+    ///
+    /// # Explodes
+    ///
+    /// Explodes unless the condition is satisfied.
+    #[inline]
+    #[cfg(test)] // Should not be used outside tests, since the drop impls should be conditional
+    pub(crate) fn disarm_assert(&mut self, call: impl FnOnce() -> bool, msg: fmt::Arguments) {
+        match mem::replace(&mut self.status, S::Disarmed) {
+            S::Disarmed => {
+                // outside cfg(test), this is the usual path.
+                // placate the compiler: we ignore all our arguments
+                let _ = call;
+                let _ = msg;
+
+                #[cfg(test)]
+                panic!("disarm_assert called more than once!");
+            }
+            #[cfg(test)]
+            S::Armed => {
+                if !call() {
+                    panic!("drop condition violated: dropped, but condition is false: {msg}");
+                }
+            }
+            #[cfg(test)]
+            #[allow(clippy::print_stderr)]
+            S::ArmedSimulated(handle) => {
+                if !call() {
+                    eprintln!("drop condition violated in simulation: {msg}");
+                    handle.set_exploded();
+                }
+            }
+        }
+    }
+}
+
+/// Ideally, if you use this, your struct's other default values meet your drop condition!
+impl Default for DropBombCondition {
+    fn default() -> DropBombCondition {
+        Self::new_armed()
+    }
+}
+
+#[cfg(test)]
+impl DropStatus for DropBombCondition {
+    fn drop_status(status: Status) {
+        assert!(matches!(status, S::Disarmed));
+    }
+}
+
+//---------- SimulationHandle impls ----------
+
+#[cfg(test)]
+impl SimulationHandle {
+    /// Determine whether a drop bomb would have been triggered
+    ///
+    /// If the corresponding [`DropBomb]` or [`DropCondition`]
+    /// would have panicked (if we weren't simulating),
+    /// returns `Err`.
+    ///
+    /// # Panics
+    ///
+    /// The corresponding `DropBomb` or `DropCondition` must have been dropped.
+    /// Otherwise, calling `outcome` will (actually) panic.
+    pub(crate) fn outcome(mut self) -> Result<(), SimulationExploded> {
+        let panicked = Arc::into_inner(mem::take(&mut self.exploded))
+            .expect("bomb has not yet been dropped")
+            .into_inner();
+        if panicked {
+            Err(SimulationExploded)
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Require that this bomb did *not* explode
+    ///
+    /// # Panics
+    ///
+    /// Panics if corresponding `DropBomb` hasn't yet been dropped,
+    /// or if it exploded when it was dropped.
+    pub(crate) fn expect_ok(self) {
+        let () = self.outcome().expect("bomb unexpectedly exploded");
+    }
+
+    /// Require that this bomb *did* explode
+    ///
+    /// # Panics
+    ///
+    /// Panics if corresponding `DropBomb` hasn't yet been dropped,
+    /// or if it did *not* explode when it was dropped.
+    pub(crate) fn expect_exploded(self) {
+        let SimulationExploded = self
+            .outcome()
+            .expect_err("bomb unexpectedly didn't explode");
+    }
+
+    /// Return a new handle with no explosion recorded
+    fn new() -> Self {
+        SimulationHandle {
+            exploded: Default::default(),
+        }
+    }
+
+    /// Return a clone of this handle
+    //
+    // Deliberately not a public Clone impl
+    fn clone(&self) -> Self {
+        SimulationHandle {
+            exploded: self.exploded.clone(),
+        }
+    }
+
+    /// Mark this simulated bomb as having exploded
+    fn set_exploded(&self) {
+        self.exploded.store(true, Ordering::Release);
+    }
+}
+
+//---------- internal impls ----------
+
+impl Status {
+    /// Armed, in tests
+    #[cfg(test)]
+    const ARMED_IN_TESTS: Status = S::Armed;
+
+    /// "Armed", outside tests, is in fact not armed
+    #[cfg(not(test))]
+    const ARMED_IN_TESTS: Status = S::Disarmed;
+}
+
+#[cfg(test)]
+mod test {
+    // @@ begin test lint list maintained by maint/add_warning @@
+    #![allow(clippy::bool_assert_comparison)]
+    #![allow(clippy::clone_on_copy)]
+    #![allow(clippy::dbg_macro)]
+    #![allow(clippy::mixed_attributes_style)]
+    #![allow(clippy::print_stderr)]
+    #![allow(clippy::print_stdout)]
+    #![allow(clippy::single_char_pattern)]
+    #![allow(clippy::unwrap_used)]
+    #![allow(clippy::unchecked_duration_subtraction)]
+    #![allow(clippy::useless_vec)]
+    #![allow(clippy::needless_pass_by_value)]
+    //! <!-- @@ end test lint list maintained by maint/add_warning @@ -->
+    #![allow(clippy::let_and_return)] // TODO this lint is annoying and we should disable it
+
+    use super::*;
+    use std::any::Any;
+    use std::panic::catch_unwind;
+
+    #[test]
+    fn bomb_disarmed() {
+        let mut b = DropBomb::new_armed();
+        b.disarm();
+        drop(b);
+    }
+
+    #[test]
+    fn bomb_panic() {
+        let mut b = DropBomb::new_armed();
+        let _: Box<dyn Any> = catch_unwind(AssertUnwindSafe(|| b.drop_impl())).unwrap_err();
+    }
+
+    #[test]
+    fn bomb_sim_disarmed() {
+        let (mut b, h) = DropBomb::new_simulated();
+        b.disarm();
+        drop(b);
+        h.expect_ok();
+    }
+
+    #[test]
+    fn bomb_sim_explosion() {
+        let (b, h) = DropBomb::new_simulated();
+        drop(b);
+        h.expect_exploded();
+    }
+
+    #[test]
+    fn bomb_make_sim_explosion() {
+        let mut b = DropBomb::new_armed();
+        let h = b.make_simulated();
+        drop(b);
+        h.expect_exploded();
+    }
+
+    struct HasBomb {
+        on_drop: Result<(), ()>,
+        bomb: DropBombCondition,
+    }
+
+    impl Drop for HasBomb {
+        fn drop(&mut self) {
+            drop_bomb_disarm_assert!(self.bomb, self.on_drop.is_ok());
+        }
+    }
+
+    #[test]
+    fn cond_ok() {
+        let hb = HasBomb {
+            on_drop: Ok(()),
+            bomb: DropBombCondition::new_armed(),
+        };
+        drop(hb);
+    }
+
+    #[test]
+    fn cond_sim_explosion() {
+        let (bomb, h) = DropBombCondition::new_simulated();
+        let hb = HasBomb {
+            on_drop: Err(()),
+            bomb,
+        };
+        drop(hb);
+        h.expect_exploded();
+    }
+
+    #[test]
+    fn cond_explosion_panic() {
+        // make an actual panic
+        let mut bomb = DropBombCondition::new_armed();
+        let _: Box<dyn Any> = catch_unwind(AssertUnwindSafe(|| {
+            bomb.disarm_assert(|| false, format_args!("testing"));
+        }))
+        .unwrap_err();
+    }
+
+    #[test]
+    fn cond_forgot_drop_impl() {
+        // pretend that we put a DropBombCondition on this,
+        // but we forgot to impl Drop and call drop_bomb_disarm_assert
+        struct ForgotDropImpl {
+            bomb: DropBombCondition,
+        }
+        let fdi = ForgotDropImpl {
+            bomb: DropBombCondition::new_armed(),
+        };
+        // pretend that fdi is being dropped
+        let mut bomb = fdi.bomb; // move out
+
+        let _: Box<dyn Any> = catch_unwind(AssertUnwindSafe(|| bomb.drop_impl())).unwrap_err();
+    }
+}
diff --git a/crates/tor-memtrack/src/drop_reentrancy.rs b/crates/tor-memtrack/src/drop_reentrancy.rs
index 328cb629a..3e157cf88 100644
--- a/crates/tor-memtrack/src/drop_reentrancy.rs
+++ b/crates/tor-memtrack/src/drop_reentrancy.rs
@@ -97,69 +97,47 @@ impl<P: ?Sized> ProtectedArc<P> {
     /// If the return value is dropped, the location must be suitable for that.
     /// Or, maybe the returned value is going to calling code in the external user,
     /// (which, therefore, wouldn't pose a reentrancy hazard).
-    pub(crate) fn promise_dropping_is_ok(self) -> Arc<P> {
+    pub(crate) fn promise_dropping_is_ok(mut self) -> Arc<P> {
         self.bomb.disarm();
         self.arc
     }
 }
 
-/// Drop bomb, which might be armed or disarmed
-///
-/// Panics on drop, if armed.
-///
-/// Separate type from `ProtectedArc` so we can move out of [`ProtectedArc`]
-#[derive(Debug)]
-struct DropBomb(Result<(), Armed>);
-
-/// Armedness of a drop bomb
-///
-/// In tests, is a unit.
-/// Uninhabited outside of tests.
-///
-/// Separate type from `DropBomb` because assigning to a field drops the old value,
-/// so this marker type mustn't itself have the drop bomb.
-///
-/// Making this an enum rather than a plain unit lets it all go away in non-test builds.
-#[derive(Debug)]
-enum Armed {
-    /// `Err(Armed::Armed)` means the bomb is armed
-    #[cfg(test)]
-    Armed,
-}
-
-impl Armed {
-    /// In tests, we start armed
-    #[cfg(test)]
-    const ARMED_IF_TEST: Option<Armed> = Some(Armed::Armed);
-    /// Otherwise, we're never armed
-    #[cfg(not(test))]
-    const ARMED_IF_TEST: Option<Armed> = None;
-}
-
-impl DropBomb {
-    /// Create a new, armed, drop bomb
-    ///
-    /// In tests, this will panic if it's dropped without calling `disarm` beforehand.
-    /// (Outside tests, there aren't any real bombs, so then it's disarmed.)
-    fn new_armed() -> Self {
-        DropBomb(Armed::ARMED_IF_TEST.map(Err).unwrap_or(Ok(())))
-    }
-
-    /// Dispose of this bomb without setting it off
-    ///
-    /// # CORRECTNESS
-    ///
-    /// It's your responsibility to assure whatever property you're trying to maintain.
-    fn disarm(mut self) {
-        self.0 = Ok(());
+#[cfg(test)]
+mod test {
+    // @@ begin test lint list maintained by maint/add_warning @@
+    #![allow(clippy::bool_assert_comparison)]
+    #![allow(clippy::clone_on_copy)]
+    #![allow(clippy::dbg_macro)]
+    #![allow(clippy::mixed_attributes_style)]
+    #![allow(clippy::print_stderr)]
+    #![allow(clippy::print_stdout)]
+    #![allow(clippy::single_char_pattern)]
+    #![allow(clippy::unwrap_used)]
+    #![allow(clippy::unchecked_duration_subtraction)]
+    #![allow(clippy::useless_vec)]
+    #![allow(clippy::needless_pass_by_value)]
+    //! <!-- @@ end test lint list maintained by maint/add_warning @@ -->
+    #![allow(clippy::let_and_return)] // TODO this lint is annoying and we should disable it
+
+    use super::*;
+
+    struct Payload;
+
+    #[test]
+    fn fine() {
+        let arc = Arc::new(Payload);
+        let prot = ProtectedArc::new(arc);
+        let arc = prot.promise_dropping_is_ok();
+        drop(arc);
     }
-}
 
-#[cfg(test)]
-impl Drop for DropBomb {
-    fn drop(&mut self) {
-        self.0
-            .as_ref()
-            .expect("dropped a ProtectedArc rather than calling promise_dropping_is_ok");
+    #[test]
+    fn bad() {
+        let arc = Arc::new(Payload);
+        let mut prot = ProtectedArc::new(arc);
+        let h = prot.bomb.make_simulated();
+        drop(prot);
+        h.expect_exploded();
     }
 }
diff --git a/crates/tor-memtrack/src/internal_prelude.rs b/crates/tor-memtrack/src/internal_prelude.rs
index c3c48b839..c727496f4 100644
--- a/crates/tor-memtrack/src/internal_prelude.rs
+++ b/crates/tor-memtrack/src/internal_prelude.rs
@@ -55,6 +55,7 @@ pub(crate) use {
 
 pub(crate) use crate::{
     config::Config,
+    drop_bomb::{DropBomb, DropBombCondition},
     drop_reentrancy,
     error::{Error, ReclaimCrashed, StartupError, TrackerCorrupted},
     refcount,
diff --git a/crates/tor-memtrack/src/lib.rs b/crates/tor-memtrack/src/lib.rs
index f87ef77aa..f6d5ad21b 100644
--- a/crates/tor-memtrack/src/lib.rs
+++ b/crates/tor-memtrack/src/lib.rs
@@ -183,6 +183,8 @@
 #![allow(clippy::blocks_in_conditions)] // TODO #1176
 
 // Internal supporting modules
+#[macro_use]
+mod drop_bomb;
 mod drop_reentrancy;
 mod internal_prelude;
 #[macro_use]
diff --git a/crates/tor-memtrack/src/mtracker/bookkeeping.rs b/crates/tor-memtrack/src/mtracker/bookkeeping.rs
index dcd97cf80..ec1241196 100644
--- a/crates/tor-memtrack/src/mtracker/bookkeeping.rs
+++ b/crates/tor-memtrack/src/mtracker/bookkeeping.rs
@@ -17,12 +17,20 @@ use super::*;
 define_derive_deftly! {
     /// Implement [`BookkeptQty`] and its supertraits
     ///
-    /// By default, dropping when nonzero is forbidden.
-    /// `#[deftly(allow_nonzero_drop)]` suppresses this drop bomb.
+    /// By default, dropping when nonzero is forbidden,
+    /// and you must have a field `bomb: `[`DropBombCondition`].
+    /// `#[deftly(allow_nonzero_drop)]` suppresses this.
     BookkeptQty =
 
+    ${defcond BOMB not(tmeta(allow_nonzero_drop))}
+
     impl BookkeepableQty for $ttype {
-        const ZERO: $ttype = $ttype { raw: Qty(0) };
+        const ZERO: $ttype = $ttype {
+            raw: Qty(0),
+          ${if BOMB {
+            bomb: DropBombCondition::new_armed(),
+          }}
+        };
 
         fn as_raw(&self) -> Qty {
             self.raw
@@ -44,7 +52,12 @@ define_derive_deftly! {
 
     impl BookkeptQty for $ttype {
         fn from_raw(q: Qty) -> Self {
-            $ttype { raw: q }
+            $ttype {
+                raw: q,
+              ${if BOMB {
+                bomb: DropBombCondition::new_armed(),
+              }}
+            }
         }
         fn into_raw(mut self) -> Qty {
             mem::replace(&mut self.raw, Qty(0))
@@ -53,14 +66,11 @@ define_derive_deftly! {
 
     assert_not_impl_any!($ttype: Clone, Into<Qty>, From<Qty>);
 
-  ${if not(tmeta(allow_nonzero_drop)) {
+  ${if BOMB {
     #[cfg(test)]
     impl Drop for $ttype {
         fn drop(&mut self) {
-            // We don't check for unwinding.
-            // We shouldn't drop a nonzero one of these even if we're panicking.
-            // If we do, it'll be a double panic => abort.
-            assert_eq!(self.raw, Qty(0));
+            drop_bomb_disarm_assert!(self.bomb, self.raw == Qty(0));
         }
     }
   }}
@@ -143,9 +153,13 @@ pub(super) struct TotalQty {
 /// This is the total amount `claim`ed, plus the caches in each `Participation`.
 #[derive(Default, Debug, Deftly, derive_more::Display)]
 #[derive_deftly(BookkeptQty)]
+#[display(fmt = "{raw}")]
 pub(super) struct ParticipQty {
     /// See [`BookkeptQty`]
     raw: Qty,
+
+    /// See [`BookkeptQty`]
+    bomb: DropBombCondition,
 }
 
 /// "Cached" claim, on behalf of a Participant
@@ -159,10 +173,14 @@ pub(super) struct ParticipQty {
 /// to store.  The participant is supposed to track this separately somehow.
 #[derive(Default, Debug, Deftly, derive_more::Display)]
 #[derive_deftly(BookkeptQty)]
+#[display(fmt = "{raw}")]
 #[must_use]
 pub(super) struct ClaimedQty {
     /// See [`BookkeptQty`]
     raw: Qty,
+
+    /// See [`BookkeptQty`]
+    bomb: DropBombCondition,
 }
 
 impl TotalQty {
diff --git a/crates/tor-memtrack/src/refcount.rs b/crates/tor-memtrack/src/refcount.rs
index 83a88c8b6..cbe17b7f0 100644
--- a/crates/tor-memtrack/src/refcount.rs
+++ b/crates/tor-memtrack/src/refcount.rs
@@ -80,9 +80,12 @@ pub(crate) struct Ref<K: slotmap::Key> {
     /// Bind to the specific key type
     #[educe(Debug(ignore))]
     marker: PhantomData<K>,
-    /// Marker to not be Clone
-    #[educe(Debug(ignore))]
-    not_clone: NotClone,
+    /// Drop bomb
+    ///
+    /// Also forces `Ref` not to be Clone
+    #[educe(Debug(ignore), Ord(ignore), Eq(ignore), PartialEq(ignore))]
+    #[allow(dead_code)]
+    bomb: DropBombCondition,
 }
 
 // educe's Ord is open-coded and triggers clippy::non_canonical_partial_ord_impl
@@ -92,11 +95,8 @@ impl<K: slotmap::Key> PartialOrd for Ref<K> {
     }
 }
 
-/// Marker used to prevent `Ref` being `Clone`
 // Ideally we'd assert_not_impl on Ref but it has generics
-#[derive(Default, Ord, PartialOrd, Eq, PartialEq)]
-struct NotClone;
-assert_not_impl_any!(NotClone: Clone);
+assert_not_impl_any!(DropBombCondition: Clone);
 
 /// Error: refcount overflowed
 #[derive(Debug, Clone, Error, Eq, PartialEq)]
@@ -166,7 +166,7 @@ impl<K: slotmap::Key> Ref<K> {
         Ref {
             raw_key,
             marker: PhantomData,
-            not_clone: NotClone,
+            bomb: DropBombCondition::new_armed(),
         }
     }
 
@@ -232,7 +232,7 @@ pub(crate) fn slotmap_try_insert<K: slotmap::Key, V, E, RD>(
     let ref_ = Ref {
         raw_key,
         marker: PhantomData,
-        not_clone: NotClone,
+        bomb: DropBombCondition::new_armed(),
     };
     Ok((ref_, data))
 }
@@ -240,7 +240,7 @@ pub(crate) fn slotmap_try_insert<K: slotmap::Key, V, E, RD>(
 #[cfg(test)]
 impl<K: slotmap::Key> Drop for Ref<K> {
     fn drop(&mut self) {
-        assert!(self.raw_key.is_null());
+        drop_bomb_disarm_assert!(self.bomb, self.raw_key.is_null(),);
     }
 }
 
@@ -249,3 +249,67 @@ impl From<Overflow> for Error {
         internal!("reference count overflow in memory tracking (out-of-control subsystem?)").into()
     }
 }
+
+#[cfg(test)]
+mod test {
+    // @@ begin test lint list maintained by maint/add_warning @@
+    #![allow(clippy::bool_assert_comparison)]
+    #![allow(clippy::clone_on_copy)]
+    #![allow(clippy::dbg_macro)]
+    #![allow(clippy::mixed_attributes_style)]
+    #![allow(clippy::print_stderr)]
+    #![allow(clippy::print_stdout)]
+    #![allow(clippy::single_char_pattern)]
+    #![allow(clippy::unwrap_used)]
+    #![allow(clippy::unchecked_duration_subtraction)]
+    #![allow(clippy::useless_vec)]
+    #![allow(clippy::needless_pass_by_value)]
+    //! <!-- @@ end test lint list maintained by maint/add_warning @@ -->
+    #![allow(clippy::let_and_return)] // TODO this lint is annoying and we should disable it
+
+    use super::*;
+
+    slotmap::new_key_type! {
+        struct Id;
+    }
+    #[derive(Eq, PartialEq, Debug)]
+    struct Record {
+        refcount: Count<Id>,
+    }
+    type Map = SlotMap<Id, Record>;
+
+    fn setup() -> (Map, Ref<Id>) {
+        let mut map = Map::default();
+        let ref_ = slotmap_insert(&mut map, |refcount| Record { refcount });
+        (map, ref_)
+    }
+
+    #[test]
+    fn good() {
+        let (mut map, ref1) = setup();
+
+        let ent = map.get_mut(*ref1).unwrap();
+        let ref2 = Ref::new(*ref1, &mut ent.refcount).unwrap();
+
+        let g1: Option<Garbage<Record>> = slotmap_dec_ref!(&mut map, ref1, &mut ent.refcount);
+        assert_eq!(g1, None);
+
+        let ent = map.get_mut(*ref2).unwrap();
+        let g2: Option<Garbage<Record>> = slotmap_dec_ref!(&mut map, ref2, &mut ent.refcount);
+        assert!(g2.is_some());
+    }
+
+    #[test]
+    fn try_insert_fail() {
+        let mut map = Map::default();
+        let () = slotmap_try_insert::<_, _, _, String>(&mut map, |_refcount| Err(())).unwrap_err();
+    }
+
+    #[test]
+    fn drop_ref_without_decrement() {
+        let (_map, mut ref1) = setup();
+        let h = ref1.bomb.make_simulated();
+        drop(ref1);
+        h.expect_exploded();
+    }
+}