Fix reloadWorkspace nested resolver context#33928
Conversation
Greptile SummaryThis PR fixes a stale-context bug in
Confidence Score: 5/5Safe to merge — the fix is targeted, the schema change is accompanied by matching frontend and test updates, and the stale-context root cause is eliminated rather than papered over. The workspace reload mutation now returns a minimal success type instead of a deep resolver tree that was susceptible to stale-context reads. The context-propagation changes in external.py address the same class of bug for the single-location reload path. Frontend follows up with a no-cache query to get current location IDs, all error paths are handled, and tests are updated end-to-end. No correctness gaps were found in the changed paths. No files require special attention. The only pre-existing concern (the unused workspace_context parameter in GrapheneWorkspace.init) was already flagged in a prior review thread.
|
| Filename | Overview |
|---|---|
| python_modules/dagster-graphql/dagster_graphql/schema/roots/mutation.py | Replaces GrapheneWorkspace with new GrapheneReloadWorkspaceSuccess in the reload mutation result; mutation now calls reload_workspace() without capturing the return and returns a simple success object. Also correctly passes workspace_context=new_context in the single-location reload path. |
| python_modules/dagster-graphql/dagster_graphql/schema/external.py | Adds optional workspace_context parameter to GrapheneRepositoryLocation, GrapheneWorkspaceLocationEntry, GrapheneRepository, and GrapheneWorkspace; context is used in place of graphene_info.context when present, fixing stale-context issues in nested resolvers. |
| js_modules/ui-core/src/nav/useRepositoryLocationReload.tsx | Rewrites reloadFnForWorkspace to fire a follow-up workspaceOrError query (no-cache) after the now-simplified mutation; includes error handling for both the mutation and the follow-up query. |
| python_modules/dagster-graphql/dagster_graphql/implementation/external.py | Passes workspace_request_context through to both GrapheneWorkspaceLocationEntry and GrapheneWorkspace constructors in fetch_workspace. |
| python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_reload_repository_location.py | Tests updated to match new mutation response type (ReloadWorkspaceSuccess); helper reload_workspace() refactored to issue the mutation then query workspaceOrError separately, matching the production code path. |
| js_modules/ui-core/src/nav/types/useRepositoryLocationReload.types.ts | Generated TypeScript types updated to replace the large Workspace branch in ReloadWorkspaceMutation with ReloadWorkspaceSuccess { success: boolean } and updated query hash. |
| js_modules/ui-core/src/graphql/schema.graphql | Schema updated: ReloadWorkspaceMutationResult union now uses ReloadWorkspaceSuccess instead of Workspace; new ReloadWorkspaceSuccess { success: Boolean! } type added. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as UI Client
participant GQL as GraphQL Server
participant WS as Workspace
Note over UI,WS: Before this PR (buggy path)
UI->>GQL: reloadWorkspace mutation
GQL->>WS: reload_workspace() returns new_context
GQL->>GQL: fetch_workspace(new_context)
Note over GQL: Nested resolvers use stale graphene_info.context
GQL-->>UI: Workspace with potential DagsterCodeLocationNotFoundError
Note over UI,WS: After this PR (fixed path)
UI->>GQL: reloadWorkspace mutation
GQL->>WS: reload_workspace() discard return
GQL-->>UI: "ReloadWorkspaceSuccess { success: true }"
UI->>GQL: workspaceOrError query (no-cache)
GQL->>WS: get fresh workspace state
GQL-->>UI: "Workspace { locationEntries ids }"
UI->>GQL: Poll each location until loaded
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as UI Client
participant GQL as GraphQL Server
participant WS as Workspace
Note over UI,WS: Before this PR (buggy path)
UI->>GQL: reloadWorkspace mutation
GQL->>WS: reload_workspace() returns new_context
GQL->>GQL: fetch_workspace(new_context)
Note over GQL: Nested resolvers use stale graphene_info.context
GQL-->>UI: Workspace with potential DagsterCodeLocationNotFoundError
Note over UI,WS: After this PR (fixed path)
UI->>GQL: reloadWorkspace mutation
GQL->>WS: reload_workspace() discard return
GQL-->>UI: "ReloadWorkspaceSuccess { success: true }"
UI->>GQL: workspaceOrError query (no-cache)
GQL->>WS: get fresh workspace state
GQL-->>UI: "Workspace { locationEntries ids }"
UI->>GQL: Poll each location until loaded
Reviews (3): Last reviewed commit: "Return success from reload workspace mut..." | Re-trigger Greptile
|
@gibsondan Can you review the PR ? |
|
@cmpadden Can you help me review this? Or direct this to the right owner? |
gibsondan
left a comment
There was a problem hiding this comment.
I support the spirit of this change, but I think the problem is that you need to remember to thread this new thing through to every entity that is a child of GrapheneWorkspace or GrapheneRepository. That's a lot of resolvers/types and i don't have confidence that we have caught them all here, or that new ones will remember about this unusual codepath that needs to be checked every time you reference graphene_info.context in any resolver
Really I'm not sure that reloadWorkspace should be returning the workspace object at all - maybe it should just return a success boolean with the expectation that you then make another graphql call after the mutation finishes to read from the updated context?
Summary
Thread the freshly loaded workspace request context through GraphQL workspace response objects returned by reloadWorkspace.
reloadWorkspace creates a new request context, but nested repository resolvers can still use the original GraphQL request context via graphene_info.context. If the reload changes the set of workspace locations, the top-level reload response can include a newly added location while nested fields such as repositories/pipelines resolve against the stale pre-reload context and fail with DagsterCodeLocationNotFoundError.
This change passes the workspace context through the workspace/location/repository Graphene wrappers and uses it for repository lookup, batch loaders, partition sets, permissions, and asset manifest context lookups when available. Existing callers that construct these wrappers without an explicit context continue to fall back to graphene_info.context.
Test Plan