fix(models): slow models controller step and inaccurate controller health check

[benmccown]

Problem

How control loops decide whether they are healthy

Background work in the platform runs as control loops. Each loop is a thread that repeats the same cycle forever: run one step(), sleep for its poll interval, run the next step(). The models controller's interval defaults to 5 seconds.

The framework needs some way to tell whether a loop is still alive, so it records a timestamp each time a step begins — a heartbeat. Readiness compares that timestamp against a deadline of 3 x interval, which for the models controller is 15 seconds. If the loop has not produced a heartbeat within 15 seconds it is reported unhealthy, /health/ready returns 503, and Kubernetes takes the pod out of service.

That works as long as a step reliably finishes well inside 15 seconds. The heartbeat was only sent once per step, so the deadline was really a bet that no step would ever take longer than three intervals.

What went wrong

Models controller steps were taking roughly 54 seconds. Since the only heartbeat came at the start of a step, the loop looked alive for the first 15 seconds of each cycle and stalled for the remaining ~44. Readiness therefore alternated between passing and failing on a roughly 59-second cycle, and the controller pod spent most of its time marked unhealthy and pulled out of the Service — even though it was working the whole time, just slowly.

There are two separate defects behind that, and this PR addresses both:

1. The health model could not tell "slow" apart from "stuck", because progress was only reported once per step.
2. The models controller step was far slower than the work actually required.

Fix 1: report progress more than once per step

A step that has 400 items to get through legitimately takes longer than three poll intervals. What matters for liveness is not "did the step finish" but "is the loop still getting somewhere". So controllers now report progress as each unit of work completes, rather than only when a step begins.

How it is put together:

• Heartbeat holds a single timestamp meaning "last observed progress".
• HeartbeatMixin gives a controller an emit_heartbeat() method to call as it finishes each item.
• TrackLastExecutionTime owns the Heartbeat and hands it to the wrapped controller when it wraps it. Entering step() still counts as progress, so a controller that reports nothing behaves exactly as it did before and no existing call site changed.
• Nested helpers (the reconcilers) receive the bound emit_heartbeat method rather than the Heartbeat itself, so they report progress without knowing anything about how liveness is measured.
• The liveness check in Loop is untouched. It already read this timestamp; the timestamp is simply updated more often now.

The important property is that beats only ever follow completed work — never a timer, never around a call that might block. A single call that hangs forever still stops the clock and is still caught. Only slow-but-advancing work is now treated as healthy.

Wired into the jobs scheduler and reconciler, workspace cleanup, the models controller, and the adapters sidecar.

Separately, when a loop does go unhealthy it now says so in the log. Previously the only record was at DEBUG, so a pod failing readiness produced no visible explanation. Loop records why the check failed and ControllerManager logs the change with the loop name and the reason, on transition only, so it is not repeated on every probe.

Fix 2: make the models controller step faster

Where the time went

Provider reconciliation walks every model a provider serves and, for each one, makes sure two things exist: a Model Entity for it, and a passthrough VirtualModel pointing at that entity. It was doing this one model at a time:

• one GET to read the Model Entity, and
• one POST attempting to create the VirtualModel, relying on a 409 conflict to mean "already there".

In a development workspace with 4 providers each serving 102 models, that is 408 served models and 816 sequential API calls per step, every 5 seconds. At the measured ~60 ms per call that is most of a minute. In the steady state nothing needed changing, so essentially all of it was re-confirming state that was already correct.

Reading the same data in bulk instead is dramatically cheaper. Measured against that workspace:

	Calls	Time
Read 404 Model Entities one at a time	404	37.7 s
Read all 404 in one paginated list	1	0.33 s
Read all 306 VirtualModels (page_size=200)	2	0.33 s

What changed

ModelEntityCache (new) reads the Model Entities once per reconciliation phase and collects up the intended changes instead of writing them as it goes:

• each entity is written at most once, and only if something actually differs — so a converged pass performs no writes at all
• the desired state for an entity is assembled in memory, so several providers contributing to the same entity are merged into one write rather than overwriting each other
• reads see staged changes layered over the snapshot, so code that stages a change and then reads the same entity sees its own write
• refresh() refuses to re-read while changes are still staged, which makes the required ordering a failure rather than a convention

Both deployment and provider reconciliation read and write these entities, and the deployment phase can remove a provider link that the provider phase would otherwise put back. So the cache is refreshed at the start of each phase that writes entities and flushed at the end of that phase, meaning neither phase ever decides based on state the other has already changed.

VirtualModels are read once per pass. Orphan cleanup was already listing all of them on every step, so reusing that read costs nothing and removes all 408 create attempts. The existence set covers every VirtualModel rather than only auto-provisioned ones, so a name already held by a user-managed VirtualModel is still left alone. Cleanup consumes the same snapshot and still runs after every provider, because it depends on the served models resolved during the pass.

Result

Per-step API calls no longer scale with the number of served models. In the steady state the pass drops from ~826 calls to roughly a dozen, and from ~54 seconds to a couple of seconds — comfortably inside the 15-second liveness deadline, with Fix 1 covering the cold-start case where there genuinely is a lot to create.

Notes for reviewers

• Each provider's update_status now lands before its entity links are written, since the flush moved to end-of-phase. Publishing served_models independently of entity initialization is existing behaviour.
• Staged changes are applied even when a step bails out early: provider deletion stages the removal of its entity links and the provider itself is already gone, so discarding them would leave the links behind with nothing to trigger another attempt.
• Reporting progress means a step that advances slowly forever will not be flagged. That is intended, and is the point of the change.

Testing

• ModelEntityCache: overlay reads, refresh-while-staged rejection, single write for an entity linked by two providers, no write when converged, two providers creating one entity collapsing to a single create, conflict adoption, per-entity failure isolation, and a phase-ordering test that a removed link is not reinstated by the next phase.
• Liveness: a long step reporting progress stays healthy, a step reporting none goes unhealthy, and health changes log once per edge.
• Provider reconciliation ordering: VirtualModels read once per pass, existing and user-managed names not recreated, one created mid-pass never deleted as an orphan, concurrent deletion tolerated, and unresolved providers leaving served_models unset so cleanup falls back to persisted values.
• 1233 tests covering the changed code pass; ruff check, ruff format --check, and ty check are clean against baseline.

Summary by CodeRabbit

• New Features

  • Added heartbeat-based progress tracking for controllers to support more accurate health monitoring of long-running work.
  • Controllers now emit heartbeats across workspace cleanup, job reconciliation/scheduling, adapter reconciliation, and model reconciliation.

• Bug Fixes

  • Health monitoring now surfaces exact unhealthy reasons (including stalled progress or inactive loop threads).
  • Continued progress reporting even when per-item cleanup or reconciliation operations fail.

• Improvements

  • Reduced repeated unhealthy logging by only reporting health transitions; improved recovery reporting when conditions return to healthy.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8ee962c7-433e-4162-973b-ccee87c02289

📥 Commits

Reviewing files that changed from the base of the PR and between 0b41623 and feb0a76.

📒 Files selected for processing (12)

• packages/nmp_common/src/nmp/common/controller/controller.py
• packages/nmp_common/tests/controller/test_controller.py
• services/core/models/src/nmp/core/models/controllers/deployment_reconciler.py
• services/core/models/src/nmp/core/models/controllers/entity_cache.py
• services/core/models/src/nmp/core/models/controllers/models_controller.py
• services/core/models/src/nmp/core/models/controllers/provider_reconciler.py
• services/core/models/src/nmp/core/models/sidecars/adapters/main.py
• services/core/models/tests/unit/controllers/conftest.py
• services/core/models/tests/unit/controllers/test_deployment_reconciler.py
• services/core/models/tests/unit/controllers/test_entity_cache.py
• services/core/models/tests/unit/controllers/test_models_controller_unit.py
• services/core/models/tests/unit/controllers/test_provider_reconciler.py

🚧 Files skipped from review as they are similar to previous changes (7)

• packages/nmp_common/src/nmp/common/controller/controller.py
• services/core/models/src/nmp/core/models/sidecars/adapters/main.py
• services/core/models/src/nmp/core/models/controllers/models_controller.py
• services/core/models/src/nmp/core/models/controllers/deployment_reconciler.py
• services/core/models/src/nmp/core/models/controllers/entity_cache.py
• services/core/models/src/nmp/core/models/controllers/provider_reconciler.py
• services/core/models/tests/unit/controllers/test_provider_reconciler.py

---

📝 Walkthrough

Walkthrough

Changes

The PR adds shared heartbeat tracking and health reasons, transition-only health logging, a batched ModelEntityCache, cache-based model reconciliation, VirtualModel pass snapshots, and heartbeat reporting across controller loops.

Heartbeat and health reporting

Layer / File(s)	Summary
Heartbeat primitives and health reporting packages/nmp_common/src/nmp/common/controller/*, packages/nmp_common/tests/controller/*	Controllers share heartbeat timestamps; loops expose failure reasons; health logs are emitted only on transitions.
Heartbeat adoption in controllers services/core/entities/..., services/core/jobs/..., services/core/models/src/nmp/core/models/sidecars/adapters/*	Controllers emit heartbeats at phase boundaries and after per-item operations.

Model reconciliation

Layer / File(s)	Summary
Model entity cache lifecycle services/core/models/src/nmp/core/models/controllers/entity_cache.py, services/core/models/tests/unit/controllers/test_entity_cache.py	The cache refreshes snapshots, stages overlays, batches writes, handles conflicts, and retains failed mutations for retry.
Shared cache orchestration services/core/models/src/nmp/core/models/controllers/models_controller.py, deployment_reconciler.py, services/core/models/tests/unit/controllers/*	ModelsController shares the cache across phases and flushes staged changes during normal and exceptional paths.
Provider and VirtualModel reconciliation services/core/models/src/nmp/core/models/controllers/provider_reconciler.py, services/core/models/tests/unit/controllers/test_provider_reconciler.py	Provider reconciliation uses cached entity mutations, snapshots VirtualModels, avoids duplicate passthrough creation, and handles concurrent deletion.

Sequence Diagram(s)

sequenceDiagram
  participant ModelsController
  participant ModelEntityCache
  participant ModelDeploymentReconciler
  participant ModelProviderReconciler
  participant ModelsAPI
  ModelsController->>ModelEntityCache: refresh phase snapshot
  ModelsController->>ModelDeploymentReconciler: reconcile deployments
  ModelDeploymentReconciler->>ModelEntityCache: stage provider unlink
  ModelsController->>ModelEntityCache: flush staged changes
  ModelsController->>ModelProviderReconciler: reconcile providers
  ModelProviderReconciler->>ModelEntityCache: stage entity mutations
  ModelsController->>ModelEntityCache: flush staged changes
  ModelEntityCache->>ModelsAPI: create or update entities

Loading

Possibly related PRs

• NVIDIA-NeMo/nemo-platform#585: Overlaps the job controller files updated here.

Suggested labels: refactor, test

Suggested reviewers: mckornfield, albcui, crookedstorm

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.49% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the two main changes: faster models controller steps and improved controller health checks.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch models-controller-efficiency-fix

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

services/core/models/src/nmp/core/models/sidecars/adapters/main.py (1)

129-143: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Emit for temporary-directory cleanup too.

Line 134 skips Line 143. Many staging directories can be removed without a heartbeat, causing a false liveness failure. Put the emission in a per-iteration finally.

Proposed fix

             for name in set(os.listdir(self.nim_peft_source)) - dirs_to_keep:
-                # Staging temp dirs (".{dir}.tmp") were never loaded into vLLM;
-                # just reap them.
-                if name.startswith("."):
-                    shutil.rmtree(f"{self.nim_peft_source}/{name}")
-                    continue
-                # Unload from vLLM before deleting on disk so a removed/disabled
-                # adapter stops being served (no-op for NIM). Only delete the
-                # directory once the unload is confirmed (or vLLM has no endpoint):
-                # if vLLM is currently unreachable, keep the dir so the unload is
-                # retried next cycle rather than orphaning a still-loaded adapter
-                # in vLLM until it restarts (the dir is the only state driving GC).
-                if self._unload_vllm_adapter(name):
-                    shutil.rmtree(f"{self.nim_peft_source}/{name}")
-                self.emit_heartbeat()
+                try:
+                    if name.startswith("."):
+                        shutil.rmtree(f"{self.nim_peft_source}/{name}")
+                        continue
+                    if self._unload_vllm_adapter(name):
+                        shutil.rmtree(f"{self.nim_peft_source}/{name}")
+                finally:
+                    self.emit_heartbeat()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/core/models/src/nmp/core/models/sidecars/adapters/main.py` around
lines 129 - 143, Update the cleanup loop in the sidecar adapter management
method so every directory iteration emits a heartbeat, including staging
directories removed by the name.startswith(".") branch and iterations where
cleanup raises. Wrap each iteration’s cleanup logic in a per-iteration finally
block and keep emit_heartbeat() there, removing the current branch-dependent
emission.

🧹 Nitpick comments (4)

services/core/models/src/nmp/core/models/controllers/models_controller.py (1)

236-247: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Cache miss now means "absent" with no live fallback.

An entity created after the phase's refresh() (concurrent registration, or a sibling service) reads as missing for the whole phase, so ctx.model_entity is None and backends that need it to compile serving objects skip the deployment until the next tick. Consider a one-shot models.retrieve fallback on miss — it costs a round trip only for genuinely-unknown names, which is the rare case this batching was meant to avoid.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/core/models/src/nmp/core/models/controllers/models_controller.py`
around lines 236 - 247, Update the cache-miss branch in the model retrieval flow
around _entity_cache.get so it performs a one-shot await
self._models_sdk.models.retrieve fallback before returning None. Preserve the
cached entity path and existing debug logging, and return None only when both
the cache lookup and live retrieval fail.

services/core/models/tests/unit/controllers/test_provider_reconciler.py (1)

988-1008: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test name promises more than it checks.

test_entity_cache_load_failure_prevents_any_entity_writes never invokes the reconciler — it only calls refresh/flush directly, so it proves nothing about reconciliation aborting. Drive it through reconcile_model_providers (or rename to match what it asserts).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/core/models/tests/unit/controllers/test_provider_reconciler.py`
around lines 988 - 1008, Update
test_entity_cache_load_failure_prevents_any_entity_writes to invoke
reconciler.reconcile_model_providers after configuring the entity-cache load
failure, and assert the APIStatusError propagates while models.create and
models.update remain uncalled. Keep the test focused on proving reconciliation
aborts before any entity writes, rather than directly calling refresh and flush.

services/core/models/tests/unit/controllers/test_entity_cache.py (1)

181-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test asserts both writes were attempted, but not what happens to the failed one.

Add coverage for the state after a failed write — whether the staged change survives for retry. This is the behaviour flagged on flush() in entity_cache.py.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/core/models/tests/unit/controllers/test_entity_cache.py` around
lines 181 - 190, Extend test_one_failing_entity_does_not_stop_the_others to
assert the failed entity’s staged provider link remains available after flush
for a later retry, while the successful entity’s staged change is cleared. Use
the cache’s existing state-inspection or retry path rather than only checking
models.update.await_count, and preserve verification that both writes are
attempted.

services/core/models/tests/unit/controllers/test_deployment_reconciler.py (1)

22-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cache-seeding test scaffolding is triplicated. _AsyncPaginator, seed_entity_cache, and the entity stand-in factory are re-implemented in three modules with subtly different behaviour (only two define model_copy), so cache-overlay semantics can drift between suites.

• services/core/models/tests/unit/controllers/test_deployment_reconciler.py#L22-L49: delete the local _AsyncPaginator, seed_entity_cache, and _entity and import them from a shared conftest.py/helper module.
• services/core/models/tests/unit/controllers/test_provider_reconciler.py#L145-L148: drop the local seed_entity_cache in favour of the shared one.
• services/core/models/tests/unit/controllers/test_entity_cache.py#L14-L36: move _AsyncPaginator and _entity into the shared module as the single definition, keeping the model_copy stub.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/core/models/tests/unit/controllers/test_deployment_reconciler.py`
around lines 22 - 49, Centralize the cache-seeding test helpers in
services/core/models/tests/unit/controllers/conftest.py or the existing shared
helper: move _AsyncPaginator and _entity from test_entity_cache.py, preserving
the _entity model_copy stub, and define the shared seed_entity_cache there. In
services/core/models/tests/unit/controllers/test_deployment_reconciler.py,
remove the local definitions and import the shared helpers; in
services/core/models/tests/unit/controllers/test_provider_reconciler.py, remove
its local seed_entity_cache and use the shared helper.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/nmp_common/src/nmp/common/controller/controller_manager.py`:
- Around line 141-146: Update the exception handling around the health check in
the controller manager so repeated readiness-probe failures are not logged
unconditionally. Route exception details through _log_health_transition() and
emit the error, including traceback when needed, only when the loop transitions
to unhealthy; preserve health_status and all_healthy updates.

In `@packages/nmp_common/src/nmp/common/controller/controller.py`:
- Line 55: Update the _heartbeat annotation to reference the already-defined
Heartbeat type directly, removing the surrounding string wrapper while
preserving the optional type.

In
`@services/core/models/src/nmp/core/models/controllers/deployment_reconciler.py`:
- Around line 802-808: Update the unlink flow around _entity_cache.get so a
missing cache snapshot is refreshed or otherwise loaded before deciding the
model entity is absent. Distinguish an uninitialized cache from a genuinely
nonexistent entity, then stage_provider_unlink for provider_id when the
refreshed entity contains it; retain the current debug-and-continue behavior
only for confirmed absence.

In `@services/core/models/src/nmp/core/models/controllers/entity_cache.py`:
- Around line 155-177: Update EntityCache.flush so an entity whose _create or
_update operation raises is retained in _pending for a later retry instead of
being lost when the batch is swapped out. Preserve per-entity exception
isolation so refresh() and other pending entities continue processing, and
ensure successful entries remain cleared while failed provider-unlink changes
can be retried.

In `@services/core/models/src/nmp/core/models/controllers/provider_reconciler.py`:
- Around line 1017-1019: The debug message in the existing_vm_names branch
should not claim that a passthrough VirtualModel exists. Update the logger.debug
call in the surrounding reconciler method to describe that creation is being
skipped because the workspace/model name is already used by another user-managed
VirtualModel, while preserving the existing early return.
- Around line 332-339: Guard the _load_virtual_models call in
reconcile_model_providers so listing failures do not abort the provider pass. On
failure, mark the VirtualModel snapshot unavailable and skip both VirtualModel
creation and orphan cleanup, while continuing provider status updates and Model
Entity linking. Preserve normal creation and cleanup behavior when the listing
succeeds.

---

Outside diff comments:
In `@services/core/models/src/nmp/core/models/sidecars/adapters/main.py`:
- Around line 129-143: Update the cleanup loop in the sidecar adapter management
method so every directory iteration emits a heartbeat, including staging
directories removed by the name.startswith(".") branch and iterations where
cleanup raises. Wrap each iteration’s cleanup logic in a per-iteration finally
block and keep emit_heartbeat() there, removing the current branch-dependent
emission.

---

Nitpick comments:
In `@services/core/models/src/nmp/core/models/controllers/models_controller.py`:
- Around line 236-247: Update the cache-miss branch in the model retrieval flow
around _entity_cache.get so it performs a one-shot await
self._models_sdk.models.retrieve fallback before returning None. Preserve the
cached entity path and existing debug logging, and return None only when both
the cache lookup and live retrieval fail.

In `@services/core/models/tests/unit/controllers/test_deployment_reconciler.py`:
- Around line 22-49: Centralize the cache-seeding test helpers in
services/core/models/tests/unit/controllers/conftest.py or the existing shared
helper: move _AsyncPaginator and _entity from test_entity_cache.py, preserving
the _entity model_copy stub, and define the shared seed_entity_cache there. In
services/core/models/tests/unit/controllers/test_deployment_reconciler.py,
remove the local definitions and import the shared helpers; in
services/core/models/tests/unit/controllers/test_provider_reconciler.py, remove
its local seed_entity_cache and use the shared helper.

In `@services/core/models/tests/unit/controllers/test_entity_cache.py`:
- Around line 181-190: Extend test_one_failing_entity_does_not_stop_the_others
to assert the failed entity’s staged provider link remains available after flush
for a later retry, while the successful entity’s staged change is cleared. Use
the cache’s existing state-inspection or retry path rather than only checking
models.update.await_count, and preserve verification that both writes are
attempted.

In `@services/core/models/tests/unit/controllers/test_provider_reconciler.py`:
- Around line 988-1008: Update
test_entity_cache_load_failure_prevents_any_entity_writes to invoke
reconciler.reconcile_model_providers after configuring the entity-cache load
failure, and assert the APIStatusError propagates while models.create and
models.update remain uncalled. Keep the test focused on proving reconciliation
aborts before any entity writes, rather than directly calling refresh and flush.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e4a62d6e-d337-4e7c-93ca-a215ba8ce03a

📥 Commits

Reviewing files that changed from the base of the PR and between 48a2614 and 56e39f4.

📒 Files selected for processing (17)

• packages/nmp_common/src/nmp/common/controller/__init__.py
• packages/nmp_common/src/nmp/common/controller/controller.py
• packages/nmp_common/src/nmp/common/controller/controller_manager.py
• packages/nmp_common/tests/controller/test_controller.py
• packages/nmp_common/tests/controller/test_controller_manager.py
• services/core/entities/src/nmp/core/entities/controllers/workspace_cleanup.py
• services/core/jobs/src/nmp/core/jobs/controllers/reconciler.py
• services/core/jobs/src/nmp/core/jobs/controllers/scheduler.py
• services/core/models/src/nmp/core/models/controllers/deployment_reconciler.py
• services/core/models/src/nmp/core/models/controllers/entity_cache.py
• services/core/models/src/nmp/core/models/controllers/models_controller.py
• services/core/models/src/nmp/core/models/controllers/provider_reconciler.py
• services/core/models/src/nmp/core/models/sidecars/adapters/main.py
• services/core/models/tests/unit/controllers/test_deployment_reconciler.py
• services/core/models/tests/unit/controllers/test_entity_cache.py
• services/core/models/tests/unit/controllers/test_models_controller_unit.py
• services/core/models/tests/unit/controllers/test_provider_reconciler.py

[github-actions]

Suite	Lines Covered	Line Rate	Branch Rate
Unit Tests	27597/35351	78.1%	62.5%
Integration Tests	16108/34069	47.3%	19.8%

[mckornfield]

tried to call out some high level things, mostly q, yay readiness/liveness fixes

[ironcommit]

LGTM

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

services/core/models/src/nmp/core/models/controllers/entity_cache.py (1)

203-213: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve create_kwargs when adopting a concurrent create.

After ConflictError, _create() retrieves the existing entity and calls _update(), but _update() only applies field_updates and provider changes. Initial attributes supplied through stage_create(..., **create_kwargs) are therefore silently lost.

Merge the non-reserved create fields into the conflict update and add a conflict-path test.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/core/models/src/nmp/core/models/controllers/entity_cache.py` around
lines 203 - 213, The ConflictError branch in _create() must preserve
non-reserved attributes from create_kwargs when adopting the concurrently
created entity. Merge those create fields with the staged field updates before
calling _update(), excluding fields handled separately such as provider changes
or other reserved create parameters, and add a test covering stage_create(...,
**create_kwargs) through the conflict path.

services/core/models/src/nmp/core/models/controllers/models_controller.py (1)

465-478: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Flush deployment mutations on every exit.

If reconcile_deployments() or reconcile_orphans() raises or is cancelled, execution skips the normal flush. _pending then remains populated, and the next step’s refresh() raises UnflushedMutationsError, preventing recovery.

Wrap the deployment reconciliation phase in try/finally and flush there while preserving the phase boundary.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/core/models/src/nmp/core/models/controllers/models_controller.py`
around lines 465 - 478, The deployment reconciliation phase in the controller’s
main flow must flush pending entity mutations on every exit. Wrap the calls to
_deployment_reconciler.reconcile_deployments and reconcile_orphans in a
try/finally block, placing the existing _entity_cache.flush() in the finally so
it runs on success, exceptions, and cancellation while preserving the subsequent
stop-signal and ERROR GC phase boundary.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@services/core/models/src/nmp/core/models/controllers/entity_cache.py`:
- Around line 203-213: The ConflictError branch in _create() must preserve
non-reserved attributes from create_kwargs when adopting the concurrently
created entity. Merge those create fields with the staged field updates before
calling _update(), excluding fields handled separately such as provider changes
or other reserved create parameters, and add a test covering stage_create(...,
**create_kwargs) through the conflict path.

In `@services/core/models/src/nmp/core/models/controllers/models_controller.py`:
- Around line 465-478: The deployment reconciliation phase in the controller’s
main flow must flush pending entity mutations on every exit. Wrap the calls to
_deployment_reconciler.reconcile_deployments and reconcile_orphans in a
try/finally block, placing the existing _entity_cache.flush() in the finally so
it runs on success, exceptions, and cancellation while preserving the subsequent
stop-signal and ERROR GC phase boundary.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c0f8d4bc-d587-4d2d-8cc7-beb3ffdc1061

📥 Commits

Reviewing files that changed from the base of the PR and between 56e39f4 and 0b41623.

📒 Files selected for processing (6)

• services/core/models/src/nmp/core/models/controllers/entity_cache.py
• services/core/models/src/nmp/core/models/controllers/models_controller.py
• services/core/models/src/nmp/core/models/controllers/provider_reconciler.py
• services/core/models/tests/unit/controllers/test_deployment_reconciler.py
• services/core/models/tests/unit/controllers/test_entity_cache.py
• services/core/models/tests/unit/controllers/test_provider_reconciler.py

🚧 Files skipped from review as they are similar to previous changes (4)

• services/core/models/tests/unit/controllers/test_entity_cache.py
• services/core/models/tests/unit/controllers/test_deployment_reconciler.py
• services/core/models/tests/unit/controllers/test_provider_reconciler.py
• services/core/models/src/nmp/core/models/controllers/provider_reconciler.py