feat(self-host): auto-clean metadata-registry on detach (gh-8749)

[nnshah1]

Overview

MetadataArtifactRegistry was a write-only sink: every move_to_self_host added entries via register, but nothing ever removed them on detach. The only cleanup method on the worker side (LocalModel::clear_self_hosted_artifacts) required a LocalModel handle that the static LocalModel::detach_from_endpoint helper — and through it, the Python unregister_model binding — doesn't have. Long-running workers leaked one entry per metadata file on every LoRA detach or model reload. This is the documented blocker on flipping DYN_SELF_HOST_METADATA default-on in the gh-8749 capstone.

Fix

Tag each registration with Owner = (connection_id, lora_slug) — the pair detach_from_endpoint already has on hand without an API change to the Python side. The registry keeps a secondary owners -> (slug, suffix) map populated by register. New unregister_for_owner(&owner) looks up that map and calls the existing unregister so there is no duplicated cleanup logic. The HTTP get read path is unchanged — no new locks on the hot path.

Details

• lib/runtime/src/metadata_registry.rs: added pub type Owner = (u64, Option<String>), new owners map field, new unregister_for_owner method, register signature gains a leading Owner parameter (single caller in the workspace — no external breakage). Existing register / unregister / get semantics preserved.
• lib/llm/src/local_model.rs: move_to_self_host now takes &Endpoint instead of &DistributedRuntime (one extra line to derive drt) and passes (drt.connection_id(), model_suffix.map(str::to_string)) as the owner. detach_from_endpoint calls unregister_for_owner before the discovery unregister — no-op when self-host was disabled or skipped. Deleted the never-called clear_self_hosted_artifacts method and the supporting attached_self_host_suffix: Option<String> field that only tracked one suffix per LocalModel anyway.

Test

• Existing two metadata_registry tests updated to the new register signature (semantics unchanged).
• New unregister_for_owner_clears_only_that_owner: registers one base + one LoRA from the same connection_id, detaches the LoRA via unregister_for_owner, asserts the LoRA entries are gone and base entries remain. Second call is a no-op (idempotent).
• Full dynamo-llm + dynamo-runtime lib test suites, clippy with -D warnings, and cargo fmt --check all green.

Out of scope

• The Python unregister_model API is unchanged — callers in components/src/dynamo/vllm/handlers.py do not need to pass a model name.
• Default-on flip of DYN_SELF_HOST_METADATA — still gated on this PR plus a multimodal positive e2e; tracked in the DEP (light): Worker self-hosted metadata files #8749 capstone.

Related

• DEP (light): Worker self-hosted metadata files #8749 — self-hosted metadata stack
• feat(self-host): native preprocessor consumes resolved slug_dir (gh-8749) #10037 — native preprocessor consumes resolved slug_dir (PR4a)
• feat(self-host): worker hosts metadata files via system_status_server (gh-8749) #8855 (worker HTTP hosting), feat(self-host): frontend MDC verify-and-cache pipeline (gh-8749) #9057 (frontend verify-and-cache), fix(self-host): harvest non-weight siblings into slug_dir (gh-8749) #9610 (frontend HF-sibling harvest), feat(self-host): worker advertises non-typed metadata siblings (gh-8749) #9707 (worker extra_files) — earlier merges in the stack

Summary by CodeRabbit

Summary

• Refactor

  • Updated self-hosted model metadata tracking to register rewritten artifacts under an explicit per-instance owner for stronger isolation.
  • Reworked metadata artifact registration and cleanup to be owner-scoped, including safer collision handling.
  • Removed the prior suffix-based self-hosted cleanup flow for local models.

• Enhancements

  • Extended the self-hosted metadata endpoint path to include namespace/component/endpoint, improving correct routing and lookup.

• Tests

  • Updated coverage for owner-scoped cleanup, collision behavior on conflicting registrations, and same-owner re-registration updates.

DYN-2477

[coderabbitai]

Walkthrough

This PR updates metadata artifact handling to use endpoint-scoped identity plus owner-scoped registration and cleanup. LocalModel and the system status server now pass namespace, component, and endpoint information through the metadata path.

Changes

Owner-scoped metadata artifact registration

Layer / File(s)	Summary
Metadata registry owner-scoped contract lib/runtime/src/metadata_registry.rs	MetadataArtifactRegistry adds Owner = (u64, Option<String>), stores (PathBuf, Owner) per expanded metadata key, changes register to return CollisionError on owner mismatch, updates get, adds unregister_for_owner, and revises tests for selective removal, collision handling, and endpoint coexistence.
LocalModel attach with owner-scoped registration lib/llm/src/local_model.rs	LocalModel drops attached_self_host_suffix, its builders stop initializing that field, attach passes the full Endpoint into move_to_self_host, and the helper derives drt, builds the owner tuple, rewrites metadata URLs with namespace/component/endpoint, and registers artifacts through registry.register(&owner, ...).
LocalModel detach cleanup lib/llm/src/local_model.rs	detach_from_endpoint derives the owner from instance_id and model_suffix, removes the public clear_self_hosted_artifacts method, and switches metadata cleanup to metadata_artifacts().unregister_for_owner(&registry_owner).
Status server metadata lookup lib/runtime/src/system_status_server.rs	spawn_system_status_server mounts /v1/metadata with namespace, component, and endpoint path segments, and metadata_file_handler uses the expanded key to fetch artifacts from the registry and logs the additional fields on missing or read-failure paths.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately captures the main change: automatic metadata cleanup on detach.
Description check	✅ Passed	The description is mostly complete and includes overview, fix, details, tests, and related context, but it omits the reviewer-start section and exact issue-link template.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

---

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

[nnshah1]

@coderabbitai full review

@devin-ai-integration please do a full code review

[coderabbitai]

@nnshah1: I’ll run a full review of the PR changes now.

✅ Action performed

Full review finished.

[nnshah1]

@coderabbitai review

(Earlier full-review request acked at 23:14 but no findings posted; re-issuing as a plain review.)

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== metadata_registry.rs ==="
cat -n lib/runtime/src/metadata_registry.rs

echo ""
echo "=== local_model.rs ==="
cat -n lib/llm/src/local_model.rs

Length of output: 44271

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Callers of MetadataArtifactRegistry methods ==="
rg -n "metadata_artifacts\(\)\." lib/ --type rust -A 1

echo ""
echo "=== Callers of registry.unregister (non-owner) ==="
rg -n '\.unregister\b' lib/ --type rust -C 2

echo ""
echo "=== clear_self_hosted_artifacts references (should be zero) ==="
rg -n "clear_self_hosted_artifacts" lib/ --type rust

echo ""
echo "=== attached_self_host_suffix references (should be zero) ==="
rg -n "attached_self_host_suffix" lib/ --type rust

echo ""
echo "=== Any other callers of move_to_self_host ==="
rg -n "move_to_self_host" lib/ --type rust -C 3

echo ""
echo "=== _instance usage in attach (check drop semantics) ==="
rg -n "_instance" lib/llm/src/local_model.rs

Length of output: 6746

---

---

@nnshah1 Here is the full review of the two changed files.

---

lib/runtime/src/metadata_registry.rs

✅ Overall design

The Owner = (u64, Option<String>) type alias is clean and unambiguous. Storing (PathBuf, Owner) per key and doing an O(n) linear scan in unregister_for_owner is entirely appropriate — the registry is bounded by the number of metadata files per model, which is small. The HTTP read path (get) acquires only a shared lock and is unchanged.

🔴 unregister (slug+suffix variant) is now dead production code

After this PR, MetadataArtifactRegistry::unregister has no production callers. The grep confirms it appears only in its own unit test (unregister_only_removes_matching_suffix). Every detach path now goes through unregister_for_owner. Consider one of:

// Option A — remove it entirely (breaking change if external crates call it)
// Option B — keep it but mark it explicitly as unused for now
#[allow(dead_code)]
pub fn unregister(&self, slug: &str, suffix: &str) { … }

Leaving it as-is risks it drifting out of sync with the owner-aware logic silently (e.g., unregister doesn't check owner, so calling it in a future code path would bypass ownership semantics).

⚠️ register panics on owner collision — consider a Result return

assert_eq!(
    prior, owner,
    "metadata-registry collision …",
);

A panic will unwind the calling tokio task (or propagate to the runtime if uncaught). The PR rationale correctly identifies this as a programming error, but in production it may surface due to a misconfigured deployment (two workers sharing a registry by accident). Returning anyhow::Result<()> would let the caller propagate the error gracefully via the existing ? chain in move_to_self_host. Keeping it as a panic is defensible if the team prefers a hard-fail posture here — just flagging it as a deliberate trade-off.

⚠️ get allocates three Strings per hot-path call

self.entries
    .read()
    .get(&(slug.to_string(), suffix.to_string(), filename.to_string()))

HashMap lookup requires an Eq + Hash-compatible key. A (&str, &str, &str) borrow won't match (String, String, String) out of the box without a custom Borrow impl. This is a micro-optimization and probably not worth the complexity now, but worth tracking if the metadata HTTP route becomes a bottleneck.

✅ Test coverage

• register_get_roundtrip ✔
• unregister_only_removes_matching_suffix ✔
• unregister_for_owner_clears_only_that_owner ✔ (new; covers the idempotent second-call case)
• register_panics_on_owner_collision ✔ (#[should_panic])
• register_same_owner_updates_path ✔

---

lib/llm/src/local_model.rs

✅ move_to_self_host signature change

Accepting &Endpoint instead of &DistributedRuntime is a cleaner boundary — the function needs both drt (for the registry + connection ID) and the model slug, and Endpoint provides both. The derived owner = (instance_id, model_suffix.map(str::to_string)) is correctly symmetric with what detach_from_endpoint reconstructs.

✅ detach_from_endpoint cleanup order

discovery.unregister(instance).await?;           // 1 — stop new requests being routed here
drt.metadata_artifacts().unregister_for_owner(&registry_owner); // 2 — clean up served files

Removing from discovery first is the right ordering: it prevents new fetch requests from being routed to this worker before the metadata entries are torn down. In-flight requests will still find the entries during the teardown window, which is the correct behavior.

✅ unregister_for_owner called unconditionally on detach

Safe because the method is documented as a no-op when owner was never registered (e.g., self_host_metadata = false). The asymmetry (conditional register, unconditional unregister) is fine.

✅ Removed dead fields and methods

attached_self_host_suffix: Option<String> and clear_self_hosted_artifacts have zero references in the codebase — confirmed clean removal.

⚠️ extra_files accumulation on repeated attach calls (pre-existing)

self.card.extra_files.extend(harvested);  // line 593

If attach is called more than once on the same LocalModel instance, extra_files grows additively. This is not introduced by this PR but is now more visible since move_to_self_host is called directly from attach. Out of scope here, but worth a follow-up if re-attach scenarios are expected.

---

Summary

Severity	Finding	File
🔴	MetadataArtifactRegistry::unregister is dead production code — remove or annotate	metadata_registry.rs
⚠️	register panics on collision — consider Result<()> for graceful handling	metadata_registry.rs
⚠️	get allocates 3 Strings per call	metadata_registry.rs
⚠️	extra_files.extend accumulates on repeated attach (pre-existing)	local_model.rs
✅	Ownership model design, detach ordering, test coverage	both

[nnshah1]

@coderabbitai re: register panic vs Result<()> — keeping the panic intentionally. The precondition is crisp (two LocalModel instances on one DRT must not attach the same (slug, suffix)) and is documented on register's doc comment. Across vllm/sglang/trtllm I don't see any backend that double-registers a slug+suffix in one process; if it ever happens, the panic surfaces the colliding owners immediately rather than letting half-cleanup leak silently.

Result<()> would just ?-propagate to move_to_self_host and bubble out of attach, killing the worker anyway — same outcome, just slower and less informative. Trimmed the message in 28d5aef to drop the 'model+suffix in one process are not supported' scaffolding; the panic now just names the colliding owners.

If a real topology surfaces this collision (disagg co-located on one DRT, hot reload), we'd convert to a HashSet<Owner> refcount in a separate change — that's the only design that actually supports the case, and Result<()> would just defer the same crash by one frame.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@nnshah1 — that rationale is airtight. The "same outcome, less informative" argument is the decisive one: Result<()> would unwind to attach → task abort anyway, losing the colliding-owners context that the panic message surfaces directly. The commit trimming the message to just the owners is the right call.

Withdrawing the Result<()> suggestion. The HashSet<Owner> refcount path you've described is the correct design if co-located disagg workers or hot-reload ever need to share a (slug, suffix) — good to have that documented as the direction.

---

✏️ Learnings added

Learnt from: nnshah1
URL: https://github.com/ai-dynamo/dynamo/pull/10351

Timestamp: 2026-06-24T16:39:47.775Z
Learning: In `lib/runtime/src/metadata_registry.rs` (ai-dynamo/dynamo), `MetadataArtifactRegistry::register` intentionally panics (rather than returning `Result<()>`) on owner collision for the same `(slug, suffix, filename)` key. The precondition — that two `LocalModel` instances on one DRT must not attach the same `(slug, suffix)` — is documented on the method's doc comment. The panic is preferred because `Result<()>` would propagate via `?` through `move_to_self_host` and kill the worker anyway, but with less context. If co-located disagg workers or hot-reload ever require shared `(slug, suffix)`, the correct design is a `HashSet<Owner>` refcount, not loosening to `Result<()>`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.