Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- **Failed sloppak loads at the highway websocket no longer crash on a `None` song.**
When `sloppak_mod.load_song()` returns `None` (cache corruption, partial
extraction, etc.), the handler previously dereferenced `loaded_slop.song`
before any guard existed, crashing the connection instead of reporting the

Copy link
Copy Markdown

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_song never returns None, 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 the None guard is kept as defence-in-depth, reword this to describe the None contract without asserting a prior crash.

failure. The load-failure guard now sits immediately after the load call —
the handler sends a `Failed to load sloppak` error and closes the socket
instead of continuing into arrangement/stem access with a `None` song.

### Added
- **Core reader for source rigs (feedpak 1.18.0).** A pack can declare what a
MIDI part should sound like by binding a rig; core now reads that binding and
Expand Down
6 changes: 6 additions & 0 deletions lib/routers/ws_highway.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is unreachable: sloppak.load_song (lib/sloppak.py:991) always returns a LoadedSloppak, and every failure path — missing/corrupt manifest, bad zip — raises rather than returning None. Those exceptions are already caught by the generic handler (ws_highway.py:1129), which sends {"error": str(e)} and closes, so a real load failure is reported (as a raw message), not a crashed connection. To make the clean Failed to load sloppak message reachable, catch the exception around the run_in_executor call instead of gating on None.

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
Comment thread
Copilot marked this conversation as resolved.
song = loaded_slop.song
tmp = str(loaded_slop.source_dir)
owns_tmp = False
Expand Down
69 changes: 69 additions & 0 deletions tests/test_highway_ws_failed_sloppak_load.py
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
Comment thread
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"},
]
Loading