Cargo.toml (1)

76-76: Fix the typo in this comment.

"Depenedency" should be spelled "Dependency" to keep the note clear.

-socket2 = "0.4" # Depenedency of tokio-sctp
+socket2 = "0.4" # Dependency of tokio-sctp

lightning-nf/omnipath/app/src/nas_old/ue_actions.rs (1)

1-1: Drop the unused UeContext import

Line 1 pulls in UeContext, but every reference remains commented out, so the compiler emits an unused-import warning. Unless we plan to resurrect that block immediately, please remove the import (or add an allow) to keep the module warning-free.

lightning-nf/omnipath/app/src/context/ue_context.rs (1)

17-19: Restrict state and nas_context visibility to avoid unsound external mutation

Both fields are public, enabling mutation outside the scheduling/serialization guarantees of the context manager. Make them private or pub(crate) and expose intentional accessors/methods.

 pub struct UeContext {
   pub ran_ue_ngap_id: RanUeNgapId,
   pub amf_ue_ngap_id: AmfUeNgapId,
   pub gnb_context: Arc<GnbContext>,
-  pub state: AtomicGmmState,
-  pub nas_context: NasContext,
+  pub(crate) state: AtomicGmmState,
+  pub(crate) nas_context: NasContext,
 }

lightning-nf/omnipath/app/src/nas_old/mod.rs (2)

1-2: Remove unused Arc import

Arc is not used in this module.

-use std::{future::Future, sync::Arc};
+use std::future::Future;

---

4-4: Explicitly import the intended NasContext
Change the import to reference the canonical type and avoid shadowing:

-use nas_context::NasContext;
+use crate::context::NasContext;

Confirm all usages in nas_old/mod.rs now refer to crate::context::NasContext, not the local nas_old::nas_context::NasContext.

lightning-nf/omnipath/app/src/ngap/core/initial_ue_message.rs (2)

5-5: Drop unused RwLock imports

OwnedRwLockWriteGuard and RwLock are no longer used after switching to SeqLock.

-use tokio::sync::{OwnedRwLockWriteGuard, RwLock};
+// (imports removed)

---

30-51: Existing‑context path returns an NGAP failure by design — confirm this matches expected behavior

Previously, implementations often schedule onto the existing context. If failing fast here is intentional, confirm AMF behavior/spec alignment for re‑entrant InitialUE when registration is in progress.

lightning-nf/omnipath/app/src/context/state/gmm_state.rs (2)

7-8: Derive Copy/Clone for ergonomic value semantics.

These states are small repr(u8) values and frequently copied; deriving Copy, Clone reduces churn and borrow issues.

Apply:

-#[derive(Eq, PartialEq, Hash)]
+#[derive(Copy, Clone, Eq, PartialEq, Hash)]

---

94-107: next_state for Registered: clarify or encode the common-procedure hop.

Comment says “Final state or can go to CommonProcedureInitiated” but next_state returns None. Consider returning Some(CommonProcedureInitiated) or clarifying that next_state encodes only the registration linear flow.

lightning-nf/omnipath/app/src/nas/registration_req/mod.rs (2)

35-37: Emit structured log for NoIdentity.

Convert TODO into a warning so operators can see malformed/empty IDs.

Apply:

-            MobileIdentity::NoIdentity(_no_identity) => {
-                // Todo push some logging here
-            }
+            MobileIdentity::NoIdentity(_no_identity) => {
+                tracing::warn!("NAS RegistrationRequest without a mobile identity");
+            }

---

26-63: Persist UE security capability from the request.

Prior handler logic populated ue_security_capability or errored; this impl omits it. Persisting this is necessary for subsequent security mode decisions.

Apply near the end of the function (before Ok(...)):

+        if let Some(sec) = &req.nas_ue_security_capability {
+            self.nas_context.ue_security_capability = Some(sec.clone());
+        } else {
+            tracing::warn!("Missing UE security capability in RegistrationRequest");
+        }

lightning-nf/omnipath/app/src/ngap/manager/context_queue.rs (3)

17-26: Docs mention RwLock—update to SeqLock.

The module docs still reference RwLock; adjust to reflect the SeqLock‑based design to prevent confusion.

---

202-209: Avoid unwrap on oneshot receive.

A drop/panic in the worker will panic callers. Return Result or map the error.

Apply:

-        rx.await.unwrap()
+        rx.await.expect("ContextQueue: operation task dropped or panicked")

Or better, change the API to return Result<O, oneshot::error::RecvError>.

---

136-140: Unnecessary Sync bound on FnOnce closures.

Requiring Sync for FnOnce is superfluous and restricts callers; remove it unless there’s a concrete need.

Apply:

-            + Send
-            + Sync
+            + Send

Do the same in schedule_and_wait.

Also applies to: 192-196

utils/client/src/nf_clients/oauth_service.rs (3)

19-23: Use Arc<[ServiceName]> instead of Vec to avoid per-call clones and shrink captures

Replace Vec with Arc<[ServiceName]> in the service and layer; accept Into<Arc<[ServiceName]>> in constructors to keep call sites ergonomic. This removes an allocation and makes .clone() cheap in call().

@@
-pub struct OAuthTokenService<S, const TARGET_NF_TYPE: NfType> {
+pub struct OAuthTokenService<S, const TARGET_NF_TYPE: NfType> {
     inner: S,
     nrf_client: Arc<NrfClient>,
-    service_names: Vec<ServiceName>,
+    service_names: Arc<[ServiceName]>,
 }
@@
-    pub fn new(
+    pub fn new(
         inner: S,
         nrf_client: Arc<NrfClient>,
-        service_names: Vec<ServiceName>,
+        service_names: impl Into<Arc<[ServiceName]>>,
     ) -> Self {
         Self {
             inner,
             nrf_client,
-            service_names,
+            service_names: service_names.into(),
         }
     }
@@
-    pub fn service_names(&self) -> &[ServiceName] {
+    pub fn service_names(&self) -> &[ServiceName] {
         &self.service_names
     }
@@
-pub struct OAuthTokenLayer<const TARGET_NF_TYPE: NfType> {
+pub struct OAuthTokenLayer<const TARGET_NF_TYPE: NfType> {
     nrf_client: Arc<NrfClient>,
-    service_names: Vec<ServiceName>,
+    service_names: Arc<[ServiceName]>,
 }
@@
-    pub fn new(
+    pub fn new(
         nrf_client: Arc<NrfClient>,
-        service_names: Vec<ServiceName>,
+        service_names: impl Into<Arc<[ServiceName]>>,
     ) -> Self {
         Self {
             nrf_client,
-            service_names,
+            service_names: service_names.into(),
         }
     }
@@
-    pub fn with_service(
+    pub fn with_service(
         nrf_client: Arc<NrfClient>,
         service_name: ServiceName,
     ) -> Self {
         Self {
             nrf_client,
-            service_names: vec![service_name],
+            service_names: vec![service_name].into(),
         }
     }
@@
-    fn layer(
+    fn layer(
         &self,
         inner: S,
     ) -> Self::Service {
-        OAuthTokenService::new(inner, self.nrf_client.clone(), self.service_names.clone())
+        OAuthTokenService::new(inner, self.nrf_client.clone(), self.service_names.clone())
     }

Also applies to: 30-37, 55-57, 112-114, 119-137, 150-159, 89-92

---

150-159: Minor: make the layer self-sufficient wrt sensitive headers

Today, SetSensitiveRequestHeadersLayer is applied by the caller (see utils/client/src/nf_clients/mod.rs). Consider documenting this requirement here, or optionally adding a tiny layer wrapper that always marks AUTHORIZATION as sensitive to prevent accidental leakage in other stacks.

---

175-180: Unit test is a stub; provide a real test via a mock NrfClient

Add a lightweight NrfClient test-double that sets a fixed Authorization header so we can assert header injection.

I can scaffold a minimal trait/adapter around set_auth_token for easy mocking and add a test if helpful.

utils/atomic-handle/src/lib.rs (2)

1-5: Prefer NonNull over raw pointer; derive Clone; tighten debug load ordering

Using NonNull avoids accidental nulls and makes unsafe blocks smaller; deriving Clone simplifies code; Relaxed is enough for debug printing.

@@
-use std::{
-    marker::PhantomData, ops::Deref, sync::{
-        atomic::{AtomicUsize, Ordering}, Arc
-    }
-};
+use std::{
+    marker::PhantomData,
+    ops::Deref,
+    ptr::NonNull,
+    sync::{atomic::{AtomicUsize, Ordering}, Arc},
+};
@@
-pub struct AtomicHandle<S, Field = ()>
+#[derive(Clone)]
+pub struct AtomicHandle<S, Field = ()>
 where
     S: AtomicOperation<Field> + Send + Sync ,
 {
     /// Keeps the original Arc<S> alive to prevent memory deallocation
     _owner: Arc<S>,
     /// Raw pointer to the specific atomic field within the struct
-    atomic_ptr: *const AtomicUsize,
+    atomic_ptr: NonNull<AtomicUsize>,
     /// Zero-sized marker to track which field this handle represents
     _phantom: PhantomData<Field>,
 }
@@
-    pub fn new(owner: Arc<S>) -> Self {
-        let atomic_ptr = owner.get_atomic() as *const AtomicUsize;
+    pub fn new(owner: Arc<S>) -> Self {
+        let atomic_ptr = NonNull::from(owner.get_atomic());
         Self {
             _owner: owner,
             atomic_ptr,
             _phantom: PhantomData,
         }
     }
@@
-    pub fn get(&self) -> &AtomicUsize {
-        unsafe { &*self.atomic_ptr }
+    pub fn get(&self) -> &AtomicUsize {
+        // Safety: NonNull points at a field inside `_owner`, whose lifetime we hold via Arc.
+        unsafe { self.atomic_ptr.as_ref() }
     }
@@
-// Manual implementation of Clone.
-impl<S, Field> Clone for AtomicHandle<S, Field>
-where
-    S: AtomicOperation<Field> + Send + Sync,
-{
-    fn clone(&self) -> Self {
-        Self {
-            _owner: Arc::clone(&self._owner),
-            atomic_ptr: self.atomic_ptr,
-            _phantom: PhantomData,
-        }
-    }
-}
+// (Clone derived above)
@@
         f.debug_struct("AtomicHandle")
-            .field("current_value", &self.load(Ordering::SeqCst))
+            .field("current_value", &self.load(Ordering::Relaxed))
             .field("field_type", &std::any::type_name::<Field>())
             .field("struct_type", &std::any::type_name::<S>())
             .finish()
     }

Also applies to: 32-42, 95-103, 108-111, 114-123, 125-137

---

8-15: Document safety invariants for implementors of AtomicOperation

Callers must ensure the returned &AtomicUsize has a stable address for the lifetime of Arc and is not inside buffers that may reallocate/move. Add explicit safety notes to prevent unsound impls.

@@
 /// The generic parameter `Field` allows the same type to implement
 /// this trait multiple times for different atomic fields.
 ///
 pub trait AtomicOperation<Field = ()> {
-    /// Returns a reference to the atomic field identified by the Field type
-    /// parameter
+    /// Returns a reference to the atomic field identified by the `Field` type parameter.
+    ///
+    /// Safety contract:
+    /// - The returned reference must point to a field whose memory address remains
+    ///   stable for as long as any `Arc<S>` clones exist (e.g., the field is inside `S`,
+    ///   not inside a relocatable buffer).
+    /// - It must not be freed while any `AtomicHandle<S, Field>` derived from the same
+    ///   owner `Arc<S>` is alive.
     fn get_atomic(&self) -> &AtomicUsize;
 }

lightning-nf/omnipath/app/src/nas/interface.rs (2)

17-21: Avoid logging full request Debug in error message (PII risk)

Printing the entire request on invalid transitions may leak identifiers. Remove it from Display; keep fields on the struct for programmatic access.

-#[error("Invalid state transition from {state:?} with request: {request:?}")]
+#[error("Invalid state transition from {state:?}")]
 pub struct InvalidStateTransition<R: Debug> {

---

212-226: Test covers spawn bounds; expand with a case that mutates self

Add a handler that updates internal state inside an async block to ensure the relaxed (no Sync) bounds compile in real scenarios.

utils/client/src/nf_clients/mod.rs (1)

162-171: Remove unused generics from request signature (H, Q, B).

They’re unused and create noise. Keep only Resp.

-pub async fn request<H, Q, B, Resp>(
+pub async fn request<Resp>(
 		&self,
 		req: HttpRequest<Body>,
 	) -> Result<(StatusCode, Resp), GenericClientError>
-	where
-		Q: Serialize,
-		H: Serialize,
-		B: Serialize,
-		Resp: DeserResponse,
+	where
+		Resp: DeserResponse,

utils/client/src/nf_clients/service_discovery/mod.rs (3)

84-93: Guard against negative validity periods to avoid cast pitfalls.

Casting negative i64 to u64 can mis-evaluate freshness. Treat non-positive as invalid.

 	pub fn is_valid(&self) -> bool {
 		if let Some(validity_seconds) = self.validity_period {
-			SystemTime::now()
+			if validity_seconds <= 0 {
+				return false;
+			}
+			SystemTime::now()
 				.duration_since(self.cached_at)
 				.unwrap_or(Duration::MAX)
-				.as_secs() < validity_seconds as u64
+				.as_secs() < validity_seconds as u64
 		} else {
 			self.nf_profile.is_some() // Valid if we have a profile and no expiry
 		}
 	}

---

95-99: Prefer service-specific URL selection using configured service_names.

get_base_url() picks an arbitrary URL from a HashMap. If multiple NF services exist, use service_names priority order to choose a base URL deterministically.

Minimal change approach:

• Pass &[ServiceName] into update_request_url.
• Choose the first matching service URL; fallback to get_base_url().

-	fn update_request_url(
-		request: &mut HttpRequest<Body>,
-		cached_profile: &CachedNfProfile,
-	) -> Result<(), ServiceDiscoveryError> {
-		match cached_profile.get_base_url() {
+	fn update_request_url(
+		request: &mut HttpRequest<Body>,
+		cached_profile: &CachedNfProfile,
+		service_names: &[ServiceName],
+	) -> Result<(), ServiceDiscoveryError> {
+		let preferred = service_names
+			.iter()
+			.find_map(|n| cached_profile.get_first_service_url(n));
+		match preferred.or_else(|| cached_profile.get_base_url()) {
 			Some(base_url) => {

And call site:

-				match Self::update_request_url(&mut request, &cached_profile) {
+				match Self::update_request_url(&mut request, &cached_profile, &self.layer.service_names) {

Also applies to: 347-373, 377-397

---

200-218: Cache reset on discovery failure is fine; consider exponential backoff later.

Current strategy invalidates cache to retry on next request. Looks good for now.