[toffee-desuwa]

Description

The Python HTTP metadata server (mooncake-wheel/mooncake/http_metadata_server.py) started its aiohttp server on a background daemon thread, then unconditionally logged "started" and slept forever without waiting for the bind to succeed. If the port was already in use, the background thread failed the bind (logging the error and setting internal state to Failed), but the main thread had already reported success and kept the process alive. A supervisor then sees a live process plus a success log while nothing is listening — a silent failure that misleads health checks and deployment.

This adds a startup handshake: run() waits on a concurrent.futures.Future that the server thread completes only after TCPSite.start() succeeds (ready) or fails (exception). "started" is logged only on success; on bind failure the exception is propagated to run() and re-raised, so the CLI exits non-zero instead of masking the failure.

This is a source-inspection find with no existing issue. Note: it touches the same file as #2527 but a different region — this changes the startup path around run()/_run_server, while #2527 changes _handle_put idempotency. The two do not overlap.

The added lines pass ruff and codespell. The file predates the repo's ruff-format hook, so the diff keeps the surrounding style unchanged rather than adding an unrelated whole-file reformat.

Module

• Python Wheel (mooncake-wheel)

Type of Change

• Bug fix

How Has This Been Tested?

Test commands:

python -m pytest mooncake-wheel/tests/test_http_metadata_server_startup.py

Test results:

• Unit tests pass (3/3)
• Integration tests pass (if applicable)
• Manual testing done (describe below)

New regression tests: occupying a loopback port asserts run() raises and logs no success message; binding to port 0 asserts normal startup and clean shutdown. A separate test confirms a runtime exception raised during startup reaches run() without being swallowed. Verified on Windows 11 (WinError 10048); the logic is platform-independent (Linux Errno 98).

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh
• I have run pre-commit run --all-files and all hooks pass
• I have updated the documentation (if applicable)
• I have added tests to prove my changes are effective
• For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

• AI tools were used (specify below)

The implementation was AI-assisted and independently reviewed across multiple models before submission. The diagnosis and fix were verified against the source and the regression tests were run locally (results above).

[gemini-code-assist]

Code Review

This pull request improves the startup synchronization of the HTTP metadata server by utilizing a concurrent.futures.Future to block the calling thread until the background server thread has fully initialized or failed. It also introduces comprehensive unit tests to validate server startup, bind failures, and exception propagation. The feedback suggests adding a timeout to startup.result() in the test suite to prevent potential hangs during CI runs.

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]

To prevent the test suite from hanging indefinitely in CI if the test fails or the background task gets stuck, it is safer to specify a timeout when calling startup.result().

Suggested change

	self.assertIsNone(startup.result())
	self.assertIsNone(startup.result(timeout=5))

[codecov-commenter]

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

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[stmatengss]

The Python-based HTTP metadata server is rarely used. Does the built-in C++ metadata server have the same issue?

[toffee-desuwa]

Yeah, the C++ one has the same issue. async_start() does the bind synchronously and hands back a Future<error_code> that's already resolved if it failed, but HttpMetadataServer::start() drops that future and sets running_ = true / returns true regardless, so an in-use port still reports a successful start. The other HTTP callers like MasterAdminServer::Start and RealClient::start_http_server check ec.hasResult() right after async_start(); the metadata server just doesn't.

That said, #2345 already seems to be fixing that exact method, so I'd rather not duplicate it — happy to keep this PR to the Python server and leave the C++ side to #2345, unless you'd prefer I fold it in here.

[he-yufeng]

Approving the Python side. The Future-based handshake is the right fix, and I walked the paths to convince myself it is tight: "started" is logged strictly after site.start() returns, a failed bind is re-raised to the caller instead of dying silently on the daemon thread, the except BaseException re-raises non-Exception types so KeyboardInterrupt/SystemExit keep their identity, and the finally + if not startup.done() guard both prevents a run() hang and avoids a double set_exception. The three tests cover the meaningful cases (bind-failure propagation with no false "started" log, ready server + clean shutdown, and a BaseException after readiness not being swallowed).

One non-blocking note: startup.result() has no timeout, so the only startup path this does not fail-fast on is a site.start() that hangs rather than raises. That is rare and not a regression, but a bounded timeout would close the last gap if you think it is worth it.

On @stmatengss's question about the C++ server and @toffee-desuwa's answer, I confirmed it. HttpMetadataServer::start() (mooncake-store/src/http_metadata_server.cpp:110-118) calls server_->async_start(), drops the returned Future<error_code>, then unconditionally sets running_ = true, logs "started", and returns true. poll() then reports KVPoll::Success off running_, so a failed bind is invisible on the C++ path exactly like it was on the Python one. Worth tracking so the two servers behave the same. Happy to send a follow-up that awaits the async_start() future and surfaces the error, or leave it to you if you would rather keep it in this line of work.

[he-yufeng]

Opened #2942 for the C++ side we discussed — it mirrors MasterAdminServer::Start()'s async_start().hasResult() check for the same server type, with a bind-failure regression test.

[toffee-desuwa]

Thanks for the careful review, and for taking the C++ side in #2942 — that fully covers @stmatengss's question: this PR stays Python-only, and the C++ path gets the same treatment over there.
On the timeout note: agreed it's not a regression (a hanging site.start() would hang today too), but happy to add a bounded startup.result(timeout=...) if you'd want it in before merge.

[toffee-desuwa]

@stmatengss the C++ side is merged now (#2942) — anything else needed on this one?