Skip to content

perf: improve cache hits and request latency - #154

Merged
samueltuyizere merged 8 commits into
mainfrom
perf/cache-hit-and-request-latency-plan
Aug 25, 2026
Merged

perf: improve cache hits and request latency#154
samueltuyizere merged 8 commits into
mainfrom
perf/cache-hit-and-request-latency-plan

Conversation

@samueltuyizere

@samueltuyizere samueltuyizere commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Implements the approved cache and request-latency improvements in one PR.

  • Adds bounded latency, TTFT, and request-stage metrics.
  • Preserves ordered content blocks and cache directives across normalization and provider transforms.
  • Adds an 8,192-entry token-count LRU cache with reload-safe configuration.
  • Moves SQLite completion telemetry to bounded asynchronous writes.
  • Uses atomic catalog snapshots and one-pass model selection.
  • Adds compatibility filtering so unsupported fallback models are skipped cleanly.

Comment thread internal/handlers/storage_adapter.go
Comment thread internal/handlers/storage_adapter.go
Comment thread internal/transformer/request.go Outdated
Comment thread internal/transformer/request.go Outdated
Comment thread internal/core/normalized.go Outdated
Comment thread internal/token/counter.go Outdated
Comment thread internal/metrics/metrics.go Outdated
Comment thread internal/metrics/metrics.go Outdated
@kilo-code-bot

kilo-code-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Code Review Roast 🔥

Verdict: No Issues Found | Recommendation: Merge

The previous normalizeContentBlock gap is patched — Output now falls back to Content when the latter is empty, and a regression test locks it in. I'm genuinely surprised; usually the second pass reveals new problems, not fewer.

📊 Overall: Like a sequel that's better than the original — rare, but when it happens, you mention it at parties.

Files Reviewed (2 files)
  • internal/core/normalize.go - Previous issue resolved (Output fallback added)
  • internal/core/normalize_test.go - New regression test added
Previous Review Summaries (2 snapshots, latest commit 852cec2)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 852cec2)

Verdict: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
🚨 critical 0
⚠️ warning 0
💡 suggestion 0
🤏 nitpick 1
Issue Details (click to expand)
File Line Roast
internal/core/normalize.go 99 normalizeContentBlock copies every field from the Anthropic wire format EXCEPT the deprecated Output field. For tool_result blocks where Content is nil but Output carries the payload, this silently drops the result. The old normalization used block.TextContent(), which falls back to Output when Content is empty. Now that fallback is gone, and any legacy client still using output instead of content will have its tool results vanish into the void.

🏆 Best part: The Blocks migration is executed cleanly — NormalizedMessage now has a single source of truth with accessor methods, and every call site (validate.go, normalized_bridge.go, router/policy.go, messages.go) was updated consistently. That's the kind of systemic refactor most teams half-ass.

💀 Worst part: The async storage writer's shutdown race and invisible drops were the headline failures of the last review, and they're both fixed now with drainBuffered() and RecordStorageDrop(). The dashboard and health endpoint even expose the drop count. Color me shocked — a bounded queue whose losses are actually observable.

📊 Overall: Like a sports car that finally got a fuel gauge — the engineering was already solid, but now you'll know when you're bleeding analytics instead of running on fumes.

Files Reviewed (22 files)
  • internal/core/normalize.go - 1 new issue
  • internal/core/normalized.go - Previous issue fixed (Blocks migration complete)
  • internal/core/validate.go - Updated to use new accessors
  • internal/gui/assets/app.js - Storage drop warning added
  • internal/gui/assets/index.html - Storage warning element added
  • internal/gui/server.go - StorageDropped exposed in metrics
  • internal/handlers/health.go - Storage drop warnings in health endpoint
  • internal/handlers/messages.go - Stage constants now used consistently
  • internal/handlers/storage_adapter.go - Previous issues fixed (drop counting + drain)
  • internal/metrics/metrics.go - Previous issues fixed (shadowed ring + unused constants)
  • internal/metrics/metrics_test.go - Storage drop test added
  • internal/provider/aws_bedrock.go - Updated to use TextContent()
  • internal/provider/aws_bedrock_test.go - Tests updated for Blocks
  • internal/provider/opencode_go_wireformat_test.go - Tests updated for Blocks
  • internal/provider/opencode_useragent_test.go - Tests updated for Blocks
  • internal/router/policy.go - Updated to use TextContent()
  • internal/server/server.go - Metrics passed to StorageAdapter
  • internal/token/counter.go - Encoding stored as field (hardcoded value remains)
  • internal/transformer/cache_control_test.go - New test for cache control on non-text blocks
  • internal/transformer/normalized_bridge.go - Full Blocks migration
  • internal/transformer/normalized_bridge_test.go - Tests updated for Blocks
  • internal/transformer/request.go - Previous issues fixed (cache control capture)

Fix these issues in Kilo Cloud

Previous review (commit 94319c6)

Verdict: 8 Issues Found | Recommendation: Address before merge

Overview

Severity Count
🚨 critical 0
⚠️ warning 2
💡 suggestion 3
🤏 nitpick 3
Issue Details (click to expand)
File Line Roast
internal/handlers/storage_adapter.go 67 Dropped completion records are only logged, never counted — ADR #1 wants a drop metric + dashboard warning
internal/handlers/storage_adapter.go 89 Shutdown can close(s.stop) and let run() exit before draining the closed queue, silently losing buffered records
internal/transformer/request.go 458 messageCacheControl only captured from text blocks, dropping image/tool_result cache directives (DeepSeek multimodal)
internal/transformer/request.go 543 Assistant path mirrors the gap: thinking block cache control is never captured
internal/core/normalized.go 44 Blocks added alongside legacy fields — ADR #3 said replace, not duplicate
internal/token/counter.go 134 Cache key hardcodes "cl100k_base" instead of the actual encoding
internal/metrics/metrics.go 194 Dead shadowed ring := newDurationRing(...) immediately re-read on line 197
internal/metrics/metrics.go 62 Stage name constants defined but every call site uses string literals — they drift

🏆 Best part: The token-count LRU, the bounded durationRing metrics, the one-pass SelectCheapest min, and the sorted-percentile math are genuinely well-built — the perf plumbing here is the kind of work that survives contact with production. I'm almost annoyed it's this solid.

💀 Worst part: The async storage writer is the headline feature, yet its drop path has no counter and its shutdown can race the drain — a bounded queue whose losses are invisible, and a graceful-shutdown that can quietly drop data. The exact failure modes ADR #1 promised to prevent.

📊 Overall: Like a sports car with no fuel gauge — gorgeous engineering, but you won't know you're bleeding analytics until you're stranded.

Fix these issues in Kilo Cloud

Files Reviewed (38 files)
  • CLAUDE.md, CONTEXT.md, docs/adr/*, docs/howto-add-model.md, docs/cache-and-latency-implementation-plan.md
  • internal/client/opencode_test.go, internal/config/config.go, internal/core/errors.go, internal/core/normalize.go, internal/core/normalize_test.go, internal/core/normalized.go, internal/core/provider.go, internal/core/request_compat.go
  • internal/gui/perf.go, internal/handlers/health.go, internal/handlers/messages.go, internal/handlers/storage_adapter.go, internal/handlers/ttft_test.go
  • internal/metrics/metrics.go, internal/metrics/metrics_test.go, internal/models/classifier.go
  • internal/provider/aws_bedrock.go, internal/provider/opencode_go.go, internal/provider/opencode_zen.go, internal/provider/provider.go
  • internal/router/model_router.go, internal/router/selector.go, internal/server/server.go, internal/storage/latency.go
  • internal/token/counter.go, internal/token/counter_test.go, internal/transformer/normalized_bridge.go, internal/transformer/request.go
  • pkg/types/anthropic.go, pkg/types/openai.go

Reviewed by free · Input: 84.9K · Output: 7K · Cached: 233K

Review guidance: REVIEW.md from base branch main (truncated)

@samueltuyizere

Copy link
Copy Markdown
Collaborator Author

Addressed all eight review threads in commit 852cec2:

  • Count storage queue drops and expose them through /health and /api/metrics, with a visible GUI warning.
  • Drain accepted completion records during shutdown before the worker exits.
  • Preserve cache_control from the first block on user and assistant messages, including image, tool_result, and thinking blocks.
  • Make ordered Blocks the single normalized message representation and update adapters/validation accordingly.
  • Include the tokenizer encoding name in token-cache keys.
  • Remove the shadowed stage ring and use exported stage constants at call sites.
  • Added coverage for cache-control preservation and storage-drop metrics.

Verification: pre-push checks passed (gofmt, go vet, golangci-lint: 0 issues, go test ./... -race, and CGO_ENABLED=0 build). All eight review threads are resolved.

Comment thread internal/core/normalize.go Outdated
@samueltuyizere

Copy link
Copy Markdown
Collaborator Author

Addressed the new review comment in commit adce320. normalizeContentBlock now falls back from an empty Content field to the legacy Output field, preserving old tool_result payloads during the Blocks migration. Added a regression test covering output-only tool results.

Verification: pre-push checks passed again (gofmt, go vet, golangci-lint: 0 issues, go test ./... -race, and CGO_ENABLED=0 build).

@samueltuyizere
samueltuyizere merged commit b64f155 into main Aug 25, 2026
4 checks passed
@samueltuyizere
samueltuyizere deleted the perf/cache-hit-and-request-latency-plan branch August 25, 2026 17:17
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.

1 participant