Protect operation attempt iteration (#858)

src/tables.rs

@@ -551,7 +551,72 @@ impl StateDb {
     }
 }
 
+/// The opt-in list of record types whose [`Table`] may be scanned generically.
+///
+/// A [`Table`] scan covers the whole column family, so it is safe only where
+/// the column family holds nothing but records. Most tables are of that shape
+/// and are listed below; `operation_attempt` is not, because it reserves the
+/// top of its key space for secondary indexes, and is therefore deliberately
+/// absent.
+///
+/// The trait is not exported, so this list is the only way in: nothing outside
+/// the crate can implement it, and a record type added here gets generic
+/// iteration only once someone writes it down. That is the point — the
+/// exclusion holds unless it is removed on purpose, rather than lapsing the
+/// next time a bound elsewhere is broadened.
+mod iteration {
+    use crate::{tables, types};
+
+    pub trait Eligible {}
+
+    impl Eligible for tables::AccessToken {}
+    impl Eligible for types::Account {}
+    impl Eligible for tables::Agent {}
+    impl Eligible for tables::AllowNetwork {}
+    impl Eligible for crate::BatchInfo {}
+    impl Eligible for tables::BlockNetwork {}
+    impl Eligible for crate::Category {}
+    impl Eligible for tables::Cluster {}
+    impl Eligible for tables::ColumnStats {}
+    impl Eligible for tables::CoreComponent {}
+    impl Eligible for tables::CsvColumnExtra {}
+    impl Eligible for tables::Customer {}
+    impl Eligible for tables::CustomerDataDeletionJob {}
+    impl Eligible for tables::DataSource {}
+    impl Eligible for tables::ExternalService {}
+    impl Eligible for tables::Filter {}
+    impl Eligible for tables::Host {}
+    impl Eligible for tables::InnerNode {}
+    impl Eligible for tables::LabelDb {}
+    impl Eligible for tables::Model {}
+    impl Eligible for tables::ModelIndicator {}
+    impl Eligible for tables::Network {}
+    impl Eligible for tables::OutlierInfo {}
+    impl Eligible for types::Qualifier {}
+    impl Eligible for tables::SamplingPolicy {}
+    impl Eligible for types::Status {}
+    impl Eligible for tables::Template {}
+    impl Eligible for tables::TimeSeries {}
+    impl Eligible for tables::TorExitNode {}
+    impl Eligible for tables::TrafficFilter {}
+    impl Eligible for tables::TriageExclusionReason {}
+    impl Eligible for tables::TriagePolicy {}
+    impl Eligible for tables::TriageResponse {}
+    impl Eligible for tables::TrustedDomain {}
+    impl Eligible for tables::TrustedUserAgent {}
+}
+
 /// Represents a table that can be iterated over.
+///
+/// A [`Table`] offers this only for a record type whose column family holds
+/// records and nothing else, because both scans cover key ranges the caller
+/// does not choose: [`iter`](Iterable::iter) covers the whole column family,
+/// and [`prefix_iter`](Iterable::prefix_iter) covers whatever the given
+/// prefix spans, which for an empty prefix is again the whole of it. A table
+/// that reserves part of its key space for secondary index entries — at
+/// present only `operation_attempt`, whose keys are described on
+/// [`OperationAttempt`] — is therefore left out, and offers a bounded
+/// iterator of its own that stops below the reserved range.
 pub trait Iterable<'i, I>
 where
     I: Iterator,
@@ -718,11 +783,16 @@ impl<R: UniqueKey + Value> Table<'_, R> {
     }
 }
 
+// The `iteration::Eligible` bound is what keeps `Table<OperationAttempt>` out
+// of this implementation, whose scans reach the index key spaces that table
+// reserves. `IndexedTable` below does not carry the bound: no table that
+// reserves part of its key space is indexed, and `NodeTable` iterates through
+// that implementation.
 impl<'i, 'j, 'k, R> Iterable<'i, TableIter<'k, R>> for Table<'j, R>
 where
     'j: 'k,
     'i: 'k,
-    R: FromKeyValue,
+    R: FromKeyValue + iteration::Eligible,
 {
     fn iter(&self, direction: Direction, from: Option<&[u8]>) -> TableIter<'k, R> {
         use rocksdb::IteratorMode;

src/tables/agent.rs

@@ -342,6 +342,37 @@ mod test {
         assert!(result.is_none());
     }
 
+    /// This column family holds records and nothing else, so the generic
+    /// `Iterable` API stays available for it: both of its scans cover a key
+    /// range the caller does not choose, and here every key in that range is a
+    /// record.
+    #[test]
+    fn generic_iteration_yields_every_stored_agent() {
+        let (_permit, store) = setup_store();
+        let table = store.agents_map();
+
+        let first = create_agent(1, "001.piglet", AgentKind::Sensor, Some(VALID_TOML), None);
+        let second = create_agent(1, "002.piglet", AgentKind::Sensor, Some(VALID_TOML), None);
+        table.insert(&first).unwrap();
+        table.insert(&second).unwrap();
+
+        let in_key_order = vec![first, second];
+        assert_eq!(
+            table
+                .iter(rocksdb::Direction::Forward, None)
+                .collect::<Result<Vec<_>>>()
+                .unwrap(),
+            in_key_order
+        );
+        assert_eq!(
+            table
+                .prefix_iter(rocksdb::Direction::Forward, None, b"")
+                .collect::<Result<Vec<_>>>()
+                .unwrap(),
+            in_key_order
+        );
+    }
+
     #[test]
     fn new_agent_has_no_install_state() {
         let agent = create_agent(1, "test_key", AgentKind::Sensor, Some(VALID_TOML), None);

src/tables/core_component.rs

@@ -440,6 +440,39 @@ mod tests {
         table.delete("roxyd", "host-99.example.com").unwrap();
     }
 
+    /// This column family holds records and nothing else, so the generic
+    /// `Iterable` API stays available for it: both of its scans cover a key
+    /// range the caller does not choose, and here every key in that range is a
+    /// record.
+    #[test]
+    fn generic_iteration_yields_every_stored_record() {
+        let test_db = TestDb::new();
+        let table = test_db.table();
+
+        let bootroot = installed("bootroot", "host-01.example.com");
+        let roxyd = installed("roxyd", "host-01.example.com");
+        table.insert(&bootroot).unwrap();
+        table.insert(&roxyd).unwrap();
+
+        // The component name is length-prefixed, so the shorter one sorts
+        // first whatever the letters say.
+        let in_key_order = vec![roxyd, bootroot];
+        assert_eq!(
+            table
+                .iter(Direction::Forward, None)
+                .collect::<Result<Vec<_>>>()
+                .unwrap(),
+            in_key_order
+        );
+        assert_eq!(
+            table
+                .prefix_iter(Direction::Forward, None, b"")
+                .collect::<Result<Vec<_>>>()
+                .unwrap(),
+            in_key_order
+        );
+    }
+
     /// `get` and `delete` build their key from the pair they are given, so
     /// iteration is the one path that hands `from_key_value` raw bytes.
     #[test]

src/tables/external_service.rs

@@ -220,8 +220,8 @@ mod test {
     use std::sync::Arc;
 
     use super::*;
-    use crate::Store;
     use crate::test::{DbGuard, acquire_db_permit};
+    use crate::{Iterable, Store};
 
     const VALID_TOML: &str = r#"test = "true""#;
 
@@ -345,6 +345,38 @@ mod test {
         assert!(external_service.bound_addrs.is_empty());
     }
 
+    /// This column family holds records and nothing else, so the generic
+    /// `Iterable` API stays available for it: both of its scans cover a key
+    /// range the caller does not choose, and here every key in that range is a
+    /// record.
+    #[test]
+    fn generic_iteration_yields_every_stored_external_service() {
+        let (_permit, store) = setup_store();
+        let table = store.external_service_map();
+
+        let first = create_external_service(1, "001.store", ExternalServiceKind::DataStore, None);
+        let second =
+            create_external_service(1, "002.store", ExternalServiceKind::TiContainer, None);
+        table.insert(&first).unwrap();
+        table.insert(&second).unwrap();
+
+        let in_key_order = vec![first, second];
+        assert_eq!(
+            table
+                .iter(rocksdb::Direction::Forward, None)
+                .collect::<Result<Vec<_>>>()
+                .unwrap(),
+            in_key_order
+        );
+        assert_eq!(
+            table
+                .prefix_iter(rocksdb::Direction::Forward, None, b"")
+                .collect::<Result<Vec<_>>>()
+                .unwrap(),
+            in_key_order
+        );
+    }
+
     #[test]
     fn install_state_round_trips() {
         let empty = create_external_service(

src/tables/node.rs

@@ -1302,6 +1302,67 @@ mod test {
         assert_eq!(returned, node);
     }
 
+    /// `NodeTable` writes its own `Iterable` implementation over an
+    /// `IndexedTable<Inner>`, so it is the one iteration route that is neither
+    /// the blanket `Table` implementation nor a table-specific inherent
+    /// iterator, and it composes each `Node` from three column families. Its
+    /// inherent `iter` shadows the trait method under method syntax, so that
+    /// half is called through UFCS to keep the trait route the thing under
+    /// test.
+    #[test]
+    fn generic_iteration_yields_every_stored_node() {
+        let (_permit, store) = setup_store();
+
+        let agent_kinds = vec![AgentKind::Sensor];
+        let agent_configs = create_agent_configs(&agent_kinds);
+        let external_service_kinds = vec![ExternalServiceKind::DataStore];
+        let external_service_configs = create_external_service_configs(&external_service_kinds);
+
+        let node_table = store.node_map();
+        let mut names = vec![];
+        for name in ["node-a", "node-b"] {
+            // Two nodes cannot share a hostname, so each carries its own.
+            let profile = Profile {
+                hostname: format!("{name}.example.com"),
+                ..Profile::default()
+            };
+            let node = create_node(
+                0,
+                name,
+                None,
+                Some(profile),
+                None,
+                create_agents(0, &agent_kinds, &agent_configs, &[None]),
+                create_external_services(0, &external_service_kinds, &external_service_configs),
+            );
+            node_table.put(&node).unwrap();
+            names.push(name.to_string());
+        }
+
+        for iterated in [
+            Iterable::iter(&node_table, Direction::Forward, None)
+                .collect::<Result<Vec<_>>>()
+                .unwrap(),
+            node_table
+                .prefix_iter(Direction::Forward, None, b"")
+                .collect::<Result<Vec<_>>>()
+                .unwrap(),
+        ] {
+            let mut read: Vec<_> = iterated
+                .iter()
+                .map(|node| node.name.clone())
+                .collect::<Vec<_>>();
+            read.sort_unstable();
+            assert_eq!(read, names);
+            // The agent and the external service come from their own column
+            // families, so an empty one would mean the composition was skipped.
+            for node in &iterated {
+                assert_eq!(node.agents.len(), 1);
+                assert_eq!(node.external_services.len(), 1);
+            }
+        }
+    }
+
     #[test]
     fn remove() {
         let (_permit, store) = setup_store();

src/tables/operation_attempt.rs

@@ -40,6 +40,17 @@
 //! learn about, or drop one and leave its entries behind. The record's key and
 //! serialized value are reached through [`OperationAttempt::record_key`] and
 //! [`OperationAttempt::record_value`], which are private to this module.
+//!
+//! The same boundary governs reading. [`Table::iter`] bounds its scan below
+//! [`RESERVED`], so it yields records and nothing else, and the generic
+//! [`Iterable`](crate::Iterable) API is not implemented for this record at
+//! all. Its scans cover key ranges the caller does not choose — the whole
+//! column family for `iter`, and for `prefix_iter` whatever the prefix spans,
+//! which an empty or deliberately reserved prefix extends over the index
+//! entries — and an index key is not a record and does not decode as one, so
+//! such a scan would yield a decoding error rather than a row. That exclusion
+//! is enforced as the write one is, by a bound this record does not satisfy
+//! rather than by convention.
 
 use std::borrow::Cow;
 use std::collections::HashMap;
@@ -200,6 +211,39 @@ pub struct RetryPolicy {
 /// fn generic_write_api<R: review_database::UniqueKey>() {}
 /// generic_write_api::<review_database::OperationAttempt>();
 /// ```
+///
+/// # Iterating
+///
+/// The record space and the index spaces share one column family, so a scan
+/// whose range the caller does not choose can run off the records and into the
+/// index entries, which are not records and do not decode as one. The generic
+/// [`Iterable`](crate::Iterable) API on [`Table`] is therefore not implemented
+/// for this record: `iter` there covers the whole column family, and
+/// `prefix_iter` covers whatever the prefix spans, which for an empty or
+/// deliberately reserved prefix is the index entries too. `Table::iter` on
+/// this table is the supported record iterator, and it bounds its scan below
+/// the reserved range.
+///
+/// A record whose column family holds records alone is admitted:
+///
+/// ```
+/// use review_database::{Iterable, Table, TorExitNode};
+///
+/// fn generic_iteration(table: &Table<TorExitNode>) {
+///     let _ = table.prefix_iter(todo!(), None, b"");
+/// }
+/// ```
+///
+/// An `OperationAttempt` is not, which is what stops a caller from reaching an
+/// index entry through a scan that expects a row:
+///
+/// ```compile_fail,E0599
+/// use review_database::{Iterable, OperationAttempt, Table};
+///
+/// fn generic_iteration(table: &Table<OperationAttempt>) {
+///     let _ = table.prefix_iter(todo!(), None, b"");
+/// }
+/// ```
 #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
 pub struct OperationAttempt {
     /// The globally unique key of the logical operation. Never empty.
@@ -504,10 +548,13 @@ impl<'d> Table<'d, OperationAttempt> {
     /// Returns an iterator over the stored attempts, whose keys are the
     /// idempotency keys in lexicographic order.
     ///
-    /// This shadows the blanket [`Iterable::iter`](crate::Iterable)
-    /// implementation on [`Table`], whose scan covers the whole column family
-    /// and would therefore also yield the index key spaces, which are not
-    /// records and do not decode as one.
+    /// This is the table's only iterator. The generic
+    /// [`Iterable`](crate::Iterable) API on [`Table`] is not implemented for
+    /// this record, because its scans cover key ranges the caller does not
+    /// choose and would therefore also yield the index key spaces, which are
+    /// not records and do not decode as one. This one stops below the first
+    /// byte the index key spaces reserve, so it yields records and nothing
+    /// else.
     #[must_use]
     pub fn iter(
         &self,
@@ -1365,6 +1412,30 @@ mod tests {
         // A key that was never written seeks to the next one in the direction
         // of travel, so a scan resuming past a deleted row does not stall.
         assert_eq!(keys(&table, Direction::Forward, Some(b"op-25")), ["op-3"]);
+
+        // An attempt that is non-terminal and still owes a cleanup owns an
+        // entry in all three indexes, so it is the case that puts the most of
+        // the reserved space in the iterator's way.
+        let mut owes_cleanup = module_attempt("op-4");
+        owes_cleanup.instance = Some(3);
+        owes_cleanup.cleanup_state = Some(CleanupState::PendingDeregister);
+        table.upsert(&owes_cleanup).unwrap();
+        let index_entries = test_db
+            .raw_keys()
+            .into_iter()
+            .filter(|key| key.first().is_some_and(|first| *first >= RESERVED))
+            .count();
+        assert_eq!(
+            index_entries, 9,
+            "the three earlier attempts own two entries each, and this one owns three"
+        );
+
+        // `keys` unwraps the decode of every item, so this is also the
+        // assertion that the bounded scan yields no index key.
+        assert_eq!(
+            keys(&table, Direction::Forward, None),
+            ["op-1", "op-2", "op-3", "op-4"]
+        );
     }
 
     #[test]