refactor: pass ModuleCacheMetrics as Arc instead of process-global static

[iduartgomez]

Background

PR #4472 introduced MODULE_CACHE_METRICS as a process-global static to share metrics between two components that live on opposite sides of an architectural boundary:

• Writers (ModuleCache::insert / evict_to_budget / remove) — owned by RuntimePool, behind the contract-handler channel.
• Reader (ring.rs:1074) — the Ring snapshot task, which has no channel or reference to RuntimePool.

This mirrors the existing TRANSPORT_METRICS static and is correct given the single-RuntimePool-per-process production invariant. The static was the right call for the PR scope.

Problem

Process-global statics make test isolation harder: labeled caches in different unit tests share the same gauge pair, so parallel tests can observe each other's metric state. This is the same problem class that TRANSPORT_METRICS has.

Proposed refactor

Replace the static with an Arc<ModuleCacheMetrics> threaded through the call graph:

1. Construct Arc<ModuleCacheMetrics> at node startup (in Node::build or equivalent).
2. Pass it into RuntimePool::new → stored on each ModuleCache via ModuleCache::with_label.
3. Store the same Arc in RouterSnapshotInfo or Ring so the snapshot task can call .snapshot() from it.

Estimated scope: ~4 function signature changes (RuntimePool::new, ModuleCache::with_label, snapshot-state construction, and one or two call sites).

Acceptance criteria

• MODULE_CACHE_METRICS static removed.
• ModuleCacheMetrics constructed once and threaded as Arc through RuntimePool and the Ring snapshot path.
• Existing metric values / Prometheus labels unchanged (no dashboard breakage).
• Unit tests for ModuleCache create isolated metric instances without cross-talk.

References

• PR feat(telemetry): add module-cache occupancy + eviction gauges (#4440) #4472 (original implementation, discussion at module_cache.rs:267)
• TRANSPORT_METRICS — the existing static this mirrors; a follow-up to that one would be a natural pairing.

[github-actions]

🤖 Auto-labeled

Applied labels:

• T-enhancement (98% confidence)
• E-medium (80% confidence)
• A-developer-xp (92% confidence)

Reasoning: The issue describes a refactor to remove a process-global static and thread an Arc through the call graph to improve test isolation and metrics handling. This is an improvement to existing code rather than a bug or new feature, so T-enhancement is appropriate. The change requires touching several function signatures and coordinating construction/propagation of the metrics Arc across components (RuntimePool, ModuleCache, Ring), so E-medium reflects moderate effort. The primary impact is on developer experience and test isolation, so A-developer-xp is the most relevant area.

Previous labels: none

If these labels are incorrect, please update them. This helps improve the auto-labeling system.

[sanity]

Issue Priority Assessment

Generated: 2026-06-19T03:20:47.243280323+00:00
Issue hash: 07e96189

Value Assessment

The static MODULE_CACHE_METRICS is still present exactly as described in the issue. The issue is current and unresolved.

This is a low-urgency developer experience refactor with narrow impact. The MODULE_CACHE_METRICS static is genuinely present at module_cache.rs:267, and there's already one test at line 1175 that reads from the global static — which confirms the cross-test contamination risk the issue describes.

Assessment:

The issue is still fully relevant. MODULE_CACHE_METRICS remains a LazyLock<ModuleCacheMetrics> static at wasm_runtime/module_cache.rs:267, accessed by writers in ModuleCache methods and read by the ring snapshot task at ring.rs:1074. Nothing has changed since PR #4472 landed; the refactor hasn't been done.

The user impact is essentially zero for end users and minimal even for developers. The contamination risk is real — a test at line 1175 reads MODULE_CACHE_METRICS.snapshot().contract_evictions_total directly from the global, so parallel unit tests that exercise eviction can interfere with each other — but in practice the test suite appears to tolerate this today. The issue is correctly labeled E-medium because the refactor is mechanical (~4 signature changes as described) but requires touching a handful of files across an architectural boundary (node startup → RuntimePool → ModuleCache → Ring snapshot path). No user-visible behavior changes at all; Prometheus labels and metric values stay identical.

Strategically, this is pure hygiene. It follows an established pattern (the issue even notes TRANSPORT_METRICS as a precedent that has the same problem), and proper Arc-threading would make future test isolation for module-cache behavior much cleaner. However, it doesn't advance any user-facing feature, fix a production bug, or unblock any known work. The TRANSPORT_METRICS static has lived with the same problem for longer and hasn't caused notable issues. Given the project's current focus on correctness bugs (the recent broadcast fan-out fixes, GET assembly retry, etc.), this refactor has low strategic urgency.

There's no blocking factor — no known PR or feature is waiting on this. The only time pressure would be if someone starts writing more module-cache unit tests that rely on metric isolation, at which point the global static becomes actively painful. Until then it's a "good to have" cleanup that a new contributor looking for a bounded, well-scoped task would be well-served by.

Cost Assessment

This issue is well-scoped and the codebase matches the description closely. Here's my assessment:

Relevance: The issue is fully actionable. The code described in the issue exists exactly as described — MODULE_CACHE_METRICS is a LazyLock<ModuleCacheMetrics> static at module_cache.rs:267, read in ring.rs:1074 during snapshot tasks, and written from ModuleCache methods. No significant refactoring has occurred since PR #4472 that would invalidate the approach.

Scope and Complexity: This is a straightforward mechanical refactor touching roughly 5-6 locations across 3 files. The plan is accurate: ModuleCache::with_label needs a new Arc<ModuleCacheMetrics> parameter (or the struct stores it), RuntimePool::new needs to construct and thread the Arc, and the Ring snapshot path at ring.rs:1074 needs a reference to the same Arc. The trickiest part is bridging the Ring side — Ring and RuntimePool are architecturally separated by a channel boundary, so the Arc has to be threaded through node startup and stored somewhere the snapshot task can reach it (likely RouterSnapshotInfo construction site or a field on Ring itself). The issue acknowledges this and suggests storing it on RouterSnapshotInfo or Ring. Looking at the code, this is the one design decision that needs care: Ring is constructed independently of RuntimePool, so the Arc would need to be passed at construction time or injected afterward. Neither is exotic Rust, but it requires tracing the construction path for Ring to find the right injection point. Total function signatures touched: approximately 4-6 as the issue estimates.

External Dependencies: None. This is entirely internal refactoring with no third-party services, certificates, organizational approvals, or coordination needed.

Risk: Low-to-minimal. The metrics values, Prometheus labels, and snapshot fields are all unchanged — this is purely a wiring change. The one regression risk is accidentally creating two separate ModuleCacheMetrics instances (one for RuntimePool, one for Ring) rather than sharing the same Arc, which would cause the Ring snapshot to show stale or zero values. This is easy to catch in review by verifying the Arc clone count. The existing test at module_cache.rs:1175 that checks contract_evictions_total would catch any accidental disconnection of metrics recording.

Testing Effort: Light. The main test addition the issue asks for is isolated ModuleCacheMetrics instances in unit tests — since the static is removed, each test creates its own instance and there's no cross-talk by construction. The existing eviction test already exercises the metrics path; it just needs to be updated to pass a locally-constructed metrics instance rather than reading from the global. No new behavioral coverage is needed since the metrics logic itself is unchanged.

Overall this is a clean, low-risk ~2-3 hour refactor with no external blockers. The E-medium label is appropriate.

---

Generated by issue-prioritizer