feat(telemetry): heartbeat v9 — TPA gate/prompt scan counters + trust_mode distribution - #1029
Merged
Merged
Conversation
…_mode distribution The v8 tpa_scanner counters only see scan JOBS, which most installs never start. The two TPA detection paths that run synchronously for ordinary users emitted nothing at all, so the fleet read as "the scanner never runs": - the trust_mode:scan tool-change gate (runtime.scanChangeIsClean) - the aggregated-prompt poisoning filter (server.scanAggregatedPrompts) Both now increment a delta counter in the tpa_scanner sub-object (tool_change_gate_scans / prompt_scans) via the telemetry counter registry, with the same window and reset-after-accepted-send semantics as every other counter. Counts of invocations only — never the server, the tool/prompt, or the verdict. The sub-object is still omitted when all counters (v8 and v9) are zero. Adds trust_mode_distribution: a state field counting configured servers per EffectiveTrustMode(), keyed by the fixed auto|scan|manual enum and recomputed from the live config on each heartbeat. It is the denominator the gate counter needs (only "scan" servers can produce a gate scan) and the first fleet-wide view of trust-tier adoption. schema_version bumped to 9. The anonymity scanner widens the tpa_scanner key whitelist and gains rule trust_mode_field_invalid so a producer-side regression leaking a server name as a histogram key blocks the send.
Deploying mcpproxy-docs with
|
| Latest commit: |
03d4154
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c688454a.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://feat-telemetry-v9-tpa-funnel.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 32687942431 --repo smart-mcp-proxy/mcpproxy-go
|
Cross-model review of the v9 payload flagged the trust_mode_distribution builder reading s.config unsynchronized. NotifyConfigChanged replaces that pointer wholesale under s.mu on every live config reload (REST apply, disk watcher), so the read is a genuine data race — and it was not new: eight sibling reads in buildHeartbeat, plus maybeRotateAnonymousID and persistConfig, had the same exposure. trust_mode_distribution just made it visible by walking cfg.Servers. buildHeartbeat now resolves the live config exactly once through a new liveConfig() accessor (lock, read, release — the mutex is non-reentrant and preflightSink takes it, so it must not be held across the build) and every downstream read uses that snapshot. maybeRotateAnonymousID and persistConfig take the config as a parameter so the rotation mutates and persists the same pointer the payload reported, instead of whatever s.config points at by then. Adds a regression test that hammers NotifyConfigChanged against concurrent buildHeartbeat calls; under -race it fails on the pre-fix code and also asserts the histogram is never torn across a swap (all three fixed keys, two servers total).
…accessor advanceUpgradeFunnel runs on the heartbeat loop right after a 2xx send and mutates config.Telemetry.LastReportedVersion, but it still read s.config directly -- the same race with the NotifyConfigChanged pointer swap that buildHeartbeat was just fixed for. Route it through liveConfig() so the whole heartbeat build-and-send path reads the live config through one accessor.
… out Round 2 of the cross-model review caught a regression in round 1's fix. The heartbeat now works from a config SNAPSHOT, and persistConfig writes the WHOLE config file — so an in-flight heartbeat could save its stale snapshot over a config the user had just applied: snapshot A, user applies B (already on disk), the in-flight anonymous-ID rotation saves A, B is silently rolled back. persistConfig now writes only while the config it was handed is still s.config, with the liveness check and the write under one s.mu hold so the swap cannot slip between them. Skipping is safe and self-healing: every caller's mutation is idempotent, so the next heartbeat re-evaluates it against the live config and persists then. advanceUpgradeFunnel drops its open-coded SaveConfig and goes through the same guarded path. Two regression tests, both verified to fail with the guard disabled: a direct persistConfig-after-swap case (asserting the guard is a liveness check, not a blanket refusal — the live config still writes) and the end-to-end rotation case that reaches persistConfig through maybeRotateAnonymousID.
Round 3 of the cross-model review found the round-2 guard was correct but that "skipping the write is safe because the mutation is idempotent" was not true for two of its callers. persistConfig now reports whether it wrote, and both callers act on that: - Anonymous-ID rotation is all-or-nothing. The caller transmits the snapshot's anonymous_id in the payload it is building, so an id that was never written to disk must never leave the machine: one annual rotation would otherwise surface as TWO identities (this heartbeat's unpersisted id, then the id the next heartbeat rotates the live config to) and fragment the install's telemetry continuity. On a skipped write the snapshot is restored. - The upgrade-funnel cursor is not self-healing. Leaving last_reported_version unadvanced makes the next heartbeat report the same previous_version again, double-counting one upgrade, so the advance is redone against the new live config when the guarded write was skipped. persistConfig also now distinguishes "nothing to persist" from "write skipped": a service with no cfgPath (in-memory/CLI use) has no on-disk state for the mutation to be inconsistent with, so it reports success and callers keep their mutation. Without that split the rotation rolled itself back in every test and CLI service — caught by TestIDRotatesAfter365Days. The residual clobber window the review also raised is documented on persistConfig rather than papered over: the daemon's config writers save the file BEFORE calling NotifyConfigChanged, so in that gap the old pointer still looks live. Closing it needs single-writer ownership of the config file, a config-layer change out of scope here; the guard narrows the exposure, it does not eliminate it. Tests: rotation rollback and the funnel end-state after a swap, both verified to fail against the unfixed code.
Round 4 of the cross-model review found the rotation rollback was itself not concurrency-safe. BuildPayload is exported and served from an HTTP handler (internal/httpapi), so a request can build a heartbeat concurrently with the heartbeat loop and two rotations can race on the same config. Interleaved, one captures the OTHER's freshly generated id as its "previous" value and restores that on rollback — leaving a never-persisted id in the config the payload then transmits, which is exactly the identity fragmentation the rollback was added to prevent. maybeRotateAnonymousID now holds s.mu across its whole check → generate → mutate → persist-or-rollback sequence, so the rotation is atomic against both the NotifyConfigChanged pointer swap and a second rotation. The loser of the race observes the winner's refreshed created_at and does nothing. Since s.mu is not reentrant, persistConfig splits into a locking wrapper and persistConfigLocked, which callers already holding the lock use. Test: eight concurrent rotations on one config must yield exactly one identity, and the id left in memory (the one the heartbeat would report) must be the id on disk. Verified to fail — as a detected data race — without the lock.
…t paths The schema-v9 work made buildHeartbeat snapshot-safe, but two writers of the same live *config.Config stayed outside s.mu. NotifyConfigChanged swaps that pointer under the mutex, so an unlocked read or mutation of it is a genuine data race, not a stale-but-safe view. advanceUpgradeFunnel resolved the pointer under the lock and then mutated cfg.Telemetry.LastReportedVersion after releasing it, while buildHeartbeat read that same field unlocked. Both run concurrently in production: the heartbeat loop advances the cursor right after a successful send, and BuildPayload is exported and served from the REST handler behind `mcpproxy telemetry show-payload`. Reproduced under -race, then fixed by moving the whole resolve -> check -> mutate -> persist pass under s.mu (advanceUpgradeFunnelOnce) and reading the four cfg.Telemetry scalars the payload reports in one locked pass (telemetryCursor). ensureAnonymousID had the same shape and one hazard more: it read s.config unlocked and called config.SaveConfig on it directly, bypassing the liveness check persistConfigLocked exists to enforce. Because Start() is launched with `go` from runtime/lifecycle.go it overlaps the daemon's config-reload path, so that write could put a stale whole-config file on top of a just-applied one. It now runs as one locked pass via persistConfigLocked, retrying against the new live config if the pointer moved, and Start's own telemetry-enabled check reads through liveConfig. Behaviour is unchanged on the single-threaded path; a genuine write error still keeps the in-memory id so the process reports one stable identity. Regression tests: TestAdvanceUpgradeFunnelConfigRace, TestEnsureAnonymousIDConfigRace, TestEnsureAnonymousIDDoesNotSaveStaleConfig.
… mutex preflightSink snapshotted s.config under s.mu but called EffectiveTelemetryEnabled(cfg) after unlocking, and that path dereferences cfg.Telemetry via config.IsTelemetryEnabled. Both ensureAnonymousIDOnce and advanceUpgradeFunnelOnce install that same pointer under s.mu when the config arrived without a telemetry block — the ordinary fresh-install shape. A locked write paired with an unlocked read is still a data race, and the two sides here are the request path (Record*, through preflightSink) and the heartbeat loop, which run concurrently by construction. Replaced the snapshot-then-dereference with telemetryEnabledLive(), which holds s.mu across the whole evaluation. EffectiveTelemetryEnabled only reads env vars and config fields, so it cannot re-enter the Service. Start()'s own telemetry-enabled check had the same shape and now goes through the same helper. Regression test: TestPreflightSinkTelemetryPointerRace.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Heartbeat schema v9, making the TPA funnel measurable.
tpa_scannersub-object —tool_change_gate_scansandprompt_scans— recorded through the telemetry counter registry (same shape asRecordTPAScanCompleted: window scoped, reset only after an accepted 2xx send).trust_mode_distribution:{auto, scan, manual}counts of configured servers byServerConfig.EffectiveTrustMode(), recomputed from the live config at heartbeat build time.schema_versionbumped 8 → 9.Why
The v8 counters only see scan jobs, which most installs never start. The two TPA detection paths that actually run for ordinary users emitted nothing at all, so the fleet read as "the scanner never runs":
internal/runtime.scanChangeIsClean— the synchronoustrust_mode: scantool-change gateinternal/server.scanAggregatedPrompts— the prompt-poisoning filterBoth called
scanner.ScanToolMetadataVerdictdirectly with no counter. And there was no fleet-wide view of which trust tier installs sit in — which is also the denominator the gate counter needs, since onlyscan-mode servers can produce a gate scan.Key decisions
prompt_scansis per prompt actually scanned (a malformed, unqualified prompt name short-circuits before the scanner and is not counted).tpa_scanneris still dropped entirely when every counter — v8 and v9 — is zero, so an install that scans nothing stays shape-identical to a v7 payload.trust_mode_distributionis a state field and is always emitted (all three keys, zeros included), matching theserver_protocol_countsconvention.docs/features/telemetry.md.tpa_scanner's key whitelist grows by the two counters (rulev8_field_invalid), and the histogram gets its own ruletrust_mode_field_invalid— fixedauto|scan|manualkeys with non-negative integer counts, or the heartbeat is blocked instead of sent. Trust tiers are resolved throughEffectiveTrustMode(), so an empty/typo'd mode and the legacyauto_approve_tool_changes/skip_quarantinefields all fold into the enum before counting.No frontend contract or OAS surface touches this payload, so no
contracts.tsregen ormake swaggerwas needed.Follow-ups (not in this PR)
mcpproxy-dash) ingestion oftool_change_gate_scans,prompt_scans, andtrust_mode_distribution.