Skip to content

fix(handler): release the page when batch_store loses the register race - #94

Open
youngrok-XCENA wants to merge 2 commits into
mainfrom
fix/batch-store-lost-race
Open

youngrok-XCENA wants to merge 2 commits into
mainfrom
fix/batch-store-lost-race

Conversation

@youngrok-XCENA

@youngrok-XCENA youngrok-XCENA commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

🤔 Background & Motivation (Why)

When two handlers store the same key concurrently, batch_store() leaks the later handler's page and incorrectly reports failure.

For example, two vLLM instances serving the same model with the same chunk settings share a Maru metadata server. Requests with a common token prefix generate the same cache key K for a shared prefix chunk. If both requests miss the cache, each instance computes and tries to store its own copy.

A's existence check does not reserve the key, so B can register it before A:

sequenceDiagram
    participant A as Handler A
    participant S as Metadata server
    participant B as Handler B

    A->>A: Write KV to page P
    B->>B: Write KV to page Q
    A->>S: Does K exist?
    S-->>A: No
    rect rgb(255, 248, 225)
        B->>S: Register K at Q
        S-->>B: Registered
        Note over S: K now points to Q
    end
    A->>S: Register K at P
    S-->>A: Already exists

    alt Before this PR
        Note over A: P stays allocated but unused<br/>Return False to caller
    else After this PR
        A->>A: Free P
        Note over A: Return True to caller<br/>K is already stored at Q
    end
Loading

Before the fix, page P cannot be reused until A closes, and the caller cannot mark K as stored.

🏗️ Design Changes

The changed branch is the per-key duplicate result from a successful registration RPC. Its page lifecycle now matches single-key store():

flowchart TB
    R["Registration response for K"] --> D{"New registration?"}
    D -->|Yes| W["Keep local page P<br/>Track K at P; return True"]
    D -->|"No: B already registered K at Q"| B0
    D -->|"No: B already registered K at Q"| A0
    subgraph BEFORE["Before"]
        B0["Set result to False"] --> B1["Skip local tracking<br/>P remains allocated and unreachable"]
    end
    subgraph AFTER["After this PR"]
        A0["Remove P from pending allocations"] --> A1["Return P to local allocator"]
        A1 --> A2["Do not track K locally<br/>Return True: K is already stored"]
    end
    Q["Metadata remains K → Q<br/>B owns the winning page Q"]
    B1 -.-> Q
    A2 -.-> Q
    classDef leak fill:#fff0ed,stroke:#b64431
    classDef fixed fill:#eaf6ed,stroke:#27763e
    class B0,B1 leak
    class A0,A1,A2 fixed
Loading

True means the key is stored in the shared pool; it does not imply ownership of the winning page. RPC failures still return False and release pending pages.

📝 Implementation Details

  • Scope: the shared CXL MaruHandler.batch_store() path, called by both vLLM direct and LMCache's MaruBackend batch put. Single-key store() already handles this case.
  • Free duplicate entries' pages when processing the registration response, and exclude them from the local location map.
  • Document the return-value semantics in the batch_store() docstring.

✅ Tests

  • Unit tests
  • Integration tests
  • Manual tests
  • No tests needed (reason: )

Unit regression checks page cleanup and [True, True] using mocked RPC responses. Python 3.12/3.13/3.14 unit CI and Ruff passed.

Real serving reproduction (2026-10-02): two independent vLLM instances sharing Maru, Qwen2.5-0.5B, CXL, 256-token chunks. Each path replayed identical prompts/token IDs on the parent (f2e18df) and this PR (63425e4), with 30 concurrent request pairs and no server-side scheduling barrier:

Serving path Duplicate registrations, before → after Leaked duplicate pages, before → after
vLLM direct, layerwise 896 → 858 896 (112 MiB) → 0
LMCache MaruBackend, non-MP, non-layerwise 8 → 8 8 (24 MiB) → 0

In both paths, every losing registration returned False before; after the fix, every losing page was freed and batch_store() returned True for each duplicate key. Traces join real client keys, RPC responses, allocations, and frees; no RPC responses were mocked. LMCache was pinned to b9c8647d, with asynchronous puts fully drained before checking page counts.

Three additional pairs per revision synchronized clients after real existence checks: direct produced 116 → 68 duplicates; LMCache produced 6 → 6. Every duplicate leaked before and was freed after. All four runs completed 66/66 race-test HTTP requests; HTTP success alone did not expose the leak.

After the fix, both paths had zero orphan pages. Separate metadata reads confirmed the winning locations for all 926 direct and 14 LMCache duplicate keys. LMCache cache-read requests also retrieved 512 tokens and preserved generated output, including a request that had lost the registration race. These are traced reproductions, not measurements of production incidence or performance.

🔗 Related Issues (optional)

🌿 Related PRs (optional)

🌿 Related Branches (optional)

📦 Release Note (for auto-generation / write in English)

NEW

CHANGED

FIXED

  • Fix a page leak and incorrect failure result when batch_store() encounters a key registered concurrently by another client.

IMPORTANT NOTES

batch_register_kv returns False for a key that another client registered
between the existence check and the register RPC. batch_store left that
page allocated with no key pointing at it and reported the key as a
failure, unlike store(), which frees the page and reports the key as
stored. Free the page, report True, and document the return contract.
@youngrok-XCENA
youngrok-XCENA requested a review from a team October 2, 2026 02:36
@youngrok-XCENA
youngrok-XCENA marked this pull request as ready for review October 2, 2026 02:36

@kihwan-XCENA kihwan-XCENA left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear root-cause analysis and a well-scoped fix. LGTM 👍

@seohui-XCENA seohui-XCENA left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the race and the fix. The freed page is no longer referenced: the server only adds a KV ref for new entries, every connector path finishes the D2H before batch_store, and LMCache resolves reads through the server lookup. The new test fails on the parent ([False, True]) and passes here, and test_maru_handler.py passes in full. Two points inline to address before merging.

Comment thread maru_handler/handler.py
is_new = (
batch_resp.results[batch_idx]
if batch_idx < len(batch_resp.results)
else True

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A short results list falls back to True, so the key is tracked locally even if the server never registered it. The connector's write-behind path treats a short response as failure; could we free the page and return False here as well?

Comment thread maru_handler/handler.py
existed (in the local map, on the server, or registered by
another client while this call ran). A page this handler did
not register is returned to the allocator. False means the
register RPC failed, in which case every page is freed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also note that batch_store only raises before consuming any handle (connection, closing, or length checks)? The connector's _free_handles_best_effort relies on that.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants