Fix Streaming EA outages when receiving different casing for same asset#720
Closed
Fix Streaming EA outages when receiving different casing for same asset#720
Conversation
alecgard
changed the title
alecgard
force-pushed
the
fix/case-sensitive-feed-outage
branch
2 times, most recently
from
dbf18b6 to
3142128
Compare
…eed death When two requests share the same normalised cache key but differ in casing (e.g. USDe/USD and usde/usd), ExpiringSortedSet.add() was overwriting the stored params with the new casing. This caused StreamingTransport's JSON.stringify diff to see a spurious change on the next background execute, triggering an unnecessary unsubscribe then resubscribe. Because sendMessages sends subscribes before unsubscribes, a case-insensitive provider would start the feed then immediately kill it — and because localSubscriptions was then set to desiredSubs, no diff would fire again, leaving the feed permanently dead and both request variants returning 504 after cache expiry. Fix: when a key already exists in ExpiringSortedSet, only refresh the TTL and keep the original value (first-writer-wins). Emit a warning when the incoming value differs from the stored one, as this indicates inconsistent request casing that may cause issues with case-sensitive providers.
alecgard
force-pushed
the
fix/case-sensitive-feed-outage
branch
from
3142128 to
261b937
Compare
cll-gg
approved these changes
Mar 19, 2026
cawthorne
approved these changes
Mar 19, 2026
alejoberardino
requested changes
Mar 19, 2026
Contributor
alejoberardino
left a comment
alejoberardino
left a comment
There was a problem hiding this comment.
@alecgard @cl-efornaciari @cawthorne
I am against doing solving our current problem (some feeds have conflicts) with a framework change right now in general, as I believe there's better alternatives:
- Changing the jobs is slow, but it's the most performant and also keeps our source of truth the cleanest. This should be done regardless, but does fix the issues we have.
- I believe the implemented EAs themselves cause this issue: many of them erase casing at the wrong stage (e.g. before subscribing) instead of making this apply before keys are resolved (i.e. as request transforms). That erasure is not performant, and when trying to squeeze out performance for high throughput like we do for these EAs it becomes very relevant; however, if necessary, we should at least fix the EAs from doing it in the wrong place.
- A framework change increases the surface for risk much more than individual EA changes or job fixes.
That said a framework should serve its users, so if the desire is for case insensitivity I can only urge caution as I do see some currently working cases breaking from the change in expectations. Happy to chat more about this too, I'm not currently maintaining this framework or the individual EAs so if you don't agree let me know or dismiss explicitly
| add(value: T, ttl: number, key: string) { | ||
| let node = this.map.get(key) | ||
| if (node) { | ||
| if (JSON.stringify(value) !== JSON.stringify(node.data.value)) { |
Contributor
There was a problem hiding this comment.
stringifies are expensive, we should not be performing these here
| `Subscription set received a value for key "${key}" that differs from the stored value. ` + | ||
| `Keeping the original value to avoid unnecessary subscription churn. ` + | ||
| `This indicates requests are using inconsistent parameter casing - ` + | ||
| `stored: ${JSON.stringify(node.data.value)}, incoming: ${JSON.stringify(value)}`, |
Contributor
There was a problem hiding this comment.
even more stringifies haha
Contributor
Author
|
Closing this PR as after discussing with @alejoberardino we found a case where this could cause the EA to return data from the wrong asset if DP is case sensitive. Back to the drawing board. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits
Can see the repro tests failed before the fix:
And passed after:
