Commit 6388276
Order RUN_FAILURE_REASON_TAG write before run status update (#24730)
## Summary & Motivation
In `SqlRunStorage.handle_run_event`, the PIPELINE_FAILURE handler used
to write the run status update and the `dagster/failure_reason` tag in
**two separate transactions**:
1. `with self.connect() as conn:` — UPDATE `runs.status = FAILURE`
(commits on exit).
2. `self.add_run_tags(run_id, {RUN_FAILURE_REASON_TAG: ...})` — opens
its own connection.
A caller polling for `run.is_finished` could observe `status=FAILURE`
between those two writes and then read `run.tags` without the tag —
`KeyError: 'dagster/failure_reason'`.
This is the source of the flake in
`test_retry_failure_all_steps_with_reexecution_params` ([example
failure](https://buildkite.com/dagster/internal/builds/152044#019e4b21-52ee-4964-807d-47d28eed96a5)):
the test calls `wait_for_runs_to_finish` (status-only) then reads the
tag immediately.
## Fix
- Compute the `failure_reason` tag value upfront and include it in the
`run_body` serialized into the runs UPDATE.
- Inside the same `with self.connect() as conn:` block, write the
`RunTagsTable` row **first**, then the runs UPDATE.
- On `SqliteRunStorage`, `connect()` wraps in `conn.begin()` so the two
writes commit as a unit.
- On `PostgresRunStorage`, the engine is configured with
`isolation_level="AUTOCOMMIT"`, so each `conn.execute` commits
independently. Ordering the tag write first ensures that by the time a
reader observes the new status, the tag row is already there.
- `update_timestamp` is written exactly once on the runs row (using the
event timestamp), avoiding the regression that calling `add_run_tags`
separately would introduce (where `runs.update_timestamp` could be
written as `now()` and then overwritten with an earlier event timestamp,
breaking `RunsFilter(updated_after=...)` cursors).
- Use the in-memory `run.tags` to choose UPDATE vs INSERT against
`RunTagsTable`, mirroring `add_run_tags` — no extra SELECT round-trip.
## Test Plan
- [x] `tox -e py312-storage_tests -- -k test_failure_event_updates_tags`
— all 3 variants (Sqlite, InMemory, Legacy) pass.
- [x] Full `dagster_tests/storage_tests/test_run_storage.py` suite —
1007 passed, 105 pre-existing skips.
- [ ] CI green on `dagster-graphql` suites that previously surfaced the
flake.
## Changelog
Eliminate a race where readers polling for a run to reach FAILURE could
miss the `dagster/failure_reason` tag.
Internal-RevId: 2cb1d47fe55e3b99a82dbbd2ed244046f1dfbc361 parent 40f9eb2 commit 6388276
1 file changed
Lines changed: 40 additions & 9 deletions
Lines changed: 40 additions & 9 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
188 | 188 | | |
189 | 189 | | |
190 | 190 | | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
191 | 205 | | |
192 | 206 | | |
193 | 207 | | |
| |||
208 | 222 | | |
209 | 223 | | |
210 | 224 | | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
211 | 250 | | |
212 | 251 | | |
213 | 252 | | |
214 | 253 | | |
215 | | - | |
| 254 | + | |
216 | 255 | | |
217 | 256 | | |
218 | 257 | | |
219 | 258 | | |
220 | 259 | | |
221 | 260 | | |
222 | | - | |
223 | | - | |
224 | | - | |
225 | | - | |
226 | | - | |
227 | | - | |
228 | | - | |
229 | | - | |
230 | 261 | | |
231 | 262 | | |
232 | 263 | | |
| |||
0 commit comments