Hold XDM context for a session instead of passing it per call - #176
Conversation
Adds Concierge.updateXDMContext(Map) so a host app can supply XDM fields once and have them travel with every conversation turn in the session. Ports the iOS implementation of the same feature. Merge semantics follow RFC 7396: nested maps merge recursively, a null value removes a key, and lists replace wholesale. identityMap is rejected -- Edge owns that namespace. The held context is snapshotted once per turn, before the asynchronous token-provider wait, so the querySubmitted tracking event and the request that goes over the wire always carry the same fields. Handoff turns merge the held context with their own fields, with call-specific values winning on collision. xdmFields are dropped before forwarding to Edge, alongside query and notes -- app-supplied XDM is a PII risk and does not belong in the customer's production pipeline. Held context is cleared when the session rolls over; the app is responsible for re-establishing it.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@cdhoffmann & @rymorale - ready for your initial review |
rymorale
left a comment
There was a problem hiding this comment.
Overall the changes look good but there is one blocker: nulls inside lists pass updateXDMContext validation but make the snapshot throw, which crashes the app from handleSendMessage (see L343). The session-rollover comment on L105 is more of a design question and may apply to iOS too. The rest are minor.
| */ | ||
| internal interface ConversationService { | ||
| fun chat(message: String): Flow<ParsedConversationMessage> | ||
| fun chat(message: String, xdmFields: Map<String, Any>): Flow<ParsedConversationMessage> = chat(message) |
There was a problem hiding this comment.
nit: this default silently drops xdmFields for any ConversationService implementation that doesn't override it (including test fakes), so a missing override fails quietly rather than at compile time. Consider making it abstract, or collapsing to a single chat(message: String, xdmFields: Map<String, Any> = emptyMap()). That would also remove the if (xdmFields.isEmpty()) branch in ConciergeChatViewModel.processChatRequest.
There was a problem hiding this comment.
Agreed, I'll collapse to a single chat(message: String, xdmFields: Map<String, Any> = emptyMap()), which also removes the if (xdmFields.isEmpty()) branch in processChatRequest.
There was a problem hiding this comment.
Updated ConversationService to use a single chat(message, xdmFields) method, so implementations can’t silently drop XDM fields. The debug fakes now implement the updated signature too.
| } | ||
| copied | ||
| } | ||
| is List<*> -> value.map { copyAndValidateXdmValue(it, allowNull) } |
There was a problem hiding this comment.
Blocking: a null inside a list passes validation here and crashes the app on the next send.
updateXDMContext validates with allowNull = true, and that flag carries into lists. mergeXdmPatch never recurses into lists, so mapOf("tags" to listOf("a", null)) or listOf(mapOf("x" to null)) is stored as-is.
On the next turn, snapshotXDMContext → copyXdmObject re-validates with allowNull = false and throws IllegalArgumentException:
- Typed chat: the snapshot runs in
handleSendMessage, outside any try/catch, so the exception escapes a UI event handler and crashes the host app. - Data handoff: the exception is caught, but every later handoff fails with
DELIVERY_FAILEDuntil the app removes the key.
toJsonValue() in the service client rejects nulls in lists anyway, so the simplest fix is to reject them at the API boundary, e.g. is List<*> -> value.map { copyAndValidateXdmValue(it, allowNull = false) } (maps nested in lists also need allowNull = false). That makes it fail fast in updateXDMContext as the KDoc promises. Please add tests for listOf(null) and listOf(mapOf("x" to null)).
(Strict RFC 7396 treats null inside an array as a literal value, but nothing downstream can serialize it today.)
There was a problem hiding this comment.
Good catch, updateXDMContext validates with allowNull = true, that flag carries into the is List branch, and mergeXdmPatch never recurses into lists, so the null survives until copyXdmObject re-validates with allowNull = false and throws. Agreed it's a crash and agreed it needs to fail at the API boundary.
One thing before I implement, because it affects which fix is right, iOS accepts this today. ConciergeXDMContextStore.validate() uses JSONSerialization.isValidJSONObject(fields), which permits NSNull inside arrays, and iOS snapshot() doesn't re-validate, it just returns held. So ["a", NSNull()] round-trips and serializes as ["a", null] on iOS.
That means rejecting on Android fixes the crash but introduces a difference between iOS and Android. Android would reject input iOS sends successfully.
Two options:
(a) Reject on both, your suggested fix here, plus a matching iOS change.
(b) Support on both, Android's toJsonValue() emits JSONObject.NULL for nulls in lists instead of throwing, and copyXdmObject tolerates them. No iOS change needed.
I lean (b) because it matches RFC 7396 (null is a literal value inside an array, only a deletion sentinel in an object), matches current iOS behavior, and JSONObject.NULL is the standard org.json representation. (a) is the right call if the BC service rejects null array elements, do you know whether it does?
Either way I'll add the listOf(null) and listOf(mapOf("x" to null)) tests, and I'll wrap the handleSendMessage snapshot in a try/catch regardless, app-supplied data shouldn't be able to crash a UI click handler even once validation is tightened.
There was a problem hiding this comment.
Implemented option B. Nulls in arrays, including maps nested inside arrays are preserved through validation and snapshots, and serialize as JSON null. Added tests for listOf(null) and listOf(mapOf("x" to null)), including request serialization. Also verified XDM fields remain available in the Assurance hub event.
| * observed isn't replacing anything, so it must not clear context an app already set before | ||
| * its first request, which is the API's primary supported use case. | ||
| */ | ||
| fun snapshotXDMContext(sessionId: String): Map<String, Any> = synchronized(xdmContextLock) { |
There was a problem hiding this comment.
Design concern: held context gets wiped every ~30 minutes, even mid-conversation, and apps have no way to re-establish it reliably.
ConciergeSessionManager.getSessionId() only writes KEY_SESSION_TIMESTAMP when it creates a session; nothing refreshes it on activity. So the session ID rolls over 30 min after creation regardless of use (the class KDoc says "30 minutes of inactivity", but that isn't what the code does). That's pre-existing, but with this PR each rollover now silently drops held context in the middle of an active chat.
Two things make it hard for apps to handle "the app is responsible for re-establishing it":
- Rollover is only detected lazily, here. An
updateXDMContextcall made after expiry but before the next turn is also wiped (thenew session id clears context updated after the previous turntest confirms this is intentional, to mirror iOS). - There's no callback or signal for rollover, so the app can't know when to re-send.
Options to consider:
- Refresh the session timestamp on each request so the timeout really is based on inactivity.
- Have
updateXDMContextresolve the current session ID and bind the update to it, so a post-expiry update is kept for the new session instead of being discarded.
If iOS parity is a hard requirement, the same issue likely exists there as well.
There was a problem hiding this comment.
Confirmed on the Android side: KEY_SESSION_TIMESTAMP is written only in the creation branch (ConciergeSessionManager L72) and on clear(). Nothing refreshes it on use, so the TTL runs from creation, not inactivity, the KDoc at L23 ("30 minutes of inactivity") describes intended behavior rather than actual.
On iOS parity though, iOS already does this correctly, so this is an Android-only gap rather than a shared design question:
// SessionManager.swift:70
func refreshSessionActivity() {
dataStore.setObject(key: ...LAST_ACTIVITY, value: Date())
}called on every successful network request (ConciergeChatService.swift:84 and :119). iOS's getOrCreateSessionId() then compares against LAST_ACTIVITY, so its timeout genuinely is inactivity-based.
So your first suggested option is what we need for parity, and it makes the Android doc true. I can add the refresh call to bring Android in line.
Question on scope: this is pre-existing, and this PR only makes it more visible. Do you want the session-refresh fix in this PR, or as a separate one?
There was a problem hiding this comment.
let's get it resolved in this pr as we are planning to release the latest concierge SDK version soon
There was a problem hiding this comment.
Added a session activity refresh when conversation, data handoff, and feedback requests start, so the 30 minute timeout measures inactivity. Added a test covering activity within the timeout and rollover after inactivity. This follows iOS behavior, which refreshes when a request starts.
| responseStartedDispatched = false | ||
| request.handoff.localMessage?.let(::appendAgentMessage) | ||
| val heldXdmFields = stateRepository.snapshotXDMContext(sessionManager.getSessionId()) | ||
| val mergedXdmFields = heldXdmFields + request.handoff.xdmFields |
There was a problem hiding this comment.
This + is a shallow, top-level merge, so it combines fields differently from the held context itself. With held loyalty = {tier: "gold"} and a handoff carrying loyalty = {points: 5}, the request ends up with loyalty = {points: 5} and tier is gone. Held context uses RFC 7396's recursive merge, so callers will probably expect handoff fields to be deep-merged on top.
Suggest running the handoff fields through the same mergeXdmPatch logic, or else documenting clearly that handoff fields replace held fields key by key at the top level.
Minor, related: the handoff snapshot is taken here, when the queue processes the request, while typed chat takes it at submit time in handleSendMessage. So an updateXDMContext made after Concierge.sendDataHandoff(...) returns can still end up in that handoff. Capturing it when the handoff is enqueued would make the two paths consistent.
There was a problem hiding this comment.
Agreed on both points, will fix.
There was a problem hiding this comment.
Data-handoff fields now deep-merge over held XDM context using the same merge semantics as context updates. The context is captured when the handoff is enqueued, so later updates don’t change the queued request. Added tests for both behaviors.
| private fun handleSendMessage(messageText: String) { | ||
| if (messageText.isBlank()) return | ||
|
|
||
| val contextSnapshot = stateRepository.snapshotXDMContext(sessionManager.getSessionId()) |
There was a problem hiding this comment.
Two small things about this snapshot:
- This is where the null-in-list
IllegalArgumentException(see the comment onConciergeStateRepository.ktL343) escapes, sincehandleSendMessageruns from a UI event without a try/catch. Fixing the validation fixes it, but a crash in a UI click handler caused by app-supplied data is worth guarding against either way. sessionManager.getSessionId()is called here and then again when the service client builds the endpoint URL. If the 30-min boundary falls between the two calls, context captured under session A goes out with session B's ID. The window is small, but passing a single resolved session ID through (or letting the snapshot return the ID it used) would close it.
There was a problem hiding this comment.
Agreed, I'll add the try/catch here independent of the validation fix, for the reason you give an app supplied data reaching a UI event handler shouldn't be able to take down the host app. On failure I'll log and send an empty context rather than propagating.
Good catch on the double getSessionId(). I'll thread a single resolved session ID through so the snapshot and the endpoint URL can't straddle the 30-min boundary. Worth noting this window gets much narrower once the session-refresh fix from the L105 thread lands, but it's still worth closing properly rather than relying on that.
There was a problem hiding this comment.
Added a guard around the chat context snapshot so invalid app supplied context won’t escape the UI handler; the failure is logged and the turn proceeds without held context. The resolved session ID is also carried with chat and handoff requests, keeping each request’s context snapshot and endpoint session ID aligned.
| } | ||
|
|
||
| dispatchTrackingEvent(ConciergeTrackingEvent.QuerySubmitted(messageText)) | ||
| dispatchTrackingEvent(ConciergeTrackingEvent.QuerySubmitted(messageText, contextSnapshot)) |
There was a problem hiding this comment.
Minor: dispatchTrackingEvent logs the full event at debug level ("Dispatching tracking event: $event"), so app-supplied xdmFields now land in logcat next to query. query was already logged, but since this PR (rightly) treats the context as potential PII and strips it from the Edge payload, it may be worth redacting both from that log line too.
There was a problem hiding this comment.
Fair, happy to redact both query and xdmFields from that debug line. Inconsistent to treat the context as PII for the Edge payload and then print it to logcat.
One clarification I want on record since it's easy to conflate, the sanitization in ConciergeEventTracker applies only to the Edge request. The hub event built in ConciergeTrackingEvent.eventData still carries xdmFields, so it remains visible in Assurance, which is where you'd actually want it for validating this feature. Redacting the logcat line doesn't change that. @cdhoffmann can you confirm?
Assuming that's the intended split, PII kept out of the customer's production datastream and out of device logs, but visible in an opt-in Assurance debug session, I'll make the log change.
There was a problem hiding this comment.
i think that the XDM fields (and potential PII) being visible via Assurance but not present in device logs is the proper way to resolve this.
There was a problem hiding this comment.
Updated tracking event logging to include only the event name, not its payload. XDM fields remain in the hub event for Assurance, but aren’t printed in device logs.
| private val _welcomeConfig = MutableStateFlow(initializeWelcomeConfig()) | ||
| internal val welcomeConfig: StateFlow<WelcomeConfig> = _welcomeConfig.asStateFlow() | ||
|
|
||
| private var stateRepository = ConciergeStateRepository.instance |
There was a problem hiding this comment.
nit: stateRepository and sessionManager are private var with an initializer, and the primary constructor immediately overwrites them. Consider private val assigned only in the constructor (like chatService), so they can't be reassigned later and the default isn't built twice.
There was a problem hiding this comment.
Changed stateRepository and sessionManager to constructor-assigned private val properties so they’re initialized once and can’t be reassigned.
| the Edge Identity map. Context is held in memory for the active conversation session. Context set | ||
| before the first chat turn is retained; when the session ID changes, all held context is cleared and | ||
| the app is responsible for re-establishing it. | ||
| `ConciergeStateRepository.clear()` also clears the context. |
There was a problem hiding this comment.
ConciergeStateRepository is internal, so apps reading this guide can't call clear(), and nothing in production code calls it either. Suggest dropping this line, or replacing it with the public way to reset context: null-valued patch entries, per the Concierge.updateXDMContext KDoc, which also says "There is no separate reset API". As written, the guide and the KDoc contradict each other.
There was a problem hiding this comment.
Agreed, the guide and the KDoc contradict each other and clear() isn't callable by apps anyway. I'll replace that line with the null-valued patch approach the updateXDMContext KDoc describes, so there's one documented way to reset context.
There was a problem hiding this comment.
Removed the internal clear() call from the guide and documented resetting context through null-valued patches. The guide also now clarifies that null is allowed inside list values.
…equests start, and carry one resolved session ID through context snapshots and requests. Deep-merge data handoff fields over held context, and add coverage for serialization, session rollover, and request snapshots.
|
@rymorale made updates and added responses for each item. |
rymorale
left a comment
There was a problem hiding this comment.
Thanks for the updates. The latest updates still have a couple of medium to low issues to resolve.
| * observed isn't replacing anything, so it must not clear context an app already set before | ||
| * its first request, which is the API's primary supported use case. | ||
| */ | ||
| fun snapshotXDMContext(sessionId: String): Map<String, Any> = synchronized(xdmContextLock) { |
There was a problem hiding this comment.
[MEDIUM] Context set after a session expires is dropped.
When a session expires, the next send gets a new ID and this clears all held context, including anything the app just re-set. The app has no rollover signal, so it can't know when to re-apply. Binding updateXDMContext to the current session ID (option 2 from the earlier thread) would fix it, or we could document the limitation.
There was a problem hiding this comment.
Context updates now resolve and bind to the current session ID. If the previous session expired, stale context is cleared before applying the new patch, so freshly re-established context survives the next turn. Added regression coverage for actual inactivity rollover and verified that updating context does not refresh session activity.
| fun mergeXdmFields(base: Map<String, Any>, overlay: Map<String, Any>): Map<String, Any> = | ||
| mergeXdmPatch(base, overlay) | ||
|
|
||
| private fun copyAndValidateXdmValue(value: Any?, allowNull: Boolean): Any? = when (value) { |
There was a problem hiding this comment.
[MEDIUM] No depth or cycle limit.
A cyclic or deeply nested map throws StackOverflowError, not the documented IllegalArgumentException, and crashes the host app. sendDataHandoff already caps depth with MAX_XDM_FIELD_VALUE_DEPTH, so the same cap should apply here and after mergeXdmFields.
There was a problem hiding this comment.
Context updates and handoffs now share a depth-bounded validator, retaining the existing limit of 20. Cyclic and over-depth input produces an IllegalArgumentException rather than overflowing the stack. Merge inputs and the resulting fields are also validated. Added tests for cycles, the exact depth boundary, and rejection without changing held context.
| } | ||
| is String, is Boolean -> value | ||
| is Number -> { | ||
| require(value is Byte || value is Short || value is Int || value is Long || |
There was a problem hiding this comment.
[LOW] Accepts Byte/Short, but sendDataHandoff rejects them.
isJsonSafeValue only allows Int, Long, Double and Float. Sharing one validator would keep the two paths from drifting.
There was a problem hiding this comment.
Fixed by sharing validation between context updates and handoffs. Both paths now accept Byte and Short alongside the previously supported numeric types, while still rejecting non-finite and unsupported values. Added acceptance tests for both paths and verified Byte/Short serialization in the request body.
| private fun handleSendMessage(messageText: String) { | ||
| if (messageText.isBlank()) return | ||
|
|
||
| val sessionId = sessionManager.getSessionId() |
There was a problem hiding this comment.
[LOW] getSessionId() now runs on the UI thread.
It reads the datastore and can write a new session ID. Resolving it in the conversation processor and keeping only the context snapshot at submit time would avoid that.
There was a problem hiding this comment.
Chat and handoff submission handlers capture only the in-memory context snapshot; the queue processor resolves the session ID on Dispatchers.IO and passes that single ID through to the request. QUERY_SUBMITTED now fires after session resolution, before the request, so its context matches the outbound fields even if the captured session expired while queued. Added threading and snapshot-consistency tests for both request paths.
| // context cannot be snapshotted, send the turn without it rather than propagating. | ||
| val contextSnapshot = try { | ||
| stateRepository.snapshotXDMContext(sessionId) | ||
| } catch (e: IllegalArgumentException) { |
There was a problem hiding this comment.
[LOW] This catch can't be reached.
snapshotXDMContext only re-copies already-validated data, so it can't throw IllegalArgumentException, which matches the low patch coverage here. The handoff path (~L1034) has the same catch but fails with DELIVERY_FAILED instead, so I'd drop the try/catch.
There was a problem hiding this comment.
Removed both snapshot-specific IllegalArgumentException catches. Invalid app input is handled at the context-update or handoff-validation boundary instead. Request preparation failures remain handled by the conversation processor, including releasing the handoff reservation; added a regression test verifying a subsequent handoff can retry after a session-resolution failure.
| is QuerySubmitted -> { | ||
| data[keys.QUERY] = query | ||
| if (xdmFields.isNotEmpty()) { | ||
| data[keys.XDM_FIELDS] = xdmFields |
There was a problem hiding this comment.
[LOW] xdmFields on the hub event is readable by any extension, not just Assurance.
This is fine per the thread, but one line in the implementation guide would tell integrators where app context can surface.
There was a problem hiding this comment.
Added the clarification to the SDK implementation guide, app context is included in the hub event and can be read by Assurance and other extensions listening to that event. The guide distinguishes this from the production Edge tracking payload, where the context is stripped, and tracking-event device logs, which do not include the payload.
Bind context updates to the current session so freshly reapplied context survives rollover. Share depth-bounded validation between context and handoff paths, align numeric types, and reject cyclic input safely. Resolve conversation session IDs off the UI thread while preserving submission-time snapshots. Emit QUERY_SUBMITTED after session resolution, remove redundant snapshot catches, and document hub-event visibility. Add regression coverage for expiry, depth limits, cycles, serialization, snapshot consistency, threading, and handoff recovery. All 1,664 phone-debug unit tests and formatting checks pass.
rymorale
left a comment
There was a problem hiding this comment.
Thanks for all the review updates. There is one medium severity issue to resolve but overall the changes look good to me.
| synchronized(xdmContextLock) { | ||
| val sessionId = sessionManager.getSessionId() | ||
| val base = if (xdmContextSessionId == sessionId) heldXdmContext else emptyMap() | ||
| heldXdmContext = copyXdmObject(mergeXdmPatch(base, copiedPatch)) | ||
| xdmContextSessionId = sessionId |
There was a problem hiding this comment.
[MEDIUM] Context set before the first chat now expires 30 minutes after it is set.
updateXDMContext calls sessionManager.getSessionId(). When no session exists yet, that creates one (S1) timestamped now. Setting context doesn't refresh the session's activity after that. If the user opens chat more than 30 minutes later, the request gets a new session (S2). resolveXDMContext (L113-120) then sees the held context bound to S1, clears it, and sends {}.
This is a regression from 854b23b, where the null session sentinel kept context set before the first request. Concierge.updateXDMContext's KDoc still says context may be set before the first chat request, but the new test at ConciergeStateRepositoryTest.kt:207-211 locks in the expiry.
Suggestion: in updateXDMContext, look up the current session without creating one (e.g. a non-mutating currentSessionIdOrNull()). If no valid session exists, bind the context to null ("pending") and have resolveXDMContext adopt null-bound context for whatever session the next request uses. This also removes the datastore write inside xdmContextLock on the caller's thread.
There was a problem hiding this comment.
Fixed by replacing the session creating lookup with a non-mutating lookup. Context remains pending when no valid session exists and is adopted by the next request, so setting context does not start the inactivity clock. Added regression coverage for a delayed first request, updates after expiry, and queued chat/handoff snapshots. Full phone debug unit suite passes.
- Check for a valid session without creating or refreshing one. - Keep fresh context pending until the next request adopts it. - Preserve queued snapshots and discard context after session rollover. - Document pending context and add lifecycle regression tests. Validation: 1,672 phone-debug unit tests passed; formatting and whitespace checks passed.
Adds Concierge.updateXDMContext(Map) so a host app can supply XDM fields
once and have them travel with every conversation turn in the session.
Ports the iOS implementation of the same feature.
Merge semantics follow RFC 7396: nested maps merge recursively, a null
value removes a key, and lists replace wholesale. identityMap is
rejected -- Edge owns that namespace.
The held context is snapshotted once per turn, before the asynchronous
token-provider wait, so the querySubmitted tracking event and the
request that goes over the wire always carry the same fields. Handoff
turns merge the held context with their own fields, with call-specific
values winning on collision.
xdmFields are dropped before forwarding to Edge, alongside query and
notes -- app-supplied XDM is a PII risk and does not belong in the
customer's production pipeline.
Held context is cleared when the session rolls over; the app is
responsible for re-establishing it.