[jfeng18]

Summary

• Fix crash on non-numeric MC_MS_AUTO_DISC (client_service.cpp): get_auto_discover() used unguarded std::stoi, which throws std::invalid_argument on inputs like "true" or "yes". Replaced with strtol + error checking, matching GetConfiguredNumaSocketId pattern in the same file.
• Check async_start() return in HttpMetadataServer::start() (http_metadata_server.cpp): Port-bind failures were silently ignored. Now checks ec.hasResult() and returns false, matching rpc_service.cpp:233 and real_client.cpp:1644 patterns.

Note: The RDMA memory leak fix on MountSegment failure has been deferred to #2364 for a more thorough review.

What's NOT changed

• No behavior change on success paths
• No new dependencies
• No other env var parsing changed

Test plan

• Adversarial workflow review (correctness + adversarial, all issues addressed)
• Review feedback incorporated
• Compilation: CI
• E2E: not verified — requires cluster environment

[gemini-code-assist]

Code Review

This pull request improves robustness by safer parsing of the auto-discovery environment variable, adding memory unregistration cleanup on mount failures, and checking the return status of the HTTP metadata server startup. The review comments correctly identify a compilation error where hasResult() is called on a std::optional instead of has_value(), and suggest using a C++ style const_cast instead of a C-style cast.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

std::optional does not have a hasResult() method in the C++ Standard Library. Using ec.hasResult() will result in a compilation error. Since server_->async_start() returns a std::optional<std::error_code>, you should check if it contains an error using ec.has_value() or simply if (ec).

Suggested change

	if (ec.hasResult()) {
	if (ec.has_value()) {

[jfeng18]

async_start() returns async_simple::Future<coro_rpc::err_code>, not std::optional. hasResult() is a method on that type. This exact pattern is used in 5+ other call sites: master_service_supervisor.cpp:410, rpc_service.cpp:233, real_client.cpp:1644, rpc_communicator.cpp:138, test_server_helpers.h:176.

[gemini-code-assist]

In C++, it is preferred to use C++ style casts instead of C-style casts. Consider using const_cast<void*>(buffer) instead of (void*)buffer to cast away the const qualifier.

Suggested change

	transfer_engine_->unregisterLocalMemory((void*)buffer);
	transfer_engine_->unregisterLocalMemory(const_cast<void*>(buffer));

[jfeng18]

Good point. Changed both sites to const_cast<void*>(buffer) in 6cc538a.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 52.38095% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
mooncake-store/src/client_service.cpp	56.25%	7 Missing ⚠️
mooncake-store/src/http_metadata_server.cpp	40.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[ykwd]

Thanks for this work.

The issue of "RDMA memory leak on MountSegment failure" does indeed exist and is a known problem. However, we do not yet have a good solution for fixing it.

Simply unregistering the memory after an RPC failure may introduce another issue: the operation may have actually succeeded on the master side, but the network could fail while the result is being returned to the client. In that case, the client would unregister the memory, while the master still considers the operation successful, leaving the system in an inconsistent state.

[jfeng18]

Good point — the network-failure-after-master-success scenario is a real concern. The current fix assumes RPC failure = operation failed, which is not always true.

I can remove the unregisterLocalMemory cleanup from this PR to keep it safe. The other two changes in this PR (strtol guard for MC_MS_AUTO_DISC, const_cast) are independent and uncontroversial.

For the RDMA memory leak on MountSegment failure, a proper fix would likely need idempotent mount/retry or a reconciliation mechanism with the master. Happy to defer that to a separate discussion/issue.

[jfeng18]

Gentle ping @ykwd — the earlier test-wheel failure was environmental and CI is now green after a re-run. This PR fixes a crash on non-numeric MC_MS_AUTO_DISC and a silent async_start failure. Would appreciate a follow-up review when convenient. Thanks!

[jfeng18]

Closing this PR — both fixes have been superseded on main:

• The async_start() return check in HttpMetadataServer::start() was fixed by [Store] Surface HTTP metadata server bind failures in start() #2942 with an equivalent implementation.
• The std::stoi crash in get_auto_discover() is already guarded by a try/catch on current main, so the crash no longer exists; the remaining strtol conversion here would be a pure style change.

Thanks @ykwd for the earlier review, and apologies for the noise from yesterday's ping — the overlap with #2942 surfaced during a rebase check today.