Skip to content

fix(shared): stop run_game masking crashes with sys.exit in finally - #851

Merged
dieterolson merged 8 commits into
mainfrom
fix/game-launcher-exception-masking
Jul 28, 2026
Merged

fix(shared): stop run_game masking crashes with sys.exit in finally#851
dieterolson merged 8 commits into
mainfrom
fix/game-launcher-exception-masking

Conversation

@dieterolson

Copy link
Copy Markdown
Contributor

Problem

run_game() in src/games/shared/game_launcher.py called sys.exit() inside a finally block:

finally:
    pygame.quit()
    sys.exit()

sys.exit() raises SystemExit, which replaces any in-flight exception. Any real crash — asset load failure, display init failure, a bug in game.run() — was silently discarded and the process exited 0, reporting success.

Confirmed empirically on a Raspberry Pi running Batocera: Force Field failed to create its display and exited 0 with no traceback whatsoever. The underlying error (pygame.error: <driver> not available from pygame.display.set_mode in src/games/shared/fps_game_base.py:133) was only recoverable by constructing the Game class directly, bypassing run_game.

This affects every game routed through run_game() — Force Field, Duum, and Zombie Survival at minimum.

Fix

Log the exception with a traceback and re-raise it, and move sys.exit() out of finally so it only runs on the success path:

except Exception:
    logger.exception("Game crashed")
    raise
finally:
    pygame.quit()

# Only reached when the game exited normally (or was interrupted).
sys.exit()

sys.exit() is kept on the success path rather than dropped entirely, so normal-exit and KeyboardInterrupt behavior is unchanged — both pre-existing tests pass unmodified. The contract is documented in a Raises: block per the repo's DbC convention.

Tests

Three regression tests added (written failing first, per the repo's TDD convention):

  • exception from game.run() propagates out of run_game()
  • exception from game construction propagates — this is the actual Force Field case, where set_mode fails in __init__
  • the crash is logged with exc_info before being re-raised

These deliberately do not patch sys.exit — patching it is precisely what hid this bug from the existing test suite.

Verification

  • RED → GREEN on the three new tests; tests/shared/test_game_launcher.py 11/11 passing.
  • Full suite: 1826 passed, 2 skipped (pre-existing skips).
  • ruff, black, mypy clean; pre-commit run --files on both changed files: all hooks passed.
  • End-to-end subprocess check reproducing the Batocera failure (pygame.error raised from __init__): now prints a full traceback and exits 1. Before this change: exit 0, silent.

sys.exit() in the finally block raised SystemExit, which replaced any
in-flight exception. Real crashes (display init failure, asset load
failure, bugs in game.run()) were silently discarded and the process
exited 0, reporting success. Confirmed on a Raspberry Pi running
Batocera: Force Field failed pygame.display.set_mode and exited 0 with
no traceback; the underlying pygame.error was only recoverable by
constructing the Game class directly, bypassing run_game.

Log the exception with a traceback and re-raise it, and move sys.exit()
out of finally so it only runs on the success path. Affects every game
routed through run_game -- Force Field, Duum, Zombie Survival.

Adds regression tests (written failing first) asserting that exceptions
from both game construction and game.run() propagate out of run_game
rather than becoming a clean exit. These deliberately do not patch
sys.exit -- patching it is what hid the bug from the existing tests.
@dieterolson
dieterolson enabled auto-merge (squash) July 27, 2026 15:10
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ SPEC.md Update Required

Source files were modified in this PR but SPEC.md was not updated.

Per the Specification-Driven Development Policy:

  • Any PR that adds, removes, or changes functionality must update SPEC.md
  • Update the relevant sections (features, architecture, tests, dependencies, etc.)
  • Bump the Spec Version if making substantive changes

To resolve:

  1. Update SPEC.md to reflect your changes, OR
  2. Add the spec-exempt label if this PR genuinely doesn't affect the spec (e.g., pure refactor with no behavior change)

This check is enforced by the spec-check CI workflow.

@dieterolson
dieterolson merged commit 6cb32e5 into main Jul 28, 2026
11 of 13 checks passed
@github-actions github-actions Bot added ci/cd config documentation Improvements or additions to documentation size/M labels Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/cd config documentation Improvements or additions to documentation size/M size/S tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant