Repository navigation
feat(proxy): expose target pool tuning, retry windows and request timeouts - #91
Merged
Merged
Conversation
…eouts
Nine flags with no deploy.yml key, including half of a timeout pair.
proxy:
request_timeout: 30
path_request_timeouts:
/uploads: 600
target:
max_conns: 100
max_idle_conns: 10
idle_conn_timeout: 90
dial_timeout: 5
disable_keep_alives: false
try_duration: 30
try_interval: 1
response_timeout and request_timeout are different deadlines and the docs now
say so side by side: the first stops once response headers arrive, the second
covers the whole request including streaming the body back. Having only the
first reachable was worse than neither -- an operator sets it, watches requests
hang anyway, and has no key for the one that would help.
path_request_timeouts is built by the same path_timeout_args as path_timeouts,
parameterised rather than copied, because kamal-proxy reads both with one
parser. That method now also rejects a negative duration, which it did not
before; leaving the older caller unchecked would have meant one of the two maps
validating and the other not.
On AC3: .compact was never the hazard. It drops nil only, 0.blank? is false, and
seconds_duration(0) is "0s" because it guards on `if value` and 0 is truthy in
Ruby. Nothing was dropping zeros. The tests pin it anyway -- that is what the AC
asks for and it is exactly the invariant a later refactor breaks quietly.
The thing that DOES bite is what a zero means, which is not uniform:
max_conns: 0 unlimited
try_duration: 0 a single attempt
request_timeout: 0 no limit
max_idle_conns: 0 the proxy's default of 100, NOT "keep none"
idle_conn_timeout: 0 / dial_timeout: 0 / try_interval: 0 proxy defaults
e1e5c10 resolves those server-side on purpose, because restored state and older
RPC clients bypass the CLI entirely. So the gem passes every value through
unchanged rather than substituting defaults of its own, and the docs list what
each zero does -- an operator writing max_idle_conns: 0 to mean "keep none" gets
100 instead, and no gem-side cleverness can fix that without inventing a
sentinel the proxy would not understand.
Closes #80
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
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.
Summary
The target connection pool, the retry window, and the missing half of the timeout pair.
lib/kamal/configuration/proxy.rb— atarget_optionsgroup,request-timeout, andpath_timeout_argsparameterised to serve both maps.lib/kamal/configuration/validator/proxy.rb— negatives and the retry pairing.lib/kamal/configuration/validator.rb—path_request_timeoutsjoins the free-form-key branch.test/proxy_flag_coverage_test.rb— issue-80 waiver deleted; all nine flags proven emitted.Closes #80
Two deadlines, now both reachable
response_timeout→--target-timeoutrequest_timeout→--request-timeoutThis pair is why the issue exists. A slow trickle of body bytes never trips
response_timeout, because the app answered promptly — so an operator who only had the first key watched requests hang past it with nothing to reach for. WebSocket and event-stream responses are exempt fromrequest_timeout.AC3:
.compactwas never the hazard — but zeros still are.compactdropsnilonly,0.blank?isfalse, andseconds_duration(0)returns"0s"because it guards onif valueand0is truthy in Ruby. Nothing was dropping zeros. The tests pin it regardless — that is what the AC asks for, and it is exactly the invariant a later refactor breaks silently.What does bite is that a zero does not mean the same thing across these keys. From
target_pool.go's resolver:0meansmax_connstry_durationrequest_timeoutmax_idle_connsidle_conn_timeoutdial_timeouttry_intervale1e5c10resolves those server-side deliberately — "restoring saved state, rolling out, and older clients over RPC all bypass the CLI" — so the gem passes every value through unchanged rather than substituting defaults of its own. The docs list what each zero does, because an operator writingmax_idle_conns: 0to mean "keep none" gets 100, and the gem cannot fix that without inventing a sentinel the proxy would not understand.Test plan
bundle exec rubocop --parallel— clean, 215 filesbuilder_test/build_test, identical on pristinedashHEAD (host-arch dependent, pass in CI)request_timeout: 5and confirm a slow-streaming response is cut off, whileresponse_timeoutalone does not cut ittarget.max_conns: 1and confirm requests queue rather than failtarget.disable_keep_alives: trueand confirm one connection per request against the apptarget.try_durationretries an idempotent request onto another target during a rolling deployAutomated coverage
response_timeout and request_timeout are separate flagsboth path timeout maps are built by the same code pathan explicit zero reaches the proxy rather than being droppedan explicit zero survives all the way to the command lineoptionize, not just the options hashno tuning keys leave the deploy command unchangeddisable_keep_alives false emits nothingtry_interval without try_duration/with a zero try_durationvalidateRetries, including the zero caseTargetOptions.ValidateandparsePathTimeoutsevery kamal-proxy deploy flag is exposed or waivedDeviations & judgment calls
Deviations
Judgment calls
request_timeoutis rejected when negative even though kamal-proxy does not check it.TargetOptions.Validatecovers the pool knobs andvalidateRetriesthe retry pair, but nothing guardsRequestTimeout. A negative deadline is nonsense, and Go would read it as already-expired.path_timeoutsgains negative validation it never had. It is the same method now servingpath_request_timeouts; leaving the older caller unchecked would mean one map rejects a bad value and the other does not. In path, per the refactor-depth rule — but it is a new rejection for an existing key, so worth a look.disable_keep_alives: falseemits nothing rather than--target-disable-keep-alives=false. The proxy default is already false, so they are identical on the wire and the shorter command reads better in an audit log.path_request_timeoutshad to joinpath_timeoutson the validator's free-form-key branch, and its docs example leads with a Go duration string so that both string and integer values type-check — same trick the existingpath_timeoutsexample relies on.