Skip to content

fix: stop double-reporting Segment transport errors and downgrade lossless retries to warnings - #9808

Draft
alejandro-jimenez-dcl wants to merge 1 commit into
mainfrom
bugsweep/segment-network-errors
Draft

fix: stop double-reporting Segment transport errors and downgrade lossless retries to warnings#9808
alejandro-jimenez-dcl wants to merge 1 commit into
mainfrom
bugsweep/segment-network-errors

Conversation

@alejandro-jimenez-dcl

Copy link
Copy Markdown
Contributor

Problem

Segment analytics transport errors bill the Sentry error budget twice per failure and
report genuinely-lossless retries at error level - a ~88 events/week family across 7 Sentry
groups, with a Sentry alert that keeps auto-minting duplicate GitHub issues from it.

Root cause

Three reporting-path defects (scope corrected by adversarial review):

  1. Channel duplication: the native report_error fans one failure into both C# callbacks -
    ErrorCallback (descriptive message) and Callback (bare Error code) - and both
    called LogException, so every failed operation was double-billed.
  2. The send-loop retry ("Error executing send loop (will retry)") is emitted when the
    native daemon retries WITHOUT consuming the spooled item - genuinely lossless, so
    Warning is the honest severity. A prior once-per-session latch band-aid (its own TODO:
    remove once the core issue is solved) is superseded.
  3. ReportHub.LogWarning(ANALYTICS, ...) was matrix-dead: no shipped
    ReportsHandlingSettings asset enabled (ANALYTICS, Warning), so any downgrade would
    have been total silence in real players.

Deliberately NOT reclassified (real data loss, kept at error level until the native fixes
land): instant-track "Network error" (event dropped, never spooled - lost revenue-funnel
events), "database is locked" flush failures (extracted batch dropped), "(will drop)",
and message too large.

Fix (~35 production LOC + 6 asset lines)

  • RustSegmentAnalyticsService: IsRetriedSendLoopError classifier matching only the
    provably-lossless retry pattern → LogWarning; everything else stays LogException; the
    once-per-session latch is deleted. Callback's not-Success report becomes a warning (the
    paired ErrorCallback always carries the descriptive error-level report for the same
    operation id).
  • ReportsHandlingSettings{Production,Development}.asset: enable (ANALYTICS, Warning) in
    the production sentry + debugLog matrices and the development debugLog matrix, pinned by
    an asset-loading test.

Net: ~66 of ~88 events/week reclassified with zero signal loss; the lossy patterns keep
billing the error budget by design. Follow-ups for the owning team (native): instant-track
enqueue fallback, flush ordering, sqlite busy_timeout.

Test

New EditMode RustSegmentErrorReportingShould (7 tests): every send-loop retry reported as
a warning (twice - proving the latch is gone), the duplicate operation-callback channel
downgraded, the shipped-matrix guard, and four Keep...AsException boundary tests pinning
the non-downgrade of the lossy/actionable patterns.

Validation

Windows Unity 6000.4 EditMode lane at the pin: RED FAIL 3/7 as intended (exception-level
logs where warnings are expected; matrices don't enable ANALYTICS warnings at pin; the 4
preservation guards pass at pin) / GREEN PASS 7/7.

Related: #8574 (open - P13 send-loop retries; stops accruing), #7906 (stale-closed JVW
tracker - NOT silenced: instant-track loss keeps error-level signal; its duplicate-channel
echo is removed), #7867 (closed - the duplicate-channel form). Intentionally unaffected:
#8218 (locked-flush batch loss) and #7866 (message-too-large) keep accruing until the
native fixes land.

Includes inspection-warning cleanup in all touched files.

Fixes #7906

…sless retries to warnings

## Problem

Segment analytics transport errors bill the Sentry error budget twice per failure and
report genuinely-lossless retries at error level — a ~88 events/week family across 7 Sentry
groups, with a Sentry alert that keeps auto-minting duplicate GitHub issues from it.

## Root cause

Three reporting-path defects (scope corrected by adversarial review):

1. Channel duplication: the native `report_error` fans one failure into both C# callbacks —
   `ErrorCallback` (descriptive message) and `Callback` (bare `Error` code) — and both
   called `LogException`, so every failed operation was double-billed.
2. The send-loop retry (`"Error executing send loop (will retry)"`) is emitted when the
   native daemon retries WITHOUT consuming the spooled item — genuinely lossless, so
   Warning is the honest severity. A prior once-per-session latch band-aid (its own TODO:
   remove once the core issue is solved) is superseded.
3. `ReportHub.LogWarning(ANALYTICS, ...)` was matrix-dead: no shipped
   `ReportsHandlingSettings` asset enabled (ANALYTICS, Warning), so any downgrade would
   have been total silence in real players.

Deliberately NOT reclassified (real data loss, kept at error level until the native fixes
land): instant-track `"Network error"` (event dropped, never spooled — lost revenue-funnel
events), `"database is locked"` flush failures (extracted batch dropped), `"(will drop)"`,
and `message too large`.

## Fix (~35 production LOC + 6 asset lines)

- `RustSegmentAnalyticsService`: `IsRetriedSendLoopError` classifier matching only the
  provably-lossless retry pattern → `LogWarning`; everything else stays `LogException`; the
  once-per-session latch is deleted. `Callback`'s not-Success report becomes a warning (the
  paired `ErrorCallback` always carries the descriptive error-level report for the same
  operation id).
- `ReportsHandlingSettings{Production,Development}.asset`: enable (ANALYTICS, Warning) in
  the production sentry + debugLog matrices and the development debugLog matrix, pinned by
  an asset-loading test.

Net: ~66 of ~88 events/week reclassified with zero signal loss; the lossy patterns keep
billing the error budget by design. Follow-ups for the owning team (native): instant-track
enqueue fallback, flush ordering, sqlite busy_timeout.

## Test

New EditMode `RustSegmentErrorReportingShould` (7 tests): every send-loop retry reported as
a warning (twice — proving the latch is gone), the duplicate operation-callback channel
downgraded, the shipped-matrix guard, and four `Keep...AsException` boundary tests pinning
the non-downgrade of the lossy/actionable patterns.

## Validation

Windows Unity 6000.4 EditMode lane at the pin: RED FAIL 3/7 as intended (exception-level
logs where warnings are expected; matrices don't enable ANALYTICS warnings at pin; the 4
preservation guards pass at pin) / GREEN PASS 7/7.

Related: #8574 (open — P13 send-loop retries; stops accruing), #7906 (stale-closed JVW
tracker — NOT silenced: instant-track loss keeps error-level signal; its duplicate-channel
echo is removed), #7867 (closed — the duplicate-channel form). Intentionally unaffected:
#8218 (locked-flush batch loss) and #7866 (message-too-large) keep accruing until the
native fixes land.

Includes inspection-warning cleanup in all touched files.
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 9m 0s —— View job


Review

  • Gather context (diff, changed files, native Rust source, report-handling framework)
  • Dependency / supply-chain review (triggered by new-dependency label)
  • Workflow / prompt file review
  • Functional code review of RustSegmentAnalyticsService + tests + report matrices
  • Post findings

Part 1 — Dependency / supply-chain review

The new-dependency label appears to be a mislabel (most likely path-matched on Explorer/Assets/Plugins/**). There are no dependency changes in this PR.

Step 1 — dependency changes identified

Item Result
Packages/manifest.json [PR] unchanged — git diff origin/main...HEAD -- Packages/ is empty
Packages/packages-lock.json [PR] unchanged
New binary / native plugin files [PR] none. Pre-computed binary list from the diff harness: None. Explorer/Assets/Plugins/RustSegment/SegmentServerWrap/Libraries/ is untouched
.asmdef / .asmref changes [PR] none. DCL.EditMode.Tests.asmdef is unmodified and already carries GUID:d28a7e4beeca475418c15757abf1b6f1, the target of RustSegment.Server.asmref, so the new test compiles without a wiring change
Editor / build / install scripts that auto-execute [PR] none
.native/Cargo.toml [PR] unchanged. Only read during this review, to confirm the provenance of a string literal (see below)

The only files touched are two .asset files, one production .cs, and one new EditMode test .cs + .meta.

Steps 1.5–3 (registry metadata, capability assessment, transitive risk) are not applicable — there is no new or changed dependency to look up.

Step W — workflow / prompt files

[PR] Nothing under .github/workflows/ or .github/prompts/ changed. The harness confirms: Workflow / prompt files changed in this PR: None. W.1–W.10 are all non-applicable.

Summary table

dependency / file version source type scope risk evidence confidence
(none added or changed) HIGH

No MEDIUM or HIGH supply-chain items.

DEPENDENCY_REVIEW: PASS


Part 2 — Functional review

The core diagnosis is sound and I verified the load-bearing claims against the sources rather than taking the description at face value:

  • Duplicate channel is real. AppContext::report_error (.native/src/server.rs:426-436) unconditionally calls error_fn(message) and callback_fn(id, Response::Error); report_success is the only other callback_fn caller. Every Response::Error is guaranteed a paired descriptive ErrorCallback.
  • Warnings really do become breadcrumbs, not issues. SentryReportHandler.CaptureMessage routes SentryLevel.Warning to AddBreadcrumb (SentryReportHandler.cs:146-148), so the error-budget claim holds.
  • The debouncer will be live in production. Both shipped assets have debounceEnabled: 1.
  • Duplicate - Category: ANALYTICS rows in the assets are correct, not a merge accident. CategorySeverityMatrix.InitializeLookupCache keys on (Category, Severity), and CategorySeverityMatrixDrawer.ModifyMatrix appends exactly this shape.
  • Deleting ONCE_PATTERN_ALREADY_CAUGHT is a strict improvement — the latch hid every distinct stuck item after the first, for the whole session.
  • The Mutex<T> nullability change is runtime-identical (annotations erased), so the reflection cast in the test is valid.

Findings

1 · HIGH — the matrix flip is category-wide and un-mutes 18 unrelated, un-debounced sites  ·  inline

(ANALYTICS, Warning) is a whole-category cell. Enabling it in the production Sentry + DebugLog matrices also un-mutes 18 ReportHub.LogWarning(new ReportData(ReportCategory.ANALYTICS), …) sites in SentryTransactionManager.cs, none of which carry a debouncer. At least one path is per-web-request: SentryWebRequestHandler.OnRequestStarted early-returns for file:// URLs (SentryWebRequestHandler.cs:45) without registering a transaction, while OnRequestFinished (:101) and OnProcessDataFinished (:111-112) call StartSpan / EndCurrentSpan / EndTransaction unconditionally — three ANALYTICS warnings per such request. Sentry's breadcrumb ring is bounded (default 100/scope), so this can evict the breadcrumbs that make real crashes diagnosable.

I did not measure the production rate of those 18 sites, so the actual volume is UNKNOWN — but the PR reasons only about the two Segment call sites and doesn't account for the other 18 riding along. Scope the flip to a dedicated ReportCategory (e.g. ANALYTICS_TRANSPORT) rather than the whole ANALYTICS category. Fix this →

2 · MEDIUM — Callback's replacement warning is undebounced and strictly redundant  ·  inline

RustSegmentAnalyticsService.cs:278 uses the implicit string → ReportData conversion, so Debounce is NONE. Since the paired ErrorCallback always carries the descriptive message, LogType.Log (as already used three lines above for the success case) is the honest level here. This line is also the only thing in the PR that requires Warning in the DebugLog matrix — dropping it to Log shrinks finding 1's blast radius.

3 · MEDIUM — the classifier string is owned by an external crate  ·  inline

"Error executing send loop (will retry)" is emitted by AnalyticsEventSendDaemon in the segment crate (.native/Cargo.toml, pinned rev = "2d46f10f…"), not by anything in this repo. Nothing couples the C# literal to that rev. It fails safe, so not a blocker — the comment at :251-256 should just name the source.

4 · MEDIUM — reflection-bound tests where [InternalsVisibleTo] is the project convention

RustSegmentErrorReportingShould.cs binds by string to RETRY_REPORT, ErrorCallback, Callback, CURRENT, afterClean, and NativeMethods.Response. CLAUDE.md §10 says to expose internals via [InternalsVisibleTo]. RustSegment.Server has no AssemblyInfo.cs; adding one with [assembly: InternalsVisibleTo("DCL.EditMode.Tests")] and making IsRetriedSendLoopError / RETRY_REPORT internal turns renames into compile errors instead of runtime NREs, and collapses most of CreateServiceWithPendingOperation's 15 lines of reflection.

5 · LOW — ReportHub.Instance static coupling in EditMode tests

The tests rely on the default DefaultReportLogger (no matrix, no debounce). Any test in DCL.EditMode.Tests that calls ReportHub.Initialize would silently change behaviour by ordering. MockedReportScope does the save/restore correctly but lives in the PlayMode assembly — worth mirroring in [SetUp]/[TearDown].

6 · LOW — DebounceRepeatedIdenticalRetryMessages mutates production static state

Correctly guarded with a per-run GUID fingerprint, but each run leaves a tracker in the shared static dictionary for up to the 5-minute cleanUpInterval. Harmless; worth a one-line note.

7 · LOW / pre-existing — the debouncer counter is shared across handlers

ReportHandlerBase.IsLogMessageAllowed calls Debounce once per handler on the same ProgressiveWindowDebouncer, and Debounce increments Count on every call. With allowedRepetitions = 3 and two enabled handlers, the warm-up allowance is effectively ~1.5 emissions and the two handlers can diverge on which ones they see. The test's AppliedTo == ReportHandler.All assertion reads as "both handlers get equal treatment", which isn't quite what happens. Same shape already exists at RPCSocialServiceBase.cs:98, so this is a framework property, not a regression.

Verdict

Findings 2–7 are refinements. Finding 1 is the one I'd want resolved before merge — as written, the PR trades ~66 error-level events/week for an unmeasured stream of un-debounced warnings across 18 unrelated call sites, on both the player log and the Sentry breadcrumb ring. Scoping the matrix change to a dedicated category keeps the entire benefit with none of that exposure.


I did not run the EditMode test lane (no Unity in this environment) or the ReSharper lint hook — the PR's own Windows Unity 6000.4 validation is the reference for those. No files were changed.
· branch bugsweep/segment-network-errors

@alejandro-jimenez-dcl alejandro-jimenez-dcl self-assigned this Aug 19, 2026
Comment on lines +505 to +506
- Category: ANALYTICS
Severity: 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Category-wide flip un-mutes 18 unrelated call sites. (ANALYTICS, Warning) is a whole-category cell, and the PR's own defect #3 correctly notes it was matrix-dead. Turning it on here doesn't just enable RustSegmentAnalyticsServiceSentryTransactionManager.cs has 18 ReportHub.LogWarning(new ReportData(ReportCategory.ANALYTICS), …) sites, none of which carry a debouncer, and they now reach both the production player log and SentrySdk.AddBreadcrumb.

At least one path is per-web-request: SentryWebRequestHandler.OnRequestStarted early-returns for file:// URLs (SentryWebRequestHandler.cs:45) so no transaction is registered, but OnRequestFinished (:101StartSpan) and OnProcessDataFinished (:111-112EndCurrentSpan + EndTransaction) run unconditionally — three ANALYTICS warnings per such request.

Sentry's breadcrumb ring is bounded (default 100 per scope), so an un-debounced ANALYTICS warning stream evicts the breadcrumbs that make real crash reports diagnosable — inverting the PR's goal.

Suggested scoping: add a dedicated constant to ReportCategory.cs (e.g. ANALYTICS_TRANSPORT) for the Segment FFI reports and enable Warning only for that cell, leaving ANALYTICS/Warning off. Alternatively, state the measured production volume of those 18 sites so the trade is explicit.

Comment on lines +251 to +258
// The native send daemon retries a "(will retry)" send-loop failure without consuming the
// spooled item, so no events are lost and it must not bill the Sentry error budget.
// Every other native error is a potential loss signal and keeps exception-level reporting:
// instant_track_and_flush drops the event on send failure (bare "Network error", never
// spooled), a failed flush drops the already-extracted batch ("Cannot flush: ..."), and
// the "(will drop)" send-loop branch consumes the item.
private static bool IsRetriedSendLoopError(string message) =>
message.Contains("Error executing send loop (will retry)");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The matched literal is not produced by anything in this repo — it comes from AnalyticsEventSendDaemon in the segment crate (.native/Cargo.toml: git = "https://github.com/decentraland/segment", rev = "2d46f10f9a5da2feac6554daedfc17d1ba49952e"). Nothing couples this C# string to that rev, so a future crate bump can silently change the wording and the classifier stops matching.

It fails in the safe direction (back to error level, i.e. the current behaviour), so this isn't a blocker — but the comment should name the source so whoever bumps the rev has a breadcrumb:

Suggested change
// The native send daemon retries a "(will retry)" send-loop failure without consuming the
// spooled item, so no events are lost and it must not bill the Sentry error budget.
// Every other native error is a potential loss signal and keeps exception-level reporting:
// instant_track_and_flush drops the event on send failure (bare "Network error", never
// spooled), a failed flush drops the already-extracted batch ("Cannot flush: ..."), and
// the "(will drop)" send-loop branch consumes the item.
private static bool IsRetriedSendLoopError(string message) =>
message.Contains("Error executing send loop (will retry)");
// The native send daemon retries a "(will retry)" send-loop failure without consuming the
// spooled item, so no events are lost and it must not bill the Sentry error budget.
// Every other native error is a potential loss signal and keeps exception-level reporting:
// instant_track_and_flush drops the event on send failure (bare "Network error", never
// spooled), a failed flush drops the already-extracted batch ("Cannot flush: ..."), and
// the "(will drop)" send-loop branch consumes the item.
// The matched text is emitted by AnalyticsEventSendDaemon in the `segment` crate pinned in
// .native/Cargo.toml; re-check this literal when that rev is bumped.
private static bool IsRetriedSendLoopError(string message) =>
message.Contains("Error executing send loop (will retry)");

// carrying the descriptive message — severity classification lives there.
if (response is not NativeMethods.Response.Success)
ReportHub.LogException(new Exception($"Segment operation {operationId} {type} failed with: {response}"), ReportCategory.ANALYTICS);
ReportHub.LogWarning(ReportCategory.ANALYTICS, $"Segment operation {operationId} {type} failed with: {response}");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplicate-channel premise checks out against the native source — AppContext::report_error (.native/src/server.rs:426-436) unconditionally calls error_fn(message) and then callback_fn(id, Response::Error), and report_success is the only other callback_fn caller. So every Response::Error that reaches here is guaranteed to have a paired descriptive ErrorCallback. Good analysis.

Two follow-ups on the replacement severity though:

  1. ReportCategory.ANALYTICS here goes through the implicit string → ReportData conversion, so Debounce is NONE. Combined with the matrix change, every failed operation now unconditionally adds a Sentry breadcrumb. Debouncing wouldn't help anyway (the message embeds a unique operationId) — which is really an argument that this report carries no information the paired ErrorCallback doesn't already carry.
  2. Given (1), LogType.Log (ReportHub.Log(...), already used three lines above for the success case) would be the honest level for a strictly-redundant report, and it wouldn't depend on the (ANALYTICS, Warning) matrix flip at all. Right now this line is the only thing in the PR that needs Warning enabled for the DebugLog matrix.

@decentraland-bot
decentraland-bot self-requested a review August 19, 2026 12:38
@alejandro-jimenez-dcl
alejandro-jimenez-dcl marked this pull request as draft August 19, 2026 12:49

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jarvis Review — PR #9808

STEP 2 — Root-cause check

The PR correctly identifies three reporting-path defects:

  1. Channel duplication: report_error fans one failure into both ErrorCallback (descriptive message) and Callback (bare Error code), and both called LogException → double Sentry error-budget billing.
  2. Lossless retry at error level: The "(will retry)" send-loop error is emitted when the native daemon retries without consuming the spooled item — genuinely lossless, so Warning is the honest severity.
  3. Matrix-dead Warning: No shipped ReportsHandlingSettings asset enabled (ANALYTICS, Warning), so any downgrade would have been silent.

Verified against native source: AppContext::report_error (.native/src/server.rs:426-436) unconditionally calls error_fn(message) and callback_fn(id, Response::Error), confirming the paired-callback premise.

The fix addresses root causes, not symptoms. The deliberate preservation of error-level for lossy patterns ("Network error", "database is locked", "(will drop)", message-too-large) is well-reasoned.

PASS


STEP 3 — Design & integration

No new lifecycle units introduced. All changes are within the existing RustSegmentAnalyticsService:

  • RETRY_REPORT — static configuration for debouncing, not a lifecycle manager
  • IsRetriedSendLoopError — pure stateless classifier
  • Nullable annotation cleanup on CURRENT — type-correctness only

No ownership concerns, no reconciliation patterns, no persistent state outside ECS (the debouncer is infrastructure configuration). The ProgressiveWindowDebouncer is static and shared across the singleton service's lifetime — appropriate since only one RustSegmentAnalyticsService instance exists at a time (mutex-enforced).

The deletion of ONCE_PATTERN_ALREADY_CAUGHT is a strict improvement — the replacement (debounced warning with progressive windowing) is superior: it bounds volume while keeping distinct stuck items visible.

PASS


STEP 4 — Member audit

No new public members. All additions are private static:

  • RETRY_REPORT — consumed only by ErrorCallback
  • IsRetriedSendLoopError — consumed only by ErrorCallback

The Callback method behavior change (LogException → LogWarning for non-Success) is internal to the native callback. No single-use accessor, absent≠false, or re-derivation issues.

PASS


STEP 5 — Line-level review

Finding 1 — [P1] Category-wide matrix flip un-mutes 18 unrelated, un-debounced warning sites

Location: ReportsHandlingSettingsProduction.asset (both debugLog and sentry matrices) and ReportsHandlingSettingsDevelopment.asset

Enabling (ANALYTICS, Warning) in the production matrices also un-mutes 18 ReportHub.LogWarning(new ReportData(ReportCategory.ANALYTICS), …) calls in SentryTransactionManager.cs (lines 69, 80, 91, 102, 113, 124, 135, 212, 218, 233, 239, 258, 264, 293, 340, 379, 385, 428), none of which carry a debouncer.

Critically, SentryWebRequestHandler.OnRequestStarted early-returns for file:// URLs without registering a transaction, but OnRequestFinishedInstance.StartSpan() and OnProcessDataFinishedInstance.EndCurrentSpan() + Instance.EndTransaction() still execute. Each hits the "transaction not found" guard, producing 3 ANALYTICS warnings per such request. These now reach both the player log and Sentry breadcrumbs.

Sentry's breadcrumb ring is bounded (default 100 per scope), so an un-debounced ANALYTICS warning stream can evict the breadcrumbs that make real crash reports diagnosable — inverting the PR's goal.

Fix: Introduce a dedicated ReportCategory constant (e.g. ANALYTICS_TRANSPORT) for the Segment FFI reports and enable Warning only for that cell. This keeps the entire Segment downgrade benefit while leaving the 18 SentryTransactionManager sites muted. See inline comment for details.

Finding 2 — [P2] Callback warning is strictly redundant — Log is the honest level

Location: RustSegmentAnalyticsService.cs:278

The native report_error always pairs a failed operation with an ErrorCallback carrying the descriptive message. This Callback path adds no information the paired ErrorCallback doesn't already carry (just "Error" + operation ID). LogType.Log — already used three lines above for the success case — is the honest level. This also removes the only dependency on (ANALYTICS, Warning) in the DebugLog matrix, shrinking Finding 1's blast radius.

See the existing inline suggestion at line 278.

Finding 3 — [P2] Classifier string is owned by an external crate — name the source

Location: RustSegmentAnalyticsService.cs:249-258

The literal "Error executing send loop (will retry)" is emitted by AnalyticsEventSendDaemon in the segment crate pinned in .native/Cargo.toml. Nothing couples this C# string to that rev. It fails safe (back to error level), so not blocking — but the comment should name the source so whoever bumps the rev has a breadcrumb.

See the existing inline suggestion at line 258.

Finding 4 — [P2] Null-forgiving operators in test file lack justifying comment

Location: RustSegmentErrorReportingShould.cs (throughout)

The test file contains ~15 ! operators on reflection calls (e.g. SERVICE_TYPE.GetField("RETRY_REPORT", PRIVATE_STATIC)!). Per CLAUDE.md, null-forgiving operators require a justifying comment. A single top-of-class comment such as:

// Reflection lookups assert non-null — null means the production API changed and the test must be updated.

would satisfy the convention for all of them.


STEP 6 — Complexity assessment

SIMPLE. Touches 3 files with meaningful changes (1 service + 2 asset configs) plus a new test file and meta. Does not modify ECS systems, components, queries, async patterns, networking, plugin registration, or other complex subsystems.


STEP 7 — QA assessment

QA_REQUIRED: YES. Modifies runtime code (RustSegmentAnalyticsService.cs) that ships in the build and changes what severity reaches Sentry in production. Although no user-visible UI/rendering/gameplay change, the diagnostics routing change warrants validation that the correct errors still surface in Sentry.


STEP 8 — Non-blocking warnings

None. Main scene not modified.


Security review

No security issues found. No secrets exposed, no auth changes, no injection risks. The native IntPtr marshaling pattern is unchanged. The static data race on ONCE_PATTERN_ALREADY_CAUGHT (bare bool read/written from native callbacks without synchronization) is eliminated by the replacement RETRY_REPORT which is immutable after construction.


CI status

  • ✅ EditMode tests: PASS (7/7)
  • ✅ PlayMode tests: PASS
  • ✅ Lint: PASS
  • ⏳ Unity Cloud Build (windows64, macOS): In progress / queued
  • ❌ Dependency Security Review: FAIL (encountered an error — unrelated to code; the new-dependency label appears to be a mislabel)

Summary

# Severity Finding Status
1 P1 Category-wide matrix flip un-mutes 18 unrelated, un-debounced SentryTransactionManager warning sites — can evict Sentry breadcrumbs ❌ Blocking
2 P2 Callback warning is strictly redundant with paired ErrorCallbackLog is the honest level Suggestion
3 P2 Classifier string is owned by external segment crate — comment should name the source Suggestion
4 P2 Test file ! operators lack justifying comment per CLAUDE.md Suggestion

The analysis and error classification in this PR are excellent — the root-cause diagnosis is correct, the lossless/lossy distinction is well-reasoned, and the test coverage is thorough. The sole blocker is the unscoped matrix flip (Finding 1), which has an unintended blast radius on 18 unrelated warning sites.


REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Changes confined to Segment transport error classification and reporting-settings asset matrices
QA_REQUIRED: YES


Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub

Comment on lines +49 to +50
- Category: ANALYTICS
Severity: 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Category-wide matrix flip un-mutes 18 unrelated, un-debounced warning sites.

Enabling (ANALYTICS, Warning) in the production Sentry + DebugLog matrices also un-mutes 18 ReportHub.LogWarning(new ReportData(ReportCategory.ANALYTICS), …) calls in SentryTransactionManager.cs, none of which carry a debouncer.

Specifically, SentryWebRequestHandler.OnRequestStarted early-returns for file:// URLs without registering a transaction, but OnRequestFinishedInstance.StartSpan() and OnProcessDataFinishedInstance.EndCurrentSpan() + Instance.EndTransaction() still execute, producing 3 ANALYTICS warnings per such request that now reach both the player log and Sentry breadcrumbs.

Sentry's breadcrumb ring is bounded (default 100/scope), so an un-debounced ANALYTICS warning stream can evict the breadcrumbs that make real crash reports diagnosable — inverting the PR's goal.

Suggested fix: Introduce a dedicated ReportCategory constant (e.g. ANALYTICS_TRANSPORT) for the Segment FFI reports and enable Warning only for that cell in the matrices. This keeps the entire Segment downgrade benefit while leaving the 18 SentryTransactionManager sites muted.

Suggested change
- Category: ANALYTICS
Severity: 2
- Category: ANALYTICS
Severity: 4

(Revert this entry to remove the category-wide Warning; re-add it under the new dedicated category instead.)

Comment on lines +275 to +278
// Native report_error always pairs a failed operation with an ErrorCallback delivery
// carrying the descriptive message — severity classification lives there.
if (response is not NativeMethods.Response.Success)
ReportHub.LogException(new Exception($"Segment operation {operationId} {type} failed with: {response}"), ReportCategory.ANALYTICS);
ReportHub.LogWarning(ReportCategory.ANALYTICS, $"Segment operation {operationId} {type} failed with: {response}");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Redundant warning — Log is the honest severity for a strictly-duplicate report.

The native report_error always pairs a failed operation with an ErrorCallback delivery carrying the descriptive message. This Callback path adds no new information (just "Error" + operation ID). Since it's strictly redundant, LogType.Log — already used three lines above for the success case — is the honest level. This also removes the only dependency on (ANALYTICS, Warning) in the DebugLog matrix.

Suggested change
// Native report_error always pairs a failed operation with an ErrorCallback delivery
// carrying the descriptive message — severity classification lives there.
if (response is not NativeMethods.Response.Success)
ReportHub.LogException(new Exception($"Segment operation {operationId} {type} failed with: {response}"), ReportCategory.ANALYTICS);
ReportHub.LogWarning(ReportCategory.ANALYTICS, $"Segment operation {operationId} {type} failed with: {response}");
if (response is not NativeMethods.Response.Success)
ReportHub.Log(ReportCategory.ANALYTICS, $"Segment operation {operationId} {type} failed with: {response}");

Comment on lines 249 to +258
}

// The native send daemon retries a "(will retry)" send-loop failure without consuming the
// spooled item, so no events are lost and it must not bill the Sentry error budget.
// Every other native error is a potential loss signal and keeps exception-level reporting:
// instant_track_and_flush drops the event on send failure (bare "Network error", never
// spooled), a failed flush drops the already-extracted batch ("Cannot flush: ..."), and
// the "(will drop)" send-loop branch consumes the item.
private static bool IsRetriedSendLoopError(string message) =>
message.Contains("Error executing send loop (will retry)");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Classifier string is owned by an external crate — name the source for future maintainers.

The literal "Error executing send loop (will retry)" is emitted by AnalyticsEventSendDaemon in the segment crate (.native/Cargo.toml, pinned rev). A future crate bump can silently change the wording and the classifier stops matching. It fails safe (back to error-level), so not blocking — but the comment should name the source.

Suggested change
}
// The native send daemon retries a "(will retry)" send-loop failure without consuming the
// spooled item, so no events are lost and it must not bill the Sentry error budget.
// Every other native error is a potential loss signal and keeps exception-level reporting:
// instant_track_and_flush drops the event on send failure (bare "Network error", never
// spooled), a failed flush drops the already-extracted batch ("Cannot flush: ..."), and
// the "(will drop)" send-loop branch consumes the item.
private static bool IsRetriedSendLoopError(string message) =>
message.Contains("Error executing send loop (will retry)");
// The native send daemon retries a "(will retry)" send-loop failure without consuming the
// spooled item, so no events are lost and it must not bill the Sentry error budget.
// Every other native error is a potential loss signal and keeps exception-level reporting:
// instant_track_and_flush drops the event on send failure (bare "Network error", never
// spooled), a failed flush drops the already-extracted batch ("Cannot flush: ..."), and
// the "(will drop)" send-loop branch consumes the item.
// The matched text is emitted by AnalyticsEventSendDaemon in the `segment` crate pinned in
// .native/Cargo.toml; re-verify this literal when that rev is bumped.
private static bool IsRetriedSendLoopError(string message) =>
message.Contains("Error executing send loop (will retry)");

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

🚦 CI Status

Build

Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below.

Name Link
Commit a507860
Logs https://github.com/decentraland/unity-explorer/actions/runs/32252175981
Download Windows https://github.com/decentraland/unity-explorer/suites/87428437224/artifacts/
Download Windows S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/bugsweep/segment-network-errors/pr-25322-a507860/Decentraland_windows64.zip
Download Mac https://github.com/decentraland/unity-explorer/suites/87428437224/artifacts/9368037560
Download Mac S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/bugsweep/segment-network-errors/pr-25322-a507860/Decentraland_macos.zip
Built on 2026-08-19T13:52:58Z

Lint

Warnings not reduced: 12724 => 13123 — remove at least 400 warnings to merge.

Warnings/errors in files changed by this PR (1)
Assets/DCL/Tests/Editor/RustSegmentErrorReportingShould.cs:135  RedundantExplicitArrayCreation  Redundant explicit array type specification

Tests

All Unity tests passed ✅

TESTS SUITE Result Passed Failed Skipped
EditMode ✅ Passed 25063 0 13
PlayMode ✅ Passed 236 0 37

@decentraland-bot

Copy link
Copy Markdown
Contributor

PR #9808, run #32260733528

Builds: Windows change, Windows baseline, macOS change, macOS baseline

How to read this table
  • Each build is measured 3 times. The values are the median, and (min–max) is the lowest and highest of those runs — a wide range means the metric is noisy and small differences are not trustworthy.
  • Δ is Change minus Baseline (a negative Δ means Change is faster).
  • 🟢 faster / 🔴 slower — a real difference: larger than both 3% and the run-to-run range.
  • ⚪ within noise — the difference is smaller than how much the build varies between its own runs, so it cannot be told apart from random variation. Treat it as no change.
  • Exceptions per run — the average number of exceptions in a run's log; more than the baseline is flagged 🔴 even when frame times look fine. The Exception breakdown under each table groups them by the explorer's report category and exception type (as totals across the runs).
  • A run that logged unusually many exceptions (at least 10 and 5× the median of its build's runs — e.g. a service was down during it) is excluded from all numbers and called out under the table.

Intel Core i5

Metric Baseline Change Δ Result
Samples 2313 (×3) 2377 (×3)
CPU average 38.6 ms (33.4–38.8) 37.6 ms (36.1–37.7) -1.0 ms ⚪ within noise
CPU 1% worst 322.6 ms (57.0–343.5) 295.1 ms (286.0–301.2) -27.5 ms ⚪ within noise
CPU 0.1% worst 344.1 ms (341.1–360.3) 311.5 ms (303.8–318.5) -32.7 ms 🟢 9% faster
GPU average 9.5 ms (9.2–9.6) 9.5 ms (9.3–9.5) -0.1 ms ⚪ within noise
GPU 1% worst 35.6 ms (23.5–37.7) 32.0 ms (29.5–32.1) -3.5 ms ⚪ within noise
GPU 0.1% worst 44.4 ms (39.8–45.0) 38.8 ms (36.3–39.3) -5.6 ms 🟢 13% faster
Exceptions per run 66 66 0 ⚪ none new
Exception breakdown
Exception Baseline (3 runs) Change (3 runs)
[UI] DllNotFoundException 192 192
[ENGINE] NullReferenceException 3 3
[ENGINE] ObjectDisposedException 3 3

Apple M1

Metric Baseline Change Δ Result
Samples 4105 (×3) 4071 (×3)
CPU average 21.8 ms (21.8–22.9) 22.0 ms (21.8–22.7) 0.2 ms ⚪ within noise
CPU 1% worst 215.9 ms (215.7–217.7) 233.4 ms (213.6–234.1) 17.5 ms ⚪ within noise
CPU 0.1% worst 226.3 ms (222.9–228.8) 238.2 ms (236.5–246.0) 12.0 ms 🔴 5% slower
GPU average 2.5 ms (2.0–3.2) 2.5 ms (1.9–2.6) 0.0 ms ⚪ within noise
GPU 1% worst 34.3 ms (34.2–36.2) 34.7 ms (33.9–36.3) 0.4 ms ⚪ within noise
GPU 0.1% worst 36.3 ms (34.8–37.5) 37.1 ms (34.6–37.4) 0.8 ms ⚪ within noise
Exceptions per run 0 0 0 ⚪ none new

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants