bugfix: log routing and sticky session decisions

[thushan]

Affinity and routing outcomes were only set on X-Olla-* response headers, so logs gave no way to see which backend a request landed on or why. The completion log now carries sticky_outcome, the routing strategy/action/reason, provider_model and the translation fallback reason. session_id and sticky_source stay at DEBUG since the session id is client-supplied.

Also took made the sticky session test is now a self-contained skill rather than a loose markdown file plus bash harness.

addresses issue #178.

Summary by CodeRabbit

Release Notes

• New Features

  • Expanded structured “Request completed” logging for sticky-session behaviour, including routing decision details, sticky outcome/source, client session identifiers, and translator fallback reasons.
  • Alias routing logs now report the actual backend models selected.

• Tests

  • Added/updated assertions to verify sticky-session, routing, fallback, session ID, and alias-related log fields appear (or are omitted) correctly.

• Chores

  • Removed the legacy manual sticky-session test scripts/targets and streamlined the associated Makefile entries.

[coderabbitai]

Walkthrough

Extends proxy and translation request handlers to capture sticky-session routing metadata (sticky_outcome, sticky_source, session_id, fallback_reason, routing_strategy) from context and emit them to structured INFO/DEBUG logs. Adds unit tests validating emission rules. Removes the manual bash sticky-session test scripts and Makefile target, replacing them with a structured Claude skill document.

Changes

Sticky Session Metadata in Request Logging

Layer / File(s)	Summary
proxyRequest struct extension and context capture internal/app/handlers/handler_proxy.go, internal/app/handlers/handler_translation.go, internal/app/handlers/handler_provider_common.go	Adds stickyOutcome, stickySource, sessionID, and translatorFallbackReason fields to proxyRequest. proxyHandler, providerProxyHandler, and translation handlers call pr.captureStickyOutcome(ctx, r) before logging the request result to read sticky outcome and X-Olla-Session-ID header. Translation handler sets pr.translatorFallbackReason from the resolved translation fallback reason.
INFO/DEBUG log field emission and alias resolution internal/app/handlers/handler_proxy.go, internal/adapter/proxy/olla/service_retry.go	logRequestResult emits provider_model, routing_strategy, routing_action, routing_reason, sticky_outcome (with noise suppression for "disabled"), session_id, and fallback_reason at INFO; buildLogFields emits sticky_source (excluding "none") and session_id at DEBUG; resolveAliasEndpoints includes a deduplicated actual_models list in the alias resolved log; service_retry.go emits a DEBUG log when an alias rewrite changes the backend model.
Unit tests for log field emission rules internal/app/handlers/handler_proxy_logging_test.go, internal/app/handlers/handler_provider_test.go	capturingLogger harness and makeTestPR helper support eight tests covering sticky_outcome present/omitted, routing decision fields, provider_model, fallback_reason present/omitted, session_id appearance, and actual_models in alias resolution logs. Provider handler regression test (TestProviderProxyHandler_StickyFieldsInCompletedLog) validates sticky fields are emitted in the completed log.

Test Infrastructure Migration: Bash → Claude Skill

Layer / File(s)	Summary
Bash scripts and Makefile target removal test/scripts/sticky/run-manual.sh, test/scripts/sticky/test-sticky-provider-routes.sh, makefile	run-manual.sh and test-sticky-provider-routes.sh are deleted; Makefile removes test-sticky-manual from .PHONY and its target definition, adds a dedicated .PHONY block for mock-up, mock-down, mock-status, and mock-logs, and updates help output.
Claude skill document replacing the bash harness .claude/skills/test-sticky-sessions/SKILL.md	Adds SKILL.md defining seven phases: pre-flight Docker and port availability checks, AIMock startup with per-instance backend markers, olla-sticky launch and health polling with bounded retry, per-route miss→hit→diversity assertions across 30 session IDs, sticky stats verification via /internal/stats/sticky, log-field assertions for sticky_outcome, routing_strategy, and session_id on "Request completed" lines, teardown, and verdict reporting. Supersedes the removed flat .claude/skills/test-sticky-sessions.md file.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• Sticky session ID and balancer outcome not logged — makes debugging affinity routing impossible #178: This PR directly implements the logging enhancements requested in that issue — capturing sticky_outcome, sticky_source, session_id, routing_strategy, and provider_model from request context and emitting them in INFO/DEBUG structured logs, with tests validating the emission and redaction rules.

Possibly related PRs

• thushan/olla#133: This PR builds on the sticky-session context infrastructure introduced there by wiring sticky_outcome, sticky_source, and session_id into the proxy and translation request logging paths.
• thushan/olla#140: This PR modifies providerProxyHandler to capture sticky-session outcomes for completion logging, building directly on the sticky-session infrastructure and context-flow patterns introduced in that PR.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'bugfix: log routing and sticky session decisions' directly and accurately summarises the main changes: adding logging for routing decisions and sticky session outcomes to address issue #178.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch bugfix/issue-178-sticky-logging

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

makefile (1)

21-21: ⚡ Quick win

Phony targets are declared twice: consolidate to avoid maintenance confusion.

The targets mock-up, mock-down, mock-status, and mock-logs appear in both the main .PHONY declaration at line 21 AND in a dedicated .PHONY block at line 367. Whilst Make handles this without error, declaring a target in .PHONY only once improves clarity. Suggest removing them from line 21 and keeping only the dedicated block at line 367 for organisational transparency.

♻️ Proposed consolidation

-.PHONY: run clean build test test-verbose test-short test-race test-cover bench version install-deps check-deps vet test-script-integration test-script-sticky mock-up mock-down mock-status mock-logs test-auth-bearer test-auth-env-fatal test-auth-manual
+.PHONY: run clean build test test-verbose test-short test-race test-cover bench version install-deps check-deps vet test-script-integration test-script-sticky test-auth-bearer test-auth-env-fatal test-auth-manual

This keeps the AIMock targets only in the dedicated .PHONY block at line 367, reducing duplication and improving readability.

Also applies to: 367-367

🤖 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 `@makefile` at line 21, Remove the duplicate phony target declarations by
deleting mock-up, mock-down, mock-status, and mock-logs from the main .PHONY
declaration line that includes targets like run, clean, build, test, and others.
Keep these four targets only in the dedicated .PHONY block reserved for
AIMock-related targets to maintain organizational clarity and eliminate the
duplication.

🤖 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 `@internal/app/handlers/handler_translation.go`:
- Around line 92-95: The context value for constants.ContextStickyOutcomeKey is
being written as *balancer.StickyOutcome type in the injectStickyKeyWithBody
function, but is being read as *domain.StickyOutcome type in both the handler
code around line 92 and another location around line 259. This type mismatch
causes the type assertion to fail silently, preventing the sticky outcome and
source from being populated. Change both type assertions from
*domain.StickyOutcome to *balancer.StickyOutcome to match the actual type being
stored in the context.

---

Nitpick comments:
In `@makefile`:
- Line 21: Remove the duplicate phony target declarations by deleting mock-up,
mock-down, mock-status, and mock-logs from the main .PHONY declaration line that
includes targets like run, clean, build, test, and others. Keep these four
targets only in the dedicated .PHONY block reserved for AIMock-related targets
to maintain organizational clarity and eliminate the duplication.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ab84eb4c-5327-43d9-a27f-54b2cf98097e

📥 Commits

Reviewing files that changed from the base of the PR and between e1e4442 and 4fdacef.

📒 Files selected for processing (9)

• .claude/skills/test-sticky-sessions.md
• .claude/skills/test-sticky-sessions/SKILL.md
• internal/adapter/proxy/olla/service_retry.go
• internal/app/handlers/handler_proxy.go
• internal/app/handlers/handler_proxy_logging_test.go
• internal/app/handlers/handler_translation.go
• makefile
• test/scripts/sticky/run-manual.sh
• test/scripts/sticky/test-sticky-provider-routes.sh

💤 Files with no reviewable changes (3)

• .claude/skills/test-sticky-sessions.md
• test/scripts/sticky/run-manual.sh
• test/scripts/sticky/test-sticky-provider-routes.sh

[coderabbitai]

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 `@internal/app/handlers/handler_provider_test.go`:
- Around line 314-317: The multi-line comment for
TestProviderProxyHandler_StickyFieldsInCompletedLog contains an em dash (—) on
line 316 between "INFO log" and "the same class", which violates repository
style guidelines requiring Australian English with no em-dashes in comments.
Replace the em dash with either a comma or a full stop to maintain consistency
with the documentation rules.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 78e49f8a-0c58-42d7-9cfa-33bbf2b98bf9

📥 Commits

Reviewing files that changed from the base of the PR and between b76ea15 and a03fb37.

📒 Files selected for processing (5)

• internal/app/handlers/handler_provider_common.go
• internal/app/handlers/handler_provider_test.go
• internal/app/handlers/handler_proxy.go
• internal/app/handlers/handler_proxy_logging_test.go
• internal/app/handlers/handler_translation.go

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/app/handlers/handler_translation.go
• internal/app/handlers/handler_proxy_logging_test.go
• internal/app/handlers/handler_proxy.go