Align lookup dispatch error handling

Author: ktsaou

.agents/sow/current/SOW-0021-20260613-netipc-at-scale.md

@@ -1049,6 +1049,14 @@ Tests or equivalent validation:
   - External review found that Go raw APPS_LOOKUP and CGROUPS_LOOKUP dispatch duplicated the protocol dispatch path and missed the protocol layer's `Finish()==0` overflow guard. Fixed `src/go/pkg/netipc/service/raw/apps_lookup.go` and `src/go/pkg/netipc/service/raw/cgroups_lookup.go` to delegate to protocol dispatchers while preserving raw-service `errHandlerFailed` semantics for typed handler failure.
   - Removed the now-unused Go raw-service `lookupMinRequired()` helper after the protocol dispatchers became the single production path for lookup response minimum-size checks.
   - Validation passed: POSIX `cd src/go && go test -count=1 -timeout=300s ./pkg/netipc/service/raw`; POSIX `cd src/go && go test -count=1 -timeout=300s ./pkg/netipc/protocol`; Windows `cd src/go && "/c/Program Files/Go/bin/go.exe" test -count=1 -timeout=300s ./pkg/netipc/service/raw`; Windows focused raw lookup/defaults tests also passed on `win11`.
+- Post-push Rust/Go reviewer fixes:
+  - External review found that Rust raw APPS_LOOKUP and CGROUPS_LOOKUP service dispatch still duplicated protocol dispatch logic, unlike C and Go. Fixed `src/crates/netipc/src/service/raw/apps_lookup.rs` and `src/crates/netipc/src/service/raw/cgroups_lookup.rs` to delegate to the Rust protocol dispatchers.
+  - External review found that Rust raw lookup dispatch returned `HandlerFailed` before checking whether the builder had already recorded `Overflow`. This could convert a recoverable response-size overflow into an internal service failure. Fixed the Rust protocol-to-service mapping and added platform-neutral tests in `src/crates/netipc/src/service/raw_lookup_dispatch_tests.rs` proving builder `Overflow` wins over handler-failed while plain handler false still maps to service `HandlerFailed`.
+  - External review found that C protocol lookup dispatch reports a no-builder-error handler false as `NIPC_ERR_HANDLER_FAILED`, while Go and Rust protocol lookup dispatch reported `BadLayout`. Fixed Go and Rust protocol dispatch to expose `ErrHandlerFailed` / `NipcError::HandlerFailed` for this case while preserving `BadLayout` when the builder itself records a layout error. This is API error parity only; no wire format changes.
+  - External review found that Rust `ensure_lookup_request_capacity()` did not check abort before disconnect/reconnect work. Fixed it to return `Aborted`, disconnect, and mark the client `Broken` before any capacity growth attempt when abort is already requested. Added a platform-neutral Rust test proving no reconnect is attempted in this path.
+  - Validation passed on POSIX: `cd src/go && go test -count=1 -timeout=300s ./pkg/netipc/protocol`; `cd src/go && go test -count=1 -timeout=300s ./pkg/netipc/service/raw`; `cargo test --manifest-path src/crates/netipc/Cargo.toml protocol::lookup -- --nocapture`; `cargo test --manifest-path src/crates/netipc/Cargo.toml lookup_dispatch_tests -- --nocapture`; `cargo test --manifest-path src/crates/netipc/Cargo.toml service::raw:: -- --nocapture`.
+  - Validation passed on Windows `win11`: `cd src/go && "/c/Program Files/Go/bin/go.exe" test -count=1 -timeout=300s ./pkg/netipc/protocol`; `cd src/go && "/c/Program Files/Go/bin/go.exe" test -count=1 -timeout=300s ./pkg/netipc/service/raw`; `PATH=/c/Users/costa/.cargo/bin:$PATH cargo test --manifest-path src/crates/netipc/Cargo.toml protocol::lookup -- --nocapture`; `PATH=/c/Users/costa/.cargo/bin:$PATH cargo test --manifest-path src/crates/netipc/Cargo.toml service::raw:: -- --nocapture`.
+  - Reviewer concern about suffix-reservation still losing partial responses was not reproduced: C POSIX `timeout 900 build-coverage/bin/test_service` passed with `642 passed, 0 failed`; focused POSIX Go `TestLookupLargeResponseSplit` passed; focused POSIX Rust `test_lookup_large_response_split_calls` passed; Windows Rust `test_lookup_large_response_split_calls_windows` passed. The concern was treated as rejected unless a failing reproducer is produced.
 - Note: `ctest` on `$PATH` resolved to a broken local Python wrapper missing the `cmake` module; validation used `/usr/bin/ctest`.
 
 Real-use evidence:
@@ -1066,7 +1074,13 @@ Reviewer findings:
 - External reviewer concern: hidden fixed payload ceilings would contradict initialization-tunable budgets. Reviewed against code and docs. `NIPC_MAX_PAYLOAD_CAP` / `MaxPayloadCap` remains a named default growth ceiling; request/response payload budgets are exposed through initialization config. Server learned-capacity growth now also honors those configured ceilings.
 - External reviewer finding: Go zero-config raw-server growth ceilings were derived after applying `MaxPayloadDefault`, effectively preventing default servers from learning above `1024` bytes. Handled by deriving growth ceilings from the original config and validating default versus explicit ceiling behavior.
 - External reviewer finding: Go raw lookup dispatchers duplicated protocol dispatcher logic and could diverge from protocol overflow handling. Handled by delegating raw APPS_LOOKUP and CGROUPS_LOOKUP dispatch to protocol dispatchers, preserving raw typed-handler failure semantics, and deleting the now-dead local minimum-size helper.
-- One requested external review run timed out before returning a usable final report. The completed reviewer findings above were triaged against the codebase; concrete defects were fixed and validated, while non-blocking style or broader design suggestions remain subject to the existing SOW close gates.
+- External reviewer finding: Rust raw lookup dispatchers duplicated protocol dispatcher logic and could diverge from protocol overflow handling. Handled by delegating raw APPS_LOOKUP and CGROUPS_LOOKUP dispatch to protocol dispatchers.
+- External reviewer finding: Rust raw lookup service dispatch hid builder `Overflow` when the typed handler returned false after a builder failure. Handled by mapping protocol builder errors before service handler-failed behavior and adding tests proving builder `Overflow` remains `DispatchError::Overflow`.
+- External reviewer finding: C, Go, and Rust protocol lookup dispatch returned different errors for handler false with no builder error. Handled by aligning Go and Rust with C's handler-failed protocol error while preserving builder-error propagation.
+- External reviewer finding: Rust lookup request-capacity growth did not check abort before reconnecting. Handled by checking abort at helper entry and validating no reconnect is attempted after abort.
+- External reviewer concern: APPS_LOOKUP/CGROUPS_LOOKUP suffix-reservation could still lose partial progress if compact `PAYLOAD_EXCEEDED` suffix entries fill the response. Rejected after direct validation: C POSIX full service tests, POSIX Go focused large-response split, POSIX Rust focused large-response split, and Windows Rust focused large-response split all pass with stitched complete responses.
+- External reviewer concern: C's service-layer zero-config response payload default is larger than Rust/Go raw defaults. Triaged as documented policy, not a hidden hardcoded protocol cap: Level 1 default/cap values remain named defaults, C service uses a named service response default, and all languages expose request/response payload budgets through initialization config.
+- Two requested external review runs timed out before returning usable final reports, and one run ended before a final report could be retrieved. Completed reviewer findings were triaged against the codebase; concrete defects were fixed and validated, while non-blocking style or broader design suggestions remain subject to the existing SOW close gates.
 - External reviewer finding: coverage scripts and broader adversarial matrix still need updates before SOW completion. Coverage script expansion is now implemented; C/Rust/Go coverage gates pass; representative `8192` and `32768` logical-call tests now pass in C/Rust/Go on POSIX and Windows; POSIX and Windows baseline/SHM lookup-scale interop now pass across all C/Rust/Go directed pairs, including mixed-status lookup cases and heavier `65536` stress-only runs; lookup status codec interop now proves `PAYLOAD_EXCEEDED` and `OVERSIZED_ITEM` wire parity across C/Rust/Go; Rust malformed typed lookup response parity is now covered on POSIX and Windows; malformed follow-up responses after partial progress are now covered in C/Rust/Go on POSIX and Windows; reordered and duplicate response-item corruption is now covered in C/Rust/Go on POSIX and Windows; invalid status enum, invalid status-dependent field, and invalid label-table corruption are now covered in C/Rust/Go on POSIX and Windows; huge valid label isolation is now covered in C/Rust/Go on POSIX and Windows; endpoint absence before call, endpoint disappearance after partial progress, and endpoint disappearance before the first subcall are now covered in C/Rust/Go on POSIX and Windows; zero-item typed lookup calls are now covered in C/Rust/Go on POSIX and Windows; duplicate and unsorted request keys under request splitting are now covered in C/Rust/Go on POSIX and Windows; full POSIX and Windows benchmark regenerations now pass; downstream topology-containers post-vendor validation now passes. Broader adversarial matrix review remains open.
 
 Same-failure scan:

src/crates/netipc/src/protocol/lookup/apps_lookup.rs

@@ -786,7 +786,7 @@ where
         ]);
     }
     if !handler(&request, &mut builder) {
-        return Err(builder.error().unwrap_or(NipcError::BadLayout));
+        return Err(builder.error().unwrap_or(NipcError::HandlerFailed));
     }
     if let Some(err) = builder.error() {
         return Err(err);
@@ -1030,6 +1030,17 @@ mod tests {
         );
     }
 
+    #[test]
+    fn apps_lookup_dispatch_reports_handler_failed() {
+        let mut req = [0u8; 64];
+        let n = encode_apps_lookup_request(&[1234], &mut req).unwrap();
+        let mut resp = vec![0u8; 256];
+        assert_eq!(
+            dispatch_apps_lookup(&req[..n], &mut resp, |_, _| false).unwrap_err(),
+            NipcError::HandlerFailed
+        );
+    }
+
     #[test]
     fn apps_lookup_request_rejects_bad_layouts() {
         let mut buf = [0u8; 128];

src/crates/netipc/src/protocol/lookup/cgroups_lookup.rs

@@ -631,7 +631,7 @@ where
         builder.set_payload_exceeded_item_lens(payload_exceeded_item_lens);
     }
     if !handler(&request, &mut builder) {
-        return Err(builder.error().unwrap_or(NipcError::BadLayout));
+        return Err(builder.error().unwrap_or(NipcError::HandlerFailed));
     }
     if let Some(err) = builder.error() {
         return Err(err);
@@ -783,6 +783,17 @@ mod tests {
         );
     }
 
+    #[test]
+    fn cgroups_lookup_dispatch_reports_handler_failed() {
+        let mut req = [0u8; 64];
+        let n = encode_cgroups_lookup_request(&[b"/x"], &mut req).unwrap();
+        let mut resp = vec![0u8; 256];
+        assert_eq!(
+            dispatch_cgroups_lookup(&req[..n], &mut resp, |_, _| false).unwrap_err(),
+            NipcError::HandlerFailed
+        );
+    }
+
     #[test]
     fn cgroups_lookup_request_rejects_bad_layouts() {
         let mut buf = [0u8; 128];

src/crates/netipc/src/protocol/mod.rs

@@ -104,6 +104,8 @@ pub enum NipcError {
     BadItemCount,
     /// Builder ran out of space.
     Overflow,
+    /// Typed dispatch handler rejected an otherwise valid request.
+    HandlerFailed,
     /// Synchronous call timed out before a complete response arrived.
     Timeout,
     /// Synchronous call was aborted by the caller.
@@ -124,6 +126,7 @@ impl core::fmt::Display for NipcError {
             NipcError::BadAlignment => write!(f, "item not 8-byte aligned"),
             NipcError::BadItemCount => write!(f, "item count inconsistent"),
             NipcError::Overflow => write!(f, "builder out of space"),
+            NipcError::HandlerFailed => write!(f, "dispatch handler failed"),
             NipcError::Timeout => write!(f, "synchronous call timed out"),
             NipcError::Aborted => write!(f, "synchronous call aborted"),
         }

src/crates/netipc/src/protocol/tests.rs

@@ -1311,6 +1311,7 @@ fn nipc_error_display_all_variants() {
         (NipcError::BadAlignment, "item not 8-byte aligned"),
         (NipcError::BadItemCount, "item count inconsistent"),
         (NipcError::Overflow, "builder out of space"),
+        (NipcError::HandlerFailed, "dispatch handler failed"),
     ];
     for (err, expected) in cases {
         let msg = format!("{}", err);

src/crates/netipc/src/service/raw.rs

@@ -70,6 +70,10 @@ use server_session_unix::poll_fd;
 #[path = "raw_unix_tests.rs"]
 mod tests;
 
+#[cfg(test)]
+#[path = "raw_lookup_dispatch_tests.rs"]
+mod lookup_dispatch_tests;
+
 #[cfg(all(test, windows))]
 #[path = "raw_windows_tests.rs"]
 mod windows_tests;

src/crates/netipc/src/service/raw/apps_lookup.rs

@@ -1,11 +1,10 @@
 use super::client::{ClientConfig, RawCallKind, RawClient};
 use super::common::lookup_raw_response_size;
-use super::dispatch::{DispatchError, DispatchHandler};
+use super::dispatch::{dispatch_error_from_protocol, DispatchHandler};
 use crate::protocol::{
     self, AppsLookupBuilder, AppsLookupRequestView, AppsLookupResponseView, NipcError,
-    APPS_LOOKUP_ITEM_HDR_SIZE, APPS_LOOKUP_KEY_SIZE, APPS_LOOKUP_REQ_HDR_SIZE,
-    APPS_LOOKUP_RESP_HDR_SIZE, LOOKUP_DIR_ENTRY_SIZE, METHOD_APPS_LOOKUP,
-    PID_LOOKUP_PAYLOAD_EXCEEDED,
+    APPS_LOOKUP_KEY_SIZE, APPS_LOOKUP_REQ_HDR_SIZE, APPS_LOOKUP_RESP_HDR_SIZE,
+    LOOKUP_DIR_ENTRY_SIZE, METHOD_APPS_LOOKUP, PID_LOOKUP_PAYLOAD_EXCEEDED,
 };
 use std::sync::Arc;
 
@@ -182,39 +181,9 @@ impl RawClient {
 
 pub fn apps_lookup_dispatch(handler: AppsLookupHandler) -> DispatchHandler {
     Arc::new(move |request, response_buf| {
-        let request =
-            AppsLookupRequestView::decode(request).map_err(|_| DispatchError::BadEnvelope)?;
-        let dir_size = (request.item_count as usize)
-            .checked_mul(LOOKUP_DIR_ENTRY_SIZE)
-            .ok_or(DispatchError::Overflow)?;
-        let min_required = protocol::APPS_LOOKUP_RESP_HDR_SIZE
-            .checked_add(dir_size)
-            .ok_or(DispatchError::Overflow)?;
-        if response_buf.len() < min_required {
-            return Err(DispatchError::Overflow);
-        }
-        let mut builder = AppsLookupBuilder::new(response_buf, request.item_count, 0);
-        if request.item_count > 0 {
-            builder.set_payload_exceeded_item_lens(vec![
-                APPS_LOOKUP_ITEM_HDR_SIZE + 3;
-                request.item_count as usize
-            ]);
-        }
-        if !handler(&request, &mut builder) {
-            return Err(DispatchError::HandlerFailed);
-        }
-        if let Some(err) = builder.error() {
-            return match err {
-                NipcError::Overflow => Err(DispatchError::Overflow),
-                _ => Err(DispatchError::BadEnvelope),
-            };
-        }
-        if builder.item_count() != request.item_count {
-            return Err(DispatchError::BadEnvelope);
-        }
-        builder.finish().map_err(|err| match err {
-            NipcError::Overflow => DispatchError::Overflow,
-            _ => DispatchError::BadEnvelope,
+        protocol::dispatch_apps_lookup(request, response_buf, |request, builder| {
+            handler(request, builder)
         })
+        .map_err(dispatch_error_from_protocol)
     })
 }

src/crates/netipc/src/service/raw/cgroups_lookup.rs

@@ -1,6 +1,6 @@
 use super::client::{ClientConfig, RawCallKind, RawClient};
 use super::common::lookup_raw_response_size;
-use super::dispatch::{DispatchError, DispatchHandler};
+use super::dispatch::{dispatch_error_from_protocol, DispatchHandler};
 use crate::protocol::{
     self, CgroupsLookupBuilder, CgroupsLookupRequestView, CgroupsLookupResponseView, NipcError,
     CGROUPS_LOOKUP_ITEM_HDR_SIZE, CGROUPS_LOOKUP_REQ_HDR_SIZE, CGROUPS_LOOKUP_RESP_HDR_SIZE,
@@ -233,45 +233,9 @@ impl RawClient {
 
 pub fn cgroups_lookup_dispatch(handler: CgroupsLookupHandler) -> DispatchHandler {
     Arc::new(move |request, response_buf| {
-        let request =
-            CgroupsLookupRequestView::decode(request).map_err(|_| DispatchError::BadEnvelope)?;
-        let dir_size = (request.item_count as usize)
-            .checked_mul(LOOKUP_DIR_ENTRY_SIZE)
-            .ok_or(DispatchError::Overflow)?;
-        let min_required = protocol::CGROUPS_LOOKUP_RESP_HDR_SIZE
-            .checked_add(dir_size)
-            .ok_or(DispatchError::Overflow)?;
-        if response_buf.len() < min_required {
-            return Err(DispatchError::Overflow);
-        }
-        let mut builder = CgroupsLookupBuilder::new(response_buf, request.item_count, 0);
-        if request.item_count > 0 {
-            let mut payload_exceeded_item_lens = Vec::with_capacity(request.item_count as usize);
-            for i in 0..request.item_count {
-                let item = request.item(i).map_err(|_| DispatchError::BadEnvelope)?;
-                let item_len = CGROUPS_LOOKUP_ITEM_HDR_SIZE
-                    .checked_add(item.as_bytes().len())
-                    .and_then(|v| v.checked_add(2))
-                    .ok_or(DispatchError::Overflow)?;
-                payload_exceeded_item_lens.push(item_len);
-            }
-            builder.set_payload_exceeded_item_lens(payload_exceeded_item_lens);
-        }
-        if !handler(&request, &mut builder) {
-            return Err(DispatchError::HandlerFailed);
-        }
-        if let Some(err) = builder.error() {
-            return match err {
-                NipcError::Overflow => Err(DispatchError::Overflow),
-                _ => Err(DispatchError::BadEnvelope),
-            };
-        }
-        if builder.item_count() != request.item_count {
-            return Err(DispatchError::BadEnvelope);
-        }
-        builder.finish().map_err(|err| match err {
-            NipcError::Overflow => DispatchError::Overflow,
-            _ => DispatchError::BadEnvelope,
+        protocol::dispatch_cgroups_lookup(request, response_buf, |request, builder| {
+            handler(request, builder)
         })
+        .map_err(dispatch_error_from_protocol)
     })
 }

src/crates/netipc/src/service/raw/client.rs

@@ -226,6 +226,12 @@ impl RawClient {
         &mut self,
         required: usize,
     ) -> Result<bool, NipcError> {
+        if self.abort_requested() {
+            self.disconnect();
+            self.state = ClientState::Broken;
+            self.error_count += 1;
+            return Err(NipcError::Aborted);
+        }
         let required = u32::try_from(required).map_err(|_| NipcError::Overflow)?;
         let max_request_payload_bytes = if self.transport_config.max_request_payload_bytes == 0 {
             crate::protocol::MAX_PAYLOAD_DEFAULT

src/crates/netipc/src/service/raw/dispatch.rs

@@ -1,4 +1,5 @@
 use super::common::next_power_of_2_u32;
+use crate::protocol::NipcError;
 use std::sync::atomic::{AtomicU32, Ordering};
 use std::sync::Arc;
 
@@ -12,6 +13,14 @@ pub enum DispatchError {
 pub type DispatchHandler =
     Arc<dyn Fn(&[u8], &mut [u8]) -> Result<usize, DispatchError> + Send + Sync>;
 
+pub(super) fn dispatch_error_from_protocol(err: NipcError) -> DispatchError {
+    match err {
+        NipcError::Overflow => DispatchError::Overflow,
+        NipcError::HandlerFailed => DispatchError::HandlerFailed,
+        _ => DispatchError::BadEnvelope,
+    }
+}
+
 pub(super) fn dispatch_single_internal(
     expected_method_code: u16,
     handler: Option<&DispatchHandler>,