feat(api): support timeout_ms on remember and search#40
Conversation
keyoku-bot
left a comment
keyoku-bot
left a comment
There was a problem hiding this comment.
Thanks for wiring timeout_ms through both endpoints — the clamping and request-scoped context are both nice additions.
I'm requesting changes for one behavior issue before we merge: when the request-scoped timeout actually fires, both handlers currently fall through writeInternalErrorWithContext(...), which turns context.DeadlineExceeded into a generic 500 internal server error. Since this PR is explicitly exposing caller-controlled request budgets, timing out on that budget should surface as a timeout/cancelation response (for example 504 with a stable error code), not an internal server failure.
Please add explicit handling for context.DeadlineExceeded / context.Canceled in the remember/search path (or centrally in the error writer), and cover it with a handler test so clients can distinguish budget exhaustion from real server faults.
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.
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.
2 participants
Summary
timeout_mstoPOST /api/v1/rememberandPOST /api/v1/searchWhy
This lets integrations tune extraction/search request budgets explicitly instead of relying only on client abort defaults.
Validation
go test ./cmd/keyoku-server/...