Fix WorkspaceContext over-fetching code locations on cascading updates. #26970
Conversation
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 01b5958. |
| if (fetchingStatusesRef.current[localKey]) { | ||
| return fetchingStatusesRef.current[localKey]; |
There was a problem hiding this comment.
should there be an await here? Im confused by the different types being returned here and below
There was a problem hiding this comment.
It behaves the same with or without an await
There was a problem hiding this comment.
ah ok and the await below is just to sequence the deletion
| return data; | ||
| } catch (error) { | ||
| console.error(`Error fetching location data for ${name}:`, error); | ||
| const localKey = name + '@' + (locationStatuses[name]?.versionKey ?? ''); |
| fetchingStatusesRef, | ||
| }); | ||
| previousLocationVersionsRef.current[location.name] = | ||
| locationStatuses[location.name]?.versionKey ?? ''; |
There was a problem hiding this comment.
[1] I assume its fine since i see some bits of existing code that do it - but what is the expected result of using empty string when we get null ?
There was a problem hiding this comment.
In most cases this will not be null (since at this point, if we're fetching it should have been triggered by the code location status query), however it's possible to call the refetch function (exposed by WorkspaceContext) before the status query has come back in which case we would end up fetching the location once without the version and then again once we know what version we're fetching. There's no good way to guard against it since its possible the verison changed in between the refetch call and the status query being made/returning.
There was a problem hiding this comment.
Is that a scenario under test or worth putting under test?
I don't have a strong mental model of all the mechanisms here so I'm just reacting to generally suspicious looking things
There was a problem hiding this comment.
I moved the version update into the fetchLocationData function instead. And instead of using the versionKey from the status update that triggered the fetch, I'm using the versionKey from the location response now so it should mitigate the weirdness here.
There was a problem hiding this comment.
But yeah, there was a theoretical issue here if the location query triggered by the refetch call was slower than a subsequent query... though actually... that issue can still happen... so maybe I should also check if the updateTimestamp is greater than the last version? ugh
There was a problem hiding this comment.
Random idea: But instead of polling for version keys on the code location status query, maybe I only use that query to get all of the locations, but separately poll the individual locations with a new query that takes the versionKey I'm currently on as an input and either returns the updated code location or a response indicating that I'm already up to date. Would that be easy to do? Could help eliminate some of the complexity here.
| if (fetchingStatusesRef.current[localKey]) { | ||
| return fetchingStatusesRef.current[localKey]; |
There was a problem hiding this comment.
ah ok and the await below is just to sequence the deletion
| fetchingStatusesRef, | ||
| }); | ||
| previousLocationVersionsRef.current[location.name] = | ||
| locationStatuses[location.name]?.versionKey ?? ''; |
There was a problem hiding this comment.
Is that a scenario under test or worth putting under test?
I don't have a strong mental model of all the mechanisms here so I'm just reacting to generally suspicious looking things
Summary & Motivation
The main issue is that
previousLocationVersionsRef.currentwas only being updated after ALL code locations were fetched. That means that whenever a code location was fetched, we'd think that all of the code locations were stale again, including the one we just fetched, becausepreviousLocationVersionsRef.currentwas still pointing to the old version. To fix this I instead updatepreviousLocationVersionsRef.currentafter each individual fetch. In addition, I added some extra guards withinfetchLocationDataitself to prevent overfetching due to someone callingrefetchmultiple times.How I Tested These Changes
Added a jest test that failed without these changes.