Skip to content

Commit 9a0e965

Browse files
smackeseyclaude
authored andcommitted
fix: get_latest_materialization_event returns stale pre-wipe materialization (#25511)
## Summary & Motivation Fixes [#15806](#15806): `instance.get_latest_materialization_event` returned a stale, wiped materialization after `dagster asset wipe`, instead of `None` as documented. Also fixes the Dagster Plus analogue of the bug for partition wipes (reported by a cloud customer). Asset wipes are soft deletes: `wipe_asset` records a wipe timestamp on the `asset_keys` row, and readers treat the asset as live when `last_materialization_timestamp > wipe_timestamp`. The problem is that `ASSET_MATERIALIZATION_PLANNED` and `ASSET_OBSERVATION` events also bump `last_materialization_timestamp` (intentionally, so planned/observed assets show in the catalog) without updating the cached `last_materialization` record — which `wipe_asset` never cleared. So the moment any new run targeting a wiped asset started (or an observation was reported), the row passed the wipe filter and `_get_latest_materialization_records` returned the stale pre-wipe record. A post-wipe observation resurfaced the stale materialization indefinitely. This also affected `get_asset_records` (`AssetRecord.asset_entry.last_materialization_record`) and `get_latest_materialization_code_versions`, which share the same path. ### OSS fixes (`SqlEventLogStorage`) - **Write side:** `wipe_asset` now also nulls out `last_materialization`, so newly wiped rows are self-consistent. (This matches what the Plus event log storage has always done.) - **Read side:** `_get_latest_materialization_records` only trusts the cached record when its timestamp postdates the wipe; otherwise it falls back to the backcompat event-log query, which now has `_add_assets_wipe_filter_to_query` applied. This covers rows wiped by older Dagster versions where the stale blob is already in the database, and also fixes the same latent staleness for legacy old-format rows that always took the backcompat path. - **Legacy visibility path:** `_fetch_backcompat_materialization_times` (used by the pre-`ASSET_KEY_INDEX_COLS`-migration read path to decide whether a wiped asset is live) now only counts materialization/observation/planned events, mirroring what bumps `last_materialization_timestamp` on the migrated path. Previously it took `max(timestamp)` over *all* events, so the `ASSET_WIPED` event stored by `instance.wipe_assets` immediately after the wipe made the wiped asset appear live in `all_asset_keys`. This was latent until the write-side fix above stopped the stale blob from short-circuiting that path — it broke `TestAssetWipe::test_asset_wipe` in the postgres graphql CI jobs, whose fixture creates tables via alembic without marking the secondary index (postgres gates `last_materialization_timestamp` writes on that index, so those storages stay on the legacy path forever). ### Plus fix (`HostCloudEventLogStorage`) Full-asset wipes already clear `last_materialization`, but `wipe_asset_partitions` deleted the per-partition materialization rows while leaving the cached `last_materialization` on the asset row untouched. If the latest materialization belonged to a wiped partition, `get_latest_materialization_events` / `get_asset_records` kept returning it. The partition wipe now recomputes the cached record from the remaining materializations when it points at a wiped partition (falling back to `None` when none remain). The cached-value read locks the `asset_keys` row (`FOR UPDATE`) through the recompute, so a materialization committing mid-wipe either lands before the recompute reads (and is included) or after the wipe commits (and overwrites with the newer value) — the recomputed value can never clobber a newer concurrent materialization. The lock is acquired in the same order as the store path (`asset_keys` last), so it adds no deadlock risk. Note the adjacent caches (`last_observation`, `last_failed_to_materialize_event`, `last_skipped_materialize_event`) have the same staleness after partition wipes and are left for follow-up. ## Test Plan - Extended the shared-suite `test_asset_wipe`: after a wipe, a planned event and then an observation must not resurface the old materialization from `get_latest_materialization_events` or `get_asset_records`, and a re-materialization must surface the new event. Runs against sqlite, consolidated sqlite, in-memory, legacy, postgres, mysql, and the Plus storages (host cloud, archive-aware, ursula graphql). - Extended the shared-suite `test_asset_partitioned_wiped_event` (Plus storages only): wiping the latest-materialized partition falls back to the latest remaining materialization, wiping a non-latest partition leaves it untouched, and wiping the only remaining partition clears it. Verified to fail without the Plus fix. - New `test_get_latest_materialization_ignores_stale_cached_row_after_wipe` (sqlite) simulates a row wiped by an older version (stale cached record restored via direct SQL after the wipe) to exercise the read-side guard. Verified to fail without the read-side fix. - New `test_all_asset_keys_excludes_wiped_asset_on_legacy_index_path` (dagster-postgres) simulates the unmigrated-index condition from the graphql CI fixture and reproduces the exact `assert AssetKey(...) not in [AssetKey(...)]` failure from build 155619 without the legacy-path fix; passes with it. - Locally green: `dagster_tests/storage_tests/test_event_log.py` (519 passed), `dagster_postgres_tests/test_event_log.py` (142 passed), the previously-failing `TestAssetWipe` postgres graphql variants, cloud-backend `test_event_log_storage.py` + `test_archive_aware_storage.py` via tox (553 passed), and the ursula graphql storage wipe tests. ## Changelog Fixed a bug where `DagsterInstance.get_latest_materialization_event` (and `get_asset_records`) could return a stale pre-wipe materialization for a wiped asset once a new run targeting the asset started or an observation was reported. In Dagster+, fixed the analogous bug where wiping partitions could leave the wiped partition's materialization as the asset's latest materialization. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Internal-RevId: d05abfe7d759123142e63f4d191113e3aab529f2
1 parent 8c85005 commit 9a0e965

4 files changed

Lines changed: 248 additions & 24 deletions

File tree

python_modules/dagster/dagster/_core/storage/event_log/sql_event_log.py

Lines changed: 50 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1164,39 +1164,54 @@ def _get_latest_materialization_records(
11641164
# Given a list of raw asset rows, returns a mapping of asset key to latest asset materialization
11651165
# event log entry. Fetches backcompat EventLogEntry records when the last_materialization
11661166
# in the raw asset row is an AssetMaterialization.
1167-
to_backcompat_fetch = set()
1167+
to_backcompat_fetch: list[AssetKey] = []
1168+
asset_details_by_key: dict[AssetKey, AssetDetails | None] = {}
11681169
results: dict[AssetKey, EventLogRecord | None] = {}
11691170
for row in raw_asset_rows:
11701171
asset_key = AssetKey.from_db_string(row["asset_key"])
11711172
if not asset_key:
11721173
continue
1174+
asset_details = AssetDetails.from_db_string(row["asset_details"])
1175+
last_wipe_timestamp = asset_details.last_wipe_timestamp if asset_details else None
11731176
event_or_materialization = (
11741177
deserialize_value(row["last_materialization"], NamedTuple) # ty: ignore[no-matching-overload]
11751178
if row["last_materialization"]
11761179
else None
11771180
)
1178-
if isinstance(event_or_materialization, EventLogRecord):
1181+
if isinstance(event_or_materialization, EventLogRecord) and (
1182+
last_wipe_timestamp is None
1183+
or event_or_materialization.event_log_entry.timestamp > last_wipe_timestamp
1184+
):
11791185
results[asset_key] = event_or_materialization
11801186
else:
1181-
to_backcompat_fetch.add(asset_key)
1182-
1183-
latest_event_subquery = db_subquery(
1184-
db_select(
1185-
[
1186-
SqlEventLogStorageTable.c.asset_key,
1187-
db.func.max(SqlEventLogStorageTable.c.id).label("id"),
1188-
]
1189-
)
1190-
.where(
1191-
db.and_(
1192-
SqlEventLogStorageTable.c.asset_key.in_(
1193-
[asset_key.to_string() for asset_key in to_backcompat_fetch]
1194-
),
1195-
SqlEventLogStorageTable.c.dagster_event_type
1196-
== DagsterEventType.ASSET_MATERIALIZATION.value,
1197-
)
1187+
# No stored record, an old-format record, or a record predating the latest wipe
1188+
# (a planned or observation event after a wipe makes the asset row visible while
1189+
# the stale pre-wipe materialization is still stored on it). Fall back to querying
1190+
# the event log table, filtering out pre-wipe events.
1191+
to_backcompat_fetch.append(asset_key)
1192+
asset_details_by_key[asset_key] = asset_details
1193+
1194+
backcompat_subquery = db_select(
1195+
[
1196+
SqlEventLogStorageTable.c.asset_key,
1197+
db.func.max(SqlEventLogStorageTable.c.id).label("id"),
1198+
]
1199+
).where(
1200+
db.and_(
1201+
SqlEventLogStorageTable.c.asset_key.in_(
1202+
[asset_key.to_string() for asset_key in to_backcompat_fetch]
1203+
),
1204+
SqlEventLogStorageTable.c.dagster_event_type
1205+
== DagsterEventType.ASSET_MATERIALIZATION.value,
11981206
)
1199-
.group_by(SqlEventLogStorageTable.c.asset_key),
1207+
)
1208+
backcompat_subquery = self._add_assets_wipe_filter_to_query(
1209+
backcompat_subquery,
1210+
[asset_details_by_key[asset_key] for asset_key in to_backcompat_fetch],
1211+
to_backcompat_fetch,
1212+
)
1213+
latest_event_subquery = db_subquery(
1214+
backcompat_subquery.group_by(SqlEventLogStorageTable.c.asset_key),
12001215
"latest_event_subquery",
12011216
)
12021217
backcompat_query = db_select(
@@ -1527,7 +1542,9 @@ def _fetch_backcompat_materialization_times(
15271542
self, asset_keys: Sequence[AssetKey]
15281543
) -> Mapping[AssetKey, datetime]:
15291544
# fetches the latest materialization timestamp for the given asset_keys. Uses the (slower)
1530-
# raw event log table.
1545+
# raw event log table. Restricted to the event types that update
1546+
# last_materialization_timestamp in `_get_asset_entry_values`, so that events like
1547+
# ASSET_WIPED (stored immediately after a wipe) do not make a wiped asset appear live.
15311548
backcompat_query = (
15321549
db_select(
15331550
[
@@ -1536,8 +1553,17 @@ def _fetch_backcompat_materialization_times(
15361553
]
15371554
)
15381555
.where(
1539-
SqlEventLogStorageTable.c.asset_key.in_(
1540-
[asset_key.to_string() for asset_key in asset_keys]
1556+
db.and_(
1557+
SqlEventLogStorageTable.c.asset_key.in_(
1558+
[asset_key.to_string() for asset_key in asset_keys]
1559+
),
1560+
SqlEventLogStorageTable.c.dagster_event_type.in_(
1561+
[
1562+
DagsterEventType.ASSET_MATERIALIZATION.value,
1563+
DagsterEventType.ASSET_OBSERVATION.value,
1564+
DagsterEventType.ASSET_MATERIALIZATION_PLANNED.value,
1565+
]
1566+
),
15411567
)
15421568
)
15431569
.group_by(SqlEventLogStorageTable.c.asset_key)
@@ -1758,6 +1784,7 @@ def _get_asset_key_values_on_wipe(self) -> Mapping[str, Any]:
17581784
wipe_timestamp = get_current_timestamp()
17591785
values: dict[str, Any] = {
17601786
"asset_details": serialize_value(AssetDetails(last_wipe_timestamp=wipe_timestamp)),
1787+
"last_materialization": None,
17611788
"last_run_id": None,
17621789
}
17631790
if self.has_asset_key_index_cols():

python_modules/dagster/dagster_tests/storage_tests/test_event_log.py

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,22 @@
1212
import sqlalchemy as db
1313
import sqlalchemy.exc
1414
from dagster import DagsterInstance
15-
from dagster._core.events import EngineEventData, SerializableErrorInfo, StepRetryData
15+
from dagster._core.events import (
16+
AssetMaterializationPlannedData,
17+
DagsterEvent,
18+
DagsterEventType,
19+
EngineEventData,
20+
SerializableErrorInfo,
21+
StepRetryData,
22+
)
1623
from dagster._core.execution.stats import (
1724
StepEventStatus,
1825
build_run_stats_from_events,
1926
build_run_step_stats_from_events,
2027
build_run_step_stats_snapshot_from_events,
2128
)
2229
from dagster._core.storage.event_log import (
30+
AssetKeyTable,
2331
ConsolidatedSqliteEventLogStorage,
2432
SqlEventLogStorageMetadata,
2533
SqlEventLogStorageTable,
@@ -31,13 +39,15 @@
3139
from dagster._core.storage.sqlalchemy_compat import db_select
3240
from dagster._core.storage.sqlite_storage import DagsterSqliteStorage
3341
from dagster._core.utils import make_new_run_id
42+
from dagster._serdes import serialize_value
3443
from dagster._utils.test import ConcurrencyEnabledSqliteTestEventLogStorage
3544
from sqlalchemy import __version__ as sqlalchemy_version
3645
from sqlalchemy.engine import Connection
3746

3847
from dagster_tests.storage_tests.utils.event_log_storage import (
3948
TestEventLogStorage,
4049
_synthesize_events,
50+
one_asset_op,
4151
)
4252

4353

@@ -218,6 +228,54 @@ def test_get_latest_tags_by_partition(self, storage, instance, dagster_event_typ
218228
pytest.skip("skip this since legacy storage is harder to mock.patch")
219229

220230

231+
def test_get_latest_materialization_ignores_stale_cached_row_after_wipe():
232+
# Regression test for https://github.com/dagster-io/dagster/issues/15806. Rows wiped by older
233+
# versions of wipe_asset retain the pre-wipe materialization in the `last_materialization`
234+
# column. A planned event stored after the wipe bumps the row past the wipe filter; ensure
235+
# the stale cached materialization is still not returned.
236+
with tempfile.TemporaryDirectory() as tmpdir_path:
237+
with dg.instance_for_test(temp_dir=tmpdir_path) as instance:
238+
storage = instance.event_log_storage
239+
assert isinstance(storage, SqliteEventLogStorage)
240+
asset_key = dg.AssetKey("asset_1")
241+
242+
_synthesize_events(one_asset_op, run_id=make_new_run_id(), instance=instance)
243+
record = storage.get_asset_records([asset_key])[
244+
0
245+
].asset_entry.last_materialization_record
246+
assert record is not None
247+
248+
storage.wipe_asset(asset_key)
249+
250+
# simulate a legacy wiped row by restoring the stale cached materialization
251+
with storage.index_connection() as conn:
252+
conn.execute(
253+
AssetKeyTable.update()
254+
.values(last_materialization=serialize_value(record))
255+
.where(AssetKeyTable.c.asset_key == asset_key.to_string())
256+
)
257+
258+
storage.store_event(
259+
dg.EventLogEntry(
260+
error_info=None,
261+
level="debug",
262+
user_message="",
263+
run_id=make_new_run_id(),
264+
timestamp=time.time(),
265+
dagster_event=DagsterEvent(
266+
DagsterEventType.ASSET_MATERIALIZATION_PLANNED.value,
267+
"nonce",
268+
event_specific_data=AssetMaterializationPlannedData(asset_key),
269+
),
270+
)
271+
)
272+
273+
assert storage.get_latest_materialization_events([asset_key]).get(asset_key) is None
274+
asset_records = storage.get_asset_records([asset_key])
275+
assert len(asset_records) == 1
276+
assert asset_records[0].asset_entry.last_materialization_record is None
277+
278+
221279
def _insert_slots(conn: Connection, concurrency_key: str, num: int, delete_num: int = 0):
222280
rows = [
223281
{

python_modules/dagster/dagster_tests/storage_tests/utils/event_log_storage.py

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2923,6 +2923,61 @@ def test_asset_wipe(self, storage, instance, test_run_id):
29232923
assert len(asset_keys) == 1
29242924
assert storage.has_asset_key(dg.AssetKey("asset_1"))
29252925

2926+
# Regression test for https://github.com/dagster-io/dagster/issues/15806 - a planned or
2927+
# observation event stored after a wipe makes the asset row visible again, but must not
2928+
# resurface the stale pre-wipe materialization.
2929+
asset_key = dg.AssetKey("asset_1")
2930+
assert storage.get_latest_materialization_events([asset_key])[asset_key] is not None
2931+
2932+
storage.wipe_asset(asset_key)
2933+
assert storage.get_latest_materialization_events([asset_key]).get(asset_key) is None
2934+
2935+
planned_run_id = make_new_run_id()
2936+
observation_run_id = make_new_run_id()
2937+
with create_and_delete_test_runs(instance, [planned_run_id, observation_run_id]):
2938+
storage.store_event(
2939+
dg.EventLogEntry(
2940+
error_info=None,
2941+
level="debug",
2942+
user_message="",
2943+
run_id=planned_run_id,
2944+
timestamp=time.time(),
2945+
dagster_event=DagsterEvent(
2946+
DagsterEventType.ASSET_MATERIALIZATION_PLANNED.value,
2947+
"nonce",
2948+
event_specific_data=AssetMaterializationPlannedData(asset_key),
2949+
),
2950+
)
2951+
)
2952+
assert storage.get_latest_materialization_events([asset_key]).get(asset_key) is None
2953+
asset_records = storage.get_asset_records([asset_key])
2954+
assert len(asset_records) == 1
2955+
assert asset_records[0].asset_entry.last_materialization_record is None
2956+
2957+
storage.store_event(
2958+
dg.EventLogEntry(
2959+
error_info=None,
2960+
level="debug",
2961+
user_message="",
2962+
run_id=observation_run_id,
2963+
timestamp=time.time(),
2964+
dagster_event=DagsterEvent(
2965+
DagsterEventType.ASSET_OBSERVATION.value,
2966+
"nonce",
2967+
event_specific_data=AssetObservationData(
2968+
dg.AssetObservation(asset_key=asset_key)
2969+
),
2970+
),
2971+
)
2972+
)
2973+
assert storage.get_latest_materialization_events([asset_key]).get(asset_key) is None
2974+
2975+
rematerialization_run_id = make_new_run_id()
2976+
_synthesize_events(one_asset_op, run_id=rematerialization_run_id, instance=instance)
2977+
latest = storage.get_latest_materialization_events([asset_key]).get(asset_key)
2978+
assert latest is not None
2979+
assert latest.run_id == rematerialization_run_id
2980+
29262981
def test_asset_wiped_event(self, instance):
29272982
@dg.asset
29282983
def asset_to_wipe():
@@ -2969,6 +3024,28 @@ def asset_to_wipe():
29693024
assert wipe_events[0].event_log_entry.dagster_event_type == DagsterEventType.ASSET_WIPED
29703025
assert wipe_events[0].event_log_entry.dagster_event.asset_wiped_data.partition_keys == ["a"]
29713026

3027+
# the latest materialization pointed at the wiped partition and must not be returned
3028+
assert instance.get_latest_materialization_event(asset_to_wipe.key) is None
3029+
3030+
materialize([asset_to_wipe], instance=instance, partition_key="b")
3031+
materialize([asset_to_wipe], instance=instance, partition_key="c")
3032+
3033+
# wiping the latest-materialized partition falls back to the latest remaining one
3034+
instance.wipe_asset_partitions(asset_to_wipe.key, ["c"])
3035+
latest = instance.get_latest_materialization_event(asset_to_wipe.key)
3036+
assert latest is not None
3037+
assert latest.dagster_event.partition == "b"
3038+
3039+
# wiping a partition that does not hold the latest materialization leaves it untouched
3040+
instance.wipe_asset_partitions(asset_to_wipe.key, ["a"])
3041+
latest = instance.get_latest_materialization_event(asset_to_wipe.key)
3042+
assert latest is not None
3043+
assert latest.dagster_event.partition == "b"
3044+
3045+
# wiping the only remaining materialized partition clears it
3046+
instance.wipe_asset_partitions(asset_to_wipe.key, ["b"])
3047+
assert instance.get_latest_materialization_event(asset_to_wipe.key) is None
3048+
29723049
def test_asset_secondary_index(self, storage, instance):
29733050
_synthesize_events(one_asset_op, instance=instance)
29743051

python_modules/libraries/dagster-postgres/dagster_postgres_tests/test_event_log.py

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,17 @@
44

55
import objgraph
66
import pytest
7+
from dagster import AssetKey, AssetMaterialization, EventLogEntry
8+
from dagster._core.events import (
9+
AssetWipedData,
10+
DagsterEvent,
11+
DagsterEventType,
12+
StepMaterializationData,
13+
)
14+
from dagster._core.instance import RUNLESS_JOB_NAME, RUNLESS_RUN_ID
715
from dagster._core.storage.event_log.base import EventLogCursor
16+
from dagster._core.storage.event_log.migration import ASSET_KEY_INDEX_COLS
17+
from dagster._core.storage.event_log.schema import SecondaryIndexMigrationTable
818
from dagster._core.test_utils import ensure_dagster_tests_import, instance_for_test
919
from dagster._core.utils import make_new_run_id
1020
from dagster_postgres.event_log import PostgresEventLogStorage
@@ -140,3 +150,55 @@ def test_load_from_config(self, hostname):
140150
from_explicit = explicit_instance._event_storage # noqa: SLF001
141151

142152
assert from_url.postgres_url == from_explicit.postgres_url # ty: ignore[unresolved-attribute]
153+
154+
155+
def test_all_asset_keys_excludes_wiped_asset_on_legacy_index_path(conn_string):
156+
# Regression test for the legacy (pre-ASSET_KEY_INDEX_COLS-migration) read path, where wiped
157+
# assets are filtered by comparing the wipe timestamp against the latest event log timestamp.
158+
# The ASSET_WIPED event stored immediately after a wipe (and any other event type that does
159+
# not update last_materialization_timestamp on the migrated path) must not make the wiped
160+
# asset appear live.
161+
asset_key = AssetKey(["asset_one"])
162+
with _clean_storage(conn_string) as storage:
163+
# simulate a storage that has not run the ASSET_KEY_INDEX_COLS data migration
164+
with storage.index_connection() as conn:
165+
conn.execute(SecondaryIndexMigrationTable.delete())
166+
storage._secondary_index_cache.clear() # noqa: SLF001
167+
assert not storage.has_secondary_index(ASSET_KEY_INDEX_COLS)
168+
169+
storage.store_event(
170+
EventLogEntry(
171+
error_info=None,
172+
level="debug",
173+
user_message="",
174+
run_id=make_new_run_id(),
175+
timestamp=time.time(),
176+
dagster_event=DagsterEvent(
177+
DagsterEventType.ASSET_MATERIALIZATION.value,
178+
"nonce",
179+
event_specific_data=StepMaterializationData(
180+
AssetMaterialization(asset_key=asset_key)
181+
),
182+
),
183+
)
184+
)
185+
assert asset_key in storage.all_asset_keys()
186+
187+
storage.wipe_asset(asset_key)
188+
storage.store_event(
189+
EventLogEntry(
190+
error_info=None,
191+
level="debug",
192+
user_message="",
193+
run_id=RUNLESS_RUN_ID,
194+
timestamp=time.time(),
195+
dagster_event=DagsterEvent(
196+
event_type_value=DagsterEventType.ASSET_WIPED.value,
197+
job_name=RUNLESS_JOB_NAME,
198+
event_specific_data=AssetWipedData(asset_key=asset_key, partition_keys=None),
199+
),
200+
)
201+
)
202+
203+
assert asset_key not in storage.all_asset_keys()
204+
assert storage.get_latest_materialization_events([asset_key]).get(asset_key) is None

0 commit comments

Comments
 (0)