Conversation
…closes #410) Reported in #410 — open-webui / perplexica / anything-llm POST install returned 200 OK but no docker container ever ran. Card stayed "stale" forever. Cause: the docker/pip branch in _legacy_install fell straight through to the installed-apps store, which only records the install in the db without invoking any actual installer. Comment said "delegate to InstalledAppsStore (docker/pip/download)" but the delegation was just a registry write. Now: when backend is "docker" or "pip", instantiate the corresponding installer and run install(). Failures bubble up as 500 with the installer's error message instead of silent success. For docker we also auto-start compose so the service is actually serving on its declared ports without a separate user step (pip installs don't have a generic start command — those are libraries the caller invokes). Existing 26 store + install-v2 tests still pass.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Default to data_dir/apps when app.state.apps_dir isn't set so installs work without /opt write access (no root needed) and so pytest's tmp data_dir captures the install side-effect cleanly. Tests for the default install branch now pass — the earlier 2 PermissionErrors on /opt/tinyagentos/apps were because the installer code was actually running for the first time.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICALWARNINGSUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments: Files Reviewed (1 files)
Reviewed by grok-code-fast-1:optimized:free · 208,743 tokens |
| # app.state.apps_dir on a tmp path. | ||
| apps_dir = getattr(request.app.state, "apps_dir", None) | ||
| if apps_dir is None: | ||
| data_dir = getattr(request.app.state, "data_dir", None) |
There was a problem hiding this comment.
WARNING: Only catching FileNotFoundError for installer availability, but ImportError (if installer modules not found) will not be caught, leading to unhandled 500 error.
Add except ImportError as exc: before the general Exception handler.
| from pathlib import Path as _Path | ||
| apps_dir = _Path(data_dir) / "apps" | ||
| try: | ||
| from tinyagentos.installers.docker_installer import DockerInstaller |
There was a problem hiding this comment.
WARNING: Broad exception catching with except Exception may hide important errors. Consider catching more specific exceptions related to installer failures.
| ) | ||
| except Exception as exc: # noqa: BLE001 | ||
| logger.exception("_legacy_install: %s installer raised", backend) | ||
| return JSONResponse({"error": f"{backend} install failed: {exc}"}, status_code=500) |
There was a problem hiding this comment.
SUGGESTION: If docker start fails, the app is still marked as installed, but the service may not be serving. Consider whether install should fail if start fails, or at least return a warning to the client.
| return JSONResponse( | ||
| {"error": inst_result.get("error", f"{backend} install failed")}, | ||
| status_code=500, | ||
| ) |
There was a problem hiding this comment.
WARNING: Broad exception catching with except Exception may hide important errors during docker start. Consider catching more specific exceptions.
- Catch FileNotFoundError + ImportError on installer construction - Narrow broad Exception → OSError + RuntimeError (real failure modes for installer.install which exec processes + touches the filesystem) - Same narrowing on docker compose-up. logger.exception still captures the traceback so debuggability is unchanged. - Documented the design choice on docker start: image is on disk after pull succeeds, so we warn instead of failing the install when compose up trips on a port collision / daemon hiccup. User can manually 'docker compose up -d' without re-pulling.
Summary
Reported in #410 — open-webui / perplexica / anything-llm POST install returned 200 OK but no docker container ever ran. Cards stayed "stale" forever.
`_legacy_install`'s docker/pip branch was a stub: it skipped past the installer code and just marked the app installed in the registry. Comment said "delegate to InstalledAppsStore (docker/pip/download)" but the delegation was a registry write only.
Now: when `backend in ("docker", "pip")`, instantiate the corresponding installer and call install(). Failures bubble up as 500 with the installer's error message. For docker we also auto-start compose so the service is actually serving on its declared ports without a separate user step.
This unblocks the new chat-UI manifests landed earlier tonight (anything-llm, librechat, jan, farfalle, plus the existing open-webui / perplexica) and the docker-method services from packs 2-9.
Test plan
Closes #410.
Jay is asleep right now so this PR was opened by Claude Opus 4.7.