Add configurable custom buffers ({mod,Mod,Opts}) + preflight validation (#15)

[eric-descourtis-thenvoi]

Summary

Two small, additive robustness features for custom buffers and configuration:

1. {mod, Module, Opts} configurable custom buffers — build a custom buffer with
   Module:new(Opts) so it can take construction-time configuration.
2. pobox:preflight/1 + start_link fail-fast — validate a config up front with a
   descriptive {error, Reason} instead of a late, cryptic crash.

Related to the "future enhancements" of #15 (the custom-buffer-config and
validation groundwork). Purely additive; existing behavior is unchanged.

Scope

A sibling to the weighting (#16) and calls (#17) PRs; kept independent for review.
Branched off master; versioned 1.5.0, sequenced after those — the maintainer can
re-order, it only touches the changelog/vsn.

What's added

• {mod, Module, Opts} buffer type → constructs with Module:new(Opts). Accepted in
  every constructor (positional start_link/3,4,5 and the map/proplist forms). The stored
  buffer type is normalized to {mod, Module}, so all downstream dispatch is unchanged.
• pobox_buf behaviour gains an optional new/1 callback; new/0 is moved into
  optional_callbacks so an opts-only buffer needn't define it. Existing {mod, Module}
  buffers (with new/0) are unaffected.
• pobox:preflight/1 — validates a map/proplist config without starting a process,
  returning ok or a descriptive reason: {bad_max, V}, {bad_initial_state, V},
  {bad_owner, V}, {bad_heir, V}, {bad_type, V}, {module_not_loaded, Mod},
  {missing_callback, {Mod, F, A}}.
• start_link fail-fast — all forms run the buffer-module check before spawning and
  return {error, Reason} for an unloadable/incomplete module, instead of a cryptic init
  crash. (Structural errors still raise badarg as before.)
• Sample pobox_configurable_buf demonstrating new/1.

Backward compatibility

Additive. Built-in-type configs and existing {mod, Module} buffers flow through
unchanged; validate_opts still raises badarg for structural errors. The behaviour
change (optional new/0/new/1) is a loosening — existing custom buffers compile with no
warning. All 67 original Common Test cases and 3 properties pass unchanged.

Implementation discipline

6 atomic, signed commits: TDD cycles C1/C2 (configurable buffers), D1 (preflight/1),
D2 (start_link fail-fast), a dialyzer suppression for the intentional bad_type catch-all,
and E1/E2 fixing two adversarial-review findings — preflight was not validating
owner/heir, and the positional start_link forms were not failing fast. The review
confirmed the {mod,Mod,Opts} normalization, start_link backward-compat, the extended
type guard, and code:ensure_loaded usage are clean.

Test plan

• 76 Common Test cases (67 original + 9 new: {mod,Mod,Opts} construction, preflight/1
  valid/bad_max/bad_type/bad_owner/bad_heir/module_not_loaded/missing_callback,
  and map + positional start_link fail-fast).
• 3 PropEr properties unchanged.
• rebar3 dialyzer clean; compile warnings-as-errors clean.

rebar3 ct       # 76 passed
rebar3 proper   # 3/3 properties
rebar3 dialyzer # 0 warnings

Files changed

 src/pobox.erl                          | 137 ++++++++++++++++++++++-----
 test/pobox_preflight_SUITE.erl         |  99 +++++++++++++++++++
 src/samples/pobox_configurable_buf.erl |  29 ++++++
 README.md                              |  34 +++++-
 src/pobox_buf.erl                      |   6 +-
 src/pobox.app.src                      |   2 +-

Follow-ups

• When weighting (Add optional message weighting (proposal 1 of #15) #16) merges, preflight can also enforce drop_one/1 for weighted
  {mod,_} buffers (its start-time validation motivation).

Part of the enhancements in #15.

🤖 Generated with Claude Code

[eric-descourtis-thenvoi]

@eric-descourtis-thenvoi — self-review of PR #18 ({mod,Mod,Opts} configurable buffers + preflight/1), fresh multi-pass sweep.

Verdict: COMMENT — one HIGH (a documented opts-only buffer crashes on its first overflow), two MED, four LOW; all prior E1/E2 fixes verified closed.

Methodology: Fresh review-rigor external-review pass on branch tip 05629f27 in an isolated worktree. 7 parallel lanes (floor 6 + an event-fan-out lane armed by push_ — a FALSE ARM, the token matched the push_drop/2 buffer callback, not a message fan-out): llm-hardening, correctness, test-adequacy, event-fan-out, a cross-model codex lane, and the two general-review completeness critics (Opus 4.8 + codex) run last. Every HIGH/MED was reproduced empirically with escript against the branch tip. rebar3 ct → 76/76 pass, proper → 3/3, dialyzer → 0 warnings (all executed, exit 0). The cross-model codex lane found the HIGH the four Claude lanes missed; the four Claude lanes independently converged on the name gap.

---

HIGH (PR-introduced — newly reachable)

H1 — {mod,Mod,Opts} opts-only buffer (only new/1, no push_drop/2) passes preflight then CRASHES on first overflow via Mod:new/0

src/pobox.erl:536 (reset) · :508-514 (fallback) · :464 (Opts-discard) · :632-637 (validation)

A buffer configured exactly as the PR documents — {mod, Module, Opts} with an opts-only buffer that omits the optional push_drop/2 — passes preflight (returns ok) and start_link (returns {ok, Pid}), then dies the first time it overflows with error:{function not exported,{Mod,new,0}}, losing all buffered messages.

Blast radius: incident — a validation-clean, documented config crashes under normal load (overflow is the whole point of pobox), taking its buffered messages with it.
Scope of cause: this pattern — the Mod:new/0 reset at :536 pre-existed on master but was unreachable (the only custom spelling was {mod,Module}, which required new/0). This PR makes it reachable by both adding new/1-built buffers and moving new/0 into optional_callbacks. A secondary correctness bug rides along: even if new/0 exists, :536 resets to Mod:new() (arity 0), discarding the Opts the buffer was configured with, because buf_new normalizes {mod,Mod,Opts}→{mod,Mod} and drops Opts at :464.
Fix: on the custom-buffer drop-all reset, do not call Mod:new/0. Either (a) delegate to the mandatory Mod:drop/2 to empty the buffer (drop(Size, Data)), so no constructor is needed — smaller, and also fixes the latent Opts-loss-on-reset; or (b) retain Opts in #buf.type ({mod,Mod,Opts}) so the reset rebuilds via Mod:new(Opts) and preserves configuration. Add a CT: {mod,Mod,Opts}, max 1, a module without push_drop/2, post twice, assert box alive + message delivered.

flowchart TD
    A["start_link / preflight #123;mod, Mod, Opts#125;"] --> B{"check_buffer_type/1<br/>requires new/1, push/2, pop/1, drop/2"}
    B -->|"push_drop/2 NOT required (optional)"| C["ok — box starts, type normalized to #123;mod, Mod#125;"]
    C --> D["buffer fills to max, overflow post arrives"]
    D --> E["insert/2 -> push_drop/4 (pobox.erl:508)"]
    E --> F{"function_exported(Mod, push_drop, 2)?"}
    F -->|"true"| G["Mod:push_drop/2 — OK"]
    F -->|"false"| H["fallback: push(T, Msg, drop(T, Size, Data))"]
    H --> I{"drop(#123;mod,Mod#125;, N, Size, Data): Size =&lt;= N?"}
    I -->|"Size &gt; N"| J["Mod:drop(N, Data) — OK"]
    I -->|"Size =&lt;= N (drop-all reset)"| K["Mod:new/0 (pobox.erl:536)"]
    K --> L["CRASH: function not exported #123;Mod, new, 0#125;<br/>box dies, buffered messages lost"]

    classDef danger fill:#ffebe9,stroke:#cf222e,color:#1A1A1A;
    classDef ok fill:#dafbe1,stroke:#1a7f37,color:#1A1A1A;
    classDef neutral fill:#fff8c5,stroke:#bf8700,color:#1A1A1A;
    class L danger
    class K danger
    class C,G,J ok
    class B,F,I neutral

Loading

Evidence & trace

Reproduced against branch tip 05629f27 in an isolated worktree. Built newone_buf exporting only new/1, push/2, pop/1, drop/2 (no new/0, no push_drop/2):

preflight(#{max=>1, type=>{mod, newone_buf, pfx}})  ->  ok
start_link(#{owner=>self(), max=>1, type=>{mod, newone_buf, pfx}, initial_state=>notify})  ->  {ok,<0.84.0>}
post(Box, m1)   %% fills to size=max=1
post(Box, m2)   %% overflow
** State machine <0.84.0> terminating
** Reason for termination = error:{'function not exported',{newone_buf,new,0}}
**   [{pobox,push_drop,4,[{file,"src/pobox.erl"},{line,511}]},
      {pobox,insert,2,[{file,"src/pobox.erl"},{line,468}]}, ...]

The resize-down and buf_filter paths were tested and do not trigger this: resize_buf keeps NewMax ≥ 1 so ToDrop < Size (Size > N → Mod:drop/2), and filter uses pop/2. The single live trigger is the overflow push_drop-fallback drop-all when the module lacks push_drop/2. The shipped sample pobox_configurable_buf is immune — it exports push_drop/2, so it takes the true branch at :510 and never reaches the reset (independently confirmed by the general-review lanes).

start_link({mod,Mod,Opts}) -> check_buffer_type({mod,Mod,_}) requires only {new,1}+push/pop/drop (:636) -> ok
  -> box starts, buf_new normalizes to {mod,Mod}, data=Mod:new(Opts) (:464)
  overflow post -> insert/2 size=max (:467-468) -> push_drop({mod,Mod},Msg,Size,Data) (:508)
    function_exported(Mod,push_drop,2)=false (:509) -> fallback push(T,Msg,drop(T,Size,Data)) (:511/514)
      drop({mod,Mod},1,Size,Data), Size=<N (:534-536) -> Mod:new()   [arity 0]
        Mod exports only new/1 -> error:{function not exported,{Mod,new,0}} -> gen_statem terminates, buffered msgs lost

---

MED (PR-introduced)

M1 — preflight/1 does not validate name; a malformed name passes preflight but start_link raises badarg

src/pobox.erl:611-625 (check_opts, no name clause) vs :592 (validate_opts guards name)

preflight returns ok for a structurally-invalid name, but start_link then raises badarg — the exact late crash preflight exists to pre-empt. This is the same class the prior self-review's E1 closed for owner/heir, left open for name.

Blast radius: contained — a caller/supervisor/test that runs preflight(Cfg)==ok then start_link(Cfg) gets a raise, not the promised {error,_}. No data corruption or tenant exposure, and start_link still fails safely; hence MED, not HIGH.
Scope of cause: this call-site — one missing clause. (All four lanes flagged it; two called it HIGH, two MED — reconciled to MED on blast radius.)
Fix: add a name clause to check_opts mirroring validate_opts's guard (:592): reject unless Name =:= undefined orelse ?PROCESS_NAME_GUARD_WITH_LOCAL_NO_PID(Name), returning {error,{bad_name, Name}}. Do not reuse is_process_name/1 — it accepts pid and rejects {local,_}, the opposite of the name rule (see L1). Add a preflight_bad_name test.

Evidence & trace

preflight(#{max=>10, type=>queue, name=>"bad_name_string"})            -> ok
start_link(#{max=>10, type=>queue, name=>"bad_name_string", owner=>self()}) -> caught error:badarg
preflight(#{max=>10, type=>queue, name=><a-pid>})                      -> ok
start_link(#{..., name=><a-pid>})                                      -> caught error:badarg

validate_opts (:580-596) has name in its guard head (:592); check_opts (:611) destructures #pobox_opts{owner, heir, initial_state, type} and never binds name.

preflight(Cfg) -> check_opts(#pobox_opts{owner,heir,initial_state,type}) (:611) [name not bound] -> ok
start_link(Cfg) -> validate_opts(Opts) (:162/:181) -> guard requires Name=:=undefined orelse PROCESS_NAME_GUARD_WITH_LOCAL_NO_PID(Name) (:592) -> fails -> erlang:error(badarg) (:596)

M2 — check_buffer_module collapses every code:ensure_loaded/1 error to {module_not_loaded, Mod}, masking on_load_failure / sticky_directory / badfile

src/pobox.erl:641-645

A buffer module that exists on disk but whose -on_load failed (or lives in a sticky dir, or is a bad .beam) is reported as {module_not_loaded, Mod} — misleading, since the module is present, it just failed to initialize.

Blast radius: contained — diagnostics only; sends whoever is debugging a config down the wrong path.
Scope of cause: this call-site.
Fix: preserve the reason for non-nofile cases: {error,nofile} -> {module_not_loaded,Mod}; {error,Reason} -> {module_load_error,Mod,Reason}. (Relatedly, preflight — advertised as validating "without starting a process" — actually loads the module and runs its -on_load as a side effect; see L2.)

Evidence & trace

Reproduced: built onload_fail with -on_load returning {error, deliberate_onload_failure}.

raw code:ensure_loaded(onload_fail)                       -> {error, on_load_failure}
pobox:preflight(#{max=>10, type=>{mod, onload_fail}})     -> {error, {module_not_loaded, onload_fail}}
%% ...and the on_load function EXECUTED (WARNING REPORT emitted) purely from calling preflight

preflight -> check_buffer_type({mod,Mod}) (:635) -> check_buffer_module(Mod,{new,0}) (:641)
  -> code:ensure_loaded(Mod) returns {error, on_load_failure} (:642) -> matched by {error,_} clause (:644)
  -> {error,{module_not_loaded,Mod}}   [real reason discarded]

LOW

L1 — PROCESS_NAME_GUARD_WITH_LOCAL_NO_PID false-accepts {local, NonAtom} names (pre-existing)

src/pobox.erl:51-54 (macro)

An invalid {local, NotAnAtom} name passes the pobox guard but gen_statem:start_link/4 raises anyway — the config still fails, one layer deeper. Pre-existing on master, not introduced here. Flagged because the M1 fix must use a dedicated name predicate, not is_process_name/1; a shared is_server_name/1 would fix both.

Evidence & trace

start_link(#{name=>{local,"str"}}) -> validate_opts guard (:592) accepts (tuple_size==2, element(1)==local) -> passes -> gen_statem:start_link({local,"str"},...) -> gen rejects non-atom local name -> badarg (deeper)

L2 — preflight/1 loads the buffer module (runs its -on_load) despite the "without starting a process" doc

src/pobox.erl:598-601 (doc) · :641-644 (side effect)

A "read-only" validator mutating the global code server and running arbitrary -on_load code is a mild surprise. No concurrency hazard (code:ensure_loaded is serialized by the code_server; -on_load runs once under its lock). Fix: either note it in the preflight/1 doc, or check exports without loading via code:which/1 + beam_lib:chunks/2.

Evidence & trace

preflight (doc "WITHOUT starting a process", :599) -> check_buffer_module (:641) -> code:ensure_loaded(Mod) (:642) -> loads Mod + runs Mod's -on_load if present   [unadvertised side effect]

L3 — start_link fail-fast converts only buffer-module errors to {error,_}; owner/heir/name/max/initial_state still raise badarg

src/pobox.erl:160-185 · :595-596

The two entry points disagree on failure representation: preflight returns {error,Reason} for every bad field, but start_link returns {error,_} only for a bad buffer module and raises for other structural errors. Likely intentional (structural errors = programmer errors → raise, matching OTP norms). Fix: document it, or route validate_opts through {error,_} too. Author's call — not blocking.

Evidence & trace

start_link(#{owner=>bad}) -> validate_opts (:162) -> guard fails -> erlang:error(badarg) (:596)   [NOT {error,_}]
  vs preflight(#{owner=>bad}) -> check_opts (:614) -> {error,{bad_owner,bad}}   [asymmetric]

L4 — README + pobox_buf behaviour comment document the H1 footgun as a safety guarantee

README.md:129-136,148-150 · src/pobox_buf.erl:12-13

The README says "Plain {mod, Module} buffers keep using new/0 and are unaffected" and lists required callbacks as "new/0 or new/1, plus push/2, pop/1, drop/2" — omitting push_drop/2. The behaviour comment goes further: "both are optional so an opts-only buffer need not define new/0." That is exactly the H1 shape, stated as safe. Fix: best resolved by the H1 code fix (delegating the reset to Mod:drop/2 removes the new/0 requirement, making the doc caveat unnecessary). Otherwise add a sentence: a custom buffer that omits push_drop/2 is reset via Mod:new/0 on a drop-all, so it must export new/0 even when constructed via new/1.

Evidence & trace

README:135 "keep using new/0 and are unaffected" + README:149 required callbacks omit push_drop/2 + pobox_buf.erl:12-13 "opts-only buffer need not define new/0" -> reader builds opts-only buffer w/o push_drop/2 -> overflow -> pobox.erl:536 Mod:new/0 -> crash (H1). The contract certifies the footgun.

Runtime-observable behavior (event fan-out)

The event-fan-out lane was armed by the push_ token — a FALSE ARM: push_ matched push_drop/2, a pure in-#buf{} data-structure callback, not a Phoenix.PubSub/channel/GenServer message. No DB write, broadcast, channel push, or async job is introduced. The two observable deltas are the synchronous return-value change (fail-fast {error,_} instead of spawn-then-crash — strictly safer; a linked caller no longer sees a trapped EXIT for the bad-module case, a supervisor gets a clean start-failure) and the code:ensure_loaded side effect (L2). Both were verified statically; the H1 crash was verified live.

New write / broadcast / message	path:line	Consumer	Verdict
(none — no runtime message/event added)	—	—	N/A

Out of scope (pre-existing / cross-PR — follow-ups, not blockers)

• {local, NonAtom} name guard — src/pobox.erl:51-54. Pre-existing (L1); tighten to {local, atom()} in a follow-up.
• vsn 1.2.0 → 1.5.0 skips 1.3.0/1.4.0 — src/pobox.app.src + changelog. Intentional cross-PR sequencing (1.3.0 = weighting PR, 1.4.0 = calls PR, per the PR body). If those don't merge first the changelog has a gap; out of scope here.

Refuted (candidates killed by verification)

• R1 — Opts discarded downstream — refuted for the happy path: buf_new runs once at init (:294); resize_buf/give_away never reconstruct the buffer, so Opts is fully consumed at construction. (The one exception — the drop-all reset — is H1, not a separate finding.)
• R2 — new/0→optional lets a {mod,Mod} buffer crash at buf_new — refuted at the validation boundary: check_buffer_type({mod,Mod}) requires {new,0} (:635), so such a config is rejected {error,{missing_callback,{Mod,new,0}}}. The real gap is the different one in H1.
• R3 — is_process_name/1 over/under-accepts owner/heir — refuted: it exactly matches PROCESS_NAME_GUARD for owner/heir; malformed {global}/{via,M}/{global,X,Y}/{local,_} all correctly reject, consistent with start_link (verified live).
• R4 — -dialyzer no_match suppression masks a dead clause — refuted: check_buffer_type(Other) is reachable via preflight's unvalidated input (test-covered); the suppression is narrow (one function) and justified. Dialyzer runs clean.
• R5 — return-widening {ok,pid()}→{ok,pid()}|{error,term()} breaks callers — refuted: valid configs still return {ok,Pid} (76 CT pass), a supervisor child MFA returning {error,_} is a valid start-failure, and all specs (positional /3,/4,/5 at :112-137 and map/proplist forms) were widened consistently.

Verified prior fixes

Round	Finding	Status
self-review E1	preflight validates owner/heir	✅ verified (but name still unvalidated → M1)
self-review E2	positional start_link/3,4,5 fail-fast on bad module	✅ verified live
—	-dialyzer no_match suppression justified	✅ verified (R4)

Test execution

CI: the PR shows no configured checks. Ran the suite locally against branch tip 05629f27:

Command	Result
rebar3 ct	✅ 76/76 passed (pobox_SUITE 52, give_away 7, heir 8, preflight 9)
rebar3 proper	✅ 3/3 properties
rebar3 dialyzer	✅ 0 warnings

Tests added by this PR (test/pobox_preflight_SUITE.erl, 9): mod_buffer_with_opts (proves Opts reaches new/1 — real, asserts the prefix on drain), preflight_valid, preflight_bad_max, preflight_bad_type, preflight_module_not_loaded, preflight_missing_callback, start_link_fails_fast_on_bad_module, preflight_bad_owner_and_heir (E1), positional_start_link_fails_fast_on_bad_module (E2).

Tests I ran live during the review (throw-away escript/modules, not committed): reproduced H1 (overflow crash), M1 (name gap, string + pid), M2 (on_load_failure masking); confirmed R3 (owner/heir arity consistency) and the sample's queue:split edges are unreachable via pobox's guards.

Coverage gaps (unit tests only — no property-based proposals): the preflight↔start_link agreement round-trip is untested (would have caught M1 generically); no {mod,Mod,Opts} happy-path test through the positional forms; no test for the H1 overflow-reset of an opts-only buffer (add with the fix).

Recommended action

H1 is the one blocker: the primary new feature (opts-only {mod,Mod,Opts} buffers) ships a config that preflight certifies and the README recommends, but that crashes on first overflow. The minimal fix (delegate the drop-all reset to Mod:drop/2) closes H1 and the latent Opts-loss-on-reset, and makes L4's doc caveat unnecessary. M1 (one missing name clause) and M2 (preserve the ensure_loaded reason) are cheap same-PR fixes. LOWs are polish/pre-existing/doc. This is a self-review COMMENT — take H1/M1/M2 as E-cycle fixes before opening for external review.

Approval gate — exactly these 3, nothing else

• H1 — drop-all reset no longer calls Mod:new/0 for a {mod,Mod,Opts} buffer (delegate to Mod:drop/2, or retain Opts and rebuild via Mod:new(Opts)) — src/pobox.erl:536 + a CT
• M1 — check_opts validates name (dedicated predicate, not is_process_name/1) — src/pobox.erl:611 + preflight_bad_name
• M2 — check_buffer_module preserves the non-nofile ensure_loaded reason — src/pobox.erl:641-644

Does NOT gate: L1–L4, the pre-existing/cross-PR out-of-scope items, and the advisory coverage-gap tests.

[eric-descourtis-thenvoi]

Review addressed ✅

All findings from the review above are fixed (TDD RED→GREEN for the behavioral ones), pushed as aaba06a + 49b7c56. Suite: 79 CT / 3 PropEr / dialyzer clean.

HIGH:

• H1 — an opts-only {mod,Mod,Opts} buffer (only new/1, no new/0, no push_drop/2) now survives overflow: the drop-all reset empties via the mandatory Mod:drop/2 rather than Mod:new/0, which also stops discarding Opts.

MED:

• M1 — preflight/1 now validates name (via is_registered_name/1) → {error, {bad_name, N}} instead of ok-then-badarg.
• M2 — code:ensure_loaded failures are distinguished: {module_not_loaded, Mod} for nofile, {module_load_error, Mod, Why} otherwise.

LOW:

• L1 — {local, NonAtom} is now rejected (a local registered name is always an atom) in both is_registered_name/1 and the guard macro.
• L2 — documented that preflight loads the {mod, Module} (running -on_load) to inspect exports — it does not start a pobox process.
• L3 — documented the start_link asymmetry (structural errors raise badarg; module errors return {error, Reason}) and pointed at preflight/1 for uniform errors.
• L4 — noted an opts-only buffer works fully including on overflow (emptied via drop/2) — the H1 fix; README/behaviour docs updated.