Rewrite streamer cpp mock by omer-dayan · Pull Request #114 · run-ai/runai-model-streamer

omer-dayan · 2025-12-16T12:03:03Z

Rewrite the streamer cpp mock to improve its functionality

greptile-apps · 2025-12-16T12:06:51Z

Greptile Summary

Rewrote the streamer C++ mock to address all previously identified issues. The implementation now uses per-instance state via MockStreamerState instead of global static variables, properly integrates the shared common::ResponseCode enum and common::description() function, and includes comprehensive error handling with input validation.

Key improvements:

Replaced global state (g_state, __multi_state) with per-instance MockStreamerState allocated in runai_start() and deallocated in runai_end()
Removed duplicated ResponseCode enum and RESPONSE_MESSAGES array, now using common::ResponseCode and common::description()
Added FileReadState struct with move semantics to properly manage file handles and read state
Implemented proper error cleanup that consistently resets has_active_request flag
Added validation for null pointers, empty requests, and chunk size consistency
Enhanced error messages with context (file paths, byte counts)

Minor issue:

Missing newline at end of file (style issue only)

Confidence Score: 4/5

This PR is safe to merge after adding a newline at the end of the file
All critical issues from previous review have been addressed: per-instance state eliminates concurrency concerns, shared ResponseCode eliminates maintenance burden, and error handling is consistent. The only remaining issue is a minor style violation (missing newline at EOF)
No files require special attention - the implementation properly addresses all previous concerns

Important Files Changed

Filename	Overview
cpp/mock/streamer-mock.cc	Complete rewrite with per-instance state, proper error handling, and shared `ResponseCode` usage - missing final newline

Sequence Diagram

sequenceDiagram
    participant Client
    participant Mock as streamer-mock
    participant State as MockStreamerState
    participant FileState as FileReadState
    participant FS as File System

    Client->>Mock: runai_start(&streamer)
    Mock->>State: new MockStreamerState()
    State-->>Mock: state instance
    Mock-->>Client: Success

    Client->>Mock: runai_request(paths, offsets, sizes, ...)
    Mock->>State: Check has_active_request
    State-->>Mock: false
    Mock->>State: Set has_active_request = true
    
    loop For each file
        Mock->>FileState: initialize_file_read()
        FileState->>FS: open(path)
        FS-->>FileState: file descriptor
        FileState->>FS: seek(offset)
        FileState->>FileState: Store chunk_sizes, destination
        FileState-->>Mock: Success
        Mock->>State: file_states.push_back(file_state)
    end
    
    Mock-->>Client: Success

    loop Until all chunks read
        Client->>Mock: runai_response(&file_index, &index)
        Mock->>State: Get current file_state
        
        alt File not complete
            Mock->>FileState: read_chunk(&chunk_index)
            FileState->>FS: read(chunk_size)
            FS-->>FileState: data
            FileState->>FileState: Update destination_offset, current_chunk_index
            FileState-->>Mock: Success + chunk_index
            Mock-->>Client: Success, file_index, chunk_index
        else File complete
            Mock->>State: current_file_index++
            Mock->>Mock: Try next file (recurse)
        else All files complete
            Mock->>State: Set has_active_request = false
            Mock-->>Client: FinishedError
        end
    end

    Client->>Mock: runai_end(streamer)
    Mock->>State: delete state
    Mock-->>Client: void

greptile-apps

_{1 file reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-12-16T12:06:49Z

cpp/mock/streamer-mock.cc

+enum class ResponseCode : int
+{
+    Success = 0,
+    FinishedError,
+    FileAccessError,
+    EofError,
+    S3NotSupported,
+    GlibcPrerequisite,
+    InsufficientFdLimit,
+    InvalidParameterError,
+    EmptyRequestError,
+    BusyError,
+    CaFileNotFound,
+    UnknownError,
+    ObjPluginLoadError,
+    GCSNotSupported,
+    __Max,


logic: ResponseCode enum duplicated in wrong namespace - should use common::ResponseCode from cpp/common/response_code/response_code.h

The real implementation has ResponseCode in runai::llm::streamer::common namespace, not runai::llm::streamer. This duplication creates maintenance issues.

Suggested change

enum class ResponseCode : int

{

Success = 0,

FinishedError,

FileAccessError,

EofError,

S3NotSupported,

GlibcPrerequisite,

InsufficientFdLimit,

InvalidParameterError,

EmptyRequestError,

BusyError,

CaFileNotFound,

UnknownError,

ObjPluginLoadError,

GCSNotSupported,

__Max,

// Use the common ResponseCode enum from the real implementation

#include "common/response_code/response_code.h"

using ResponseCode = common::ResponseCode;

greptile-apps · 2025-12-16T12:06:49Z

cpp/mock/streamer-mock.cc

+constexpr const char* RESPONSE_MESSAGES[] = {
+    "Request sent successfuly",
+    "Finished all responses",
+    "File access error",
+    "End of file reached",
+    "S3 not supported",
+    "GLIBC version should be at least 2.29",
+    "Increase process fd limit or decrease the concurrency level. Recommended value for the streamer alone is the concurrency multiplied by 64, in addition to your application fd usage",
+    "Invalid request parameters",
+    "Empty request parameters",
+    "Streamer is handling previous request",
+    "CA bundle file not found",
+    "Unknown Error",
+    "Error loading object storage plugin",
+    "GCS not supported",
+};


logic: RESPONSE_MESSAGES array duplicated - should use common::description() function

The real implementation provides common::description(int) function. Duplicating the messages creates maintenance burden.

Suggested change

constexpr const char* RESPONSE_MESSAGES[] = {

"Request sent successfuly",

"Finished all responses",

"File access error",

"End of file reached",

"S3 not supported",

"GLIBC version should be at least 2.29",

"Increase process fd limit or decrease the concurrency level. Recommended value for the streamer alone is the concurrency multiplied by 64, in addition to your application fd usage",

"Invalid request parameters",

"Empty request parameters",

"Streamer is handling previous request",

"CA bundle file not found",

"Unknown Error",

"Error loading object storage plugin",

"GCS not supported",

};

// Remove this array and use common::description() instead in get_response_message()

greptile-apps · 2025-12-16T12:06:50Z

cpp/mock/streamer-mock.cc

+const char* get_response_message(int response_code)
+{
+    if (response_code < 0 || response_code >= static_cast<int>(ResponseCode::__Max))
+    {
+        return "Invalid response code";
+    }
+    return RESPONSE_MESSAGES[response_code];
+}


logic: get_response_message() duplicates functionality that already exists

Use common::description(int) instead.

Suggested change

const char* get_response_message(int response_code)

{

if (response_code < 0 || response_code >= static_cast<int>(ResponseCode::__Max))

{

return "Invalid response code";

}

return RESPONSE_MESSAGES[response_code];

}

// Remove this function and use common::description() directly in runai_response_str()

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

greptile-apps · 2025-12-16T12:06:50Z

cpp/mock/streamer-mock.cc

+const char* runai_response_str(int response_code)
 {
-    return 0;
+    using namespace runai::llm::streamer;
+    return get_response_message(response_code);
 }


logic: Use common::description() instead of get_response_message()

Suggested change

const char* runai_response_str(int response_code)

{

return 0;

using namespace runai::llm::streamer;

return get_response_message(response_code);

}

const char* runai_response_str(int response_code)

{

return common::description(response_code);

}

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

greptile-apps

_{7 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-12-18T08:56:53Z

cpp/mock/streamer-mock.cc

-    state->current_item++;
-    return 0;
+    // Open file
+    state.file = utils::Fd(::open(path, O_RDONLY));


style: Direct use of ::open without checking errno on failure means error messages may be misleading

Suggested change

state.file = utils::Fd(::open(path, O_RDONLY));

int fd = ::open(path, O_RDONLY);

if (fd == -1)

{

LOG(ERROR) << "Failed to open file: " << path << " (errno: " << errno << ")";

return static_cast<int>(common::ResponseCode::FileAccessError);

}

state.file = utils::Fd(fd);

greptile-apps · 2025-12-18T08:56:54Z

cpp/mock/streamer-mock.cc

+};

+// Global state instance
+static MockStreamerState g_state;


logic: Static global state (g_state) means multiple streamer instances share state, violating isolation

The real implementation uses per-instance state via the streamer pointer, but this mock uses a global singleton. If multiple tests or callers use separate streamer instances concurrently, they'll interfere with each other. Is concurrent usage with multiple streamer instances expected in tests?

greptile-apps · 2025-12-18T08:56:54Z

cpp/mock/streamer-mock.cc

+            // Clean up on error
+            g_state.file_states.clear();
+            g_state.has_active_request = false;


logic: Error cleanup doesn't reset has_active_request consistently

If initialize_file_read fails after the first file succeeds, has_active_request is set to false, but if it fails on the first file, the flag remains true from line 276.

greptile-apps · 2025-12-18T08:56:55Z

py/runai_model_streamer/runai_model_streamer/file_streamer/file_streamer.py

+
+        for i in range(expected_chunks):
            file_relative_index, chunk_relative_index = runai_response(self.streamer)
            if chunk_relative_index == None:


style: Explained behavior when None is returned - happens when FINISHED_ERROR_CODE is returned by C++ (see libstreamer.py:92-93), indicating all chunks complete or error occurred

greptile-apps

_{5 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-12-18T11:31:24Z

cpp/mock/streamer-mock.cc

 }
-} // namespace runai::llm::streamer
+
+} // extern "C"


style: Missing newline at end of file

Suggested change

} // extern "C"

} // extern "C"

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-12-18T12:18:06Z

cpp/mock/streamer-mock.cc

 }
-} // namespace runai::llm::streamer
+
+} // extern "C"


style: Missing newline at end of file

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Rewrite streamer cpp mock

e5a74d7

greptile-apps bot reviewed Dec 16, 2025

View reviewed changes

Fix: Use common ResponseCode and description for streamer mock

3ec957d

omer-dayan force-pushed the testing-cpp-mock branch from a2cd5d7 to 3ec957d Compare December 18, 2025 07:30

Fix: Add missing common/response_code dependency to BUILD files

4bba84d

greptile-apps bot reviewed Dec 18, 2025

View reviewed changes

omer-dayan force-pushed the testing-cpp-mock branch from 573df8e to 4bba84d Compare December 18, 2025 09:10

Fix: Refactor streamer mock to use per-instance state

42bc3ad

greptile-apps bot reviewed Dec 18, 2025

View reviewed changes

Revert BUILD file changes

ce61039

greptile-apps bot reviewed Dec 18, 2025

View reviewed changes

-    state.file = utils::Fd(::open(path, O_RDONLY));
+    int fd = ::open(path, O_RDONLY);
+    if (fd == -1)
+    {
+        LOG(ERROR) << "Failed to open file: " << path << " (errno: " << errno << ")";
+        return static_cast<int>(common::ResponseCode::FileAccessError);
+    }
+    state.file = utils::Fd(fd);

Conversation

omer-dayan commented Dec 16, 2025

Uh oh!

greptile-apps bot commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps bot commented Dec 16, 2025 •

edited

Loading