codeant-ai-for-open-source[bot] commented on code in PR #40084:
URL: https://github.com/apache/superset/pull/40084#discussion_r3530974559
##########
superset/extensions/local_extensions_watcher.py:
##########
@@ -29,37 +29,201 @@
logger = logging.getLogger(__name__)
+# Sentinel file Flask watches via --extra-files. Touching it on a real change
+# triggers a server reload without depending on cwd or the location of any
+# Python source file.
+RELOAD_TRIGGER: Path = Path(__file__).resolve().parent / ".reload_trigger"
+
# Guard to prevent multiple initializations
_watcher_initialized = False
_watcher_lock = threading.Lock()
-def _get_file_handler_class() -> Any:
+def _get_file_handler_class() -> Any: # noqa: C901
"""Get the file handler class, importing watchdog only when needed."""
try:
- from watchdog.events import FileSystemEventHandler
+ import hashlib
+
+ from watchdog.events import (
+ FileCreatedEvent,
+ FileDeletedEvent,
+ FileModifiedEvent,
+ FileMovedEvent,
+ FileSystemEventHandler,
+ )
class LocalExtensionFileHandler(FileSystemEventHandler):
- """Custom file system event handler for LOCAL_EXTENSIONS
directories."""
+ """Custom file system event handler for LOCAL_EXTENSIONS
directories.
+
+ Only reacts to genuine content changes (create / modify / move) in
the
+ dist directory, verified by comparing a SHA-256 of the file's
content.
+ This avoids the Docker VirtioFS / osxfs problem where reading a
file
+ generates inotify events that watchdog surfaces as modifications.
+ """
+
+ def __init__(self) -> None:
+ super().__init__()
+ # sha256 of last-seen content, keyed by absolute path.
Populated
+ # from existing files in watched `dist` dirs at startup (see
+ # `prime_baseline`) so that startup-noise inotify events from
+ # Docker VirtioFS reads don't get treated as the first real
edit.
+ self._file_hashes: dict[str, str] = {}
+ self._lock = threading.Lock()
+ # Trailing debounce: schedule a single reload after a quiet
+ # window so simultaneous webpack writes coalesce into one
+ # restart that fires *after* the build settles.
+ self._debounce_seconds: float = 1.0
+ self._pending_timer: threading.Timer | None = None
+ # Monotonically increasing token identifying the most recently
+ # scheduled timer. Guards the timer-already-fired race where
+ # `Timer.cancel()` can't stop a callback that has begun
running.
+ self._reload_generation: int = 0
+
+ # ── helpers ──────────────────────────────────────────────────────
+
+ @staticmethod
+ def _sha256(path: str) -> str | None:
+ try:
+ with open(path, "rb") as fh:
+ return hashlib.sha256(fh.read()).hexdigest()
+ except OSError:
+ return None
+
+ def prime_baseline(self, watch_dirs: set[str]) -> None:
+ """Pre-populate content hashes for existing files in watched
+ `dist` directories. Called once at watcher startup so a
+ developer's first real edit registers as a content change
+ rather than as the file's 'first observation'."""
+ for root_dir in watch_dirs:
+ root = Path(root_dir)
+ for path in root.rglob("*"):
+ if not path.is_file():
+ continue
+ if "dist" not in path.parts:
+ continue
+ digest = self._sha256(str(path))
+ if digest is not None:
+ self._file_hashes[str(path)] = digest
+
+ def _content_changed(self, path: str) -> bool:
+ """Return True when the file's content differs from last seen.
+
+ With `prime_baseline` called at startup, the baseline reflects
+ what was on disk when the watcher started. A first observation
+ that differs (or doesn't exist in baseline) is treated as a
+ genuine change.
+ """
+ digest = self._sha256(path)
+ if digest is None:
+ return False
+ old_digest = self._file_hashes.get(path)
+ self._file_hashes[path] = digest
+ # New file (not in baseline) is a real change; otherwise
compare.
+ return old_digest != digest
+
+ def _trigger_reload(self, source_path: str, generation: int) ->
None:
+ """Touch the reload-trigger sentinel; Flask's --extra-files
+ watcher reloads on its mtime change."""
+ # A newer event may have superseded this timer after it began
+ # running (`cancel()` can't stop an in-flight callback), so
only
+ # the most recently scheduled generation is allowed to fire.
+ with self._lock:
+ if generation != self._reload_generation:
+ return
+ logger.info("File change settled in LOCAL_EXTENSIONS: %s",
source_path)
+ logger.info("Triggering restart by touching %s",
RELOAD_TRIGGER)
+ try:
+ os.utime(RELOAD_TRIGGER, (time.time(), time.time()))
+ except OSError as e:
+ logger.warning(
+ "Failed to touch reload trigger %s: %s",
RELOAD_TRIGGER, e
+ )
Review Comment:
**Suggestion:** The debounce generation check is not atomic with the reload
trigger write: after the `generation` check passes, another file event can
schedule a newer timer before `os.utime(...)` runs, so this stale callback can
still trigger an extra restart. Keep the generation validation and sentinel
touch in one critical section (or re-check generation immediately before
touching) so superseded timers cannot fire. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Debug LOCAL_EXTENSIONS watcher can trigger duplicate restarts.
- ⚠️ Docker dev loop occasionally stalls from extra reloads.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset in debug mode so `SupersetAppFactory.create_app()` in
`superset/app.py:100-117` sets `app.debug = True` and calls
`start_local_extensions_watcher_thread(app)` at `superset/app.py:114`, which
in turn
invokes `setup_local_extensions_watcher(app)` in
`superset/extensions/local_extensions_watcher.py:234`.
2. Configure `app.config["LOCAL_EXTENSIONS"]` with at least one extension
directory so
`setup_local_extensions_watcher` populates `watch_dirs` at
`superset/extensions/local_extensions_watcher.py:260-35` and starts a
`watchdog.Observer`
with `LocalExtensionFileHandler` at `lines 46-66`, wiring `on_any_event()`
at `lines
185-226` to filesystem events under those directories.
3. Run a bundler (e.g., webpack) that writes multiple files into a `dist`
subdirectory of
the watched extension path in quick succession: each
`FileCreatedEvent`/`FileModifiedEvent` causes `on_any_event()` (`lines
190-226`) to call
`_schedule_reload()` at `lines 142-157`, which schedules a `threading.Timer`
whose
callback is `_trigger_reload(source_path, generation)` at `lines 124-141`.
4. Hit the timing window where one timer (`generation == n`) has already
entered
`_trigger_reload()` and passed the generation guard (`with self._lock` / `if
generation !=
self._reload_generation: return` at `lines 130-132`), then a new file event
arrives before
`os.utime(RELOAD_TRIGGER, ...)` at `line 136` and schedules a newer timer
(`generation ==
n+1`); the first, now-stale callback still touches `RELOAD_TRIGGER`, and the
second
callback later passes the updated generation check and touches
`RELOAD_TRIGGER` again,
producing two restarts and two corresponding log lines from
`_trigger_reload()`
(`logger.info(...)` at `lines 133-134`) instead of the single debounced
restart the design
intends.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a6de5fcffe1e4a2d9d6af6f66997fef1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a6de5fcffe1e4a2d9d6af6f66997fef1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/extensions/local_extensions_watcher.py
**Line:** 130:140
**Comment:**
*Race Condition: The debounce generation check is not atomic with the
reload trigger write: after the `generation` check passes, another file event
can schedule a newer timer before `os.utime(...)` runs, so this stale callback
can still trigger an extra restart. Keep the generation validation and sentinel
touch in one critical section (or re-check generation immediately before
touching) so superseded timers cannot fire.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40084&comment_hash=c68c228b4ce022343a66b90ef82dc5654e041ee7e43cc27046c7506738067286&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40084&comment_hash=c68c228b4ce022343a66b90ef82dc5654e041ee7e43cc27046c7506738067286&reaction=dislike'>👎</a>
--
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]
