[common] Wire onboarding recorder into the :auth WebView host (brokered), AB#3708195, Fixes AB#3708195

[wzhipan]

Summary

Implements the common (AndroidCommon) half of AB#3708195 — wiring the onboarding telemetry recorder into the interactive :auth WebView host so the brokered flow records WebView-observed onboarding steps and the Auth UX log_telemetry error code into the blob the broker finalizes and returns.

Stacked on #3201 (AB#3688632). Base = copilot/pbi-3688632-onboarding-blob-append. The paired broker PR (AccountChooser registration + registry cleanup + broker-side blob logging) lands separately.

Problem

The onboarding recorder is built per-request by the broker's AccountChooser, but the interactive WebView runs in a separate AuthorizationActivity created by the OS from an Intent — a live recorder can't ride that Intent. As a result AzureActiveDirectoryWebViewClient's recorder hooks (recordOnboardingStep, recordLastLoadedDomain, and the log_telemetry recordAuthUxServerErrorCode sink from #3201) were dead in production: nothing ever called setOnboardingTelemetryRecorder(...).

Changes

• OnboardingRecorderRegistry (new) — a process-static handoff keyed by request correlationId. The owner (AccountChooser) and the WebView host both run in the broker :auth process, so an in-process registry bridges them. Stores the concrete OnboardingTelemetryRecorder because the WebView hooks (e.g. setLastLoadedDomain) use methods not on the common4j interface.
  • Rejects the unset correlation-id sentinel. DiagnosticContext seeds every thread's request context with UNSET, and the authorization Intent extra is populated by reading that map directly rather than through getThreadCorrelationId() — so the raw sentinel can reach the registry. It is shared by definition, so accepting it would let two unrelated requests resolve to the same recorder and merge one flow's blocking errors into the other's uploaded blob. All three accessors reject it: the feature goes inert (and register warns) instead of silently mis-attributing telemetry.
  • Bounded (cap 16, access-ordered LinkedHashMap). Entries must be unregistered on terminal outcome, and since this is process-static state in a process that lives as long as the device is up, a missed unregister would leak the recorder and its collected steps/blocking errors permanently (not an Activity — OnboardingTelemetryRecorder holds only the application context). On overflow it evicts the least-recently-used entry and logs a warning, bounding the leak and surfacing the underlying bug. The cap is far above real concurrency (one interactive request at a time), so it should never evict in legitimate use; LRU is not load-bearing today (the host resolves once and holds the reference) but is the safer default if a future caller re-resolves.
• AuthorizationFragment — captures the correlation id in extractState and round-trips it through onSaveInstanceState. The read was already a base-class responsibility, so the save belongs next to it: previously the key was never saved at all, so a recreated fragment read null back and blanked its diagnostic context, losing the join key on every subsequent log line for the request. Fixing it in the parent covers all three authorization fragments (WebView, Browser, CurrentTaskBrowser) — all already call super — and removes any chance of the save and read drifting apart.
• WebViewAuthorizationFragment — resolves the recorder in onCreateView from the inherited mCorrelationId and calls setOnboardingTelemetryRecorder(...) right before initializeAuthUxJavaScriptApi(...). No-op when the request seeded no recorder. Reading from the state bundle rather than activity.getIntent() also matters because this fragment supports being hosted by an activity it does not own, whose Intent would not carry the extra.
• DiagnosticContext (common4j) — UNSET_CORRELATION_ID is now public, and the internal duplicate "UNSET" literal in getThreadCorrelationId() references it. Callers that use the correlation id as a key rather than for logging must reject the sentinel explicitly, and a copied literal in the registry would silently stop matching if this ever changed — reintroducing cross-request contamination with no signal.
• Logger (common4j) — notes that its own UNSET literal is a display placeholder, deliberately not the same thing: it also stands in for a missing thread id and is never used as a key, so it must not be collapsed into the new constant.

Tests — OnboardingRecorderRegistryTest (15): correlation-id keying (one request must never see another's recorder), the UNSET sentinel — including the concrete two-request cross-wiring it prevents — removal/lifecycle, absent-key behaviour (the no-seed MSAL path gets null rather than a throw), null/empty inputs, idempotent unregister, and the two leak-bound properties.

AuthorizationFragmentCorrelationIdTest (6, new): the fragment half of the handoff — extractState captures the id, the captured id resolves the registered recorder, it survives save/restore, and the no-recorder / missing-id / sentinel paths resolve to null rather than throwing. Without these, a regression in the correlation-id plumbing would leave every hook inert while the registry tests still passed.

All four behaviours are revert-tested: removing the sentinel guard fails with the sentinel must never become a key expected:<0> but was:<1>, disabling the cap fails with expected:<16> but was:<100>, switching to insertion order evicts a live in-use recorder, and dropping the onSaveInstanceState round-trip fails with the correlation id must round-trip through the saved bundle expected:<…> but was:<null>.

⚠️ This PR alone changes no behaviour. register / unregister have no caller in this repo — both live in the paired broker PR — so get() always returns null and the WebView onboarding hooks stay inert, exactly as before. The registry's "entries MUST be unregistered" contract is likewise unenforceable from here, which is why the bounded-LRU cap is the only in-repo defence against a leak and should not be removed as over-engineering.

Merge order: #3197 → #3201 → #3204 → broker PR. All four must land together; publish common, then bump the broker dependency.

The paired broker PR registers the recorder in the registry (keyed by the same correlationId) and unregisters it on terminal outcome in a finally. Load-bearing assumption to confirm on that side: the value AccountChooser registers under must be identical to DiagnosticContext.INSTANCE.requestContext[CORRELATION_ID] as read on the thread that builds the authorization Intent. The on-device E2E below shows they matched; the sentinel guard now converts a mismatch from silent cross-contamination into an inert feature plus a logged warning, but the two sides still have to agree for the handoff to work at all.

Verified end-to-end on device

Built brokerHost against these changes and drove a brokered MAM/Conditional-Access flow (MSAL test app → Broker Host, MAMCA account) on an emulator. Logcat confirmed the full chain, and the finalized blob returned on the failure path:

{"schema_version":"1.0.0","session_correlation_id":"…","onboarding_mode":"brokered",
 "steps_list":[{"step_id":"AuthenticationStarted",…},{"step_id":"CABlockReceived",…},
   {"step_id":"InterruptFlowStarted",…},{"step_id":"UserCanceled",…}],
 "blocking_errors":["530003"],"last_blocking_error":"530003",
 "last_loaded_domain":"login.microsoftonline.com","last_completed_step":"UserCanceled"}

• Recorder registered by AccountChooser and resolved+wired by the fragment — correlationId matched on both sides.
• last_loaded_domain populated (a previously-dead WebViewClient hook, now active).
• 530003 present via the log_telemetry bridge → sink (a debug-only bridge simulation stood in for the not-yet-built server page JS, AB#3696811).

Notes

• Bridge exposure remains broker-:auth-only (ProcessUtil.isRunningOnAuthService + ENABLE_JS_API_FOR_AUTHUX); non-brokered/iOS is AB#3688630.
• The debug-only scaffolding used for the on-device E2E (synthetic seed when a client sends none, and a synthetic log_telemetry post) is kept on a local build branch, not in this PR.

---

Draft PR — opened for early review. AB#3708195.

[github-actions]

✅ Work item link check complete. Description contains link AB#3708195 to an Azure Boards work item.

[wzhipan]

Follow-up noted from the #3201 round-2 review: OnboardingRecorderRegistry has no unit test.

Raising it here rather than on #3201 because the class is added by this PR, not that one.

grep -r OnboardingRecorderRegistry common/src/test returns nothing, so nothing currently pins:

• the correlation-id keying — put under one id, get under another must not cross-wire two flows;
• removal / lifecycle — the entry must not outlive the request and leak the recorder. This is the one that matters most: the registry is process-static, so a missed removal keeps a recorder (and the Context it holds) alive for the life of the process, and a later request with a recycled correlation id could attach to a stale recorder;
• absent-key behaviour — a get for an unknown id returns null rather than throwing, which the WebView host relies on when no seed was supplied (the MSAL-client case, where no onboarding telemetry is expected at all).

This is not a regression and does not block the stack — flagging it because "process-static + holds a recorder reference" is the shape where an untested lifecycle bites later, and the registry is the one piece of this feature with no direct coverage. I'd like to add these before the stack merges.

[wzhipan]

Fixed in 8d0a325.

All three cases I listed are now pinned by OnboardingRecorderRegistryTest (12 tests): correlation-id keying, removal/lifecycle, and absent-key behaviour, plus null/empty inputs and idempotent unregister.

The self-review also turned up two things beyond the missing tests:

The map was unbounded. The "entries MUST be unregistered" contract was enforced only by convention, in a process that lives as long as the device is up — so any terminal path that missed unregister leaked a recorder (and its Context) permanently. It is now a bounded access-ordered LinkedHashMap (cap 16) that evicts and logs a warning when full. That turns an unbounded leak into a bounded one and surfaces the underlying bug rather than hiding it. Eviction is least-recently-used, not oldest-registered, so a long-running request still fetching its recorder is not evicted by newer leaked entries. The cap is far above real concurrency (the broker drives one interactive request at a time), so it should never evict in legitimate use.

Logging dropped the correlationId. Both the registry and WebViewAuthorizationFragment passed correlationId inside the log message instead of using Logger's correlationID parameter, so it was not filterable in the log pipeline and diverged from the rest of this feature.

Both behaviour changes are revert-tested rather than assumed:

Injected fault	Observed failure
cap disabled	a missed unregister must be a bounded leak, not an unbounded one expected:<16> but was:<100>
insertion order instead of access order	testEviction_KeepsTheEntryStillBeingUsed — live recorder evicted to null

178 tests green in this area (WebViewClient 93, bridge 41, recorder 18, registry 12, payload 10, correlation store 4). The full :common suite reports 138 failures, all pre-existing NativeAuth MockApi environment failures (MalformedURLException: no protocol: 1234/...) — confirmed identical with these changes stashed.

[Copilot]

Pull request overview

Wires brokered onboarding telemetry into the :auth WebView through a correlation-ID registry.

Changes:

• Adds a bounded, thread-safe recorder registry.
• Restores and uses correlation IDs to attach recorders.
• Exposes the unset-correlation sentinel and adds registry tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
DiagnosticContext.java	Exposes the unset sentinel.
OnboardingRecorderRegistry.kt	Implements recorder handoff and eviction.
OnboardingRecorderRegistryTest.kt	Tests registry behavior and bounds.
WebViewAuthorizationFragment.java	Resolves and attaches the recorder.
changelog.txt	Documents the changes.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

[Praveen Kumar (Prvnkmr337)]

Nit (non-blocking): extractState can pass a null correlation id into setDiagnosticContextForNewThread, which has no null guard.

In extractState:

mCorrelationId = state.getString(DiagnosticContext.CORRELATION_ID); // may be null
setDiagnosticContextForNewThread(mCorrelationId);

setDiagnosticContextForNewThread then does rc.put(CORRELATION_ID, correlationId) with no null check.

I checked the full path and this is safe today: RequestContext extends HashMap<String, String>, so put(key, null) is legal, and the only reader — DiagnosticContext.getThreadCorrelationId() — explicitly guards correlationId == null and substitutes a random UUID. So a null flows through harmlessly, and this matches the pre-existing behavior.

The only reason to tighten it is defense-in-depth: the null-tolerance is load-bearing on RequestContext staying a HashMap. If it were ever swapped to a ConcurrentHashMap/Hashtable, put(key, null) would throw. If you want to harden against that, the cleanest spot is guarding the write in onSaveInstanceState so a null is never persisted in the first place:

if (mCorrelationId != null) {
    outState.putString(DiagnosticContext.CORRELATION_ID, mCorrelationId);
}

Not blocking — purely a robustness nit.

[wzhipan]

Taken, in 518a4b2fe — and thanks for doing the full trace rather than just flagging it.

I verified your analysis rather than assuming it: RequestContext extends HashMap, so put(key, null) is legal, and getThreadCorrelationId() is the only reader and guards null with a random UUID. Safe today, exactly as you said.

Took it anyway for the reason you gave — the safety is load-bearing on the map type, and a future swap to ConcurrentHashMap/Hashtable would turn it into a throw. Guarded the write, as you suggested: an absent key is already handled identically to "never saved", so nothing downstream has to tolerate a null at all.

Test asserts the key is absent and that the recreation path still reads back null without throwing; revert-tested. No changelog entry — the existing round-trip entry already covers this method.

[Shahzaib (shahzaibj)]

nit: can we put @GuardedBy("recorders") on evictedKey? It's a plain var that's only safe because removeEldestEntry writes it and register reads/clears it under the same lock. The comment already spells that out, the annotation just makes it harder for a future edit to touch it off-lock without noticing.

[wzhipan]

Done, in 01a2b2d25.

Worth flagging that the comment it points at was wrong, in a way that mattered for exactly this annotation. It said evictedKey is "read and cleared by register() immediately after the lock is released" — but the read-and-clear is evictedKey.also { evictedKey = null } inside the synchronized(recorders) block; only the resulting warning is logged off-lock. So the prose described a lock discipline that would have made @GuardedBy("recorders") a lie, and a future reader trusting it would have concluded the annotation was wrong and removed it rather than the access. Reworded so the two now agree.

Both accesses verified under the monitor: removeEldestEntry runs inside put(), which the caller already holds the lock for, and register()'s read/clear is in the same critical section.

Used androidx.annotation.GuardedBy — same artifact as the VisibleForTesting already imported here, so no new dependency, and it's the variant Android Lint's GuardedBy check understands.

[wzhipan]

Synced the stack with dev (10 commits). #3201 merged clean; this PR had two real conflicts worth a note, since dev landed the MAM-CA install-referrer work in the same two fragments:

• AuthorizationFragment — dev added mMamCaInstallReferrerEnabled to the same extractState / onSaveInstanceState pair this PR added the correlation-id round-trip to. Kept both; they're independent pieces of restored state.
• WebViewAuthorizationFragment — dev extracted the client construction out of onCreateView into a new createAADWebViewClient(). Git left this PR's recorder wiring stranded at the tail of that new method; moved it back into onCreateView, still between setUpWebView and initializeAuthUxJavaScriptApi so the client holds the recorder before any page can reach the bridge. Made that ordering constraint explicit in the comment rather than leaving it implied by position. Also re-declared the methodTag that moved out with the refactor.

Verification that the resolution kept dev's side intact: its own new WebViewAuthorizationFragmentInstallReferrerTest and AuthorizationFragmentInstallReferrerTest pass unchanged. 274 tests green in-area, 0 failures; the PR's own diff vs its base is the same 8 files as before.

Also took Shahzaib (@shahzaibj)'s @GuardedBy nit — replied on that thread.