fix(notify): close core notify correctness and safety gaps (#4502)

Land the remaining notify-crate audit fixes.

backlog#979(b): remove_target now enforces the same bucket-binding guard as
remove_target_config, refusing to delete a target still referenced by a bucket
rule so notification rules are not left orphaned.

backlog#984:
- event.rs: an unversioned object omits versionId entirely instead of
  serializing versionId:"" (empty object/request versions treated as "no
  version").
- notifier.rs: RUSTFS_NOTIFY_SEND_CONCURRENCY=0 coerces back to the default
  instead of building a zero-permit semaphore that deadlocks every dispatch;
  init_bucket_targets_shared closes the replaced targets instead of dropping
  them without close() (connection leak).
- subscriber_index.rs: store_snapshot uses an atomic compute_if_absent upsert,
  removing the get-then-insert TOCTOU that could clobber a concurrent
  first-writer's snapshot cell.
- pipeline.rs: send_event assigns the history sequence and broadcasts to live
  subscribers under one critical section so broadcast order matches recorded
  sequence order.
- xml_config.rs: filter value length is bounded by character count, not byte
  length, so valid multi-byte keys are no longer wrongly rejected.
- global.rs: a losing initialize() race shuts the just-initialized system down
  instead of leaking its targets/replay workers.

backlog#970 (notify part): reload_config stops the running replay workers
before activating the new ones, so old and new workers do not concurrently
drain the same persisted stores. The full signal+join shutdown lives in the
targets crate under the same issue.

Tests: added regression coverage for each fix.
cargo build -p rustfs-notify, cargo test -p rustfs-notify --lib (98 passed),
cargo clippy -p rustfs-notify --all-targets (clean).

Relates to rustfs/backlog#979
Relates to rustfs/backlog#984
Relates to rustfs/backlog#970

Co-authored-by: heihutu <heihutu@gmail.com>

crates/notify/src/rules/subscriber_index.rs

@@ -93,12 +93,17 @@ impl SubscriberIndex {
     pub fn store_snapshot(&self, bucket: &str, new_snapshot: BucketRulesSnapshot<DynRulesContainer>) {
         let key = bucket.to_string();
 
-        let cell = self.inner.get(&key).unwrap_or_else(|| {
-            // Insert a default cell (empty snapshot)
-            let init = Arc::new(ArcSwap::from_pointee(BucketRulesSnapshot::empty(self.empty_rules.clone())));
-            self.inner.insert(key.clone(), init.clone());
-            init
-        });
+        // Atomic get-or-create of the bucket's cell. The previous `get()` then
+        // `insert()` was a TOCTOU: two concurrent first-writers for the same bucket
+        // could both observe `None`, each build a distinct `ArcSwap` cell, and both
+        // `insert` — the second insert overwrites the first cell, so the snapshot
+        // stored into the discarded cell is silently lost. `compute_if_absent` holds
+        // the shard write lock across the check-and-insert, so every caller shares the
+        // one winning cell and no snapshot is dropped (backlog#984).
+        let empty_rules = self.empty_rules.clone();
+        let cell = self
+            .inner
+            .compute_if_absent(key, || Arc::new(ArcSwap::from_pointee(BucketRulesSnapshot::empty(empty_rules.clone()))));
 
         cell.store(Arc::new(new_snapshot));
     }
@@ -116,16 +121,74 @@ impl SubscriberIndex {
 
 impl Default for SubscriberIndex {
     fn default() -> Self {
-        // An available empty rule container is required; here it is implemented using minimal empty
-        #[derive(Debug)]
-        struct EmptyRules;
-        impl crate::rules::subscriber_snapshot::RulesContainer for EmptyRules {
-            type Rule = dyn crate::rules::subscriber_snapshot::RuleEvents;
-            fn iter_rules<'a>(&'a self) -> Box<dyn Iterator<Item = &'a Self::Rule> + 'a> {
-                Box::new(std::iter::empty())
-            }
-        }
-
         Self::new(Arc::new(EmptyRules) as Arc<DynRulesContainer>)
     }
 }
+
+/// A minimal empty rules container used for empty snapshots and tests.
+#[derive(Debug)]
+struct EmptyRules;
+
+impl crate::rules::subscriber_snapshot::RulesContainer for EmptyRules {
+    type Rule = dyn crate::rules::subscriber_snapshot::RuleEvents;
+    fn iter_rules<'a>(&'a self) -> Box<dyn Iterator<Item = &'a Self::Rule> + 'a> {
+        Box::new(std::iter::empty())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use rustfs_s3_types::EventName;
+
+    fn snapshot_with_mask(mask: u64) -> BucketRulesSnapshot<DynRulesContainer> {
+        BucketRulesSnapshot {
+            event_mask: mask,
+            rules: Arc::new(EmptyRules) as Arc<DynRulesContainer>,
+        }
+    }
+
+    #[test]
+    fn store_then_load_roundtrips_snapshot() {
+        let index = SubscriberIndex::default();
+        assert!(!index.has_subscriber("bucket", &EventName::ObjectCreatedPut));
+
+        index.store_snapshot("bucket", snapshot_with_mask(EventName::ObjectCreatedPut.mask()));
+        assert!(index.has_subscriber("bucket", &EventName::ObjectCreatedPut));
+
+        index.clear_bucket("bucket");
+        assert!(!index.has_subscriber("bucket", &EventName::ObjectCreatedPut));
+    }
+
+    /// Regression test for backlog#984 (subscriber_index TOCTOU): many tasks
+    /// concurrently perform the first write for the *same* new bucket. The old
+    /// `get()`-then-`insert()` path could have concurrent first-writers each build
+    /// a distinct cell and clobber one another in the map, orphaning a stored
+    /// snapshot. With the atomic `compute_if_absent` upsert every writer shares the
+    /// one winning cell, so the final snapshot is always a real, non-empty store —
+    /// never the discarded empty default.
+    #[tokio::test(flavor = "multi_thread", worker_threads = 8)]
+    async fn concurrent_first_writes_share_one_cell() {
+        let mask = EventName::ObjectCreatedPut.mask();
+        for _round in 0..64 {
+            let index = Arc::new(SubscriberIndex::default());
+            const WRITERS: usize = 16;
+
+            let mut handles = Vec::with_capacity(WRITERS);
+            for _ in 0..WRITERS {
+                let index = index.clone();
+                handles.push(tokio::spawn(async move {
+                    index.store_snapshot("shared-bucket", snapshot_with_mask(mask));
+                }));
+            }
+            for handle in handles {
+                handle.await.expect("writer task must not panic");
+            }
+
+            assert!(
+                index.has_subscriber("shared-bucket", &EventName::ObjectCreatedPut),
+                "a concurrently-stored snapshot must survive; none was lost to a clobbered cell"
+            );
+        }
+    }
+}

crates/notify/src/rules/xml_config.rs

@@ -70,13 +70,18 @@ impl FilterRule {
             return Err(ParseConfigError::InvalidFilterName(self.name.clone()));
         }
         // ValidateFilterRuleValue from Go:
-        // no "." or ".." path segments, <= 1024 chars, valid UTF-8, no '\'.
+        // no "." or ".." path segments, <= 1024 characters, no '\'.
         for segment in self.value.split('/') {
             if segment == "." || segment == ".." {
                 return Err(ParseConfigError::InvalidFilterValue(self.value.clone()));
             }
         }
-        if self.value.len() > 1024 || self.value.contains('\\') || std::str::from_utf8(self.value.as_bytes()).is_err() {
+        // The limit is 1024 *characters* (runes), matching S3/Go semantics. Using the
+        // byte length (`str::len`) would wrongly reject valid keys whose multi-byte
+        // UTF-8 encoding exceeds 1024 bytes while staying under 1024 characters
+        // (backlog#984). `self.value` is a `String`, so it is already valid UTF-8 —
+        // no separate UTF-8 check is needed.
+        if self.value.chars().count() > 1024 || self.value.contains('\\') {
             return Err(ParseConfigError::InvalidFilterValue(self.value.clone()));
         }
         Ok(())
@@ -374,3 +379,43 @@ impl NotificationConfiguration {
         // You may also need to set the default value here. But according to the current definition, they only contain ARN strings.
     }
 }
+
+#[cfg(test)]
+mod filter_rule_length_tests {
+    use super::{FilterRule, ParseConfigError};
+
+    fn prefix(value: String) -> FilterRule {
+        FilterRule {
+            name: "prefix".to_string(),
+            value,
+        }
+    }
+
+    #[test]
+    fn length_limit_counts_characters_not_bytes() {
+        // 1024 multi-byte characters: 3072 bytes but exactly 1024 chars — must pass.
+        // The old byte-length check (`str::len`) would have rejected this valid key
+        // (backlog#984).
+        let multibyte = "中".repeat(1024);
+        assert_eq!(multibyte.chars().count(), 1024);
+        assert!(multibyte.len() > 1024, "test setup: byte length must exceed the char limit");
+        prefix(multibyte).validate().expect("1024-character value must be accepted");
+
+        // 1025 characters must be rejected.
+        let too_long = "a".repeat(1025);
+        assert!(matches!(prefix(too_long).validate(), Err(ParseConfigError::InvalidFilterValue(_))));
+
+        // Exactly 1024 ASCII characters must pass.
+        prefix("a".repeat(1024))
+            .validate()
+            .expect("1024 ASCII chars must be accepted");
+    }
+
+    #[test]
+    fn backslash_is_still_rejected() {
+        assert!(matches!(
+            prefix("bad\\path".to_string()).validate(),
+            Err(ParseConfigError::InvalidFilterValue(_))
+        ));
+    }
+}