fix(extensions): make LOCAL_EXTENSIONS hot reload reliable in Docker#40084
fix(extensions): make LOCAL_EXTENSIONS hot reload reliable in Docker#40084rusackas wants to merge 5 commits into
Conversation
rusackas
requested review from
eschutho,
geido,
kgabryje,
michael-s-molina and
villebro
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40084 +/- ##
==========================================
- Coverage 64.61% 63.71% -0.91%
==========================================
Files 2684 2676 -8
Lines 148514 147302 -1212
Branches 34263 33683 -580
==========================================
- Hits 95969 93857 -2112
- Misses 50786 51680 +894
- Partials 1759 1765 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #faeec6
Actionable Suggestions - 1
-
superset/extensions/local_extensions_watcher.py - 1
- Incorrect FileMovedEvent handling · Line 101-138
Review Details
-
Files reviewed - 4 · Commit Range:
4381fd9..4381fd9- docker/docker-bootstrap.sh
- superset/extensions/api.py
- superset/extensions/local_extensions_watcher.py
- superset/extensions/utils.py
-
Files skipped - 1
- docker-compose.yml - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Per @CodeAnt-AI's and @Bito's review on #40084: 1. First-edit dropped on startup. Pre-populate baseline hashes from existing files in watched `dist` dirs via `prime_baseline()`, called once at watcher startup. Real edits now diff against the on-disk baseline instead of being silently swallowed as "first observation". 2. Move events used src_path. Atomic-build workflows (webpack tmp + rename into `dist`) mean `src_path` may point outside the watched tree. Use `dest_path` for `FileMovedEvent`, falling back to `src_path`. 3. Moves trigger regardless of content match. A move into/out of `dist` is itself the signal — don't gate it on hashing the (potentially missing) source. 4. Leading-edge debounce replaced with trailing debounce via `threading.Timer`. Each event resets the timer so the reload fires once after the build settles, instead of triggering immediately and dropping the writes that finish the build. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #302329Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Per @CodeAnt-AI's and @Bito's review on #40084: 1. First-edit dropped on startup. Pre-populate baseline hashes from existing files in watched `dist` dirs via `prime_baseline()`, called once at watcher startup. Real edits now diff against the on-disk baseline instead of being silently swallowed as "first observation". 2. Move events used src_path. Atomic-build workflows (webpack tmp + rename into `dist`) mean `src_path` may point outside the watched tree. Use `dest_path` for `FileMovedEvent`, falling back to `src_path`. 3. Moves trigger regardless of content match. A move into/out of `dist` is itself the signal — don't gate it on hashing the (potentially missing) source. 4. Leading-edge debounce replaced with trailing debounce via `threading.Timer`. Each event resets the timer so the reload fires once after the build settles, instead of triggering immediately and dropping the writes that finish the build. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rusackas
force-pushed
the
fix/extensions-dev-hot-reload
branch
from
e7a863b to
dc8b9fc
Compare
There was a problem hiding this comment.
Code Review Agent Run #dc75ca
Actionable Suggestions - 1
-
docker/docker-bootstrap.sh - 1
- CWE-20: Path Mismatch in Reload Trigger · Line 99-101
Additional Suggestions - 2
-
superset/extensions/utils.py - 1
-
Semantic path validation relaxed · Line 38-38The change from `[^/]+` to `.+` now permits files with path separators (e.g., `frontend/dist/subdir/file.js`) which the old pattern would reject at line 217. While this aligns with `BACKEND_REGEX` on line 39, verify this is the intended behavior for extension frontend bundles.
-
-
superset/extensions/local_extensions_watcher.py - 1
-
Silent content-filtering · Line 190-190The `_content_changed()` check on line 187 silently returns when content is unchanged, making it difficult to diagnose why expected reloads aren't triggering. A debug-level log here would aid development troubleshooting.
Code suggestion
--- superset/extensions/local_extensions_watcher.py +++ superset/extensions/local_extensions_watcher.py @@ -184,6 +184,7 @@ class LocalExtensionFileHandler(FileSystemEventHandler): # For Create/Modify, verify the content actually changed to # ignore spurious inotify events generated by Docker bind-mount # reads. + logger.debug("Content unchanged, skipping reload: %s", target) if not self._content_changed(target): return
-
Review Details
-
Files reviewed - 4 · Commit Range:
5ab8565..dc8b9fc- docker/docker-bootstrap.sh
- superset/extensions/api.py
- superset/extensions/local_extensions_watcher.py
- superset/extensions/utils.py
-
Files skipped - 1
- docker-compose.yml - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
The local-extensions watcher previously surfaced spurious reloads under Docker on macOS (VirtioFS / osxfs generates inotify events on every read, not just writes), and rebuilds that wrote many files at once produced one restart per file. It also touched superset/__init__.py to trigger Flask's reloader, which then meant any Python read of that file also looked like a change and recursed. Changes: - Watcher only acts on FileCreated/Modified/Moved events, verifies the file content actually changed via SHA-256, and debounces to one trigger per second. - Use a dedicated sentinel file (superset/extensions/.reload_trigger) that Python never reads, registered with Flask via --extra-files. The watcher ensures the sentinel exists on startup. - Bootstrap excludes superset/__init__.py from the file watcher so the old trigger path can't reintroduce the loop. - docker-compose mounts ./local_extensions:/app/local_extensions so the bind path matches LOCAL_EXTENSIONS in the dev superset_config. - Extension content endpoint and FRONTEND_REGEX accept nested paths inside frontend/dist so extensions can serve worker / WASM / chunk subfolders. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per @CodeAnt-AI's and @Bito's review on #40084: 1. First-edit dropped on startup. Pre-populate baseline hashes from existing files in watched `dist` dirs via `prime_baseline()`, called once at watcher startup. Real edits now diff against the on-disk baseline instead of being silently swallowed as "first observation". 2. Move events used src_path. Atomic-build workflows (webpack tmp + rename into `dist`) mean `src_path` may point outside the watched tree. Use `dest_path` for `FileMovedEvent`, falling back to `src_path`. 3. Moves trigger regardless of content match. A move into/out of `dist` is itself the signal — don't gate it on hashing the (potentially missing) source. 4. Leading-edge debounce replaced with trailing debounce via `threading.Timer`. Each event resets the timer so the reload fires once after the build settles, instead of triggering immediately and dropping the writes that finish the build. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rusackas
force-pushed
the
fix/extensions-dev-hot-reload
branch
from
dc8b9fc to
b25ebcf
Compare
Code Review Agent Run #bad9c7Actionable Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
FWIW, @michael-s-molina @villebro this was bycatch of working on the Notebook extension, and seemed to stabilize things as I was working on them in hot-rebuild mode. Worth an AI sanity check at least to see if you think it's worthwhile. |
…s out of dist Annotate the debounce interval and the frontend/backend bundle regexes, and treat a rename that moves a built artifact out of dist as a reload signal (previously only the dest path was checked, so moves out of dist were dropped despite the comment claiming they are handled). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #fa6580Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| @protect() | ||
| @safe | ||
| @expose("/<publisher>/<name>/<file>", methods=("GET",)) | ||
| @expose("/<publisher>/<name>/<path:file>", methods=("GET",)) |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Nested extension assets retain Vary: Cookie header.
- ⚠️ Shared caches cannot reuse nested extension asset responses.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| 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 |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ LOCAL_EXTENSIONS watcher retains hashes for deleted artifacts.
- ⚠️ Long-running dev sessions see increasing Python process memory.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
Code Review Agent Run #ecc6d4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
While developing local Superset extensions inside Docker on macOS, the hot-reload loop was unreliable:
on_any_eventreacted to all of them, so every Python import that touched a file under the extension directory triggered a server restart.webpack --watchrebuild that wrote N files produced N restarts in quick succession — Flask would begin restarting before the build finished, and the dev loop would stall.superset/__init__.pyto signal Flask's reloader. Any subsequent read of__init__.pylooked like another change → another restart → another read → loop.FRONTEND_REGEXand the/api/v1/extensions/<publisher>/<name>/<file>route both used single-segment matchers, so extensions that needed to serve worker / WASM / chunk files in subdirectories (e.g. DuckDB WASM, Pyodide) couldn't.Changes
superset/extensions/local_extensions_watcher.pyFileCreated,FileModified, andFileMovedevents (skip access/open/close).superset/__init__.pyto a dedicated sentinelsuperset/extensions/.reload_triggerthat Python never imports. Watcher creates the sentinel on startup.docker/docker-bootstrap.sh--extra-files /app/superset/extensions/.reload_triggerso Flask watches the sentinel.superset/__init__.pyfrom the Flask reloader so the previous trigger path can't reintroduce the loop.docker-compose.yml./local_extensions:/app/local_extensionsto matchLOCAL_EXTENSIONSin the devsuperset_config_docker.py.superset/extensions/api.py+superset/extensions/utils.pyFRONTEND_REGEXaccepts nested paths insidefrontend/dist./api/v1/extensions/<publisher>/<name>/<path:file>URL converter accepts nested paths so extensions can serve worker / WASM / chunk subfolders.TESTING INSTRUCTIONS
docker compose up../local_extensions/and register it viaLOCAL_EXTENSIONSindocker/pythonpath_dev/superset_config_docker.py.npm run watchinside the extension's frontend dir./api/v1/extensions/{publisher}/{name}/subdir/file.wasm) are served, not 404.ADDITIONAL INFORMATION