implement last-known-good fallback for dynamic limit settings - #2339
implement last-known-good fallback for dynamic limit settings#2339tarcisiozf wants to merge 4 commits into
Conversation
📊 API Diff Results
|
There was a problem hiding this comment.
Pull request overview
This pull request updates the settings-based limiters to prefer a “last known good” (most recently resolved) value when settings reads fail, instead of always falling back to the compiled default, and adds tests to validate the new fallback behavior.
Changes:
- Added atomic “last known good” storage to the shared
updaterand used it during polling/subscription failures. - Updated bound/gate/range/time limiters to return usable values even when a settings read fails, with the error treated as advisory (except for missing required tenant context).
- Added new unit tests covering last-known-good fallback and compiled-default fallback when no successful read has occurred.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/settings/limits/updater.go | Stores and reuses last known good values on update failures; logging clarified. |
| pkg/settings/limits/time.go | Makes WithTimeout/Limit return usable values/contexts on read failure while returning the error as advisory. |
| pkg/settings/limits/range.go | Returns last known good range on read failure (or compiled default if none yet). |
| pkg/settings/limits/gate.go | Returns last known good gate value on read failure (or compiled default if none yet). |
| pkg/settings/limits/bound.go | Returns last known good bound on read failure (or compiled default if none yet). |
| pkg/settings/limits/default_fallback_test.go | Adds tests validating last-known-good vs compiled-default fallback behavior across limiter types. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fe664ba to
2206913
Compare
tarcisiozf
changed the title
| if err != nil { | ||
| return nil, nil, err // no tenant: get() never resolved a value | ||
| } | ||
| return ctx, func() {}, nil // fail open |
There was a problem hiding this comment.
Isn't there more to consider here? timeout may be set (or is even always set?), regardless of whether err is nil or tenant is empty, so shouldn't we be using it?
There was a problem hiding this comment.
Should this whole short circuit block be droppped?
There was a problem hiding this comment.
The problem in this cases is that the "no-tenant path" leaves timeout at zero, but Limit() intentionally returns -1 there to distinguish from "a real 0s timeout." So fail-open branch would apply an already-expired deadline instead of leaving the context unbounded
There was a problem hiding this comment.
Sorry, why is it important to have meaningful 0 vs. -1 cases?
There was a problem hiding this comment.
I've checked again and it won't matter for Limit(), as the caller already handles it (like limit <= 0). Will clean up this
There was a problem hiding this comment.
But for WithTimeout it's a different case, the value is passed to a context.WithTimeoutCause, so the returned context is already expired. That is a fail-closed (cancel immediatly) instead of current fail-open (run unbounded) behavior, which differs from what the original code comment said.
There was a problem hiding this comment.
Well why does Limit return -1? Should it be returning the timeout value alongside the error instead?
There was a problem hiding this comment.
And I believe part of the inconsistency in the "fail open" behavior is because some things are trivial to apply a default to (like this case) while others are complex data structures that must be indexed on the tenant value, meaning we don't really have a clean "default" fallback behavior. We should probably consider using a default rather than unbounded in this case 🤔
This pull request improves the behavior and documentation of the settings limiters to ensure that, on a settings read failure, the limiters return the compiled default value alongside the error, rather than discarding the value. This makes the error advisory rather than fatal, allowing callers to proceed with a sensible default even when the settings service is unavailable. The changes also add comprehensive tests to verify this behavior across all limiter types.
Behavioral improvements to limiter error handling:
Limitmethods (boundLimiter,rangeLimiter,gateLimiter,timeLimiter) now return the resolved value (including the compiled default on error) alongside any error, rather than discarding the value on a settings read failure. This ensures callers can always use the returned value unless the tenant is missing.WithTimeoutmethod intimeLimiternow returns a usable context and the resolved default timeout even when a settings read fails, with the error being advisory.Documentation updates:
Limiterinterface documentation is updated to clarify that returned values are always usable even when an error is present, except when the tenant is missing.