[R2-8][P2] Surface silent save/mutation errors to user and log

[MichaelFisher1997]

Part of #839 — [Audit][Round 2] A++ umbrella.
Phase: P2 · Finding: R2-8

Problem

User-facing save operations and player block mutations silently swallow errors with catch {}. The user gets no feedback that their save data wasn't deleted/renamed, and level.dat corruption is possible without surfacing. Masks real bugs (out-of-bounds, chunk-unloaded races, disk full).

Evidence

Silent save failures (user-facing)

modules/game-ui/src/screens/world_list.zig:364,384,413 (verified in audit):

self.confirmDelete(idx) catch {};      // line 364 — world delete failure silent
self.clearAllWorlds() catch {};        // line 384 — clear-all failure silent
self.renameWorld(idx) catch {};        // line 413 — rename failure silent

These touch disk. A failure means the user clicked "Delete World", got a green toast, and the world is still on disk.

Silent block mutations

modules/game-core/src/player.zig:386,405:

world.setBlock(target.x, target.y, target.z, .air) catch {};   // line 386 — break block
world.setBlock(px, py, pz, block_type) catch {};               // line 405 — place block

Minor for gameplay, but masks real bugs (chunk-unloaded races, out-of-bounds).

Logged-then-dropped save errors

modules/world-persistence/src/save_manager.zig:131-132, 167-168:

self.level_data.saveToFile(self.allocator, self.save_dir) catch |err| {
    log.log.err("Failed to save level.dat: {}", .{err});
};

The save thread logs the error then drops it. level.dat corruption is possible without surfacing to the player. No failed_chunks retry-on-next-session logic visible here.

Count for context

• 27 catch {} total across the codebase; most benign (tmp-file cleanup, optional UI actions, sleep fallbacks). The 7 above are the user-facing/risky ones.

Fix

Tier 1 — surface to user (Tier 1 PR)

• world_list.zig:364,384,413: replace catch {} with catch |err| { log.log.err("...", .{err}); showErrorToast("Failed to delete world"); }. Use the existing toast/notification system in engine-ui.
• save_manager.zig:131-132, 167-168: introduce a SaveFailureCounter (in-memory) that's checked on next save-session startup. If non-zero, surface "N chunks failed to save last session" warning to the player on world load.

Tier 2 — propagate to logs (Tier 2 PR)

• player.zig:386,405: replace catch {} with catch |err| { log.log.warn("Block mutation failed at ({}, {}, {}): {}", .{ x, y, z, err }); }. Use warn (not err) — these are expected under chunk-unload races.
• Audit the remaining 20 catch {} calls; convert any that touch disk, GPU, or user state to catch |err| log....

Tier 3 — surface to UI thread

• The save thread failure should set an atomic flag that WorldScreen.update() polls and surfaces via a non-blocking "Save warning" indicator (similar to Minecraft's "saving..." badge).

Verification

• nix develop --command zig build test
• New unit tests:
  • world_list_tests.zig: inject failing filesystem call, assert error toast is shown (mock the toast system)
  • save_manager_tests.zig: inject failing saveToFile, assert log message and counter increment
  • player_tests.zig: inject failing setBlock, assert warning log
• grep -rE "catch \{\}" modules/game-ui/ modules/game-core/src/player.zig modules/world-persistence/src/save_manager.zig should return zero matches after fix

Constraints

• Behavior changes are limited to: previously-silent errors now produce user-visible feedback and/or log entries. No new panics.
• Tier 1 and Tier 2 can land as separate PRs.
• Conventional commits: fix(game-ui): surface world-delete errors, fix(world-persistence): log and counter level.dat save failures, fix(game-core): log block-mutation failures.

Notes

• This is not about making every error fatal — most of these are legitimately recoverable. It's about observability: a silent disk-write failure that loses player data is a real production bug class.
• Coordinate with R2-9 (game-ui test coverage) — the toast-mock harness developed here benefits both.

Tracking: #839

[github-actions]

I've validated the issue. All seven catch {} sites are at the line numbers cited. One minor correction worth noting: failed_chunks already exists in save_manager.zig:66,288 and is returned by flush() at line 229–232, so the Tier 1 "SaveFailureCounter" maps onto the existing list — the gap is just surfacing it to the user, not adding a new counter.

---

Comment to post

Validated — all 7 sites confirmed at the cited line numbers. One small correction: failed_chunks is already tracked in save_manager.zig:66,288 and returned by flush() at line 229–232 (caller currently throws it away). Tier 1's "SaveFailureCounter" therefore doesn't need to be a new structure — the gap is purely the missing handoff to WorldScreen. Good candidate for a single small PR.

Classification: Bug (silent-failure observability defect).

Scope assessment: Already well-decomposed. Tier 1 / Tier 2 / Tier 3 are the natural sub-PR boundaries and should land in that order:

1. Tier 1 — fix(game-ui): surface world-delete / clear / rename errors (world_list.zig:364,384,413 + save_manager.zig failure-handoff). Required mock: a single toast-mock seam in engine-ui benefits both this issue and R2-9, so coordinate there first. Suggest a thin Notifier / ToastBus interface in engine-ui with a showError(msg) method that WorldScreen already has a reference to.
2. Tier 2 — fix(game-core): log block-mutation failures (player.zig:386,405). warn (not err) is correct — these are genuinely recoverable under chunk-unload races, and err would spam logs during normal play if a chunk streams out mid-mine.
3. Tier 2b — fix(world-persistence): log enqueue + level.dat failures with chunk coords (save_manager.zig:130-132,167-169). Note the failed_chunks.append(...) catch {} at save_manager.zig:288 is the same class of bug and should be fixed in this same commit.
4. Tier 3 — atomic save-warning flag polled by WorldScreen.update(). Nice-to-have; the failure list already comes back from flush() so the wire is in place.

Verification additions:

• The proposed grep -rE "catch \{\}" modules/game-ui/ modules/game-core/src/player.zig modules/world-persistence/src/save_manager.zig is a good zero-match gate.
• For the save-manager test, prefer injecting a failing_writer via the existing LevelData save path rather than mocking the filesystem — it's a smaller seam and matches the codebase's pattern of dependency-injectable writers in the meshing/persistence modules.
• The 20 remaining catch {} sites don't all need converting; the suggested filter ("touches disk, GPU, or user state") is the right one. Worth a follow-up audit issue rather than blocking this PR on them.

No blocking questions. Ready for a Tier 1 PR.

opencode session  |  github run