feat(api): map timeout_ms exhaustion to 504 request_timeout (builds on #40) - #43
feat(api): map timeout_ms exhaustion to 504 request_timeout (builds on #40)#43tyejcoleman wants to merge 2 commits into
Conversation
|
Thank you for your contribution! Before we can merge this PR, we need you to sign our Contributor License Agreement. To sign, please comment on this PR with the following text:
I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
There was a problem hiding this comment.
Pull request overview
This PR improves the HTTP API’s error semantics around request-scoped timeouts (timeout_ms) by mapping timeout exhaustion to a stable, retryable 504 error response, and by avoiding response writes when the request context is canceled (client disconnect).
Changes:
- Add request-scoped context timeouts for
/rememberand/searchusingtimeout_ms(clamped to 1s..300s). - Map
context.DeadlineExceeded(including wrapped and string-fallback forms) to HTTP 504 with stablecode: request_timeoutandretryable: true. - Add unit tests covering wrapped deadline exceeded, string fallback, and canceled-no-write behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/keyoku-server/handlers_memory.go | Applies clamped request-scoped timeouts to Remember/Search via context.WithTimeout. |
| cmd/keyoku-server/handlers.go | Adds deadline/cancel detection and maps deadline exhaustion to 504 with stable error code. |
| cmd/keyoku-server/handlers_errors_test.go | Adds tests verifying 504 mapping behavior and cancel no-write behavior. |
| if errors.Is(err, context.Canceled) { | ||
| return true | ||
| } | ||
| return strings.Contains(err.Error(), "context canceled") |
There was a problem hiding this comment.
isCanceled uses strings.Contains(err.Error(), "context canceled"). That can misclassify unrelated errors that merely include this substring, and (via the early return in writeInternalErrorWithContext) can lead to an empty/default 200 response. Consider restricting cancellation detection to errors.Is(err, context.Canceled) (or an exact-match fallback) to avoid swallowing real failures.
| return strings.Contains(err.Error(), "context canceled") | |
| return err != nil && err.Error() == "context canceled" |
7ed4481 to
5070daa
Compare
Builds on #40. When a request-scoped timeout (from client-supplied timeout_ms or an upstream provider deadline) fires, the handler path previously unwrapped to writeInternalErrorWithContext and surfaced a generic 500. Since this endpoint now exposes a caller-controlled budget, budget exhaustion needs a distinct, retryable signal so clients can separate "my budget was too tight" from "the engine is broken". - errors.Is(err, context.DeadlineExceeded) -> 504 with code=request_timeout, retryable=true. String fallback covers provider SDKs that surface "context deadline exceeded" without wrapping. - errors.Is(err, context.Canceled) -> log and return without writing, since the client socket is gone. - Tests cover both wrapped and string-fallback forms plus the canceled no-write case.
5070daa to
d38bf26
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
@moltar-bot — this PR builds on your commit from #40, so the CLA check wants your signature too. Could you comment:
Once you do, the check re-runs automatically and this can merge. Thanks! |
Summary
timeout_mson /remember and /search).code: request_timeout,retryable: true.context.Canceledto log-only (no response written — client is gone).Why
Review on #40 requested this: before this change, hitting the caller-supplied
timeout_msdegraded into a generic 500 viawriteInternalErrorWithContext. Clients couldn't distinguish "my budget was too tight" from "the engine is broken" and had no stable retry signal.Harness spec amendment:
20-contracts/http-api.bridge.mdgets a 504 row in the status table and a stable-error-codes table entry forrequest_timeout. See companion commit in keyoku-harness.Validation
go test ./cmd/keyoku-server/... -run TestWriteInternalError -v— all 6 tests pass including the three new ones (DeadlineExceededMapsTo504,DeadlineExceededStringFallback,CanceledWritesNoResponse).TestHandleHeartbeatContext_*failures on main are unrelated (reproduce on untouchedmain).Supersedes
Closes #40 (this branch includes moltar-bot's commit plus the requested fix).
Test plan
fmt.Errorf("...: %w", context.DeadlineExceeded)→ 504"context deadline exceeded"→ 504context.Canceled→ no body, default 200 status (header not written)/api/v1/rememberwithtimeout_ms=1000against Ollama + qwen2.5:7b repeated same-entity; confirm 504 withcode=request_timeout🤖 Generated with Claude Code