fix(ui): fix /ui/chat 404 — Dockerfile pre-restructure + heuristic false-positive#24247
fix(ui): fix /ui/chat 404 — Dockerfile pre-restructure + heuristic false-positive#24247Asseel-Naji wants to merge 2 commits intoBerriAI:mainfrom
Conversation
…e-restructure in Dockerfile
Starlette StaticFiles(html=True) serves extensionless routes (e.g. /ui/chat)
by looking for {route}/index.html. Next.js static export generates login/index.html
for older pages but chat.html (root-level) for the new Chat UI page added in BerriAI#22937.
The _is_ui_pre_restructured() fallback heuristic returned True as soon as it found
any {dir}/index.html (e.g. login/index.html), skipping the restructure step and
leaving chat.html in place — causing /ui/chat to 404.
Fix 1: proxy_server.py — tighten the heuristic to also scan for orphaned root-level
.html files; return False if any exist so restructuring runs.
Fix 2: Dockerfile — add pre-restructure step (matching Dockerfile.non_root) so the
main published image pre-moves all {page}.html → {page}/index.html and writes the
.litellm_ui_ready marker, bypassing the heuristic entirely on subsequent starts.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Asseel Naji seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Greptile SummaryThis PR fixes Key changes:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| Dockerfile | Adds a RUN step after build_admin_ui.sh to move root-level {page}.html files into {page}/index.html and write .litellm_ui_ready marker — mirrors the existing step in Dockerfile.non_root. The shell loop lacks set -e / ` |
| litellm/proxy/proxy_server.py | Tightens _is_ui_pre_restructured() to scan for root-level orphaned .html files whenever a restructured directory is found; returns False if any exist so _restructure_ui_html_files() can run. The inner os.scandir is called inside the outer loop (redundant scan noted in prior thread) and silently swallows OSError into an empty list — both minor but no logic bugs in the happy path. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Docker build: build_admin_ui.sh\nGenerates out/ with mixed structure:\n login/index.html ✓\n chat.html ✗"] --> B["NEW: Pre-restructure RUN step\nmv chat.html → chat/index.html\ntouch .litellm_ui_ready"]
B --> C["python -m build\nWheel baked with restructured\nfiles + marker"]
C --> D["Proxy server startup\n_is_ui_pre_restructured(ui_path)"]
D --> E{".litellm_ui_ready\nmarker exists?"}
E -- "Yes (Docker image path)" --> F["Return True\nSkip restructure"]
E -- "No (source / older image)" --> G{"Any {dir}/index.html\nfound?"}
G -- "No" --> H["Return False\nRun _restructure_ui_html_files()"]
G -- "Yes" --> I{"NEW: Any orphaned\n*.html at root?\n(excl. index.html, 404.html)"}
I -- "Yes (e.g. chat.html)" --> H
I -- "No" --> F
F --> J["StaticFiles(html=True)\nServes /ui/chat → chat/index.html ✓"]
H --> K["_restructure_ui_html_files()\nmoves remaining *.html → */index.html"] --> J
Comments Outside Diff (1)
-
tests/proxy_unit_tests/test_ui_path_detection.py, line 119-136 (link)No test coverage for the heuristic false-positive scenario
The
test_structural_routes_existtest verifies a fully restructured state (all routes as{route}/index.html), but the specific scenario this PR fixes — a mixed output where some routes are pre-restructured (e.g.login/index.html) and at least one new route is still a root-level.htmlfile (e.g.chat.html) — is not exercised by any test.Without a test for this case, a future refactor of
_is_ui_pre_restructuredcould silently reintroduce the false-positive. Consider adding a test case:def test_mixed_state_returns_not_pre_restructured(self): """Mixed state: some routes restructured, but orphaned chat.html still present.""" # Simulate login already restructured login_dir = os.path.join(self.temp_dir, "login") os.makedirs(login_dir) Path(os.path.join(login_dir, "index.html")).touch() # Simulate chat still as a root-level .html (the bug scenario) Path(os.path.join(self.temp_dir, "index.html")).touch() Path(os.path.join(self.temp_dir, "chat.html")).touch() orphaned = [ e.name for e in os.scandir(self.temp_dir) if os.path.isfile(os.path.join(self.temp_dir, e.name)) and e.name.endswith(".html") and e.name not in ("index.html", "404.html") ] assert orphaned == ["chat.html"], "chat.html should be detected as orphaned"
Rule Used: What: Ensure that any PR claiming to fix an issue ... (source)
Last reviewed commit: "Merge branch 'main' ..."
| RUN cd litellm/proxy/_experimental/out && \ | ||
| for html_file in *.html; do \ | ||
| if [ "$html_file" != "index.html" ] && [ "$html_file" != "404.html" ] && [ -f "$html_file" ]; then \ | ||
| folder_name="${html_file%.html}" && \ | ||
| mkdir -p "$folder_name" && \ | ||
| mv "$html_file" "$folder_name/index.html"; \ | ||
| fi; \ | ||
| done && \ | ||
| touch .litellm_ui_ready |
There was a problem hiding this comment.
Silent partial restructure with marker file written
If any individual mv operation fails (e.g. a pre-existing {page}/index.html conflicts, or a filesystem error occurs), the for loop continues because shell for loops do not propagate inner command failures to the outer flow. The final touch .litellm_ui_ready then still executes, marking the UI as fully restructured even if some HTML files were not moved.
At runtime _is_ui_pre_restructured() sees the marker file and returns True early, skipping the Python fallback, so the unprocessed route will still 404.
Consider adding set -e at the top of the RUN or using || exit 1 after the move to ensure the build fails loudly rather than silently producing a partial result:
| RUN cd litellm/proxy/_experimental/out && \ | |
| for html_file in *.html; do \ | |
| if [ "$html_file" != "index.html" ] && [ "$html_file" != "404.html" ] && [ -f "$html_file" ]; then \ | |
| folder_name="${html_file%.html}" && \ | |
| mkdir -p "$folder_name" && \ | |
| mv "$html_file" "$folder_name/index.html"; \ | |
| fi; \ | |
| done && \ | |
| touch .litellm_ui_ready | |
| RUN set -e && cd litellm/proxy/_experimental/out && \ | |
| for html_file in *.html; do \ | |
| if [ "$html_file" != "index.html" ] && [ "$html_file" != "404.html" ] && [ -f "$html_file" ]; then \ | |
| folder_name="${html_file%.html}" && \ | |
| mkdir -p "$folder_name" && \ | |
| mv "$html_file" "$folder_name/index.html"; \ | |
| fi; \ | |
| done && \ | |
| touch .litellm_ui_ready |
| try: | ||
| orphaned = [ | ||
| e.name | ||
| for e in os.scandir(ui_dir) | ||
| if e.is_file() | ||
| and e.name.endswith(".html") | ||
| and e.name not in ("index.html", "404.html") | ||
| ] | ||
| except (PermissionError, OSError): | ||
| orphaned = [] | ||
| if orphaned: | ||
| verbose_proxy_logger.debug( | ||
| f"Found un-restructured HTML files at root: {orphaned}. " | ||
| f"Restructuring needed despite existing {entry.name}/index.html." | ||
| ) | ||
| return False |
There was a problem hiding this comment.
Redundant directory scan on every matched entry
The inner os.scandir(ui_dir) is launched inside the outer for entry in os.scandir(ui_dir) loop, effectively scanning the same directory twice. In the current implementation the function always returns on the first matching entry, so the double-scan is harmless, but it is an implicit invariant that future readers may not notice.
Consider hoisting the orphan check outside the loop so it is only performed once after confirming at least one restructured directory exists:
restructured_found = None
for entry in os.scandir(ui_dir):
if entry.is_dir() and not entry.name.startswith("_"):
index_path = os.path.join(entry.path, "index.html")
if os.path.exists(index_path):
restructured_found = entry.name
break
if restructured_found:
try:
orphaned = [
e.name
for e in os.scandir(ui_dir)
if e.is_file()
and e.name.endswith(".html")
and e.name not in ("index.html", "404.html")
]
except (PermissionError, OSError):
orphaned = []
if orphaned:
verbose_proxy_logger.debug(...)
return False
verbose_proxy_logger.debug(...)
return True
Root Cause
The Chat UI added in #22937 (and routing fixed in #22945) returns 404 in the published Docker images (
main-stable,main-latest).Why it breaks
Next.js static export generates
_experimental/out/chat.htmlfor the new/chatroute, but generateslogin/index.html,playground/index.htmletc. for older routes (mixed output structure).Starlette
StaticFiles(html=True)serves extensionless routes by looking for{route}/index.html. It does not try{route}.html, so/ui/chat→ 404._restructure_ui_html_files()already exists to move{page}.html → {page}/index.htmlat runtime — but it is gated by_is_ui_pre_restructured()._is_ui_pre_restructured()fires a false-positive: it returnsTrueas soon as it finds any{dir}/index.html(e.g.login/index.html), concluding the UI is fully restructured. This skips the restructure step and leaveschat.htmlin place.The main
Dockerfile(used for published images) never runs the pre-restructure step thatDockerfile.non_rootdoes, so no.litellm_ui_readymarker is written either.Fixes
Dockerfile— add the same pre-restructure step asDockerfile.non_root:This moves
chat.html → chat/index.htmlat image build time and writes the marker, so the proxy skips the heuristic entirely.proxy_server.py— tighten the_is_ui_pre_restructured()fallback heuristic: when a restructured route is found, also scan for orphaned root-level.htmlfiles; returnFalseif any exist so that_restructure_ui_html_files()can run.Testing
Related