Skip to content

Add KVS cache connector for KV cache reuse#1154

Open
RulinJuice wants to merge 1 commit into
alibaba:feat/dsv4_on_dev_refactorfrom
RulinJuice:feature/kvs-cache-connector
Open

Add KVS cache connector for KV cache reuse#1154
RulinJuice wants to merge 1 commit into
alibaba:feat/dsv4_on_dev_refactorfrom
RulinJuice:feature/kvs-cache-connector

Conversation

@RulinJuice

Copy link
Copy Markdown
Collaborator

Summary

  • Add KVS cache connector for KV cache reuse.
  • Wire KVS connector into cache connector coordinator and resource context.
  • Add KVS cache configuration and server args.
  • Build stable KVS cache object keys with namespace and version.

@RulinJuice RulinJuice requested a review from LLLLKKKK as a code owner July 1, 2026 01:13
@CLAassistant

CLAassistant commented Jul 1, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@LLLLKKKK

LLLLKKKK commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1154

Status: BLOCKING

Summary: P0/0 · P1/1 · P2/3 · P3/1

Blocking Issues

P1

  • recvJsonMessage 未限制消息长度可导致 OOM @ rtp_llm/cpp/cache/connector/kvs_connector/KVSClient.cc:87
    • 建议:添加合理上限检查(如 16MB),超过则返回 std::nullopt 并打日志:
if (len > 16 * 1024 * 1024) {
RTP_LLM_LOG_WARNING("KVSClient recv message too large: %llu", (unsigned long long)len);
return std::nullopt;
}

Non-blocking Suggestions

P2

  • asyncMatch 中 keys.pop_back() 的语义依赖隐式约定且与 write 逻辑不对称 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSConnector.cc:219
    • 建议:如果是有意设计(最后一个 key 对应当前正在增长的 token 序列,不应匹配远程 cache),添加简短注释说明理由。如果不是有意设计,建议与 asyncWriteTask 对齐,仅在 !resource->lastBlockAligned() 时 pop_back。
  • buildDeploymentId 依赖 CHECKPOINT_PATH 环境变量路径,可能阻碍跨实例复用 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSConnector.cc:415
    • 建议:如果 KVS 仅用于同 Pod/同挂载路径的实例间复用(如扩缩容场景),在代码注释中说明这一限制。如果需要真正跨实例复用,考虑使用模型配置的内容哈希或去除路径前缀仅保留 checkpoint 名称。
  • KVSConnector 析构中线程池任务持有 this 指针的生命周期依赖 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSConnector.cc:109
    • 建议:确认 autil::ThreadPool::stop() 会丢弃未执行的排队任务(如果是,添加注释标明这一语义依赖)。否则考虑将核心状态(如 client_)提取为独立 shared_ptr,lambda 捕获 shared_ptr 而非 this。

P3

  • asyncReadTask 中 (void)resource 与实际使用矛盾 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSConnector.cc:293
    • 建议:删除 (void)resource; 这一行。

Checklist Violations (3 fail / 56 total)

General Principles Checklist

  • [6.1] Architecture — 兼容性:公开 API/持久数据/配置/环境迁移安全 → issue buildDeploymentId 依赖 CHECKPOINT_PATH 环境变量路径,可能阻碍跨实例复用
    buildDeploymentId()CHECKPOINT_PATH 环境变量作为 hash 输入。同一模型在不同机器上 CHECKPOINT_PATH 可能不同(如 /mnt/oss/model_v1 vs /data/models/model_v1),导致 deployment_id 不同,相同模型无法跨实例复用 KV cache。这与 KVS cache 跨实例复用的核心目标可能矛盾。
  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue KVSConnector 析构中线程池任务持有 this 指针的生命周期依赖
    ~KVSConnector() 调用 thread_pool_->stop(); thread_pool_->waitFinish()。线程池中 task lambda 通过 [this, ...] 捕获了 this 指针。安全性取决于 autil::ThreadPool::stop() 是否保证不再执行已排队的 task——如果 waitFinish() 会执行剩余排队任务,则任务体内访问 this->client_ 等成员时,成员析构顺序可能已经开始(特别是 client_ 是 shared_ptr,在基类成员之后析构)。

RTP-LLM Checklist

  • [I] 代码质量 — RAII span 作用域匹配 → issue asyncReadTask 中 (void)resource 与实际使用矛盾
    第 293 行写了 (void)resource; 表示该参数未使用,但第 317 行实际调用了 resource->setRemoteReuseBlockNum(...)(void)resource 是无效且误导代码审阅者的死代码。

Strengths

  • 架构分层清晰:KVSClient 负责 v6d 协议交互,KVSConnector 负责 cache 语义编排,职责划分合理。
  • 完整的错误降级策略:acquireForRead 失败时降级为 cache miss 而非整体失败,保证推理可用性。
  • 测试覆盖较好:FakeKVSClient mock 设计合理,核心路径(write/match/read)和边界场景(acquire 失败、partial match)都有验证。
  • 与现有 connector 体系的集成方式一致:复用 ThreadPool、CacheConfig 等已有基础设施,配置绑定方式与 RemoteConnector 保持对齐。

@LLLLKKKK

LLLLKKKK commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1154

Status: BLOCKING

Summary: P0/0 · P1/1 · P2/2 · P3/1

Blocking Issues

P1

  • KVSClient::load 用 expected 覆盖服务器上报的 handle.bytes,存在越界读风险 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSClient.cc:494
    • 建议:在覆盖 handle.bytes = expected 之前,先与服务器上报的原始 handle.bytes(即 handle_it->second.bytes)比较:expected 大于服务器 size 则记录 warning 并 return false;相等或更小才允许继续(更小时也应校验 expected 不越过 handle_it->second.bytes)。或者干脆把边界检查改成基于服务器 size 做 expected <= server_bytesserver_bytes <= mmap_size_ - offset 的两段检查,copyFromObject 内保留对 expected 的精确匹配。

Non-blocking Suggestions

P2

  • buildDeploymentId 缺 head 与 layer 相关维度,容易在 CHECKPOINT_PATH 缺失时碰撞 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSConnector.cc:438
    • 建议:把 head 布局与 layer 数一起纳入 buildDeploymentId 的 fingerprint 输入(如 cache_config_.getBlockSize()layer_all_numlocal_head_num_kvsize_per_head),或者在启动时对 fingerprint 与已有 vineyard namespace 做一次 sanity 探测(例如首次 acquire 成功后校验 server 返回的 object size 与本地期望一致再进入 read 路径),避免依赖运维额外设置 CHECKPOINT_PATH 才能保正确。
  • enable_remote_cache 被 kvs 分支复用,executeFunctionremote_request 语义可能混淆 @ rtp_llm/cpp/engine_base/stream/ResourceContext.cc:11
    • 建议:要么给 ResourceContext 增加一个独立的 enable_kvs_cache 字段(并由所有决定是否走 remote 分支的上游改为 enable_remote_cache || enable_kvs_cache 时明确挑选正确 backend),要么在 KVCacheConnectorCoordinator::executeFunctionremote_request 分支加 KVS 兜底(收到 remote_request 但只有 kvs_connector_ 时,返回 false 记录 warning,而不是 CHECK fail)。另外建议给 KVS 启用后上游走 remote 语义的路径补一个集成或单测。

P3

  • KVSAsyncContext::waitDone() 无实际等待,语义与基类契约不一致 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSConnector.h:75
    • 建议:要么让 waitDone 真正阻塞(在 KVSConnectorState 上加 std::condition_variableset(State) 时 notify_all,waitDonewait(!done())),要么在类注释里显式声明 waitDone 为 no-op、约定必须调用方轮询 done()。当前实现建议至少加一行注释说明与 coordinator 轮询模型的耦合,避免误用。

Checklist Violations (5 fail / 56 total)

General Principles Checklist

  • [6.1] Architecture — 兼容性:公开 API/持久数据/配置/环境迁移安全 → issue ``buildDeploymentId 缺 head 与 layer 相关维度,容易在 CHECKPOINT_PATH 缺失时碰撞
    `buildDeploymentId` 拼装 fingerprint 时只包含 `model_name`、`CHECKPOINT_PATH`、`dtype`、`seq_size_per_block`、`kernel_seq_size_per_block`、`tp_size`、`fp8/int8/mla` 与 `groupNums()/typed`,未包含 `local_head_num_kv`、`size_per_head`、`layer_num/layer_all_num` 等直接决定 block 字节大小的字段。同时 `CHECKPOINT_PATH` 通过 `autil::EnvUtil::getEnv("CHECKPOINT_PATH", std::string(""))` 拿默认空串。当同一 `model_name` 的两个部署未设置 `CHECKPOINT_PATH`,但 head_num_kv 或 layer 数目不同(例如同名不同微调、开关 speculative 主/草稿模型),deployment_id 会碰撞,object_key 相同但每 block 字节数不同
  • [6.1] Architecture — 分层边界:新概念在正确层级,不泄漏内部 → issue ``enable_remote_cache 被 kvs 分支复用,executeFunction` 的 `remote_request` 语义可能混淆`
    `ResourceContext::initCacheConfig` 改为 `enable_remote_cache = kv_cache_config.enable_remote_cache || kv_cache_config.enable_kvs_cache;`,同时 `KVCacheConnectorCoordinator::init` 又把 remote 与 kvs 视为互斥(`enable_remote_cache && !enable_kvs_cache` 才初始化 `remote_connector`)。这样启用 `enable_kvs_cache` 后,`ResourceContext::enable_remote_cache=true`,但 coordinator 端并没有 `remote_connector_`。`KVCacheConnectorCoordinator::executeFunction` 中 `has_remote_request` 分支执行 `RTP_LLM_CHECK(remote_connector_ != nullptr)`——KVS 部_
  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue ``enable_remote_cache 被 kvs 分支复用,executeFunction` 的 `remote_request` 语义可能混淆`
    `ResourceContext::initCacheConfig` 改为 `enable_remote_cache = kv_cache_config.enable_remote_cache || kv_cache_config.enable_kvs_cache;`,同时 `KVCacheConnectorCoordinator::init` 又把 remote 与 kvs 视为互斥(`enable_remote_cache && !enable_kvs_cache` 才初始化 `remote_connector`)。这样启用 `enable_kvs_cache` 后,`ResourceContext::enable_remote_cache=true`,但 coordinator 端并没有 `remote_connector_`。`KVCacheConnectorCoordinator::executeFunction` 中 `has_remote_request` 分支执行 `RTP_LLM_CHECK(remote_connector_ != nullptr)`——KVS 部_
  • [6.1] Architecture — 错误语义:fail-fast/retry/fallback/silent 行为显式 → issue ``buildDeploymentId 缺 head 与 layer 相关维度,容易在 CHECKPOINT_PATH 缺失时碰撞
    `buildDeploymentId` 拼装 fingerprint 时只包含 `model_name`、`CHECKPOINT_PATH`、`dtype`、`seq_size_per_block`、`kernel_seq_size_per_block`、`tp_size`、`fp8/int8/mla` 与 `groupNums()/typed`,未包含 `local_head_num_kv`、`size_per_head`、`layer_num/layer_all_num` 等直接决定 block 字节大小的字段。同时 `CHECKPOINT_PATH` 通过 `autil::EnvUtil::getEnv("CHECKPOINT_PATH", std::string(""))` 拿默认空串。当同一 `model_name` 的两个部署未设置 `CHECKPOINT_PATH`,但 head_num_kv 或 layer 数目不同(例如同名不同微调、开关 speculative 主/草稿模型),deployment_id 会碰撞,object_key 相同但每 block 字节数不同
  • [6.1] Software Engineering — LSP:子类/重写保持基类契约 → issue ``KVSAsyncContext::waitDone() 无实际等待,语义与基类契约不一致
    `KVSAsyncMatchContext::waitDone()` 与 `KVSAsyncContext::waitDone()` 都被写成 `{}` 空实现。当前 coordinator 只在 update thread 里轮询 `done()`(`processReadContexts` / `processWriteContexts`),所以空实现工作正常;但基类 `AsyncContext::waitDone` 契约通常是"阻塞至 done"。在测试里也是通过手写 `for (int i = 0; i < 200 && !context->done(); ++i) sleep(1ms)` 绕过 `waitDone`。未来若有任何调用方对 `waitDone` 依赖阻塞语义(例如同步收尾、shutdown 前 drain),会直接返回 false success 而不知情。

Strengths

  • KVSConnectorSharedState 让异步 task 只捕获 state 与上下文,避免依赖 KVSConnector 对象生命周期,析构中 thread_pool_->stop() + waitFinish() 也是安全的关闭顺序。
  • KVSAsyncMatchContext 析构里自动 release(lease_id),并在 asyncMatchTask 未匹配到新块时主动 release + 清空 lease,避免 lease 泄漏,两个路径不会重复 release。
  • 单元测试覆盖了 acquire 失败降级为 miss、buffer 为空/非法不上报命中、read 后释放 lease、write 跳过非对齐尾块等关键分支;集成测试通过 KVS_INTEGRATION_URL/SOCKET 环境变量开关,避免在 CI 强依赖 vineyard。
  • KVCacheConfig 的 pickle 反序列化对新增字段使用 t.size() > extra_idx 逐字段兼容旧 tuple,能兼容旧 checkpoint 状态。

@RulinJuice RulinJuice force-pushed the feature/kvs-cache-connector branch 3 times, most recently from c881c41 to de10de8 Compare July 1, 2026 04:42
@LLLLKKKK

LLLLKKKK commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1154

Status: BLOCKING

Summary: P0/0 · P1/0 · P2/2 · P3/2

Non-blocking Suggestions

P2

  • 同步 cudaMemcpy 使用默认流可能阻塞 GPU 流水线 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSClient.cc:287
    • 建议:建议使用 cudaMemcpyAsync 搭配独立的 CUDA stream(在 KVSClient 初始化时通过 cudaStreamCreate 创建),拷贝完成后通过 cudaStreamSynchronize 等待。这样拷贝操作不会与推理流的 kernel 产生交叉同步。可作为 V2 优化项。
  • totalBytes() 辅助函数在两个编译单元中重复定义 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSConnector.cc:74
    • 建议:将 totalBytes() 提取到 BlockInfo.h 中作为 inline 工具函数(或新建 BlockInfoUtils.h),两处统一引用,消除重复。

P3

  • FakeKVSClient 未覆盖 discard() 方法,异常路径测试覆盖不足 @ rtp_llm/cpp/cache/connector/kvs_connector/test/KVSConnectorTest.cc:109
    • 建议:在 FakeKVSClient 中添加 void discard(const std::string& lease) override,记录调用以验证失败路径的租约清理行为。建议补充 store_ok = false 的测试用例覆盖 store 失败后的 discard 路径。
  • CURL handle 每次请求创建销毁,无连接复用 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSClient.cc:369
    • 建议:考虑在 KVSClient 中维护持久 CURL handle(线程安全场景下用 handle 池),通过 curl_easy_reset() 复用连接。也可使用 CURLOPT_TCP_KEEPALIVE 保持长连接。当前实现作为 V1 可接受,建议后续迭代优化。

Checklist Violations (4 fail / 56 total)

General Principles Checklist

  • [6.1] Architecture — 可观测性:日志/指标/超时可操作、非噪声 → issue 同步 cudaMemcpy 使用默认流可能阻塞 GPU 流水线
    copyHostAndDevice() 使用同步 cudaMemcpy() 在 mmap 与 GPU 内存之间拷贝 KV cache block。此调用发生在 KVS 线程池的 worker 线程中,使用 CUDA legacy 默认流(null stream)。legacy 默认流上的操作会与同一 device 上所有其他流产生隐式同步屏障,可能在 per-request 拷贝时阻塞推理热路径的 GPU 流水线。每个 block 可能有几百 KB 到数 MB,多层多 group 的 block 拷贝累积延迟不可忽视。
  • [6.1] Software Engineering — DRY:重复非平凡逻辑被抽取或显式复用 → issue ``totalBytes() 辅助函数在两个编译单元中重复定义
    `KVSClient.cc:256` 和 `KVSConnector.cc:74` 各自的匿名命名空间中定义了完全相同的 `totalBytes(const std::vector& iovs)` 函数,包含相同的溢出保护逻辑。两处逻辑耦合但独立维护,未来修改一处易遗漏另一处。
  • [6.1] Software Engineering — KISS/YAGNI:无投机性抽象 → issue CURL handle 每次请求创建销毁,无连接复用
    httpGet()httpPost() 每次调用都执行 curl_easy_init() / curl_easy_cleanup() 创建和销毁 CURL handle。KVS 的 store 操作每次至少 3 次 HTTP 调用(acquire + seal + release),每次都重新建立 TCP 连接。在高频 cache store 场景下,TCP 连接建立/销毁的开销会累积。
  • [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue ``FakeKVSClient未覆盖discard()` 方法,异常路径测试覆盖不足`
    `FakeKVSClient` 覆盖了 `release()` 但未覆盖 `discard()`。`KVSClient::store()` 在 copy 或 seal 失败时调用 `discard(lease_id)` 做租约回滚(KVSClient.cc:801)。当前测试中 `store_ok` 始终为 true 不会触发此路径,但若未来增加 store 失败测试场景,`discard()` 会调用基类实现尝试真实 HTTP POST。

Strengths

  • 架构设计清晰:KVSConnector 正确继承 KVCacheConnector 接口,通过 KVSConnectorSharedState 共享状态实现异步任务与 connector 对象的生命周期解耦,避免了析构期间的 use-after-free 风险。
  • 配置兼容性处理得当:pybind pickle __setstate__ 使用逐字段 if (t.size() > extra_idx) 条件解包,正确支持旧版本 tuple 反序列化。
  • 测试覆盖合理:FakeKVSClient mock 覆盖了 match/read/write 主路径及 acquire 失败降级、empty buffer 和 null buffer 边界场景,集成测试 env-gated 避免 CI 依赖。
  • KVS 与 remote connector 的互斥逻辑正确:!enable_kvs_cache 守卫避免了两者同时初始化,executeFunctionremote_connector_ 空指针检查替代了原 RTP_LLM_CHECK 硬断言,提升了容错性。
  • 对象 key 设计稳定:buildDeploymentId 通过 cache layout 参数的完整指纹哈希隔离不同配置,buildObjectKey 编入 tp_rank 和 group_id 避免跨 rank 数据污染。

@RulinJuice RulinJuice force-pushed the feature/kvs-cache-connector branch from de10de8 to a0f8f26 Compare July 7, 2026 02:31
@LLLLKKKK

LLLLKKKK commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1154

Status: BLOCKING

Summary: P0/0 · P1/0 · P2/4 · P3/0

Non-blocking Suggestions

P2

  • KVSCacheStore 未接入生产且与 KVSConnector 大量重复实现 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSCacheStore.cc:1
    • 建议:若 KVSCacheStore 是同步 fallback 的规划实现,建议本 PR 暂不引入(YAGNI)或加注释说明用途与接入计划;若无用则删除。无论哪种,sanitizeKeyPart/hashString/buildObjectKey/buildDeploymentId 等 key/layout 生成逻辑应抽取到单一公共工具(如 kvs_connector 内的共享头),由两者复用,避免 key 规则分叉。
  • KVSClient 的 HTTP/mmap/JSON 解析核心逻辑缺少可在 CI 运行的单测 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSClient.cc:187
    • 建议:对不依赖真实服务的纯函数补充单测:parseObjectHandle 针对缺字段/非法 object_id(uint64 max)/meta.size 覆盖 result.bytes/越界 offset 等分支;recvJsonMessage 针对 len > kMaxJsonMessageBytes 与空消息;copyFromObject/copyToObject 针对 offset+size > handle.bytes 与末尾 offset == handle.bytes 校验。这些均可用内存 buffer 直接构造,不需要 vineyard 依赖。
  • init 阶段 vineyard unix socket 握手无超时,异常时可能永久 hang @ rtp_llm/cpp/cache/connector/kvs_connector/KVSClient.cc:266
    • 建议:对 socket_fd_ 设置基于 config_.timeout_msSO_RCVTIMEO/SO_SNDTIMEO(并可考虑非阻塞 connect + 超时),使握手失败可在有限时间内返回 false,从而走已有的 init 失败告警路径,保证启动可观测、可 fail-fast。
  • load 用目标 expected 覆盖 handle.bytes,未校验远端对象真实大小 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSClient.cc:512
    • 建议:在 load 覆盖前校验一致性:当 acquire 得到的真实 handle.bytes 已知且 > 0 时,要求其 == expected(或 >= expected),不一致则告警并返回 false 走 degrade-to-miss,避免用错误长度读取相邻对象数据。

Checklist Violations (7 fail / 56 total)

General Principles Checklist

  • [6.1] Architecture — 可观测性:日志/指标/超时可操作、非噪声 → issue init 阶段 vineyard unix socket 握手无超时,异常时可能永久 hang
    initMmap()sendAll/recvAll/recvmsgsocket_fd_ 上执行阻塞 IO,socket 创建后未设置 SO_SNDTIMEO/SO_RCVTIMEOconnect 也用默认阻塞语义。而 HTTP 面用 CURLOPT_TIMEOUT_MS 有超时。若 vineyard daemon 存活但 register/get_mmap_fd 握手挂起,recvJsonMessage/recvmsg 会无限阻塞,且该逻辑在 KVSConnector::init()RTP_LLM_CHECK_WITH_INFO 的 server 启动关键路径上,导致进程启动永久 hang 而非 fail-fast。
  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue load 用目标 expected 覆盖 handle.bytes,未校验远端对象真实大小
    KVSClient::load 中先做 mmap 越界校验,随后 handle.bytes = expected;(expected 来自目标 iovs 累加),再 copyFromObjectmmap_base_ + data_offset 拷贝 expected 字节,并要求 offset == handle.bytes 结束。此处完全丢弃了 acquireForRead/parseObjectHandle 解析出的远端对象真实 bytes。若远端对象实际大小小于本地 expected(key 规则升级、hash 碰撞或 layout 不一致),拷贝仍按 expected 进行,会读到该对象之后相邻 mmap 区域的数据,形成静默的脏 KV(模型输出错误),且因 mmap 内不触发崩溃而难以察觉。
  • [6.1] Architecture — 错误语义:fail-fast/retry/fallback/silent 行为显式 → issue init 阶段 vineyard unix socket 握手无超时,异常时可能永久 hang
    initMmap()sendAll/recvAll/recvmsgsocket_fd_ 上执行阻塞 IO,socket 创建后未设置 SO_SNDTIMEO/SO_RCVTIMEOconnect 也用默认阻塞语义。而 HTTP 面用 CURLOPT_TIMEOUT_MS 有超时。若 vineyard daemon 存活但 register/get_mmap_fd 握手挂起,recvJsonMessage/recvmsg 会无限阻塞,且该逻辑在 KVSConnector::init()RTP_LLM_CHECK_WITH_INFO 的 server 启动关键路径上,导致进程启动永久 hang 而非 fail-fast。
  • [6.1] Software Engineering — DRY:重复非平凡逻辑被抽取或显式复用 → issue KVSCacheStore 未接入生产且与 KVSConnector 大量重复实现
    KVCacheConnectorCoordinator::initKVSConnector() 只实例化 KVSConnector,全仓库 KVSCacheStore 仅被自身头文件、KVSConnector.h(引用其中的 struct)和一个单测引用,未接入任何生产路径。但 KVSCacheStore.cc 完整复制了 sanitizeKeyPart/hashString/totalBytes/isValidBlockInfo 以及 buildBlockObjects/buildObjectKey/buildDeploymentId 与 match/read/write 的同一套 wire 协议逻辑。两份实现今后极易分叉,deployment_id/object_key 规则一旦只改一处就会导致读写 key 不一致。
  • [6.1] Software Engineering — KISS/YAGNI:无投机性抽象 → issue KVSCacheStore 未接入生产且与 KVSConnector 大量重复实现
    KVCacheConnectorCoordinator::initKVSConnector() 只实例化 KVSConnector,全仓库 KVSCacheStore 仅被自身头文件、KVSConnector.h(引用其中的 struct)和一个单测引用,未接入任何生产路径。但 KVSCacheStore.cc 完整复制了 sanitizeKeyPart/hashString/totalBytes/isValidBlockInfo 以及 buildBlockObjects/buildObjectKey/buildDeploymentId 与 match/read/write 的同一套 wire 协议逻辑。两份实现今后极易分叉,deployment_id/object_key 规则一旦只改一处就会导致读写 key 不一致。
  • [6.1] Tests — 新逻辑有聚焦单测 + 相关集成/smoke 测试 → issue KVSClient 的 HTTP/mmap/JSON 解析核心逻辑缺少可在 CI 运行的单测
    KVSConnectorTest 全部通过 FakeKVSClient 打桩,真正的协议解析(parseObjectHandle、acquire/store 的 rapidjson 解析、recvJsonMessage 长度校验、copyFromObject/copyToObject 的偏移边界)只在 KVSClientIntegrationTest 中覆盖,而后者在缺少 KVS_INTEGRATION_URL/KVS_INTEGRATION_SOCKETGTEST_SKIP(),CI 常态下不执行。这些是最易出错、最影响正确性的解析/拷贝逻辑,却没有可离线运行的单测。
  • [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue KVSClient 的 HTTP/mmap/JSON 解析核心逻辑缺少可在 CI 运行的单测
    KVSConnectorTest 全部通过 FakeKVSClient 打桩,真正的协议解析(parseObjectHandle、acquire/store 的 rapidjson 解析、recvJsonMessage 长度校验、copyFromObject/copyToObject 的偏移边界)只在 KVSClientIntegrationTest 中覆盖,而后者在缺少 KVS_INTEGRATION_URL/KVS_INTEGRATION_SOCKETGTEST_SKIP(),CI 常态下不执行。这些是最易出错、最影响正确性的解析/拷贝逻辑,却没有可离线运行的单测。

Strengths

  • lease 生命周期管理清晰:KVSAsyncMatchContext 析构、asyncReadTaskmatch 失败路径都能释放 lease,且 read 后 reset() 避免二次释放。
  • 错误语义显式且安全:acquire/fetch/load/store 任一失败都 degrade 到 miss 或 CLIENT_ERROR,不 crash;parseObjectHandle/load/storedata_offset/bytes 都做了防溢出的 mmap 越界校验。
  • pickle 向后兼容处理细致:通过 py::isinstance<py::dict> 探测旧 map 字段并正确推进 extra_idx,新增字段全部走 t.size() > extra_idx 可选读取,旧 pickle 仍可解析。
  • executeFunctionremote_requestRTP_LLM_CHECK(false) 改为返回 false + 告警,避免了仅启用 KVS 时收到 remote 请求直接 abort 的风险。
  • KVSConnector 单测覆盖了 match 命中/降级/空 buffer/非法 buffer、read 成功与 fetch 失败不提升 remote_reuse、write 跳过未对齐尾块等关键路径,并用 Fake client/allocator 隔离依赖。

@RulinJuice RulinJuice force-pushed the feature/kvs-cache-connector branch from a0f8f26 to a3dd518 Compare July 7, 2026 03:07
@RulinJuice RulinJuice force-pushed the feature/kvs-cache-connector branch from a3dd518 to 85c0232 Compare July 7, 2026 03:13
@LLLLKKKK

LLLLKKKK commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1154

Status: BLOCKING

Summary: P0/0 · P1/0 · P2/4 · P3/1

Non-blocking Suggestions

P2

  • KVSCacheStore 与 KVSConnector 核心逻辑整份重复且仅测试使用 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSCacheStore.cc:566
    • 建议:建议二选一收敛:要么删除生产未用的 KVSCacheStore 类、让单测直接测 KVSConnector(保留其中 KVSBlockObject/KVSBlockObjects 结构体供复用);要么让 KVSConnector 直接委托 KVSCacheStore 实现同步核心逻辑。同时将 buildObjectKey/buildDeploymentIdsanitizeKeyPart/hashString/totalBytes 抽到单一共享 helper,消除三处重复,保证 key/layout 规则单点定义。
  • KVSConnector 传入 metrics_reporter 却从不上报任何指标 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSConnector.cc:107
    • 建议:参照 RemoteConnector 的 MetricsHelper 模式,为 KVS 的 match/read/write 增加耗时与命中/失败指标(如命中 block 数、acquire/fetch/load/store 耗时、degrade-to-miss 计数、client_error 计数),使其与既有 remote cache 指标口径对齐,便于线上灰度与容量评估。
  • KVSClient 解析/越界与 pickle 逻辑在 CI 中无可运行覆盖 @ rtp_llm/cpp/cache/connector/kvs_connector/test/KVSClientIntegrationTest.cc:33
    • 建议:parseObjectHandle 接收 rapidjson::Value,可直接用构造好的 JSON 文本写 CI 可运行单测,覆盖正常/缺字段/object_id 非法/越界等边界;同理为 getJsonUint64 的各分支补测。建议再补一个 KVCacheConfig pickle round-trip 单测,断言含 kvs 字段与不含 kvs 字段(旧 state)两种 tuple 都能正确还原。
  • curl/rapidjson 被无条件引入核心 cache connector 依赖链 @ rtp_llm/cpp/cache/connector/kvs_connector/BUILD:26
    • 建议:建议确认 curl/rapidjson 在全部平台依赖集(含内部 @rtp_deps 覆盖)均可解析;并考虑与 remote connector 一致,将 KVS 相关源文件与重依赖用 feature flag(如 using_kvs_cache 或复用 using_remote_kv_cache)门控,避免为默认关闭的功能给所有部署引入额外编译面与二进制体积。

P3

  • 读成功后按整段区间提升 remoteReuseBlockNum,未校验每个 block 均被覆盖 @ rtp_llm/cpp/cache/connector/kvs_connector/KVSConnector.cc:1871
    • 建议:在提升 remoteReuseBlockNum 前,增加对“[start, end) 内每个 block 均在 matched_blocks_ 中且产出了 dst buffer”的校验(如断言覆盖的 block 数等于 read_block_num,否则回退为 CLIENT_ERROR 或仅提升到实际连续覆盖的末尾),使该不变量在代码层面显式成立而非隐式依赖调用方。

Checklist Violations (9 fail / 56 total)

General Principles Checklist

  • [6.1] Architecture — 依赖方向:无循环依赖/跨层惊喜 → issue curl/rapidjson 被无条件引入核心 cache connector 依赖链
    kvs_connector 依赖 @curl//:curl@rapidjson//:rapidjson(BUILD:26、BUILD:29),而全仓 BUILD 检索显示这两个外部仓库此前未被 rtp_llm/cpp 任何 target 使用(仅本次新增引用)。同时 rtp_llm/cpp/cache/connector/BUILDkvs_connector 无条件加入 deps(不在 select 内),而既有 remote connector 是被 select({"//:using_remote_kv_cache": [...]}) 门控的。这意味着 curl/rapidjson 成为所有平台(cuda12/12.9/arm、rocm、ppu、cpu)cache 路径的强制依赖,即使部署未开启 KVS 也会被编译进二进制;若某内部平台(如 PPU/ROCm)的依赖集中未提供 @curl,会直接导致该平台构建失败。
  • [6.1] Architecture — 可观测性:日志/指标/超时可操作、非噪声 → issue KVSConnector 传入 metrics_reporter 却从不上报任何指标
    KVSConnectorSharedState::metrics_reporter 在构造时赋值(KVSConnector.cc:107),KVSCacheStore 也持有 metrics_reporter_(KVSCacheStore.cc:57),但在 match/read/write 全流程中均无任何 report(...) 调用。而 KVS 所替代的 RemoteConnector 上报了 match/read/write/SDK 的耗时与命中类指标(remote_connector 中 RtpLLMRemoteCacheMatchMetrics/ReadMetrics/WriteMetrics/SDKMetrics)。一旦线上开启 enable_kvs_cache,remote cache 路径的命中率、延迟、失败计数等可观测性将全部丢失,问题定位只能靠日志。
  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue 读成功后按整段区间提升 remoteReuseBlockNum,未校验每个 block 均被覆盖
    asyncReadTask 在 client 三步(fetch/load/complete)成功后执行 resource->setRemoteReuseBlockNum(start_read_block_index + read_block_num)(KVSConnector.cc:1871),但 client 操作只作用于 dst_buffers 里实际存在的 object key,而 dst_buffers 仅由落在 [start, end) 区间且存在于 matched_blocks_ 的 block 构成。当前调用方(coordinator)按 matchedBlockCount() 连续区间下发读请求,matched_blocks_prev_reuse 起连续,故正常契约下不会出现空洞;但代码本身未对“请求区间内每个 block 都产出了 object”做校验。若上游契约变化或 allocator 出现不一致导致区间内某 block 无 object,会把未真正加载的 block 计入 remote_reuse,进而在 attenti
  • [6.1] Software Engineering — DRY:重复非平凡逻辑被抽取或显式复用 → issue KVSCacheStore 与 KVSConnector 核心逻辑整份重复且仅测试使用
    KVSCacheStorebuildObjectKey/buildDeploymentId/buildBlockObjects/match/read/writeKVSConnector 中的同名逻辑几乎逐行重复(KVSCacheStore.cc:566 buildObjectKey vs KVSConnector.cc:1956;KVSCacheStore.cc:574 buildDeploymentId vs KVSConnector.cc:1966),且 sanitizeKeyPart/hashString/totalBytes/isValidBlockInfo 在 KVSCacheStore.cc、KVSConnector.cc、KVSClient.cc 三个匿名命名空间各自再实现一份。全仓检索显示 KVSCacheStore 类仅被 test/KVSConnectorTest.cc:368 引用,生产链路(coordinator)只用 KVSConnector。object key / deployme
  • [6.1] Software Engineering — KISS/YAGNI:无投机性抽象 → issue curl/rapidjson 被无条件引入核心 cache connector 依赖链
    kvs_connector 依赖 @curl//:curl@rapidjson//:rapidjson(BUILD:26、BUILD:29),而全仓 BUILD 检索显示这两个外部仓库此前未被 rtp_llm/cpp 任何 target 使用(仅本次新增引用)。同时 rtp_llm/cpp/cache/connector/BUILDkvs_connector 无条件加入 deps(不在 select 内),而既有 remote connector 是被 select({"//:using_remote_kv_cache": [...]}) 门控的。这意味着 curl/rapidjson 成为所有平台(cuda12/12.9/arm、rocm、ppu、cpu)cache 路径的强制依赖,即使部署未开启 KVS 也会被编译进二进制;若某内部平台(如 PPU/ROCm)的依赖集中未提供 @curl,会直接导致该平台构建失败。
  • [6.1] Software Engineering — SRP:模块/类职责单一 → issue KVSCacheStore 与 KVSConnector 核心逻辑整份重复且仅测试使用
    KVSCacheStorebuildObjectKey/buildDeploymentId/buildBlockObjects/match/read/writeKVSConnector 中的同名逻辑几乎逐行重复(KVSCacheStore.cc:566 buildObjectKey vs KVSConnector.cc:1956;KVSCacheStore.cc:574 buildDeploymentId vs KVSConnector.cc:1966),且 sanitizeKeyPart/hashString/totalBytes/isValidBlockInfo 在 KVSCacheStore.cc、KVSConnector.cc、KVSClient.cc 三个匿名命名空间各自再实现一份。全仓检索显示 KVSCacheStore 类仅被 test/KVSConnectorTest.cc:368 引用,生产链路(coordinator)只用 KVSConnector。object key / deployme
  • [6.1] Tests — 新逻辑有聚焦单测 + 相关集成/smoke 测试 → issue KVSClient 解析/越界与 pickle 逻辑在 CI 中无可运行覆盖
    KVSClientIntegrationTest 三个用例均以 if (!url || !socket) { GTEST_SKIP(); } 开头(KVSClientIntegrationTest.cc:33 起),依赖 KVS_INTEGRATION_URL/KVS_INTEGRATION_SOCKET 环境变量,CI 默认跳过。因此 KVSClient 中较复杂且易错的纯逻辑——parseObjectHandle(blob/handle 字段校验、object_id==uint64_max 判无效、mmap 越界判定)、getJsonUint64 的多类型分支、load/store 的 offset/size 边界——没有任何在 CI 中实际运行的单测。此外 ConfigInit.cc 中新增的 pickle getstate/setstate(可选 dict 甄别 + 7 个可选字段)也缺少 round-trip 测试,而该 pickle 在多进程 spawn 时会真实执行,出错将直接影响启动。
  • [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue KVSClient 解析/越界与 pickle 逻辑在 CI 中无可运行覆盖
    KVSClientIntegrationTest 三个用例均以 if (!url || !socket) { GTEST_SKIP(); } 开头(KVSClientIntegrationTest.cc:33 起),依赖 KVS_INTEGRATION_URL/KVS_INTEGRATION_SOCKET 环境变量,CI 默认跳过。因此 KVSClient 中较复杂且易错的纯逻辑——parseObjectHandle(blob/handle 字段校验、object_id==uint64_max 判无效、mmap 越界判定)、getJsonUint64 的多类型分支、load/store 的 offset/size 边界——没有任何在 CI 中实际运行的单测。此外 ConfigInit.cc 中新增的 pickle getstate/setstate(可选 dict 甄别 + 7 个可选字段)也缺少 round-trip 测试,而该 pickle 在多进程 spawn 时会真实执行,出错将直接影响启动。

RTP-LLM Checklist

  • [I] 代码质量 — 同一功能用统一工具函数 → issue KVSCacheStore 与 KVSConnector 核心逻辑整份重复且仅测试使用
    KVSCacheStorebuildObjectKey/buildDeploymentId/buildBlockObjects/match/read/writeKVSConnector 中的同名逻辑几乎逐行重复(KVSCacheStore.cc:566 buildObjectKey vs KVSConnector.cc:1956;KVSCacheStore.cc:574 buildDeploymentId vs KVSConnector.cc:1966),且 sanitizeKeyPart/hashString/totalBytes/isValidBlockInfo 在 KVSCacheStore.cc、KVSConnector.cc、KVSClient.cc 三个匿名命名空间各自再实现一份。全仓检索显示 KVSCacheStore 类仅被 test/KVSConnectorTest.cc:368 引用,生产链路(coordinator)只用 KVSConnector。object key / deployme

Strengths

  • 分层清晰:KVSClient(curl 控制面 + vineyard unix socket / mmap 数据面)与 KVSConnector(线程池异步编排)职责分离,接口与状态机与既有 RemoteConnector 保持一致,降低了接入 coordinator 的风险。
  • 降级语义良好:acquire 失败时 match 不报错而是降级为 miss(返回 SUCCESS 且 matched=prev_reuse),符合缓存路径“不可用即回退本地计算”的预期。
  • 越界防护到位:parseObjectHandleloadstore 均对 data_offset/bytes 相对 mmap_size_ 做了边界检查,recvJsonMessage 对长度前缀设有 16MB 上限,避免向共享内存越界读写。
  • Lease 无泄漏:读完成、match 无命中、KVSAsyncMatchContext 析构三条路径都覆盖了 lease release;线程池任务捕获 state_ 而非 this,关机时排队任务不依赖 connector 对象生命周期。
  • 异步 connector 路径单测较充分:覆盖了 acquire 失败降级、空/非法 buffer 不误报命中、fetch 失败不提升 remote_reuse、lease 释放、object key 稳定性与非对齐尾块跳过等边界。
  • pybind pickle 做了向后兼容处理:新增 7 个字段全部为可选读取,并用 py::isinstance<py::dict> 甄别历史可选 dict 字段,避免把 bool 误当 map。

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.

3 participants