fix(models): share the cold-rebuild budget fairly across custom-provider probes - #7506
HarukiTakehata wants to merge 8 commits into
Conversation
The cold model-catalog rebuild probes the active endpoint first and each named custom_providers entry after it, serially, off one shared _LIVE_REBUILD_BUDGET_SECONDS budget while every probe is individually capped at CUSTOM_MODELS_ENDPOINT_TIMEOUT_SECONDS. One unreachable endpoint -- the common case being a LAN LM Studio/Ollama host the webui container cannot route to, since the probe runs server-side -- spent the whole budget on its own connect timeout. Every reachable provider behind it then got no in-band /v1/models probe at all: its group rendered from a stale disk cache or stayed empty, and the caller was served the over-budget fallback. Introduce _CustomProbeSchedule, which hands each probe a fair slice of the remaining window (with one slot of headroom so a fully-burned chain still finishes inside the budget) instead of the whole cap, so no single endpoint can starve the ones scheduled after it. The LM Studio provider-group fallback -- a second consumer of the same dead endpoint, previously on a hardcoded 5s timeout outside any budget -- now draws from the same schedule. The legacy unbounded path (budget <= 0) and the out-of-band continuation keep the unthrottled cap, so their behaviour is unchanged. Also drop an out-of-band rebuild result that would overwrite a strictly newer published catalog generation. Probe order, per-endpoint SSRF and authentication rules, and the total rebuild budget are all preserved. Refs nesquena#7481
Greptile SummaryThis PR fairly schedules custom-provider model probes within the cold-rebuild budget, deduplicates equivalent endpoint probes, and orders cache publication by rebuild generation.
Confidence Score: 5/5The reviewed changes appear safe to merge, with no accepted new findings or outstanding previous findings. The prior timeout-floor and generation-order findings are resolved in the current implementation, and the required runtime-behavior documentation is present. All previous threads were manually resolved. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant Catalog as Catalog Rebuild
participant Schedule as Probe Schedule
participant Providers
participant Cache
Caller->>Catalog: Request cold model catalog
loop Active and named custom providers
Catalog->>Schedule: next_timeout()
Schedule-->>Catalog: Fair share of remaining budget
Catalog->>Providers: GET /v1/models
Providers-->>Catalog: Payload or failure
Catalog->>Catalog: Memoize by endpoint + credential
end
alt Rebuild finishes in budget
Catalog->>Cache: Publish current generation
Catalog-->>Caller: Fresh catalog
else Foreground budget expires
Caller->>Catalog: Mark rebuild abandoned
Catalog-->>Caller: Cached/static fallback
Catalog->>Providers: Continue with full endpoint cap
Catalog->>Cache: Publish only if generation is current
end
Reviews (7): Last reviewed commit: "fix(models): fence superseded catalog pu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e headroom Review follow-up on the nesquena#7481 fix. Publication ordering: the superseded-generation guard compared one build's completion stamp against another build's start stamp, which inverts the ordering when an older rebuild publishes *after* a newer one has started - the newer build's result was then discarded, leaving a stale catalog published while its caller received a result that was never published. Order by an explicit rebuild sequence instead (_allocate_models_rebuild_seq / _models_published_seq): the number is taken under _available_models_cache_lock, and a result is dropped only when it is strictly older than the published catalog. Probe headroom: a fixed 0.5s per-probe floor let eight timeouts spend the whole four-second window and nine spend past it, so a long chain of dead endpoints could still push a reachable provider out of the in-band rebuild. The floor is now a 0.01s arithmetic guard against a zero (non-blocking) timeout, and the slice is additionally clamped against the time left, so a probe can never be handed more than the window holds and the N/(N+1) headroom holds at any chain length. Document the schedule, its trade-off, and generation-ordered publication in ARCHITECTURE.md section 4.9. Refs nesquena#7481
|
Review round 1 addressed in Both P1 findings were real — details and reasoning are on the inline threads. Summary:
Verification on
One limitation is documented rather than papered over: the window is fixed and shared, so a slow-but-reachable endpoint sitting in a long chain can still be cut off. ARCHITECTURE.md §4.9 names the budget knob and the static-allowlist escape hatch for that case. |
SummaryReading the complete three-file diff at Code referenceThe load-bearing branch at left = self._deadline - time.monotonic()
if left <= 0:
return self._cap
slice_seconds = left / (remaining + 1)
return min(self._cap, max(slice_seconds, min(self._min, left)))
Diagnosis / recommendationFor 1,000 pending probes with a four-second window, the first computed share is about 0.004 seconds. The regression at Please remove the fixed lower bound from in-window allocation, or otherwise make the lower bound a function of both time left and calls remaining so its cumulative cost cannot exceed the reserved window. Keep the full cap only for the deliberately out-of-band state. The scheduler may need an explicit foreground-versus-out-of-band signal if deadline expiry alone cannot distinguish those states. Verification stepExtend the fake-clock test with counts above the threshold, at least 401 and 1,000. Advance the clock by every returned timeout and assert after every allocation that elapsed time remains below four seconds and that a later reachable slot is still attempted. Preserve the separate test proving that a genuinely out-of-band continuation receives the full cap. |
…ength The 0.01s guard added in 0515ab0 was still a per-probe floor: once the computed share dropped below it, `max(slice_seconds, min(self._min, left))` raised every allocation, so a configured chain of ~400 dead endpoints drained the shared 4s window and each probe after that was handed the full per-endpoint cap — recreating the starvation the schedule exists to prevent, and contradicting the invariant ARCHITECTURE.md claimed for it. `custom_providers` is unbounded, so this was reachable from configuration. - Drop the lower bound from the in-window allocation. A slice is `min(cap, left / (remaining + 1))` with no floor; the slices telescope, so a chain of N probes spends N/(N+1) of the window for every N, and a positive `left` always divides to a positive slice. - Recognise the out-of-band continuation explicitly instead of inferring it from a spent deadline. `_CustomProbeSchedule` takes an `out_of_band` predicate; `get_available_models` passes the per-rebuild event it sets when the foreground caller stops waiting. Only that state keeps the full cap. A probe that finds the window spent while the caller is still waiting is a distinct, bounded state: it gets an attempt-only timeout (`CUSTOM_MODELS_ATTEMPT_ONLY_TIMEOUT_SECONDS`, renamed — no longer a floor and not a share of anything), never the cap. - Tests: the chain-length regression now runs N in {1, 2, 8, 24, 401, 1000} — past both the 0.5s and 0.01s thresholds — advances the fake clock by every returned timeout, and asserts after every allocation that the window is not spent and that each slot still gets a positive, sub-cap attempt. Added the in-band-spent and signal-wiring cases; the N=401/1000, in-band-spent and wiring cases all fail on 0515ab0. - ARCHITECTURE.md §4.9: state the no-floor rule and name the two states that keep the cap. Refs nesquena#7481 🤖 Generated with Hermes Agent (agent-authored submission, reviewed before posting)
|
Review round 2 addressed in You're right, and the invariant I claimed was false. The load-bearing branch was a floor in disguise: What changed:
Fail-first: with I also corrected the PR description: the round-1 text repeated the false "no chain length can outspend the window" claim, and it now records this round. Verification numbers and the honest limits are there too — including that a ~4ms slice at N ≈ 1000 still cannot complete a TLS handshake, so a reachable provider at the tail of an absurdly long chain is now bounded rather than starved, but not usefully probed. Cleaning that up honestly means fewer probes or a bigger budget, not a floor. I kept |
One rebuild can consult the same endpoint more than once: step 4 probes the active `model.base_url`, and the LM Studio provider-group fallback later reads `providers.lmstudio.base_url` — which falls back to `model.base_url` when lmstudio is the active provider. In the config shape nesquena#7481 reports both point at the same unreachable LAN host, so that dead endpoint cost the rebuild its connect timeout twice and consumed two slices of the shared window. Named `custom_providers` entries can repeat an endpoint the same way. Memoise each probe for the duration of one rebuild, keyed by (endpoint URL, credential), and reuse the first outcome: - The credential is part of the identity: the same URL with a different key is a different probe and still runs, because it can change the outcome. - Failures are memoised too — re-probing an endpoint this rebuild already found unreachable is exactly the stall being removed. - The raw payload is cached rather than the parsed entries, so each consumer still shapes the list for its own provider, and a memoised failure is re-reported with the asking provider's label (`_custom_endpoint_error` now takes the status code directly, so it can be re-shaped without inventing an exception). - The memo is consulted only AFTER the per-endpoint SSRF/validation checks, so a consumer that would have been refused still is; no validation is skipped on a hit. Probe order and the per-endpoint cap are unchanged. Tests: `tests/test_issue7481_probe_dedup.py` covers the shared dead endpoint (probed once, not twice), the shared reachable endpoint (the group still renders its models), two distinct endpoints, a same-URL/different-credential pair, two named entries on one endpoint, a named entry repeating the active endpoint, and the trailing-slash spelling of one endpoint. Five of the seven fail on the previous revision; the other two are the no-over-merge guards. The nesquena#7506 repro test's dead-probe count expectation drops from two probes to one, which is what this change is about. ARCHITECTURE.md §4.9 documents the one-probe-per-endpoint rule. Refs nesquena#7481 🤖 Generated with Hermes Agent (agent-authored submission, reviewed before posting)
|
Follow-up included in While fixing the round-2 finding I noticed the dead endpoint in the issue's config is consulted twice: step 4 reads the active
New Verification for the whole revision: 23 tests across the two #7481 modules pass; a 35-file neighbouring batch (385 tests) is Two process notes so you can push back: this went into this PR rather than a second one because it updates an assertion in this PR's own test file, and a stacked PR cannot target a fork branch (GitHub requires the base branch to exist in the base repo, so a follow-up PR would have to carry this PR's commits). If you would rather keep this PR strictly to budget and ordering, say so and I will split it into a PR that lands on top of this one. |
|
Thanks for this — I warmed it ahead of review (rebased clean onto current master; the only conflicting surface, #7323's GLM-5.3 catalog add, doesn't touch the probe path). The One thing to fix before review, on the tests (not the fix): two of them fail deterministically off-CI because they run against the real clock —
Both use a real The other tests in the same file already avoid this by installing Once those two are deterministic I'll take it through the full gate. |
… injected clock Both tests mocked an unreachable endpoint by really sleeping the timeout it was handed, while `_CustomProbeSchedule` measures the rebuild window with the real `time.monotonic()`. Whether the endpoints behind the dead one were reached in-band therefore depended on how fast the runner is: on a slower box the active endpoint's real sleep crosses the 4s deadline first, `observed["dead"]` comes back empty, and the ordering assertion fails — they pass on CI and failed 3/3 on the reviewer's box (review, 2026-09-10). - `_install_urlopen` takes an optional `clock`; a dead probe advances it by the timeout it was handed instead of really sleeping. - Both tests install `_FakeClock` through `monkeypatch.setattr(cfg, "time", clock)` — the same seam the schedule tests and the budget path use — so the window maths runs on virtual time. - The elapsed-time assertion in the repro test becomes `clock.now < _BUDGET`: it now states "the chain fitted inside the window" instead of measuring the host. - `_FakeClock` gained `time()` (delegates to the real clock, so the module's epoch-based comparisons keep their meaning) and `sleep()` (virtual). Nothing in `api/config.py` calls `time.sleep`. - The second test's ordering assertion is now exact — active endpoint, then `dead-one`, then `dead-two` — rather than filtering the active host out of the list, which makes the "probed in config order, in-band" claim explicit. No production code changes. The module drops from ~14.4s to ~6.4s for the same 16 tests, and both tests pass 5/5 repeats with a sub-10ms call phase. Refs nesquena#7481
The dedup module imported `isolate_models_catalog_state` from its sibling module and requested it as a test parameter. A fixture imported purely so it can be used *as* a fixture reads as an unused import to ruff (F401) and collides with the very parameters that request it (F811) — the repo's curated ruff gate flags both on new code, and it did. Each module defining the harness it uses is what the rest of this suite does, so the fixture now lives in the module that needs it. Test-only; no behaviour change. Refs nesquena#7481
|
Round-3 request done — Both tests run on the injected clock, as you suggested:
Evidence: the module runs its 16 tests in ~6.4s, down from ~14.4s with the real sleeps; both converted tests pass 5/5 repeats and their call phase no longer shows up among the slowest durations. All 23 tests across the two #7481 modules pass, and One self-inflicted follow-up in I did not touch the branch's position relative to master — it still reports |
|
Thanks for routing the two end-to-end repro tests through The one that still fails locally is
So the test asserts an in-band outcome that depends on real wall-clock work completing inside 4s — which holds on CI's faster/hermetic runner but not on a slower or loaded box. It's the same real-vs-virtual-clock seam as before, just one level deeper (the CV wait, not the sleep). Two ways to make it robust regardless of box speed, whichever you prefer:
No rush — the fix itself reads well, and this is purely about making the last repro test deterministic across runners. I've kept a full dossier so I can re-gate quickly once it's pinned down. |
nesquena-hermes
left a comment
nesquena-hermes
left a comment
There was a problem hiding this comment.
Thanks for the test-harness cleanup. I did a second, static exact-head pass over daf4458b1928, and there are still production-state races behind the scheduling fix that need to be closed before the ship gate.
1. The probe schedule and foreground wait do not share one deadline
_CustomProbeSchedule creates its deadline only when _build_available_models_uncached() reaches the custom-probe phase. The foreground independently starts build_done.wait(timeout=_LIVE_REBUILD_BUDGET_SECONDS) after starting the worker. Any discovery work before _CustomProbeSchedule(...) therefore consumes the caller's real budget without consuming the schedule's later window. The schedule can still hand the custom chain nearly four seconds after the foreground window has already mostly elapsed, so a reachable provider can land only after fallback was served.
Please capture one absolute rebuild deadline before _worker.start(), pass that exact deadline into the builder/schedule, and wait only for max(0, deadline - time.monotonic()). Preserve budget <= 0 as the unbounded path and keep the explicit abandonment event for the true out-of-band continuation.
2. Generation ordering does not fence invalidation or build ownership
The publish guard rejects only rebuild_seq < _models_published_seq. invalidate_models_cache() clears _cache_build_in_progress without advancing a minimum-valid generation, so an already-running out-of-band build can publish after invalidation. If invalidation starts build B while older A is still alive, A may publish first, stamp its stale result with the current source fingerprint, and unconditionally clear the one shared _cache_build_in_progress boolean while B is still active. That permits another rebuild and makes waiter ownership ambiguous.
Please make build ownership explicit (sequence/token), fence all pre-invalidation builds, capture and validate the build's source/profile identity at publication, and clear/notify only when the completing build still owns the in-progress slot.
3. Disk publication is outside the generation fence
_save_models_cache_to_disk() runs after the memory-side sequence check and uses one process-wide .<pid>.tmp path. Overlapping A/B publishers can write/rename the same temp file, and an older publisher can replace the newer disk cache after the in-lock generation decision.
Please use a unique temp file per build and re-check the accepted generation plus source/profile identity immediately before os.replace; stale publishers should discard their temp file.
Tests to add or strengthen
- Compose invalidation while A is out-of-band, then start B. Cover both completion orders and assert that A cannot restore memory/provenance/disk or clear B's ownership.
- Add a delayed pre-custom-discovery case proving that the schedule and foreground use one absolute deadline.
- Own/join every daemon worker before fixture teardown. The current polling/fixed-sleep tests and direct mutation of
_models_rebuild_seq/_models_published_seqdo not exercise these races or disk state. - Keep the injected-clock arithmetic tests, but do not combine a virtual
cfg.time.monotonic()schedule with a realEvent.wait()and call the result end-to-end deterministic. Use a synchronous seam or inject both the clock and waiter.
I did not execute this PR: the required numeric threat scan failed before producing a verdict because GitHub returned HTTP 403 (pr_threat_scan.py rc=3), so the run stayed mandatory NO-RUN/static-only. That transport failure is not the reason for this review; the blockers above are direct source-level control-flow findings at the exact head. The local static tree was clean and git diff --check origin/master...HEAD passed.
…cation in the nesquena#7481 repro
|
已按维护者的定位，把该 PR 最后一个非确定性测试改为与机器速度无关的断言。 改了什么（仅改测试，未动
为什么这样改 fail-first / 确定性证据
Lint 新 head sha Refs #7481 备注：本容器到 |
SummaryI reread the latest Code referenceThe stale guard at with _cache_build_cv:
if rebuild_seq < _models_published_seq:
# ...
returnHowever, The test at Diagnosis / recommendationA superseded worker must be rejected against the latest allocated generation, not only the latest published generation. Under VerificationAdd an event-driven regression: block worker N past the foreground budget, invalidate, start and block worker N+1 before publication, release N first, and assert that no cache/disk publish occurs and the build flag remains true. Then release N+1 and assert it alone publishes. A second variant where N+1 raises should prove the invalidated N catalog is not resurrected. |
|
Due to my machine problem, the pull request may suspend in several days, I'll fix it as soon as possible |
nesquena-hermes
left a comment
nesquena-hermes
left a comment
There was a problem hiding this comment.
Re-gate result — SHIP ONLY WITH FIXES (all 3 prior findings still open, each reproduced)
Thanks for the continued work on the test harness. I rebased onto current master myself (was 56 behind, clean) and re-ran the full adversarial gate against the current head. The #7481 budget-slicing idea is sound and the 23 targeted tests pass — but a concurrency harness reproduced all three of the races from the 2026-09-11 review, so none of them is actually closed yet. The recent commits hardened the test harness (own the fixture, injected-clock repro, assert invariants) without changing the production control flow the findings are about.
1. [CORE] The probe schedule and foreground wait still use two independent windows — api/config.py:6882, 9373, 10630-10632
_CustomProbeSchedule.__init__ still creates its own deadline (time.monotonic() + _LIVE_REBUILD_BUDGET_SECONDS) at construction, which happens deep inside the worker at ~9373; the foreground independently waits build_done.wait(timeout=_LIVE_REBUILD_BUDGET_SECONDS) at 10632, after _worker.start() at 10630. Reproduced: an 80 ms pre-custom delay against a 60 ms foreground budget — the schedule was constructed after fallback had already been served, with a fresh deadline, and handed the dead endpoint its full 100 ms cap. So a reachable custom provider can still land only after fallback was returned — the exact defect #7481 is meant to fix, for the pre-custom-discovery case.
Fix: capture one absolute deadline before _worker.start(), pass it into _CustomProbeSchedule, and wait only max(0, deadline - time.monotonic()). Keep budget <= 0 as the unbounded path.
2. [CORE] Cache invalidation does not fence an already-running publisher — api/config.py:8283-8373, 10542-10582
The publish guard compares only against an already-published sequence, and ownership is an unqualified shared _cache_build_in_progress boolean. Reproduced with real detached builds A and B in both completion orders: after invalidation and B's start, older A published stale memory/provenance/disk state and cleared _cache_build_in_progress while B was still blocked.
Fix: advance a minimum-valid generation at every invalidation site; make build ownership an explicit sequence/token; reject publications older than the fence; clear/notify ownership only when the completing token still owns the slot (including the error-cleanup path).
3. [SILENT] Concurrent publishers can silently revert the durable catalog — api/config.py:8213-8247, 10570-10571
_save_models_cache_to_disk() uses one process-wide models_cache.json.<pid>.tmp. Reproduced with two real filesystem writers: both chose the same temp path; the newer writer renamed it, then the older writer's still-open descriptor overwrote the published file, leaving stale-but-valid JSON. There's also no generation/source/profile recheck between memory acceptance and disk replacement.
Fix: unique same-directory temp file per publication; immediately before os.replace, recheck accepted generation + source fingerprint + profile + destination under the publication lock; stale publishers delete their temp file.
Tests
The newer-first ordering was safe in every case; older-first is where 2 & 3 fail. Please add: invalidation-while-A-out-of-band then start-B covering both completion orders (assert A cannot restore memory/provenance/disk or clear B's ownership); a delayed pre-custom-discovery case proving one shared absolute deadline; own/join every daemon worker before teardown.
The direction is right and the harness is now in good shape — this is about moving the three fixes from the test layer into the production control flow. Re-request review when pushed and I'll re-gate.
nesquena-hermes
added
the
gate-fail
`invalidate_models_cache()` clears `_cache_build_in_progress` without cancelling a running worker, so a newer rebuild can be allocated while an older one is still in flight. The publication guard compared against the last *published* sequence, which only fences a build once a newer one has actually published: if the older worker finished first, it published the invalidated catalog and then released the build flag that already belonged to the newer rebuild. Callers could observe the stale catalog and another cold rebuild could enter concurrently; if the newer rebuild then failed, the invalidated result stayed authoritative (maintainer review, 2026-09-11). Fence on the newest *allocated* generation instead: - `_models_rebuild_superseded(rebuild_seq)` compares `rebuild_seq` with `_models_rebuild_seq`, read under `_cache_build_cv` so it is atomic against `_allocate_models_rebuild_seq`. - `_publish_models_result` discards a superseded result and deliberately leaves the flag alone — it now belongs to the newer rebuild. - `_clear_build_in_progress(rebuild_seq)` takes the sequence and refuses to release the flag for a superseded build, so an errored/no-result older worker cannot wake waiters to an empty cache or let a third rebuild enter. - The legacy synchronous path applies the same fence to its cache publish, disk save, and flag release, so it cannot resurrect an invalidated catalog either. Tests (`tests/test_issue7481_custom_probe_budget_fairness.py`, 2 new, 18 total): block worker N past the budget, invalidate, start and block N+1, release N first, then release N+1. The first asserts N publishes nothing and leaves the flag set, and that N+1 alone publishes. The second makes N+1 raise and asserts the invalidated N catalog is not resurrected. Both capture the worker threads inside the mocked builder and `join` them, so the ordering is event-driven rather than wall-clock dependent. Both fail on the previous revision. ARCHITECTURE.md §4.9 now documents the allocated-generation fence. Refs nesquena#7481
|
Now I'm manually take the session on my laptop: PR #7506 — Round 4 report
Round 4 — You're right, and the window was real. Fixed by fencing on the newest allocated generation:
Tests (
Both capture the worker threads inside the mocked builder and Docs: Verification:
Residual I did not change: an in-flight worker invalidated with no newer rebuild No rebase; branch still behind master with the existing threads anchored. Refs #7481 Push notePush from this machine was denied: the SSH key authenticates as Local verification commands used |
This branch has not been deployed
3 participants
@Yang-qwq
Refs #7481
Thinking Path
The cold catalog rebuild probes the active endpoint first and each named
custom_providersentry after it, serially, off one shared_LIVE_REBUILD_BUDGET_SECONDS(4.0s) window, while every probe is individually capped atCUSTOM_MODELS_ENDPOINT_TIMEOUT_SECONDS(5.0s). Because the per-endpoint cap (5s) is larger than the whole chain's budget (4s), the first endpoint in the chain can consume the entire window on its own connect timeout. Everything behind it never gets an in-band/v1/modelsprobe, so its group renders from stale disk cache or stays empty, and the caller is handed the over-budget fallback.I reproduced this against current
masterwith the issue's own config shape (dead LAN endpoint as the active provider and asproviders.lmstudio, plus a reachable named gateway), instrumentingurllib.request.urlopen:Two things feed that stall, not one. Besides the chain's step 4/step 5 probes, the LM Studio provider-group branch (
pid == "lmstudio") re-probes the same dead endpoint again fromproviders.lmstudio.base_urlunder a hardcodedtimeout=5that sits outside any budget at all. Bounding only the chain leaves the rebuild over budget anyway, so the fix has to cover both to actually publish the reachable provider in-band.What Changed
api/config.py_CustomProbeSchedule(new): hands each probe in the chain a fair slice of the remaining window instead of the whole per-endpoint cap, so an endpoint that times out cannot starve the endpoints scheduled after it. The slice ismin(CAP, left / (remaining + 1))with no lower bound: the+1reserves a slot of headroom, the slices telescope, so a chain of N probes spends exactlyN/(N+1)of the window for every N and the foreground gets a published catalog rather than a fallback. A fixed floor made the total grow with chain length at 0.5s (first revision) and at 0.01s (second revision) — a positiveleftalready divides to a positive slice, so there is nothing to floor; see Review Round 2._CustomProbeScheduletakes anout_of_bandpredicate, andget_available_modelspasses the per-rebuild event it sets when the foreground caller stops waiting. Only that deliberately out-of-band continuation keeps the full per-endpoint cap (its probes are no longer holding anyone up, and the refresh must be able to complete). A probe that finds the window spent while the caller is still waiting is a separate, bounded state: it gets an attempt-only timeout (CUSTOM_MODELS_ATTEMPT_ONLY_TIMEOUT_SECONDS— not a floor, not a share of the window, and unreachable from the fair-share path), never the cap.models:allowlist + the LM Studio fallback when it will fire), and supplies the timeout at all three probe sites: step 4, step 5, and the LM Studio provider-group fallback (previously the hardcodedtimeout=5)._publish_models_result, ordered by an explicit rebuild sequence rather than by wall clock:_allocate_models_rebuild_seq()hands each cold rebuild the next number (taken under_available_models_cache_lock), the published catalog records the sequence it came from (_models_published_seq), and a result is dropped only whenrebuild_seq < _models_published_seq. Only the out-of-band publisher can reach that path (it outlives the foreground caller that gave up on it), and without the guard it could resurrect a superseded catalog. Ordering by timestamp cannot express this — an older build can publish after a newer one has started, which would then wrongly drop the newer result.ARCHITECTURE.mdCUSTOM_MODELS_ATTEMPT_ONLY_TIMEOUT_SECONDSis neither a floor nor a slice), the serial probe order and fair-share rule with its trade-off, the two states that keep the unthrottled cap (legacy unbounded path, explicit out-of-band continuation) with the hand-off race called out as a distinct bounded state, and generation-ordered out-of-band publication.CHANGELOG.mdis untouched, perAGENTS.md/CONTRIBUTING.md.Deliberately unchanged, per the maintainer's scope note:
_LIVE_REBUILD_BUDGET_SECONDS, same env override); the chain now simply fits inside it. The legacy synchronous path (budget <= 0) and the out-of-band continuation both keep the historical unthrottled cap, so their behaviour is identical to before.tests/test_issue7481_custom_probe_budget_fairness.py(new, 16 tests) — bound to the issue's pinned config shape, with the host resolver andurlopenstubbed so the probes are exactly the ones the issue describes:test_unreachable_lan_active_endpoint_does_not_starve_the_gateway_behind_ittest_every_dead_endpoint_in_the_chain_still_lets_the_live_one_throughmodels:providerstest_static_allowlist_provider_is_never_probed_and_does_not_dilute_the_scheduletest_probe_schedule_restores_the_cap_once_the_foreground_gives_up(+ the repro asserting no fallback was served)test_late_out_of_band_result_cannot_overwrite_a_newer_generation,test_older_publish_does_not_cost_a_newer_rebuild_its_resulttest_probe_schedule_cannot_outspend_the_window_at_any_chain_length(N ∈ {1, 2, 8, 24, 401, 1000}),test_probe_schedule_stays_bounded_in_band_after_the_window_is_spent,test_probe_schedule_is_wired_to_the_foreground_giving_uptest_probe_schedule_keeps_the_documented_cap_when_the_budget_is_disabled,test_probe_schedule_shares_the_window_and_reserves_headroomWhy It Matters
With the fix, the same reproduction finishes in-band and the reachable gateway is in the catalog the caller receives:
A user with an unreachable LAN endpoint configured stops seeing a stale/empty custom-provider group, and every cold model-list load stops paying a full-budget stall followed by a fallback.
Verification
Local runs used Python 3.12 (CI's middle matrix entry), against
master@71689c6ce7031f4b6ffc90d68a61a2c1f9afe52a.f9fb6ff): 7 of the module's tests fail on pre-fixmasterand all pass with the fix; the repro test fails pre-fix because the gateway is never probed and the over-budget fallback is served.8e333e5): withapi/config.pyreverted to0515ab0,test_probe_schedule_cannot_outspend_the_window_at_any_chain_length[401],[1000],test_probe_schedule_stays_bounded_in_band_after_the_window_is_spentandtest_probe_schedule_is_wired_to_the_foreground_giving_upfail, while[1]/[2]/[8]/[24]still pass there — exactly the threshold behaviour the round-2 review described. All 16 pass on8e333e5.476 passed, 6 skipped, 1 failed. The single failure istest_model_picker_badges.py::test_available_models_exposes_primary_and_fallback_badges, a pre-existing cross-file ordering failure: it passes in isolation on this revision, and it fails identically when the same batch is run against0515ab0with this new module excluded (1 failed, 464 passed, 2 skippedthen), so it is not introduced by this diff.216 passed) shows the same pattern: its two failures (test_issue2513_custom_provider_remote_models.py,test_issue2540_models_endpoint_error.py) reproduce byte-for-byte on0515ab0in that batch and pass in isolation on both revisions.python3 scripts/ruff_lint.py --diff origin/master→0 new violations on added/modified lines.test_extensions_settings_panel,test_issue1908_docker_hardening,test_issue3012_3006_docker_docs,test_nix_flake_module_source) fail collection on this checkout for missing repo files, on both revisions — environmental, not touched here.Review Round 1 →
0515ab0Both P1 findings from the automated review were real and are fixed (replies on the inline threads):
_models_rebuild_seq/_models_published_seq); a result is dropped only when strictly older than the published catalogARCHITECTURE.md§4.9Review Round 2 →
8e333e5The review's finding was correct and is fixed:
min(CAP, left / (remaining + 1))— nomax(...), no floor. With the+1headroom the slices telescope: after burning every one of N probes,left = budget · 1/(N+1) > 0, so the chain spendsN/(N+1)of the window at every length and always finishes inside it.custom_providersis unbounded, so this now holds for a configured chain of any size, not just the counts the earlier revision happened to test._CustomProbeSchedule(endpoint_count, *, out_of_band=...)takes the predicate;get_available_modelspasses the per-rebuild event it sets at the same point it setsbudget_exceeded(i.e. when the foreground caller stops waiting). A probe that finds the window spent while the caller is still waiting is explicitly not that state: it gets the attempt-only timeout, renamedCUSTOM_MODELS_ATTEMPT_ONLY_TIMEOUT_SECONDS— not a floor, not a share of anything. That is the foreground-vs-out-of-band signal you suggested; the two states were genuinely indistinguishable from the clock alone.test_probe_schedule_cannot_outspend_the_window_at_any_chain_lengthis now parametrised over[1, 2, 8, 24, 401, 1000]— past both the 0.5s and 0.01s thresholds — advances the fake clock by every returned timeout, and asserts after every allocation that elapsed time is still below four seconds and that each slot received a positive, sub-cap attempt. Addedtest_probe_schedule_stays_bounded_in_band_after_the_window_is_spent(spent window, no signal → attempt-only, never the cap) andtest_probe_schedule_is_wired_to_the_foreground_giving_up(the schedule really receives the signal and observes the hand-off).test_probe_schedule_restores_the_cap_once_the_foreground_gives_upis preserved for the genuinely out-of-band continuation, and now sets the signal explicitly.What I could not verify: real-world latency of a sub-10ms slice against a live TLS endpoint (no live network here), and the fair-share cut-off itself is a real limit — at N ≈ 1000 each probe gets ~4ms, which no HTTPS handshake completes in, so a reachable provider at the tail of an absurdly long chain is still effectively unprobed in-band. That is now bounded rather than an implicit cap bump, but it is not useful; addressing it honestly means fewer probes or a bigger budget, not a floor.
Follow-up: the duplicated probe ->
635e93bWhile fixing the budget I noted that the dead endpoint in the issue's config is consulted twice —
step 4 reads the active
model.base_url, and the LM Studio provider-group fallback readsproviders.lmstudio.base_url, which falls back tomodel.base_urlwhen lmstudio is the activeprovider. One unreachable host therefore cost two connect timeouts out of the same shared window,
which no amount of fair-share scheduling can win back. It is now handled rather than only documented:
first outcome — including a failure, since re-probing an endpoint this rebuild already found
unreachable is the stall.
its own probe.
own provider, and a memoised failure is re-reported with the asking provider's label.
would have been refused still is.
Tests (
tests/test_issue7481_probe_dedup.py, 7): shared dead endpoint probed once · sharedreachable endpoint still renders its group · two distinct endpoints still each probed · same URL
with a different credential still probed twice · two named entries on one endpoint probed once ·
a named entry repeating the active endpoint probed once · trailing-slash spelling treated as one
endpoint. Five of the seven fail on
8e333e5; the other two are the no-over-merge guards. The#7506 repro test's dead-probe count expectation drops from two to one, which is what this change is
about.
Process note, so you can push back: this went into this PR rather than a second one because it
updates an assertion in this PR's own test file, and a stacked PR cannot target a fork branch
(GitHub requires the base branch to exist in the base repo, so a follow-up PR would have to carry
this PR's commits). If you would rather keep this PR strictly to budget and ordering, say so and
I will split it into a PR that lands on top of this one.
Review Round 3 ->
2c6d9f1, headdaf4458(tests only)You were right that those two tests were decided by wall-clock: their dead-host mock really slept the slice it was handed while
_CustomProbeSchedulemeasured the window with the realtime.monotonic(), so on a slower box the active endpoint's sleep crossed the 4s deadline beforedead-one/dead-twowere reached. Both now run on the injected clock:_install_urlopentakes an optionalclock; a "dead" probe advances it by the timeout it was handed instead of really sleeping._FakeClockviamonkeypatch.setattr(cfg, "time", clock)— the same seam the schedule tests and the budget path use — so the window arithmetic is virtual.clock.now < _BUDGET: it states "the chain fitted inside the window" instead of measuring this host.dead-one, thendead-two— instead of filtering the active host out, so "walked in config order, in-band" is asserted rather than implied._FakeClockgainedtime()(delegates to the real clock, so the module's epoch-based comparisons keep their meaning) andsleep()(virtual). Nothing inapi/config.pycallstime.sleep.Evidence: the module runs its 16 tests in ~6.4s (was ~14.4s with the real sleeps), both converted tests pass 5/5 repeats, and their call phase no longer appears among the slowest durations.
api/config.pyis byte-identical to635e93bacross these two commits.daf4458also fixes a gate-cleanliness issue of my own making: the dedup module imported the sibling's harness fixture, which the curated ruff gate flags as F401 (unused import) plus F811 (the import collides with the parameters requesting it), so that module now defines the fixture it uses.ruff_lint --diff origin/master: no new violations.Risks / Follow-ups
HERMES_WEBUI_MODELS_REBUILD_BUDGETfor the endpoints actually in use, or give an entry a staticmodels:allowlist. The invariant that now holds at any chain length — and is tested over N ∈ {1, 2, 8, 24, 401, 1000} — is that the chain cannot outspend the window while the caller is still waiting.635e93bmemoises each probe by (endpoint URL, credential) and probes a repeated endpoint once per rebuild — see Follow-up below.CUSTOM_MODELS_MIN_PROBE_TIMEOUT_SECONDS→CUSTOM_MODELS_ATTEMPT_ONLY_TIMEOUT_SECONDS: the old name asserted exactly the property that turned out to be false. The constant is introduced by this PR, so there are no other consumers.master(71689c6); I have not rebased, to keep the existing review threads anchored. Happy to rebase on request.AI Usage Disclosure / Model Used
deepseek-v4-flash