Recovery for locations stuck after failed refresh#33642
Conversation
A race condition can lead to a daemon or webserver getting stuck
in a permanent state where the workspace has registered a code
location as being in an errored state, but the watch thread
believes (correctly) that everything is healthy, so the watch
thread will not trigger the callback that is necessary for the
workspace to re-refresh the code location.
This can happen during Kubernetes rolling deployments (a code
location refresh can fail when the gRPC call routes to a dying
old pod via the EndpointSlice). But it can also happen for more
mundane reasons, such as a transient network failure during
`refresh_code_location()`. The failure sequence is something
like this:
- T+0: `dagster._grpc.server_watcher:watch_grpc_server_thread()`:
- `client.get_server_id()` returns `"old-id"`
- `set_server_id("old-id")`
- T+1: New code location pod starts.
- T+2: Watch thread poll hits new pod:
- `client.get_server_id()` returns `"new-id"`
- `"old-id" != "new-id"`
- `set_server_id("new-id")`
- `on_updated(name, "new-id")` → `LOCATION_UPDATED` event →
`_send_state_event_to_subscribers()` →
`_location_state_events_handler()` → `refresh_code_location()`
- Note: `on_updated` is called synchronously on the watch
thread, so `refresh_code_location()` (T+3) runs to
completion before `watch_for_changes()` resumes looping (T+4).
- T+3: `dagster._core.workspace.context:refresh_code_location()`:
- → `_load_location()` → `origin.create_location()`
- → `GrpcServerCodeLocation()` → constructor makes gRPC calls
- → at least one call routes to dying old pod via K8s EndpointSlice
- → FAILS → the refreshed `CodeLocationEntry` is constructed
with `load_error` set, `code_location=None`.
- T+4: The watch thread continues polling within
`watch_for_changes()`. All calls to `client.get_server_id()`
succeed and return `"new-id"` from the new pod.
`curr == new_server_id == "new-id"`, so no callbacks are
invoked. The `CodeLocationEntry` still has `load_error` set,
`code_location=None`.
- T+5: The watch thread continues polling. Same result. Nothing
happens. The `CodeLocationEntry` is still in an error state.
- T+∞: Location is stuck in error state forever. The watch
thread is alive, the server is healthy, and the `server_id`
isn't changing, so the watch thread has no reason to call a
callback that would trigger a retry for
`refresh_code_location()`.
The race condition doesn't have to involve a rolling deployment.
T+1 and T+2 can also be a gRPC failure that triggers the
`reconnect_loop()`, followed by triggering `LOCATION_ERROR` and
then `LOCATION_UPDATED`, which triggers
`refresh_code_location()` for T+3. Then another brief network
blip happens, causing T+3 to still fail while T+4 still
succeeds, and then control returns to the infinite
`watch_for_changes()` loop.
In practice, the error state doesn't literally last forever. If
the watch thread eventually detects a new server_id, or hits a
prolonged network blip followed by eventual recovery, then a
callback is invoked, and `refresh_code_location()` will have
another chance to succeed.
This change adds three layers of recovery:
(1) Event handler recovery: any non-disconnect event for an errored location
triggers a refresh, so `LOCATION_RECONNECTED` events from the normal
reconnect path also recover errored locations.
(2) Retry with exponential backoff in refresh_code_location (5
attempts with 0.5s/1s/2s/4s backoff, ~7.5s worst-case sleep).
This handles the common case where the underlying network
issue resolves quickly.
(3) Watch thread error-aware polling, for cases of very bad luck
where all `refresh_code_location` retries fail, but then the
next and all subsequent watch thread calls all succeed. Inject a
`needs_location_refresh` callback so the watch thread can detect
when the workspace entry is errored or has a stale version key.
When detected, it fires `on_disconnect()` + `on_reconnected()`
to trigger a recovery refresh, with a double `get_server_id()`
check to confirm the server is actually reachable.
Also make all callers provide all callbacks to
`create_grpc_watch_thread`, and make the callbacks required to
prevent accidental bugs.
With these improvements, the race condition is avoided as
follows:
- T+3 is less likely to fail, due to the retries with
exponential backoff from fix (2).
- If T+3 does still fail and then the network recovers between
T+3 and T+4, step T+4 will no longer be a no-op, but instead
will invoke `on_disconnect()` followed by `on_reconnected()` due
to fix (3). The latter will trigger `refresh_code_location()`,
due to fix (1).
Fixes dagster-io#17319
Fixes dagster-io#32216
Identification and fixing of the issue was tool assisted, as was
writing this commit message, but both the code and the commit
message have been significantly reviewed and edited by me.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR adds recovery for gRPC code locations that get stuck after a transient refresh failure. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| python_modules/dagster/dagster/_grpc/server_watcher.py | Adds periodic stuck-error recovery and keeps reconnect polling alive after an exhausted outage. |
| python_modules/dagster/dagster/_core/workspace/context.py | Passes workspace lookup and refresh callbacks into the watch thread. |
| python_modules/dagster/dagster_tests/general_tests/grpc_tests/test_watch_server.py | Expands watcher tests for reconnect, error, and periodic recovery behavior. |
| python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_watch_grpc_server.py | Adds integration coverage for stuck-location recovery and post-error refresh signaling. |
Reviews (6): Last reviewed commit: "Merge pull request #1 from dagster-io/gi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… test timeouts Remove the retry-with-exponential-backoff loop from refresh_code_location, reverting it to its original single-attempt behavior. The retry loop ran synchronously within the _state_subscribers_lock callback chain, blocking event delivery to all other locations for up to ~7.5s. Recovery is now handled entirely by the watch thread's needs_location_refresh mechanism, which runs on its own per-location thread and doesn't hold any shared locks. Add outer-level backoff to the watch thread's recovery polling: wait 2 poll cycles to confirm the error persists (giving get_server_id() a chance to throw and trigger reconnect_loop instead), then retry every 10 cycles. This prevents log spam and unnecessary gRPC calls when a location is permanently stuck. Increase test timeouts from watch_interval * 5 (0.5s) to a fixed 5 seconds to avoid flakiness under CI load, since the recovery path involves multiple gRPC calls with 2-second request timeouts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@greptileai Please re-evaluate my PR, given the feedback that was addressed in commit f4e9fa7 . Does this fully address your prior concerns? Do you have any new concerns? Can you re-evaluate your confidence score and review comments? |
|
Yes, commit f4e9fa7 fully addresses all my prior concerns! Here's what was successfully resolved: ✅ P2 - Lock contention removed: The retry loop is completely gone from ✅ P2 - Second ✅ P2 - Test timeout flakiness: Changing to fixed 5-second timeouts (from ✅ P2 - Outer-level backoff implemented: The 2-cycle confirmation + retry-every-10-cycles pattern is a good balance. The 2-cycle wait gives New observations: The implementation is solid. The only edge case worth considering is the scenario you mentioned in your comment - where Confidence: 9/10 ⬆️ (previously 7/10) The approach is well-tested, the concerns are fully addressed, and the code is cleaner than before. The integration test provides good end-to-end coverage of the recovery path. Ready for merge from my perspective! |
|
@greptileai Can you provide an updated Summary and Sequence Diagram? |
SummaryThis PR fixes a race condition where code locations get permanently stuck in an error state after a failed refresh. The issue occurs when the gRPC watch thread detects a server change and triggers a refresh, but the refresh fails due to a transient network error (e.g., during Kubernetes rolling deployments). After the failure, the watch thread continues polling the healthy server but never retries the refresh, leaving the location errored indefinitely. The fix adds two recovery mechanisms:
Sequence DiagramsequenceDiagram
participant WT as Watch Thread
participant WS as Workspace
participant gRPC as gRPC Server
Note over WT,gRPC: PROBLEM: Race Condition (Before Fix)
WT->>gRPC: poll() - detect server ID change
WT->>WS: trigger refresh_code_location()
WS->>gRPC: load location (network fails)
gRPC--xWS: transient error
WS->>WS: mark location as ERRORED
loop Subsequent polls
WT->>gRPC: poll() - server healthy ✓
Note over WT: Server looks good,<br/>no refresh triggered
Note over WS: Location stuck ERRORED
end
Note over WT,gRPC: SOLUTION: Two-Layer Recovery
rect rgb(200, 240, 200)
Note over WT,WS: Layer 1: Event Handler Recovery
WT->>WS: LOCATION_RECONNECTED event
WS->>WS: check: location errored? YES
WS->>gRPC: trigger refresh_code_location()
gRPC-->>WS: success
WS->>WS: clear error state
end
rect rgb(200, 220, 240)
Note over WT,WS: Layer 2: Watch Thread Error Detection
WT->>WS: needs_location_refresh() callback
WS-->>WT: location is errored
Note over WT: Wait 2 cycles to confirm
WT->>WS: needs_location_refresh()
WS-->>WT: still errored
WT->>WS: fire on_disconnect + on_reconnected
WS->>gRPC: trigger refresh_code_location()
gRPC-->>WS: success
WS->>WS: clear error state
end
The diagram shows how a transient network error during refresh leaves the location permanently stuck (top section), and how the two-layer recovery mechanism solves it through either normal reconnect events or watch thread error detection (bottom sections). |
|
@gibsondan @cmpadden @greptile-apps[bot] @greptileai With the recent commit e32df69, I believe that reviewing and merging this PR has become even more important. Before commit e32df69, the Issues #17319 and #32216 are particularly likely to be triggered during a rolling deployment/restart. I created a very hacky (and extremely non-ideal) set of probes to specifically avoid the race condition in that specific case (but doesn't mitigate #17319 and #32216 in the more general sense), but that hack relies on the I don't want commit e32df69 to be reverted, as I would like to take advantage of |
|
Your analysis is exactly right, and this is an important escalation. Here's a clear summary of the regression impact for maintainer context: Before
After
This means From a code review standpoint, this PR is in good shape. All three P2 concerns I raised in the original review were addressed in commit
The one remaining open review thread (backoff for persistent failures) was addressed by the This PR is ready for maintainer review and merge. @gibsondan @cmpadden — the urgency here has materially increased since |
|
@jmoldow thanks for sending this out and sorry for the delay, will take a look at this. Wanted to confirm one thing about the recovery though first to make sure i fully understand - manually reloading the location in the UI does work to get it out of the bad state, right? (but i agree ideally it would be automatic and retry) And the daemon should automatically retry and get out of the bad state automatically either way, before and after this change, since it periodically refreshes the whole workspace every 60 seconds? https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_daemon/controller.py#L275-L282 |
|
i.e. i'm confused by the claim that "the only recovery path now is to kill the impacted daemon/webserver pod(s)". It's certainly not ideal, but you can press this Reload button in the UI to recover too i think? |
gibsondan
left a comment
gibsondan
left a comment
There was a problem hiding this comment.
couple of questions for you inline
|
@gibsondan We definitely observed it more for the webserver than the daemon. I'm not sure if that's a function of the webserver just being more obvious to observe; or if the daemon was successfully auto-recovering per the controller.py#L275-L282 snippet you linked. I thought we observed it stuck on the daemon once, but I haven't been able to find any old logs / chat messages about that, so it's possible I'm mis-remembering. The most I could find was a commit message from myself, saying "This could explain the multi-hour outage of scheduled jobs that we saw once", but alas I didn't document when that occurred or save any evidence for why I believed this was a potential hypothesis. Regarding the Reload button: here's two things I vaguely remember (it's been a few months since I did these investigations and made some of these changes):
So you're right, I was incorrect in my last message to say that killing the webserver pod is the only recovery method. But it's a bit more deterministic than hitting Reload an unknown number of times. And for what it's worth, we're on Dagster 1.12.8 at the moment. I know that issues #17319 and #32216 haven't been addressed yet in Dagster 1.13, but I don't know if anything about the Reload button or |
gibsondan
left a comment
gibsondan
left a comment
There was a problem hiding this comment.
ok cool - if you can make that change - (which i think largely just involves not passing in the version_key to _should_recover_location and taking out the version_key mismatch checking) and rebase I can prioritize getting this in. I think starting with just the transient error makes sense too, put more on that inline. Sorry again for the delay and thanks for pushing on this
…or and direct-refresh Replace the prior needs_location_refresh(name, version_key) callback — which triggered recovery on both error-state and stale-version-key mismatches — with two lower-level hooks: get_location_entry(name) and refresh_code_location(name). The watch thread now re-raises DagsterUserCodeUnreachableError only when the workspace entry's load_error matches that class, and within reconnect_loop it calls refresh_code_location directly once GetServerId succeeds (rather than firing synthetic on_disconnect/on_reconnected events). This narrows recovery to transient gRPC failures, per PR review feedback: permanent user code errors (e.g. DagsterUserCodeProcessError from a syntax error) will no longer be retried, and the multi-pod rolling-deploy race that produces a stale-but-not-errored version_key is intentionally out of scope (tracked separately). Also in this commit: - Workspace: rename _should_recover_location(name, version_key) → _get_location_entry_without_locking(name); drop the version_key parameter. - Workspace event handler: drop the LOCATION_DISCONNECTED special case so that any event on an errored location triggers a refresh. - Server watcher: replace the server_id dict with a typed ServerId inner class; add type annotations throughout. - Server watcher: rate-limit recovery refresh attempts to one per max_reconnect_attempts cycles to avoid log spam on permanently stuck locations. - Server watcher: keep the thread alive after on_error so eventual recovery is signaled via on_updated (the stored server_id is cleared on error to force the on_updated branch).
Polishing pass on server_watcher.py from a docstring/comment audit: - watch_grpc_server_thread docstring: tighten "at most once per disconnect cycle" to "at most once per reconnect_loop invocation". - reconnect_loop: replace contradictory comment block on the server-ID-changed/post-on_error branch with a single explanation that flapping is prevented by get_location_entry_or_raise_unreachable_error. - reconnect_loop: clarify retry-cadence comment to distinguish the pre-on_error (retry every poll) and post-on_error (retry every Nth poll) paths. Comments/docstrings only; no behavior change.
…mments Polishing pass on the watch-server test files: - test_watch_grpc_server.py: replace the misleading "should have arrived before" comment (the assertion only checks presence, not ordering) with an explanation of why both events are expected by the time LOCATION_ERROR is observed. - test_watch_server.py: add a docstring to wait_for_condition explaining the min(5, interval * 60) default-timeout formula (cap at 5s for the long-interval case, cap at interval * 60 for the short-interval case). - test_watch_server.py: explain the >= 2 poll-count check and the chained on_reconnected == on_disconnect <= 1 assertion in the create_server_process_and_watch_thread fixture, and note that the trailing counter reset establishes a clean baseline for each test. Comments/docstrings only; no behavior change.
…rrored[20] The post-recovery assertion at the end of test_grpc_watch_thread_recovery_when_errored was timing-racy: after firing on_updated to recover from the errored state, the test captured a counter snapshot and then slept watch_interval * 2 (0.2s) before asserting that get_location_entry_count had advanced by >= 2. In the [20] parametrization (recovery via on_updated after on_error fired) the watch thread can already be mid-shutdown_event.wait() by the time the test captures the snapshot, so only one full poll cycle reliably completes in the 0.2s sleep window. Observed ~30% failure rate locally (3/10 runs) with `assert 26 >= (25 + 2)`. Replace the fixed sleep + assert with wait_for_condition (the pattern used elsewhere in the same test file), polling for the +2 increment with a timeout of watch_interval * 5. The meaningful post-recovery invariant — that refresh_code_location is not called again — is preserved as a hard assert after the liveness check. Verified: 10/10 passes on the previously-flaky [20] parametrization.
…rtion Follow-up to the previous de-flake commit, which inadvertently dropped the `assert called_event == called_event_expected` check at the end of test_grpc_watch_thread_recovery_when_errored. That assertion catches a different regression than the get_location_entry_count liveness check and the refresh_code_location_count == snapshot check: it verifies that no *additional* events (on_updated, on_disconnect, on_reconnected, on_error) fired after recovery — i.e. the system has truly settled, not just continued polling. Place it after the liveness wait_for_condition so any latent extra event has at least 2 poll cycles to surface.
…d tighten logging - Rename set_server_id → update_server_id to reflect that it can also fire on_updated. - Move the module logger to module scope so it can be reused without re-creating per call. - Switch caught-exception logging from logger.warning to logger.exception to capture tracebacks. - Annotate the reconnect_loop "new server ID before on_error" branch with a comment explaining why it deliberately skips the unreachable-error gate. - Convert the test-suite should_not_be_called helper into a pytest fixture backed by a shared list, so unexpected callback invocations fail the test on teardown rather than raising from inside the watch thread.
…est type annotations Condense the brand-new portions of the watch_grpc_server_thread docstring and inline reconnect_loop comments to read as documentation rather than review responses, while preserving the original master-era sentences. Add missing type annotations to test fixtures and test function signatures in test_watch_server.py. No behavior change.
…rror recovery test - Shorten WATCH_INTERVAL via patch.object inside a class-scoped graphql_context fixture so the recovery tests don't wait on the default 1s poll. - Use DagsterUserCodeUnreachableError (not RuntimeError) as the injected load_error so the watch thread's get_location_entry_or_raise_unreachable_error path actually fires. - Assert update_timestamp advances and LOCATION_RECONNECTED is observed after recovery. - Add test_grpc_server_on_error_then_recovery_via_on_updated: patch DagsterGrpcClient.get_server_id to fail past MAX_RECONNECT_ATTEMPTS, then succeed, and assert recovery routes through LOCATION_UPDATED (not LOCATION_RECONNECTED) so the workspace handler clears the error state.
…odeUnreachableError Note in the docstring that non-transient load errors (e.g. DagsterUserCodeProcessError from a user-code syntax error) are intentionally not retried, to avoid log spam on permanently broken locations.
|
@greptileai Please re-evaluate my PR, given the feedback that was addressed in the commits that have been added. Do these commits fully address all prior concerns? Do you have any new concerns? Can you re-evaluate your confidence score and review comments? Please provide an updated Summary and Sequence Diagram. |
| logger.exception( | ||
| f"In gRPC watch server thread for location {location_name}, on_disconnect raised {exc.__class__.__name__}: {exc}" | ||
| ) | ||
| except Exception as exc: |
There was a problem hiding this comment.
I noticed that we had no protection from any other exceptions besides DagsterUserCodeUnreachableError being raised. That seems like an issue waiting to happen: the on_* calls, and the refresh_code_location(), can raise other Dagster* errors. And since it is possible to register other callbacks besides just _location_state_events_handler(), theoretically any exception could be thrown.
If that were to happen before, the watcher thread would die, and the code location would be stuck in whatever state it was in, until/unless a different mechanism refreshed the code location state.
These new except Exception blocks prevent the watcher thread from completely dying.
During iteration on the recent changes, this already came in handy. I had made a mistake in some code I had written, and an exception was being thrown all the way up to here. Before I added the logging, I had no idea why my test was failing. After the logging, the cause was made clear.
| logger.exception( | ||
| f"In gRPC watch server thread for location {location_name}, watch_for_changes raised {exc.__class__.__name__}: {exc}" | ||
| ) | ||
| shutdown_event.wait(watch_interval * max_reconnect_attempts) |
There was a problem hiding this comment.
The unknown exception could potentially be spammy and permanent, or at least long lived. We don't really know the nature of the exception. Since the previous behavior was for the thread to completely die forever, it seems fine to have a "long" sleep before repeating.
The original code used a dict. I changed it to Counter because I thought I might end up using one or more Counter methods, but that ended up not being necessary. @greptileai flagged a major footgun that I never knew before about `Counter.update()`: > Like `dict.update()` but adds counts instead of replacing them https://docs.python.org/3/library/collections.html#collections.Counter.update I also realized that using `Counter` instead of `dict` could lead to bugs or false-positives if a key is typo'd, since it would return 0 instead of throwing a `KeyError`.
@greptileai Thanks, I didn't know this about |
|
Good fix — reverting to |
gibsondan
left a comment
gibsondan
left a comment
There was a problem hiding this comment.
ok I promise this is extremely close and I am on the verge of merging this. I have one hopefully final question for you about whether we still need this _location_state_events_handler business in this new world or whether we can just rely on claling refresh_code_location directly in the watcher thread now?
| Called from the watch thread without holding self._lock. This is safe because | ||
| _current_workspace is replaced atomically (single reference assignment) and we only need a | ||
| consistent-enough snapshot — correctness doesn't depend on reading the latest value. |
There was a problem hiding this comment.
i suspect this same logic actually applies anywhere we read self._current_workspace currently - but I can clean that up separately
| # Refresh on update/error events, or on any event when the location is errored (to attempt | ||
| # recovery). | ||
| should_refresh = location_is_errored or event.event_type in refresh_event_types | ||
|
|
||
| if should_refresh: |
There was a problem hiding this comment.
I am 100% on board with the watch thread change now but I am still a little confused about what this other recovery layer is buying us exactly now. Do you have a specific example/event in mind where it would be useful to refresh whenever the location is in an error state? Also just noting that the summary says this one is also scoped to the UnreachableError but i'm not seeing that here - seems like it will refresh on any location that's on an error state whenever it either disconnects or reconnects.
Are you sure this wasn't an artifact of the old logic before where we were implementing the refresh via a disconnect-then-reconnect call, is it possible that we no longer need this?
There was a problem hiding this comment.
Also just noting that the summary says this one is also scoped to the UnreachableError but i'm not seeing that here - seems like it will refresh on any location that's on an error state whenever it either disconnects or reconnects.
Thanks, that's a mistake in the summary. This was not intended to be scoped to any particular type of error.
I am 100% on board with the watch thread change now but I am still a little confused about what this other recovery layer is buying us exactly now. Do you have a specific example/event in mind where it would be useful to refresh whenever the location is in an error state?
A few thoughts:
(general thought) Since the error recovery in the watcher has been scoped specifically to UnreachableError, and since the old (and new) code will trigger a refresh on every on_error/on_update event, I thought it could be useful to have something in the middle: something that triggers a refresh on other events, but only when an error is already present, and for any error not just UnreachableError.
(specific scenario)
- The watcher enters a reconnect loop (from its direct GetServerId call)
- Some other thread triggers a refresh for the code location, and it is registered as being in an error state (maybe an UnreachableError, but maybe it progressed to a different transient error state)
- The watcher detects a reconnect, and fires an on_reconnected event
- (with the new code) A refresh will be triggered, and the code location will recover (unless a non-unreachable error still persists)
- (with the old code for
_location_state_events_handlerand the new code for the watcher thread, and if the error state is UnreachableError)- No refresh is triggered
- Watcher exits the reconnect loop and enters the
watch_for_changesloop - Watcher sees the error state, triggers on_disconnect, and enters the reconnect loop
- Watcher triggers a refresh, and the code location will recover
- Watcher triggers on_reconnect, exits the reconnect loop, and enters the
watch_for_changesloop
The code location ends up recovering in the end either way, but there is more thrashing and a slower recovery if we're using the old _location_state_events_handler code.
Let me know what you think about that scenario.
Are you sure this wasn't an artifact of the old logic before where we were implementing the refresh via a disconnect-then-reconnect call, is it possible that we no longer need this?
It's true that this logic was originally written for the first approach. But it was somewhat intentional (though not strictly required) that I kept it even with the new approach.
I will think about your suggestion and the diff you sent me.
There was a problem hiding this comment.
The specific scenario isn't relevant under the structure of your suggested code changes. I merged all your suggestions as-is into this branch. I did leave some comments in jmoldow#1 , if you want to consider making any suggested improvements into this branch (you should have permissions) before you merge it.
| # max_reconnect_attempts cycles — spacing out retries on persistently-stuck | ||
| # locations avoids log spam, excess gRPC traffic, and repeated wasted refresh | ||
| # work that we already know is likely to fail again. | ||
| refresh_code_location(location_name) |
There was a problem hiding this comment.
I like this a lot more than the previous disconnect+reconnect thing - seems more clear what is going on
|
one thing that I am thinking through currently is whether things would get simpler if this 'when the location is in a transient error state, refresh it automatically preiodically' logic was in a totally separate periodic check within the watch thread, instead of being integrated into the existing disconnect/reconnect loop, which ends up doing similar things but is a bit different |
This reverts the change to _location_state_events_handler that triggers a refresh on any event (including LOCATION_RECONNECTED / LOCATION_DISCONNECTED) when the location is already in an error state. The watch-thread recovery already clears the stuck error: once GetServerId succeeds but the workspace entry is still errored, recovery is signaled via on_updated / on_reconnected, and the original handler already refreshes on LOCATION_UPDATED. The handler-side refresh on reconnect/disconnect is redundant and untested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The gRPC watch thread previously detected a location stuck in a transient unreachable-error state by re-raising DagsterUserCodeUnreachableError from inside the server-id poll, routing it through the on_disconnect/reconnect loop, and calling refresh_code_location from within reconnect_loop with rate-limiting tied to max_reconnect_attempts. Folding the load_error recovery into the reconnect machinery made both hard to follow. This decouples the two concerns. The server-id watch and reconnect loop go back to handling only server-id changes and actual server unreachability (firing on_updated / on_disconnect / on_reconnected / on_error). A new, independent check runs inside the same watch thread on a fixed cadence (ERROR_RECOVERY_INTERVAL, default 10s): if the location entry's load_error is a DagsterUserCodeUnreachableError, it calls refresh_code_location directly. It fires no on_* event - the refresh emits whatever state events are warranted - so recovery is fully separate from the reconnect path. Tests updated: the watch-thread recovery test now asserts the periodic check refreshes a stuck location and fires no events; the integration test asserts recovery happens with no disconnect/reconnect events. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@jmoldow i took a stab on top of this change with what I was imagining there here: jmoldow#1 - i think it accomplishes the same goal, but I find it a lot easier to wrap my head around what's going on there when it is separate from the existing disconnect/reconnect loop since its a bit different (even though both can result in a refresh/update). What do you think? |
…ry-redesign Decouple stuck-location recovery into a separate periodic check (proposal on top of dagster-io#33642) ## Summary & Motivation A proposal on top of your dagster-io#33642 (`jordan/server-watcher-error-recovery`), opened against your branch so the diff here is exactly the two commits I'm suggesting. Opening it for visibility/discussion — not necessarily to merge as-is; feel free to squash into dagster-io#33642, cherry-pick, or push back. Two changes: 1. **Revert the `_location_state_events_handler` change (recovery layer #1).** Restores the handler to refreshing only on `LOCATION_UPDATED` / `LOCATION_ERROR`. The watch-thread recovery already clears the stuck error (recovery is signaled via `on_updated` / `on_reconnected`, and the original handler already refreshes on `LOCATION_UPDATED`), so the handler-side refresh on reconnect/disconnect is redundant and currently untested — both recovery tests pass with it reverted. 2. **Move stuck-location recovery into a separate periodic check (recovery layer dagster-io#2).** Instead of weaving the `load_error` detection into the server-id poll and the reconnect loop (re-raising `DagsterUserCodeUnreachableError`, refreshing from inside `reconnect_loop`, rate-limiting via `max_reconnect_attempts`), the gRPC watch + reconnect loop goes back to handling only server-id changes and actual server unreachability. A new independent check runs inside the same watch thread on a fixed cadence (`ERROR_RECOVERY_INTERVAL`, default 10s): if the entry's `load_error` is a `DagsterUserCodeUnreachableError`, it calls `refresh_code_location` directly. It fires no `on_*` event, so recovery is fully decoupled from the reconnect path. The motivation is readability: the layer-dagster-io#2 integration interleaved two unrelated concerns (server-id watching vs. error recovery) in a way that was hard to follow. A separate fixed-interval check expresses "retry a stuck location every 10s" directly. ## How I Tested These Changes Updated the existing tests: - `test_watch_server.py` — the watch-thread recovery test now asserts the periodic check refreshes a stuck location and fires no events; the server-id watch/reconnect/error tests are unchanged. - `test_watch_grpc_server.py` — the integration recovery test asserts a stuck location recovers with no disconnect/reconnect events; the on_error/reconnect-path test is unchanged. All of `test_watch_server.py` and `test_watch_grpc_server.py` pass locally; `ruff` and `ty` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
I merged all of @gibsondan's suggestions as-is into this branch. I did leave some comments in jmoldow#1 , if you want to consider making any suggested improvements into this branch (you should have permissions) before you merge it. I'm fine with merging immediately, or making some/all/none of the suggested changes first. @greptileai Please fully re-review this PR. |
gibsondan
left a comment
gibsondan
left a comment
There was a problem hiding this comment.
i think this turned out great - thanks for pushing on it!
2 participants
Summary & Motivation
Fixes #17319 and #32216.
A code location can be left permanently stuck in an error state after a failed
refresh_code_location(). The gRPC server watcher detects a server change (new server ID, or server reachable again after a transient blip) and triggers a refresh, but the refresh itself fails — typically during a Kubernetes rolling deployment where the gRPC call routes to a dying pod via the EndpointSlice. After the failure, the watch thread keeps polling successfully, sees no further changes, and never retries. The location stays errored until something external (manual reload, daemon/webserver restart, or a newserver_idchange) forces a retry. The recent--fixed-server-idchange (commit e32df69) removed pod-restart as a reliable recovery path, making this a long-lived failure mode in some configurations. (Manually clicking Reload in the UI can also recover, but in multi-replica webserver deployments it has to land on the affected pod, so it may take several clicks.)Recovery is implemented as an independent periodic check inside the existing gRPC watch thread. On a fixed cadence (
ERROR_RECOVERY_INTERVAL, default 10s), the watch thread inspects the workspace entry'sload_error. If it is aDagsterUserCodeUnreachableError, it callsrefresh_code_locationdirectly. The check fires noon_*event. The server-id watch and the disconnect/reconnect loop continue to handle only server-id changes and actual server unreachability.Recovery is intentionally scoped to
DagsterUserCodeUnreachableError. Other load errors (e.g.DagsterUserCodeProcessErrorfrom a user-code syntax error) are non-transient and not retried, to avoid log spam on permanently broken locations. A related rolling-deploy race — where individual gRPC calls within a single_load_locationroute to different pods and produce a stale-but-not-erroredversion_key— is intentionally out of scope and tracked separately.Other changes:
create_grpc_watch_threadnow requires allon_*callbacks (no longer optional) and addsget_location_entryandrefresh_code_locationparameters.server_iddict is replaced with a typedServerIdinner class; full type annotations added throughoutserver_watcher.py.Identification and fixing of the issue was tool assisted, as was writing this commit message, but both the code and the commit message have been significantly reviewed and edited by me.
How I Tested These Changes
New unit tests in
dagster_tests/general_tests/grpc_tests/test_watch_server.pycovering the periodic recovery check (asserting it refreshes a stuck location and fires no events). New integration tests indagster_graphql_tests/graphql/test_watch_grpc_server.pyexercising the full recovery flow end-to-end against a real gRPC server. Existing test suite.Changelog
Fixed a bug where a code location could be left stuck in an error state after a transient gRPC failure during refresh (common during Kubernetes rolling deployments). The gRPC server watcher now periodically retries refresh for locations whose load error is a
DagsterUserCodeUnreachableError.