Summary
crates/weaverd/src/dispatch/act/refactor/metrics.rs currently tracks position error counts using two process-level AtomicU64 statics:
POSITION_PARSE_ERROR_COUNTPOSITION_CONVERSION_ERROR_COUNT
These are incremented via a PositionMetrics trait whose production implementation (AtomicPositionMetrics) mutates the statics directly. This is global mutable state: it is shared across all concurrent request handlers, cannot be scoped to a single request or test, and requires explicit reset_test_counters() calls and #[serial] test annotations to avoid data races in the test suite.
Problem
- Test isolation: Tests that exercise any code path invoking
AtomicPositionMetrics share state. Without #[serial], parallel test runs produce non-deterministic counter values. The workaround (reset_test_counters()) is fragile and couples tests to global state.
- Composability: A single
AtomicPositionMetrics singleton cannot distinguish errors from different concurrent rename operations or different workspace sessions.
- Exportability: The statics are private; no clean seam exists to hook them into a metrics exporter without exposing the raw atomics or adding another global singleton.
Proposed Direction
Replace the global statics with an encapsulated metrics actor or a per-instance metrics registry:
- Actor approach: Introduce a long-lived
MetricsActor task that owns a HashMap<MetricKey, u64> and accepts increment messages over a channel. Each handler receives a MetricsSender handle (a cheaply-cloneable mpsc sender) injected at startup. The actor exposes a snapshot query for tests and for exporter integration.
- Registry approach: Use the
metrics crate façade (already used elsewhere in the daemon, if applicable) and replace the raw atomics with metrics::counter! calls. This delegates ownership to the metrics crate's global recorder, which can be swapped for a MockRecorder in tests without #[serial].
Either approach eliminates the global statics, removes the need for reset_test_counters(), and provides a clean seam for the export work tracked in issue #142.
Immediate Workaround
Until this is resolved, all tests in metrics.rs that read or mutate the global counters are annotated with #[serial] (from the serial_test crate) and carry a FIXME comment referencing this issue.
Background
Raised as an out-of-scope architectural follow-up from PR #126.
Requested by @leynos.
Related: #142 (metrics export), #125 (structured logging/tracing).
Backlink: #126
Summary
crates/weaverd/src/dispatch/act/refactor/metrics.rscurrently tracks position error counts using two process-levelAtomicU64statics:POSITION_PARSE_ERROR_COUNTPOSITION_CONVERSION_ERROR_COUNTThese are incremented via a
PositionMetricstrait whose production implementation (AtomicPositionMetrics) mutates the statics directly. This is global mutable state: it is shared across all concurrent request handlers, cannot be scoped to a single request or test, and requires explicitreset_test_counters()calls and#[serial]test annotations to avoid data races in the test suite.Problem
AtomicPositionMetricsshare state. Without#[serial], parallel test runs produce non-deterministic counter values. The workaround (reset_test_counters()) is fragile and couples tests to global state.AtomicPositionMetricssingleton cannot distinguish errors from different concurrent rename operations or different workspace sessions.Proposed Direction
Replace the global statics with an encapsulated metrics actor or a per-instance metrics registry:
MetricsActortask that owns aHashMap<MetricKey, u64>and accepts increment messages over a channel. Each handler receives aMetricsSenderhandle (a cheaply-cloneablempscsender) injected at startup. The actor exposes a snapshot query for tests and for exporter integration.metricscrate façade (already used elsewhere in the daemon, if applicable) and replace the raw atomics withmetrics::counter!calls. This delegates ownership to themetricscrate's global recorder, which can be swapped for aMockRecorderin tests without#[serial].Either approach eliminates the global statics, removes the need for
reset_test_counters(), and provides a clean seam for the export work tracked in issue#142.Immediate Workaround
Until this is resolved, all tests in
metrics.rsthat read or mutate the global counters are annotated with#[serial](from theserial_testcrate) and carry a FIXME comment referencing this issue.Background
Raised as an out-of-scope architectural follow-up from PR
#126.Requested by
@leynos.Related:
#142(metrics export),#125(structured logging/tracing).Backlink: #126