fix(metaserver): preserve active blocks and skip healthy sweep scans - #451
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed redundancy-counter correctness bug during heartbeats (causing metric drift), and the new unauthenticated destructive admin endpoint is exposed on the default 0.0.0.0 HTTP listener.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the MetaServer’s lifecycle management to stop expiring block owners based on registration age during the periodic sweep, and instead provides an explicit operator-triggered cleanup endpoint while maintaining redundancy metrics without full scans.
Changes:
- Reworked
sweep_expiredto remove owners via a node→block reverse index and maintain redundancy counters incrementally. - Added
POST /admin/cleanup-expired-blocksto explicitly remove owners older than 1 hour. - Deprecated
ttl_minutesbehavior in the MetaServer CLI and logs, keeping the flag for compatibility.
File summaries
| File | Description |
|---|---|
| pegaflow-metaserver/src/store.rs | Removes owner-age TTL from periodic sweep, adds reverse index + incremental redundancy counters, and adds manual cleanup API support logic. |
| pegaflow-metaserver/src/lib.rs | Deprecates ttl_minutes semantics and wires the store into the HTTP server startup. |
| pegaflow-metaserver/src/http_server.rs | Adds the admin cleanup endpoint and a small route-level test. |
| pegaflow-metaserver/Cargo.toml | Adds deps needed for JSON response serialization and HTTP route testing. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed the review feedback in ab4215e: heartbeat redundancy accounting uses one definition, admin cleanup is loopback-only on a separate listener, and synchronous store work is batched/offloaded to the blocking pool. Added regression coverage and updated the PR description with RTX 4090 validation data. Please re-review. |
|
Since these services are internal by default, could we reuse the existing HTTP listener for cleanup and drop the separate admin listener and configuration? |
|
Implemented in commit The manual cleanup route now reuses the existing Updated the route tests and verified |
|
Follow-up: the PR description now reflects the shared listener as well. The implementation is pushed as |
xiaguan
left a comment
There was a problem hiding this comment.
LGTM on 2e139c1.
Reviewed lifecycle cleanup, session replacement and unregister races, incremental owner accounting, and the shared HTTP listener. Local validation: 45 metaserver tests, strict Clippy, formatting, and a real HTTP smoke test passed; all 12 CI checks are green.
Non-blocking cleanup: remove the obsolete README statement that the public listener returns 404 for the cleanup route, and consider dropping the redundant route-availability test already covered by the preceding cleanup test.
The known lost-removal metadata risk remains a follow-up. The dedicated vLLM correctness E2E gate was not run; the maintainer has elected to proceed with this limitation disclosed. No independent performance claim is made here.
Problem
The 600-second MetaServer sweep walked every block and rebuilt redundancy state even when all nodes were healthy. During a real vLLM run this can hold block shard locks long enough for query-prefetch retries to grow.
Change
POST /admin/cleanup-expired-blocksto the existing--http-addrlistener (default0.0.0.0:9092). It removes owners registered strictly more than one hour before the call, preserves nodes and physical KV data, and returnsremoved_owners/removed_keys.RTX 4090 vLLM A/B
Master
0c49ca3and feature6365f1aran sequentially on the same host with two TP2 DeepSeek-V2-Lite-Chat instances, block size 16, two PegaFlow servers, 400 GB pools, 1M+ retained logical keys, 660 seconds of steady traffic, and an added identical 20-request/s real short-cold stream. The measurement proxy adds one identical local gRPC hop to both variants.The master query stall and 2.85x query-prefetch increase occur in the same sweep bin. Feature has no corresponding query stall or rate spike, but its isolated Insert maximum is higher in that window; causality is unresolved. Each steady run includes approximately 25k Inserts and 88 Heartbeats, limiting heartbeat tail conclusions. All steady RPCs and inference requests succeeded; one feature pressure request returned 500 during teardown after
finished.The proxy captured QueryPrefixBlocks, InsertBlockHashes and HeartbeatNode. No RemoveBlockHashes or UnregisterNode calls were recorded, so their latency is unmeasured. HTTP cleanup measures an internal MetaServer scan; it does not cover the server-to-MetaServer RemoveBlockHashes RPC. RPC timing starts at proxy forwarding and excludes time queued in the PegaFlow server.
Cleanup endpoint measurement
All feature requests were made while the real vLLM load continued.
The repeat call overlapped only 4 Queries / 9 Inserts. Query mean before/during/after was 0.588/4.709/0.600 ms; Insert was 0.572/1.084/0.507 ms (30-second before/after windows). This shows a transient increase, with too few samples for stable tail estimates. The extra 77,952 deletions match seed registrations crossing the one-hour cutoff during the 30-second gap.
The cleanup POST is served on the existing HTTP listener and remains POST-only (GET returns 405). Two nodes remained active throughout; final retained keys were 1,066,748. The aged calls delete metadata owners only, so existing physical KV allocations are not reclaimed by this endpoint.
This is one sequential A/B on a single host using real vLLM-generated metadata registrations; the vLLM correctness gate was not run. Full raw artifacts and analysis are in
.local/reports/pr451-vllm/; the remote run directories are under/root/kexi/vllm_meta_ab/.