Repository navigation
feat(distance): use plugin store for cache, budget - #79
Merged
Merged
Conversation
With provider dbip, tes.routing.distance keeps the locations of addresses (also misses and failures) in FOCA's plugin store (ctx.store), shared by all processes and replicas, instead of the per-process TtlCache, and limits the uncached lookups of each caller to dbip_user_daily_lookups (default 100) per day: a fixed-window counter keyed by ctx.store.key_for() of the principal, never the raw identifier. Store failures never fail a call: the cache then counts addresses as not cached, and the budget fails closed. Transient lookup failures, which send no request, give the lookup back. The plugin declares its entries (data_stored). All plugins now need FOCA 0.19.0, the first release with the plugin store (CI matrix, meta package, docs); FOCA 0.18 is dropped. Closes #21 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review fixes. LookupFailed gets "sent": whether the lookup may have sent a request. The DB-IP provider sets it to false when closed, backed off or out of time before any request, and the budget gives such lookups back, also non-transient ones (no more charges while DB-IP is backed off); transient failures after requests count. give_back() leaves expired counters alone, so that no window starts at -1. The store tests move to test_store.py. The README recommends FOCA's key secret with dbip, and the CHANGELOG notes the new default limit and the stored data as breaking changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Round-2 review fixes. Failed lookups that sent no request (e.g., this process's DB-IP back-off, or a closed provider) are no longer cached as unlocated, as the cache in the plugin store is shared with other processes and replicas. give_back() checks counters with a simpler guard, and the budget test also checks what is cached. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideData structuresAdd the store-backed location cache and per-caller budget, including hashed keys, validation, expiry, failure handling, and request-count refunds.
Define whether a provider failure may have sent a request so callers can charge or refund the lookup budget correctly.
APIExpose the daily lookup limit, stored-data declaration, store requirement, and provider-specific wiring for shared cache and budget use.
ConfigurationRaise the minimum FOCA version to the first release with the plugin store across package metadata and the CI compatibility matrix.
DocumentationDocument the new FOCA support floor for the project and plugins.
Explain the distance plugin’s shared DB-IP cache, daily budget, stored personal data, retention, and FOCA 0.18 breaking change.
TestsTest shared cache behavior, caller budgets and refunds, store failures, provider request tracking, and the new store implementation.
Lock files and vendored codeUpdate the dependency lockfile for the dependency and FOCA version changes.
Other changesFiles not grouped.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Documentation build overview
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #79 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 10 11 +1
Lines 886 1001 +115
Branches 125 143 +18
==========================================
+ Hits 886 1001 +115
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes CVE-2026-128089, which fails the dependency audit of the test jobs (pip-audit). Unrelated to #21. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
uniqueg
marked this pull request as ready for review
November 13, 2026 23:30
Linked issuesChecked at commit 3408b15 #21 feat(distance): use plugin store for cache, budget · 5 of 5 met
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 9 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py" line_range="122" />
<code_context>
+ if not self.enabled or self.failed:
+ return False, None
+ try:
+ value = self.store.get(self.store.key_for(LOCATION_KEY, key))
+ except StoreError as exc:
+ self._failure(exc)
</code_context>
<issue_to_address>
**Stored addresses are enumerable**
When an operator enables `provider: dbip` without configuring FOCA's store key secret and someone obtains a database or backup read, `StoreCache.get()` and `UserBudget.__init__()` call `store.key_for()` without requiring a secret, so with the default unkeyed hashes a reader of the database or a backup can brute-force public IPv4 addresses and associate them with stored locations; caller identifiers can also be guessed. The README acknowledges this exposure but only recommends setting a secret.
Require a configured secret for DB-IP storage, or use a keyed derivation that is available by default.
Also at `plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py:198`.
</issue_to_address>
### Comment 2
<location path="plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py" line_range="54" />
<code_context>
+ if (
+ isinstance(number, bool)
+ or not isinstance(number, int | float)
+ or not math.isfinite(number)
+ ):
+ return False, None
</code_context>
<issue_to_address>
**Malformed coordinates break routing**
When the plugin store contains a location list with an integer too large for float conversion, `_location` passes arbitrarily large integers to `math.isfinite`, which raises `OverflowError` when the integer cannot be converted to a float. `StoreCache.get` does not catch it, so reading that entry aborts routing instead of treating the entry as invalid.
Catch `OverflowError` while validating stored numbers, or convert them to floats with the same guarded validation used by `coordinates`.
</issue_to_address>
### Comment 3
<location path="plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py" line_range="214" />
<code_context>
+ if self.exhausted or self.failed:
+ return False
+ try:
+ count = self.store.increment(self.key, ttl=BUDGET_WINDOW, limit=self.limit)
+ except StoreError as exc:
+ self.failed = True
</code_context>
<issue_to_address>
**Daily lookup limits expire early**
When the plugin store's `max_ttl` is shorter than 86,400 seconds, `UserBudget.take()` requests a one-day TTL, but the store caps it at `max_ttl`, so the counter expires early and the caller can start another budget window before a day has passed, exceeding the configured daily limit.
Ensure budget counters remain valid for the full one-day window, including when the store's `max_ttl` is shorter.
</issue_to_address>
### Comment 4
<location path="plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py" line_range="235-238" />
<code_context>
+ if self.failed:
+ return
+ try:
+ count = self.store.get(self.key)
+ if not isinstance(count, int) or count <= 0:
+ return
+ self.store.increment(self.key, -1, ttl=BUDGET_WINDOW)
+ except StoreError as exc:
+ self.failed = True
</code_context>
<issue_to_address>
**Refunds reduce a new window's count**
When a budget counter expires between `give_back()`'s read and decrement, and the decrement runs after expiry, `UserBudget.give_back()` decrements the key without verifying it is still the window that was charged; the decrement can reduce a new window's count or recreate the counter below zero, allowing extra lookups against that window's limit.
Make refunds atomic with the counter-window check so a refund cannot change a different window's count.
</issue_to_address>
### Comment 5
<location path="plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/__init__.py" line_range="258" />
<code_context>
+ if (
+ self.ctx.remaining() <= 0
+ or (self.max_lookups is not None and self.batch_lookups >= self.max_lookups)
+ or (self.budget is not None and not self.budget.take())
):
self.skipped_lookups += 1
</code_context>
<issue_to_address>
**No-request attempts consume lookup budget**
When the shared cache misses but the provider serves a process-local cache hit, or the DB-IP provider has no HTTP client, `_Locator._lookup()` takes a budget slot before `DbipProvider.locate()` and retains it on a successful provider-cache return; when `ctx.http` is absent, `PluginError` also escapes without the `LookupFailed(sent=False)` refund. These no-request attempts consume the caller's allowance and can cause later lookups to be skipped.
Refund the budget slot whenever the provider completes without sending a request, including cache hits and errors before a request is sent.
Also at `plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/__init__.py:259`.
</issue_to_address>
### Comment 6
<location path="plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py" line_range="220" />
<code_context>
+ self.log.warning("Plugin store unavailable for the budget: %s", exc)
+ return False
+ if count is None:
+ self.exhausted = True
+ self.log.info("The caller's daily budget of lookups is used up")
+ return False
</code_context>
<issue_to_address>
**Refund leaves budget exhausted**
When one call observes an exhausted budget and another call subsequently refunds a lookup that sent no request, a caller's `UserBudget` object remembers an exhausted count even after another concurrent call refunds an unused lookup. Later lookups in the first call keep getting skipped although the shared counter has room, so the caller gets fewer locations than its remaining budget allows.
Do not memoize exhaustion while concurrent refunds can free capacity; recheck the shared counter for later lookups or use a design that makes exhaustion and refunds visible to all budget instances.
</issue_to_address>
### Comment 7
<location path="plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py" line_range="198" />
<code_context>
+ """
+ self.store = store
+ self.key = store.key_for(
+ BUDGET_KEY, principal.subject_type or "", principal.subject or ""
+ )
+ self.limit = limit
</code_context>
<issue_to_address>
**Anonymous callers share an exhaustible budget**
When the gateway accepts anonymous requests and the DB-IP provider is enabled, `UserBudget` maps every principal with missing identity fields to the same key, so one unauthenticated caller can consume the shared daily allowance and prevent other anonymous callers from using DB-IP location lookups; their routing falls back without the distance information.
Require an authenticated principal for this budget or define an explicit anonymous policy that does not let one caller exhaust every anonymous caller's allowance.
</issue_to_address>
### Comment 8
<location path="plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py" line_range="157-159" />
<code_context>
+ """
+ if not self.enabled or ttl <= 0:
+ return
+ cached, _ = self.get(key)
+ if not cached:
+ self.put(key, value, ttl)
+
+
</code_context>
<issue_to_address>
**Failed lookups overwrite cached locations**
When a service failure and a successful lookup for the same address overlap, `StoreCache.add()` checks for an existing entry and writes the failure marker in separate operations, so the failure can overwrite the successful location; later calls treat the address as unlocated until the failure entry expires.
Make the failure-marker write conditional and atomic with checking that no entry exists.
</issue_to_address>
### Comment 9
<location path="plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py" line_range="122" />
<code_context>
+ if not self.enabled or self.failed:
+ return False, None
+ try:
+ value = self.store.get(self.store.key_for(LOCATION_KEY, key))
+ except StoreError as exc:
+ self._failure(exc)
</code_context>
<issue_to_address>
**Concurrent misses send duplicate requests**
When multiple callers miss the shared cache for the same address before any lookup completes, `StoreCache.get()` allows both callers to observe a miss before either writes a result, so both invoke the provider and send requests, consuming upstream quota and each caller's budget.
Coordinate in-flight lookups for the same address so only one caller invokes the provider.
Also at `plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py:140`, `plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/__init__.py:409`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 9 findings to address first, and with provider: dbip, the plugin now persists geolocations and caller lookup counts in FOCA’s store under hashed keys; without the configured key secret, IPv4 addresses can be recovered by hashing candidate addresses. Reverting stops new writes but does not remove existing entries, which remain until their TTL expires or the store is cleared.
Blocking findings: plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py:122, plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py:54, plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py:214, plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/store.py:238, plugins/tes/routing/distance/src/foca_plugins/tes/routing/distance/__init__.py:258, and 4 more
Automated review fixes. Stored coordinates are checked by range only, which also rejects NaN, infinities and integers too large for floats (math.isfinite() raised OverflowError on them). The README says that the budget window is capped by the store's max_ttl. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
tes.routing.distancewith its opt-indbipprovider:ctx.store), shared by all processes and replicas, instead of the per-processTtlCache(theofflineprovider keeps the per-process cache);dbip_user_daily_lookups(default 100,nullfor no limit) per day: a fixed-window counter (ctx.store.increment(key, ttl=86400, limit=n)) keyed byctx.store.key_for()of the principal'ssubject_typeandsubject, never the raw identifier;LookupFailed.sentis false when the provider is closed, backed off, or out of time before any request, and such lookups are given back (and not cached as failures in the shared store); giving back leaves expired counters alone;StoreFullError, aStoreError) counts as a failure;data_stored; README (Data stored section with a key-secret recommendation, configuration table, cache and limits) and CHANGELOG (breaking: new default limit, stored data) updated.All plugins now require FOCA 0.19.0, the first release with the plugin store: lower bounds, the meta package, the CI matrix (0.18.0 dropped, coverage on 0.19.0) and the docs. Breaking for
generic.routing.randomtoo (CHANGELOG), as the repository tests one lowest FOCA version for all plugins.Plugin abstraction: the store-backed cache and budget are specific to this plugin's DB-IP use; a reusable abstraction (e.g., a generic per-user budget helper) can follow once a second plugin needs one.
Verification
ruff check .,ruff format --check .: pass.python -m scripts.packages mypy plugins/tes/routing/distance: pass.coverage run --source plugins/tes/routing/distance/src -m pytest plugins/tes/routing/distance: all pass, 100% coverage, with FOCA 0.22.0 (lock), 0.19.0, and FOCA dev plus feat(plugins): limit store namespace size foca#552 (namespace size limit) (the earlier commit; re-run after the review fixes with 0.22.0: 174 passed).generic.routing.randomandtes.data.crypt4ghpass with FOCA 0.19.0 and 0.22.0;pytest tests: 142 passed.mkdocs build --strict: pass.pre-commit run --all-filescouldn't run locally (the sandbox proxy blocks fetching hook repositories); CI's lint job runs it.Unrelated fixes
build(deps): bump dnspython to 2.9.2: CVE-2026-128089 failed the dependency audit of every test job.Declined review findings
route()into a helper, and build the per-processTtlCacheonly foroffline: the block is short and readable; the unused cache underdbipcosts nothing.LocationCacheprotocol andStoreCache: repo convention is full Google docstrings on every method.docs/writing-plugins.mdandci.ymloutside the lines this PR changes: pre-existing.tes.data.crypt4gh: it was never released; its first release starts with the new bound.Open questions
None.
Closes #21
🤖 Generated with Claude Code