fix: record activation cache prefix identities - #1041
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughResident activation recording now returns multiple validated cache records, with stricter restoration checks. Binary transport centralizes completed-prefill accounting and emits telemetry per record, while frontend prefill handling iterates over all activation records. ChangesResident activation recording
Binary transport accounting and completed-prefill recording
Frontend activation telemetry
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BinaryConnection
participant PrefillRecording
participant KvStageIntegration
participant ActivationCache
participant Telemetry
BinaryConnection->>PrefillRecording: record_completed_prefill
PrefillRecording->>KvStageIntegration: record_resident_activation
KvStageIntegration->>ActivationCache: store candidate activation identities
ActivationCache-->>KvStageIntegration: activation records
KvStageIntegration-->>PrefillRecording: activation records
PrefillRecording->>Telemetry: emit per-record binary KV decisions
PrefillRecording-->>BinaryConnection: BinaryKvRecordResult
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/skippy-server/src/binary_transport/binary_messaging/connection.rs (1)
83-102: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile exceeds the 1,000-line modification threshold.
handle_binary_connectionand its surrounding module already run well past 1,000 lines, and this PR modifies it (lines 804-823) without extracting a separable responsibility to bring it under the limit.As per coding guidelines: "When modifying a Rust file already over 1,000 lines, extract any separable responsibility into a named module, keep the new file under 1,000 lines, and move or add its tests with the extracted behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/binary_transport/binary_messaging/connection.rs` around lines 83 - 102, Extract a separable responsibility from handle_binary_connection and its surrounding binary messaging logic into a named Rust module, keeping the new module under 1,000 lines. Move or add tests for the extracted behavior alongside that module, and update handle_binary_connection to delegate to it while preserving existing behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
crates/skippy-server/src/kv_integration/activation.rs (1)
98-124: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider sharing prefix bytes instead of cloning per candidate.
activation_prefix_frameclones the full frame payload for the exact-match case and clones-then-truncates for each grid-floor candidate. Withshared_prefix_record_limit> 1, every recorded prefill pays for N copies of the (potentially large) activation payload. Since every candidate is a strict byte-prefix of the same buffer, a shared/reference-counted buffer (e.g.Arc<[u8]>orbytes::Bytes) could avoid the repeated allocation/copy on this hot path — though this would require touching theActivationFramepayload representation inskippy-runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/kv_integration/activation.rs` around lines 98 - 124, Update activation_prefix_frame and the ActivationFrame payload representation to use a shared/reference-counted byte buffer, such as Arc<[u8]> or bytes::Bytes, so exact-match and truncated prefix candidates share the original allocation instead of cloning payload bytes. Preserve the existing validation, prefix lengths, metadata updates, and returned frame behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/skippy-server/src/binary_transport/binary_messaging/connection.rs`:
- Around line 83-102: Extract a separable responsibility from
handle_binary_connection and its surrounding binary messaging logic into a named
Rust module, keeping the new module under 1,000 lines. Move or add tests for the
extracted behavior alongside that module, and update handle_binary_connection to
delegate to it while preserving existing behavior.
---
Nitpick comments:
In `@crates/skippy-server/src/kv_integration/activation.rs`:
- Around line 98-124: Update activation_prefix_frame and the ActivationFrame
payload representation to use a shared/reference-counted byte buffer, such as
Arc<[u8]> or bytes::Bytes, so exact-match and truncated prefix candidates share
the original allocation instead of cloning payload bytes. Preserve the existing
validation, prefix lengths, metadata updates, and returned frame behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c9b511e-a540-483b-882f-cab614fc3da9
📒 Files selected for processing (6)
crates/skippy-server/src/binary_transport.rscrates/skippy-server/src/binary_transport/activation_cache.rscrates/skippy-server/src/binary_transport/binary_kv.rscrates/skippy-server/src/binary_transport/binary_messaging/connection.rscrates/skippy-server/src/frontend/prefix_cache.rscrates/skippy-server/src/kv_integration/activation.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
ndizazzo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@crates/skippy-server/src/binary_transport/binary_messaging/prefill_recording.rs`:
- Around line 41-54: Remove the Clippy suppression from record_completed_prefill
and introduce cohesive context/request-input structs to reduce its argument
count while preserving behavior. Update
crates/skippy-server/src/binary_transport/binary_messaging/prefill_recording.rs
lines 41-54 to accept those grouped parameters, and update lines 85-96 so
record_full_prefill_with_activations reuses the shared context; do not add
another suppression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68c9fbfe-25a4-47ff-956b-06132d47c4a4
📒 Files selected for processing (3)
crates/skippy-server/src/binary_transport/binary_messaging.rscrates/skippy-server/src/binary_transport/binary_messaging/connection.rscrates/skippy-server/src/binary_transport/binary_messaging/prefill_recording.rs
38e85ef to
81e06b8
Compare
|
Codex review feedback: I found one blocking issue in the activation-cache lookup path.
That means the newly stored shorter activation frames are not considered for a later prompt that extends the same prefix. The new unit test only calls restore with Please update activation restore to walk the same lookup candidates used for resident KV and pass the matching prefix token slice/identity through, or add an end-to-end test showing a longer prompt can hit the newly recorded shared-prefix activation. As written, the multi-record activation recording adds extra cache entries/telemetry, but the normal binary downstream path still appears to restore only exact activation identities. |
81e06b8 to
a8e51e9
Compare
* origin/main: Fix Metal small-batch matmul parity for GLM verification (#1078) Handle K-only transposed KV page import and export (#1084) Refresh llama.cpp upstream patch queue (#1085) chore: improve embedded native-runtime compatibility guidance (#1043) fix(console-ui): chat transcript snapping during live status updates (#1083) ci: bump Linux CUDA slim container to gha-convention base runner image fix: record activation cache prefix identities (#1041) fix: read-only model download caches (#1042) ci: disable sccache for Windows ROCm native runtime build (#1087) ci: fix v0.74 release GPU builds (sccache disk-only + force_hosted_runners) (#1086) Make release sccache failures non-fatal (#1079) Keep client-only nodes out of model election (#1074) # Conflicts: # crates/mesh-llm-host-runtime/src/runtime/auto_join.rs # crates/mesh-llm-host-runtime/src/runtime/tests/auto_join.rs # third_party/llama.cpp/patches/0004-Add-lanes-external-media-and-chat-grammar-support.patch
2 participants
Closes #683.
Summary
Validation
cargo test -p skippy-server --lib(277 passed)cargo clippy -p skippy-server --all-targets -- -D warningscargo check -p mesh-llmcargo clippy -p mesh-llm --all-targets -- -D warningscargo test -p skippy-protocol --libjust test-allSummary by CodeRabbit