From dc150ff8eeca4fd388843f48ccad61b8961cc0fd Mon Sep 17 00:00:00 2001 From: nicodes Date: Sun, 2 Aug 2026 19:13:35 -0600 Subject: [PATCH] Say things a player can act on, and test the crash containment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small things, all the same complaint: the arcade explained itself to a developer. The registry client turned a transport failure into Go's error, which names a host, a port, a query string and a syscall. None of that is actionable, and the TUI rendered it straight into a notice — so a marketplace that was merely not running looked like a stack trace. It is one message now, with the detail behind TERMCADE_DEBUG for whoever is actually debugging. before: registry unreachable: Get "http://127.0.0.1:8080/v1/games/ aviorstudio/tetris/resolve?abi=1": dial tcp 127.0.0.1:8080: connect: connection refused after: the marketplace is not answering — check your connection and try again The registry writes its own messages for a person to read, so they pass through unprefixed; "termcade: registry: ..." was a stutter rather than attribution. Same reason the TUI stopped prefixing "marketplace unreachable:" onto a message that already says so. `list` printed nothing at all with nothing installed, which reads as a broken command rather than an empty arcade. The TUI has empty states on every screen; the CLI had none. A bare slug is the likeliest way to get an id wrong, and "open asteroid: no such file or directory" answers a question about the filesystem that nobody asked. SafeGame had no tests, which was the wrong place for the gap: it is the whole of "a broken game can never take the arcade down". It now has eight, including that every method contains a panic, that the first crash latches and nothing reaches the game afterwards, and that Close still runs after a crash — which is exactly when a wasm instance needs releasing. Co-Authored-By: Claude Opus 5 (1M context) --- cli.go | 26 ++++- internal/engine/safe_test.go | 187 +++++++++++++++++++++++++++++++++++ internal/registry/client.go | 52 +++++++--- internal/shell/market.go | 5 +- 4 files changed, 257 insertions(+), 13 deletions(-) create mode 100644 internal/engine/safe_test.go diff --git a/cli.go b/cli.go index 771d5f7..79292a9 100644 --- a/cli.go +++ b/cli.go @@ -90,6 +90,11 @@ func runCommand(args []string) bool { // gameIDRe matches a marketplace id, e.g. "aviorstudio/brickough". var gameIDRe = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*/[a-z0-9][a-z0-9-]*$`) +// slugOnlyRe matches half of one — "brickough" with the author left off. The +// character class is gameIDRe's without the slash, so nothing with a path, a +// scheme, or a file extension can reach it. +var slugOnlyRe = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*$`) + func cmdAdd(args []string) error { if len(args) != 1 { return fmt.Errorf("usage: termcade add ") @@ -105,6 +110,16 @@ func cmdAdd(args []string) error { return fmt.Errorf("versions cannot be pinned — `termcade add %s` installs what %s currently ships", id, id) } + // Forgetting the author is the likeliest way to get this wrong, and + // "open asteroid: no such file or directory" answers a question about the + // filesystem that nobody asked. Anything with a slash, a scheme, or a + // .tcade on the end meant a file or a URL and is left alone. + if slugOnlyRe.MatchString(src) { + if _, statErr := os.Stat(src); statErr != nil { + return fmt.Errorf("%q is missing an author — marketplace ids look like author/slug, e.g. aviorstudio/%s", src, src) + } + } + session, err := registry.LoadSession() if err != nil { return err @@ -271,7 +286,16 @@ func cmdList() error { rt := plugin.NewRuntime(context.Background()) defer rt.Close() - for _, g := range discoverGames(rt) { + games := discoverGames(rt) + // The TUI has empty states everywhere and the CLI had none, so removing + // your last game made `list` answer with nothing at all — which reads as + // a broken command rather than an empty arcade. + if len(games) == 0 { + fmt.Println("no games installed — run `termcade` and press m for the marketplace") + return nil + } + + for _, g := range games { if g.Err != nil { fmt.Printf("%-24s %-28s broken: %v\n", g.Info.Title, g.Info.ID, g.Err) continue diff --git a/internal/engine/safe_test.go b/internal/engine/safe_test.go new file mode 100644 index 0000000..8c460b9 --- /dev/null +++ b/internal/engine/safe_test.go @@ -0,0 +1,187 @@ +package engine + +import ( + "errors" + "strings" + "testing" + + "github.com/aviorstudio/termcade/sdk" +) + +// panicAt is a game that panics in exactly one method and counts every call +// it receives, so a test can prove the calls after a crash never arrive. +type panicAt struct { + where string + calls map[string]int + closed bool + // closeErr is returned by Close; closePanics makes it panic instead. + closeErr error + closePanics bool +} + +func newPanicAt(where string) *panicAt { + return &panicAt{where: where, calls: map[string]int{}} +} + +func (g *panicAt) hit(what string) { + g.calls[what]++ + if g.where == what { + panic("boom in " + what) + } +} + +func (g *panicAt) Info() sdk.Info { + g.hit("Info") + return sdk.Info{ID: "test/game", Title: "TEST", PixelW: 64, PixelH: 40} +} +func (g *panicAt) Reset() { g.hit("Reset") } +func (g *panicAt) HandleKey(sdk.Key) { g.hit("HandleKey") } +func (g *panicAt) HandleKeyUp(sdk.Key) { g.hit("HandleKeyUp") } +func (g *panicAt) Draw(c *sdk.Canvas) { g.hit("Draw") } +func (g *panicAt) Update() sdk.Status { g.hit("Update"); return sdk.StatusRunning } +func (g *panicAt) Score() int { g.hit("Score"); return 7 } +func (g *panicAt) HUD() sdk.HUD { g.hit("HUD"); return sdk.HUD{} } +func (g *panicAt) Close() error { + g.closed = true + if g.closePanics { + panic("boom in Close") + } + return g.closeErr +} + +// The claim the README makes: a broken game shows a crash screen instead of +// taking the arcade down. Every method has to hold that, not just the ones +// that happen to be covered elsewhere. +func TestSafeGameContainsAPanicInEveryMethod(t *testing.T) { + for _, where := range []string{"Reset", "HandleKey", "HandleKeyUp", "Draw", "Update", "Score", "HUD"} { + t.Run(where, func(t *testing.T) { + s := Safe(newPanicAt(where)) + if s.Err() != nil { + t.Fatalf("wrapping alone crashed: %v", s.Err()) + } + + // Whichever method panics, driving the whole surface must not + // propagate it — this call is what would take the arcade down. + drive(s) + + if s.Err() == nil { + t.Fatalf("a panic in %s was swallowed without latching an error", where) + } + if got := s.Err().Error(); !strings.Contains(got, where) { + t.Errorf("error %q does not name the method that failed", got) + } + }) + } +} + +// A panic during Info is the awkward case: it happens inside Safe itself, +// before the caller holds anything. +func TestSafeGamePanicInInfoIsContained(t *testing.T) { + s := Safe(newPanicAt("Info")) + if s.Err() == nil { + t.Fatal("a panic in Info was swallowed") + } + // Info is still answerable, with a zero value rather than a crash, so a + // caller can render a crash screen without a second check. + _ = s.Info() + drive(s) +} + +// Latching is what stops a game that panics every frame from burning the +// arcade's time: the first crash wins and nothing reaches the game again. +func TestSafeGameStopsCallingACrashedGame(t *testing.T) { + g := newPanicAt("Update") + s := Safe(g) + + s.Update() + first := s.Err() + if first == nil { + t.Fatal("no error latched") + } + callsAtCrash := g.calls["Update"] + + for range 5 { + s.Update() + s.Draw(nil) + s.HandleKey(sdk.KeyA) + } + + if g.calls["Update"] != callsAtCrash { + t.Errorf("a crashed game was called %d more times", g.calls["Update"]-callsAtCrash) + } + if g.calls["Draw"] != 0 || g.calls["HandleKey"] != 0 { + t.Errorf("calls reached a crashed game: %v", g.calls) + } + if s.Err() != first { + t.Error("a later call replaced the original crash; the first one is the useful one") + } +} + +// Cleanup runs after a crash, which is exactly when it matters — a wasm +// instance still needs releasing. +func TestSafeGameClosesAfterACrash(t *testing.T) { + g := newPanicAt("Update") + s := Safe(g) + s.Update() + + if err := s.Close(); err != nil { + t.Fatalf("Close after a crash: %v", err) + } + if !g.closed { + t.Error("a crashed game was never closed") + } +} + +func TestSafeGameContainsAPanicInClose(t *testing.T) { + g := newPanicAt("") + g.closePanics = true + + err := Safe(g).Close() + if err == nil { + t.Fatal("a panic in Close escaped as success") + } + if !strings.Contains(err.Error(), "Close") { + t.Errorf("error %q does not name Close", err) + } +} + +func TestSafeGameReportsACloseError(t *testing.T) { + sentinel := errors.New("released badly") + g := newPanicAt("") + g.closeErr = sentinel + + if err := Safe(g).Close(); !errors.Is(err, sentinel) { + t.Fatalf("Close returned %v, want the game's own error", err) + } +} + +// A healthy game is left alone: containment must not cost correctness. +func TestSafeGamePassesThroughWhenHealthy(t *testing.T) { + g := newPanicAt("") + s := Safe(g) + + if got := s.Info().ID; got != "test/game" { + t.Errorf("Info().ID = %q", got) + } + if got := s.Update(); got != sdk.StatusRunning { + t.Errorf("Update() = %v, want StatusRunning", got) + } + if got := s.Score(); got != 7 { + t.Errorf("Score() = %d, want 7", got) + } + if s.Err() != nil { + t.Errorf("a healthy game latched %v", s.Err()) + } +} + +// drive calls every method that can panic. Draw gets a nil canvas on purpose: +// the wrapper must not care what the game does with it. +func drive(s *SafeGame) { + s.Reset() + s.HandleKey(sdk.KeyA) + s.HandleKeyUp(sdk.KeyA) + s.Draw(nil) + s.Update() + s.Score() + s.HUD() +} diff --git a/internal/registry/client.go b/internal/registry/client.go index 586c02f..4e0b75f 100644 --- a/internal/registry/client.go +++ b/internal/registry/client.go @@ -1,11 +1,12 @@ // Package registry is the termca.de client: marketplace catalog, package // resolution, account auth, and the user's library. // -// The registry stores no packages. It is an index that answers "where does -// this version live, and what must it hash to" — the bytes come from the -// GitHub release the author published, and are verified here against the -// digest the registry recorded when it validated them. Browsing and -// installing are anonymous; account operations send the session token. +// The registry stores no packages. It is an index that answers "which release +// should I install, and what must it hash to" — the bytes originate from the +// GitHub release the author published, arrive through the registry, and are +// verified here against the digest it recorded when it validated them. +// Browsing is anonymous; installing and account operations send the session +// token. package registry import ( @@ -40,6 +41,26 @@ const maxPackageSize = 64 << 20 // ErrLoginRequired distinguishes "you need an account" from real failures. var ErrLoginRequired = errors.New("login required") +// ErrUnreachable is the marketplace not answering at all: no network, or +// nothing listening where the registry is supposed to be. +// +// It carries no transport detail. Go's is accurate and useless to a player — +// `Get "http://127.0.0.1:8080/v1/games/aviorstudio/tetris/resolve?abi=1": +// dial tcp 127.0.0.1:8080: connect: connection refused` names a host, a port, +// a query string and a syscall, none of which anyone can act on, and the +// arcade renders it into a TUI notice. TERMCADE_DEBUG puts it back for +// whoever is actually debugging. +var ErrUnreachable = errors.New("the marketplace is not answering — check your connection and try again") + +// unreachable wraps a transport failure as ErrUnreachable, keeping the +// original only when someone asked for it. +func unreachable(err error) error { + if os.Getenv("TERMCADE_DEBUG") != "" { + return fmt.Errorf("%w: %v", ErrUnreachable, err) + } + return ErrUnreachable +} + // Game is one marketplace catalog entry. The version and requirements are // those of its newest release; a game with none reports has_package false. type Game struct { @@ -133,7 +154,7 @@ func (c *Client) do(method, path string, body, out any) error { } resp, err := c.http.Do(req) if err != nil { - return fmt.Errorf("registry unreachable: %w", err) + return unreachable(err) } defer resp.Body.Close() @@ -141,12 +162,18 @@ func (c *Client) do(method, path string, body, out any) error { return ErrLoginRequired } if resp.StatusCode < 200 || resp.StatusCode > 299 { + // The registry writes its own messages for a person to read, so they + // are passed through unprefixed — the CLI already says "termcade:", + // and "termcade: registry: ..." is a stutter, not attribution. var msg apiMessage raw, _ := io.ReadAll(io.LimitReader(resp.Body, 8<<10)) if json.Unmarshal(raw, &msg) == nil && msg.Message != "" { - return fmt.Errorf("registry: %s", msg.Message) + return errors.New(msg.Message) + } + if resp.StatusCode >= 500 { + return fmt.Errorf("the marketplace is having trouble (HTTP %d) — try again shortly", resp.StatusCode) } - return fmt.Errorf("registry: HTTP %d", resp.StatusCode) + return fmt.Errorf("the marketplace refused that request (HTTP %d)", resp.StatusCode) } if out == nil { return nil @@ -211,7 +238,7 @@ func (c *Client) Download(author, slug string) (string, error) { resp, err := c.http.Do(req) if err != nil { - return "", fmt.Errorf("registry unreachable: %w", err) + return "", unreachable(err) } defer resp.Body.Close() if resp.StatusCode == http.StatusUnauthorized { @@ -221,9 +248,12 @@ func (c *Client) Download(author, slug string) (string, error) { var msg apiMessage raw, _ := io.ReadAll(io.LimitReader(resp.Body, 8<<10)) if json.Unmarshal(raw, &msg) == nil && msg.Message != "" { - return "", fmt.Errorf("downloading %s: %s", resolved.ID, msg.Message) + return "", fmt.Errorf("could not download %s: %s", resolved.ID, msg.Message) + } + if resp.StatusCode >= 500 { + return "", fmt.Errorf("could not download %s: the marketplace is having trouble — try again shortly", resolved.ID) } - return "", fmt.Errorf("downloading %s: HTTP %d", resolved.ID, resp.StatusCode) + return "", fmt.Errorf("could not download %s (HTTP %d)", resolved.ID, resp.StatusCode) } tmp, err := os.CreateTemp("", slug+"-*.tcade") diff --git a/internal/shell/market.go b/internal/shell/market.go index 43731ec..8fa45f0 100644 --- a/internal/shell/market.go +++ b/internal/shell/market.go @@ -121,7 +121,10 @@ func (m Model) updateMarketMsg(msg tea.Msg) (Model, tea.Cmd, bool) { m.market.busy = false m.market.loaded = true if msg.err != nil { - m.market.notice = "marketplace unreachable: " + msg.err.Error() + // Unprefixed: the client writes messages meant to be read, and + // "marketplace unreachable: the marketplace is not answering" + // says it twice. + m.market.notice = msg.err.Error() return m, nil, true } m.market.games = msg.games