rebenitez1802 commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4403682415
##########
superset/utils/screenshots.py:
##########
@@ -191,32 +221,93 @@ def get_status(self) -> str:
return self.status.value
def get_invalid_image_reason(self) -> str | None:
- """Reason this payload's image should not be served/cached, or None if
- it passes validation (or it isn't claiming a successful screenshot)."""
- if self.status != StatusValues.UPDATED:
+ # Validate whenever an image is present, regardless of status: retained
+ # ERROR/COMPUTING entries still carry their previous image and are now
+ # served by the read paths, so a corrupt/blank retained image must be
+ # rejected (treated as a cache miss) too.
+ if self._image is None:
return None
return validate_screenshot_image(self._image)
+ def _age_seconds(self) -> float | None:
+ """Seconds since this entry's timestamp.
+
+ Timestamps are stored NAIVE (``datetime.now().isoformat()``), so the
age
+ is computed against a naive ``datetime.now()``. Keeping the naive
format
+ means old pods reading entries written by a newer pod (and vice versa)
+ during a rolling deploy never hit a naive-minus-aware ``TypeError``.
+
+ Corrupt or offset-aware timestamps return None so callers can recover.
+ Negative ages are preserved: clock/timezone skew must not expire error
+ backoff or computation leases on every request. Only the opt-in UPDATED
+ freshness check applies a bound to future timestamps.
+ """
+ try:
+ age = (
+ datetime.now() - datetime.fromisoformat(self.get_timestamp())
+ ).total_seconds()
+ except (ValueError, TypeError):
+ logger.warning(
+ "Unusable screenshot cache timestamp %r; "
+ "treating entry as expired/stale",
+ self.get_timestamp(),
+ )
+ return None
+ return age
+
def is_error_cache_ttl_expired(self) -> bool:
- error_cache_ttl = app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+ # strict '>' (an entry exactly at the TTL is still fresh). An unusable
+ # timestamp (age is None) is treated as expired so the entry
self-heals.
+ age_seconds = self._age_seconds()
return (
- datetime.now() - datetime.fromisoformat(self.get_timestamp())
- ).total_seconds() > error_cache_ttl
+ age_seconds is None or age_seconds >
app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+ )
def is_computing_stale(self) -> bool:
"""Check if a COMPUTING status is stale (task likely failed or
stuck)."""
return self.is_in_progress_stale()
def is_in_progress_stale(self, max_age_seconds: float | None = None) ->
bool:
"""Check if a pending or computing request has exceeded its lease."""
+ # '>=' (unlike the strict '>' of the ERROR/UPDATED helpers). An
unusable
+ # timestamp (age is None) is treated as stale so the entry self-heals.
computing_ttl = (
max_age_seconds
if max_age_seconds is not None
else app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]
)
- return (
- datetime.now() - datetime.fromisoformat(self.get_timestamp())
- ).total_seconds() >= computing_ttl
+ age_seconds = self._age_seconds()
+ return age_seconds is None or age_seconds >= computing_ttl
+
+ def is_updated_stale(self) -> bool:
+ """Return whether an UPDATED entry needs refreshing when freshness is
enabled.
+
+ An unset/zero TTL disables the check. Unusable timestamps and future
+ timestamps beyond the skew tolerance are stale; bounded negative ages
+ remain fresh so web and worker clocks can converge.
+ """
+ # `.get` (not `[]` like the sibling ERROR/COMPUTING helpers) on
purpose:
+ # a deployment whose config predates this key should silently disable
the
+ # feature, not raise KeyError. Checked first so a disabled feature
never
+ # parses/logs an unusable timestamp.
+ updated_ttl = app.config.get("THUMBNAIL_UPDATED_CACHE_TTL")
+ if not updated_ttl: # None or 0 => disabled
+ return False
+ # strict '>' (an image exactly at the TTL is still fresh), matching
+ # is_error_cache_ttl_expired -- not the '>=' of is_computing_stale. An
+ # unusable timestamp (age is None) is treated as stale so it
self-heals.
+ age_seconds = self._age_seconds()
+ if (
+ age_seconds is not None
+ and age_seconds < -_FUTURE_TIMESTAMP_TOLERANCE_SECONDS
Review Comment:
Good catch, fixed in e8921f25d64f93d7b6d2f5db476412427e6a4ad5. I went with
your first option. `is_updated_stale` treats a future `UPDATED` timestamp as
fresh, the same way ERROR backoff and COMPUTING leases already treat negative
ages. An entry written by a worker whose clock is ahead expires once its
web-clock age passes `THUMBNAIL_UPDATED_CACHE_TTL` (real age > TTL + skew).
`_FUTURE_TIMESTAMP_TOLERANCE_SECONDS` is gone, and unusable timestamps still
self-heal. I didn't use a bounded variant such as `-max(300, TTL)` because it
still loops once the skew exceeds the TTL.
A unit test pins the TTL + skew boundary. A new API test shows that a
force-less POST against an `UPDATED` generation stamped 1h ahead returns 200
without enqueuing.
This deliberately reverses the
[r3878463337](https://github.com/apache/superset/pull/43523#discussion_r3878463337)
self-heal for `UPDATED` entries. A stuck far-future value clears with
`force=true` or cache eviction, and it is never worse than master, which has no
freshness check. The opposite case, a worker running *behind* the web host by
more than the TTL, can't be fixed without a web-side clock. The cache docs and
UPDATING therefore say that web and worker hosts need the same `TZ` and a TTL
above any offset between them. The 7-day default covers any timezone gap.
##########
superset/charts/api.py:
##########
@@ -1213,9 +1212,21 @@ def build_response(status_code: int) -> WerkzeugResponse:
task_status=cache_payload.get_status(),
)
- if cache_payload.should_trigger_task(force,
expected_scope=f"chart:{chart.id}"):
+ if cache_payload.should_trigger_task(
+ force,
+ expected_scope=f"chart:{chart.id}",
+ check_updated_staleness=screenshot_obj.supports_updated_staleness,
Review Comment:
Confirmed: flipping that argument to `True` left the suite green, and the
chart worker then skips the enqueued task. In
392b62b73774f633a7f6307df4f23cc784c42918 I added
`test_cache_screenshot_does_not_recompute_stale_updated`. It seeds a valid
`chart:<id>` PNG entry aged 400s against a 300s `THUMBNAIL_UPDATED_CACHE_TTL`.
It then asserts that a force-less `cache_screenshot` returns 200, never calls
`delay`, and leaves the cached entry byte-identical. With the argument set to
`True`, it fails with a 202.
I also checked the other opt-in sites (dashboard on-demand, both card
thumbnail endpoints, and the class flags). Existing tests already catch
flipping any of them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
