Skip to content

Hold XDM context for a session instead of passing it per call - #176

Merged
rymorale merged 4 commits into
adobe:devfrom
josej-ea:feature/held-xdm-context
Oct 2, 2026
Merged

rymorale merged 4 commits into
adobe:devfrom
josej-ea:feature/held-xdm-context

Conversation

@josej-ea

Copy link
Copy Markdown

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.

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

codecov Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

@josej-ea

Copy link
Copy Markdown
Author

@cdhoffmann & @rymorale - ready for your initial review

@rymorale rymorale 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.

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)

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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) }

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.

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_FAILED until 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.)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

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.

let's go with b

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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) {

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.

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":

  1. Rollover is only detected lazily, here. An updateXDMContext call made after expiry but before the next turn is also wiped (the new session id clears context updated after the previous turn test confirms this is intentional, to mirror iOS).
  2. 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 updateXDMContext resolve 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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?

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.

let's get it resolved in this pr as we are planning to release the latest concierge SDK version soon

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed on both points, will fix.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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())

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.

Two small things about this snapshot:

  1. This is where the null-in-list IllegalArgumentException (see the comment on ConciergeStateRepository.kt L343) escapes, since handleSendMessage runs 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.
  2. 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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))

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed stateRepository and sessionManager to constructor-assigned private val properties so they’re initialized once and can’t be reassigned.

Comment thread Documentation/implementation-guide.md Outdated
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.

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.
@josej-ea

Copy link
Copy Markdown
Author

@rymorale made updates and added responses for each item.

@josej-ea
josej-ea requested a review from rymorale September 29, 2026 16:55

@rymorale rymorale 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.

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) {

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.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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) {

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.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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 ||

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.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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()

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.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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) {

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.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

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.

[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.

@josej-ea josej-ea Oct 1, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.
@josej-ea
josej-ea requested a review from rymorale October 1, 2026 04:56

@rymorale rymorale 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.

Thanks for all the review updates. There is one medium severity issue to resolve but overall the changes look good to me.

Comment on lines +93 to +97
synchronized(xdmContextLock) {
val sessionId = sessionManager.getSessionId()
val base = if (xdmContextSessionId == sessionId) heldXdmContext else emptyMap()
heldXdmContext = copyXdmObject(mergeXdmPatch(base, copiedPatch))
xdmContextSessionId = sessionId

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.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.
@josej-ea
josej-ea requested a review from rymorale October 1, 2026 19:08
@rymorale
rymorale merged commit d9e0690 into adobe:dev Oct 2, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants