mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-18 04:49:13 +00:00
fix(core): unique snapshot temp name, honour on-demand requests, skip empty starlark
The preview snapshot wrote through a fixed "<snapshot>.tmp". /tmp is world-writable and sticky, and the display service runs as a different user from the tooling, so a leftover temp owned by anyone else became unopenable even by root -- fs.protected_regular refuses O_CREAT on a foreign file in a sticky directory. The preview and the health check's liveness proxy then froze until someone deleted the file by hand; on the test rig that meant 23 hours of a healthy display reporting "hardware: stale". Now uses tempfile.mkstemp with cleanup on failure, matching the hardware-status write a few hundred lines above. Closes #528. _poll_on_demand_requests read its mailbox with max_age=3600, and get() defaults the in-memory TTL to max_age -- so the first request was pinned in memory for an hour and every later poll returned that stale copy. No second on-demand request was honoured until the service restarted, while the API kept returning 200. get() already documents memory_ttl=0 for exactly this cross-process case. The consumed request is also now deleted: leaving it on disk meant a restart replayed the previous request, activated it, and ignored the one the caller had just made. Closes #530. starlark-apps returned None from display() when it has no app to show, which is the state of every install without Pixlet and of a fresh one before any app is added. The controller only skips on a boolean False, so that held a black panel for the full display_duration instead of rotating on. Closes #456 (core side). Verified on the rig: two consecutive on-demand requests with no restart between them are both activated, where the second was previously dropped in silence.
This commit is contained in:
@@ -681,12 +681,18 @@ class StarlarkAppsPlugin(BasePlugin):
|
|||||||
if app.is_enabled() and app.should_render(current_time):
|
if app.is_enabled() and app.should_render(current_time):
|
||||||
self._render_app(app, force=False)
|
self._render_app(app, force=False)
|
||||||
|
|
||||||
def display(self, force_clear: bool = False) -> None:
|
def display(self, force_clear: bool = False) -> bool:
|
||||||
"""
|
"""
|
||||||
Display current Starlark app.
|
Display current Starlark app.
|
||||||
|
|
||||||
This method is called during the display rotation.
|
This method is called during the display rotation.
|
||||||
Displays frames from the currently active app.
|
Displays frames from the currently active app.
|
||||||
|
|
||||||
|
Returns False when there is no app to show -- which is the state of
|
||||||
|
every install without Pixlet, and of a fresh one before any app is
|
||||||
|
added. The display controller only skips a mode on a boolean False
|
||||||
|
(it checks isinstance(result, bool)), so returning None held a black
|
||||||
|
panel for the full display_duration instead of rotating on.
|
||||||
"""
|
"""
|
||||||
try:
|
try:
|
||||||
if force_clear:
|
if force_clear:
|
||||||
@@ -699,20 +705,22 @@ class StarlarkAppsPlugin(BasePlugin):
|
|||||||
if not self.current_app:
|
if not self.current_app:
|
||||||
# No apps available
|
# No apps available
|
||||||
self.logger.debug("No Starlark apps to display")
|
self.logger.debug("No Starlark apps to display")
|
||||||
return
|
return False
|
||||||
|
|
||||||
# Render app if needed
|
# Render app if needed
|
||||||
if not self.current_app.frames:
|
if not self.current_app.frames:
|
||||||
success = self._render_app(self.current_app, force=True)
|
success = self._render_app(self.current_app, force=True)
|
||||||
if not success:
|
if not success:
|
||||||
self.logger.error(f"Failed to render app: {self.current_app.app_id}")
|
self.logger.error(f"Failed to render app: {self.current_app.app_id}")
|
||||||
return
|
return False
|
||||||
|
|
||||||
# Display current frame
|
# Display current frame
|
||||||
self._display_frame()
|
self._display_frame()
|
||||||
|
return True
|
||||||
|
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
self.logger.error(f"Error displaying Starlark app: {e}")
|
self.logger.error(f"Error displaying Starlark app: {e}")
|
||||||
|
return False
|
||||||
|
|
||||||
def _select_next_app(self) -> None:
|
def _select_next_app(self) -> None:
|
||||||
"""Select the next enabled app for display."""
|
"""Select the next enabled app for display."""
|
||||||
|
|||||||
@@ -1271,8 +1271,16 @@ class DisplayController:
|
|||||||
"""Poll cache for new on-demand requests from external controllers."""
|
"""Poll cache for new on-demand requests from external controllers."""
|
||||||
try:
|
try:
|
||||||
# Use a long max_age (1 hour) to ensure requests aren't expired before processing
|
# Use a long max_age (1 hour) to ensure requests aren't expired before processing
|
||||||
# The request_id check prevents duplicate processing
|
# The request_id check prevents duplicate processing.
|
||||||
request = self.cache_manager.get('display_on_demand_request', max_age=3600)
|
#
|
||||||
|
# memory_ttl=0 is required, not optional: this key is a mailbox the
|
||||||
|
# web process writes and this process reads. get() defaults the
|
||||||
|
# in-memory TTL to max_age, so without it the first request read was
|
||||||
|
# pinned in memory for the full hour and every later poll returned
|
||||||
|
# that stale copy -- meaning no second on-demand request was honoured
|
||||||
|
# for an hour, while the API still reported success.
|
||||||
|
request = self.cache_manager.get('display_on_demand_request',
|
||||||
|
max_age=3600, memory_ttl=0)
|
||||||
except (OSError, RuntimeError, ValueError, TypeError) as err:
|
except (OSError, RuntimeError, ValueError, TypeError) as err:
|
||||||
logger.error("Failed to read on-demand request: %s", err, exc_info=True)
|
logger.error("Failed to read on-demand request: %s", err, exc_info=True)
|
||||||
return
|
return
|
||||||
@@ -1318,6 +1326,15 @@ class DisplayController:
|
|||||||
# Mark as processed BEFORE processing (to prevent duplicate processing)
|
# Mark as processed BEFORE processing (to prevent duplicate processing)
|
||||||
self.cache_manager.set('display_on_demand_processed_id', request_id, ttl=3600)
|
self.cache_manager.set('display_on_demand_processed_id', request_id, ttl=3600)
|
||||||
self.on_demand_request_id = request_id
|
self.on_demand_request_id = request_id
|
||||||
|
# Consume the mailbox entry. Leaving it on disk meant a restart replayed
|
||||||
|
# the previous request: the fresh controller read it, activated it and
|
||||||
|
# cached it, so the request the caller had just made was ignored and the
|
||||||
|
# panel silently showed the earlier plugin. processed_id still guards
|
||||||
|
# against double-processing if this delete fails.
|
||||||
|
try:
|
||||||
|
self.cache_manager.delete('display_on_demand_request')
|
||||||
|
except Exception as err: # pragma: no cover - best-effort cleanup
|
||||||
|
logger.debug("Could not clear the on-demand request mailbox: %s", err)
|
||||||
|
|
||||||
if action == 'start':
|
if action == 'start':
|
||||||
logger.info("Processing on-demand start request for plugin: %s", request.get('plugin_id'))
|
logger.info("Processing on-demand start request for plugin: %s", request.get('plugin_id'))
|
||||||
|
|||||||
+21
-3
@@ -1453,12 +1453,30 @@ class DisplayManager:
|
|||||||
if parent_dir and str(parent_dir) != '/tmp': # nosec B108 - guard to skip /tmp for permission ops
|
if parent_dir and str(parent_dir) != '/tmp': # nosec B108 - guard to skip /tmp for permission ops
|
||||||
ensure_directory_permissions(parent_dir, get_assets_dir_mode())
|
ensure_directory_permissions(parent_dir, get_assets_dir_mode())
|
||||||
self._snapshot_dir_prepared = True
|
self._snapshot_dir_prepared = True
|
||||||
# Write atomically: temp then replace
|
# Write atomically: temp then replace. The temp name must be
|
||||||
tmp_path = f"{self._snapshot_path}.tmp"
|
# unique, not "<snapshot>.tmp": /tmp is world-writable and sticky,
|
||||||
self.image.save(tmp_path, format='PNG')
|
# and this file is written by whichever user the display service
|
||||||
|
# runs as while tests and tooling run as someone else. A leftover
|
||||||
|
# fixed-name temp owned by another user is then unopenable even by
|
||||||
|
# root (fs.protected_regular refuses O_CREAT on a foreign file in a
|
||||||
|
# sticky dir), which froze the preview and the health check's
|
||||||
|
# liveness proxy until somebody deleted it by hand. Same pattern as
|
||||||
|
# the hardware-status write above.
|
||||||
|
_fd, tmp_path = tempfile.mkstemp(
|
||||||
|
dir=str(snapshot_path_obj.parent),
|
||||||
|
prefix=f".{snapshot_path_obj.name}.", suffix=".tmp")
|
||||||
try:
|
try:
|
||||||
|
with os.fdopen(_fd, "wb") as _f:
|
||||||
|
self.image.save(_f, format='PNG')
|
||||||
|
os.chmod(tmp_path, 0o644)
|
||||||
os.replace(tmp_path, self._snapshot_path)
|
os.replace(tmp_path, self._snapshot_path)
|
||||||
except Exception:
|
except Exception:
|
||||||
|
# Never leave the temp behind -- that is what made the failure
|
||||||
|
# permanent rather than transient.
|
||||||
|
try:
|
||||||
|
os.unlink(tmp_path)
|
||||||
|
except OSError:
|
||||||
|
pass
|
||||||
# Fallback to direct save if replace not supported
|
# Fallback to direct save if replace not supported
|
||||||
self.image.save(self._snapshot_path, format='PNG')
|
self.image.save(self._snapshot_path, format='PNG')
|
||||||
# Set proper file permissions after saving
|
# Set proper file permissions after saving
|
||||||
|
|||||||
Reference in New Issue
Block a user