Repository navigation
Conversation
|
👋 Hi chanh! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
feat
external-contribution
a59eb73 to
69c5687
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughAdds an environment-controlled, process-local cache-history store for KV routing. Integrates hash collection, request lifecycle recording, FIFO retention, and optional metrics for completed, incomplete, and aborted requests. ChangesKV cache history
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The opt-in telemetry change has no substantiated merge-blocking issue; the duplicate metric is only optional cleanup. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 7 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description contains detailed implementation and validation information, but it does not follow the required template. It omits the required Overview and Details headings, uses an incomplete Related Issue section, and describes peer-history sharing and metric-name revisions that are not reflected in the provided changeset objectives. Resolution Rewrite the description using the required Overview, Details, Where should the reviewer start?, and Related Issues sections. State the actual F1 cache reuse history changes from this pull request. Replace the current Related Issue text with either a valid issue reference such as "Relates to
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
There was a problem hiding this comment.
Questions for the author:
- lib/llm/src/kv_router/routing_host/kv_selection.rs: For the enabled KV-routing path, please provide a request-path allocation/latency calculation or measurement for retaining routing block hashes and constructing/retaining sequence hashes.
return_routing_hashesnow forces retained hashes for every non-query request, including callers that previously did not record routing decisions.
69c5687 to
8b7ad77
Compare
8b7ad77 to
576d24e
Compare
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Reviewed at 3feafefb. I focused on the two words in the title that carry the risk — opt-in and history — and verified each by execution rather than by reading.
Feature-off differential: byte-identical
I instrumented both trees with the same deterministic routing-decision trace (4 workers, 4 prompt families pre-planted on fixed workers so prefix overlap gives a strict best and the tie reservoir / HashMap iteration order cannot change the winner), then drove 64 requests through select_with_affinity -> track_selection -> on_item -> finish and recorded (worker_id, dp_rank, cached_tokens, effective_overlap_blocks, routing_hashes.len()) per decision.
| run | sha256 of trace |
|---|---|
base f1cb3ae6 (feature off) |
0e5223b4...c792fe |
head 3feafefb (feature off) |
0e5223b4...c792fe |
head 3feafefb (DYN_CACHE_REUSE_HISTORY_ENABLED=1) |
identical on all 64 decisions |
Base-vs-head with the feature off is byte-identical across the whole trace. With the feature on, all 64 decisions are still identical — only the cache_history_metrics flag I printed differs. Re-ran each 2-3 times to rule out the process-global tie nondeterminism. Cross-checked by grep: previously_computed_tokens has exactly one caller (CacheHistoryTracking::new) and its result reaches only a Prometheus counter, so the history cannot feed worker selection. selection.routing_hashes is read only inside track_selection_with_cleanup, so the new .take() in the non-recording branch steals nothing a later stage needs.
Memory bound, measured
Rebuilt the exact structure (64 FxHashSet<u64> shards + Mutex<VecDeque<u64>>, same insert/evict path as record_completed) under a counting global allocator and ran it on Linux x86_64/glibc:
budget 256 MiB -> capacity 8,388,608 entries
live heap (requested) 208.0 MiB = 26.00 B/entry (code claims 32)
RSS after fill 210.6 MiB (VmHWM 210.6 MiB)
ESTIMATED_BYTES_PER_ENTRY = 32 is conservative — 8 B for the deque plus ~18 B for hashbrown at the ~50% load factor the power-of-two bucket count forces. Confirmed at 16 MiB / 256 MiB / 1 GiB budgets (all 26.00 B/entry). On macOS ARM64 the same fill shows ~298 MiB RSS, but that is the platform allocator retaining freed doubling transients, not the structure, so it does not apply to the deployed target. At the bound the policy is evict-oldest, never refuse and never grow; duplicates neither consume capacity nor refresh age. Good.
Invalidation contract
There is none, and that is the right call for this metric: nothing removes an entry when a worker leaves, a block is evicted from a real KV cache, or the cache is flushed — only FIFO eviction at capacity. F1 is "previously computed", a funnel ceiling, not "currently resident". Worth making explicit in the module doc, since the two readings are easy to confuse.
Two properties that follow and are not stated anywhere: the history is process-local, so with N frontend replicas a prefix computed on replica A is an F1 miss on replica B and the aggregate ratio is biased low; and it resets to zero on restart. ApproximateLruMetrics right below carries exactly this class of caveat in a doc comment; CacheHistoryMetrics has none.
Concurrency
Lock order is total (FIFO mutex then shard write locks), the same-shard eviction case reuses the live guard so parking_lot's non-reentrancy is not hit, and no guard is held across an await (finish_cache_history is sync and the guard drops before cleanup.finish().await). record_completed's allocation-free membership pre-pass does keep the common duplicate case off the global FIFO lock. I did not find a way for one request's tracking to contaminate another's — CacheHistoryTracking is owned by the RequestGuard, and finalize_cache_history takes the Option, so the exactly-once lifecycle holds (covered by the two new tests, and Drop after finish() is a no-op).
CI
All pre-merge checks green at this SHA, including rust-tests (.) (32m), which is the job that runs the 8 new cache_history and lifecycle tests. The skipped docs/Fern/operator jobs are consistent with a Rust-only diff. Nothing to triage.
Findings
4 comments below: 1 x P2, 3 x P3. No P0/P1. Not approving only because the combined P2+P3 count is at my threshold; none of them is a correctness defect in the disabled path.
|
Narrowing the review bot's open question on The O(tokens) hashing is not new work.
For a 16k-token prompt at block size 64 that is ~250 blocks: ~4 KB retained, ~250 hash-chain steps, ~2 KB cloned. Per request, on the request path, and only when the feature is enabled. Stated as a code-path reading, not a measurement — I did not benchmark the request path, so this bounds the shape of the cost rather than its latency. |
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Re-reviewed at df8cce0c3. The merge base is still f1cb3ae6, so the real delta is +651/-25 across 13 files. The fix commit adds 90 lines and removes 20 across 5 files.
Feature-off differential, re-run at the new head
The fix commit adds 5 lines to request_guard.rs, which sits on the routing path, so I re-ran the same deterministic routing-decision trace as the last round: 4 workers, 4 prompt families pre-planted on fixed workers so prefix overlap gives a strict best, then 64 requests through select_with_affinity -> track_selection -> on_item -> finish, recording (worker_id, dp_rank, cached_tokens, effective_overlap_blocks, routing_hashes.len()) per decision.
The default probe uses the default approximate cache policy (ttl). The new approximate_lru_active flag is only reachable under router_approximate_cache_policy: Lru, so I added a second probe in that configuration and had it also report ApproximateLruStats from the indexer actor.
| run | trace sha256 |
|---|---|
base f1cb3ae6, ttl policy, feature off |
c650bbfd...e18098 |
head df8cce0c, ttl policy, feature off (3 runs) |
c650bbfd...e18098 |
head df8cce0c, ttl policy, DYN_CACHE_REUSE_HISTORY_ENABLED=1 |
c650bbfd...e18098 |
base f1cb3ae6, lru policy, feature off |
c650bbfd...e18098 |
head df8cce0c, lru policy, feature off |
c650bbfd...e18098 |
Base and head are byte-identical with the feature off, in both policies. The actor stats match too, at three rank capacities:
1024 blocks base==head requests=68 request_messages=264 output_batches=64 fallback_activations=0 resident=144 evicted=0
4 blocks base==head requests=68 request_messages=264 output_batches=64 fallback_activations=0 resident=16 evicted=128
0 blocks base==head requests=68 request_messages=136 output_batches=0 fallback_activations=4 resident=0 evicted=0
Two controls, because a passing differential is only evidence when the harness can detect a difference.
- Routing control. In the head tree I recorded each routing decision against
worker.worker_id ^ 1. The trace sha256 moved to05e2368a...f854e7. The harness detects a perturbed decision. - Gate control. In the head tree I forced
self.approximate_lru_active = false. The actor stats moved fromrequest_messages=264 output_batches=64 resident=144torequest_messages=136 output_batches=0 resident=80. The stats probe detects the new gate.
cache_history.rs, semantic read
The +29/-7 is three changes and no more: the struct doc comment, the from_env parse arm, and one new test. with_byte_budget, record_completed, previously_computed_tokens and stats are untouched.
No bound, eviction rule, or capacity limit changed. capacity_bytes = requested_bytes.max(ESTIMATED_BYTES_PER_ENTRY) and capacity_entries = capacity_bytes / ESTIMATED_BYTES_PER_ENTRY still hold, and record_completed still pops the oldest FIFO entry at capacity. The history stays bounded. Under memory pressure the policy is still evict-oldest, never refuse and never grow, which is the measured 208.0 MiB live heap per host at the default budget from the last round.
Metric audit: Rust list, Python list, registration site
All three agree, checked name by name.
| constant | value | in Rust | in Python | registered |
|---|---|---|---|---|
CACHE_LOSS_OBSERVATION_INPUT_TOKENS_TOTAL |
cache_loss_observation_input_tokens_total |
yes | yes | counter |
CACHE_LOSS_FUNNEL_TOKENS_TOTAL |
cache_loss_funnel_tokens_total |
yes | yes | counter, label stage |
CACHE_LOSS_OBSERVATIONS_TOTAL |
cache_loss_observations_total |
yes | yes | counter, label result |
CACHE_LOSS_HISTORY_UNIQUE_HASHES |
cache_loss_history_unique_hashes |
yes | yes | gauge |
CACHE_LOSS_HISTORY_REPRESENTED_TOKENS |
cache_loss_history_represented_tokens |
yes | yes | gauge |
CACHE_LOSS_HISTORY_ESTIMATED_BYTES |
cache_loss_history_estimated_bytes |
yes | yes | gauge |
CACHE_LOSS_HISTORY_CAPACITY_BLOCKS |
cache_loss_history_capacity_blocks |
yes | yes | gauge |
CACHE_LOSS_HISTORY_CAPACITY_BYTES |
cache_loss_history_capacity_bytes |
yes | yes | gauge |
Three checks, all by execution:
- The 8 constant values are byte-identical to the 8 string literals they replaced at
df8cce0c3^. cargo run -p dynamo-codegen --bin gen-python-prometheus-namesregeneratedlib/bindings/python/src/dynamo/prometheus_names.pywith no diff against the committed file.- I scraped the registry at
3feafefband atdf8cce0cand compared the exposition output. The 8 names, their types, and their label sets are identical. Only the HELP strings changed, and HELP is not part of a PromQL contract. With the feature off, none of the 8 register at either SHA.
So the +13/-13 in metrics.rs breaks no dashboard and no alert.
Findings
The four earlier threads are answered below. One is a retraction: my approximate-LRU finding had a wrong premise, and I say so on the thread with the measurement that disproves it.
One new P3 on the added test. Nothing else. No P0, no P1.
Approving
Zero P0, zero P1, one P3. The differential holds at the new head with both controls, the metric contract is unchanged, and the three earlier findings that were real are fixed by measurement, not by assertion. The fourth was mine and it was wrong.
Deferred on purpose, so it is a decision and not an oversight: the 8 names are absent from docs/fern/pages/reference/observability/metrics-catalog.mdx, and neither DYN_CACHE_REUSE_HISTORY_ENABLED nor DYN_CACHE_REUSE_HISTORY_BYTES is documented outside the source. Both are cheap follow-ups and neither blocks this change.
I authored no commits on this pull request.
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Re-reviewed at b9006851c. My earlier approval was for df8cce0c3, so I am re-approving at this SHA rather than letting the old one carry.
The merge base is still f1cb3ae6, so the PR-scoped delta is now +693/-25 across 13 files. cache_history.rs grew from 252 lines to 294. Comparing against the merge base and comparing df8cce0c3..b9006851c give the same answer: this commit touches one file and nothing else moved.
What the 53 added lines are
A test alone does not account for 53 lines, so I split the file at mod tests and diffed the two halves separately.
Outside the tests the commit changes exactly two lines:
-pub(crate) const CACHE_HISTORY_ENABLED_ENV: &str = "DYN_CACHE_REUSE_HISTORY_ENABLED";
+pub(crate) const CACHE_REUSE_FUNNEL_F1_ENABLED_ENV: &str = "DYN_CACHE_REUSE_FUNNEL_F1_ENABLED";
- dynamo_runtime::config::env_is_truthy(CACHE_HISTORY_ENABLED_ENV)
+ dynamo_runtime::config::env_is_truthy(CACHE_REUSE_FUNNEL_F1_ENABLED_ENV)
The title says "clarify configuration" and this is a rename of the opt-in flag, so I treated it as a behavior change and checked it as one.
- No parse arm, bound, default, capacity rule, or eviction rule moved.
from_env,with_byte_budget,record_completed,previously_computed_tokensandstatsare byte-identical todf8cce0c3. The history is still bounded atcapacity_bytes / ESTIMATED_BYTES_PER_ENTRYentries with evict-oldest at capacity, which is the property I measured in round one. - The rename is complete.
DYN_CACHE_REUSE_HISTORY_ENABLEDandCACHE_HISTORY_ENABLED_ENVappear nowhere in the tree. The PR description already carries the new name. - The rename takes effect, checked by execution. I scraped the registry in-process. With
DYN_CACHE_REUSE_FUNNEL_F1_ENABLED=1all 8cache_lossmetric families register. WithDYN_CACHE_REUSE_HISTORY_ENABLED=1none register, which is the same as setting nothing.
The old name never shipped, so no operator can be relying on it. I am not raising the rename as a finding.
The rest of the 53 lines are the CapturedLogs writer, the history_with_captured_logs helper, the rewritten invalid_byte_budget_warns_and_uses_default, and the new valid_or_unset_byte_budget_does_not_warn control.
Mutation test on the new test
Detail is on the thread. In short: reverting only the tracing::warn! arm at b9006851c makes invalid_byte_budget_warns_and_uses_default fail with exit status 101, and restoring it gives 10 passed and exit status 0. The same test grafted onto 3feafefb34, the last tree without the warning, also fails. The control passes on every tree, which is what a control must do.
Feature-off differential at this head
Re-run rather than carried forward, because this commit touches a file the earlier work touched.
| run | trace sha256 |
|---|---|
base f1cb3ae6, ttl policy, feature off |
c650bbfd...e18098 |
head b9006851c, ttl policy, feature off (3 runs) |
c650bbfd...e18098 |
head b9006851c, ttl policy, DYN_CACHE_REUSE_FUNNEL_F1_ENABLED=1 |
c650bbfd...e18098 |
base f1cb3ae6, lru policy, feature off |
c650bbfd...e18098 |
head b9006851c, lru policy, feature off |
c650bbfd...e18098 |
Indexer actor stats, base equal to head at three rank capacities:
1024 blocks requests=68 request_messages=264 output_batches=64 fallback_activations=0 resident=144 evicted=0
4 blocks requests=68 request_messages=264 output_batches=64 fallback_activations=0 resident=16 evicted=128
0 blocks requests=68 request_messages=136 output_batches=0 fallback_activations=4 resident=0 evicted=0
Both controls, re-run at this head:
- Routing control. Recording each decision against
worker.worker_id ^ 1moved the trace sha256 offc650bbfd...e18098. The perturbed digest is not stable between runs, because the control destroys the strict-best property that makes the clean trace deterministic. Detection is the point, and it detects. - Gate control. Forcing
self.approximate_lru_active = falsemoved the actor stats fromrequest_messages=264 output_batches=64 resident=144to136 / 0 / 80.
What I did not re-run
I skipped the full metric name, label and codegen audit. The diff touches no metric name, no label, no registration site, and neither prometheus_names file. I did re-run the registration scrape, because the gate that decides whether those 8 families register is the line this commit changed.
Re-approving at b9006851c
Zero P0, zero P1, zero P2, zero P3 outstanding. My one open P3 from the previous round is verified and resolved.
Still deferred on purpose, unchanged from the last round: the 8 metric names are absent from docs/fern/pages/reference/observability/metrics-catalog.mdx, and neither DYN_CACHE_REUSE_FUNNEL_F1_ENABLED nor DYN_CACHE_REUSE_HISTORY_BYTES is documented outside the source. Both are cheap follow-ups and neither blocks this change.
I authored no commits on this pull request.
|
/ok to test bfc7d0d |
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at 912abc73bd.
I read the delta from e97a63309b: the replica sync in fddb7e17bf, the test fix in bfc7d0d3ae, and the merge of main in 912abc73bd. bfc7d0d3ae applies my own suggestion as written, so on that line I am partly reviewing my own change.
What I ran, on x86_64 Linux with rustc 1.96.1:
- The prefix test fix holds. The
"rtr_"prefix mutation now fails the test, and it passed ate97a63309b. - The 17
cache_historytests pass at the head and at the head merged with the currentmaintipa83ba19b4e. At that merge, all 406kv_router::tests pass, andcargo metadata --lockedpasses for all three lock files. - Six mutations of the sync code each fail a test: no self filter, no domain check, rebroadcast through
record_completed, no chunking, a publisher on another subject, and no publish. - The flags gate the sync as the description says. A
cache-history-v1publisher registers only when F1 androuter_replica_syncare both on.
I read the one merge conflict, in RequestGuard::abort, but did not probe it. abort() now calls abort_with_error, which records the incomplete observation before cleanup. finalize_cache_history takes the tracking state, so each exit records at most one observation.
Two P3s, inline:
sync.rs:73: while the replica transport is down, every completed request logs a WARN.sync.rs:86: no test covers therouter_replica_syncgate.
Deferred from earlier rounds, not findings: the two environment variables are still documented only in code, and the metric names are still not in the metrics catalog.
|
Renamed F1 to |
8d856d5 to
1bb4eea
Compare
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at 1bb4eea8ad. The F1 rename is complete, and the old name never reached main or a release.
I read the one new commit. It renames the F1 counter to dynamo_component_router_kv_history_cached_prefix_tokens_total in the Rust constant, the registration, the generated Python module, and the name test. The name is the same as the F1 name in the #14103 metrics catalog. The test lines come from my earlier suggestion, so there I am partly reviewing my own change.
Open from my last review, and this commit does not touch them:
- [P3]
sync.rs:73: while the replica transport is down, every completed request logs a WARN. - [P3]
sync.rs:86: no test covers therouter_replica_syncgate.
Deferred from earlier rounds, not findings: the two environment variables are documented only in code, and the metrics catalog does not list the cache-history metrics.
Measured: a wrong name fails the drift test and the name test.
Commands, on macOS arm64 with rustc 1.96.1, at 1bb4eea8ad:
cargo test -p dynamo-codegen
cargo test -p dynamo-runtime --lib metrics::prometheus_names
| Tree | sha256 of the edited file | gen-python-prometheus-names tests |
metrics::prometheus_names tests |
|---|---|---|---|
1bb4eea8ad |
no edit | ok. 3 passed |
ok. 8 passed |
line 137 of prometheus_names.py set to the old name |
50c995ae0cd2e87c |
FAILED. 2 passed; 1 failed |
not run |
| the Rust constant set to the old name | 6c0a98ee2c4b2e14 |
FAILED. 2 passed; 1 failed |
FAILED. 7 passed; 1 failed |
control: METRIC_PREFIX set to "rtr_" |
9c12088b002a0f68 |
not run | FAILED. 7 passed; 1 failed |
The generator has no --check mode. Its drift test is committed_python_matches_generated. The hand edit fails it, and the message starts with prometheus_names.py is out of sync with prometheus_names.rs at line 137. The generator, run with --output to a scratch file, writes a file that cmp finds identical to the committed module. The old name fails the name test with left: "dynamo_component_router_cache_loss_history_prefix_tokens_total". Every restore returned each file to its baseline checksum, and both tests passed again after the restore.
In CI, Pre Merge passed on this head. Its rust-tests (.) job ran committed_python_matches_generated and test_cache_history_prefix_metric_name, and both passed.
Measured: the counter renders under the new name, and only with the flag on.
I added a probe test to metrics.rs. It builds a process-local DistributedRuntime, calls RouterRequestMetrics::from_component, and records one complete request with 100 prompt tokens and 64 F1 tokens. Then it prints the exposition text. Each row is one process, because ROUTER_REQUEST_METRICS is a OnceLock. The build is cargo test -p dynamo-llm --no-default-features --lib.
| Tree | DYN_ROUTER_CACHE_REUSE_HISTORY |
router_kv_history_cached_prefix_tokens_total |
router_cache_loss_history_prefix_tokens_total |
other cache_loss samples |
control: router_requests_total |
|---|---|---|---|---|---|
1bb4eea8ad |
true |
1 sample, value 64 | 0 | 9 | 1 |
1bb4eea8ad |
unset | 0 | 0 | 0 | 1 |
| the Rust constant set to the old name | true |
0 | 1 sample, value 64 | 9 | 1 |
Each name in the table starts with dynamo_component_. The component registry and the runtime registry give the same counts.
Measured: the earlier F1 names never reached main, a tag, or a release branch.
I read every version of the three name files. They are lib/runtime/src/metrics/prometheus_names.rs, lib/llm/src/kv_router/metrics.rs, and lib/bindings/python/src/dynamo/prometheus_names.py.
| Scope | Versions read | Versions with cache_loss, history_prefix or kv_history_cached |
|---|---|---|
every commit on main that touched the three files, 102 commits up to 0ce693b3b0 |
158 | 0 |
| all 96 tags and all 85 release branches, 164 of which hold at least one of the files | 78 | 0 |
control: prometheus_names.rs at 912abc73bd |
1 | 1, with cache_loss 10 times |
control: prometheus_names.rs at 1bb4eea8ad |
1 | 1, with kv_history_cached 3 times |
lib/llm/src/kv_router/cache_history.rs has no commit on main either. So neither earlier name, cache_loss_history_prefix_tokens_total or cache_loss_funnel_tokens_total{stage="f1"}, ever reached main or a release.
In the head tree, the new name is in 3 files: prometheus_names.rs, prometheus_names.py, and metrics.rs. The old name is in no file. Outside those 3 files, no dashboard, Grafana JSON, doc, test, or Python code names either one.
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at 8f67efceb4. This commit fixes both of my open P3 findings, and I found no new defect in it.
I read the one new commit. It changes the level of one log line in sync.rs and adds one test to routing_host/tests.rs. Both changes follow what I asked for, so here I am partly reviewing my own advice.
- [P3, fixed]
sync.rs:73: a dropped peer update now logs at TRACE. The retry warning keeps its WARN level and its backoff. The evidence is in my reply on that thread. - [P3, fixed]
sync.rs:86: the new testcache_history_replica_sync_requires_both_flagscatches a gate that never opens and a gate that opens withoutrouter_replica_sync. The evidence is in my reply on that thread.
Deferred from earlier rounds, not findings: only the code documents the two environment variables, and the metrics catalog does not list the cache-history metrics.
In CI, rust-tests (.) in Pre Merge passed on the merge of this head with main at 887a6c1251. It ran the new test, and the dynamo_llm unit tests gave ok. 3063 passed; 0 failed; 3 ignored.
8f67efc to
6d171d2
Compare
github-actions
Bot
added
the
frontend
|
Refreshed against main in 6d171d2. The conflict was between adjacent test modules in request_guard.rs; both the cache-history tests and the new output-block regression tests are retained. F1 behavior and metric names are unchanged. Formatting and diff checks pass locally; fresh Rust checks and the full CI matrix are running. |
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at 6d171d27b9. The refresh keeps the F1 change that I approved at 8f67efceb4, and the change works with #14613 on main.
- [P3] This commit also carries a copy of #15306. The merge does not change its two files, but
responses/mod.rsbrings adynamo-frontend-codeownersreview request into this PR. Please rebase ontomainat50889c0e37or later, so that the diff shows only the F1 change.
Measured: the two files equal main, and only one of them routes to the frontend team.
responses/mod.rs and responses_http_replay.rs at this head have the same blobs as main at 50889c0e37, which is the merge commit of #15306. They show in this diff because the parent of this commit is 3aa1d3d1d5, the parent of 50889c0e37.
Of the 17 files in this diff, .github/codeowners/who_owns.py gives only responses/mod.rs to @ai-dynamo/dynamo-frontend-codeowners. When you pushed this head at 04:06:11Z, GitHub requested that team. main requires code-owner review.
What I measured on this head.
The 15 F1 files carry the same change as at 8f67efceb4. The added and removed lines are the same, in the same order. When I replay the old change onto main at 50889c0e37, only request_guard.rs has a conflict. There, #14613 adds its output_block_tests module at the same place as your cache_history_tests. This head keeps both modules complete.
This head has the same tree as its merge with main at 50889c0e37. I ran the tests on x86_64 Linux with rustc 1.96.1 and --no-default-features:
| Suite | main at 50889c0e37 |
This head |
|---|---|---|
dynamo-llm lib |
2794 tests | 2812 tests: 2811 passed, 1 ignored |
responses_http_replay |
23 tests | 23 tests: 23 passed |
dynamo-runtime lib |
850 tests | 851 tests: 849 passed, 2 ignored |
dynamo-codegen, with the Python names check |
5 tests | 5 tests: 5 passed |
The 18 new dynamo-llm tests and the 1 new dynamo-runtime test are the tests that this PR adds.
Each mutation below fails a test at this head. The restored files have the same sha256 as the commit, and they pass.
| Change at this head | Test | Result |
|---|---|---|
sync.rs:86 set to if true { |
cache_history_replica_sync_requires_both_flags |
FAILED, did not follow the enable flags |
sync.rs:86-88 deleted |
cache_history_replica_sync_requires_both_flags |
FAILED, history=true, sync=false |
SUBJECT set to "cache-history-v9" |
cache_history_replica_sync_requires_both_flags |
FAILED, did not follow the enable flags |
request_guard.rs:606 set to let num_blocks = 1;, which undoes #14613 |
the 5 output-block tests of #14613 | 4 FAILED |
With DYN_ROUTER_CACHE_REUSE_HISTORY=true, the #14613 test output_block_accounting_tracks_grouped_chunks passes, with the same block counts as with history off. A probe sent the same chunks of 1, 32 and 15 output tokens after a 16-token prompt, with history on and off. The router booked 3 output blocks in both cases. With history on, the history kept 3 hashes: the prompt block and the 2 output blocks that the 32-token chunk completed. The block of the last token does not count yet, because CanonicalOutputTracker on main counts an output block only after one more token follows it. A probe mutant that keeps only the first block of each chunk kept 2 hashes.
I also sent 49 output tokens, as chunks of 1, 32 and 16 and as one chunk. In both cases, the router booked 4 output blocks and the history kept 4 hashes. The probe mutant kept 3 and 2 hashes.
In CI, rust-tests (.) in Pre Merge passed on merge commit 10b3d08, which has the same tree as this head. dynamo_llm gave ok. 3095 passed; 0 failed; 3 ignored, and each test named above passed there.
Deferred from earlier rounds, not findings: only the code documents the two environment variables, and the metrics catalog does not list the cache-history metrics.
✅ Dynamo PR CI passed — run 36900869520 (attempt 1) on
|
Rebase onto main at 50889c0 to exclude the already-merged Responses API changes. Preserve the reviewed source tree unchanged. Signed-off-by: Chanh Nguyen <chanhnguyen@gmail.com>
6d171d2 to
a978c79
Compare
|
Addressed the rebase request in a978c79: the commit now sits on main at 50889c0, and the diff contains only the 15 F1 files. The two already-merged Responses API files are no longer in the diff. Verified that the entire source tree is identical to the reviewed 6d171d2 tree, including both cache-history and output-block regression tests. Fresh CI is pending. Replying to #14889 (review) |
F1 (kv_history_cached_prefix_tokens_total) comes from ai-dynamo#14889 and is not registered by this change, so the catalog should not list it yet. Signed-off-by: PeaBrane <yanrpei@gmail.com>
- F1: return to upstream ai-dynamo#14889 completion semantics. Abort and drop always finalize the attempt as incomplete, so a multi-choice request whose other choice fails after a Stop, or a consumer that stops after the terminal item, never teaches the history (stricter than admission; documented). - Query-only (advisory) selections record their sampled decision trace on the request tracker instead of discarding it. - Output-hash capture hashes each complete block in place and reuses the buffer. - Decision-trace setting is resolved by `decision_trace_setting(get_env)`, tested against the environment cases; selectors take the process value. - Tests: multi-choice failure after Stop through the stream monitor, concurrent gauge publication, F1 lifecycle with the upstream semantics. - Docs: per-router histories behind process-wide F1 series; when an attempt counts as complete. Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Signed-off-by: Manoj Kilaru <mkilaru@nvidia.com>
`CacheHistory::insert` returned early, without the FIFO lock, when every hash was already present. A concurrent insert could evict one of those hashes between the check and the return, so a just-completed prefix was not re-inserted (inherited from upstream ai-dynamo#14889). Membership is now checked only under the FIFO lock; completions almost always add generated blocks, so that lock was already taken on nearly every call. Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Signed-off-by: Manoj Kilaru <mkilaru@nvidia.com>
This branch was successfully deployed
2 participants
Summary: share completed history between router replicas
Validation: peer-history update
mainat6e21a3a646,cargo fmt --all -- --check,git diff --check, and 17 focused history/lifecycle tests passed against the resolved tree.fddb7e17bfwith the companion F2–F5 change under mirrored production traffic and controlled cold/warm/churn workloads. It processed 6,233,943 F0 tokens; all three frontends and both workers contributed, all controlled requests completed, and all funnel counters reported zero resets. At the measurement point F1 was 962,384 tokens (15.44% of F0). This validates stable peer-enabled operation and funnel behavior, not isolated publication/subscription latency.What this adds
F1 answers one simple question: how many prompt tokens belong to complete blocks whose identifiers this router has seen before? F1 is published as
dynamo_component_router_kv_history_cached_prefix_tokens_total, with no stage label. This replacesdynamo_component_router_cache_loss_funnel_tokens_total{stage="f1"}. The F0 series and all accounting behavior are unchanged by the rename.This telemetry is opt-in.
DYN_ROUTER_CACHE_REUSE_HISTORYturns it on; it is off by default. When it is off, the router does not create the history, keep extra hashes, track generated output, or run the F1 lookup.Why F1 is a separate PR
F1 needs a bounded history in the router, so its review is mainly about memory use, lookup cost, and lock contention. F2–F5 use worker-reported data and have a different cost profile. Splitting F1 from #14103 lets reviewers evaluate those two overhead surfaces independently.
How it works
DYN_ROUTER_CACHE_REUSE_HISTORY_BYTESsets the history budget. The default is 256 MiB for each independent KV router/indexer history, enough for an estimated 8,388,608 distinct identifiers using the conservative 32-byte planning estimate.What happens to a request
Test and overhead results
Functional validation on ARM
F0=1,452,F1=960, 3 complete observations, 0 incomplete observations, populated history gauges, and no F2–F5 seriesF0=13.22M,F1=0.903M(6.83%), no F2–F5 series, zero request errors, and zero process restartsLookup microbenchmark
The release-build benchmark uses the same 64-set lookup and FIFO behavior as this PR, with the history filled to 8.39 million entries. It ran locally on x86; it is not a target-host latency measurement.
Measured router throughput
This release-mode Rust benchmark sends prebuilt requests through the real in-process
RoutingHost::generatepath. It includes KV worker scoring, exact admitted dispatch, response-stream finalization, and F1 completion accounting. A fixed fake discovery snapshot supplies workers and an immediate backend removes HTTP, NATS, and model execution.The table is the 26-worker, concurrency-one, duplicate-heavy case: 10,000 measured requests after 1,000 warmups. Each result is the median of six runs split across opposite off/on orders on two pinned x86 logical CPUs.
The full 48-cell sweep also covered concurrency 1/8/32, 26/1,000 workers, and duplicate-heavy/all-new traffic. At 1,000 workers, existing worker scoring dominates: the planned 34-block throughput change ranged from +0.69% to -2.76% across the six concurrency/workload cells. The current four-block shape ranged from +1.26% to -5.08%; that spread, including apparent wins, is the local noise envelope rather than evidence that F1 improves throughput.
Completion pressure and retention
Duplicate-only completions bypassed the FIFO exactly as designed: after 11,000 repeated requests, enabled histories retained only 4/34/417/834 identifiers. All-new runs retained exactly
(warmup + measured requests) × blocksuntil capacity.The best F1-on all-new rates observed through the real 26-worker router path were 28,139 completions/s (112,558 new blocks/s) at 4 blocks, 23,568/s (801,316 blocks/s) at 34 blocks, 7,937/s (3.31M blocks/s) at 417 blocks, and 4,761/s (3.97M blocks/s) at 834 blocks. These include all router lifecycle work and are not isolated FIFO ceilings.
The direct completion microbenchmark remains useful as a lock-pressure bound: 210.5k/31.7k/3.09k/1.56k all-new completions/s at 4/34/417/834 blocks. Current Kimi traffic adds about 1.55 new identifiers/s, orders of magnitude below either measurement.
At that observed new-identifier rate, the default history is modeled to retain about 62.8 days with the current 12,288-token block size. If the rate grows linearly with the planned 8× increase in block count, modeled retention is 7.85 days at 1,536 tokens. These are estimates, not direct long-duration measurements.
Limitations
Where reviewers should start
lib/llm/src/kv_router/cache_history.rs: the 64 sets, byte limit, insertion, and eviction behavior.lib/llm/src/kv_router/routing_host/kv.rs: reuse of the block identifiers already computed during routing.lib/llm/src/kv_router/routing_host/request_guard.rs: exactly-once completion/incomplete accounting and generated-block tracking.Related issue
Validation
Metric-name revision: all 8 tests in the metric-names Rust module passed using an isolated
rustc --testbuild against cached dependencies. Regenerated the Python constants and verified the new constant. Changed-file rustfmt andgit diff --checkpassed. No full LLM rebuild or new shadow rollout for this naming-only revision.cargo fmt --all -- --checkgit diff --checkcargo test -p dynamo-llm --no-default-features cache_history --lib(10 passed)cargo test -p dynamo-llm --no-default-features output_hash_tests --lib(5 passed)Summary by CodeRabbit
New Features
Observability
F1 naming alignment
Renamed the F1 counter to
dynamo_component_router_kv_history_cached_prefix_tokens_totalin 1bb4eea. Semantics and enable flag are unchanged. Regenerated Python constants; all eight metric-name module unit tests pass in an isolated harness. Dashboard queries prefer the new name and retain the old name as a fallback. No new shadow deployment for this rename.