-
Notifications
You must be signed in to change notification settings - Fork 0
Add regression for failed sloppak websocket load #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -212,6 +212,12 @@ async def _send_keepalives(): | |
| None, | ||
| lambda: _ctx.run(sloppak_mod.load_song, filename, dlc, appstate.sloppak_cache_dir), | ||
| ) | ||
| if loaded_slop is None: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This branch is unreachable: Technical details# Guard targets a return value the loader never produces
## Affected sites
- lib/routers/ws_highway.py:215 — `if loaded_slop is None:` branch
- lib/sloppak.py:1466 — `load_song` unconditionally returns `LoadedSloppak(...)`
## Evidence
- `lib/sloppak.py::load_song` (lines 991–1482) has no `return None`; it always reaches `return LoadedSloppak(...)` at line 1466.
- Its failure paths raise instead: `resolve_source_dir` (`path.stat()` → `FileNotFoundError`; `_unpack_zip` → `BadZipFile`) and `_read_manifest` → `FileNotFoundError`/`ValueError`.
- Those exceptions are caught by the generic handler at ws_highway.py:1129–1135 (`log.exception` + `{"error": str(e)}` + close). Verified empirically: with the guard removed, the test fails with `{'error': "'NoneType' object has no attribute 'song'"}` — the exception path is caught and reported, not a silent crash.
## Required outcome
- The handler must produce a clean `Failed to load sloppak` error for the real failure mode (a raising `load_song`), OR the PR should be reframed as a defensive guard with a corrected changelog claim.
## Suggested approach
- Wrap the `run_in_executor` load in `try/except` and emit `{"error": "Failed to load sloppak"}` on exception — the reachable analogue of this guard. If the `None` guard is kept anyway, correct the changelog wording so it doesn't assert a prior crash. |
||
| _keepalive_active = False | ||
| keepalive_task.cancel() | ||
| await websocket.send_json({"error": "Failed to load sloppak"}) | ||
| await websocket.close() | ||
| return | ||
|
Copilot marked this conversation as resolved.
|
||
| song = loaded_slop.song | ||
| tmp = str(loaded_slop.source_dir) | ||
| owns_tmp = False | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| """Regression coverage for sloppak load failures in the highway websocket.""" | ||
|
|
||
| import asyncio | ||
| import importlib | ||
| import sys | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| class _CapturingWS: | ||
| def __init__(self): | ||
| self.messages = [] | ||
| self.accepted = False | ||
| self.closed = False | ||
| self.close_calls = 0 | ||
|
|
||
| async def accept(self): | ||
| self.accepted = True | ||
|
|
||
| async def send_json(self, data): | ||
| self.messages.append(data) | ||
|
|
||
| async def receive_text(self): | ||
| await asyncio.sleep(0) | ||
| return "" | ||
|
|
||
| async def close(self): | ||
| self.close_calls += 1 | ||
| self.closed = True | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def server(tmp_path, monkeypatch): | ||
| (tmp_path / "dlc").mkdir() | ||
| monkeypatch.setenv("CONFIG_DIR", str(tmp_path / "config")) | ||
| monkeypatch.setenv("DLC_DIR", str(tmp_path / "dlc")) | ||
| monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1") | ||
| sys.modules.pop("server", None) | ||
| mod = importlib.import_module("server") | ||
| yield mod, tmp_path / "dlc", tmp_path / "cache" | ||
| conn = getattr(getattr(mod, "meta_db", None), "conn", None) | ||
| if conn is not None: | ||
| getattr(mod, "_join_background_db_threads", lambda: None)() | ||
| conn.close() | ||
| sys.modules.pop("server", None) | ||
|
|
||
|
|
||
| def test_sloppak_loader_returning_none_sends_error_without_touching_stems( | ||
| server, monkeypatch | ||
| ): | ||
| _server, dlc, cache = server | ||
| (dlc / "broken.feedpak").mkdir() | ||
|
|
||
| import appstate | ||
| from routers import ws_highway | ||
|
|
||
| monkeypatch.setattr(appstate, "sloppak_cache_dir", cache) | ||
| monkeypatch.setattr(ws_highway.sloppak_mod, "load_song", lambda *a, **kw: None) | ||
|
|
||
| ws = _CapturingWS() | ||
| asyncio.run(ws_highway.highway_ws(ws, "broken.feedpak", arrangement=0)) | ||
|
|
||
| assert ws.accepted is True | ||
| assert ws.closed is True | ||
| assert ws.close_calls == 1 | ||
| assert ws.messages == [ | ||
| {"type": "loading", "stage": "Extracting..."}, | ||
| {"error": "Failed to load sloppak"}, | ||
| ] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wording doesn't match the current loader:
load_songnever returnsNone, and real failures raise and are already reported by the generic handler (as a raw exception message + close), so the connection isn't crashing — the failure is being surfaced ungracefully. If theNoneguard is kept as defence-in-depth, reword this to describe theNonecontract without asserting a prior crash.