Keep ModelManager lock generations and fixed-size cache bookkeeping consistent by voropaevv · Pull Request #2821 · roboflow/inference

voropaevv · 2026-08-19T10:23:44Z

Why

ModelManager could remove a per-model lock entry while the same lock
generation was still held or referenced by waiting callers.

A subsequent add_model() for the same resolved model identifier could then
create a new generation. A caller waiting on the old lock and a newcomer using
the new lock could initialize the same model concurrently.

The same mutation lifecycle exposed related consistency problems in manager
decorators:

an unlocked warm-model fast path could bypass an already-established
remove/add order;
failed removal or eviction could leave an identifier registered in the
underlying manager but absent from the LRU queue;
concurrent cold additions could create duplicate queue entries;
aliases were not consistently used as the resolved lifecycle and cache key;
failed direct removal could change the LRU position even though the mutation
did not complete;
a failed multi-model eviction could drop skipped pinned entries and the
failed eviction candidate from the queue.

Fixes #2819.

What changed

Store each per-model lock and its active reservation count in one lifecycle
entry.
Reserve the exact entry before waiting for its lock.
Retain that generation through holder, waiter, timeout, and exception paths.
Retire an entry only when:
- it is still the current generation;
- it has no active reservations; and
- the resolved model identifier is absent.
Treat decorator-level existence checks as hints and make the final warm-model
decision under the stable per-model lifecycle lock.
Use the resolved model identifier consistently for alias-sensitive lifecycle
and cache bookkeeping.
Reconcile fixed-size cache entries after both successful and failed
add/remove attempts.
Preserve the exact existing LRU position when a direct removal fails.
Restore skipped pinned entries and failed eviction candidates in their
original relative order.
Do not restore entries whose removal completed successfully before a later
eviction failure.
Ensure completed additions and removals leave at most one queue entry per
resolved identifier.
Add deterministic concurrency and rollback coverage using explicit
synchronization points rather than scheduling sleeps.

Public method signatures and configured per-model lock-acquisition timeout
values are unchanged. The shared lifecycle-entry guard is not held while
waiting for a per-model lock, constructing a model, or clearing model
resources.

This change addresses the demonstrated process-local load/remove/reload races
and fixed-size cache bookkeeping inconsistencies within one ModelManager
instance. It does not introduce active inference-use leases or cross-process
coordination.

Verification

The parent behavior was verified from:

aa9306bd8e3c674c3c0a324635307b7272eb54ef

Parent regression proof

The original lock-generation regression failed on the unchanged parent:

1 failed, 8 deselected

It observed two live lock generations and two concurrent constructors for the
same resolved model identifier.

The expanded mutation regression selection produced:

7 failed, 4 deselected

It reproduced:

split lock generations;
decorator warm-path bypass of an already-established remove/add order;
failed direct-removal rollback;
failed eviction rollback;
duplicate LRU entries;
alias lifecycle inconsistency;
eviction/reload inconsistency.

The additional exact-order rollback tests all failed on the unchanged parent:

3 failed

The tests cover:

test_fixed_size_cache_failed_direct_remove_preserves_exact_lru_order
test_fixed_size_cache_failed_eviction_restores_skipped_pinned_order
test_fixed_size_cache_failed_multi_eviction_keeps_only_registered_models

Candidate

The expanded mutation regression selection passed:

7 passed, 4 deselected

The additional exact-order rollback selection passed:

3 passed

Targeted manager tests:

python -m pytest \
  tests/inference/unit_tests/core/managers/test_model_lock_lifecycle.py \
  tests/inference/unit_tests/core/managers/test_decorators.py -q

23 passed

Manager and cache suites:

python -m pytest \
  tests/inference/unit_tests/core/managers \
  tests/inference/unit_tests/core/cache -q

300 passed

Full applicable CPU unit suite with a fresh model cache:

CACHE_ROOT="$(mktemp -d)"
MODEL_CACHE_DIR="$CACHE_ROOT/models" \
INFERENCE_HOME="$CACHE_ROOT/home" \
python -m pytest tests/inference/unit_tests -q

2580 passed, 10 skipped, 3 failed

The same three failures reproduce on the unchanged parent:

test_factory_logs_construction_failures_before_falling_back
test_cached_model_aggregation_excludes_conflicting_metadata
test_index_list_parameters_by_frame_id_warns_once_per_key

They are existing logging-capture failures where the warning is emitted but is
not available through caplog.

Incomplete implementations rejected

The regression coverage rejects intentionally incomplete alternatives:

retiring an entry immediately after the holder releases it;
protecting only lock creation without retaining the generation;
keeping the unlocked decorator warm return;
omitting failed-candidate eviction rollback;
omitting skipped-pinned rollback;
moving a registered model to the end after failed direct removal.

Quality gates

make check_code_quality

Passed:

Black: 830 files unchanged
isort
fatal Flake8 checks
scoped formatting and lint checks
git diff --check

The informational Flake8 pass still reports the existing upstream warning set.

Not verified

Live GPU or production-server behavior.
A measured RAM/VRAM spike or production OOM.
Upstream Python/platform CI has not run on this branch yet.
Removing or evicting a model while inference is actively using it.
Queue reconciliation after failed inference, preprocess, or other
non-mutation access paths.
Cross-process, cross-worker, cross-pod, or independent-manager coordination.
Lock fairness or starvation guarantees.
A strict global max_size guarantee during simultaneous cold loads of
different identifiers.

Risks / rollback

The lifecycle-entry guard briefly serializes entry reservation and retirement,
but it is never held during per-model lock waiting, model construction, or
resource cleanup.

Post-mutation queue reconciliation uses a blocking _queue_lock acquisition so
a secondary bookkeeping timeout cannot replace the original add/remove result
or exception. It can therefore wait behind an ongoing eviction.

The existing eviction path already holds _queue_lock while removing an
evicted model and running clear_cache(). This patch does not acquire
_queue_lock while holding a per-model lifecycle lock, so it does not
introduce the opposite lock order. Reducing the existing eviction critical
section is outside this change.

Different resolved model identifiers continue to use independent per-model
locks.

Reverting this PR restores the previous mutation and cache-bookkeeping
behavior.

Migrations / external effects

No migrations or public API changes.

A warm add_model() call that overlaps an already-started removal may now wait
for that removal to complete instead of returning from an unlocked decorator
fast path. Configured per-lock acquisition timeout values are unchanged, although this path may perform two sequential lock acquisitions.

Screenshots / preview

Not applicable.

Keep lifecycle entries reserved for holders, waiters, timeout callers, and exception paths until the exact generation is unused. Revalidate decorator warm paths and keep fixed-size cache bookkeeping consistent across aliases, rollback, and reload operations.

PawelPeczek-Roboflow · 2026-08-20T09:14:17Z

Hi there,
We appreciate contribution - since this is bigger, we would need more time to review it and will not be part of tomorrows release.

PawelPeczek-Roboflow · 2026-08-21T16:13:20Z

Hi there
Really sorry but we did not finish checking other PRs prior to todays release. We will try to make it reviewed next cycle.
Thanks for the contribution.

PawelPeczek-Roboflow · 2026-08-28T15:56:25Z

Hi there,

Thanks for the contribution. I've taken a look and looks like the problem you pointed is existing and the resolution may be correct - however, this is quite fundamental piece of the server which I would prefer not to introduce big changes for two reasons:

we are now building a replacement for the server which is meant to pay back a lot of technical debt and issues with model management like that
we are not seeing impact of this problem on the daily basis

That's not ideal state - I know. When I have a moment in the following weeks I will come back and reflect on the decision, but we are not merging it to todays release.

voropaevv · 2026-09-07T11:37:59Z

Thanks for explaining the server replacement work. I’ll keep this PR at its current scope rather than expand the existing manager.

The deterministic lock-generation and cache-rollback cases may still be useful as regression requirements for the replacement, even if this implementation is not adopted.

Is there a public component or issue in that effort where my concurrency and testing work would be useful? I’d like to align the next contribution with the direction the team is taking.

PawelPeczek-Roboflow · 2026-09-11T16:27:54Z

@voropaevv - sure - told this in couple of places already - but I wish to build better coms channel with contributors to coordinate actions. Drop me an e-mail: [email protected]

Regarding this PR - re-evaluating now to figure what we can do with it to both de-risk and solve problems

PawelPeczek-Roboflow · 2026-09-11T17:38:31Z

Took some consideration - this looks promising as a solution for the problem. To push it further, we would need to examine this closer under load tests in different scenarios before merging. Happy to do it as other priorities allow us. In the meantime - here you can take a look our work around new server: #2251
currently - we take an attempt to cut dependencies from workflows to old inference to effectively make it extractable from old ecosystem and pluggable into new server, which would be unblock for next steps.

voropaevv · 2026-09-12T10:30:14Z

Thanks for updating the branch, Paweł.

I checked ea2c6460 against the merged-in main baseline 0b201310.
make check_code_quality passes on both; the scoped formatting and lint
checks for the five PR files also pass.

My local pytest rerun stopped before test collection because the environment
is missing required dependencies (cv2 is the first missing import). The
same happens on the baseline, so I don’t have fresh unit-test results for
this head. The patch and your merge commit are unchanged.

Could you approve the pending UNIT TESTS - inference run
for this head?

For the subsequent load tests, I suggest comparing candidate and baseline
under same-key load/failure/reload contention, mixed-key cache churn, and
warm-path latency and resource retention. Is there a workload or deployment
configuration you’d prioritize for the first run?

voropaevv · 2026-09-17T05:32:22Z

Thanks for the discussion, Paweł. I completed the bounded real-server validation of ea2c6460 against its merged-in baseline 0b201310.

Each revision ran 90 minutes on ARM64 CPU and RTX 4090: four small ONNX models, cache size two, two closed-loop clients, cached artifacts and no request retries. BASE ran before the PR on each host. All four soaks completed with sustained model churn.

Environment	BASE requests / HTTP 503	PR requests / HTTP 503	Successful-request p95, BASE / PR
ARM64 CPU	58,382 / 47	58,344 / 49	374.8 / 374.2 ms
RTX 4090	46,013 / 499	45,771 / 435	430.6 / 433.3 ms

All 1,030 soak errors and 15 additional calibration errors have the response size and SHA-256 matching the InferenceModelNotFound response on both revisions. This identifies the response class, not the precise inter-thread ordering of every failure. The results do not establish a new candidate-specific regression, but they are not a clean correctness pass. One paired run also does not establish performance equivalence or improvement.

The original CPU retention comparison remains inconclusive because the resident sets differed. A separate short CPU pair used fresh processes, 10 minutes of churn, then explicit clear/reload to the same two models and five minutes of idle. First-to-last idle-minute RSS medians changed by +4 KiB on BASE and +196 KiB on the PR. This is a post-clear observation, not replacement evidence for retention after the original 90-minute soak, and it does not establish leak freedom. That short churn produced a separate 48/58 HTTP 503s on BASE/PR.

The recorded candidate manager/cache suite completed with 311 passing tests. The evidence includes the pinned revisions, environment and model manifests, test harness, request/resource logs, restored rotated server logs and replayable analysis. Production code was not changed for these runs.

The evidence is split into three ZIPs: report/reproduction, CPU evidence and GPU evidence. Extract all three into one directory, enter roboflow-2821-evidence/, and follow START_HERE.md. Verify the public-file checksums before running python3 analysis/replay_reader.py, which rewrites a derived memory-analysis file. Offline replay was verified after combined extraction; this recalculates the recorded measurements, not a new server run. The original live-run runbook contains environment-specific setup references.

For the next review, do these residual InferenceModelNotFound responses require further investigation before deciding on #2821, or would you prefer to track them as a separate follow-up?

roboflow-2821-cpu-evidence.zip
roboflow-2821-gpu-evidence.zip
roboflow-2821-report-reproduction.zip

PawelPeczek-Roboflow

We double-reviewed with @dkosowski87
looks good - approving now, next week we will merge, load-test internally and ship.

[ON HOLD FOR NEXT RELEASE]

voropaevv · 2026-10-12T15:39:49Z

Hi Paweł, a quick follow-up on this PR.

The pending CI runs have now expired while awaiting approval; the
failure annotations indicate approval expiry rather than test failures.
The head remains ea2c646.

Would you prefer me to update the branch from main before another CI
run, or leave it unchanged for now?

Thanks!

voropaevv · 2026-10-25T14:44:09Z

Thanks, Paweł. I updated the branch to 5e4f01ed, merging main at 649b4793 while preserving your merge commit and the existing PR history.

The conflict resolution keeps the lifecycle-lock semantics and uses the current alias/capability/output registration key consistently for the warm path. The diff against that main remains confined to the five PR files.

Local validation on this head: 355 manager/cache tests passed, and make check_code_quality passed. I also exercised the legacy HTTP interface with the actual manager and fixed-size cache on both candidate and baseline: load, warm reuse, clear, reload and a short two-client/four-key churn. Each completed 123 inference requests successfully, with 62 loads/clears and an empty registry at shutdown. This ASGI check used a deterministic test model, not neural weights.

The earlier 90-minute CPU/GPU results still refer to ea2c6460; this short integration check does not establish that the residual 503s are resolved. The scope remains unchanged, without active-inference leases.

Could you approve any CI runs that require maintainer authorization for the updated head?

voropaevv marked this pull request as ready for review August 19, 2026 10:35

voropaevv requested review from PawelPeczek-Roboflow, dkosowski87, grzegorz-roboflow, hansent, probicheaux, rafel-roboflow and yeldarby as code owners August 19, 2026 10:35

voropaevv mentioned this pull request Aug 20, 2026

Synchronous model loads run inside the shared workflows thread pool, stalling all concurrent workflow runs (latency collapse with idle GPU/CPU) #2448

Open

voropaevv mentioned this pull request Sep 7, 2026

Fix concurrent ModelManager mutations and LRU cache synchronization #2938

Open

Merge branch 'main' into codex/fix-2819-model-manager-mutation-lifecycle

ea2c646

PawelPeczek-Roboflow previously approved these changes Sep 25, 2026

View reviewed changes

merge: integrate main into model lock lifecycle fix

5e4f01e

voropaevv dismissed PawelPeczek-Roboflow’s stale review via 5e4f01e October 25, 2026 14:43

voropaevv requested a review from MTaraszewski as a code owner October 25, 2026 14:43

Navigation Menu

Repository navigation

Keep ModelManager lock generations and fixed-size cache bookkeeping consistent - #2821

Keep ModelManager lock generations and fixed-size cache bookkeeping consistent#2821
voropaevv wants to merge 3 commits into
roboflow:mainfrom
voropaevv:codex/fix-2819-model-manager-mutation-lifecycle

voropaevv commented Aug 19, 2026 •

edited

Loading

Uh oh!

PawelPeczek-Roboflow commented Aug 20, 2026

Uh oh!

PawelPeczek-Roboflow commented Aug 21, 2026

Uh oh!

PawelPeczek-Roboflow commented Aug 28, 2026

Uh oh!

voropaevv commented Sep 7, 2026

Uh oh!

PawelPeczek-Roboflow commented Sep 11, 2026

Uh oh!

PawelPeczek-Roboflow commented Sep 11, 2026

Uh oh!

voropaevv commented Sep 12, 2026

Uh oh!

voropaevv commented Sep 17, 2026

Uh oh!

PawelPeczek-Roboflow left a comment

Uh oh!

voropaevv commented Oct 12, 2026

Uh oh!

voropaevv commented Oct 25, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

voropaevv commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

Verification

Parent regression proof

Candidate

Incomplete implementations rejected

Quality gates

Not verified

Risks / rollback

Migrations / external effects

Screenshots / preview

Uh oh!

PawelPeczek-Roboflow commented Aug 20, 2026

Uh oh!

PawelPeczek-Roboflow commented Aug 21, 2026

Uh oh!

PawelPeczek-Roboflow commented Aug 28, 2026

Uh oh!

voropaevv commented Sep 7, 2026

Uh oh!

PawelPeczek-Roboflow commented Sep 11, 2026

Uh oh!

PawelPeczek-Roboflow commented Sep 11, 2026

Uh oh!

voropaevv commented Sep 12, 2026

Uh oh!

voropaevv commented Sep 17, 2026

Uh oh!

PawelPeczek-Roboflow left a comment

Choose a reason for hiding this comment

Uh oh!

voropaevv commented Oct 12, 2026

Uh oh!

voropaevv commented Oct 25, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

voropaevv commented Aug 19, 2026 •

edited

Loading