feature: Anthropic agent fixes and improvements#113
Merged
Conversation
address defaults to be more aligned to streaming fix translator handling bugs for passthrough
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds Olla per-host idle-connection config and wiring, removes manual per-endpoint connection counting in retry paths, improves streaming header synchronization and translator-mode propagation, extends AnthropicRequest with OutputConfig, switches default proxy engine to Olla, and adds multiple transport/streaming/connection-counting tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as TranslationHandler
participant Recorder as streamingResponseRecorder
participant ProxyGo as ProxyGoroutine
participant Upstream
rect rgba(200,100,100,0.5)
Note over Handler,ProxyGo: OLD boolean signalling (risk of blocking)
Client->>Handler: POST /translate (streaming)
Handler->>ProxyGo: spawn proxy goroutine
ProxyGo->>Upstream: forward request
Upstream-->>ProxyGo: response headers
ProxyGo->>Recorder: WriteHeader()/Write()
Recorder->>Handler: set boolean headerSent
Handler->>Client: wait on headerSent or ctx.Done()
end
rect rgba(100,200,100,0.5)
Note over Handler,ProxyGo: NEW sync.Once + headersReady (unblocks reliably)
Client->>Handler: POST /translate (streaming)
Handler->>Recorder: create with headersReady chan
Handler->>ProxyGo: spawn proxy goroutine
ProxyGo->>Upstream: forward request
Upstream-->>ProxyGo: response headers
ProxyGo->>Recorder: WriteHeader()
Recorder->>Recorder: ensureHeadersReady() via sync.Once
Recorder->>Recorder: close(headersReady)
par
Handler->>Handler: select { <-headersReady | ctx.Done() }
and
ProxyGo->>Upstream: read SSE data
ProxyGo->>Recorder: Write() data
end
Handler->>Client: stream data without deadlock
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
internal/app/handlers/handler_proxy.go (1)
175-178: Translator mode logged on success but not on error — consider consistency.The
translator_modeis included in the success path oflogRequestResult(Line 216) and inlogRequestStart(Line 176), but the error path at Line 197–198 delegates tobuildLogFieldswhich doesn't include it. If you're debugging a failed translation request, knowing the mode would be useful.This is minor since the mode is already captured in
logRequestStart.Also applies to: 215-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/app/handlers/handler_proxy.go` around lines 175 - 178, Ensure translator_mode is included in error logs by adding pr.translatorMode to the fields built for error paths: update buildLogFields (or its caller) so it accepts/uses pr.translatorMode (or reads pr.translatorMode) and appends "translator_mode" when non-empty, so both logRequestStart and logRequestResult/error use the same field set; modify the call sites in logRequestResult and the error branch (where buildLogFields is invoked) to pass through translatorMode if needed.internal/app/handlers/handler_translation_passthrough_test.go (2)
1693-1765: Dead variablecapturedMode— leftover from an earlier approach.
capturedModeis declared (Line 1701) and immediately blanked (Line 1732) but never actually written to. The test correctly asserts via the response header instead. Remove the dead code to avoid confusion.Remove dead code
- // Capture the proxyRequest state when logRequestStart fires (i.e. when the - // underlying proxy service is invoked — at that point pr.translatorMode must - // already be set). - var capturedMode constants.TranslatorMode - endpoints := []*domain.Endpoint{- // Intercept the proxyRequest by wrapping executePassthroughRequest via a - // custom translator whose PreparePassthrough stores the mode from the header. - _ = capturedMode - app := &Application{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/app/handlers/handler_translation_passthrough_test.go` around lines 1693 - 1765, The variable capturedMode in TestTranslatorMode_SetOnPassthroughPath is dead/unused; remove its declaration "var capturedMode constants.TranslatorMode" and the no-op "_ = capturedMode" so the test no longer contains unused variables; locate them in the TestTranslatorMode_SetOnPassthroughPath function and delete both occurrences (references to capturedMode in that function are safe to remove).
1767-1843: Translation path test asserts absence of the passthrough header — consider asserting the mode positively.The test confirms the translation path was taken by checking
X-Olla-Modeis empty. This is an indirect assertion — it proves passthrough wasn't used, but doesn't confirmtranslatorModewas actually set toTranslatorModeTranslationon theproxyRequest. If the handler were to add the header for translation mode in future, this test would need updating.That said, this works for the current implementation and the existing passthrough tests already cover the positive case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/app/handlers/handler_translation_passthrough_test.go` around lines 1767 - 1843, Update TestTranslatorMode_SetOnTranslationPath to assert translation mode positively: instead of asserting the X-Olla-Mode header is empty, expect it to equal TranslatorModeTranslation (use constants.HeaderXOllaMode and the TranslatorModeTranslation symbol) so the test explicitly verifies the handler produced translation mode; adjust the handler/test setup if necessary to ensure the translation handler sets that header so the assertion can pass.internal/config/config.go (1)
25-25: Breaking default change — ensure migration path is documented.Changing
DefaultProxyEnginefrom"sherpa"to"olla"silently alters behaviour for any deployment relying on the default (i.e. no explicitproxy.enginein their config file). Consider adding a note in release/migration documentation so operators are aware.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` at line 25, DefaultProxyEngine was changed from "sherpa" to "olla", which is a breaking default change; update project release/migration documentation and add an in-repo note (e.g., in CHANGELOG.md or docs/migration.md) explaining the default change, its impact on deployments that omit proxy.engine, and steps operators should take (explicitly set proxy.engine to "sherpa" to keep previous behavior or to "olla" to adopt the new default). Reference the constant DefaultProxyEngine in internal/config/config.go in the note so operators can find the impacted code and include a brief example config snippet and upgrade checklist.internal/adapter/proxy/olla/service.go (1)
696-710:UpdateConfigdoes not recreate the transport — config drift for pooling fields.When
UpdateConfigis called,s.configurationis swapped buts.transport(and existing per-endpoint pool transports) retain staleMaxIdleConnsPerHost/MaxConnsPerHostvalues. Only new endpoint pools created after the update will pick up the new values. This is consistent with howMaxIdleConnsandIdleConnTimeoutalready behave here, but worth noting if hot-reconfiguration of connection pool sizes is ever expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/proxy/olla/service.go` around lines 696 - 710, UpdateConfig currently swaps s.configuration but leaves s.transport and per-endpoint pool transports unchanged, causing connection-pool-size drift; modify UpdateConfig (and any helper that builds transports) to detect changes to pooling fields (MaxIdleConns, IdleConnTimeout, MaxConnsPerHost, MaxIdleConnsPerHost) and recreate the HTTP transport(s) when those values change: build a new transport from newConfig, atomically replace s.transport with the new transport, close/cleanup the old transport (or call CloseIdleConnections) and iterate existing per-endpoint pool transports (e.g., entries in the endpoint pool map) to recreate or update them so they pick up the new MaxConnsPerHost/MaxIdleConnsPerHost values; keep the atomic swap semantics for s.configuration and ensure proper synchronization around s.transport and endpoint pools during the swap.internal/adapter/proxy/proxy_olla_connection_counting_test.go (1)
95-150: Multi-request net-zero delta test is a solid complement.One minor nit:
neton Line 147 shadows the built-in package name. It's harmless here sincenetisn't imported, but a rename to e.g.deltawould be clearer.Suggested rename
- if net := inc - dec; net != 0 { - t.Errorf("net connection delta is %d after all requests completed; want 0", net) + if delta := inc - dec; delta != 0 { + t.Errorf("net connection delta is %d after all requests completed; want 0", delta) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/proxy/proxy_olla_connection_counting_test.go` around lines 95 - 150, In TestOllaProxy_ConnectionCountReturnsToZero rename the local variable net (used to compute inc - dec) to a non-conflicting name like delta to avoid shadowing Go's net package; update the computation and the subsequent error message to reference delta instead of net so Inc/Dec values (selector.incrementCalls.Load(), selector.decrementCalls.Load()) and the final comparison remain correct.internal/adapter/proxy/olla/service_transport_test.go (2)
36-58: Test name/comment claims to verifyNewServicedefaulting, but the body only exercisescreateOptimisedTransport.The docstring says "verifies that NewService fills in sensible defaults" yet the test manually assigns all fields from config constants and calls
createOptimisedTransportdirectly — it never invokesNewServicewith a zero-value config. This makes the test equivalent toTestCreateOptimisedTransport_ConnectionLimitswith different values and gives a false sense of default-coverage.To actually test defaults, consider calling
NewService(or the function that applies defaults) with a zero-valueConfigurationand then asserting the resulting transport fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/proxy/olla/service_transport_test.go` around lines 36 - 58, TestCreateOptimisedTransport_DefaultsApplied claims to verify NewService defaulting but instead sets cfg fields manually and calls createOptimisedTransport; change the test to construct a zero-value Configuration, call NewService (or the internal apply-defaults helper that NewService uses) to get the service/transport, and then assert transport.MaxConnsPerHost, transport.MaxIdleConnsPerHost, transport.MaxIdleConns and transport.IdleConnTimeout equal the config.OllaDefault* constants; remove the manual assignments to cfg before calling createOptimisedTransport so the test actually verifies default application.
14-34: IdleConnTimeout is set but never asserted.Line 21 sets
cfg.IdleConnTimeout = 90 * time.Secondbut no assertion checks thattransport.IdleConnTimeoutis mapped correctly. Either add the assertion or remove the assignment to avoid giving a false sense of coverage.Suggested fix
if transport.MaxIdleConns != 200 { t.Errorf("MaxIdleConns: want 200, got %d", transport.MaxIdleConns) } + if transport.IdleConnTimeout != 90*time.Second { + t.Errorf("IdleConnTimeout: want %v, got %v", 90*time.Second, transport.IdleConnTimeout) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/proxy/olla/service_transport_test.go` around lines 14 - 34, The test TestCreateOptimisedTransport_ConnectionLimits sets cfg.IdleConnTimeout but never asserts it; update the test to assert that createOptimisedTransport maps cfg.IdleConnTimeout to transport.IdleConnTimeout (compare transport.IdleConnTimeout == cfg.IdleConnTimeout) so the Configuration.IdleConnTimeout is validated, or remove the cfg.IdleConnTimeout assignment if you prefer not to test that mapping; locate the test, the Configuration struct usage and the createOptimisedTransport call to make the change.internal/app/handlers/handler_translation_test.go (1)
1580-1663: Context-cancellation test is well-structured.One nuance: this test only covers the case where the proxy hasn't written anything at all when the context fires. It would also be valuable to test the scenario where the proxy is mid-write (blocked on
pipeWriter.Writebecause nobody is reading the pipe) when the context cancels — that's the goroutine-leak path flagged inhandler_translation.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/app/handlers/handler_translation_test.go` around lines 1580 - 1663, Add a second test variant that simulates the proxy goroutine becoming blocked while writing to the response pipe (e.g., have mockProxyService.proxyFunc call w.Write or pipeWriter.Write and block until ctx is cancelled) to exercise the goroutine-leak path referenced in handler_translation.go; in the existing TestExecuteTranslatedStreamingRequest_ContextCancellationUnblocks flow reuse translationHandler and the mockTranslator but change the proxy to perform a blocking write (or block after a partial Write) so when the request context created with context.WithTimeout is cancelled the handler unblocks and the proxy goroutine returns — ensure the test asserts the handler finishes promptly (like the current select on done vs timeout) and that the proxy goroutine exits when ctx.Done is triggered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/app/handlers/handler_translation_test.go`:
- Around line 1464-1487: Rename the test function
TestStreamingResponseRecorder_EnsureHeadersReady_IdemPotent to
TestStreamingResponseRecorder_EnsureHeadersReady_Idempotent to correct the typo;
update the function declaration and any references (e.g., in go test -run
patterns) to use the new name so CI output and test invocation reflect the
correct spelling for the identifier
TestStreamingResponseRecorder_EnsureHeadersReady_Idempotent.
In `@internal/app/handlers/handler_translation.go`:
- Around line 495-501: When handling the select that waits on
streamRecorder.headersReady or ctx.Done(), ensure the pipe reader is closed
before returning on ctx cancellation to avoid leaving the proxy goroutine
blocked; specifically, in the branch where <-ctx.Done() triggers, call
pipeReader.Close() (and optionally pipeWriter.Close() if not already closed)
before returning the fmt.Errorf so both ends of the pipe are cleaned up and
mirror the panic-path cleanup logic present around the proxy goroutine.
---
Nitpick comments:
In `@internal/adapter/proxy/olla/service_transport_test.go`:
- Around line 36-58: TestCreateOptimisedTransport_DefaultsApplied claims to
verify NewService defaulting but instead sets cfg fields manually and calls
createOptimisedTransport; change the test to construct a zero-value
Configuration, call NewService (or the internal apply-defaults helper that
NewService uses) to get the service/transport, and then assert
transport.MaxConnsPerHost, transport.MaxIdleConnsPerHost, transport.MaxIdleConns
and transport.IdleConnTimeout equal the config.OllaDefault* constants; remove
the manual assignments to cfg before calling createOptimisedTransport so the
test actually verifies default application.
- Around line 14-34: The test TestCreateOptimisedTransport_ConnectionLimits sets
cfg.IdleConnTimeout but never asserts it; update the test to assert that
createOptimisedTransport maps cfg.IdleConnTimeout to transport.IdleConnTimeout
(compare transport.IdleConnTimeout == cfg.IdleConnTimeout) so the
Configuration.IdleConnTimeout is validated, or remove the cfg.IdleConnTimeout
assignment if you prefer not to test that mapping; locate the test, the
Configuration struct usage and the createOptimisedTransport call to make the
change.
In `@internal/adapter/proxy/olla/service.go`:
- Around line 696-710: UpdateConfig currently swaps s.configuration but leaves
s.transport and per-endpoint pool transports unchanged, causing
connection-pool-size drift; modify UpdateConfig (and any helper that builds
transports) to detect changes to pooling fields (MaxIdleConns, IdleConnTimeout,
MaxConnsPerHost, MaxIdleConnsPerHost) and recreate the HTTP transport(s) when
those values change: build a new transport from newConfig, atomically replace
s.transport with the new transport, close/cleanup the old transport (or call
CloseIdleConnections) and iterate existing per-endpoint pool transports (e.g.,
entries in the endpoint pool map) to recreate or update them so they pick up the
new MaxConnsPerHost/MaxIdleConnsPerHost values; keep the atomic swap semantics
for s.configuration and ensure proper synchronization around s.transport and
endpoint pools during the swap.
In `@internal/adapter/proxy/proxy_olla_connection_counting_test.go`:
- Around line 95-150: In TestOllaProxy_ConnectionCountReturnsToZero rename the
local variable net (used to compute inc - dec) to a non-conflicting name like
delta to avoid shadowing Go's net package; update the computation and the
subsequent error message to reference delta instead of net so Inc/Dec values
(selector.incrementCalls.Load(), selector.decrementCalls.Load()) and the final
comparison remain correct.
In `@internal/app/handlers/handler_proxy.go`:
- Around line 175-178: Ensure translator_mode is included in error logs by
adding pr.translatorMode to the fields built for error paths: update
buildLogFields (or its caller) so it accepts/uses pr.translatorMode (or reads
pr.translatorMode) and appends "translator_mode" when non-empty, so both
logRequestStart and logRequestResult/error use the same field set; modify the
call sites in logRequestResult and the error branch (where buildLogFields is
invoked) to pass through translatorMode if needed.
In `@internal/app/handlers/handler_translation_passthrough_test.go`:
- Around line 1693-1765: The variable capturedMode in
TestTranslatorMode_SetOnPassthroughPath is dead/unused; remove its declaration
"var capturedMode constants.TranslatorMode" and the no-op "_ = capturedMode" so
the test no longer contains unused variables; locate them in the
TestTranslatorMode_SetOnPassthroughPath function and delete both occurrences
(references to capturedMode in that function are safe to remove).
- Around line 1767-1843: Update TestTranslatorMode_SetOnTranslationPath to
assert translation mode positively: instead of asserting the X-Olla-Mode header
is empty, expect it to equal TranslatorModeTranslation (use
constants.HeaderXOllaMode and the TranslatorModeTranslation symbol) so the test
explicitly verifies the handler produced translation mode; adjust the
handler/test setup if necessary to ensure the translation handler sets that
header so the assertion can pass.
In `@internal/app/handlers/handler_translation_test.go`:
- Around line 1580-1663: Add a second test variant that simulates the proxy
goroutine becoming blocked while writing to the response pipe (e.g., have
mockProxyService.proxyFunc call w.Write or pipeWriter.Write and block until ctx
is cancelled) to exercise the goroutine-leak path referenced in
handler_translation.go; in the existing
TestExecuteTranslatedStreamingRequest_ContextCancellationUnblocks flow reuse
translationHandler and the mockTranslator but change the proxy to perform a
blocking write (or block after a partial Write) so when the request context
created with context.WithTimeout is cancelled the handler unblocks and the proxy
goroutine returns — ensure the test asserts the handler finishes promptly (like
the current select on done vs timeout) and that the proxy goroutine exits when
ctx.Done is triggered.
In `@internal/config/config.go`:
- Line 25: DefaultProxyEngine was changed from "sherpa" to "olla", which is a
breaking default change; update project release/migration documentation and add
an in-repo note (e.g., in CHANGELOG.md or docs/migration.md) explaining the
default change, its impact on deployments that omit proxy.engine, and steps
operators should take (explicitly set proxy.engine to "sherpa" to keep previous
behavior or to "olla" to adopt the new default). Reference the constant
DefaultProxyEngine in internal/config/config.go in the note so operators can
find the impacted code and include a brief example config snippet and upgrade
checklist.
Comment on lines
+1464
to
+1487
| // TestStreamingResponseRecorder_EnsureHeadersReady_IdemPotent verifies that | ||
| // ensureHeadersReady can be called multiple times without panicking. | ||
| func TestStreamingResponseRecorder_EnsureHeadersReady_IdemPotent(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| pr, pw := io.Pipe() | ||
| defer pr.Close() | ||
| defer pw.Close() | ||
|
|
||
| rec := newStreamingResponseRecorder(pw) | ||
|
|
||
| // Calling multiple times must not panic (sync.Once guards the close). | ||
| rec.ensureHeadersReady() | ||
| rec.ensureHeadersReady() | ||
| rec.WriteHeader(http.StatusOK) | ||
| rec.ensureHeadersReady() | ||
|
|
||
| // Channel must already be closed. | ||
| select { | ||
| case <-rec.headersReady: | ||
| default: | ||
| t.Fatal("headersReady should be closed after ensureHeadersReady") | ||
| } | ||
| } |
There was a problem hiding this comment.
Typo in test name: "IdemPotent" → "Idempotent".
TestStreamingResponseRecorder_EnsureHeadersReady_IdemPotent has an extra capital P — "Idempotent" is a single word. Since test names appear in CI output and go test -run, the inconsistency could be confusing.
-func TestStreamingResponseRecorder_EnsureHeadersReady_IdemPotent(t *testing.T) {
+func TestStreamingResponseRecorder_EnsureHeadersReady_Idempotent(t *testing.T) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TestStreamingResponseRecorder_EnsureHeadersReady_IdemPotent verifies that | |
| // ensureHeadersReady can be called multiple times without panicking. | |
| func TestStreamingResponseRecorder_EnsureHeadersReady_IdemPotent(t *testing.T) { | |
| t.Parallel() | |
| pr, pw := io.Pipe() | |
| defer pr.Close() | |
| defer pw.Close() | |
| rec := newStreamingResponseRecorder(pw) | |
| // Calling multiple times must not panic (sync.Once guards the close). | |
| rec.ensureHeadersReady() | |
| rec.ensureHeadersReady() | |
| rec.WriteHeader(http.StatusOK) | |
| rec.ensureHeadersReady() | |
| // Channel must already be closed. | |
| select { | |
| case <-rec.headersReady: | |
| default: | |
| t.Fatal("headersReady should be closed after ensureHeadersReady") | |
| } | |
| } | |
| // TestStreamingResponseRecorder_EnsureHeadersReady_IdemPotent verifies that | |
| // ensureHeadersReady can be called multiple times without panicking. | |
| func TestStreamingResponseRecorder_EnsureHeadersReady_Idempotent(t *testing.T) { | |
| t.Parallel() | |
| pr, pw := io.Pipe() | |
| defer pr.Close() | |
| defer pw.Close() | |
| rec := newStreamingResponseRecorder(pw) | |
| // Calling multiple times must not panic (sync.Once guards the close). | |
| rec.ensureHeadersReady() | |
| rec.ensureHeadersReady() | |
| rec.WriteHeader(http.StatusOK) | |
| rec.ensureHeadersReady() | |
| // Channel must already be closed. | |
| select { | |
| case <-rec.headersReady: | |
| default: | |
| t.Fatal("headersReady should be closed after ensureHeadersReady") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/app/handlers/handler_translation_test.go` around lines 1464 - 1487,
Rename the test function
TestStreamingResponseRecorder_EnsureHeadersReady_IdemPotent to
TestStreamingResponseRecorder_EnsureHeadersReady_Idempotent to correct the typo;
update the function declaration and any references (e.g., in go test -run
patterns) to use the new name so CI output and test invocation reflect the
correct spelling for the identifier
TestStreamingResponseRecorder_EnsureHeadersReady_Idempotent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses some limitations in the agent flows and fixes logging and some concerns around stream flushes & tooling bugs.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests