Issue/1611 stale async expression cache

[janadh]

@leoniedickson, When you get a chance, can you please verify if the change is okay and does not break any other functionalities?

[leoniedickson]

PR Review: #1928 — Fix stale async expression cache results

What the PR does

In evaluateLinkIdVariables and evaluateQuestionnaireLevelVariables, the cache key changes from expression (expression-only) to expression|JSON.stringify(focusNode) (compound). This ensures that when the user changes their answer (different qrItem → different focusNode), the compound key misses and the expression is correctly re-evaluated instead of returning a stale result.

The core logic is correct. The fix directly addresses the scenario Sean described.

---

Issues

1. Cache hit doesn't propagate the cached value into the new context — significant

This is the deeper problem Sean's ChatGPT link was pointing at. The continue on a cache hit skips the assignment:

if (isExpressionCached(variable.expression, fhirPathTerminologyCache, focusNode)) {
  continue;  // ← skips: fhirPathContext[variable.name] = result
}

fhirPathContext starts as a spread of existingFhirPathContext (the store's stale snapshot). On a cache hit, the cached value isn't applied — so the new context keeps the stale value.

This is directly observable in practice: one invocation correctly evaluates memberOf() → [true] → caches under compound key → returns context with isSeverity: [true]. A subsequent concurrent invocation with the same qrItem hits that cache, skips the assignment, and returns isSeverity: [] from the stale store spread — then overwrites the store.

The fix:

const key =
  focusNode \!== undefined ? `${expression}|${JSON.stringify(focusNode)}` : expression;
if (Object.prototype.hasOwnProperty.call(fhirPathTerminologyCache, key)) {
  fhirPathContext[variable.name] = fhirPathTerminologyCache[key]; // apply cached value
  continue;
}

This is what makes Sean say "there could be a better way." The compound key correctly avoids stale results on focusNode change, but the cache-hit path still silently produces stale context in same-focusNode concurrent invocations. Both have the same root cause.

2. Incomplete coverage across callers — moderate

The PR only updates fhirpath.ts. There are 8 other isExpressionCached / cacheTerminologyResult call sites across 5 files that still use expression-only keys:

File	Lines
calculatedExpression.ts	116, 135, 196, 215
enableWhenExpression.ts	112, 131
answerOptionsToggleExpressions.ts	79, 114, 153, 194
targetConstraint.ts	75, 110, 141, 173
parameterisedValueSets.ts	151, 170, 233, 252

calculatedExpression.ts is the most notable — if a calculatedExpression uses memberOf() without %variables, it could cache under the expression-only key and serve stale results when the answer changes. The same bug, different call site.

3. The "valid cache hit" test masks the real problem — minor

it('skips re-evaluation when qrItem is unchanged (valid cache hit)', async () => {
  const result = await evaluateLinkIdVariables(
    'item-link',
    variablesFhirPath,
    { myVar: ['A'] }, // ← context already has the correct value
    terminologyCache,
    terminologyServerUrl,
    qrItem
  );
  expect(result.fhirPathContext.myVar).toEqual(['A']);
});

The starting context is { myVar: ['A'] }, which is already correct. The test passes regardless of whether the cache hit applies the cached value or just skips. A test where the starting context is stale — { myVar: [] } — would expose the issue from point 1 and would currently fail. Worth adding.

4. evaluateQuestionnaireLevelVariables serializes the entire QR — minor

resource is the full QuestionnaireResponse. JSON.stringify(resource) on every expression evaluation could be expensive for large QRs with many items and answers, and it invalidates the cache whenever any part of the QR changes, including unrelated items. No unit test covers this code path in the PR.

---

What's good

• Core compound key logic is correct for the focusNode-change scenario (#1611)
• % expression exclusion correctly preserved
• evaluateLinkIdVariables and evaluateQuestionnaireLevelVariables updated consistently
• Tests for isExpressionCached and cacheTerminologyResult are well-structured
• The regression test (re-evaluates when qrItem changes) directly demonstrates the bug

---

Summary

The fix correctly addresses the focusNode-change case from #1611 and is an improvement over the status quo. But it is incomplete: the cache-hit path doesn't apply the cached value to the new context (leaving stale results in same-focusNode concurrent invocations), and 8 call sites across 5 other files still use expression-only keys. The continue-without-apply pattern is the "better way" Sean was pointing at. This should either be addressed here or tracked as a follow-up.