From 77682ece386ff2bb3d573c63eb646c155c5c2d72 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 11:33:28 -0400 Subject: [PATCH 01/29] feat(trogonstack-elixir): add trogonstack-elixir plugin with GenServer design skill Signed-off-by: Yordis Prieto --- .claude-plugin/marketplace.json | 6 + .github/release-please-config.json | 3 + .github/release-please-manifest.json | 3 +- .../.claude-plugin/plugin.json | 9 ++ plugins/trogonstack-elixir/README.md | 7 + .../skills/elixir-design-genserver/SKILL.md | 141 ++++++++++++++++++ 6 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 plugins/trogonstack-elixir/.claude-plugin/plugin.json create mode 100644 plugins/trogonstack-elixir/README.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 9254a61..a032986 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -60,6 +60,12 @@ "description": "Skills for writing and reviewing Product Requirements Documents (PRDs) and Feature Requirements Documents (FRDs)", "source": "./plugins/trogonstack-product-requirements", "category": "development" + }, + { + "name": "trogonstack-elixir", + "description": "Elixir skills for designing and reviewing OTP processes such as GenServers", + "source": "./plugins/trogonstack-elixir", + "category": "development" } ] } diff --git a/.github/release-please-config.json b/.github/release-please-config.json index 4e54971..32473e3 100644 --- a/.github/release-please-config.json +++ b/.github/release-please-config.json @@ -50,6 +50,9 @@ }, "plugins/trogonstack-product-requirements": { "component": "trogonstack-product-requirements" + }, + "plugins/trogonstack-elixir": { + "component": "trogonstack-elixir" } }, "plugins": [ diff --git a/.github/release-please-manifest.json b/.github/release-please-manifest.json index f18a564..93ad411 100644 --- a/.github/release-please-manifest.json +++ b/.github/release-please-manifest.json @@ -7,5 +7,6 @@ "plugins/trogonstack-otel": "0.1.2", "plugins/trogonstack-eventmodeling": "0.1.2", "plugins/trogonstack-eda": "0.1.2", - "plugins/trogonstack-product-requirements": "0.1.1" + "plugins/trogonstack-product-requirements": "0.1.1", + "plugins/trogonstack-elixir": "0.0.0" } diff --git a/plugins/trogonstack-elixir/.claude-plugin/plugin.json b/plugins/trogonstack-elixir/.claude-plugin/plugin.json new file mode 100644 index 0000000..d3ab4f6 --- /dev/null +++ b/plugins/trogonstack-elixir/.claude-plugin/plugin.json @@ -0,0 +1,9 @@ +{ + "name": "trogonstack-elixir", + "description": "Elixir skills for designing and reviewing OTP processes such as GenServers", + "version": "0.0.0", + "author": { + "name": "TrogonStack", + "url": "https://github.com/TrogonStack" + } +} diff --git a/plugins/trogonstack-elixir/README.md b/plugins/trogonstack-elixir/README.md new file mode 100644 index 0000000..29b32c2 --- /dev/null +++ b/plugins/trogonstack-elixir/README.md @@ -0,0 +1,7 @@ +# trogonstack-elixir + +Elixir skills for designing and reviewing OTP processes such as GenServers. + +```bash +claude plugin install trogonstack-elixir@trogonstack +``` diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md new file mode 100644 index 0000000..53b9445 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -0,0 +1,141 @@ +--- +name: elixir-design-genserver +description: "Design or review Elixir GenServer modules. Decides whether a process is needed at all, shapes the client API, picks call vs cast, handles slow initialization, timeouts, and unexpected messages, and places the server under supervision. Use when writing a new GenServer, reviewing an existing one, or refactoring a process that became a bottleneck. Do not use for: (1) designing full supervision trees across an application, (2) distributed Erlang or clustering, (3) Phoenix LiveView or Channel processes, (4) GenStage, Broadway, or Flow pipelines." +allowed-tools: + - AskUserQuestion + - Read + - Write + - Shell +--- + +# Design an Elixir GenServer + +Design GenServers that exist for a runtime reason, expose a small client API, keep business logic out of callbacks, and fail in ways their supervisor can recover from. + +## Core Principle + +Use a process to model runtime concerns (shared mutable state, concurrency, fault isolation, lifecycle), never to organize code. Code organization belongs in modules and functions. + +## Do You Need a GenServer? + +Answer before writing any callback: + +| Need | Use | +|------|-----| +| Pure transformation of data | Plain module and functions | +| One-off concurrent work, result awaited | `Task` / `Task.async_stream` | +| Fire-and-forget work that must survive the caller | `Task.Supervisor.start_child` | +| Read-heavy shared data, many concurrent readers | ETS table (owned by a process) or `:persistent_term` for rarely changing data | +| Simple shared state with no logic | `Agent` (rarely worth it over a GenServer) | +| Shared state with logic, serialized access, lifecycle, or periodic work | `GenServer` | +| Many processes of the same kind, looked up by key | `GenServer` + `Registry` + `DynamicSupervisor` | + +A GenServer handles one message at a time. Every call through a single named server is serialized, so a global singleton on a hot path is a bottleneck by design. + +## Module Layout + +Keep three layers in one module, or split the pure core into its own module when it grows: + +```elixir +defmodule MyApp.RateLimiter do + use GenServer + + alias MyApp.RateLimiter.Bucket + + # Client API + + def start_link(opts) do + {name, opts} = Keyword.pop(opts, :name, __MODULE__) + GenServer.start_link(__MODULE__, opts, name: name) + end + + def allow?(server \\ __MODULE__, key) do + GenServer.call(server, {:allow?, key}) + end + + # Server callbacks + + @impl GenServer + def init(opts) do + {:ok, Bucket.new(opts)} + end + + @impl GenServer + def handle_call({:allow?, key}, _from, bucket) do + {result, bucket} = Bucket.take(bucket, key) + {:reply, result, bucket} + end +end +``` + +Rules: + +- Callers never call `GenServer.call/cast` directly. The client API is the contract; message shapes are private. +- Callbacks delegate to pure functions (`Bucket.take/2` above) that take state and return new state. Test those without a process. +- Mark every callback with `@impl GenServer`. +- Accept `:name` (and the server reference in client functions) so tests can start isolated instances. +- Hold state in a struct, not a loose map, so its shape is documented and enforced. + +## call vs cast + +- Default to `call`. It gives back-pressure, surfaces errors to the caller, and confirms the work happened. +- Use `cast` only when the caller truly does not care about the outcome and producers cannot outrun the server. Unbounded casts grow the mailbox until the node runs out of memory. +- Use `send/2` + `handle_info/2` for messages from things that are not your client API: timers, monitors, ports, other libraries. + +## Initialization + +`init/1` blocks the caller of `start_link`, which during boot means it blocks the supervisor and every sibling started after it. + +- Keep `init/1` fast. Return `{:ok, state, {:continue, :load}}` and do slow work (DB reads, network) in `handle_continue/2`. +- Do not hide failures: if the server cannot work without the loaded data, let `handle_continue` crash so the supervisor restarts it. +- Return `:ignore` when configuration disables the server, rather than starting an idle process. + +## Timeouts and Long Work + +- `GenServer.call/3` defaults to a 5 second timeout and exits the caller on expiry. Set an explicit timeout when work is known to be slow, and question why it is slow inside a serialized process. +- Do not block the server on slow I/O. Offload to a `Task` under a `Task.Supervisor`, reply later with `GenServer.reply/2` (return `{:noreply, state}` from `handle_call` and keep `from`), and handle the task result in `handle_info/2`. +- For periodic work use `Process.send_after/3` and reschedule inside `handle_info/2`. Avoid `:timer.send_interval/2`, which keeps firing even when the server falls behind. + +## Unexpected Messages + +`use GenServer` injects a default `handle_info/2` that logs unexpected messages. Once you define your own `handle_info/2`, that default is gone, so add a catch-all clause that logs and keeps state, otherwise an unknown message crashes the server with a `FunctionClauseError`. + +```elixir +@impl GenServer +def handle_info(msg, state) do + Logger.warning("unexpected message: #{inspect(msg)}") + {:noreply, state} +end +``` + +## Naming and Discovery + +- Singleton: `name: __MODULE__`. Only for genuinely one-per-node services. +- Many instances: `name: {:via, Registry, {MyApp.Registry, key}}`, started under a `DynamicSupervisor`. +- Never create atoms from user input to name processes. Atoms are not garbage collected. + +## Supervision + +- `use GenServer` generates `child_spec/1`. Override with `use GenServer, restart: :transient` (or `:temporary`) when the default `:permanent` is wrong. +- `:permanent`: always restart. `:transient`: restart only on abnormal exit. `:temporary`: never restart. +- Trap exits (`Process.flag(:trap_exit, true)`) only when you must run `terminate/2` for cleanup, and set an explicit `shutdown` in the child spec. `terminate/2` is not guaranteed to run otherwise. +- State is lost on restart. If it must survive a crash, persist it or rebuild it in `handle_continue`. + +## Testing + +- Test the pure core directly, without processes. +- Start servers with `start_supervised!/1` so ExUnit stops them between tests, and pass a unique `:name` to keep tests `async: true`. +- Do not assert on internal state with `:sys.get_state/1` except as a last resort; assert through the client API. + +## Review Checklist + +- [ ] A process is justified by a runtime concern, not code organization +- [ ] Client API wraps every `call`/`cast`; message tuples never leak +- [ ] Callbacks delegate to pure, separately tested functions +- [ ] `call` is the default; every `cast` has a reason +- [ ] `init/1` is fast; slow work moved to `handle_continue/2` +- [ ] No blocking I/O inside callbacks on a hot path +- [ ] `handle_info/2` has a catch-all clause +- [ ] Restart strategy matches the process lifecycle +- [ ] Process names never built from untrusted input +- [ ] Tests use `start_supervised!/1` with isolated names From b24b94a666010aba1ab363fe46a84f45fd689ff3 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 11:58:56 -0400 Subject: [PATCH 02/29] fix(trogonstack-elixir): set initial version to 0.0.1 Signed-off-by: Yordis Prieto --- .github/release-please-manifest.json | 2 +- plugins/trogonstack-elixir/.claude-plugin/plugin.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/release-please-manifest.json b/.github/release-please-manifest.json index 93ad411..e684f48 100644 --- a/.github/release-please-manifest.json +++ b/.github/release-please-manifest.json @@ -8,5 +8,5 @@ "plugins/trogonstack-eventmodeling": "0.1.2", "plugins/trogonstack-eda": "0.1.2", "plugins/trogonstack-product-requirements": "0.1.1", - "plugins/trogonstack-elixir": "0.0.0" + "plugins/trogonstack-elixir": "0.0.1" } diff --git a/plugins/trogonstack-elixir/.claude-plugin/plugin.json b/plugins/trogonstack-elixir/.claude-plugin/plugin.json index d3ab4f6..a08d385 100644 --- a/plugins/trogonstack-elixir/.claude-plugin/plugin.json +++ b/plugins/trogonstack-elixir/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "trogonstack-elixir", "description": "Elixir skills for designing and reviewing OTP processes such as GenServers", - "version": "0.0.0", + "version": "0.0.1", "author": { "name": "TrogonStack", "url": "https://github.com/TrogonStack" From 4791aa4d2e95071c5d54d6bf60e2944b77f7e387 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 12:01:05 -0400 Subject: [PATCH 03/29] feat(trogonstack-elixir): teach where a GenServer belongs and the queuing it introduces Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 130 ++++-------------- .../references/callback-patterns.md | 75 ++++++++++ .../references/mailbox-queuing.md | 78 +++++++++++ .../references/process-justification.md | 66 +++++++++ .../references/supervision-and-testing.md | 33 +++++ 5 files changed, 276 insertions(+), 106 deletions(-) create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index 53b9445..a76a501 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -1,6 +1,6 @@ --- name: elixir-design-genserver -description: "Design or review Elixir GenServer modules. Decides whether a process is needed at all, shapes the client API, picks call vs cast, handles slow initialization, timeouts, and unexpected messages, and places the server under supervision. Use when writing a new GenServer, reviewing an existing one, or refactoring a process that became a bottleneck. Do not use for: (1) designing full supervision trees across an application, (2) distributed Erlang or clustering, (3) Phoenix LiveView or Channel processes, (4) GenStage, Broadway, or Flow pipelines." +description: "Design or review Elixir GenServer modules. Decides whether a process is justified at all, detects mailbox queuing and serialization bottlenecks, shapes the client API, picks call vs cast, and places the server under supervision. Use when writing a new GenServer, reviewing an existing one, deciding where an actor belongs, or diagnosing a process that became a bottleneck. Do not use for: (1) designing full supervision trees across an application, (2) distributed Erlang or clustering, (3) Phoenix LiveView or Channel processes, (4) GenStage, Broadway, or Flow pipelines." allowed-tools: - AskUserQuestion - Read @@ -10,132 +10,50 @@ allowed-tools: # Design an Elixir GenServer -Design GenServers that exist for a runtime reason, expose a small client API, keep business logic out of callbacks, and fail in ways their supervisor can recover from. +Put a process only where the runtime needs one, and know what it costs: every GenServer is a queue with a single worker. ## Core Principle -Use a process to model runtime concerns (shared mutable state, concurrency, fault isolation, lifecycle), never to organize code. Code organization belongs in modules and functions. +A process is the unit of concurrency, of serialization, and of failure, all at once. Use one to model a runtime concern (state shared across callers, an owned resource, an independent lifecycle, fault isolation). Never use one to organize code; modules and functions do that. -## Do You Need a GenServer? +Wrapping an operation that needs none of those concerns in a GenServer does not add structure. It adds a queue that turns concurrent callers into sequential ones. -Answer before writing any callback: +## Workflow -| Need | Use | -|------|-----| -| Pure transformation of data | Plain module and functions | -| One-off concurrent work, result awaited | `Task` / `Task.async_stream` | -| Fire-and-forget work that must survive the caller | `Task.Supervisor.start_child` | -| Read-heavy shared data, many concurrent readers | ETS table (owned by a process) or `:persistent_term` for rarely changing data | -| Simple shared state with no logic | `Agent` (rarely worth it over a GenServer) | -| Shared state with logic, serialized access, lifecycle, or periodic work | `GenServer` | -| Many processes of the same kind, looked up by key | `GenServer` + `Registry` + `DynamicSupervisor` | +### 1. Justify the Process -A GenServer handles one message at a time. Every call through a single named server is serialized, so a global singleton on a hot path is a bottleneck by design. +Name what the process owns. If the answer is "the functions in this module", or the work is a DB query, HTTP request, or pure computation the caller could run itself, do not write a GenServer. -## Module Layout +Read [references/process-justification.md](references/process-justification.md) when the decision is not obvious, or when reviewing code that uses GenServers as a service layer. -Keep three layers in one module, or split the pure core into its own module when it grows: +### 2. Check the Queue -```elixir -defmodule MyApp.RateLimiter do - use GenServer +Estimate arrival rate and handling time on the hot path. A single server tops out at `1 / handling_time` messages per second regardless of core count. If callers can outpace it, the mailbox grows and latency grows with it. - alias MyApp.RateLimiter.Bucket +Read [references/mailbox-queuing.md](references/mailbox-queuing.md) when the server sits on a request path, receives casts from many producers, does I/O inside callbacks, or already shows timeouts or a growing `message_queue_len`. - # Client API +### 3. Shape the Module - def start_link(opts) do - {name, opts} = Keyword.pop(opts, :name, __MODULE__) - GenServer.start_link(__MODULE__, opts, name: name) - end +Client API wraps every `call`/`cast`, callbacks delegate to a pure core, `call` is the default, `init/1` stays fast, and `handle_info/2` has a catch-all. - def allow?(server \\ __MODULE__, key) do - GenServer.call(server, {:allow?, key}) - end +Read [references/callback-patterns.md](references/callback-patterns.md) when writing or reviewing the module body. - # Server callbacks +### 4. Supervise and Test - @impl GenServer - def init(opts) do - {:ok, Bucket.new(opts)} - end +Pick a restart strategy that matches the lifecycle, and test the pure core without processes. - @impl GenServer - def handle_call({:allow?, key}, _from, bucket) do - {result, bucket} = Bucket.take(bucket, key) - {:reply, result, bucket} - end -end -``` - -Rules: - -- Callers never call `GenServer.call/cast` directly. The client API is the contract; message shapes are private. -- Callbacks delegate to pure functions (`Bucket.take/2` above) that take state and return new state. Test those without a process. -- Mark every callback with `@impl GenServer`. -- Accept `:name` (and the server reference in client functions) so tests can start isolated instances. -- Hold state in a struct, not a loose map, so its shape is documented and enforced. - -## call vs cast - -- Default to `call`. It gives back-pressure, surfaces errors to the caller, and confirms the work happened. -- Use `cast` only when the caller truly does not care about the outcome and producers cannot outrun the server. Unbounded casts grow the mailbox until the node runs out of memory. -- Use `send/2` + `handle_info/2` for messages from things that are not your client API: timers, monitors, ports, other libraries. - -## Initialization - -`init/1` blocks the caller of `start_link`, which during boot means it blocks the supervisor and every sibling started after it. - -- Keep `init/1` fast. Return `{:ok, state, {:continue, :load}}` and do slow work (DB reads, network) in `handle_continue/2`. -- Do not hide failures: if the server cannot work without the loaded data, let `handle_continue` crash so the supervisor restarts it. -- Return `:ignore` when configuration disables the server, rather than starting an idle process. - -## Timeouts and Long Work - -- `GenServer.call/3` defaults to a 5 second timeout and exits the caller on expiry. Set an explicit timeout when work is known to be slow, and question why it is slow inside a serialized process. -- Do not block the server on slow I/O. Offload to a `Task` under a `Task.Supervisor`, reply later with `GenServer.reply/2` (return `{:noreply, state}` from `handle_call` and keep `from`), and handle the task result in `handle_info/2`. -- For periodic work use `Process.send_after/3` and reschedule inside `handle_info/2`. Avoid `:timer.send_interval/2`, which keeps firing even when the server falls behind. - -## Unexpected Messages - -`use GenServer` injects a default `handle_info/2` that logs unexpected messages. Once you define your own `handle_info/2`, that default is gone, so add a catch-all clause that logs and keeps state, otherwise an unknown message crashes the server with a `FunctionClauseError`. - -```elixir -@impl GenServer -def handle_info(msg, state) do - Logger.warning("unexpected message: #{inspect(msg)}") - {:noreply, state} -end -``` - -## Naming and Discovery - -- Singleton: `name: __MODULE__`. Only for genuinely one-per-node services. -- Many instances: `name: {:via, Registry, {MyApp.Registry, key}}`, started under a `DynamicSupervisor`. -- Never create atoms from user input to name processes. Atoms are not garbage collected. - -## Supervision - -- `use GenServer` generates `child_spec/1`. Override with `use GenServer, restart: :transient` (or `:temporary`) when the default `:permanent` is wrong. -- `:permanent`: always restart. `:transient`: restart only on abnormal exit. `:temporary`: never restart. -- Trap exits (`Process.flag(:trap_exit, true)`) only when you must run `terminate/2` for cleanup, and set an explicit `shutdown` in the child spec. `terminate/2` is not guaranteed to run otherwise. -- State is lost on restart. If it must survive a crash, persist it or rebuild it in `handle_continue`. - -## Testing - -- Test the pure core directly, without processes. -- Start servers with `start_supervised!/1` so ExUnit stops them between tests, and pass a unique `:name` to keep tests `async: true`. -- Do not assert on internal state with `:sys.get_state/1` except as a last resort; assert through the client API. +Read [references/supervision-and-testing.md](references/supervision-and-testing.md) when wiring the child spec, handling cleanup, or writing tests. ## Review Checklist -- [ ] A process is justified by a runtime concern, not code organization -- [ ] Client API wraps every `call`/`cast`; message tuples never leak -- [ ] Callbacks delegate to pure, separately tested functions -- [ ] `call` is the default; every `cast` has a reason -- [ ] `init/1` is fast; slow work moved to `handle_continue/2` +- [ ] The process owns something concrete: shared state, a resource, a lifecycle, or a failure boundary +- [ ] No stateless operation (DB, HTTP, computation) is routed through it +- [ ] Hot-path throughput fits within `1 / handling_time` of one process, or the work is partitioned +- [ ] No unbounded casts from producers that can outrun the server - [ ] No blocking I/O inside callbacks on a hot path +- [ ] Client API wraps every message; message tuples never leak +- [ ] Callbacks delegate to pure, separately tested functions +- [ ] `init/1` is fast; slow work runs in `handle_continue/2` - [ ] `handle_info/2` has a catch-all clause - [ ] Restart strategy matches the process lifecycle - [ ] Process names never built from untrusted input -- [ ] Tests use `start_supervised!/1` with isolated names diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md new file mode 100644 index 0000000..f795d39 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -0,0 +1,75 @@ +# Callback Patterns + +## Module Layout + +```elixir +defmodule MyApp.RateLimiter do + use GenServer + + alias MyApp.RateLimiter.Bucket + + def start_link(opts) do + {name, opts} = Keyword.pop(opts, :name, __MODULE__) + GenServer.start_link(__MODULE__, opts, name: name) + end + + def allow?(server \\ __MODULE__, key) do + GenServer.call(server, {:allow?, key}) + end + + @impl GenServer + def init(opts) do + {:ok, Bucket.new(opts)} + end + + @impl GenServer + def handle_call({:allow?, key}, _from, bucket) do + {result, bucket} = Bucket.take(bucket, key) + {:reply, result, bucket} + end +end +``` + +- Callers never call `GenServer.call/cast` directly. The client API is the contract; message shapes are private. +- Callbacks delegate to pure functions (`Bucket.take/2`) that take state and return new state. +- Mark every callback with `@impl GenServer`. +- Accept `:name`, and a server reference in client functions, so tests can start isolated instances. +- Hold state in a struct so its shape is documented and enforced. + +## call vs cast vs send + +- `call`: the default. Back-pressure, errors reach the caller, the work is confirmed. +- `cast`: only when the caller does not care about the outcome and producers cannot outrun the server. See [mailbox-queuing.md](mailbox-queuing.md). +- `send/2` + `handle_info/2`: messages that are not part of the client API, such as timers, monitors, ports, and other libraries. + +## Initialization + +`init/1` blocks the caller of `start_link`; during boot that is the supervisor and every sibling after it. + +- Return `{:ok, state, {:continue, :load}}` and do slow work in `handle_continue/2`. +- If the server cannot work without the loaded data, let `handle_continue/2` crash so the supervisor restarts it. +- Return `:ignore` when configuration disables the server. + +## Timeouts and Long Work + +- `GenServer.call/3` defaults to 5 seconds and exits the caller on expiry. A slow call inside a serialized process is a design question before it is a timeout question. +- Offload slow work to a `Task` and reply later with `GenServer.reply/2`. +- For periodic work use `Process.send_after/3` and reschedule inside `handle_info/2`. `:timer.send_interval/2` keeps firing even when the server falls behind, which piles messages into the mailbox. + +## Unexpected Messages + +`use GenServer` injects a `handle_info/2` that logs unexpected messages. Defining your own replaces it, so add a catch-all clause or an unknown message crashes the server with a `FunctionClauseError`. + +```elixir +@impl GenServer +def handle_info(msg, state) do + Logger.warning("unexpected message: #{inspect(msg)}") + {:noreply, state} +end +``` + +## Naming + +- Singleton: `name: __MODULE__`, only for genuinely one-per-node services. A singleton on a request path is a global queue. +- Many instances: `name: {:via, Registry, {MyApp.Registry, key}}` under a `DynamicSupervisor`. +- Never create atoms from user input to name processes. Atoms are not garbage collected. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md new file mode 100644 index 0000000..57a7177 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md @@ -0,0 +1,78 @@ +# Mailbox Queuing + +## The Model + +A GenServer is a queue with exactly one worker: + +- Messages land in an unbounded FIFO mailbox. +- The server handles one message at a time, to completion, before taking the next. +- Adding cores, schedulers, or callers does not add workers. + +Throughput ceiling: `1 / average_handling_time`. A callback that takes 10 ms caps the server at 100 messages per second on a 64-core machine. + +Latency under load (Little's law): a message waits roughly `queue_length * average_handling_time` before it is handled. When arrival rate exceeds the ceiling, the queue grows without bound and so does latency. + +## How Queuing Fails + +### With `call` + +- Each caller blocks until every message ahead of it is handled. +- The default 5 second timeout exits the caller, but the request stays in the mailbox and the server still does the work, then the reply is discarded. Load does not drop when callers give up. +- Callers that retry on timeout add more messages to a queue that is already behind, so the backlog feeds itself. +- A server that calls another busy server inherits that server's queue as well. + +### With `cast` + +- No back-pressure: producers never slow down, so a slow server lets the mailbox grow until memory runs out. +- A large mailbox makes the process heap and garbage collection expensive, which slows handling further. +- Nothing tells the producer the server is behind; the first visible symptom is often memory alarms or the node going down. + +### Hidden Serialization + +The worst queues are the ones nobody meant to build: a singleton GenServer that wraps work which was already concurrent (DB pool, HTTP client, pure functions). Each request is fast in isolation, tests pass, and production latency grows with traffic. + +## Detecting It + +```elixir +Process.info(pid, :message_queue_len) +:sys.get_status(pid) +``` + +- `:recon.proc_count(:message_queue_len, 10)` lists the processes with the longest mailboxes. +- `:observer.start()` Processes tab, sorted by MsgQ. +- Telemetry: emit handling duration per callback and sample `message_queue_len` periodically. + +Symptoms: `GenServer.call` timeouts that rise with load, latency that grows linearly with traffic, one scheduler pegged while others idle, process memory climbing on a single pid. + +## Fixes, Least Invasive First + +1. **Remove the process.** If it owns nothing (see [process-justification.md](process-justification.md)), call the functions directly from the caller. +2. **Move reads out of the mailbox.** The server owns writes; readers go straight to ETS. + + ```elixir + :ets.new(__MODULE__, [:named_table, :protected, read_concurrency: true]) + + def lookup(key) do + case :ets.lookup(__MODULE__, key) do + [{^key, value}] -> {:ok, value} + [] -> :error + end + end + ``` + +3. **Move slow work out of the callback.** Start a `Task` under a `Task.Supervisor`, return `{:noreply, state}` while keeping `from`, and answer with `GenServer.reply/2` when the task result arrives in `handle_info/2`. The server keeps accepting messages while the work runs. +4. **Partition by key.** Run N copies and route each key to one, so unrelated keys stop waiting on each other. + + ```elixir + children = [ + {PartitionSupervisor, child_spec: MyApp.Counter, name: MyApp.Counters} + ] + + GenServer.call({:via, PartitionSupervisor, {MyApp.Counters, key}}, {:incr, key}) + ``` + +5. **One process per entity.** Use `Registry` + `DynamicSupervisor` when each entity has its own state and lifecycle (a chat room, a device session). +6. **Pool a scarce resource.** Use a pool (`NimblePool`, `:poolboy`) when the bottleneck is a limited external resource rather than state. +7. **Shed load.** When the queue cannot be avoided, bound it: check `message_queue_len` or a counter before enqueueing and reject early with `{:error, :overloaded}` instead of letting callers time out. + +Prefer `call` over `cast` from producers that can outrun the server: the blocked caller is the back-pressure. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md new file mode 100644 index 0000000..508c166 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md @@ -0,0 +1,66 @@ +# Where an Actor Belongs, and Why + +## The Question to Ask + +"What does this process own that cannot live in the caller?" + +Valid answers: + +| Owns | Example | +|------|---------| +| Mutable state shared by many callers that must change consistently | Rate limiter buckets, in-memory counters, a game room | +| A resource with its own lifecycle | Socket, port, file handle, external connection | +| Time | Periodic jobs, debouncing, expiring sessions | +| A failure boundary | Code that may crash and must not take the caller down | + +Invalid answers: + +- "It is the service layer for users / orders / billing." That is a module. +- "It encapsulates the logic." Modules encapsulate logic. +- "It wraps the database / HTTP client." Those calls are already concurrent and pooled; a GenServer in front serializes them. +- "It runs in the background." That is a `Task` under a `Task.Supervisor`. +- "It caches data." Reads belong in ETS or `:persistent_term`; a process may own the table, but readers should not message it. + +## Picking the Right Tool + +| Need | Use | +|------|-----| +| Pure transformation of data | Plain module and functions | +| DB query or HTTP request | Call it from the caller's process | +| One-off concurrent work, result awaited | `Task` / `Task.async_stream` | +| Fire-and-forget work that must survive the caller | `Task.Supervisor.start_child` | +| Read-heavy shared data | ETS table owned by a process, or `:persistent_term` for rarely changing data | +| Shared state with logic, lifecycle, or timers | `GenServer` | +| One process per entity, looked up by key | `GenServer` + `Registry` + `DynamicSupervisor` | +| Stateless server that is a bottleneck | `PartitionSupervisor` (Elixir 1.14+) | + +## The Anti-Pattern + +```elixir +defmodule MyApp.Users do + use GenServer + + def get(id), do: GenServer.call(__MODULE__, {:get, id}) + + @impl GenServer + def handle_call({:get, id}, _from, state) do + {:reply, Repo.get(User, id), state} + end +end +``` + +The state is never used. Every request in the system now waits in one mailbox for every other `get/1` ahead of it, while the Repo connection pool sits mostly idle. The fix is deleting the process: + +```elixir +defmodule MyApp.Users do + def get(id), do: Repo.get(User, id) +end +``` + +## Smells in Review + +- `state` is ignored or is `nil` / `%{}` forever +- Every `handle_call` clause is a thin pass-through to another module +- The module is a named singleton and every web request calls it +- The GenServer exists "for consistency" with other modules +- Removing `use GenServer` and the callbacks would change nothing but concurrency diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md new file mode 100644 index 0000000..0ff65c4 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md @@ -0,0 +1,33 @@ +# Supervision and Testing + +## Restart Strategy + +`use GenServer` generates `child_spec/1` with `restart: :permanent`. Override it when that is wrong: + +```elixir +use GenServer, restart: :transient +``` + +| Strategy | Restarts when | Fits | +|----------|---------------|------| +| `:permanent` | Always | Long-lived services | +| `:transient` | Abnormal exit only | Work that can finish normally, per-entity processes | +| `:temporary` | Never | One-shot processes whose failure the caller handles | + +## Cleanup + +- `terminate/2` runs on supervisor shutdown only if the process traps exits (`Process.flag(:trap_exit, true)`) and finishes within the child spec `shutdown` value. +- Trap exits only when cleanup is required, and set an explicit `shutdown`. +- Do not rely on `terminate/2` for anything that must happen; a `:kill` skips it. + +## State Across Restarts + +A restart starts from `init/1` with fresh state. If state must survive a crash, persist it, or rebuild it in `handle_continue/2` from a durable source. + +## Testing + +- Test the pure core directly, without processes. Most logic lives there. +- Start servers with `start_supervised!/1` so ExUnit stops them between tests. +- Pass a unique `:name` per test to keep `async: true`. +- Assert through the client API. Use `:sys.get_state/1` only as a last resort. +- For queuing behavior, measure handling time of the pure core and reason with `1 / handling_time` rather than load-testing the process in unit tests. From b0b3fe3ca65c195b3b3c4420917ce666a567dbdc Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 12:03:54 -0400 Subject: [PATCH 04/29] refactor(trogonstack-elixir): load the GenServer review checklist only when reviewing Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 16 +++---------- .../references/review-checklist.md | 24 +++++++++++++++++++ 2 files changed, 27 insertions(+), 13 deletions(-) create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index a76a501..ea65791 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -44,16 +44,6 @@ Pick a restart strategy that matches the lifecycle, and test the pure core witho Read [references/supervision-and-testing.md](references/supervision-and-testing.md) when wiring the child spec, handling cleanup, or writing tests. -## Review Checklist - -- [ ] The process owns something concrete: shared state, a resource, a lifecycle, or a failure boundary -- [ ] No stateless operation (DB, HTTP, computation) is routed through it -- [ ] Hot-path throughput fits within `1 / handling_time` of one process, or the work is partitioned -- [ ] No unbounded casts from producers that can outrun the server -- [ ] No blocking I/O inside callbacks on a hot path -- [ ] Client API wraps every message; message tuples never leak -- [ ] Callbacks delegate to pure, separately tested functions -- [ ] `init/1` is fast; slow work runs in `handle_continue/2` -- [ ] `handle_info/2` has a catch-all clause -- [ ] Restart strategy matches the process lifecycle -- [ ] Process names never built from untrusted input +## Reviewing an Existing GenServer + +Read [references/review-checklist.md](references/review-checklist.md) when reviewing or auditing GenServer code. Each section links to the reference that explains a failing item. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md new file mode 100644 index 0000000..81837ae --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -0,0 +1,24 @@ +# Review Checklist + +## Justification ([process-justification.md](process-justification.md)) + +- [ ] The process owns something concrete: shared state, a resource, a lifecycle, or a failure boundary +- [ ] No stateless operation (DB, HTTP, computation) is routed through it + +## Queuing ([mailbox-queuing.md](mailbox-queuing.md)) + +- [ ] Hot-path throughput fits within `1 / handling_time` of one process, or the work is partitioned +- [ ] No unbounded casts from producers that can outrun the server +- [ ] No blocking I/O inside callbacks on a hot path + +## Module Shape ([callback-patterns.md](callback-patterns.md)) + +- [ ] Client API wraps every message; message tuples never leak +- [ ] Callbacks delegate to pure, separately tested functions +- [ ] `init/1` is fast; slow work runs in `handle_continue/2` +- [ ] `handle_info/2` has a catch-all clause +- [ ] Process names never built from untrusted input + +## Supervision ([supervision-and-testing.md](supervision-and-testing.md)) + +- [ ] Restart strategy matches the process lifecycle From b1a825e28b2ecb84e121688c8458b1833cba5b10 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 12:28:21 -0400 Subject: [PATCH 05/29] fix(trogonstack-elixir): cover dependent siblings and restart loops during GenServer startup Signed-off-by: Yordis Prieto --- .../elixir-design-genserver/references/callback-patterns.md | 3 ++- .../elixir-design-genserver/references/review-checklist.md | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index f795d39..bd229b1 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -47,7 +47,8 @@ end `init/1` blocks the caller of `start_link`; during boot that is the supervisor and every sibling after it. - Return `{:ok, state, {:continue, :load}}` and do slow work in `handle_continue/2`. -- If the server cannot work without the loaded data, let `handle_continue/2` crash so the supervisor restarts it. +- The supervisor starts the next sibling as soon as `init/1` returns, before `handle_continue/2` finishes. A sibling that calls the server during its own startup waits in the mailbox behind the load and can time out. When siblings need the loaded state at boot, keep loading in `init/1`, or expose an explicit readiness signal (a `ready?/0` call, a `Registry` entry, a `:persistent_term` flag) that they check. +- Crash from `handle_continue/2` only when a restart can fix the failure. A dependency that stays down (database, remote API) crashes the server repeatedly, exhausts the supervisor's restart intensity (3 restarts in 5 seconds by default), and takes the supervisor and its other children down with it. For those failures, stay up in a degraded state and retry with `Process.send_after/3` and backoff. - Return `:ignore` when configuration disables the server. ## Timeouts and Long Work diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index 81837ae..cd104ba 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -15,7 +15,8 @@ - [ ] Client API wraps every message; message tuples never leak - [ ] Callbacks delegate to pure, separately tested functions -- [ ] `init/1` is fast; slow work runs in `handle_continue/2` +- [ ] `init/1` is fast; slow work runs in `handle_continue/2`, unless siblings need the state at boot +- [ ] Persistent dependency failures retry with backoff instead of crash-looping the supervisor - [ ] `handle_info/2` has a catch-all clause - [ ] Process names never built from untrusted input From 01136a80f4124286c9b80fd0300907f724d200d6 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 12:39:16 -0400 Subject: [PATCH 06/29] fix(trogonstack-elixir): make the GenServer instance explicit in client APIs Signed-off-by: Yordis Prieto --- .../elixir-design-genserver/references/callback-patterns.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index bd229b1..fc78fa4 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -9,11 +9,11 @@ defmodule MyApp.RateLimiter do alias MyApp.RateLimiter.Bucket def start_link(opts) do - {name, opts} = Keyword.pop(opts, :name, __MODULE__) + {name, opts} = Keyword.pop!(opts, :name) GenServer.start_link(__MODULE__, opts, name: name) end - def allow?(server \\ __MODULE__, key) do + def allow?(server, key) do GenServer.call(server, {:allow?, key}) end @@ -33,7 +33,7 @@ end - Callers never call `GenServer.call/cast` directly. The client API is the contract; message shapes are private. - Callbacks delegate to pure functions (`Bucket.take/2`) that take state and return new state. - Mark every callback with `@impl GenServer`. -- Accept `:name`, and a server reference in client functions, so tests can start isolated instances. +- Require `:name` in `start_link/1` and take the server as the first argument of every client function. Callers and tests always say which instance they talk to; a default argument hides that choice. - Hold state in a struct so its shape is documented and enforced. ## call vs cast vs send From abe4dfc0b5e51c2d16e0a75aa9d12f3dea43a94c Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 12:48:41 -0400 Subject: [PATCH 07/29] feat(trogonstack-elixir): recommend names for the GenServer, its pure core, and their contract Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 2 + .../references/callback-patterns.md | 17 ++-- .../references/naming.md | 78 +++++++++++++++++++ .../references/review-checklist.md | 6 ++ 4 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index ea65791..373f224 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -38,6 +38,8 @@ Client API wraps every `call`/`cast`, callbacks delegate to a pure core, `call` Read [references/callback-patterns.md](references/callback-patterns.md) when writing or reviewing the module body. +Read [references/naming.md](references/naming.md) when naming the public module, the pure core, the process state, or the structs passed in and out of the core. + ### 4. Supervise and Test Pick a restart strategy that matches the lifecycle, and test the pure core without processes. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index fc78fa4..24c635c 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -6,7 +6,8 @@ defmodule MyApp.RateLimiter do use GenServer - alias MyApp.RateLimiter.Bucket + alias MyApp.RateLimiter.{Bucket, ServerState} + alias MyApp.RateLimiter.Bucket.{TakeRequest, TakeResponse} def start_link(opts) do {name, opts} = Keyword.pop!(opts, :name) @@ -19,22 +20,24 @@ defmodule MyApp.RateLimiter do @impl GenServer def init(opts) do - {:ok, Bucket.new(opts)} + {:ok, %ServerState{bucket: Bucket.new(opts)}} end @impl GenServer - def handle_call({:allow?, key}, _from, bucket) do - {result, bucket} = Bucket.take(bucket, key) - {:reply, result, bucket} + def handle_call({:allow?, key}, _from, %ServerState{} = state) do + %TakeResponse{allowed?: allowed?, bucket: bucket} = + Bucket.take(state.bucket, %TakeRequest{key: key}) + + {:reply, allowed?, %ServerState{state | bucket: bucket}} end end ``` - Callers never call `GenServer.call/cast` directly. The client API is the contract; message shapes are private. -- Callbacks delegate to pure functions (`Bucket.take/2`) that take state and return new state. +- Callbacks delegate to a pure core (`Bucket.take/2`) that takes structs in and returns structs out. See [naming.md](naming.md). - Mark every callback with `@impl GenServer`. - Require `:name` in `start_link/1` and take the server as the first argument of every client function. Callers and tests always say which instance they talk to; a default argument hides that choice. -- Hold state in a struct so its shape is documented and enforced. +- Hold process state in a `ServerState` struct so its shape is documented and enforced. ## call vs cast vs send diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md new file mode 100644 index 0000000..0df8300 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md @@ -0,0 +1,78 @@ +# Naming + +A GenServer usually involves a public API with process callbacks, a pure deterministic core, and the process state. Name each one after what it means, not where it sits. + +## Recommended Layout + +```text +MyApp.RateLimiter public API and GenServer callbacks +MyApp.RateLimiter.ServerState process bookkeeping, holds the core as a field +MyApp.RateLimiter.Bucket pure deterministic core, struct and functions +MyApp.RateLimiter.Bucket.TakeRequest input to Bucket.take/2 +MyApp.RateLimiter.Bucket.TakeResponse output of Bucket.take/2 +``` + +```elixir +defmodule MyApp.RateLimiter.Bucket do + alias MyApp.RateLimiter.Bucket.{TakeRequest, TakeResponse} + + defstruct [:capacity, :tokens] + + def take(%__MODULE__{} = bucket, %TakeRequest{key: key}) do + case Map.get(bucket.tokens, key, bucket.capacity) do + 0 -> + %TakeResponse{allowed?: false, bucket: bucket} + + remaining -> + tokens = Map.put(bucket.tokens, key, remaining - 1) + %TakeResponse{allowed?: true, bucket: %__MODULE__{bucket | tokens: tokens}} + end + end +end +``` + +```elixir +@impl GenServer +def handle_call({:allow?, key}, _from, %ServerState{} = state) do + %TakeResponse{allowed?: allowed?, bucket: bucket} = + Bucket.take(state.bucket, %TakeRequest{key: key}) + + {:reply, allowed?, %ServerState{state | bucket: bucket}} +end +``` + +## Public Module + +Name it after the capability callers use: `MyApp.RateLimiter`. That it runs as a process is an implementation detail. + +When the callbacks outgrow a single module, move them to `MyApp.RateLimiter.Server` and keep the client API in `MyApp.RateLimiter`. Callers never reference `.Server`. + +## Pure Core + +Name it after the domain concept: `Bucket`, `Room`, `Session`, `Ledger`. The struct and the functions that operate on it live in the same module. + +Avoid role names: `State`, `Core`, `Logic`, `Impl`, `Engine`, `Worker`, `Manager`, `Service`. They describe architecture rather than meaning, repeat in every GenServer, and tie the core to the process even when it is used without one. + +Name core functions with domain verbs (`take`, `refill`, `expire`), never GenServer vocabulary (`handle_*`). + +## Process State + +Name the process struct `ServerState`, or `Server.State` when callbacks live in `.Server`. It holds process concerns (timers, monitors, pending replies) and the domain core as a field: + +```elixir +defmodule MyApp.RateLimiter.ServerState do + defstruct [:bucket, :refill_timer, :pending] +end +``` + +When the process has no domain logic at all, `ServerState` alone is enough and no core module is needed. + +## Inputs and Outputs + +Pass structs in and out of the core. Tuples like `{result, bucket}` leave the contract unnamed and break callers when a field is added. + +Name them `Request` and `Response`, nested under the core module: + +- The pairing stays unambiguous as operations grow: `TakeRequest` and `TakeResponse`, `RefillRequest` and `RefillResponse`. +- The response carries the new core state, so the server takes the reply and the next state from one value. +- These are the core's contract, not the GenServer message. The message tuple stays private to the public module. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index cd104ba..3074351 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -20,6 +20,12 @@ - [ ] `handle_info/2` has a catch-all clause - [ ] Process names never built from untrusted input +## Naming ([naming.md](naming.md)) + +- [ ] Pure core named after a domain concept, not a role (`State`, `Core`, `Logic`, `Impl`) +- [ ] Process bookkeeping lives in `ServerState` and holds the core as a field +- [ ] Core takes and returns `Request` / `Response` structs, not tuples + ## Supervision ([supervision-and-testing.md](supervision-and-testing.md)) - [ ] Restart strategy matches the process lifecycle From 6a53f5d26381609ba302daa5c62fc49c946d1052 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 13:02:41 -0400 Subject: [PATCH 08/29] fix(trogonstack-elixir): keep Request and Response structs at the GenServer boundary Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 2 +- .../references/callback-patterns.md | 24 +++---- .../references/naming.md | 65 +++++++++---------- .../references/review-checklist.md | 5 +- 4 files changed, 48 insertions(+), 48 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index 373f224..e9feabf 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -38,7 +38,7 @@ Client API wraps every `call`/`cast`, callbacks delegate to a pure core, `call` Read [references/callback-patterns.md](references/callback-patterns.md) when writing or reviewing the module body. -Read [references/naming.md](references/naming.md) when naming the public module, the pure core, the process state, or the structs passed in and out of the core. +Read [references/naming.md](references/naming.md) when naming the public module, the pure core, the process state, or the Request/Response structs the GenServer exchanges. ### 4. Supervise and Test diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index 24c635c..db43bef 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -6,16 +6,15 @@ defmodule MyApp.RateLimiter do use GenServer - alias MyApp.RateLimiter.{Bucket, ServerState} - alias MyApp.RateLimiter.Bucket.{TakeRequest, TakeResponse} + alias MyApp.RateLimiter.{Bucket, ServerState, TakeRequest, TakeResponse} def start_link(opts) do {name, opts} = Keyword.pop!(opts, :name) GenServer.start_link(__MODULE__, opts, name: name) end - def allow?(server, key) do - GenServer.call(server, {:allow?, key}) + def take(server, %TakeRequest{} = request) do + GenServer.call(server, request) end @impl GenServer @@ -24,17 +23,20 @@ defmodule MyApp.RateLimiter do end @impl GenServer - def handle_call({:allow?, key}, _from, %ServerState{} = state) do - %TakeResponse{allowed?: allowed?, bucket: bucket} = - Bucket.take(state.bucket, %TakeRequest{key: key}) - - {:reply, allowed?, %ServerState{state | bucket: bucket}} + def handle_call(%TakeRequest{key: key}, _from, %ServerState{} = state) do + case Bucket.take(state.bucket, key) do + {:ok, bucket} -> + {:reply, %TakeResponse{allowed?: true}, %ServerState{state | bucket: bucket}} + + {:error, :exhausted} -> + {:reply, %TakeResponse{allowed?: false}, state} + end end end ``` -- Callers never call `GenServer.call/cast` directly. The client API is the contract; message shapes are private. -- Callbacks delegate to a pure core (`Bucket.take/2`) that takes structs in and returns structs out. See [naming.md](naming.md). +- Callers never call `GenServer.call/cast` directly. The client API owns the call; the messages are `Request` and `Response` structs. See [naming.md](naming.md). +- Callbacks delegate to a pure core (`Bucket.take/2`) that knows nothing about the GenServer or its messages. - Mark every callback with `@impl GenServer`. - Require `:name` in `start_link/1` and take the server as the first argument of every client function. Callers and tests always say which instance they talk to; a default argument hides that choice. - Hold process state in a `ServerState` struct so its shape is documented and enforced. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md index 0df8300..d39df10 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md @@ -1,43 +1,40 @@ # Naming -A GenServer usually involves a public API with process callbacks, a pure deterministic core, and the process state. Name each one after what it means, not where it sits. +A GenServer usually involves a public API with process callbacks, the messages it exchanges, the process state, and a pure deterministic core. Name each one after what it means, not where it sits. ## Recommended Layout ```text -MyApp.RateLimiter public API and GenServer callbacks -MyApp.RateLimiter.ServerState process bookkeeping, holds the core as a field -MyApp.RateLimiter.Bucket pure deterministic core, struct and functions -MyApp.RateLimiter.Bucket.TakeRequest input to Bucket.take/2 -MyApp.RateLimiter.Bucket.TakeResponse output of Bucket.take/2 +MyApp.RateLimiter public API and GenServer callbacks +MyApp.RateLimiter.TakeRequest message sent to the GenServer +MyApp.RateLimiter.TakeResponse reply from the GenServer +MyApp.RateLimiter.ServerState process bookkeeping, holds the core as a field +MyApp.RateLimiter.Bucket pure deterministic core, struct and functions ``` ```elixir -defmodule MyApp.RateLimiter.Bucket do - alias MyApp.RateLimiter.Bucket.{TakeRequest, TakeResponse} - - defstruct [:capacity, :tokens] - - def take(%__MODULE__{} = bucket, %TakeRequest{key: key}) do - case Map.get(bucket.tokens, key, bucket.capacity) do - 0 -> - %TakeResponse{allowed?: false, bucket: bucket} +@impl GenServer +def handle_call(%TakeRequest{key: key}, _from, %ServerState{} = state) do + case Bucket.take(state.bucket, key) do + {:ok, bucket} -> + {:reply, %TakeResponse{allowed?: true}, %ServerState{state | bucket: bucket}} - remaining -> - tokens = Map.put(bucket.tokens, key, remaining - 1) - %TakeResponse{allowed?: true, bucket: %__MODULE__{bucket | tokens: tokens}} - end + {:error, :exhausted} -> + {:reply, %TakeResponse{allowed?: false}, state} end end ``` ```elixir -@impl GenServer -def handle_call({:allow?, key}, _from, %ServerState{} = state) do - %TakeResponse{allowed?: allowed?, bucket: bucket} = - Bucket.take(state.bucket, %TakeRequest{key: key}) +defmodule MyApp.RateLimiter.Bucket do + defstruct [:capacity, tokens: %{}] - {:reply, allowed?, %ServerState{state | bucket: bucket}} + def take(%__MODULE__{} = bucket, key) do + case Map.get(bucket.tokens, key, bucket.capacity) do + 0 -> {:error, :exhausted} + remaining -> {:ok, %__MODULE__{bucket | tokens: Map.put(bucket.tokens, key, remaining - 1)}} + end + end end ``` @@ -47,13 +44,15 @@ Name it after the capability callers use: `MyApp.RateLimiter`. That it runs as a When the callbacks outgrow a single module, move them to `MyApp.RateLimiter.Server` and keep the client API in `MyApp.RateLimiter`. Callers never reference `.Server`. -## Pure Core +## Messages -Name it after the domain concept: `Bucket`, `Room`, `Session`, `Ledger`. The struct and the functions that operate on it live in the same module. +Exchange structs with the GenServer instead of tuples like `{:allow?, key}`. Name them `Request` and `Response`, nested under the public module: -Avoid role names: `State`, `Core`, `Logic`, `Impl`, `Engine`, `Worker`, `Manager`, `Service`. They describe architecture rather than meaning, repeat in every GenServer, and tie the core to the process even when it is used without one. +- The pairing stays unambiguous as operations grow: `TakeRequest` and `TakeResponse`, `RefillRequest` and `RefillResponse`. +- Adding a field does not break pattern matches the way growing a tuple does. +- The client function takes the request and returns the response: `RateLimiter.take(server, %TakeRequest{key: key})`. -Name core functions with domain verbs (`take`, `refill`, `expire`), never GenServer vocabulary (`handle_*`). +Request and Response exist only at the GenServer boundary. The pure core never sees them; `handle_call/3` unpacks the request, calls the core with domain values, and builds the response. ## Process State @@ -67,12 +66,10 @@ end When the process has no domain logic at all, `ServerState` alone is enough and no core module is needed. -## Inputs and Outputs +## Pure Core -Pass structs in and out of the core. Tuples like `{result, bucket}` leave the contract unnamed and break callers when a field is added. +Name it after the domain concept: `Bucket`, `Room`, `Session`, `Ledger`. The struct and the functions that operate on it live in the same module, and they take and return that struct. -Name them `Request` and `Response`, nested under the core module: +Avoid role names: `State`, `Core`, `Logic`, `Impl`, `Engine`, `Worker`, `Manager`, `Service`. They describe architecture rather than meaning, repeat in every GenServer, and tie the core to the process even when it is used without one. -- The pairing stays unambiguous as operations grow: `TakeRequest` and `TakeResponse`, `RefillRequest` and `RefillResponse`. -- The response carries the new core state, so the server takes the reply and the next state from one value. -- These are the core's contract, not the GenServer message. The message tuple stays private to the public module. +Name core functions with domain verbs (`take`, `refill`, `expire`), never GenServer vocabulary (`handle_*`). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index 3074351..83d27d9 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -13,7 +13,7 @@ ## Module Shape ([callback-patterns.md](callback-patterns.md)) -- [ ] Client API wraps every message; message tuples never leak +- [ ] Client API wraps every `call`/`cast`; callers never send messages directly - [ ] Callbacks delegate to pure, separately tested functions - [ ] `init/1` is fast; slow work runs in `handle_continue/2`, unless siblings need the state at boot - [ ] Persistent dependency failures retry with backoff instead of crash-looping the supervisor @@ -24,7 +24,8 @@ - [ ] Pure core named after a domain concept, not a role (`State`, `Core`, `Logic`, `Impl`) - [ ] Process bookkeeping lives in `ServerState` and holds the core as a field -- [ ] Core takes and returns `Request` / `Response` structs, not tuples +- [ ] GenServer messages are `Request` / `Response` structs, not tuples +- [ ] The pure core never references the Request/Response structs ## Supervision ([supervision-and-testing.md](supervision-and-testing.md)) From d396eaf86469afe1f54fc76deef3e8cacc83061c Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 13:10:52 -0400 Subject: [PATCH 09/29] fix(trogonstack-elixir): keep GenServer replies to two-element result tuples Signed-off-by: Yordis Prieto --- .../references/callback-patterns.md | 30 ++++++--- .../references/mailbox-queuing.md | 4 +- .../references/naming.md | 66 +++++++++++-------- .../references/review-checklist.md | 3 +- 4 files changed, 65 insertions(+), 38 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index db43bef..d34fc66 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -6,7 +6,7 @@ defmodule MyApp.RateLimiter do use GenServer - alias MyApp.RateLimiter.{Bucket, ServerState, TakeRequest, TakeResponse} + alias MyApp.RateLimiter.{Bucket, TakeRequest, TakeResponse} def start_link(opts) do {name, opts} = Keyword.pop!(opts, :name) @@ -19,17 +19,17 @@ defmodule MyApp.RateLimiter do @impl GenServer def init(opts) do - {:ok, %ServerState{bucket: Bucket.new(opts)}} + {:ok, Bucket.new(opts)} end @impl GenServer - def handle_call(%TakeRequest{key: key}, _from, %ServerState{} = state) do - case Bucket.take(state.bucket, key) do + def handle_call(%TakeRequest{key: key}, _from, %Bucket{} = bucket) do + case Bucket.take(bucket, key) do {:ok, bucket} -> - {:reply, %TakeResponse{allowed?: true}, %ServerState{state | bucket: bucket}} + {:reply, {:ok, %TakeResponse{remaining: Bucket.remaining(bucket, key)}}, bucket} - {:error, :exhausted} -> - {:reply, %TakeResponse{allowed?: false}, state} + {:error, error} -> + {:reply, {:error, error}, bucket} end end end @@ -39,7 +39,21 @@ end - Callbacks delegate to a pure core (`Bucket.take/2`) that knows nothing about the GenServer or its messages. - Mark every callback with `@impl GenServer`. - Require `:name` in `start_link/1` and take the server as the first argument of every client function. Callers and tests always say which instance they talk to; a default argument hides that choice. -- Hold process state in a `ServerState` struct so its shape is documented and enforced. +- Hold state in a struct so its shape is documented and enforced. The domain struct is the state until process-only data appears; see [naming.md](naming.md). + +## Reply Shapes + +Every reply, and every result the pure core returns, is one of: + +| Shape | When | +|-------|------| +| `:ok` | Success with nothing to return | +| `{:ok, value}` | Success with a value, usually an `Response` struct | +| `{:error, error}` | Failure, with an error struct (`defexception`) | + +Never reply with a tuple of more than two elements such as `{:ok, value, extra}` or `{:error, reason, details}`. Put the extra data in the struct. Callers can then handle every operation with the same `case` or `with`, and adding a field never breaks a pattern match. + +This rule covers the replies you design. OTP callback return values such as `{:reply, reply, state}` or `{:ok, state, {:continue, term}}` keep the shapes OTP requires. ## call vs cast vs send diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md index 57a7177..e9be623 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md @@ -68,11 +68,11 @@ Symptoms: `GenServer.call` timeouts that rise with load, latency that grows line {PartitionSupervisor, child_spec: MyApp.Counter, name: MyApp.Counters} ] - GenServer.call({:via, PartitionSupervisor, {MyApp.Counters, key}}, {:incr, key}) + GenServer.call({:via, PartitionSupervisor, {MyApp.Counters, key}}, %IncrementRequest{key: key}) ``` 5. **One process per entity.** Use `Registry` + `DynamicSupervisor` when each entity has its own state and lifecycle (a chat room, a device session). 6. **Pool a scarce resource.** Use a pool (`NimblePool`, `:poolboy`) when the bottleneck is a limited external resource rather than state. -7. **Shed load.** When the queue cannot be avoided, bound it: check `message_queue_len` or a counter before enqueueing and reject early with `{:error, :overloaded}` instead of letting callers time out. +7. **Shed load.** When the queue cannot be avoided, bound it: check `message_queue_len` or a counter before enqueueing and reject early with `{:error, %OverloadedError{}}` instead of letting callers time out. Prefer `call` over `cast` from producers that can outrun the server: the blocked caller is the back-pressure. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md index d39df10..f6b3299 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md @@ -1,43 +1,51 @@ # Naming -A GenServer usually involves a public API with process callbacks, the messages it exchanges, the process state, and a pure deterministic core. Name each one after what it means, not where it sits. +A GenServer usually involves a public API with process callbacks, the messages it exchanges, and a pure deterministic core that doubles as the process state. Name each one after what it means, not where it sits. ## Recommended Layout ```text -MyApp.RateLimiter public API and GenServer callbacks -MyApp.RateLimiter.TakeRequest message sent to the GenServer -MyApp.RateLimiter.TakeResponse reply from the GenServer -MyApp.RateLimiter.ServerState process bookkeeping, holds the core as a field -MyApp.RateLimiter.Bucket pure deterministic core, struct and functions +MyApp.RateLimiter public API and GenServer callbacks +MyApp.RateLimiter.TakeRequest message sent to the GenServer +MyApp.RateLimiter.TakeResponse value in the {:ok, response} reply +MyApp.RateLimiter.Bucket pure deterministic core, and the process state +MyApp.RateLimiter.Bucket.ExhaustedError error in the {:error, error} reply ``` ```elixir @impl GenServer -def handle_call(%TakeRequest{key: key}, _from, %ServerState{} = state) do - case Bucket.take(state.bucket, key) do +def handle_call(%TakeRequest{key: key}, _from, %Bucket{} = bucket) do + case Bucket.take(bucket, key) do {:ok, bucket} -> - {:reply, %TakeResponse{allowed?: true}, %ServerState{state | bucket: bucket}} + {:reply, {:ok, %TakeResponse{remaining: Bucket.remaining(bucket, key)}}, bucket} - {:error, :exhausted} -> - {:reply, %TakeResponse{allowed?: false}, state} + {:error, error} -> + {:reply, {:error, error}, bucket} end end ``` ```elixir defmodule MyApp.RateLimiter.Bucket do + alias MyApp.RateLimiter.Bucket.ExhaustedError + defstruct [:capacity, tokens: %{}] def take(%__MODULE__{} = bucket, key) do - case Map.get(bucket.tokens, key, bucket.capacity) do - 0 -> {:error, :exhausted} - remaining -> {:ok, %__MODULE__{bucket | tokens: Map.put(bucket.tokens, key, remaining - 1)}} + case remaining(bucket, key) do + 0 -> {:error, %ExhaustedError{key: key}} + n -> {:ok, %__MODULE__{bucket | tokens: Map.put(bucket.tokens, key, n - 1)}} end end + + def remaining(%__MODULE__{} = bucket, key) do + Map.get(bucket.tokens, key, bucket.capacity) + end end ``` +Replies follow the shapes in [callback-patterns.md](callback-patterns.md#reply-shapes): `:ok`, `{:ok, value}`, or `{:error, error}`. + ## Public Module Name it after the capability callers use: `MyApp.RateLimiter`. That it runs as a process is an implementation detail. @@ -50,22 +58,11 @@ Exchange structs with the GenServer instead of tuples like `{:allow?, key}`. Nam - The pairing stays unambiguous as operations grow: `TakeRequest` and `TakeResponse`, `RefillRequest` and `RefillResponse`. - Adding a field does not break pattern matches the way growing a tuple does. -- The client function takes the request and returns the response: `RateLimiter.take(server, %TakeRequest{key: key})`. +- The client function takes the request and returns `{:ok, %TakeResponse{}}` or `{:error, error}`. +- An operation with nothing to return replies `:ok` and needs no Response struct. Request and Response exist only at the GenServer boundary. The pure core never sees them; `handle_call/3` unpacks the request, calls the core with domain values, and builds the response. -## Process State - -Name the process struct `ServerState`, or `Server.State` when callbacks live in `.Server`. It holds process concerns (timers, monitors, pending replies) and the domain core as a field: - -```elixir -defmodule MyApp.RateLimiter.ServerState do - defstruct [:bucket, :refill_timer, :pending] -end -``` - -When the process has no domain logic at all, `ServerState` alone is enough and no core module is needed. - ## Pure Core Name it after the domain concept: `Bucket`, `Room`, `Session`, `Ledger`. The struct and the functions that operate on it live in the same module, and they take and return that struct. @@ -73,3 +70,18 @@ Name it after the domain concept: `Bucket`, `Room`, `Session`, `Ledger`. The str Avoid role names: `State`, `Core`, `Logic`, `Impl`, `Engine`, `Worker`, `Manager`, `Service`. They describe architecture rather than meaning, repeat in every GenServer, and tie the core to the process even when it is used without one. Name core functions with domain verbs (`take`, `refill`, `expire`), never GenServer vocabulary (`handle_*`). + +## Process State + +Start with the domain struct as the process state: `init/1` returns `{:ok, %Bucket{}}`. Add a wrapper only when the process holds something the domain should not know about. + +| Situation | State | +|-----------|-------| +| Only domain data | The domain struct: `%Bucket{}` | +| Process-only data too: timer refs, monitor refs, pending `from`s, task refs | `%ServerState{bucket: %Bucket{}, refill_timer: ref}` | +| Several domain structs | `%ServerState{bucket: %Bucket{}, quota: %Quota{}}` | +| No domain logic, only bookkeeping | `%ServerState{}` and no core module | + +Process-only data stays out of the domain struct because refs and `from`s are runtime plumbing, not deterministic data; putting them in the core makes it harder to test and reuse without a process. + +Name the wrapper `ServerState`, or `Server.State` when callbacks live in `.Server`. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index 83d27d9..a4f8e6c 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -17,13 +17,14 @@ - [ ] Callbacks delegate to pure, separately tested functions - [ ] `init/1` is fast; slow work runs in `handle_continue/2`, unless siblings need the state at boot - [ ] Persistent dependency failures retry with backoff instead of crash-looping the supervisor +- [ ] Replies and core results are `:ok`, `{:ok, value}`, or `{:error, error}`, never longer tuples - [ ] `handle_info/2` has a catch-all clause - [ ] Process names never built from untrusted input ## Naming ([naming.md](naming.md)) - [ ] Pure core named after a domain concept, not a role (`State`, `Core`, `Logic`, `Impl`) -- [ ] Process bookkeeping lives in `ServerState` and holds the core as a field +- [ ] The domain struct is the state; `ServerState` exists only for process-only data or several domain structs - [ ] GenServer messages are `Request` / `Response` structs, not tuples - [ ] The pure core never references the Request/Response structs From 92822dae578837910229fe8f5e0cb6e89e5d94ca Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 13:18:15 -0400 Subject: [PATCH 10/29] fix(trogonstack-elixir): name GenServer modules after their domain to avoid collisions Signed-off-by: Yordis Prieto --- .../elixir-design-genserver/references/naming.md | 12 +++++++----- .../references/review-checklist.md | 3 ++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md index f6b3299..a502e67 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md @@ -50,7 +50,9 @@ Replies follow the shapes in [callback-patterns.md](callback-patterns.md#reply-s Name it after the capability callers use: `MyApp.RateLimiter`. That it runs as a process is an implementation detail. -When the callbacks outgrow a single module, move them to `MyApp.RateLimiter.Server` and keep the client API in `MyApp.RateLimiter`. Callers never reference `.Server`. +When the callbacks outgrow a single module, move them to `MyApp.RateLimiter.RateLimiterServer` and keep the client API in `MyApp.RateLimiter`. Callers never reference the server module. + +Prefix the server module with the domain name instead of a bare `.Server`. A context may grow more than one server, and a bare `Server` collides across the codebase: every alias, stack trace, and search result reads `Server`, and aliasing two of them in one module forces `as:` renames. ## Messages @@ -78,10 +80,10 @@ Start with the domain struct as the process state: `init/1` returns `{:ok, %Buck | Situation | State | |-----------|-------| | Only domain data | The domain struct: `%Bucket{}` | -| Process-only data too: timer refs, monitor refs, pending `from`s, task refs | `%ServerState{bucket: %Bucket{}, refill_timer: ref}` | -| Several domain structs | `%ServerState{bucket: %Bucket{}, quota: %Quota{}}` | -| No domain logic, only bookkeeping | `%ServerState{}` and no core module | +| Process-only data too: timer refs, monitor refs, pending `from`s, task refs | `%RateLimiterServerState{bucket: %Bucket{}, refill_timer: ref}` | +| Several domain structs | `%RateLimiterServerState{bucket: %Bucket{}, quota: %Quota{}}` | +| No domain logic, only bookkeeping | `%RateLimiterServerState{}` and no core module | Process-only data stays out of the domain struct because refs and `from`s are runtime plumbing, not deterministic data; putting them in the core makes it harder to test and reuse without a process. -Name the wrapper `ServerState`, or `Server.State` when callbacks live in `.Server`. +Name the wrapper `ServerState`, for example `MyApp.RateLimiter.RateLimiterServerState`, for the same reason the server module carries the domain name. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index a4f8e6c..cd96f76 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -24,7 +24,8 @@ ## Naming ([naming.md](naming.md)) - [ ] Pure core named after a domain concept, not a role (`State`, `Core`, `Logic`, `Impl`) -- [ ] The domain struct is the state; `ServerState` exists only for process-only data or several domain structs +- [ ] The domain struct is the state; a `ServerState` wrapper exists only for process-only data or several domain structs +- [ ] Server modules carry the domain name (`RateLimiterServer`), never a bare `.Server` - [ ] GenServer messages are `Request` / `Response` structs, not tuples - [ ] The pure core never references the Request/Response structs From bf5ef06f62f4058be0d54260642ac689aa44dee1 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 13:29:38 -0400 Subject: [PATCH 11/29] feat(trogonstack-elixir): show how to remove GenServer queues and handle non-client messages Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 4 +- .../references/callback-patterns.md | 19 +-- .../references/handle-info.md | 114 ++++++++++++++++++ .../references/mailbox-queuing.md | 63 +++++----- .../references/naming.md | 25 ++++ .../references/process-justification.md | 27 +---- .../references/queuing-chunk-long-work.md | 50 ++++++++ .../references/queuing-large-replies.md | 42 +++++++ .../references/queuing-offload-slow-work.md | 65 ++++++++++ .../references/queuing-partition-by-key.md | 57 +++++++++ .../references/queuing-pool-resources.md | 46 +++++++ .../references/queuing-process-per-entity.md | 74 ++++++++++++ .../references/queuing-reads-from-ets.md | 66 ++++++++++ .../references/queuing-remove-the-process.md | 51 ++++++++ .../references/queuing-shed-load.md | 46 +++++++ .../references/review-checklist.md | 12 +- 16 files changed, 697 insertions(+), 64 deletions(-) create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-large-replies.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-offload-slow-work.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-partition-by-key.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-pool-resources.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index e9feabf..a9c53f2 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -30,7 +30,7 @@ Read [references/process-justification.md](references/process-justification.md) Estimate arrival rate and handling time on the hot path. A single server tops out at `1 / handling_time` messages per second regardless of core count. If callers can outpace it, the mailbox grows and latency grows with it. -Read [references/mailbox-queuing.md](references/mailbox-queuing.md) when the server sits on a request path, receives casts from many producers, does I/O inside callbacks, or already shows timeouts or a growing `message_queue_len`. +Read [references/mailbox-queuing.md](references/mailbox-queuing.md) when the server sits on a request path, receives casts from many producers, does I/O inside callbacks, or already shows timeouts or a growing `message_queue_len`. It links to an avoid/prefer example for each way of removing the queue. ### 3. Shape the Module @@ -38,6 +38,8 @@ Client API wraps every `call`/`cast`, callbacks delegate to a pure core, `call` Read [references/callback-patterns.md](references/callback-patterns.md) when writing or reviewing the module body. +Read [references/handle-info.md](references/handle-info.md) when the server sends itself messages, uses timers, monitors other processes, or receives task results. + Read [references/naming.md](references/naming.md) when naming the public module, the pure core, the process state, or the Request/Response structs the GenServer exchanges. ### 4. Supervise and Test diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index d34fc66..31240d3 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -59,7 +59,7 @@ This rule covers the replies you design. OTP callback return values such as `{:r - `call`: the default. Back-pressure, errors reach the caller, the work is confirmed. - `cast`: only when the caller does not care about the outcome and producers cannot outrun the server. See [mailbox-queuing.md](mailbox-queuing.md). -- `send/2` + `handle_info/2`: messages that are not part of the client API, such as timers, monitors, ports, and other libraries. +- `send/2` + `handle_info/2`: messages that are not part of the client API, such as timers, monitors, task results, and other libraries. See [handle-info.md](handle-info.md). ## Initialization @@ -73,22 +73,23 @@ This rule covers the replies you design. OTP callback return values such as `{:r ## Timeouts and Long Work - `GenServer.call/3` defaults to 5 seconds and exits the caller on expiry. A slow call inside a serialized process is a design question before it is a timeout question. -- Offload slow work to a `Task` and reply later with `GenServer.reply/2`. -- For periodic work use `Process.send_after/3` and reschedule inside `handle_info/2`. `:timer.send_interval/2` keeps firing even when the server falls behind, which piles messages into the mailbox. +- Offload slow work to a task and reply later with `GenServer.reply/2`; see [queuing-offload-slow-work.md](queuing-offload-slow-work.md). +- For periodic work, timers, monitors, and the `handle_info/2` catch-all, see [handle-info.md](handle-info.md). -## Unexpected Messages +## Redacting State in Crash Logs -`use GenServer` injects a `handle_info/2` that logs unexpected messages. Defining your own replaces it, so add a catch-all clause or an unknown message crashes the server with a `FunctionClauseError`. +Crash reports and `:sys.get_status/1` print the full state. When it holds secrets (tokens, credentials, personal data), implement `format_status/1` (OTP 25+) to redact them: ```elixir @impl GenServer -def handle_info(msg, state) do - Logger.warning("unexpected message: #{inspect(msg)}") - {:noreply, state} +def format_status(%{state: %ClientServerState{} = state} = status) do + %{status | state: %ClientServerState{state | api_key: :redacted}} end + +def format_status(status), do: status ``` -## Naming +## Process Names - Singleton: `name: __MODULE__`, only for genuinely one-per-node services. A singleton on a request path is a global queue. - Many instances: `name: {:via, Registry, {MyApp.Registry, key}}` under a `DynamicSupervisor`. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md new file mode 100644 index 0000000..1937cc7 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md @@ -0,0 +1,114 @@ +# handle_info and Self-Messaging + +`handle_info/2` receives every message that did not come through `GenServer.call/cast`: messages the server sends itself, timers, monitors, task results, exit signals, and messages from libraries. + +## What Arrives Here + +| Message | Source | Shape | +|---------|--------|-------| +| Deferred work | `send(self(), msg)` | Your struct, for example `%ImportNextBatch{}` | +| Timer | `Process.send_after(self(), msg, ms)` | Your struct, for example `%RefillTick{}` | +| Monitored process died | `Process.monitor/1` | `{:DOWN, ref, :process, pid, reason}` | +| Task finished | `Task.Supervisor.async_nolink/2` | `{ref, result}`, then `:DOWN` unless flushed | +| Linked process exited | Only when trapping exits | `{:EXIT, pid, reason}` | +| Library message | PubSub, sockets, ports | Whatever the library defines | + +## Internal Message Structs + +Messages you send yourself are a private protocol. Define them as structs nested under the server module, like `Request` for client messages: + +```elixir +defmodule MyApp.RateLimiter.RefillTick do + defstruct [:token] +end +``` + +Match the tuples that OTP and libraries define (`:DOWN`, `{ref, result}`, `:EXIT`) as they are; you do not own those shapes. + +## Never Call Yourself + +```elixir +@impl GenServer +def handle_info(%RefillTick{}, %Bucket{} = bucket) do + RateLimiter.take(self(), %TakeRequest{key: :system}) + {:noreply, bucket} +end +``` + +`GenServer.call/3` to `self()` exits immediately with `:calling_self`. The indirect version is worse: server A calls B while B calls A, and both block until the timeout. Inside callbacks, call the pure core directly and never the module's own client API. Between servers, break call cycles with `cast`, `send`, or by moving the shared logic into a pure module both can use. + +## Deferring Work to Yourself + +`send(self(), msg)` and `{:continue, term}` both defer work, with opposite ordering: + +| Mechanism | Runs | Use for | +|-----------|------|---------| +| `{:continue, term}` | Immediately after the current callback, before any other message | Finishing setup or a follow-up step that must not interleave with clients | +| `send(self(), msg)` | After every message already in the mailbox | Yielding to clients between batches; see [queuing-chunk-long-work.md](queuing-chunk-long-work.md) | + +## Timers + +Use `Process.send_after/3` and reschedule from `handle_info/2`. Avoid `:timer.send_interval/2`, which keeps firing when the server falls behind and piles ticks into the mailbox. + +`Process.cancel_timer/1` cannot cancel a message that was already delivered. Tag each timer with a token and ignore ticks that do not match the current one: + +```elixir +@refill_interval :timer.seconds(1) + +defp schedule_refill(%RateLimiterServerState{} = state) do + if state.refill_timer, do: Process.cancel_timer(state.refill_timer) + token = make_ref() + timer = Process.send_after(self(), %RefillTick{token: token}, @refill_interval) + %RateLimiterServerState{state | refill_timer: timer, refill_token: token} +end + +@impl GenServer +def handle_info(%RefillTick{token: token}, %RateLimiterServerState{refill_token: token} = state) do + bucket = Bucket.refill(state.bucket, System.monotonic_time(:millisecond)) + {:noreply, schedule_refill(%RateLimiterServerState{state | bucket: bucket})} +end + +def handle_info(%RefillTick{}, %RateLimiterServerState{} = state) do + {:noreply, state} +end +``` + +- The server reads the clock and passes it to the core; `Bucket.refill/2` stays deterministic. See [naming.md](naming.md#time-is-an-input). +- Timer refs and tokens are process-only data, so they live in `RateLimiterServerState`, not in `Bucket`. + +## Monitors + +When state holds pids (subscribers, pending callers, workers), monitor them. Otherwise entries for dead processes stay in state forever. + +```elixir +@impl GenServer +def handle_call(%SubscribeRequest{pid: pid}, _from, %FeedServerState{} = state) do + ref = Process.monitor(pid) + {:reply, :ok, %FeedServerState{state | subscribers: Map.put(state.subscribers, ref, pid)}} +end + +@impl GenServer +def handle_info({:DOWN, ref, :process, _pid, _reason}, %FeedServerState{} = state) + when is_map_key(state.subscribers, ref) do + {:noreply, %FeedServerState{state | subscribers: Map.delete(state.subscribers, ref)}} +end +``` + +Monitor instead of link: a link makes the server die with the subscriber, a monitor only tells it. + +## Task Results + +Results of `Task.Supervisor.async_nolink/2` arrive as `{ref, result}`, and a crash arrives as `:DOWN`. See [queuing-offload-slow-work.md](queuing-offload-slow-work.md) for the full pattern. + +## Catch-All + +`use GenServer` injects a `handle_info/2` that logs unexpected messages. Defining your own replaces it, so end with a catch-all clause or an unknown message crashes the server with a `FunctionClauseError`: + +```elixir +def handle_info(msg, state) do + Logger.warning("unexpected message: #{inspect(msg)}") + {:noreply, state} +end +``` + +Keep it as the last clause; clauses after it never match. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md index e9be623..82cb5d4 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md @@ -29,50 +29,53 @@ Latency under load (Little's law): a message waits roughly `queue_length * avera ### Hidden Serialization -The worst queues are the ones nobody meant to build: a singleton GenServer that wraps work which was already concurrent (DB pool, HTTP client, pure functions). Each request is fast in isolation, tests pass, and production latency grows with traffic. +The worst queues are the ones nobody meant to build: a GenServer that wraps work which was already concurrent (DB pool, HTTP client, pure functions). Each request is fast in isolation, tests pass, and production latency grows with traffic. ## Detecting It ```elixir Process.info(pid, :message_queue_len) -:sys.get_status(pid) ``` - `:recon.proc_count(:message_queue_len, 10)` lists the processes with the longest mailboxes. - `:observer.start()` Processes tab, sorted by MsgQ. -- Telemetry: emit handling duration per callback and sample `message_queue_len` periodically. -Symptoms: `GenServer.call` timeouts that rise with load, latency that grows linearly with traffic, one scheduler pegged while others idle, process memory climbing on a single pid. - -## Fixes, Least Invasive First - -1. **Remove the process.** If it owns nothing (see [process-justification.md](process-justification.md)), call the functions directly from the caller. -2. **Move reads out of the mailbox.** The server owns writes; readers go straight to ETS. +Measure handling time per callback, so the `1 / handling_time` ceiling is a number rather than a guess: - ```elixir - :ets.new(__MODULE__, [:named_table, :protected, read_concurrency: true]) - - def lookup(key) do - case :ets.lookup(__MODULE__, key) do - [{^key, value}] -> {:ok, value} - [] -> :error - end - end - ``` +```elixir +@impl GenServer +def handle_call(%TakeRequest{} = request, _from, %Bucket{} = bucket) do + :telemetry.span([:my_app, :rate_limiter, :take], %{}, fn -> + {take(request, bucket), %{}} + end) +end +``` -3. **Move slow work out of the callback.** Start a `Task` under a `Task.Supervisor`, return `{:noreply, state}` while keeping `from`, and answer with `GenServer.reply/2` when the task result arrives in `handle_info/2`. The server keeps accepting messages while the work runs. -4. **Partition by key.** Run N copies and route each key to one, so unrelated keys stop waiting on each other. +Sample mailbox length with `telemetry_poller`: - ```elixir - children = [ - {PartitionSupervisor, child_spec: MyApp.Counter, name: MyApp.Counters} - ] +```elixir +{:telemetry_poller, + measurements: [ + {:process_info, name: MyApp.RateLimiter, event: [:my_app, :rate_limiter], keys: [:message_queue_len]} + ]} +``` - GenServer.call({:via, PartitionSupervisor, {MyApp.Counters, key}}, %IncrementRequest{key: key}) - ``` +Symptoms: `GenServer.call` timeouts that rise with load, latency that grows linearly with traffic, one scheduler pegged while others idle, process memory climbing on a single pid. -5. **One process per entity.** Use `Registry` + `DynamicSupervisor` when each entity has its own state and lifecycle (a chat room, a device session). -6. **Pool a scarce resource.** Use a pool (`NimblePool`, `:poolboy`) when the bottleneck is a limited external resource rather than state. -7. **Shed load.** When the queue cannot be avoided, bound it: check `message_queue_len` or a counter before enqueueing and reject early with `{:error, %OverloadedError{}}` instead of letting callers time out. +## Patterns + +Each pattern has an avoid/prefer example. Pick the least invasive one that removes the queue. + +| Pattern | Read when | +|---------|-----------| +| [Remove the process](queuing-remove-the-process.md) | The server wraps a DB query, HTTP call, or pure computation and owns no state | +| [Serve reads from ETS](queuing-reads-from-ets.md) | Reads vastly outnumber writes and every read goes through the mailbox | +| [Offload slow work](queuing-offload-slow-work.md) | A callback blocks on I/O while other callers wait | +| [Chunk long work](queuing-chunk-long-work.md) | One message triggers a long job and client calls stall until it finishes | +| [Keep replies small](queuing-large-replies.md) | A `call` returns a large map or list | +| [Partition by key](queuing-partition-by-key.md) | Unrelated keys wait on each other in one server | +| [Process per entity](queuing-process-per-entity.md) | One server holds many independent entities (rooms, sessions, devices) | +| [Pool a resource](queuing-pool-resources.md) | The bottleneck is a scarce external resource, not state | +| [Shed load](queuing-shed-load.md) | The queue cannot be removed and callers must fail fast instead of timing out | Prefer `call` over `cast` from producers that can outrun the server: the blocked caller is the back-pressure. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md index a502e67..8f1624c 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md @@ -44,6 +44,15 @@ defmodule MyApp.RateLimiter.Bucket do end ``` +```elixir +defmodule MyApp.RateLimiter.Bucket.ExhaustedError do + defexception [:key] + + @impl Exception + def message(%__MODULE__{key: key}), do: "rate limit exhausted for #{inspect(key)}" +end +``` + Replies follow the shapes in [callback-patterns.md](callback-patterns.md#reply-shapes): `:ok`, `{:ok, value}`, or `{:error, error}`. ## Public Module @@ -63,6 +72,8 @@ Exchange structs with the GenServer instead of tuples like `{:allow?, key}`. Nam - The client function takes the request and returns `{:ok, %TakeResponse{}}` or `{:error, error}`. - An operation with nothing to return replies `:ok` and needs no Response struct. +Messages the server sends itself (`send/2`, timers) are structs too, for example `%RefillTick{}`; see [handle-info.md](handle-info.md#internal-message-structs). + Request and Response exist only at the GenServer boundary. The pure core never sees them; `handle_call/3` unpacks the request, calls the core with domain values, and builds the response. ## Pure Core @@ -73,6 +84,20 @@ Avoid role names: `State`, `Core`, `Logic`, `Impl`, `Engine`, `Worker`, `Manager Name core functions with domain verbs (`take`, `refill`, `expire`), never GenServer vocabulary (`handle_*`). +## Errors + +Return errors as `defexception` structs named `*Error`, nested under the module that produces them: `Bucket.ExhaustedError`. An exception struct in `{:error, error}` is not raised; it gains `Exception.message/1` for logs and lets a bang variant `raise` the same value. Build the message from structured fields so callers can still match on them. + +## Time Is an Input + +The core never reads the clock. The server reads it and passes it in, so the same inputs always produce the same output and tests need no sleeping or mocking: + +```elixir +Bucket.refill(bucket, System.monotonic_time(:millisecond)) +``` + +The same applies to randomness and ids: generate them in the server, pass them to the core. + ## Process State Start with the domain struct as the process state: `init/1` returns `{:ok, %Bucket{}}`. Add a wrapper only when the process holds something the domain should not know about. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md index 508c166..91a7e44 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md @@ -29,33 +29,14 @@ Invalid answers: | DB query or HTTP request | Call it from the caller's process | | One-off concurrent work, result awaited | `Task` / `Task.async_stream` | | Fire-and-forget work that must survive the caller | `Task.Supervisor.start_child` | -| Read-heavy shared data | ETS table owned by a process, or `:persistent_term` for rarely changing data | +| Read-heavy shared data | ETS table owned by a process ([queuing-reads-from-ets.md](queuing-reads-from-ets.md)), or `:persistent_term` for rarely changing data | | Shared state with logic, lifecycle, or timers | `GenServer` | -| One process per entity, looked up by key | `GenServer` + `Registry` + `DynamicSupervisor` | -| Stateless server that is a bottleneck | `PartitionSupervisor` (Elixir 1.14+) | +| One process per entity, looked up by key | `GenServer` + `Registry` + `DynamicSupervisor` ([queuing-process-per-entity.md](queuing-process-per-entity.md)) | +| Keyed state bottlenecked in one server | `PartitionSupervisor` (Elixir 1.14+, [queuing-partition-by-key.md](queuing-partition-by-key.md)) | ## The Anti-Pattern -```elixir -defmodule MyApp.Users do - use GenServer - - def get(id), do: GenServer.call(__MODULE__, {:get, id}) - - @impl GenServer - def handle_call({:get, id}, _from, state) do - {:reply, Repo.get(User, id), state} - end -end -``` - -The state is never used. Every request in the system now waits in one mailbox for every other `get/1` ahead of it, while the Repo connection pool sits mostly idle. The fix is deleting the process: - -```elixir -defmodule MyApp.Users do - def get(id), do: Repo.get(User, id) -end -``` +A GenServer whose state is never used, forwarding every request to the database or an HTTP client. See [queuing-remove-the-process.md](queuing-remove-the-process.md) for the example and its fix. ## Smells in Review diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md new file mode 100644 index 0000000..a3cd890 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md @@ -0,0 +1,50 @@ +# Chunk Long Work + +A single message that triggers a long job holds the server for the whole job. Split the job into batches and send the next batch to yourself, so client messages that arrive in between are handled between batches. + +If the job does not need server state, run it in a `Task` instead; see [queuing-offload-slow-work.md](queuing-offload-slow-work.md). + +## Avoid + +```elixir +@impl GenServer +def handle_call(%ImportRequest{rows: rows}, _from, %Catalog{} = catalog) do + catalog = Enum.reduce(rows, catalog, &Catalog.import_row(&2, &1)) + {:reply, :ok, catalog} +end +``` + +Importing 100,000 rows blocks every lookup until the import ends. + +## Prefer + +```elixir +@batch_size 500 + +@impl GenServer +def handle_call(%ImportRequest{rows: rows}, _from, %CatalogServerState{} = state) do + send(self(), %ImportNextBatch{}) + {:reply, :ok, %CatalogServerState{state | import_queue: state.import_queue ++ rows}} +end + +@impl GenServer +def handle_info(%ImportNextBatch{}, %CatalogServerState{import_queue: []} = state) do + {:noreply, state} +end + +def handle_info(%ImportNextBatch{}, %CatalogServerState{} = state) do + {batch, rest} = Enum.split(state.import_queue, @batch_size) + catalog = Enum.reduce(batch, state.catalog, &Catalog.import_row(&2, &1)) + send(self(), %ImportNextBatch{}) + {:noreply, %CatalogServerState{state | catalog: catalog, import_queue: rest}} +end +``` + +- `send(self(), msg)` puts the message at the back of the mailbox, behind client messages already waiting. That is what lets them interleave. +- `{:continue, term}` would run before any other message, which defeats the purpose here; see [handle-info.md](handle-info.md#deferring-work-to-yourself). +- The reply `:ok` means "accepted", not "imported". Expose progress through a separate request if callers need it. + +## Trade-offs + +- Lookups during the import see a partially imported catalog. If that is not acceptable, build the new catalog in a task and swap it in with one message. +- Batch size trades import throughput against lookup latency: a lookup waits at most one batch. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-large-replies.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-large-replies.md new file mode 100644 index 0000000..9427e71 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-large-replies.md @@ -0,0 +1,42 @@ +# Keep Replies Small + +Every message, including every reply, is copied from one process heap to the other. A `call` that returns a large map or list makes the server spend its time copying while the queue grows, and spikes memory in the caller. + +Binaries larger than 64 bytes are the exception: they live in a shared heap and only a reference is copied. + +## Avoid + +```elixir +@impl GenServer +def handle_call(%ListSessionsRequest{}, _from, %Sessions{} = sessions) do + {:reply, {:ok, %ListSessionsResponse{sessions: Sessions.all(sessions)}}, sessions} +end +``` + +```elixir +{:ok, %ListSessionsResponse{sessions: sessions}} = SessionStore.list_sessions(server, %ListSessionsRequest{}) +Enum.find(sessions, &(&1.user_id == user_id)) +``` + +The caller wants one session and receives 100,000 of them. + +## Prefer + +Ask the server for what you need, and let it do the filtering: + +```elixir +@impl GenServer +def handle_call(%FindSessionRequest{user_id: user_id}, _from, %Sessions{} = sessions) do + case Sessions.find_by_user(sessions, user_id) do + {:ok, session} -> {:reply, {:ok, %FindSessionResponse{session: session}}, sessions} + {:error, error} -> {:reply, {:error, error}, sessions} + end +end +``` + +When callers genuinely need to scan large data, serve it from ETS instead; see [queuing-reads-from-ets.md](queuing-reads-from-ets.md). + +## Trade-offs + +- Filtering in the server moves CPU into the serialized process. Keep the filter cheap, an indexed lookup, not a scan. +- The server state itself is never copied by callbacks; only messages are. A large state is a memory concern, not a copying one. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-offload-slow-work.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-offload-slow-work.md new file mode 100644 index 0000000..f0ba0d0 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-offload-slow-work.md @@ -0,0 +1,65 @@ +# Offload Slow Work + +A callback that blocks on I/O stalls every caller behind it. Start the work in a task, return immediately, and reply when the task finishes. + +First ask whether the server is needed at all. If the work does not read or change server state, the caller can run it; see [queuing-remove-the-process.md](queuing-remove-the-process.md). This pattern is for servers that must coordinate the work, for example to deduplicate concurrent fetches or to update state with the result. + +## Avoid + +```elixir +@impl GenServer +def handle_call(%FetchRateRequest{currency: currency}, _from, %RateCache{} = cache) do + case RatesClient.fetch(currency) do + {:ok, rate} -> {:reply, {:ok, %FetchRateResponse{rate: rate}}, RateCache.put(cache, currency, rate)} + {:error, error} -> {:reply, {:error, error}, cache} + end +end +``` + +A 2 second HTTP call caps the server at one request every 2 seconds, and every other caller times out. + +## Prefer + +```elixir +@impl GenServer +def handle_call(%FetchRateRequest{currency: currency}, from, %RatesServerState{} = state) do + task = Task.Supervisor.async_nolink(state.task_supervisor, fn -> RatesClient.fetch(currency) end) + pending = Map.put(state.pending, task.ref, %PendingFetch{from: from, currency: currency}) + {:noreply, %RatesServerState{state | pending: pending}} +end + +@impl GenServer +def handle_info({ref, result}, %RatesServerState{} = state) when is_map_key(state.pending, ref) do + Process.demonitor(ref, [:flush]) + {%PendingFetch{} = fetch, pending} = Map.pop(state.pending, ref) + state = %RatesServerState{state | pending: pending} + + case result do + {:ok, rate} -> + GenServer.reply(fetch.from, {:ok, %FetchRateResponse{rate: rate}}) + {:noreply, %RatesServerState{state | cache: RateCache.put(state.cache, fetch.currency, rate)}} + + {:error, error} -> + GenServer.reply(fetch.from, {:error, error}) + {:noreply, state} + end +end + +def handle_info({:DOWN, ref, :process, _pid, reason}, %RatesServerState{} = state) + when is_map_key(state.pending, ref) do + {%PendingFetch{} = fetch, pending} = Map.pop(state.pending, ref) + GenServer.reply(fetch.from, {:error, %FetchFailedError{reason: reason}}) + {:noreply, %RatesServerState{state | pending: pending}} +end +``` + +- `async_nolink` keeps a crashing task from taking the server down; the crash arrives as `:DOWN`. +- Returning `{:noreply, state}` from `handle_call/3` frees the server; `GenServer.reply/2` answers the caller later. +- `Process.demonitor(ref, [:flush])` drops the `:DOWN` message that would otherwise follow a successful result. +- The state is a `RatesServerState` wrapper because pending `from`s and task refs are process-only data; see [naming.md](naming.md#process-state). + +## Trade-offs + +- The caller still waits for the I/O, but other callers no longer wait for it. +- The caller's `call` timeout still applies. A reply after the timeout is discarded. +- Keep the clauses above the `handle_info/2` catch-all; see [handle-info.md](handle-info.md). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-partition-by-key.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-partition-by-key.md new file mode 100644 index 0000000..9967c61 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-partition-by-key.md @@ -0,0 +1,57 @@ +# Partition by Key + +When requests for unrelated keys wait on each other in one server, run several copies and route each key to the same copy. Keys still serialize against themselves, which preserves per-key consistency, but no longer against each other. + +## Avoid + +```elixir +children = [ + {MyApp.Counter, name: MyApp.Counter} +] + +MyApp.Counter.increment(MyApp.Counter, %IncrementRequest{key: key}) +``` + +Every increment for every key goes through one mailbox. + +## Prefer + +```elixir +children = [ + {PartitionSupervisor, child_spec: MyApp.Counter, name: MyApp.CounterPartitions} +] +``` + +```elixir +defmodule MyApp.Counter do + use GenServer + + alias MyApp.Counter.{Counts, IncrementRequest} + + def start_link(opts) do + GenServer.start_link(__MODULE__, opts) + end + + def increment(partitions, %IncrementRequest{key: key} = request) do + GenServer.call({:via, PartitionSupervisor, {partitions, key}}, request) + end + + @impl GenServer + def init(_opts), do: {:ok, Counts.new()} + + @impl GenServer + def handle_call(%IncrementRequest{key: key}, _from, %Counts{} = counts) do + {:reply, :ok, Counts.increment(counts, key)} + end +end +``` + +- `PartitionSupervisor` starts one child per scheduler by default (`partitions: System.schedulers_online()`). +- Partitions are not named. Callers address them through the `PartitionSupervisor` with the routing key, so `start_link/1` takes no `:name`. +- The same key always routes to the same partition. + +## Trade-offs + +- Anything that spans keys (totals, "list all") must ask every partition; `PartitionSupervisor.which_children/1` lists them. +- A hot key still serializes on its partition. If one key dominates, partitioning does not help; see [queuing-process-per-entity.md](queuing-process-per-entity.md) or [queuing-reads-from-ets.md](queuing-reads-from-ets.md). +- A crashed partition loses only its own keys' state. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-pool-resources.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-pool-resources.md new file mode 100644 index 0000000..ddcb8fa --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-pool-resources.md @@ -0,0 +1,46 @@ +# Pool a Resource + +When the bottleneck is a scarce external resource (connections, sockets, licenses), the fix is a pool of them, not one process guarding a single instance. + +## Avoid + +```elixir +@impl GenServer +def init(opts) do + {:ok, conn} = PaymentsClient.connect(opts) + {:ok, conn} +end + +@impl GenServer +def handle_call(%ChargeRequest{} = request, _from, conn) do + {:reply, PaymentsClient.charge(conn, request), conn} +end +``` + +One connection, one request at a time, for the whole node. + +## Prefer + +Use the pool your client library already ships before writing one. Ecto pools database connections and Finch pools HTTP connections: + +```elixir +children = [ + {Finch, name: MyApp.Finch, pools: %{"https://payments.example.com" => [size: 25]}} +] + +Finch.build(:post, "https://payments.example.com/charges", headers, body) +|> Finch.request(MyApp.Finch) +``` + +When no library pool exists, build one with `NimblePool`, which checks a resource out to the caller's process so the work runs there and the pool process only hands out resources: + +```elixir +NimblePool.checkout!(MyApp.PaymentsPool, :checkout, fn _from, conn -> + {PaymentsClient.charge(conn, request), conn} +end) +``` + +## Trade-offs + +- Pool size is a capacity decision: too small queues callers at checkout, too large overwhelms the external system. +- Checkout has its own timeout; treat a checkout timeout as overload, not as a bug. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md new file mode 100644 index 0000000..e0fe31d --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md @@ -0,0 +1,74 @@ +# Process per Entity + +When one server holds many independent entities, each with its own state and lifecycle, give each entity its own process. Activity in one room no longer queues behind activity in every other room, and a crash affects one entity. + +## Avoid + +```elixir +@impl GenServer +def handle_call(%PostMessageRequest{room_id: room_id, message: message}, _from, %Rooms{} = rooms) do + case Rooms.post(rooms, room_id, message) do + {:ok, rooms} -> {:reply, :ok, rooms} + {:error, error} -> {:reply, {:error, error}, rooms} + end +end +``` + +One process holds every chat room; a busy room slows down all of them. + +## Prefer + +```elixir +children = [ + {Registry, keys: :unique, name: MyApp.Chat.RoomRegistry}, + {DynamicSupervisor, name: MyApp.Chat.RoomSupervisor, strategy: :one_for_one} +] +``` + +```elixir +defmodule MyApp.Chat.RoomServer do + use GenServer, restart: :transient + + alias MyApp.Chat.{PostMessageRequest, Room} + + def start_link(opts) do + room_id = Keyword.fetch!(opts, :room_id) + GenServer.start_link(__MODULE__, room_id, name: via(room_id)) + end + + def ensure_started(room_id) do + case DynamicSupervisor.start_child(MyApp.Chat.RoomSupervisor, {__MODULE__, room_id: room_id}) do + {:ok, _pid} -> :ok + {:error, {:already_started, _pid}} -> :ok + {:error, error} -> {:error, error} + end + end + + def post(room_id, %PostMessageRequest{} = request) do + GenServer.call(via(room_id), request) + end + + defp via(room_id), do: {:via, Registry, {MyApp.Chat.RoomRegistry, room_id}} + + @impl GenServer + def init(room_id), do: {:ok, Room.new(room_id)} + + @impl GenServer + def handle_call(%PostMessageRequest{message: message}, _from, %Room{} = room) do + case Room.post(room, message) do + {:ok, room} -> {:reply, :ok, room} + {:error, error} -> {:reply, {:error, error}, room} + end + end +end +``` + +- `Registry` maps the entity id to its pid, so callers address rooms by id. +- `:transient` restarts a room that crashes, but not one that stops normally. +- `ensure_started/1` treats `:already_started` as success, which covers two callers racing to start the same room. + +## Trade-offs + +- Idle entities hold memory. Stop them after inactivity, for example by returning a timeout from callbacks and stopping with `{:stop, :normal, state}` on `:timeout`. +- State is lost when the process stops. Load it from durable storage in `handle_continue/2` if it must survive. +- Cross-entity operations become messages between processes; keep them rare or move them to a separate read model. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md new file mode 100644 index 0000000..ae5c270 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md @@ -0,0 +1,66 @@ +# Serve Reads from ETS + +When reads vastly outnumber writes, the server should own the data but not serve every read. Readers go straight to an ETS table; only writes go through the mailbox. + +## Avoid + +```elixir +def enabled?(server, %EnabledRequest{} = request) do + GenServer.call(server, request) +end + +@impl GenServer +def handle_call(%EnabledRequest{flag: flag}, _from, %Flags{} = flags) do + {:reply, Flags.fetch(flags, flag), flags} +end +``` + +Every feature-flag check on every request queues behind every other check. + +## Prefer + +```elixir +defmodule MyApp.FeatureFlags do + use GenServer + + alias MyApp.FeatureFlags.{EnableRequest, FlagNotFoundError} + + def start_link(opts) do + {name, opts} = Keyword.pop!(opts, :name) + GenServer.start_link(__MODULE__, opts, name: name) + end + + def enabled?(table, flag) do + case :ets.lookup(table, flag) do + [{^flag, enabled?}] -> {:ok, enabled?} + [] -> {:error, %FlagNotFoundError{flag: flag}} + end + end + + def enable(server, %EnableRequest{} = request) do + GenServer.call(server, request) + end + + @impl GenServer + def init(opts) do + table = :ets.new(Keyword.fetch!(opts, :table), [:named_table, :protected, read_concurrency: true]) + {:ok, table} + end + + @impl GenServer + def handle_call(%EnableRequest{flag: flag}, _from, table) do + :ets.insert(table, {flag, true}) + {:reply, :ok, table} + end +end +``` + +- `:protected` lets any process read and only the owner write, so writes stay serialized and consistent. +- `read_concurrency: true` optimizes for concurrent readers. +- The caller passes the table name, so several instances can coexist. + +## Trade-offs + +- The table dies with its owner. Readers get `ArgumentError` during the restart window; rebuild the table in `init/1` or `handle_continue/2`, or have a parent process own it. +- A write is visible to readers as soon as `:ets.insert/2` returns, and the `call` returns after that, so the writer reads its own write. +- For data that almost never changes, `:persistent_term` reads are faster still, but every update triggers a global garbage collection scan. Use it for configuration, not for anything updated at runtime. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md new file mode 100644 index 0000000..7462420 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md @@ -0,0 +1,51 @@ +# Remove the Process + +The cheapest queue to fix is the one that should not exist. A GenServer that only forwards to the database, an HTTP client, or a pure function owns nothing, and serializes work that was already concurrent. + +## Avoid + +```elixir +defmodule MyApp.Accounts do + use GenServer + + alias MyApp.Accounts.{GetUserRequest, GetUserResponse, UserNotFoundError} + + def get_user(server, %GetUserRequest{} = request) do + GenServer.call(server, request) + end + + @impl GenServer + def init(_opts), do: {:ok, nil} + + @impl GenServer + def handle_call(%GetUserRequest{id: id}, _from, state) do + case Repo.get(User, id) do + nil -> {:reply, {:error, %UserNotFoundError{id: id}}, state} + user -> {:reply, {:ok, %GetUserResponse{user: user}}, state} + end + end +end +``` + +The state is `nil` forever. Every request in the system waits in this one mailbox while the Repo connection pool sits mostly idle. + +## Prefer + +```elixir +defmodule MyApp.Accounts do + alias MyApp.Accounts.UserNotFoundError + + def get_user(id) do + case Repo.get(User, id) do + nil -> {:error, %UserNotFoundError{id: id}} + user -> {:ok, user} + end + end +end +``` + +The caller's process runs the query. Concurrency is bounded by the Repo pool, which is the resource that actually needs bounding. + +## Trade-offs + +None, as long as the process owned nothing. If removing it breaks something, that something is what the process owns; see [process-justification.md](process-justification.md). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md new file mode 100644 index 0000000..04b983f --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md @@ -0,0 +1,46 @@ +# Shed Load + +When the queue cannot be removed, bound it. Reject new work early with an error the caller can act on, instead of letting it wait and time out after the server already committed to doing the work. + +## Avoid + +```elixir +def take(server, %TakeRequest{} = request) do + GenServer.call(server, request, 30_000) +end +``` + +A longer timeout hides the overload: callers wait longer, the mailbox keeps growing, and work for callers that already gave up still runs. + +## Prefer + +```elixir +@max_queue_length 1_000 + +def take(server, %TakeRequest{} = request) do + if overloaded?(server) do + {:error, %OverloadedError{}} + else + GenServer.call(server, request) + end +end + +defp overloaded?(server) do + with pid when is_pid(pid) <- GenServer.whereis(server), + {:message_queue_len, length} <- Process.info(pid, :message_queue_len) do + length > @max_queue_length + else + _ -> false + end +end +``` + +- The check runs in the caller, so it costs the server nothing. +- `{:error, %OverloadedError{}}` follows the reply shapes in [callback-patterns.md](callback-patterns.md#reply-shapes); callers map it to a retry with backoff or an HTTP 429. +- When the server is not running, `overloaded?/1` returns `false` and the `call` exits as it would have anyway. + +## Trade-offs + +- The check is approximate: the queue can grow between the check and the send. That is fine for shedding, which only needs to stop runaway growth. +- `Process.info/2` works only for local pids. For remote servers, track in-flight requests with a counter (`:atomics` or `:ets.update_counter/3`) instead. +- Shedding is a last resort. It keeps the node alive, but callers still fail; prefer the patterns that remove the queue. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index cd96f76..ecad336 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -10,6 +10,8 @@ - [ ] Hot-path throughput fits within `1 / handling_time` of one process, or the work is partitioned - [ ] No unbounded casts from producers that can outrun the server - [ ] No blocking I/O inside callbacks on a hot path +- [ ] Replies return only what the caller needs, not whole collections +- [ ] Handling time and mailbox length are measured with telemetry ## Module Shape ([callback-patterns.md](callback-patterns.md)) @@ -18,8 +20,16 @@ - [ ] `init/1` is fast; slow work runs in `handle_continue/2`, unless siblings need the state at boot - [ ] Persistent dependency failures retry with backoff instead of crash-looping the supervisor - [ ] Replies and core results are `:ok`, `{:ok, value}`, or `{:error, error}`, never longer tuples -- [ ] `handle_info/2` has a catch-all clause - [ ] Process names never built from untrusted input +- [ ] Secrets in state are redacted with `format_status/1` + +## handle_info ([handle-info.md](handle-info.md)) + +- [ ] Callbacks never call the module's own client API +- [ ] Self-sent and timer messages are structs; OTP and library tuples are matched as defined +- [ ] Timers use `Process.send_after/3` with a token to ignore stale ticks +- [ ] Pids held in state are monitored and removed on `:DOWN` +- [ ] `handle_info/2` ends with a catch-all clause ## Naming ([naming.md](naming.md)) From b71a595d5a5e7ca1f4a53394dd05127ce94928c9 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 13:33:54 -0400 Subject: [PATCH 12/29] fix(trogonstack-elixir): enforce a hard queue bound and compile the catch-all example Signed-off-by: Yordis Prieto --- .../references/handle-info.md | 4 ++- .../references/queuing-shed-load.md | 32 +++++++++++++++++-- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md index 1937cc7..bc0c8e0 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md @@ -105,10 +105,12 @@ Results of `Task.Supervisor.async_nolink/2` arrive as `{ref, result}`, and a cra `use GenServer` injects a `handle_info/2` that logs unexpected messages. Defining your own replaces it, so end with a catch-all clause or an unknown message crashes the server with a `FunctionClauseError`: ```elixir +require Logger + def handle_info(msg, state) do Logger.warning("unexpected message: #{inspect(msg)}") {:noreply, state} end ``` -Keep it as the last clause; clauses after it never match. +Keep it as the last clause; clauses after it never match. `Logger.warning/1` is a macro, so the module needs `require Logger`. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md index 04b983f..e866797 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md @@ -39,8 +39,36 @@ end - `{:error, %OverloadedError{}}` follows the reply shapes in [callback-patterns.md](callback-patterns.md#reply-shapes); callers map it to a retry with backoff or an HTTP 429. - When the server is not running, `overloaded?/1` returns `false` and the `call` exits as it would have anyway. +The mailbox check is best-effort: concurrent callers can all see a length below the limit and enqueue together, so it cannot enforce a hard bound. + +## Hard Bound + +Reserve a slot atomically before sending, and release it when the call returns: + +```elixir +@max_in_flight 1_000 + +def take(server, admission, %TakeRequest{} = request) do + if :atomics.add_get(admission, 1, 1) > @max_in_flight do + :atomics.sub(admission, 1, 1) + {:error, %OverloadedError{}} + else + try do + GenServer.call(server, request) + after + :atomics.sub(admission, 1, 1) + end + end +end +``` + +- `admission` is `:atomics.new(1, signed: true)`, created once by whoever starts the server and passed to callers, for example through the same place the server name lives. +- The counter bounds calls in flight (queued plus being handled), which is a hard upper bound on the mailbox for this operation. +- `after` releases the slot even when the call exits on timeout. + ## Trade-offs -- The check is approximate: the queue can grow between the check and the send. That is fine for shedding, which only needs to stop runaway growth. -- `Process.info/2` works only for local pids. For remote servers, track in-flight requests with a counter (`:atomics` or `:ets.update_counter/3`) instead. +- Use the mailbox check when stopping runaway growth is enough; use admission control when the bound must hold. +- `Process.info/2` works only for local pids. For remote servers, use admission control. +- Admission control counts only callers that go through it; casts and messages from elsewhere still reach the mailbox. - Shedding is a last resort. It keeps the node alive, but callers still fail; prefer the patterns that remove the queue. From 375a9438138ad4c8f27d1f5d5f6831f4807976ad Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 13:46:54 -0400 Subject: [PATCH 13/29] feat(trogonstack-elixir): keep GenServer contracts extensible so new fields do not break callers Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 2 + .../references/callback-patterns.md | 2 + .../references/naming.md | 4 +- .../references/request-response.md | 79 +++++++++++++++++++ .../references/review-checklist.md | 7 ++ 5 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index a9c53f2..7827eaf 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -42,6 +42,8 @@ Read [references/handle-info.md](references/handle-info.md) when the server send Read [references/naming.md](references/naming.md) when naming the public module, the pure core, the process state, or the Request/Response structs the GenServer exchanges. +Read [references/request-response.md](references/request-response.md) when designing or changing what a client function takes or returns, especially a reply that holds a list. + ### 4. Supervise and Test Pick a restart strategy that matches the lifecycle, and test the pure core without processes. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index 31240d3..f83d10f 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -53,6 +53,8 @@ Every reply, and every result the pure core returns, is one of: Never reply with a tuple of more than two elements such as `{:ok, value, extra}` or `{:error, reason, details}`. Put the extra data in the struct. Callers can then handle every operation with the same `case` or `with`, and adding a field never breaks a pattern match. +Reply with a Response struct rather than a bare list or value, so the reply can grow; see [request-response.md](request-response.md). + This rule covers the replies you design. OTP callback return values such as `{:reply, reply, state}` or `{:ok, state, {:continue, term}}` keep the shapes OTP requires. ## call vs cast vs send diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md index 8f1624c..2d124c9 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md @@ -68,9 +68,9 @@ Prefix the server module with the domain name instead of a bare `.Server`. A con Exchange structs with the GenServer instead of tuples like `{:allow?, key}`. Name them `Request` and `Response`, nested under the public module: - The pairing stays unambiguous as operations grow: `TakeRequest` and `TakeResponse`, `RefillRequest` and `RefillResponse`. -- Adding a field does not break pattern matches the way growing a tuple does. - The client function takes the request and returns `{:ok, %TakeResponse{}}` or `{:error, error}`. -- An operation with nothing to return replies `:ok` and needs no Response struct. + +For why bare lists and values never appear in replies, and how to grow these structs without breaking callers, see [request-response.md](request-response.md). Messages the server sends itself (`send/2`, timers) are structs too, for example `%RefillTick{}`; see [handle-info.md](handle-info.md#internal-message-structs). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md new file mode 100644 index 0000000..e62d32b --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md @@ -0,0 +1,79 @@ +# Request and Response Contracts + +The client API is a contract with every caller. Shape it so it can grow without breaking them: wrap what goes in and what comes out in structs, never bare values. + +## Avoid + +```elixir +def list_sessions(server) do + GenServer.call(server, :list_sessions) +end + +@impl GenServer +def handle_call(:list_sessions, _from, %Sessions{} = sessions) do + {:reply, {:ok, Sessions.all(sessions)}, sessions} +end +``` + +```elixir +{:ok, sessions} = SessionStore.list_sessions(server) +``` + +The day the operation needs pagination, there is nowhere to put the cursor or the page size. Every option breaks callers: + +- `{:ok, sessions, cursor}` breaks every match and violates the [reply shapes](callback-patterns.md#reply-shapes). +- `{:ok, %{sessions: sessions, cursor: cursor}}` breaks every caller that matched a list. +- `list_sessions(server, limit)` changes the arity, and every call site. + +## Prefer + +```elixir +defmodule MyApp.SessionStore.ListSessionsRequest do + defstruct limit: 100, cursor: nil +end + +defmodule MyApp.SessionStore.ListSessionsResponse do + defstruct sessions: [], next_cursor: nil +end +``` + +```elixir +def list_sessions(server, %ListSessionsRequest{} = request) do + GenServer.call(server, request) +end + +@impl GenServer +def handle_call(%ListSessionsRequest{} = request, _from, %Sessions{} = sessions) do + {page, next_cursor} = Sessions.page(sessions, request.cursor, request.limit) + {:reply, {:ok, %ListSessionsResponse{sessions: page, next_cursor: next_cursor}}, sessions} +end +``` + +```elixir +{:ok, %ListSessionsResponse{sessions: sessions}} = + SessionStore.list_sessions(server, %ListSessionsRequest{}) +``` + +New request options and new response fields are new struct fields with defaults. Existing callers keep compiling and keep matching. + +## Rules + +- Every operation takes an `Request`, even with no fields today: `%ListSessionsRequest{}` leaves room for filters and pagination. +- Every success with data replies `{:ok, %Response{}}`. Never reply with a bare list, map, integer, or boolean. +- Collections live in a named field (`sessions`, `items`), never as the whole reply. The struct is where metadata (cursor, total, truncation) lands later. +- Reply `:ok` only for commands that will never return data. When an operation might, start with an empty Response struct; changing `:ok` to `{:ok, response}` later breaks every caller. +- Callers match on the struct name and the fields they read, `%ListSessionsResponse{sessions: sessions}`, never on the whole struct. Matching the fields they need keeps them indifferent to new ones. +- Add fields with defaults. Never rename or remove a field callers may read; add the new one and keep the old one until callers move. +- Decide `@enforce_keys` when the struct is created. Enforcing a key later breaks every caller that builds the struct without it. +- Errors follow the same rule: `defexception` structs, so a new detail is a new field. See [naming.md](naming.md#errors). + +## Boundaries + +Request and Response structs exist only at the GenServer boundary. `handle_call/3` unpacks the request and calls the pure core with domain values; the core returns domain values that the server wraps in the response. See [naming.md](naming.md#messages). + +Items inside a response are domain structs (`%Session{}`), not tuples, for the same reason the reply is a struct. + +## Trade-offs + +- A struct per operation is more modules. That cost is paid once; a breaking change is paid at every call site. +- A response struct does not make a large reply cheap; it is still copied to the caller. Return a page or a lookup, not the whole collection. See [queuing-large-replies.md](queuing-large-replies.md). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index ecad336..8f37bb6 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -39,6 +39,13 @@ - [ ] GenServer messages are `Request` / `Response` structs, not tuples - [ ] The pure core never references the Request/Response structs +## Contracts ([request-response.md](request-response.md)) + +- [ ] Every operation takes a Request struct, even with no fields +- [ ] Replies carry a Response struct, never a bare list, map, or scalar +- [ ] Collections sit in a named field of the Response +- [ ] New fields have defaults; no field callers read was renamed or removed + ## Supervision ([supervision-and-testing.md](supervision-and-testing.md)) - [ ] Restart strategy matches the process lifecycle From 371431ec2cb9b42fc6e6190b2dce55f118612dc4 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 13:51:39 -0400 Subject: [PATCH 14/29] feat(trogonstack-elixir): warn against GenServers that gatekeep database writes for false safety Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 2 + .../anti-pattern-database-gatekeeper.md | 92 +++++++++++++++++++ .../references/process-justification.md | 2 + .../references/queuing-remove-the-process.md | 4 +- .../references/review-checklist.md | 7 ++ 5 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index 7827eaf..1958bad 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -26,6 +26,8 @@ Name what the process owns. If the answer is "the functions in this module", or Read [references/process-justification.md](references/process-justification.md) when the decision is not obvious, or when reviewing code that uses GenServers as a service layer. +Read [references/anti-pattern-database-gatekeeper.md](references/anti-pattern-database-gatekeeper.md) when a GenServer wraps database writes, or is justified as fault tolerance or as a way to avoid race conditions. + ### 2. Check the Queue Estimate arrival rate and handling time on the hot path. A single server tops out at `1 / handling_time` messages per second regardless of core count. If callers can outpace it, the mailbox grows and latency grows with it. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md new file mode 100644 index 0000000..f51e881 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md @@ -0,0 +1,92 @@ +# Anti-Pattern: Database Gatekeeper + +A GenServer stands in front of the database and every callback wraps a `Repo` call. It is usually built for fault tolerance or to serialize writes, and it delivers neither. The database already provides both, for every writer, on every node. + +## Avoid + +```elixir +defmodule MyApp.Accounts.AccountsServer do + use GenServer + + @impl GenServer + def init(_opts), do: {:ok, nil} + + @impl GenServer + def handle_call(%DebitRequest{account_id: account_id, amount: amount}, _from, state) do + account = Repo.get!(Account, account_id) + + reply = + account + |> Account.debit_changeset(amount) + |> Repo.update() + + {:reply, reply, state} + end +end +``` + +## What It Seems to Give + +| Belief | Reality | +|--------|---------| +| Supervision makes the write fault tolerant | A restart does not retry the write. The in-flight call is lost and the caller exits. The transaction is the failure boundary; retries belong to the caller, with an idempotency key. | +| One process linearizes writes, so there are no races | It serializes only callers that go through it, on one node. Other nodes, the overlap during a rolling deploy, migrations, jobs, other services, and `iex` all write around it. | +| It protects the database from load | The Repo pool already bounds connections and queues callers. The GenServer adds a pool of size one in front of it. | +| `cast` makes writes fast and async | The mailbox is not durable. A crash or deploy drops every queued write without a trace. | +| It caches rows in its state | Two sources of truth that drift the moment anyone else writes. | + +## Prefer + +Run the write in the caller's process and let the database linearize it: + +```elixir +defmodule MyApp.Accounts do + import Ecto.Query + + def debit(account_id, amount) do + Repo.transact(fn -> + Account + |> where([a], a.id == ^account_id) + |> lock("FOR UPDATE") + |> Repo.one!() + |> Account.debit_changeset(amount) + |> Repo.update() + end) + end +end +``` + +The row lock serializes concurrent debits of the same account, from any process on any node, while debits of other accounts run in parallel. `Repo.transact/2` (Ecto 3.13+) commits on `{:ok, value}` and rolls back on `{:error, error}`. + +Pick the database mechanism that matches the guarantee: + +| Need | Use | +|------|-----| +| No duplicates | Unique index, and `Repo.insert(changeset, on_conflict: :nothing, conflict_target: :email)` for upserts | +| No lost updates on a contended row | `lock("FOR UPDATE")` inside a transaction | +| No lost updates on a rarely contended row | `Ecto.Changeset.optimistic_lock/3`, retrying on `Ecto.StaleEntryError` | +| Ordering or sequence numbers | A database sequence or a version column | +| Mutual exclusion that is not a single row | `SELECT pg_advisory_xact_lock($1)` inside the transaction | +| Async writes that must survive a crash | A persisted job queue such as Oban | + +## When a Process Belongs Near the Database + +The process must own something the database does not: + +- Time and a buffer: write-behind batching that collects rows and flushes them with `Repo.insert_all/3` on a timer or size threshold. It trades durability for throughput, so document the loss window and flush in `terminate/2` with `Process.flag(:trap_exit, true)`. +- A connection-shaped resource: a `Postgrex.Notifications` listener for `LISTEN`/`NOTIFY`. +- Authoritative in-memory state: a game room or auction that runs in memory and persists snapshots. While it runs, the process is the source of truth and the database is the backup, never both at once. + +## Smells in Review + +- `handle_call/3` bodies are `Repo` calls and the state is `nil` or unused +- The process is justified as "fault tolerance" or "to avoid race conditions" +- Writes go through `cast` +- The state holds rows that other code also writes +- The module has no unique indexes, locks, or transactions because the process "already serializes" + +## Trade-offs + +- Row locks and advisory locks hold a connection while waiting; keep transactions short and never call external services inside them. +- Optimistic locking pushes retries to the caller; it fits rows that rarely conflict. +- Throughput cost of the gatekeeper is covered in [queuing-remove-the-process.md](queuing-remove-the-process.md). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md index 91a7e44..71c01ad 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md @@ -18,6 +18,7 @@ Invalid answers: - "It is the service layer for users / orders / billing." That is a module. - "It encapsulates the logic." Modules encapsulate logic. - "It wraps the database / HTTP client." Those calls are already concurrent and pooled; a GenServer in front serializes them. +- "It makes writes fault tolerant" or "it prevents race conditions." Neither holds for database writes; see [anti-pattern-database-gatekeeper.md](anti-pattern-database-gatekeeper.md). - "It runs in the background." That is a `Task` under a `Task.Supervisor`. - "It caches data." Reads belong in ETS or `:persistent_term`; a process may own the table, but readers should not message it. @@ -42,6 +43,7 @@ A GenServer whose state is never used, forwarding every request to the database - `state` is ignored or is `nil` / `%{}` forever - Every `handle_call` clause is a thin pass-through to another module +- Callbacks wrap `Repo` calls ([anti-pattern-database-gatekeeper.md](anti-pattern-database-gatekeeper.md)) - The module is a named singleton and every web request calls it - The GenServer exists "for consistency" with other modules - Removing `use GenServer` and the callbacks would change nothing but concurrency diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md index 7462420..0c17dda 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md @@ -48,4 +48,6 @@ The caller's process runs the query. Concurrency is bounded by the Repo pool, wh ## Trade-offs -None, as long as the process owned nothing. If removing it breaks something, that something is what the process owns; see [process-justification.md](process-justification.md). +None, as long as the process owned nothing. If it was built to make writes fault tolerant or race free, see [anti-pattern-database-gatekeeper.md](anti-pattern-database-gatekeeper.md) for what provides those guarantees instead. + +If removing it breaks something, that something is what the process owns; see [process-justification.md](process-justification.md). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index 8f37bb6..d82d51b 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -5,6 +5,13 @@ - [ ] The process owns something concrete: shared state, a resource, a lifecycle, or a failure boundary - [ ] No stateless operation (DB, HTTP, computation) is routed through it +## Database Writes ([anti-pattern-database-gatekeeper.md](anti-pattern-database-gatekeeper.md)) + +- [ ] Write correctness comes from constraints, transactions, and locks, not from routing writes through one process +- [ ] No writes that must survive a crash go through `cast` or a mailbox +- [ ] State does not cache rows that other code also writes +- [ ] Write-behind buffers document their loss window and flush on shutdown + ## Queuing ([mailbox-queuing.md](mailbox-queuing.md)) - [ ] Hot-path throughput fits within `1 / handling_time` of one process, or the work is partitioned From 356a5c94182e71574569cde5de6cda4065b559a2 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 13:58:41 -0400 Subject: [PATCH 15/29] docs(trogonstack-elixir): lead with principles so readers can judge cases the examples do not cover Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 28 +++++++++++-------- .../references/callback-patterns.md | 2 ++ .../references/handle-info.md | 2 ++ .../references/mailbox-queuing.md | 2 ++ .../references/process-justification.md | 2 ++ .../references/request-response.md | 22 ++++++++------- .../references/review-checklist.md | 2 ++ .../references/supervision-and-testing.md | 2 ++ 8 files changed, 41 insertions(+), 21 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index 1958bad..f42b25f 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -18,40 +18,46 @@ A process is the unit of concurrency, of serialization, and of failure, all at o Wrapping an operation that needs none of those concerns in a GenServer does not add structure. It adds a queue that turns concurrent callers into sequential ones. -## Workflow +## Principles -### 1. Justify the Process +Apply them in order. A design that fails an earlier principle makes the later ones moot. Each one names the references that show it in practice; read them when the situation matches, not up front. -Name what the process owns. If the answer is "the functions in this module", or the work is a DB query, HTTP request, or pure computation the caller could run itself, do not write a GenServer. +### 1. A Process Must Own Something + +A process earns its place by owning what cannot live in the caller: shared state that must change consistently, a resource with its own lifecycle, time, or a failure boundary. If it owns nothing, the caller can do the work itself, concurrently. Read [references/process-justification.md](references/process-justification.md) when the decision is not obvious, or when reviewing code that uses GenServers as a service layer. Read [references/anti-pattern-database-gatekeeper.md](references/anti-pattern-database-gatekeeper.md) when a GenServer wraps database writes, or is justified as fault tolerance or as a way to avoid race conditions. -### 2. Check the Queue +### 2. Every Process Is a Queue -Estimate arrival rate and handling time on the hot path. A single server tops out at `1 / handling_time` messages per second regardless of core count. If callers can outpace it, the mailbox grows and latency grows with it. +One process handles one message at a time, so it caps throughput at `1 / handling_time` regardless of core count, and every caller waits behind every other. Know the arrival rate and handling time on the hot path before adding one, and remove the queue rather than tuning around it. Read [references/mailbox-queuing.md](references/mailbox-queuing.md) when the server sits on a request path, receives casts from many producers, does I/O inside callbacks, or already shows timeouts or a growing `message_queue_len`. It links to an avoid/prefer example for each way of removing the queue. -### 3. Shape the Module +### 3. The Process Is a Shell Around Meaning -Client API wraps every `call`/`cast`, callbacks delegate to a pure core, `call` is the default, `init/1` stays fast, and `handle_info/2` has a catch-all. +The domain lives in a pure, deterministic core named after what it means. The GenServer only translates messages into calls on that core and owns what the core must not know about: the clock, timers, monitors, and other processes. Logic that needs no process stays testable and reusable without one. Read [references/callback-patterns.md](references/callback-patterns.md) when writing or reviewing the module body. Read [references/handle-info.md](references/handle-info.md) when the server sends itself messages, uses timers, monitors other processes, or receives task results. -Read [references/naming.md](references/naming.md) when naming the public module, the pure core, the process state, or the Request/Response structs the GenServer exchanges. +Read [references/naming.md](references/naming.md) when naming the public module, the pure core, the process state, or the messages the GenServer exchanges. + +### 4. Contracts Outlive Implementations + +The client API is used by code you do not control and cannot change in lockstep. Shape what goes in and what comes out so it can grow without breaking callers. Read [references/request-response.md](references/request-response.md) when designing or changing what a client function takes or returns, especially a reply that holds a list. -### 4. Supervise and Test +### 5. Crash Only When a Restart Helps -Pick a restart strategy that matches the lifecycle, and test the pure core without processes. +A restart starts from a clean state, which fixes corrupt state and transient faults and nothing else. Match the restart strategy to the lifecycle, and handle failures that persist across restarts instead of crash-looping. Read [references/supervision-and-testing.md](references/supervision-and-testing.md) when wiring the child spec, handling cleanup, or writing tests. ## Reviewing an Existing GenServer -Read [references/review-checklist.md](references/review-checklist.md) when reviewing or auditing GenServer code. Each section links to the reference that explains a failing item. +Read [references/review-checklist.md](references/review-checklist.md) when reviewing or auditing GenServer code. Each item is a symptom; a failing item is a question about the principle behind it, answered in the linked reference. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index f83d10f..c757a10 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -1,5 +1,7 @@ # Callback Patterns +The GenServer module is plumbing between callers and the pure core. The patterns below keep that plumbing thin, give callers one predictable way to handle results, and keep the process safe to start and restart. + ## Module Layout ```elixir diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md index bc0c8e0..2c0bc2a 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md @@ -1,5 +1,7 @@ # handle_info and Self-Messaging +Messages outside the client API are still a protocol. Give the ones you send a name, accept that you will receive ones you did not send, and never assume a message reflects the current state by the time it arrives. + `handle_info/2` receives every message that did not come through `GenServer.call/cast`: messages the server sends itself, timers, monitors, task results, exit signals, and messages from libraries. ## What Arrives Here diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md index 82cb5d4..8866783 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md @@ -1,5 +1,7 @@ # Mailbox Queuing +A GenServer serializes everything sent to it. That is the point when it protects state that must change consistently, and pure cost when it does not. Remove serialization the problem does not need; bound the serialization it does. + ## The Model A GenServer is a queue with exactly one worker: diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md index 71c01ad..e3525cb 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/process-justification.md @@ -1,5 +1,7 @@ # Where an Actor Belongs, and Why +A process is justified by what it owns at runtime, never by the code it contains. Code is organized by modules; a process exists because something must be owned by exactly one place while many callers use it. + ## The Question to Ask "What does this process own that cannot live in the caller?" diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md index e62d32b..87d1eaf 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md @@ -56,16 +56,18 @@ end New request options and new response fields are new struct fields with defaults. Existing callers keep compiling and keep matching. -## Rules - -- Every operation takes an `Request`, even with no fields today: `%ListSessionsRequest{}` leaves room for filters and pagination. -- Every success with data replies `{:ok, %Response{}}`. Never reply with a bare list, map, integer, or boolean. -- Collections live in a named field (`sessions`, `items`), never as the whole reply. The struct is where metadata (cursor, total, truncation) lands later. -- Reply `:ok` only for commands that will never return data. When an operation might, start with an empty Response struct; changing `:ok` to `{:ok, response}` later breaks every caller. -- Callers match on the struct name and the fields they read, `%ListSessionsResponse{sessions: sessions}`, never on the whole struct. Matching the fields they need keeps them indifferent to new ones. -- Add fields with defaults. Never rename or remove a field callers may read; add the new one and keep the old one until callers move. -- Decide `@enforce_keys` when the struct is created. Enforcing a key later breaks every caller that builds the struct without it. -- Errors follow the same rule: `defexception` structs, so a new detail is a new field. See [naming.md](naming.md#errors). +## Growing Without Breaking + +A change to the contract should be additive: a new field with a default. A shape that can only change by being replaced forces every caller to change at once. That one idea explains every choice below. + +- **A Request, even with no fields.** An operation that takes nothing today takes a filter or a page size tomorrow. A field is additive; a new argument changes the arity. +- **A Response, not a bare value.** A bare list, map, integer, or boolean has no room for a second piece of data, so the first piece of metadata (cursor, total, truncation) replaces the shape. +- **Collections in a named field.** Data about a collection belongs next to it, and a field named `sessions` leaves room for `next_cursor`. +- **`:ok` only when there is never anything to say.** Moving from `:ok` to `{:ok, response}` is a replacement. When an operation might return data later, start with an empty Response struct. +- **Callers match what they read.** A struct pattern such as `%ListSessionsResponse{sessions: sessions}` ignores fields added later. Comparing whole structs with `==`, in code or in test assertions, breaks as soon as a new field carries a value. +- **Evolve by addition.** Renaming or removing a field is a replacement. Add the new field and keep the old one until callers move. +- **Enforced keys are decided once.** Adding `@enforce_keys` later breaks every caller that builds the struct without the key. +- **Errors grow the same way.** `defexception` structs carry new detail as new fields; see [naming.md](naming.md#errors). ## Boundaries diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index d82d51b..0ad62b4 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -1,5 +1,7 @@ # Review Checklist +Each item is a symptom of a principle, not a rule on its own. When an item fails, read the linked reference and judge the code by the principle behind it; a justified exception passes, a compliant design that misses the point does not. + ## Justification ([process-justification.md](process-justification.md)) - [ ] The process owns something concrete: shared state, a resource, a lifecycle, or a failure boundary diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md index 0ff65c4..39455a0 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md @@ -1,5 +1,7 @@ # Supervision and Testing +A restart repairs state and nothing else: it fixes corruption and transient faults, and repeats any fault that lives outside the process. Supervise for the faults a clean start fixes, and test logic where it lives, in the pure core. + ## Restart Strategy `use GenServer` generates `child_spec/1` with `restart: :permanent`. Override it when that is wrong: From 85f18b5a972593de43916fde30ad871dd55612cc Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 14:08:25 -0400 Subject: [PATCH 16/29] fix(trogonstack-elixir): keep examples from demonstrating the anti-patterns they warn about Signed-off-by: Yordis Prieto --- .../anti-pattern-database-gatekeeper.md | 12 ++-- .../references/callback-patterns.md | 4 +- .../references/mailbox-queuing.md | 14 +++-- .../references/queuing-chunk-long-work.md | 10 +++- .../references/queuing-large-replies.md | 22 +++---- .../references/queuing-offload-slow-work.md | 60 ++++++++++++++----- .../references/queuing-partition-by-key.md | 26 ++++---- .../references/queuing-pool-resources.md | 6 +- .../references/queuing-reads-from-ets.md | 29 +++++---- .../references/request-response.md | 48 ++++++++------- 10 files changed, 142 insertions(+), 89 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md index f51e881..eff62c1 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md @@ -45,12 +45,12 @@ defmodule MyApp.Accounts do def debit(account_id, amount) do Repo.transact(fn -> - Account - |> where([a], a.id == ^account_id) - |> lock("FOR UPDATE") - |> Repo.one!() - |> Account.debit_changeset(amount) - |> Repo.update() + query = Account |> where([a], a.id == ^account_id) |> lock("FOR UPDATE") + + case Repo.one(query) do + nil -> {:error, %AccountNotFoundError{account_id: account_id}} + %Account{} = account -> account |> Account.debit_changeset(amount) |> Repo.update() + end end) end end diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index c757a10..c0bace0 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -45,7 +45,7 @@ end ## Reply Shapes -Every reply, and every result the pure core returns, is one of: +Every reply, and every core function that can fail, returns one of: | Shape | When | |-------|------| @@ -55,6 +55,8 @@ Every reply, and every result the pure core returns, is one of: Never reply with a tuple of more than two elements such as `{:ok, value, extra}` or `{:error, reason, details}`. Put the extra data in the struct. Callers can then handle every operation with the same `case` or `with`, and adding a field never breaks a pattern match. +Core functions that cannot fail return their value directly, as `Bucket.remaining/2` returns an integer. When they return more than one value, they return a struct, never a loose tuple such as `{messages, more?}`. + Reply with a Response struct rather than a bare list or value, so the reply can grow; see [request-response.md](request-response.md). This rule covers the replies you design. OTP callback return values such as `{:reply, reply, state}` or `{:ok, state, {:continue, term}}` keep the shapes OTP requires. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md index 8866783..1a8677c 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md @@ -46,10 +46,16 @@ Measure handling time per callback, so the `1 / handling_time` ceiling is a numb ```elixir @impl GenServer -def handle_call(%TakeRequest{} = request, _from, %Bucket{} = bucket) do - :telemetry.span([:my_app, :rate_limiter, :take], %{}, fn -> - {take(request, bucket), %{}} - end) +def handle_call(%TakeRequest{key: key}, _from, %Bucket{} = bucket) do + result = + :telemetry.span([:my_app, :rate_limiter, :take], %{}, fn -> + {Bucket.take(bucket, key), %{}} + end) + + case result do + {:ok, bucket} -> {:reply, {:ok, %TakeResponse{remaining: Bucket.remaining(bucket, key)}}, bucket} + {:error, error} -> {:reply, {:error, error}, bucket} + end end ``` diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md index a3cd890..7123bf8 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md @@ -23,8 +23,10 @@ Importing 100,000 rows blocks every lookup until the import ends. @impl GenServer def handle_call(%ImportRequest{rows: rows}, _from, %CatalogServerState{} = state) do - send(self(), %ImportNextBatch{}) - {:reply, :ok, %CatalogServerState{state | import_queue: state.import_queue ++ rows}} + if state.import_queue == [], do: send(self(), %ImportNextBatch{}) + import_queue = state.import_queue ++ rows + response = %ImportResponse{pending_rows: length(import_queue)} + {:reply, {:ok, response}, %CatalogServerState{state | import_queue: import_queue}} end @impl GenServer @@ -42,9 +44,11 @@ end - `send(self(), msg)` puts the message at the back of the mailbox, behind client messages already waiting. That is what lets them interleave. - `{:continue, term}` would run before any other message, which defeats the purpose here; see [handle-info.md](handle-info.md#deferring-work-to-yourself). -- The reply `:ok` means "accepted", not "imported". Expose progress through a separate request if callers need it. +- Only a request that finds the queue empty schedules a batch. A second import joins the queue; scheduling again would start a second chain and double the batch rate. +- The reply means "accepted", not "imported", and reports the backlog. Progress is new fields on `ImportResponse` or a separate request. ## Trade-offs - Lookups during the import see a partially imported catalog. If that is not acceptable, build the new catalog in a task and swap it in with one message. - Batch size trades import throughput against lookup latency: a lookup waits at most one batch. +- The rows are copied into the server with the request; see [queuing-large-replies.md](queuing-large-replies.md). For large imports, send a source such as a file path and read it batch by batch. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-large-replies.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-large-replies.md index 9427e71..4781020 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-large-replies.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-large-replies.md @@ -8,33 +8,33 @@ Binaries larger than 64 bytes are the exception: they live in a shared heap and ```elixir @impl GenServer -def handle_call(%ListSessionsRequest{}, _from, %Sessions{} = sessions) do - {:reply, {:ok, %ListSessionsResponse{sessions: Sessions.all(sessions)}}, sessions} +def handle_call(%GetRoomRequest{}, _from, %Room{} = room) do + {:reply, {:ok, %GetRoomResponse{room: room}}, room} end ``` ```elixir -{:ok, %ListSessionsResponse{sessions: sessions}} = SessionStore.list_sessions(server, %ListSessionsRequest{}) -Enum.find(sessions, &(&1.user_id == user_id)) +{:ok, %GetRoomResponse{room: room}} = RoomServer.get_room(room_id, %GetRoomRequest{}) +Enum.find(room.members, &(&1.user_id == user_id)) ``` -The caller wants one session and receives 100,000 of them. +The caller wants one member and receives the whole room: every member and the full message history. Handing out the state is how this usually starts. ## Prefer -Ask the server for what you need, and let it do the filtering: +Ask the process the question instead of asking for its state: ```elixir @impl GenServer -def handle_call(%FindSessionRequest{user_id: user_id}, _from, %Sessions{} = sessions) do - case Sessions.find_by_user(sessions, user_id) do - {:ok, session} -> {:reply, {:ok, %FindSessionResponse{session: session}}, sessions} - {:error, error} -> {:reply, {:error, error}, sessions} +def handle_call(%FetchMemberRequest{user_id: user_id}, _from, %Room{} = room) do + case Room.fetch_member(room, user_id) do + {:ok, member} -> {:reply, {:ok, %FetchMemberResponse{member: member}}, room} + {:error, error} -> {:reply, {:error, error}, room} end end ``` -When callers genuinely need to scan large data, serve it from ETS instead; see [queuing-reads-from-ets.md](queuing-reads-from-ets.md). +When many callers read the same data, serve it from ETS instead of the mailbox; see [queuing-reads-from-ets.md](queuing-reads-from-ets.md). ## Trade-offs diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-offload-slow-work.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-offload-slow-work.md index f0ba0d0..abdd8b7 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-offload-slow-work.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-offload-slow-work.md @@ -23,43 +23,73 @@ A 2 second HTTP call caps the server at one request every 2 seconds, and every o ```elixir @impl GenServer def handle_call(%FetchRateRequest{currency: currency}, from, %RatesServerState{} = state) do - task = Task.Supervisor.async_nolink(state.task_supervisor, fn -> RatesClient.fetch(currency) end) - pending = Map.put(state.pending, task.ref, %PendingFetch{from: from, currency: currency}) - {:noreply, %RatesServerState{state | pending: pending}} + case RateCache.fetch(state.cache, currency) do + {:ok, rate} -> {:reply, {:ok, %FetchRateResponse{rate: rate}}, state} + {:error, %RateNotCachedError{}} -> {:noreply, await_fetch(state, currency, from)} + end end @impl GenServer -def handle_info({ref, result}, %RatesServerState{} = state) when is_map_key(state.pending, ref) do +def handle_info({ref, result}, %RatesServerState{} = state) when is_map_key(state.fetching, ref) do Process.demonitor(ref, [:flush]) - {%PendingFetch{} = fetch, pending} = Map.pop(state.pending, ref) - state = %RatesServerState{state | pending: pending} + {%PendingFetch{} = fetch, state} = pop_fetch(state, ref) case result do {:ok, rate} -> - GenServer.reply(fetch.from, {:ok, %FetchRateResponse{rate: rate}}) + reply_all(fetch, {:ok, %FetchRateResponse{rate: rate}}) {:noreply, %RatesServerState{state | cache: RateCache.put(state.cache, fetch.currency, rate)}} {:error, error} -> - GenServer.reply(fetch.from, {:error, error}) + reply_all(fetch, {:error, error}) {:noreply, state} end end def handle_info({:DOWN, ref, :process, _pid, reason}, %RatesServerState{} = state) - when is_map_key(state.pending, ref) do - {%PendingFetch{} = fetch, pending} = Map.pop(state.pending, ref) - GenServer.reply(fetch.from, {:error, %FetchFailedError{reason: reason}}) - {:noreply, %RatesServerState{state | pending: pending}} + when is_map_key(state.fetching, ref) do + {%PendingFetch{} = fetch, state} = pop_fetch(state, ref) + reply_all(fetch, {:error, %FetchFailedError{reason: reason}}) + {:noreply, state} +end + +defp await_fetch(%RatesServerState{} = state, currency, from) do + case Map.fetch(state.pending, currency) do + {:ok, %PendingFetch{} = fetch} -> + pending = Map.put(state.pending, currency, %PendingFetch{fetch | callers: [from | fetch.callers]}) + %RatesServerState{state | pending: pending} + + :error -> + task = Task.Supervisor.async_nolink(state.task_supervisor, RatesClient, :fetch, [currency]) + fetch = %PendingFetch{currency: currency, callers: [from]} + + %RatesServerState{ + state + | pending: Map.put(state.pending, currency, fetch), + fetching: Map.put(state.fetching, task.ref, currency) + } + end +end + +defp pop_fetch(%RatesServerState{} = state, ref) do + {currency, fetching} = Map.pop!(state.fetching, ref) + {fetch, pending} = Map.pop!(state.pending, currency) + {fetch, %RatesServerState{state | pending: pending, fetching: fetching}} +end + +defp reply_all(%PendingFetch{callers: callers}, reply) do + Enum.each(callers, &GenServer.reply(&1, reply)) end ``` -- `async_nolink` keeps a crashing task from taking the server down; the crash arrives as `:DOWN`. -- Returning `{:noreply, state}` from `handle_call/3` frees the server; `GenServer.reply/2` answers the caller later. +- What the process owns is coordination: a cache hit replies at once, and concurrent misses for the same currency share one fetch instead of each calling the API. +- `async_nolink` keeps a crashing task from taking the server down; the crash arrives as `:DOWN` and every waiting caller gets an error. +- Returning `{:noreply, state}` from `handle_call/3` frees the server; `GenServer.reply/2` answers the callers later. - `Process.demonitor(ref, [:flush])` drops the `:DOWN` message that would otherwise follow a successful result. -- The state is a `RatesServerState` wrapper because pending `from`s and task refs are process-only data; see [naming.md](naming.md#process-state). +- Pending callers and task refs are process-only data, so they live in the `RatesServerState` wrapper and not in `RateCache`; see [naming.md](naming.md#process-state). ## Trade-offs - The caller still waits for the I/O, but other callers no longer wait for it. +- Cache hits still go through the mailbox. When they dominate, serve them from ETS as well; see [queuing-reads-from-ets.md](queuing-reads-from-ets.md). - The caller's `call` timeout still applies. A reply after the timeout is discarded. - Keep the clauses above the `handle_info/2` catch-all; see [handle-info.md](handle-info.md). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-partition-by-key.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-partition-by-key.md index 9967c61..2075afa 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-partition-by-key.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-partition-by-key.md @@ -6,52 +6,56 @@ When requests for unrelated keys wait on each other in one server, run several c ```elixir children = [ - {MyApp.Counter, name: MyApp.Counter} + {MyApp.RateLimiter, name: MyApp.RateLimiter, capacity: 100} ] -MyApp.Counter.increment(MyApp.Counter, %IncrementRequest{key: key}) +MyApp.RateLimiter.take(MyApp.RateLimiter, %TakeRequest{key: key}) ``` -Every increment for every key goes through one mailbox. +Every take for every key goes through one mailbox. ## Prefer ```elixir children = [ - {PartitionSupervisor, child_spec: MyApp.Counter, name: MyApp.CounterPartitions} + {PartitionSupervisor, child_spec: {MyApp.RateLimiter, capacity: 100}, name: MyApp.RateLimiterPartitions} ] ``` ```elixir -defmodule MyApp.Counter do +defmodule MyApp.RateLimiter do use GenServer - alias MyApp.Counter.{Counts, IncrementRequest} + alias MyApp.RateLimiter.{Bucket, TakeRequest, TakeResponse} def start_link(opts) do GenServer.start_link(__MODULE__, opts) end - def increment(partitions, %IncrementRequest{key: key} = request) do + def take(partitions, %TakeRequest{key: key} = request) do GenServer.call({:via, PartitionSupervisor, {partitions, key}}, request) end @impl GenServer - def init(_opts), do: {:ok, Counts.new()} + def init(opts), do: {:ok, Bucket.new(opts)} @impl GenServer - def handle_call(%IncrementRequest{key: key}, _from, %Counts{} = counts) do - {:reply, :ok, Counts.increment(counts, key)} + def handle_call(%TakeRequest{key: key}, _from, %Bucket{} = bucket) do + case Bucket.take(bucket, key) do + {:ok, bucket} -> {:reply, {:ok, %TakeResponse{remaining: Bucket.remaining(bucket, key)}}, bucket} + {:error, error} -> {:reply, {:error, error}, bucket} + end end end ``` - `PartitionSupervisor` starts one child per scheduler by default (`partitions: System.schedulers_online()`). - Partitions are not named. Callers address them through the `PartitionSupervisor` with the routing key, so `start_link/1` takes no `:name`. -- The same key always routes to the same partition. +- The same key always routes to the same partition, so each key keeps its own bucket. ## Trade-offs +- Partition only when per-key logic needs a process. A plain counter needs none: `:counters` or `:ets.update_counter/3` updates it from the caller without a mailbox. - Anything that spans keys (totals, "list all") must ask every partition; `PartitionSupervisor.which_children/1` lists them. - A hot key still serializes on its partition. If one key dominates, partitioning does not help; see [queuing-process-per-entity.md](queuing-process-per-entity.md) or [queuing-reads-from-ets.md](queuing-reads-from-ets.md). - A crashed partition loses only its own keys' state. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-pool-resources.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-pool-resources.md index ddcb8fa..96daf39 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-pool-resources.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-pool-resources.md @@ -12,8 +12,8 @@ def init(opts) do end @impl GenServer -def handle_call(%ChargeRequest{} = request, _from, conn) do - {:reply, PaymentsClient.charge(conn, request), conn} +def handle_call(%ChargeRequest{charge: charge}, _from, conn) do + {:reply, PaymentsClient.charge(conn, charge), conn} end ``` @@ -36,7 +36,7 @@ When no library pool exists, build one with `NimblePool`, which checks a resourc ```elixir NimblePool.checkout!(MyApp.PaymentsPool, :checkout, fn _from, conn -> - {PaymentsClient.charge(conn, request), conn} + {PaymentsClient.charge(conn, charge), conn} end) ``` diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md index ae5c270..0efc153 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md @@ -5,13 +5,16 @@ When reads vastly outnumber writes, the server should own the data but not serve ## Avoid ```elixir -def enabled?(server, %EnabledRequest{} = request) do +def fetch_flag(server, %FetchFlagRequest{} = request) do GenServer.call(server, request) end @impl GenServer -def handle_call(%EnabledRequest{flag: flag}, _from, %Flags{} = flags) do - {:reply, Flags.fetch(flags, flag), flags} +def handle_call(%FetchFlagRequest{name: name}, _from, %Flags{} = flags) do + case Flags.fetch(flags, name) do + {:ok, flag} -> {:reply, {:ok, %FetchFlagResponse{flag: flag}}, flags} + {:error, error} -> {:reply, {:error, error}, flags} + end end ``` @@ -23,17 +26,17 @@ Every feature-flag check on every request queues behind every other check. defmodule MyApp.FeatureFlags do use GenServer - alias MyApp.FeatureFlags.{EnableRequest, FlagNotFoundError} + alias MyApp.FeatureFlags.{EnableRequest, FeatureFlagsServerState, Flag, FlagNotFoundError} def start_link(opts) do {name, opts} = Keyword.pop!(opts, :name) GenServer.start_link(__MODULE__, opts, name: name) end - def enabled?(table, flag) do - case :ets.lookup(table, flag) do - [{^flag, enabled?}] -> {:ok, enabled?} - [] -> {:error, %FlagNotFoundError{flag: flag}} + def fetch_flag(table, name) do + case :ets.lookup(table, name) do + [{^name, %Flag{} = flag}] -> {:ok, flag} + [] -> {:error, %FlagNotFoundError{name: name}} end end @@ -44,19 +47,21 @@ defmodule MyApp.FeatureFlags do @impl GenServer def init(opts) do table = :ets.new(Keyword.fetch!(opts, :table), [:named_table, :protected, read_concurrency: true]) - {:ok, table} + {:ok, %FeatureFlagsServerState{table: table}} end @impl GenServer - def handle_call(%EnableRequest{flag: flag}, _from, table) do - :ets.insert(table, {flag, true}) - {:reply, :ok, table} + def handle_call(%EnableRequest{name: name}, _from, %FeatureFlagsServerState{} = state) do + :ets.insert(state.table, {name, %Flag{name: name, enabled?: true}}) + {:reply, :ok, state} end end ``` - `:protected` lets any process read and only the owner write, so writes stay serialized and consistent. - `read_concurrency: true` optimizes for concurrent readers. +- Reads never reach the GenServer, so `fetch_flag/2` takes domain values and returns the `Flag` struct; Request and Response structs exist only for messages to the server. +- The table is process-only data, so the state is a `FeatureFlagsServerState` wrapper; see [naming.md](naming.md#process-state). - The caller passes the table name, so several instances can coexist. ## Trade-offs diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md index 87d1eaf..000466b 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/request-response.md @@ -5,66 +5,68 @@ The client API is a contract with every caller. Shape it so it can grow without ## Avoid ```elixir -def list_sessions(server) do - GenServer.call(server, :list_sessions) +def recent_messages(room_id) do + GenServer.call(via(room_id), :recent_messages) end @impl GenServer -def handle_call(:list_sessions, _from, %Sessions{} = sessions) do - {:reply, {:ok, Sessions.all(sessions)}, sessions} +def handle_call(:recent_messages, _from, %Room{} = room) do + {:reply, {:ok, Room.recent_messages(room)}, room} end ``` ```elixir -{:ok, sessions} = SessionStore.list_sessions(server) +{:ok, messages} = RoomServer.recent_messages(room_id) ``` -The day the operation needs pagination, there is nowhere to put the cursor or the page size. Every option breaks callers: +The day the room needs to page back through its history, there is nowhere to put the page size or the marker for "more". Every option breaks callers: -- `{:ok, sessions, cursor}` breaks every match and violates the [reply shapes](callback-patterns.md#reply-shapes). -- `{:ok, %{sessions: sessions, cursor: cursor}}` breaks every caller that matched a list. -- `list_sessions(server, limit)` changes the arity, and every call site. +- `{:ok, messages, more?}` breaks every match and violates the [reply shapes](callback-patterns.md#reply-shapes). +- `{:ok, %{messages: messages, more?: more?}}` breaks every caller that matched a list. +- `recent_messages(room_id, limit)` changes the arity, and every call site. ## Prefer ```elixir -defmodule MyApp.SessionStore.ListSessionsRequest do - defstruct limit: 100, cursor: nil +defmodule MyApp.Chat.ListMessagesRequest do + defstruct limit: 50, before: nil end -defmodule MyApp.SessionStore.ListSessionsResponse do - defstruct sessions: [], next_cursor: nil +defmodule MyApp.Chat.ListMessagesResponse do + defstruct messages: [], more?: false end ``` ```elixir -def list_sessions(server, %ListSessionsRequest{} = request) do - GenServer.call(server, request) +def list_messages(room_id, %ListMessagesRequest{} = request) do + GenServer.call(via(room_id), request) end @impl GenServer -def handle_call(%ListSessionsRequest{} = request, _from, %Sessions{} = sessions) do - {page, next_cursor} = Sessions.page(sessions, request.cursor, request.limit) - {:reply, {:ok, %ListSessionsResponse{sessions: page, next_cursor: next_cursor}}, sessions} +def handle_call(%ListMessagesRequest{} = request, _from, %Room{} = room) do + %History{} = history = Room.history(room, request.before, request.limit) + {:reply, {:ok, %ListMessagesResponse{messages: history.messages, more?: history.more?}}, room} end ``` ```elixir -{:ok, %ListSessionsResponse{sessions: sessions}} = - SessionStore.list_sessions(server, %ListSessionsRequest{}) +{:ok, %ListMessagesResponse{messages: messages}} = + RoomServer.list_messages(room_id, %ListMessagesRequest{}) ``` New request options and new response fields are new struct fields with defaults. Existing callers keep compiling and keep matching. +The core follows the same idea one layer down: `Room.history/3` returns a `Room.History` struct, never a loose `{messages, more?}` tuple. The room is a process per entity that keeps a bounded history; see [queuing-process-per-entity.md](queuing-process-per-entity.md). + ## Growing Without Breaking A change to the contract should be additive: a new field with a default. A shape that can only change by being replaced forces every caller to change at once. That one idea explains every choice below. - **A Request, even with no fields.** An operation that takes nothing today takes a filter or a page size tomorrow. A field is additive; a new argument changes the arity. - **A Response, not a bare value.** A bare list, map, integer, or boolean has no room for a second piece of data, so the first piece of metadata (cursor, total, truncation) replaces the shape. -- **Collections in a named field.** Data about a collection belongs next to it, and a field named `sessions` leaves room for `next_cursor`. +- **Collections in a named field.** Data about a collection belongs next to it, and a field named `messages` leaves room for `more?`. - **`:ok` only when there is never anything to say.** Moving from `:ok` to `{:ok, response}` is a replacement. When an operation might return data later, start with an empty Response struct. -- **Callers match what they read.** A struct pattern such as `%ListSessionsResponse{sessions: sessions}` ignores fields added later. Comparing whole structs with `==`, in code or in test assertions, breaks as soon as a new field carries a value. +- **Callers match what they read.** A struct pattern such as `%ListMessagesResponse{messages: messages}` ignores fields added later. Comparing whole structs with `==`, in code or in test assertions, breaks as soon as a new field carries a value. - **Evolve by addition.** Renaming or removing a field is a replacement. Add the new field and keep the old one until callers move. - **Enforced keys are decided once.** Adding `@enforce_keys` later breaks every caller that builds the struct without the key. - **Errors grow the same way.** `defexception` structs carry new detail as new fields; see [naming.md](naming.md#errors). @@ -73,7 +75,7 @@ A change to the contract should be additive: a new field with a default. A shape Request and Response structs exist only at the GenServer boundary. `handle_call/3` unpacks the request and calls the pure core with domain values; the core returns domain values that the server wraps in the response. See [naming.md](naming.md#messages). -Items inside a response are domain structs (`%Session{}`), not tuples, for the same reason the reply is a struct. +Items inside a response are domain structs (`%Message{}`), not tuples, for the same reason the reply is a struct. ## Trade-offs From 89af4d0291a82d61f03afd543b8ac8f2f0355500 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 14:08:26 -0400 Subject: [PATCH 17/29] docs(trogonstack-elixir): add a complete GenServer and a pooled-driver example to learn from Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 4 + .../references/example-chat-room.md | 326 ++++++++++++++++++ .../references/queuing-process-per-entity.md | 1 + .../references/queuing-remove-the-process.md | 24 ++ 4 files changed, 355 insertions(+) create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/example-chat-room.md diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index f42b25f..177b30f 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -58,6 +58,10 @@ A restart starts from a clean state, which fixes corrupt state and transient fau Read [references/supervision-and-testing.md](references/supervision-and-testing.md) when wiring the child spec, handling cleanup, or writing tests. +## Worked Example + +Read [references/example-chat-room.md](references/example-chat-room.md) to see every principle applied in one complete, tested GenServer. + ## Reviewing an Existing GenServer Read [references/review-checklist.md](references/review-checklist.md) when reviewing or auditing GenServer code. Each item is a symptom; a failing item is a question about the principle behind it, answered in the linked reference. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/example-chat-room.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/example-chat-room.md new file mode 100644 index 0000000..5b250ff --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/example-chat-room.md @@ -0,0 +1,326 @@ +# Example: Chat Room + +One complete GenServer with every principle applied. Read it to see how the pieces fit; read the linked references for why each piece looks the way it does. + +## Why a Process + +Each room owns the live conversation: the order of messages and who is in the room must change consistently while many members post at once. It also owns the subscriber processes it pushes messages to, and time, because an empty room stops itself. + +The room is ephemeral, like a live support chat: when it stops, its history goes with it. If history must survive, the database is the source of truth and reads go to the database, not through this process; see [anti-pattern-database-gatekeeper.md](anti-pattern-database-gatekeeper.md). + +Rooms are independent, so each one is its own process and a busy room never queues another; see [queuing-process-per-entity.md](queuing-process-per-entity.md). + +## Layout + +```text +MyApp.Chat public API, the only module callers use +MyApp.Chat.RoomServer one GenServer per room +MyApp.Chat.RoomServerState the room plus process-only data +MyApp.Chat.Room pure core: membership and bounded history +MyApp.Chat.Room.History core result for a page of messages +MyApp.Chat.Room.NotAMemberError core error +MyApp.Chat.Member domain struct +MyApp.Chat.Message domain struct +MyApp.Chat.*Request, *Response contract at the GenServer boundary +MyApp.Chat.MessagePosted event pushed to subscribers +``` + +```elixir +children = [ + {Registry, keys: :unique, name: MyApp.Chat.RoomRegistry}, + {DynamicSupervisor, name: MyApp.Chat.RoomSupervisor, strategy: :one_for_one} +] +``` + +## Pure Core + +```elixir +defmodule MyApp.Chat.Member do + defstruct [:id, :name] +end + +defmodule MyApp.Chat.Message do + defstruct [:id, :member_id, :body, :posted_at] +end + +defmodule MyApp.Chat.Room.History do + defstruct messages: [], more?: false +end + +defmodule MyApp.Chat.Room.NotAMemberError do + defexception [:member_id] + + @impl Exception + def message(%__MODULE__{member_id: member_id}), do: "#{inspect(member_id)} is not a member of the room" +end + +defmodule MyApp.Chat.Room do + alias MyApp.Chat.{Member, Message} + alias MyApp.Chat.Room.{History, NotAMemberError} + + defstruct [:id, :max_messages, members: %{}, messages: []] + + def new(id, max_messages), do: %__MODULE__{id: id, max_messages: max_messages} + + def join(%__MODULE__{} = room, %Member{} = member) do + %__MODULE__{room | members: Map.put(room.members, member.id, member)} + end + + def leave(%__MODULE__{} = room, member_id) do + %__MODULE__{room | members: Map.delete(room.members, member_id)} + end + + def post(%__MODULE__{} = room, %Message{} = message) do + if Map.has_key?(room.members, message.member_id) do + messages = Enum.take([message | room.messages], room.max_messages) + {:ok, %__MODULE__{room | messages: messages}} + else + {:error, %NotAMemberError{member_id: message.member_id}} + end + end + + def history(%__MODULE__{} = room, before, limit) do + older = if before, do: Enum.drop_while(room.messages, &(&1.id >= before)), else: room.messages + {page, rest} = Enum.split(older, limit) + %History{messages: page, more?: rest != []} + end +end +``` + +- No process, no clock, no ids: the server generates the message id and timestamp and passes a complete `Message` in. See [naming.md](naming.md#time-is-an-input). +- `join/2` and `leave/2` cannot fail, so they return the room. `post/2` can, so it returns `{:ok, room}` or `{:error, error}`. `history/3` returns a `History` struct, not a loose tuple. See [callback-patterns.md](callback-patterns.md#reply-shapes). +- History is bounded by `max_messages`, so the state and every reply stay small. See [queuing-large-replies.md](queuing-large-replies.md). + +## Contract + +```elixir +defmodule MyApp.Chat.JoinRequest do + @enforce_keys [:member] + defstruct [:member] +end + +defmodule MyApp.Chat.JoinResponse do + defstruct recent_messages: [], room_ref: nil +end + +defmodule MyApp.Chat.PostMessageRequest do + @enforce_keys [:member_id, :body] + defstruct [:member_id, :body] +end + +defmodule MyApp.Chat.PostMessageResponse do + defstruct [:message] +end + +defmodule MyApp.Chat.ListMessagesRequest do + defstruct limit: 50, before: nil +end + +defmodule MyApp.Chat.ListMessagesResponse do + defstruct messages: [], more?: false +end + +defmodule MyApp.Chat.MessagePosted do + defstruct [:room_id, :message] +end + +defmodule MyApp.Chat.RoomNotFoundError do + defexception [:room_id] + + @impl Exception + def message(%__MODULE__{room_id: room_id}), do: "room #{inspect(room_id)} is not running" +end +``` + +- Every operation takes a Request and answers with a Response, so each can grow by adding fields. See [request-response.md](request-response.md). +- `JoinResponse` carries the recent messages. Joining and reading the backlog in one call means no message can slip in between them. +- `@enforce_keys` is decided now, while there are no callers to break. + +## Server + +```elixir +defmodule MyApp.Chat.Subscriber do + defstruct [:member_id, :pid] +end + +defmodule MyApp.Chat.RoomServerState do + defstruct [:room, subscribers: %{}] +end + +defmodule MyApp.Chat.RoomServer do + use GenServer, restart: :temporary + + require Logger + + alias MyApp.Chat.{ + JoinRequest, + JoinResponse, + ListMessagesRequest, + ListMessagesResponse, + Message, + MessagePosted, + PostMessageRequest, + PostMessageResponse, + Room, + RoomNotFoundError, + RoomServerState, + Subscriber + } + + alias MyApp.Chat.Room.History + + @max_messages 500 + @backlog 50 + @idle_timeout :timer.minutes(30) + + def start_link(opts) do + room_id = Keyword.fetch!(opts, :room_id) + GenServer.start_link(__MODULE__, room_id, name: via(room_id)) + end + + def whereis(room_id) do + case Registry.lookup(MyApp.Chat.RoomRegistry, room_id) do + [{pid, _value}] -> {:ok, pid} + [] -> {:error, %RoomNotFoundError{room_id: room_id}} + end + end + + def ensure_started(room_id) do + with {:error, %RoomNotFoundError{}} <- whereis(room_id) do + case DynamicSupervisor.start_child(MyApp.Chat.RoomSupervisor, {__MODULE__, room_id: room_id}) do + {:ok, pid} -> {:ok, pid} + {:error, {:already_started, pid}} -> {:ok, pid} + {:error, error} -> {:error, error} + end + end + end + + defp via(room_id), do: {:via, Registry, {MyApp.Chat.RoomRegistry, room_id}} + + @impl GenServer + def init(room_id) do + {:ok, %RoomServerState{room: Room.new(room_id, @max_messages)}, @idle_timeout} + end + + @impl GenServer + def handle_call(%JoinRequest{member: member}, {pid, _tag}, %RoomServerState{} = state) do + ref = Process.monitor(pid) + subscribers = Map.put(state.subscribers, ref, %Subscriber{member_id: member.id, pid: pid}) + room = Room.join(state.room, member) + %History{} = history = Room.history(room, nil, @backlog) + reply(%RoomServerState{state | room: room, subscribers: subscribers}, {:ok, %JoinResponse{recent_messages: history.messages}}) + end + + def handle_call(%PostMessageRequest{} = request, _from, %RoomServerState{} = state) do + message = %Message{ + id: System.unique_integer([:positive, :monotonic]), + member_id: request.member_id, + body: request.body, + posted_at: DateTime.utc_now() + } + + case Room.post(state.room, message) do + {:ok, room} -> + broadcast(state, %MessagePosted{room_id: room.id, message: message}) + reply(%RoomServerState{state | room: room}, {:ok, %PostMessageResponse{message: message}}) + + {:error, error} -> + reply(state, {:error, error}) + end + end + + def handle_call(%ListMessagesRequest{} = request, _from, %RoomServerState{} = state) do + %History{} = history = Room.history(state.room, request.before, request.limit) + reply(state, {:ok, %ListMessagesResponse{messages: history.messages, more?: history.more?}}) + end + + @impl GenServer + def handle_info({:DOWN, ref, :process, _pid, _reason}, %RoomServerState{} = state) + when is_map_key(state.subscribers, ref) do + {%Subscriber{} = subscriber, subscribers} = Map.pop!(state.subscribers, ref) + still_present? = Enum.any?(Map.values(subscribers), &(&1.member_id == subscriber.member_id)) + room = if still_present?, do: state.room, else: Room.leave(state.room, subscriber.member_id) + noreply(%RoomServerState{state | room: room, subscribers: subscribers}) + end + + def handle_info(:timeout, %RoomServerState{subscribers: subscribers} = state) + when map_size(subscribers) == 0 do + {:stop, :normal, state} + end + + def handle_info(msg, %RoomServerState{} = state) do + Logger.warning("unexpected message: #{inspect(msg)}") + noreply(state) + end + + defp broadcast(%RoomServerState{} = state, %MessagePosted{} = event) do + Enum.each(Map.values(state.subscribers), &send(&1.pid, event)) + end + + defp reply(%RoomServerState{} = state, reply), do: {:reply, reply, state, idle_timeout(state)} + + defp noreply(%RoomServerState{} = state), do: {:noreply, state, idle_timeout(state)} + + defp idle_timeout(%RoomServerState{subscribers: subscribers}) when map_size(subscribers) == 0, + do: @idle_timeout + + defp idle_timeout(%RoomServerState{}), do: :infinity +end +``` + +- Callbacks translate messages into core calls and own only what the core must not know: subscriber pids, monitors, the clock, ids, and the idle timeout. That process-only data is why `RoomServerState` exists. See [naming.md](naming.md#process-state). +- The joining process is the subscriber, taken from `from`. The room monitors it and removes the member when its last subscriber goes down. See [handle-info.md](handle-info.md#monitors). +- `broadcast/2` uses `send/2`, which never blocks the room on a slow subscriber. +- An empty room returns `@idle_timeout` from every callback and stops with `:normal` when it fires. A room with subscribers returns `:infinity`. +- `restart: :temporary`: a restart would bring back an empty room that no subscriber is listening to, so a crashed room stays down and the next join starts a fresh one. +- `handle_info/2` ends with a catch-all. See [handle-info.md](handle-info.md#catch-all). + +## Public API + +```elixir +defmodule MyApp.Chat do + alias MyApp.Chat.{ + JoinRequest, + JoinResponse, + ListMessagesRequest, + PostMessageRequest, + RoomServer + } + + def join(room_id, %JoinRequest{} = request) do + with {:ok, pid} <- RoomServer.ensure_started(room_id) do + ref = Process.monitor(pid) + + case GenServer.call(pid, request) do + {:ok, %JoinResponse{} = response} -> + {:ok, %JoinResponse{response | room_ref: ref}} + + {:error, error} -> + Process.demonitor(ref, [:flush]) + {:error, error} + end + end + end + + def post_message(room_id, %PostMessageRequest{} = request), do: call(room_id, request) + + def list_messages(room_id, %ListMessagesRequest{} = request), do: call(room_id, request) + + defp call(room_id, request) do + with {:ok, pid} <- RoomServer.whereis(room_id) do + GenServer.call(pid, request) + end + end +end +``` + +- Callers use `MyApp.Chat` and never reference `RoomServer` or call `GenServer.call/2` themselves. +- Only `join/2` starts a room. Posting to or reading from a room that is not running returns `{:error, %RoomNotFoundError{}}` instead of creating an empty one. +- `whereis/1` checks the `Registry` first, so the `DynamicSupervisor` is called only when a room actually starts, not on every request. +- `join/2` monitors the room in the caller's process and returns the ref in `JoinResponse`. A subscriber that receives `:DOWN` for it knows the room is gone and can join again. + +## Trade-offs + +- A caller can look up a room just before it stops idle and get an exit from `GenServer.call/2`. The room stops only when it has no subscribers, so active members never hit this. +- Subscribers receive `MessagePosted` in their mailbox. A subscriber that cannot keep up grows its own queue, not the room's; for fan-out across nodes, use `Phoenix.PubSub` instead of pids. +- One room is still one queue. A room with thousands of active posters needs its traffic measured; see [mailbox-queuing.md](mailbox-queuing.md). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md index e0fe31d..acad030 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md @@ -66,6 +66,7 @@ end - `Registry` maps the entity id to its pid, so callers address rooms by id. - `:transient` restarts a room that crashes, but not one that stops normally. - `ensure_started/1` treats `:already_started` as success, which covers two callers racing to start the same room. +- For the full room with its core, contract, monitors, and idle stop, see [example-chat-room.md](example-chat-room.md). ## Trade-offs diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md index 0c17dda..184affa 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-remove-the-process.md @@ -46,6 +46,30 @@ end The caller's process runs the query. Concurrency is bounded by the Repo pool, which is the resource that actually needs bounding. +The same holds without Ecto. A Postgrex connection started under the supervisor is already a pool, and any process can query it: + +```elixir +children = [ + {Postgrex, name: MyApp.DB, hostname: "localhost", database: "my_app", pool_size: 10} +] +``` + +```elixir +defmodule MyApp.Accounts do + alias MyApp.Accounts.{User, UserNotFoundError} + + def get_user(id) do + case Postgrex.query(MyApp.DB, "SELECT id, email FROM users WHERE id = $1", [id]) do + {:ok, %Postgrex.Result{rows: [[id, email]]}} -> {:ok, %User{id: id, email: email}} + {:ok, %Postgrex.Result{rows: []}} -> {:error, %UserNotFoundError{id: id}} + {:error, error} -> {:error, error} + end + end +end +``` + +`Postgrex.query/4` checks a connection out of the pool for the caller and returns it after the query, so concurrent callers run concurrent queries up to `pool_size`. HTTP clients work the same way: Finch, and Req on top of it, pool connections and run requests in the caller's process; see [queuing-pool-resources.md](queuing-pool-resources.md). + ## Trade-offs None, as long as the process owned nothing. If it was built to make writes fault tolerant or race free, see [anti-pattern-database-gatekeeper.md](anti-pattern-database-gatekeeper.md) for what provides those guarantees instead. From c746b0bd2e99d1f35a1abe92a6356cc77eb2d1f3 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 14:21:13 -0400 Subject: [PATCH 18/29] fix(trogonstack-elixir): keep shed-load and chunking examples correct under timeouts and large backlogs Signed-off-by: Yordis Prieto --- .../references/callback-patterns.md | 2 +- .../references/queuing-chunk-long-work.md | 23 +++++++++--------- .../references/queuing-shed-load.md | 24 +++++++++++++++---- .../references/review-checklist.md | 2 +- 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index c0bace0..b1a2f4b 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -84,7 +84,7 @@ This rule covers the replies you design. OTP callback return values such as `{:r ## Redacting State in Crash Logs -Crash reports and `:sys.get_status/1` print the full state. When it holds secrets (tokens, credentials, personal data), implement `format_status/1` (OTP 25+) to redact them: +Crash reports and `:sys.get_status/1` print the full state. When it holds secrets (tokens, credentials, personal data), implement `format_status/1` (Elixir 1.17+; earlier versions use the deprecated `format_status/2`) to redact them: ```elixir @impl GenServer diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md index 7123bf8..3dd9db7 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md @@ -23,27 +23,28 @@ Importing 100,000 rows blocks every lookup until the import ends. @impl GenServer def handle_call(%ImportRequest{rows: rows}, _from, %CatalogServerState{} = state) do - if state.import_queue == [], do: send(self(), %ImportNextBatch{}) - import_queue = state.import_queue ++ rows - response = %ImportResponse{pending_rows: length(import_queue)} + if :queue.is_empty(state.import_queue), do: send(self(), %ImportNextBatch{}) + import_queue = Enum.reduce(rows, state.import_queue, &:queue.in/2) + response = %ImportResponse{pending_rows: :queue.len(import_queue)} {:reply, {:ok, response}, %CatalogServerState{state | import_queue: import_queue}} end @impl GenServer -def handle_info(%ImportNextBatch{}, %CatalogServerState{import_queue: []} = state) do - {:noreply, state} -end - def handle_info(%ImportNextBatch{}, %CatalogServerState{} = state) do - {batch, rest} = Enum.split(state.import_queue, @batch_size) - catalog = Enum.reduce(batch, state.catalog, &Catalog.import_row(&2, &1)) - send(self(), %ImportNextBatch{}) - {:noreply, %CatalogServerState{state | catalog: catalog, import_queue: rest}} + if :queue.is_empty(state.import_queue) do + {:noreply, state} + else + {batch, rest} = :queue.split(min(@batch_size, :queue.len(state.import_queue)), state.import_queue) + catalog = Enum.reduce(:queue.to_list(batch), state.catalog, &Catalog.import_row(&2, &1)) + send(self(), %ImportNextBatch{}) + {:noreply, %CatalogServerState{state | catalog: catalog, import_queue: rest}} + end end ``` - `send(self(), msg)` puts the message at the back of the mailbox, behind client messages already waiting. That is what lets them interleave. - `{:continue, term}` would run before any other message, which defeats the purpose here; see [handle-info.md](handle-info.md#deferring-work-to-yourself). +- `import_queue` starts as `:queue.new()`. Appending with `++` copies the whole backlog on every import; `:queue` appends and splits without that cost. - Only a request that finds the queue empty schedules a batch. A second import joins the queue; scheduling again would start a second chain and double the batch rate. - The reply means "accepted", not "imported", and reports the backlog. Progress is new fields on `ImportResponse` or a separate request. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md index e866797..8333c60 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md @@ -43,28 +43,44 @@ The mailbox check is best-effort: concurrent callers can all see a length below ## Hard Bound -Reserve a slot atomically before sending, and release it when the call returns: +Reserve a slot atomically before sending, release it when the call returns, and give the request a deadline so the server skips work nobody is waiting for: ```elixir @max_in_flight 1_000 +@timeout 5_000 def take(server, admission, %TakeRequest{} = request) do if :atomics.add_get(admission, 1, 1) > @max_in_flight do :atomics.sub(admission, 1, 1) {:error, %OverloadedError{}} else + deadline = System.monotonic_time(:millisecond) + @timeout + try do - GenServer.call(server, request) + GenServer.call(server, %TakeRequest{request | deadline: deadline}, @timeout) after :atomics.sub(admission, 1, 1) end end end + +@impl GenServer +def handle_call(%TakeRequest{key: key, deadline: deadline}, _from, %Bucket{} = bucket) do + if System.monotonic_time(:millisecond) > deadline do + {:noreply, bucket} + else + case Bucket.take(bucket, key) do + {:ok, bucket} -> {:reply, {:ok, %TakeResponse{remaining: Bucket.remaining(bucket, key)}}, bucket} + {:error, error} -> {:reply, {:error, error}, bucket} + end + end +end ``` - `admission` is `:atomics.new(1, signed: true)`, created once by whoever starts the server and passed to callers, for example through the same place the server name lives. -- The counter bounds calls in flight (queued plus being handled), which is a hard upper bound on the mailbox for this operation. -- `after` releases the slot even when the call exits on timeout. +- The counter bounds callers waiting, not the mailbox. A caller that times out frees its slot while its request stays queued, so the mailbox can briefly hold more than `@max_in_flight` requests. +- The deadline keeps that overflow cheap: the server drops an expired request without doing the work, and `:noreply` avoids sending a late reply to a caller that already gave up. +- `System.monotonic_time/1` is per node, so the deadline holds only for a local server. ## Trade-offs diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index 0ad62b4..5f6b452 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -30,7 +30,7 @@ Each item is a symptom of a principle, not a rule on its own. When an item fails - [ ] Persistent dependency failures retry with backoff instead of crash-looping the supervisor - [ ] Replies and core results are `:ok`, `{:ok, value}`, or `{:error, error}`, never longer tuples - [ ] Process names never built from untrusted input -- [ ] Secrets in state are redacted with `format_status/1` +- [ ] Secrets in state are redacted with `format_status/1` (Elixir 1.17+; `format_status/2` before that) ## handle_info ([handle-info.md](handle-info.md)) From 0aeea9fa4380573e0712fbe9ad6133aa28715565 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 16:31:00 -0400 Subject: [PATCH 19/29] docs(trogonstack-elixir): tie resource cleanup to process exit so crashes and kills cannot leak it Signed-off-by: Yordis Prieto --- .../skills/elixir-design-genserver/SKILL.md | 2 + .../references/handle-info.md | 2 +- .../references/queuing-reads-from-ets.md | 2 +- .../resource-lifecycle-ownership.md | 134 ++++++++++++++++++ .../references/review-checklist.md | 5 + .../references/supervision-and-testing.md | 2 +- 6 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 plugins/trogonstack-elixir/skills/elixir-design-genserver/references/resource-lifecycle-ownership.md diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index 177b30f..21c3cd5 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -28,6 +28,8 @@ A process earns its place by owning what cannot live in the caller: shared state Read [references/process-justification.md](references/process-justification.md) when the decision is not obvious, or when reviewing code that uses GenServers as a service layer. +Read [references/resource-lifecycle-ownership.md](references/resource-lifecycle-ownership.md) when a resource must be released when some process exits, or when cleanup depends on callers calling `release` or on `terminate/2`. + Read [references/anti-pattern-database-gatekeeper.md](references/anti-pattern-database-gatekeeper.md) when a GenServer wraps database writes, or is justified as fault tolerance or as a way to avoid race conditions. ### 2. Every Process Is a Queue diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md index 2c0bc2a..fc74beb 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/handle-info.md @@ -96,7 +96,7 @@ def handle_info({:DOWN, ref, :process, _pid, _reason}, %FeedServerState{} = stat end ``` -Monitor instead of link: a link makes the server die with the subscriber, a monitor only tells it. +Monitor instead of link: a link makes the server die with the subscriber, a monitor only tells it. When the monitor exists to release a resource, see [resource-lifecycle-ownership.md](resource-lifecycle-ownership.md). ## Task Results diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md index 0efc153..7d7f8a6 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-reads-from-ets.md @@ -66,6 +66,6 @@ end ## Trade-offs -- The table dies with its owner. Readers get `ArgumentError` during the restart window; rebuild the table in `init/1` or `handle_continue/2`, or have a parent process own it. +- The table dies with its owner. Readers get `ArgumentError` during the restart window; rebuild the table in `init/1` or `handle_continue/2`, or have a parent process own it. See [resource-lifecycle-ownership.md](resource-lifecycle-ownership.md). - A write is visible to readers as soon as `:ets.insert/2` returns, and the `call` returns after that, so the writer reads its own write. - For data that almost never changes, `:persistent_term` reads are faster still, but every update triggers a global garbage collection scan. Use it for configuration, not for anything updated at runtime. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/resource-lifecycle-ownership.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/resource-lifecycle-ownership.md new file mode 100644 index 0000000..13e5210 --- /dev/null +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/resource-lifecycle-ownership.md @@ -0,0 +1,134 @@ +# Resource Lifecycle Ownership + +When a resource should live exactly as long as a process, tie it to that process and let the exit release it. Exit is the one cleanup path that always runs: a crash, a `:kill`, or a node shutdown skips `terminate/2` and any explicit `release/1`, but never the exit. + +## What the Runtime Cleans Up + +| Resource | When the owning process exits | +|----------|-------------------------------| +| ETS table | Deleted, or given to its `heir` | +| Port or socket | Closed | +| `Registry` key | Unregistered | +| Monitor | The watcher receives `:DOWN` | +| Link | Linked processes receive an exit signal | + +Files, rows, external leases, and anything else outside the VM are not on this list. The runtime cannot release them; see [External Resources](#external-resources). + +## Two Roles + +- **Owner:** the process is the lifetime. It holds the resource, and the resource ends when the process ends. A connection process owns its socket; a room owns its ETS table. +- **Observer:** the process watches someone else's lifetime. It monitors that process and releases the resource on `:DOWN`. + +Neither role serves calls on the hot path, so neither becomes a queue. Both satisfy "A Process Must Own Something": the process owns a lifecycle. + +## Avoid + +```elixir +@impl GenServer +def handle_call(%AcquireRequest{key: key}, {pid, _tag}, %LockServerState{} = state) do + if Map.has_key?(state.holders, key) do + {:reply, {:error, %LockHeldError{key: key}}, state} + else + {:reply, :ok, %LockServerState{state | holders: Map.put(state.holders, key, pid)}} + end +end + +@impl GenServer +def handle_call(%ReleaseRequest{key: key}, _from, %LockServerState{} = state) do + {:reply, :ok, %LockServerState{state | holders: Map.delete(state.holders, key)}} +end +``` + +The lock is released only when the holder remembers to call `release`. A holder that crashes, times out, or is killed holds the lock forever, and every caller queues behind one process to acquire it. + +## Prefer + +Let the holder own the lock. A unique `Registry` key belongs to the process that registered it and disappears when that process exits: + +```elixir +children = [{Registry, keys: :unique, name: MyApp.Locks}] +``` + +```elixir +defmodule MyApp.Locks do + def acquire(key) do + case Registry.register(__MODULE__, key, nil) do + {:ok, _owner} -> :ok + {:error, {:already_registered, holder}} -> {:error, %LockHeldError{key: key, holder: holder}} + end + end + + def release(key), do: Registry.unregister(__MODULE__, key) +end +``` + +- `release/1` is an optimization for holders that finish early. Correctness never depends on it. +- There is no lock server to queue behind. `Registry` partitions its own bookkeeping. +- `Registry` is local to one node. A lock that must hold across nodes belongs in the database; see [anti-pattern-database-gatekeeper.md](anti-pattern-database-gatekeeper.md). + +## External Resources + +When the resource lives outside the VM, a process still gives it a lifetime, but the release is code that can be skipped. Make the process an observer of the real owner and keep a backstop for the cases it misses: + +```elixir +defmodule MyApp.Uploads.ScratchDirServer do + use GenServer, restart: :temporary + + def start_link(%ScratchDir{} = scratch_dir) do + GenServer.start_link(__MODULE__, scratch_dir) + end + + @impl GenServer + def init(%ScratchDir{} = scratch_dir) do + Process.flag(:trap_exit, true) + owner_ref = Process.monitor(scratch_dir.owner) + + case File.mkdir_p(scratch_dir.path) do + :ok -> {:ok, %ScratchDirServerState{scratch_dir: scratch_dir, owner_ref: owner_ref}} + {:error, reason} -> {:stop, reason} + end + end + + @impl GenServer + def handle_info({:DOWN, ref, :process, _pid, _reason}, %ScratchDirServerState{owner_ref: ref} = state) do + {:stop, :normal, state} + end + + @impl GenServer + def terminate(_reason, %ScratchDirServerState{} = state) do + File.rm_rf(state.scratch_dir.path) + end +end +``` + +```elixir +def open_scratch_dir(owner \\ self()) do + path = Path.join(System.tmp_dir!(), "upload-#{System.unique_integer([:positive])}") + scratch_dir = %ScratchDir{owner: owner, path: path} + + case DynamicSupervisor.start_child(MyApp.ScratchDirs, {ScratchDirServer, scratch_dir}) do + {:ok, _pid} -> {:ok, scratch_dir} + {:error, error} -> {:error, error} + end +end +``` + +- The directory follows the owner, a request or a LiveView, without the owner calling anything. When the owner exits, the server stops and removes it. +- `restart: :temporary`: a restarted server would watch nothing, and its owner may already be gone. +- Trapping exits makes `terminate/2` run on supervisor shutdown, within the child spec `shutdown`. +- A monitor, not a link: the owner's crash should stop the server cleanly, and the server's crash should not take the owner down. +- `terminate/2` is still skipped by `:kill` and by a VM crash. Sweep stale directories when the application boots; the sweep is what makes the cleanup certain, the server is what makes it prompt. + +## Smells in Review + +- Cleanup depends on callers remembering to call `release`, `close`, or `stop` +- `terminate/2` is the only place a resource is released +- State holds pids with no monitor, so entries for dead processes stay forever +- A link where a monitor belongs: the observer dies with the process it watches +- A process recreates, on restart, a resource whose owner is gone + +## Trade-offs + +- An ETS table dies with its owner; give it an `heir` or a longer-lived parent when readers must survive a crash. See [queuing-reads-from-ets.md](queuing-reads-from-ets.md). +- One observer per resource is a process per resource. That is cheap, but it is still a process to supervise; a single observer that monitors many owners fits when the resources are small and uniform. See [handle-info.md](handle-info.md#monitors). +- When `terminate/2` runs and when it does not is covered in [supervision-and-testing.md](supervision-and-testing.md#cleanup). diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index 5f6b452..2c081f9 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -7,6 +7,11 @@ Each item is a symptom of a principle, not a rule on its own. When an item fails - [ ] The process owns something concrete: shared state, a resource, a lifecycle, or a failure boundary - [ ] No stateless operation (DB, HTTP, computation) is routed through it +## Resource Lifecycle ([resource-lifecycle-ownership.md](resource-lifecycle-ownership.md)) + +- [ ] Resources are released by process exit, not only by an explicit call or `terminate/2` +- [ ] Resources outside the VM have a boot-time sweep as a backstop + ## Database Writes ([anti-pattern-database-gatekeeper.md](anti-pattern-database-gatekeeper.md)) - [ ] Write correctness comes from constraints, transactions, and locks, not from routing writes through one process diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md index 39455a0..8c04412 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md @@ -20,7 +20,7 @@ use GenServer, restart: :transient - `terminate/2` runs on supervisor shutdown only if the process traps exits (`Process.flag(:trap_exit, true)`) and finishes within the child spec `shutdown` value. - Trap exits only when cleanup is required, and set an explicit `shutdown`. -- Do not rely on `terminate/2` for anything that must happen; a `:kill` skips it. +- Do not rely on `terminate/2` for anything that must happen; a `:kill` skips it. Tie the resource to a process exit instead; see [resource-lifecycle-ownership.md](resource-lifecycle-ownership.md). ## State Across Restarts From afe6003bac2aaf11cffe6e8f891e0ede7ebf67cd Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 17:01:59 -0400 Subject: [PATCH 20/29] fix(trogonstack-elixir): stop chunked imports from starting a second batch chain after the queue drains Signed-off-by: Yordis Prieto --- .../references/queuing-chunk-long-work.md | 14 +++++--------- .../references/queuing-shed-load.md | 6 +++--- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md index 3dd9db7..1bf9a2a 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-chunk-long-work.md @@ -31,21 +31,17 @@ end @impl GenServer def handle_info(%ImportNextBatch{}, %CatalogServerState{} = state) do - if :queue.is_empty(state.import_queue) do - {:noreply, state} - else - {batch, rest} = :queue.split(min(@batch_size, :queue.len(state.import_queue)), state.import_queue) - catalog = Enum.reduce(:queue.to_list(batch), state.catalog, &Catalog.import_row(&2, &1)) - send(self(), %ImportNextBatch{}) - {:noreply, %CatalogServerState{state | catalog: catalog, import_queue: rest}} - end + {batch, rest} = :queue.split(min(@batch_size, :queue.len(state.import_queue)), state.import_queue) + catalog = Enum.reduce(:queue.to_list(batch), state.catalog, &Catalog.import_row(&2, &1)) + if :queue.len(rest) > 0, do: send(self(), %ImportNextBatch{}) + {:noreply, %CatalogServerState{state | catalog: catalog, import_queue: rest}} end ``` - `send(self(), msg)` puts the message at the back of the mailbox, behind client messages already waiting. That is what lets them interleave. - `{:continue, term}` would run before any other message, which defeats the purpose here; see [handle-info.md](handle-info.md#deferring-work-to-yourself). - `import_queue` starts as `:queue.new()`. Appending with `++` copies the whole backlog on every import; `:queue` appends and splits without that cost. -- Only a request that finds the queue empty schedules a batch. A second import joins the queue; scheduling again would start a second chain and double the batch rate. +- A marker is pending exactly when the queue is not empty: a request that finds the queue empty schedules the first batch, and a batch schedules the next one only if rows remain. A second import joins the queue; a second marker would start a second chain and double the batch rate. - The reply means "accepted", not "imported", and reports the backlog. Progress is new fields on `ImportResponse` or a separate request. ## Trade-offs diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md index 8333c60..3eb06b1 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md @@ -39,9 +39,9 @@ end - `{:error, %OverloadedError{}}` follows the reply shapes in [callback-patterns.md](callback-patterns.md#reply-shapes); callers map it to a retry with backoff or an HTTP 429. - When the server is not running, `overloaded?/1` returns `false` and the `call` exits as it would have anyway. -The mailbox check is best-effort: concurrent callers can all see a length below the limit and enqueue together, so it cannot enforce a hard bound. +The mailbox check is best-effort: concurrent callers can all see a length below the limit and enqueue together, so it cannot enforce a strict bound. -## Hard Bound +## Bound Waiting Callers Reserve a slot atomically before sending, release it when the call returns, and give the request a deadline so the server skips work nobody is waiting for: @@ -84,7 +84,7 @@ end ## Trade-offs -- Use the mailbox check when stopping runaway growth is enough; use admission control when the bound must hold. +- Use the mailbox check when stopping runaway growth is enough; use admission control when the number of waiting callers must stay bounded. - `Process.info/2` works only for local pids. For remote servers, use admission control. - Admission control counts only callers that go through it; casts and messages from elsewhere still reach the mailbox. - Shedding is a last resort. It keeps the node alive, but callers still fail; prefer the patterns that remove the queue. From 4847ba2cf6fe609b434f0c8ec0b7f3b85ce4108e Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Mon, 5 Oct 2026 17:12:16 -0400 Subject: [PATCH 21/29] fix(trogonstack-elixir): hold admission slots until the server consumes the request so timeouts cannot exceed the bound Signed-off-by: Yordis Prieto --- .../references/queuing-shed-load.md | 55 +++++++++++-------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md index 3eb06b1..18e1f86 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-shed-load.md @@ -39,52 +39,63 @@ end - `{:error, %OverloadedError{}}` follows the reply shapes in [callback-patterns.md](callback-patterns.md#reply-shapes); callers map it to a retry with backoff or an HTTP 429. - When the server is not running, `overloaded?/1` returns `false` and the `call` exits as it would have anyway. -The mailbox check is best-effort: concurrent callers can all see a length below the limit and enqueue together, so it cannot enforce a strict bound. +The mailbox check is best-effort: concurrent callers can all see a length below the limit and enqueue together, so it cannot enforce a hard bound. -## Bound Waiting Callers +## Hard Bound -Reserve a slot atomically before sending, release it when the call returns, and give the request a deadline so the server skips work nobody is waiting for: +Count requests from the moment a caller sends one until the server takes it out of the mailbox. The caller reserves a slot, and the server releases it: ```elixir -@max_in_flight 1_000 +@max_queued 1_000 @timeout 5_000 -def take(server, admission, %TakeRequest{} = request) do - if :atomics.add_get(admission, 1, 1) > @max_in_flight do +def take(name, %TakeRequest{} = request) do + case Registry.lookup(MyApp.RateLimiters, name) do + [{pid, admission}] -> admit(pid, admission, request) + [] -> {:error, %RateLimiterNotFoundError{name: name}} + end +end + +defp admit(pid, admission, %TakeRequest{} = request) do + if :atomics.add_get(admission, 1, 1) > @max_queued do :atomics.sub(admission, 1, 1) {:error, %OverloadedError{}} else deadline = System.monotonic_time(:millisecond) + @timeout - - try do - GenServer.call(server, %TakeRequest{request | deadline: deadline}, @timeout) - after - :atomics.sub(admission, 1, 1) - end + GenServer.call(pid, %TakeRequest{request | admission: admission, deadline: deadline}, @timeout) end end @impl GenServer -def handle_call(%TakeRequest{key: key, deadline: deadline}, _from, %Bucket{} = bucket) do - if System.monotonic_time(:millisecond) > deadline do +def init(opts) do + admission = :atomics.new(1, signed: true) + {:ok, _owner} = Registry.register(MyApp.RateLimiters, Keyword.fetch!(opts, :name), admission) + {:ok, Bucket.new(Keyword.fetch!(opts, :capacity))} +end + +@impl GenServer +def handle_call(%TakeRequest{} = request, _from, %Bucket{} = bucket) do + :atomics.sub(request.admission, 1, 1) + + if System.monotonic_time(:millisecond) > request.deadline do {:noreply, bucket} else - case Bucket.take(bucket, key) do - {:ok, bucket} -> {:reply, {:ok, %TakeResponse{remaining: Bucket.remaining(bucket, key)}}, bucket} + case Bucket.take(bucket, request.key) do + {:ok, bucket} -> {:reply, {:ok, %TakeResponse{remaining: Bucket.remaining(bucket, request.key)}}, bucket} {:error, error} -> {:reply, {:error, error}, bucket} end end end ``` -- `admission` is `:atomics.new(1, signed: true)`, created once by whoever starts the server and passed to callers, for example through the same place the server name lives. -- The counter bounds callers waiting, not the mailbox. A caller that times out frees its slot while its request stays queued, so the mailbox can briefly hold more than `@max_in_flight` requests. -- The deadline keeps that overflow cheap: the server drops an expired request without doing the work, and `:noreply` avoids sending a late reply to a caller that already gave up. -- `System.monotonic_time/1` is per node, so the deadline holds only for a local server. +- The slot is released when the request leaves the mailbox, not when the caller stops waiting. A caller that times out leaves its request queued and its slot taken, so the count never drops below what the mailbox holds. +- The server creates the counter in `init/1` and publishes it as its `Registry` value. A restart drops the old entry with the old process and starts from a fresh counter, so slots held by requests lost in the crash do not leak into the new server. See [resource-lifecycle-ownership.md](resource-lifecycle-ownership.md). +- Callers send to the pid they looked up, and the request carries the counter it reserved, so the server always releases the right one. +- The deadline lets the server drop a request whose caller already gave up, without doing the work; `:noreply` avoids a late reply to it. ## Trade-offs -- Use the mailbox check when stopping runaway growth is enough; use admission control when the number of waiting callers must stay bounded. -- `Process.info/2` works only for local pids. For remote servers, use admission control. +- Use the mailbox check when stopping runaway growth is enough; use admission control when the bound must hold. +- `Process.info/2`, `:atomics`, `Registry`, and `System.monotonic_time/1` are local to one node, so both checks fit callers on the server's node. - Admission control counts only callers that go through it; casts and messages from elsewhere still reach the mailbox. - Shedding is a last resort. It keeps the node alive, but callers still fail; prefer the patterns that remove the queue. From e8d5cfeb5068f68c2d37d4fe9a4deef7000f828b Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 9 Oct 2026 18:34:52 -0400 Subject: [PATCH 22/29] docs(trogonstack-elixir): compose the locked debit as an Ecto.Multi so each step is named and its failure is explicit Signed-off-by: Yordis Prieto --- .../anti-pattern-database-gatekeeper.md | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md index eff62c1..07474f9 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md @@ -42,21 +42,31 @@ Run the write in the caller's process and let the database linearize it: ```elixir defmodule MyApp.Accounts do import Ecto.Query + alias Ecto.Multi def debit(account_id, amount) do - Repo.transact(fn -> - query = Account |> where([a], a.id == ^account_id) |> lock("FOR UPDATE") - - case Repo.one(query) do - nil -> {:error, %AccountNotFoundError{account_id: account_id}} - %Account{} = account -> account |> Account.debit_changeset(amount) |> Repo.update() - end - end) + Multi.new() + |> Multi.run(:account, fn repo, _changes -> fetch_for_update(repo, account_id) end) + |> Multi.update(:debited, fn %{account: account} -> Account.debit_changeset(account, amount) end) + |> Repo.transact() + |> case do + {:ok, %{debited: account}} -> {:ok, account} + {:error, _step, error, _changes} -> {:error, error} + end + end + + defp fetch_for_update(repo, account_id) do + query = Account |> where([a], a.id == ^account_id) |> lock("FOR UPDATE") + + case repo.one(query) do + nil -> {:error, %AccountNotFoundError{account_id: account_id}} + %Account{} = account -> {:ok, account} + end end end ``` -The row lock serializes concurrent debits of the same account, from any process on any node, while debits of other accounts run in parallel. `Repo.transact/2` (Ecto 3.13+) commits on `{:ok, value}` and rolls back on `{:error, error}`. +The row lock serializes concurrent debits of the same account, from any process on any node, while debits of other accounts run in parallel. Each `Ecto.Multi` step runs in order inside one transaction; the first step that fails, a missing account or an invalid changeset, rolls the whole transaction back. `Repo.transact/2` (Ecto 3.13+) runs the multi and returns `{:error, step, error, changes}` on failure, which `debit/2` narrows to the [reply shapes](callback-patterns.md#reply-shapes). Pick the database mechanism that matches the guarantee: From 8348cef4fbad2c3f61a3f54a4dafee39c4094487 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 9 Oct 2026 18:35:27 -0400 Subject: [PATCH 23/29] docs(trogonstack-elixir): recommend error structs without forbidding atoms that callers already match on Signed-off-by: Yordis Prieto --- .../elixir-design-genserver/references/callback-patterns.md | 2 +- .../skills/elixir-design-genserver/references/naming.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index b1a2f4b..e737f13 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -51,7 +51,7 @@ Every reply, and every core function that can fail, returns one of: |-------|------| | `:ok` | Success with nothing to return | | `{:ok, value}` | Success with a value, usually an `Response` struct | -| `{:error, error}` | Failure, with an error struct (`defexception`) | +| `{:error, error}` | Failure. Any term works; an error struct (`defexception`) is recommended, see [naming.md](naming.md#errors) | Never reply with a tuple of more than two elements such as `{:ok, value, extra}` or `{:error, reason, details}`. Put the extra data in the struct. Callers can then handle every operation with the same `case` or `with`, and adding a field never breaks a pattern match. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md index 2d124c9..39ea56e 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/naming.md @@ -86,7 +86,7 @@ Name core functions with domain verbs (`take`, `refill`, `expire`), never GenSer ## Errors -Return errors as `defexception` structs named `*Error`, nested under the module that produces them: `Bucket.ExhaustedError`. An exception struct in `{:error, error}` is not raised; it gains `Exception.message/1` for logs and lets a bang variant `raise` the same value. Build the message from structured fields so callers can still match on them. +Any term can be the error in `{:error, error}`, and an atom such as `:not_found` is common. Prefer `defexception` structs named `*Error`, nested under the module that produces them: `Bucket.ExhaustedError`. An exception struct in `{:error, error}` is not raised; it gains `Exception.message/1` for logs and lets a bang variant `raise` the same value. Build the message from structured fields so callers can still match on them. An atom has no room for detail, so the first field it needs replaces the shape, the same problem [request-response.md](request-response.md#growing-without-breaking) describes for replies. ## Time Is an Input From ef2dd5fe07bd8cba2690622bfa28bb992b73c759 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 9 Oct 2026 18:36:12 -0400 Subject: [PATCH 24/29] docs(trogonstack-elixir): match on the transaction result directly instead of piping into case Signed-off-by: Yordis Prieto --- .../references/anti-pattern-database-gatekeeper.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md index 07474f9..f9b7243 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md @@ -45,11 +45,12 @@ defmodule MyApp.Accounts do alias Ecto.Multi def debit(account_id, amount) do - Multi.new() - |> Multi.run(:account, fn repo, _changes -> fetch_for_update(repo, account_id) end) - |> Multi.update(:debited, fn %{account: account} -> Account.debit_changeset(account, amount) end) - |> Repo.transact() - |> case do + multi = + Multi.new() + |> Multi.run(:account, fn repo, _changes -> fetch_for_update(repo, account_id) end) + |> Multi.update(:debited, fn %{account: account} -> Account.debit_changeset(account, amount) end) + + case Repo.transact(multi) do {:ok, %{debited: account}} -> {:ok, account} {:error, _step, error, _changes} -> {:error, error} end From 66d53f35c9ac9a115ad183e9a60b78bd71e1b49c Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 13:25:50 -0400 Subject: [PATCH 25/29] fix(trogonstack-elixir): stop restarting in-memory entities whose restart only brings back an empty shell Signed-off-by: Yordis Prieto --- .../references/queuing-process-per-entity.md | 8 ++++---- .../references/supervision-and-testing.md | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md index acad030..24f61fa 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/queuing-process-per-entity.md @@ -27,7 +27,7 @@ children = [ ```elixir defmodule MyApp.Chat.RoomServer do - use GenServer, restart: :transient + use GenServer, restart: :temporary alias MyApp.Chat.{PostMessageRequest, Room} @@ -64,12 +64,12 @@ end ``` - `Registry` maps the entity id to its pid, so callers address rooms by id. -- `:transient` restarts a room that crashes, but not one that stops normally. -- `ensure_started/1` treats `:already_started` as success, which covers two callers racing to start the same room. +- `:temporary`: the room lives only in memory, so a restart would bring back an empty room. A crashed room stays down and the next `ensure_started/1` starts a fresh one. +- `ensure_started/1` treats `:already_started` as success, which covers two callers racing to start the same room: the `:unique` `Registry` key makes the second registration fail atomically. - For the full room with its core, contract, monitors, and idle stop, see [example-chat-room.md](example-chat-room.md). ## Trade-offs - Idle entities hold memory. Stop them after inactivity, for example by returning a timeout from callbacks and stopping with `{:stop, :normal, state}` on `:timeout`. -- State is lost when the process stops. Load it from durable storage in `handle_continue/2` if it must survive. +- State is lost when the process stops. Load it from durable storage in `handle_continue/2` if it must survive; a restart then recovers real state, so `:transient` fits. - Cross-entity operations become messages between processes; keep them rare or move them to a separate read model. diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md index 8c04412..e635601 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/supervision-and-testing.md @@ -13,8 +13,8 @@ use GenServer, restart: :transient | Strategy | Restarts when | Fits | |----------|---------------|------| | `:permanent` | Always | Long-lived services | -| `:transient` | Abnormal exit only | Work that can finish normally, per-entity processes | -| `:temporary` | Never | One-shot processes whose failure the caller handles | +| `:transient` | Abnormal exit only | Work that can finish normally, per-entity processes that reload durable state | +| `:temporary` | Never | One-shot processes whose failure the caller handles, in-memory per-entity processes | ## Cleanup From 3f3498432fbfda0cd3ecdfc926da031a4a24ed97 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 13:25:50 -0400 Subject: [PATCH 26/29] docs(trogonstack-elixir): back the worked example's tested claim with tests readers can run Signed-off-by: Yordis Prieto --- .../references/example-chat-room.md | 144 ++++++++++++++++++ 1 file changed, 144 insertions(+) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/example-chat-room.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/example-chat-room.md index 5b250ff..0270f21 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/example-chat-room.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/example-chat-room.md @@ -319,6 +319,150 @@ end - `whereis/1` checks the `Registry` first, so the `DynamicSupervisor` is called only when a room actually starts, not on every request. - `join/2` monitors the room in the caller's process and returns the ref in `JoinResponse`. A subscriber that receives `:DOWN` for it knows the room is gone and can join again. +## Tests + +```elixir +defmodule MyApp.Chat.RoomTest do + use ExUnit.Case, async: true + + alias MyApp.Chat.{Member, Message, Room} + alias MyApp.Chat.Room.{History, NotAMemberError} + + defp message(id, member_id), + do: %Message{id: id, member_id: member_id, body: "msg-#{id}", posted_at: DateTime.utc_now()} + + test "post by a non-member returns NotAMemberError" do + room = Room.new("room-1", 500) + + assert {:error, %NotAMemberError{member_id: 1}} = Room.post(room, message(1, 1)) + end + + test "history is bounded by max_messages" do + room = Room.new("room-1", 2) |> Room.join(%Member{id: 1, name: "alice"}) + + room = + Enum.reduce(1..3, room, fn id, room -> + {:ok, room} = Room.post(room, message(id, 1)) + room + end) + + assert %History{messages: messages} = Room.history(room, nil, 10) + assert Enum.map(messages, & &1.id) == [3, 2] + end + + test "history pages with before and reports more?" do + room = Room.new("room-1", 500) |> Room.join(%Member{id: 1, name: "alice"}) + + room = + Enum.reduce(1..5, room, fn id, room -> + {:ok, room} = Room.post(room, message(id, 1)) + room + end) + + assert %History{messages: page_1, more?: true} = Room.history(room, nil, 2) + assert Enum.map(page_1, & &1.id) == [5, 4] + + assert %History{messages: page_2, more?: true} = Room.history(room, 4, 2) + assert Enum.map(page_2, & &1.id) == [3, 2] + + assert %History{messages: page_3, more?: false} = Room.history(room, 2, 2) + assert Enum.map(page_3, & &1.id) == [1] + end +end +``` + +- The core needs no process, so every case builds a `Room` and calls it directly. See [supervision-and-testing.md](supervision-and-testing.md#testing). +- `before` takes the oldest id already seen and returns the next older page; walking the full history proves `more?` turns false only on the last page. + +```elixir +defmodule MyApp.Chat.RoomServerTest do + use ExUnit.Case, async: false + + alias MyApp.Chat + alias MyApp.Chat.{ + JoinRequest, + ListMessagesRequest, + Member, + MessagePosted, + PostMessageRequest, + PostMessageResponse, + RoomServer + } + + alias MyApp.Chat.Room.NotAMemberError + alias MyApp.Chat.RoomNotFoundError + + setup do + start_supervised!({Registry, keys: :unique, name: MyApp.Chat.RoomRegistry}) + start_supervised!({DynamicSupervisor, name: MyApp.Chat.RoomSupervisor, strategy: :one_for_one}) + :ok + end + + defp room_id, do: "room-#{System.unique_integer([:positive])}" + + test "join then post broadcasts MessagePosted to the subscriber" do + room_id = room_id() + member = %Member{id: 1, name: "alice"} + + assert {:ok, _} = Chat.join(room_id, %JoinRequest{member: member}) + + assert {:ok, %PostMessageResponse{message: message}} = + Chat.post_message(room_id, %PostMessageRequest{member_id: 1, body: "hi"}) + + assert_receive %MessagePosted{room_id: ^room_id, message: ^message} + end + + test "post and list on a room that is not running return RoomNotFoundError" do + room_id = room_id() + + assert {:error, %RoomNotFoundError{room_id: ^room_id}} = + Chat.post_message(room_id, %PostMessageRequest{member_id: 1, body: "hi"}) + + assert {:error, %RoomNotFoundError{room_id: ^room_id}} = + Chat.list_messages(room_id, %ListMessagesRequest{}) + end + + test "member is removed once its only subscriber process exits" do + room_id = room_id() + member = %Member{id: 1, name: "alice"} + test_pid = self() + + joiner = + spawn(fn -> + {:ok, _} = Chat.join(room_id, %JoinRequest{member: member}) + send(test_pid, :joined) + + receive do + :stop -> :ok + end + end) + + assert_receive :joined + ref = Process.monitor(joiner) + send(joiner, :stop) + assert_receive {:DOWN, ^ref, :process, ^joiner, :normal} + + assert {:error, %NotAMemberError{member_id: 1}} = + Chat.post_message(room_id, %PostMessageRequest{member_id: 1, body: "hi"}) + end + + test "an empty room stops normally on idle timeout" do + room_id = room_id() + {:ok, pid} = RoomServer.ensure_started(room_id) + + ref = Process.monitor(pid) + send(pid, :timeout) + + assert_receive {:DOWN, ^ref, :process, ^pid, :normal} + end +end +``` + +- The server is tested through `MyApp.Chat`, the same public API callers use, under the same children as [Layout](#layout). See [supervision-and-testing.md](supervision-and-testing.md#testing). +- `Registry` and `DynamicSupervisor` are started with the fixed names the example hardcodes, not a unique name per test, so tests running concurrently would share them; that forces `async: false`. Passing the names in, as [callback-patterns.md](callback-patterns.md) recommends for shared servers, is what keeps `async: true` possible. +- The exiting member is a separate spawned process, since the room monitors whoever called `join/2` and the test process itself must stay alive to assert. `assert_receive` on that process's own `:DOWN` confirms the exit before the next call runs. +- The idle-timeout test sends `:timeout` straight to the room instead of waiting `@idle_timeout` out; the room cannot tell a real timer from this message, so the assertion exercises the same `handle_info/2` clause either way. + ## Trade-offs - A caller can look up a room just before it stops idle and get an exit from `GenServer.call/2`. The room stops only when it has no subscribers, so active members never hit this. From a084072ffe5bda4e5a9225317f241f210206106f Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 13:25:50 -0400 Subject: [PATCH 27/29] fix(trogonstack-elixir): stop claiming a raised exception skips terminate/2 Signed-off-by: Yordis Prieto --- .../references/resource-lifecycle-ownership.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/resource-lifecycle-ownership.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/resource-lifecycle-ownership.md index 13e5210..4567b49 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/resource-lifecycle-ownership.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/resource-lifecycle-ownership.md @@ -1,6 +1,6 @@ # Resource Lifecycle Ownership -When a resource should live exactly as long as a process, tie it to that process and let the exit release it. Exit is the one cleanup path that always runs: a crash, a `:kill`, or a node shutdown skips `terminate/2` and any explicit `release/1`, but never the exit. +When a resource should live exactly as long as a process, tie it to that process and let the exit release it. Exit is the one cleanup path that always runs: a `:kill`, a VM crash, or an exit signal received without trapping exits (including a supervisor shutdown) skips `terminate/2`, and a crashed caller never reaches its `release/1`, but none of them skips the exit. ## What the Runtime Cleans Up From b93127e812d7f5d2bf397a1cc29f6f0f8654c6db Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 13:25:51 -0400 Subject: [PATCH 28/29] fix(trogonstack-elixir): keep review rules from flagging the worked example and infallible core functions Signed-off-by: Yordis Prieto --- .../elixir-design-genserver/references/callback-patterns.md | 2 +- .../elixir-design-genserver/references/review-checklist.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md index e737f13..f9fd110 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/callback-patterns.md @@ -40,7 +40,7 @@ end - Callers never call `GenServer.call/cast` directly. The client API owns the call; the messages are `Request` and `Response` structs. See [naming.md](naming.md). - Callbacks delegate to a pure core (`Bucket.take/2`) that knows nothing about the GenServer or its messages. - Mark every callback with `@impl GenServer`. -- Require `:name` in `start_link/1` and take the server as the first argument of every client function. Callers and tests always say which instance they talk to; a default argument hides that choice. +- Require `:name` in `start_link/1` and take the server as the first argument of every client function. Callers and tests always say which instance they talk to; a default argument hides that choice. A per-entity process derives its name from its key instead, and every client function takes that key first; see [example-chat-room.md](example-chat-room.md). - Hold state in a struct so its shape is documented and enforced. The domain struct is the state until process-only data appears; see [naming.md](naming.md). ## Reply Shapes diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md index 2c081f9..e01face 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/review-checklist.md @@ -33,7 +33,7 @@ Each item is a symptom of a principle, not a rule on its own. When an item fails - [ ] Callbacks delegate to pure, separately tested functions - [ ] `init/1` is fast; slow work runs in `handle_continue/2`, unless siblings need the state at boot - [ ] Persistent dependency failures retry with backoff instead of crash-looping the supervisor -- [ ] Replies and core results are `:ok`, `{:ok, value}`, or `{:error, error}`, never longer tuples +- [ ] Replies and fallible core results are `:ok`, `{:ok, value}`, or `{:error, error}`, never longer tuples; core functions that cannot fail return a value or a struct - [ ] Process names never built from untrusted input - [ ] Secrets in state are redacted with `format_status/1` (Elixir 1.17+; `format_status/2` before that) From 9fcda6c701073656f53e9b8d413ddd5a317fd8ed Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 13:25:51 -0400 Subject: [PATCH 29/29] docs(trogonstack-elixir): correct the upsert label, explain why late call replies are harmless, and order the description by principle Signed-off-by: Yordis Prieto --- .../trogonstack-elixir/skills/elixir-design-genserver/SKILL.md | 2 +- .../references/anti-pattern-database-gatekeeper.md | 2 +- .../elixir-design-genserver/references/mailbox-queuing.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md index 21c3cd5..9e92ec3 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/SKILL.md @@ -1,6 +1,6 @@ --- name: elixir-design-genserver -description: "Design or review Elixir GenServer modules. Decides whether a process is justified at all, detects mailbox queuing and serialization bottlenecks, shapes the client API, picks call vs cast, and places the server under supervision. Use when writing a new GenServer, reviewing an existing one, deciding where an actor belongs, or diagnosing a process that became a bottleneck. Do not use for: (1) designing full supervision trees across an application, (2) distributed Erlang or clustering, (3) Phoenix LiveView or Channel processes, (4) GenStage, Broadway, or Flow pipelines." +description: "Design or review Elixir GenServer modules. Decides whether a process is justified at all, detects mailbox queuing and serialization bottlenecks, picks call vs cast, shapes the client API, and places the server under supervision. Use when writing a new GenServer, reviewing an existing one, deciding where an actor belongs, or diagnosing a process that became a bottleneck. Do not use for: (1) designing full supervision trees across an application, (2) distributed Erlang or clustering, (3) Phoenix LiveView or Channel processes, (4) GenStage, Broadway, or Flow pipelines." allowed-tools: - AskUserQuestion - Read diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md index f9b7243..5b08279 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/anti-pattern-database-gatekeeper.md @@ -73,7 +73,7 @@ Pick the database mechanism that matches the guarantee: | Need | Use | |------|-----| -| No duplicates | Unique index, and `Repo.insert(changeset, on_conflict: :nothing, conflict_target: :email)` for upserts | +| No duplicates | Unique index, and `Repo.insert(changeset, on_conflict: :nothing, conflict_target: :email)` for idempotent inserts, `on_conflict: {:replace, fields}` to update the existing row | | No lost updates on a contended row | `lock("FOR UPDATE")` inside a transaction | | No lost updates on a rarely contended row | `Ecto.Changeset.optimistic_lock/3`, retrying on `Ecto.StaleEntryError` | | Ordering or sequence numbers | A database sequence or a version column | diff --git a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md index 1a8677c..631ad10 100644 --- a/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md +++ b/plugins/trogonstack-elixir/skills/elixir-design-genserver/references/mailbox-queuing.md @@ -19,7 +19,7 @@ Latency under load (Little's law): a message waits roughly `queue_length * avera ### With `call` - Each caller blocks until every message ahead of it is handled. -- The default 5 second timeout exits the caller, but the request stays in the mailbox and the server still does the work, then the reply is discarded. Load does not drop when callers give up. +- The default 5 second timeout exits the caller, but the request stays in the mailbox and the server still does the work, then the reply is dropped. The call was tagged with an alias the caller removed on timeout, so the late reply never reaches its mailbox. Load does not drop when callers give up. - Callers that retry on timeout add more messages to a queue that is already behind, so the backlog feeds itself. - A server that calls another busy server inherits that server's queue as well.