Skip to content

fix(l2): reconcile fs-native eviction ledger drift - #67

Closed
yatesdr wants to merge 2 commits into
devfrom
fix/l2-ledger-drift
Closed

yatesdr wants to merge 2 commits into
devfrom
fix/l2-ledger-drift

Conversation

@yatesdr

@yatesdr yatesdr commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

What this PR does / why we need it:

fs_native returned a false per-key delete result when an object was already
absent. NativeConnectorL2Adapter consequently retained that key's byte
accounting and LRU entry forever, causing repeated no-op eviction and eventual
store admission failures. A concurrent re-store could create this state because
L2 eviction did not protect keys with pending native store completions.

This change makes native filesystem deletion idempotent for an already-absent
object while preserving filesystem errors, and excludes pending-store keys from
adapter deletion. Regression tests cover both ledger reconciliation and the
store/delete completion race.

Special notes for your reviewers:

Reproduced on cn4 with qualified image
localinferencelab/vllm@sha256:c9ad4a6ef4aa55232df9ed1a37e85d94eb8c7d5349561a6cbe828e72b61de83c
(LMCache 35ad809f): after removing a tracked payload, two deletes left the
20-byte ledger entry and key tracking unchanged.

Validation:

  • Rebuilt LMCache with NO_GPU_EXT=1 in the qualified r32 environment.
  • 108 tests passed across test_native_connector_l2_adapter.py and
    test_fs_native_connector.py.
  • Focused Ruff lint/format, mypy, and clang-format checks passed.
  • GitHub Code Quality, CodeQL, and CPU artifact build checks pass.

Current unrelated dev CI failures:

  • The full Python 3.11 run completes 5,803 tests and fails one unchanged
    multiprocess test because its _FakeStorageManager lacks the existing
    abort_write method; the remaining Python matrix jobs are then cancelled.
  • All four CPU-device jobs install the current vLLM nightly and fail in the
    unchanged vLLM integration path because KVConnectorBlockState.block_ids
    has been replaced by get_block_ids.

CodeRabbit reports success but skips automatic review because this OSS
repository has fewer than ten stars and therefore requires a manual review
trigger.

If applicable:

  • this PR contains user facing changes - docs added
  • this PR contains unit tests

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a42816c1-1098-4cb3-af58-3df8837000d9


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@voipmonitor

Copy link
Copy Markdown

Independent validation of the filesystem eviction-ledger fix on the shared GLM/Qwen/DS4 serving image:

  • Exact R32 reproduces the absent-file deletion failure for both short and bounded long keys. The adapter retains stale byte/LRU accounting when the file is already absent.
  • With both commits from this PR, 116 installed-image connector/adapter tests pass, including absent-file deletion, pending-store eviction exclusion and actual filesystem-error propagation. A nonempty-directory delete still fails without retiring its accounting.
  • TP4/DCP1 MTP3 with FP8 KV, a 4096-token scheduler budget, 4 GiB RAM tier and fs_native passes exact 54,641-token cold/GPU/RAM/filesystem/restart checks. RAM restore is 0.260 s, filesystem restore 0.277 s and restore after restarting workers plus sidecar 0.414 s. All restores recompute zero prompt tokens. Shared SYSTEM reuse and changed-SYSTEM isolation pass too. Filesystem timings include a warm OS page cache.

The integration preserves R32's bounded long-key paths and legacy-path cleanup: every representable path is processed, missing files count as successful deletion, and genuine I/O errors propagate. The CPU filesystem extension is rebuilt; all audited CUDA/vLLM/FlashKDA binaries remain byte-identical. The sidecar remains CPU-only and GPU copies stay worker-owned.

Complete attributed integration preserves Derek Yates's two commits with source-commit references. This PR remains the canonical review unit; no duplicate PR was created. Scope is filesystem-ledger correctness and a bounded GLM transport regression check, not fresh TP8, NVFP4-KV or Qwen/DS4 external-cache qualification.

@voipmonitor

Copy link
Copy Markdown

Superseded by #49, which now preserves Derek Yates’s filesystem eviction-ledger commits together with bounded cache-key paths. The audited #67 head ec09897 is an ancestor of #49 head b4b79a0. Canonical and legacy files must both be absent before retiring ledger accounting; missing files are idempotent and actual deletion errors retain the ledger. The affected native implementation matches the published R34 source, and 116 filesystem/native-adapter tests pass. Closing this separate review unit avoids stacking two overlapping filesystem lifecycle patches; contribution credit and Git history remain intact.

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.

2 participants