Add Skippy request cache epoch telemetry - #817
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds StageRequestEpoch (identity + checkpoint/prompt/decode fields), epoch extraction on wire messages, staleness/flow-checking methods and tests, metric attribute constants, and refactors server telemetry to use epoch-derived attributes. ChangesRequest Epoch Type and Telemetry Integration
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/skippy-protocol/src/binary/types.rs (1)
360-371: ⚡ Quick winConsider adding documentation for staleness semantics.
The
is_stale_formethod uses lexicographic comparison of(checkpoint_generation, prompt_token_count, decode_step)to determine staleness, which gives checkpoint_generation the highest priority, followed by prompt_token_count, then decode_step. While the implementation is correct, a doc comment would clarify the intended semantics and precedence order for future maintainers.📝 Suggested documentation
+ /// Returns true if this epoch is stale (outdated) compared to `current`. + /// + /// Two epochs can only be compared if they belong to the same flow (same request_id and session_id). + /// Staleness is determined by lexicographic comparison of (checkpoint_generation, prompt_token_count, decode_step), + /// meaning checkpoint_generation takes precedence, followed by prompt_token_count, then decode_step. pub fn is_stale_for(self, current: Self) -> bool {🤖 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-protocol/src/binary/types.rs` around lines 360 - 371, Add a doc comment to the is_stale_for method that explains the staleness semantics: state is considered stale only if same_flow(current) is true and the tuple (checkpoint_generation, prompt_token_count, decode_step) is lexicographically less than the current tuple, meaning checkpoint_generation has highest precedence, then prompt_token_count, then decode_step; mention what each field represents for staleness comparison and that the comparison is strict (<), and reference same_flow to clarify flow matching is required before comparing generations/steps.crates/skippy-protocol/src/binary/mod.rs (1)
217-243: 💤 Low valueConsider expanding test coverage for lexicographic ordering.
The test verifies
decode_stepordering but doesn't test thatcheckpoint_generationtakes precedence overprompt_token_count, or thatprompt_token_counttakes precedence overdecode_step. While the implementation uses standard Rust tuple comparison (which is well-defined), explicit tests would document the intended behavior more clearly.🧪 Example additional test cases
// Verify checkpoint_generation takes precedence let lower_checkpoint = StageRequestEpoch { request_id: 7, session_id: 11, checkpoint_generation: 0, prompt_token_count: 100, decode_step: 100, }; let higher_checkpoint = StageRequestEpoch { request_id: 7, session_id: 11, checkpoint_generation: 1, prompt_token_count: 0, decode_step: 0, }; assert!(lower_checkpoint.is_stale_for(higher_checkpoint)); assert!(!higher_checkpoint.is_stale_for(lower_checkpoint));🤖 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-protocol/src/binary/mod.rs` around lines 217 - 243, Add explicit unit tests to cover lexicographic ordering precedence for StageRequestEpoch: add cases that verify checkpoint_generation dominates prompt_token_count and prompt_token_count dominates decode_step by constructing StageRequestEpoch instances with differing checkpoint_generation/prompt_token_count/decode_step (while keeping request_id and session_id equal) and asserting expected results from is_stale_for and same_flow; reference the StageRequestEpoch struct and its methods is_stale_for and same_flow and add tests similar to the existing request_epoch_orders_only_matching_flows test to document and validate the intended ordering (checkpoint_generation > prompt_token_count > decode_step).
🤖 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.
Nitpick comments:
In `@crates/skippy-protocol/src/binary/mod.rs`:
- Around line 217-243: Add explicit unit tests to cover lexicographic ordering
precedence for StageRequestEpoch: add cases that verify checkpoint_generation
dominates prompt_token_count and prompt_token_count dominates decode_step by
constructing StageRequestEpoch instances with differing
checkpoint_generation/prompt_token_count/decode_step (while keeping request_id
and session_id equal) and asserting expected results from is_stale_for and
same_flow; reference the StageRequestEpoch struct and its methods is_stale_for
and same_flow and add tests similar to the existing
request_epoch_orders_only_matching_flows test to document and validate the
intended ordering (checkpoint_generation > prompt_token_count > decode_step).
In `@crates/skippy-protocol/src/binary/types.rs`:
- Around line 360-371: Add a doc comment to the is_stale_for method that
explains the staleness semantics: state is considered stale only if
same_flow(current) is true and the tuple (checkpoint_generation,
prompt_token_count, decode_step) is lexicographically less than the current
tuple, meaning checkpoint_generation has highest precedence, then
prompt_token_count, then decode_step; mention what each field represents for
staleness comparison and that the comparison is strict (<), and reference
same_flow to clarify flow matching is required before comparing
generations/steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49ff12c1-b515-4473-a3ab-4a97d4ece819
📒 Files selected for processing (3)
crates/skippy-protocol/src/binary/mod.rscrates/skippy-protocol/src/binary/types.rscrates/skippy-server/src/binary_transport.rs
|
@michaelneale could you review this one when you get a chance? |
* origin/main: Add transport-aware Skippy stage ordering (#814) Share Skippy stage wire byte accounting (#818) Report Skippy artifact cold-start costs (#815) fix: debug output capturing for TUI / panics (#827) fix(hero): visual corrections for iPhone SE size devices (#838) Add Skippy stage role metadata (#816) Add Skippy request cache epoch telemetry (#817) Consolidate agent skills and fix stale docs (Windows deploy, repo map, design docs) (#836) feature(version): normalize version markers for different build types (#831) fix(website): fix visual regressions (#835) fix(gh): change micn to michaelneale in auto_assign.yml Revert "fix(gh): replace micn with IvGolovach in auto_assign.yml (not a collaborator)" fix(gh): replace micn with IvGolovach in auto_assign.yml (not a collaborator)
* origin/main: (29 commits) MoA: don't let small-model consensus pre-empt a still-running large model (#837) fix(console): render thinking traces as markdown Add bounded direct path repair (#846) Fix skippy smoke PR gate (#850) Stabilize skippy smoke chain startup (#849) fix(ci): switch back to auto-assign workflow fix(website): polish longform visual explainer (#843) fix: gemma thinking Carry GLM llama MTP patches (#840) Refresh llama.cpp canary patch queue (#839) Add transport-aware Skippy stage ordering (#814) Share Skippy stage wire byte accounting (#818) Report Skippy artifact cold-start costs (#815) fix: debug output capturing for TUI / panics (#827) fix(hero): visual corrections for iPhone SE size devices (#838) Add Skippy stage role metadata (#816) Add Skippy request cache epoch telemetry (#817) Consolidate agent skills and fix stale docs (Windows deploy, repo map, design docs) (#836) feature(version): normalize version markers for different build types (#831) fix(website): fix visual regressions (#835) ... # Conflicts: # AGENTS.md
Summary
Add a typed request/cache epoch view for Skippy binary stage messages and emit checkpoint generation in server telemetry.
This PR is about request/cache coordination vocabulary. It does not add protocol fields, change encoded bytes, or reject stale frames yet.
Before
The stage message already carried the pieces needed to identify a cache generation:
request_idsession_idcheckpoint_generationprompt_token_countdecode_stepBut code treated them as separate fields. Telemetry had prompt/decode information, but not checkpoint generation in the common binary-message attribute set.
How It Works
StageRequestEpochgroups the existing identity fields:StageWireMessage::request_epoch()derives the value from an existing message.The helper also supports:
same_flow(...)for request/session matchingis_stale_for(...)for future stale-frame/cache-epoch checksskippy-servernow uses the epoch helper in binary transport telemetry and emits:Why This Is Good
Skippy is getting more aggressive about prefix/cache reuse, restore, trim, prefill, and decode overlap. Those paths need one shared way to talk about which cache generation a frame belongs to.
This PR creates that vocabulary without changing behavior yet. It makes future stale restore/trim/decode checks smaller and easier to review because the identity comparison is centralized.
Compatibility
This is not a wire-format change.
The
skippy-protocolcrate changes, but the bytes sent between stages do not.Validation
cargo fmt --all -- --checkcargo test -p skippy-protocol --libcargo test -p skippy-server --libcargo clippy -p skippy-protocol --all-targets -- -D warningscargo clippy -p skippy-server --all-targets -- -D warningscargo check -p mesh-llmcargo clippy -p mesh-llm --all-targets -- -D warningsSummary by CodeRabbit