Give fault reporting a seam a test can observe - #823
Merged
Conversation
`report_anomaly` and `report_error` were associated functions calling the crash SDK directly. They worked only because that SDK keeps a process-global client — which meant they bypassed `Telemetry`'s state entirely and, more importantly, **nothing could observe that a path reported a fault**. That matters now rather than in principle. Instrumenting the background engine is mostly about failures, and d-outcome-not-intent settled that failure counting rides the crash sink rather than the analytics vocabulary — so the mechanism the telemetry work leans on for failure visibility was the one with no way to assert against it. Both are methods over a `Faults` field now: `Live` captures via the SDK, `DryRun` writes to the developer's console, and a cfg(test) `Recorded` variant keeps them in memory so a test can read them back. The dry run is a real gain of its own — until now a fault in a local build went nowhere at all, since a dev build has no crash client. Deliberately not a trait, per d-no-provider-traits: this is a seam for observation, not an abstraction over vendors. What it immediately bought — the spec's claim about logout is now checked rather than merely written. A logout that finds nothing stored is still a success (a second click must not error) but emits no event and reports an anomaly, because the settings page lists hosts from those very directories, so a missing one means the list and the erase disagree. Two tests: that the anomaly is reported, and that a logout which does erase something reports nothing — without the second, the first would pass on an implementation that reported every time. The failed-send path inside `track` also routes through the field now, so the whole fault path is observable rather than most of it. just lint 0, cargo fmt --check 0, 307 quilt-sync tests, workspace green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 5, 2026
Merged
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
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.
Third code unit of quiltdata/quilt-specs#37 (
w-report-seam), after #820 and #822.Problem
report_anomalyandreport_errorwere associated functions calling the crash SDK directly. They worked only because that SDK keeps a process-global client — which meant they bypassedTelemetry's state entirely and, more to the point, nothing could observe that a path reported a fault.That matters now rather than in principle. Instrumenting the background engine (next unit) is mostly about failures, and the change already settled that failure counting rides the crash sink rather than the analytics vocabulary — there are no
*_failedevents. So the one mechanism the telemetry work leans on for failure visibility was the one nothing could assert against.Approach
Both are methods over a
Faultsfield:Live— captures via the SDK, as before.DryRun— writes to the developer's console. A real gain on its own: until now a fault in a local build went nowhere at all, because a dev build has no crash client.Recorded(cfg(test)) — keeps them in memory so a test can read them back.Deliberately not a trait, per the
d-no-provider-traitsdecision on that PR: this is a seam for observation, not an abstraction over vendors.The failed-send path inside
trackroutes through the field too, so the whole fault path is observable rather than most of it.What it immediately bought
The spec's claim about logout is now checked rather than merely written. A logout that finds nothing stored is still a success — a second click must not raise an error — but it emits no event and reports an anomaly, because the settings page lists hosts from those very directories, so a missing one means the list and the erase disagree about the name.
Two tests, and the second earns its place: that the anomaly is reported, and that a logout which does erase something reports nothing. Without the second, the first would pass on an implementation that reported an anomaly every time.
Verification
just lintexit 0,cargo fmt --checkexit 0, 307 quilt-sync tests, workspace green. CI green on the branch.Five new tests: two on the logout behaviour above, and three on
Faultsitself — anomalies and errors recorded distinguishably and in order, only the recorder reports (so a test holding a live sink cannot pass by reading someone else's recording), and a local build dry-runs its faults.Locally checkable via the dry run from #820 — faults now print alongside events:
Note
One
cfg(test)enum variant, which is unusual enough to flag. The alternative was a trait — declined for the reasons on #37 — or leaving fault reporting unobservable, which is the status quo this unit exists to end.🤖 Generated with Claude Code
Greptile Summary
The PR introduces an instance-owned fault-reporting seam while preserving production Sentry reporting and making development and test behavior observable.
Faultsmodes for live capture, development console output, and test-only recording.Confidence Score: 5/5
The PR appears safe to merge, with production fault delivery preserved and the new development and test observation paths consistently wired.
All fault-reporting callers use the new instance methods, production still reaches the same Sentry capture APIs, and test recorders are isolated per telemetry instance.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Caller[Telemetry caller] --> Report[Telemetry report_anomaly / report_error] Track[Failed analytics send] --> Report Report --> Faults{Faults mode} Faults -->|Live| Sentry[Sentry SDK] Faults -->|DryRun| Console[Developer console] Faults -->|Recorded in tests| Memory[In-memory fault log] Memory --> Assertions[Test assertions]Reviews (1): Last reviewed commit: "Give fault reporting a seam a test can o..." | Re-trigger Greptile