codeant-ai-for-open-source[bot] commented on code in PR #40084:
URL: https://github.com/apache/superset/pull/40084#discussion_r3525765165
##########
superset/extensions/api.py:
##########
@@ -169,7 +169,7 @@ def get(self, publisher: str, name: str, **kwargs: Any) ->
Response:
@protect()
@safe
- @expose("/<publisher>/<name>/<file>", methods=("GET",))
+ @expose("/<publisher>/<name>/<path:file>", methods=("GET",))
Review Comment:
**Suggestion:** Changing the route to accept nested file paths introduces an
integration mismatch: extension asset requests with subdirectories now bypass
the cache middleware pattern that still matches only single-segment filenames,
so `Vary: Cookie` is not stripped for those responses and shared/browser
caching is degraded. Update the middleware asset-path regex to support nested
paths so the new route behavior remains cache-consistent. [cache]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Nested extension assets retain Vary: Cookie header.
- ⚠️ Shared caches cannot reuse nested extension asset responses.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/app.py:23-87`, call `create_app()`, which wraps
`app.wsgi_app` with
`ExtensionCacheMiddleware` at lines 61-64 so every request passes through
the cache
middleware before routing.
2. Load an extension bundle whose frontend assets include nested paths under
`frontend/dist`, which are accepted by `FRONTEND_REGEX` in
`superset/extensions/utils.py:9-15` and mapped into `frontend[...]` by
`get_loaded_extension()` at `superset/extensions/utils.py:190-193`.
3. Observe that extension frontend URLs are constructed with the asset path
in
`build_extension_data()` at `superset/extensions/utils.py:213-235`, e.g.
`/api/v1/extensions/{publisher}/{name}/{frontend.remoteEntry}`, and that the
asset route
`content()` in `superset/extensions/api.py:170-233` is exposed at line 172
using a Flask
`path` converter to accept nested segments.
4. Issue an HTTP GET to a nested asset like
`/api/v1/extensions/acme/my-ext/nested/chunk.wasm`, which is handled by
`ExtensionsRestApi.content()` (api.py:172-233) but whose `PATH_INFO` does
not match
`_ASSET_PATH_RE` (single-segment file regex) in
`superset/extensions/cache_middleware.py:26-28`, so
`ExtensionCacheMiddleware.__call__()`
(lines 40-73) bypasses Vary-header stripping and leaves `Vary: Cookie` in
the response,
degrading shared/browser caching for nested assets.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=37aa7ebcefad45e5bc26d736c32da79c&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=37aa7ebcefad45e5bc26d736c32da79c&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/api.py
**Line:** 172:172
**Comment:**
*Cache: Changing the route to accept nested file paths introduces an
integration mismatch: extension asset requests with subdirectories now bypass
the cache middleware pattern that still matches only single-segment filenames,
so `Vary: Cookie` is not stripped for those responses and shared/browser
caching is degraded. Update the middleware asset-path regex to support nested
paths so the new route behavior remains cache-consistent.
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=d001a4de05d084c0e7593c75f6cb37d15534d94b2481e9575d5c15812af62869&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40084&comment_hash=d001a4de05d084c0e7593c75f6cb37d15534d94b2481e9575d5c15812af62869&reaction=dislike'>👎</a>
##########
superset/extensions/local_extensions_watcher.py:
##########
@@ -29,37 +29,183 @@
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,
+ 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
Review Comment:
**Suggestion:** The in-memory hash index grows without bound because every
newly observed build artifact path is inserted and never evicted, which will
steadily increase memory usage during long dev sessions with hashed chunk
filenames. Remove entries on delete/move-out events (or periodically prune
non-existent paths) to avoid unbounded state growth. [memory leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ LOCAL_EXTENSIONS watcher retains hashes for deleted artifacts.
- ⚠️ Long-running dev sessions see increasing Python process memory.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset in debug mode via `create_app()` in
`superset/app.py:23-87`; when
`app.debug` is true, `start_local_extensions_watcher_thread(app)` is invoked
at lines
83-85, which calls `setup_local_extensions_watcher()` in
`superset/extensions/local_extensions_watcher.py:216-323`.
2. In `setup_local_extensions_watcher()`, configure `LOCAL_EXTENSIONS` so at
least one
extension directory is watched; `handler_class = _get_file_handler_class()`
(lines
238-241) returns `LocalExtensionFileHandler`, which allocates
`self._file_hashes:
dict[str, str] = {}` in its `__init__` at line 69.
3. When the watcher thread starts, it instantiates `event_handler =
handler_class()` and
calls `event_handler.prime_baseline(watch_dirs)` at
`superset/extensions/local_extensions_watcher.py:291-295`, then begins
observing file
events; for each new or modified asset under `dist`, `on_any_event()` (lines
160-208)
calls `_content_changed(target)` at lines 107-121.
4. `_content_changed()` computes a digest and unconditionally assigns
`self._file_hashes[path] = digest` at line 119; because there is no code
anywhere in
`local_extensions_watcher.py` that deletes keys from `_file_hashes`, repeated
webpack-style rebuilds that emit new hashed chunk filenames (as implied by
the comment
“Chunk filenames include a content hash” in
`superset/extensions/api.py:229`) steadily
accumulate entries for old paths, causing `_file_hashes` to grow without
bound over long
dev sessions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=917f235abec5473d86396e96b08252c3&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=917f235abec5473d86396e96b08252c3&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:** 69:119
**Comment:**
*Memory Leak: The in-memory hash index grows without bound because
every newly observed build artifact path is inserted and never evicted, which
will steadily increase memory usage during long dev sessions with hashed chunk
filenames. Remove entries on delete/move-out events (or periodically prune
non-existent paths) to avoid unbounded state growth.
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=87a6545afac74bd72a4dd9b1e74339dd8d26721f0ce5063bf7e3d052107f56c1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40084&comment_hash=87a6545afac74bd72a4dd9b1e74339dd8d26721f0ce5063bf7e3d052107f56c1&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]
