Skip to content

Add optional message weighting (proposal 1 of #15) - #16

Open
eric-descourtis-thenvoi wants to merge 15 commits into
ferd:masterfrom
eric-descourtis-thenvoi:feat/issue-15-weighting
Open

Add optional message weighting (proposal 1 of #15)#16
eric-descourtis-thenvoi wants to merge 15 commits into
ferd:masterfrom
eric-descourtis-thenvoi:feat/issue-15-weighting

Conversation

@eric-descourtis-thenvoi

Copy link
Copy Markdown

Summary

Adds optional message weighting to pobox: a second, opt-in cap on the total
weight of buffered messages alongside the existing count cap. This implements
proposal 1 of #15. Weighting is entirely opt-in via a max_weight start option — a
box started without it behaves exactly as in 1.2, and the unweighted code paths are
left unchanged.

Scope — why weighting only

#15 bundles three enhancements. This PR is proposal 1 (message weighting) only;
proposals 2 (async post) and 3 (native PO Box calls) are intended as separate PRs so
each stays small and reviewable. Ships as 1.3.0 — a strict superset of 1.2, so a
minor bump.

What's added (all opt-in)

  • max_weight start option → dual cap: a box overflows when count > max
    or weight > max_weight.
  • post/3 and post_sync/4 — a pre-calculated per-message weight (no
    weight-calculation function, per the issue's performance note; the default weight
    is 1).
  • Per-type weighted overflow mirroring each type's count-only behavior: queue
    drops the oldest, stack drops the most-recent existing (keeping oldest+newest),
    keep_old rejects the new message. A message heavier than the whole cap is rejected
    without disturbing the buffer.
  • usage_detailed/1,2#{count, max, weight, max_weight} (works on unweighted
    boxes too, where weight == count and max_weight == infinity). usage/1 is
    unchanged.
  • Map-form resize(Box, #{max, max_weight}) to retune caps at runtime; shrinking
    either cap drops-to-fit. A resize that would flip a box between weighted and
    unweighted is refused with {error, badarg}.
  • Opt-in detailed_mail → drained mail becomes
    {mail, Box, Msgs, #{count, lost, weight, lost_weight}} instead of the trailing
    count/lost scalars.
  • Optional drop_one/1 pobox_buf callback so custom {mod, _} buffers can be
    weighted (sample: samples/pobox_weighted_buf.erl).

Backward compatibility

Strict superset of 1.2. usage/1, post/2, post_sync/2,3, the default mail tuple,
and all default drop behavior are unchanged. The new pobox_buf callbacks
(drop_one/1) are optional, so existing custom buffers compile and run untouched. All
67 original Common Test cases and the 3 original properties pass unchanged.

Implementation discipline

  • 12 atomic, signed commits: 10 test-driven cycles (A1–A10, each test written RED and
    verified failing before the code), plus 2 fixes (E1–E2) from an adversarial
    self-review that found and fixed two real issues before this PR:
    • weightless post_sync/2,3 on a weighted box was deciding full/ok by count
      only;
    • map-form resize could flip a box's weighted-ness, mixing wrapped/unwrapped
      elements.

Test plan

  • 79 Common Test cases (67 original + 12 new weighting cases across
    queue/stack/keep_old and a custom {mod, _} buffer).
  • 4 PropEr properties, including a new invariant — weight =< max_weight AND count =< max — holding at 2000 iterations across all buffer types.
  • rebar3 dialyzer clean; compile with warnings-as-errors clean.
rebar3 ct       # 79 passed
rebar3 proper   # 4/4 properties
rebar3 dialyzer # 0 warnings

Files changed

 src/pobox.erl                      | 282 +++++++++++++++++++++++++++++--
 test/pobox_weight_SUITE.erl        | 198 ++++++++++++++++++++++
 README.md                          |  62 ++++++
 src/samples/pobox_weighted_buf.erl |  31 ++
 test/prop_pobox.erl                |  22 +-
 src/pobox_buf.erl                  |   6 +-
 src/pobox.app.src                  |   2 +-

Follow-ups (separate PRs)

  • Proposal 3: native PO Box calls (call/reply with drop-notification).
  • Proposal 2: async post promises.
  • A preflight/validation layer (fail-fast on custom-buffer + weighting misconfig).

Implements proposal 1 (message weighting) of #15.

🤖 Generated with Claude Code

eric-descourtis-thenvoi and others added 12 commits July 6, 2026 05:13
Introduce a detailed usage view returning a map with count, max, weight
and max_weight. On an unweighted box each message implicitly weighs 1, so
weight == count and max_weight == infinity. This is the reporting surface
the weighting work (issue 15, proposal 1) builds on; usage/1 is untouched.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A box started with max_weight becomes "weighted": post/3 supplies a
pre-calculated per-message weight, elements are stored wrapped as
{Weight, Msg}, and the running total is tracked on the buffer. Draining
unwraps to the bare Msg for the owner filter and decrements the total,
so usage_detailed reports an accurate weight/max_weight. Unweighted boxes
keep the byte-identical count-only fast path (insert/2, filter/4).

Cap enforcement on overflow is the next cycle; this establishes storage,
accounting, and delivery for weighted boxes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A weighted insert now enforces both caps: after pushing, it drops from the
buffer type's drop-end (via drop_one/2) until count =< max and weight =<
max_weight, subtracting each dropped element's weight and bumping the drop
counter. drop_one/2 encodes the per-type drop-end: queue front (oldest),
stack head (newest), keep_old back (newest = reject-new); {mod,_} delegates
to an optional Mod:drop_one/1 (wired in a later cycle). Unweighted boxes are
untouched — they never enter enforce_caps.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the push-then-drop enforcement with make-room-then-push so each
type's weighted overflow mirrors its count-only behavior:
  - queue: drop the oldest (front) to fit the new message;
  - stack: drop the most-recent EXISTING element, keeping oldest + newest
    (the previous impl wrongly dropped the message just posted);
  - keep_old: reject the new message, never dropping what is buffered.
Adds fits_after_add/2, make_room/2 and weighted_push/3.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A message whose weight exceeds max_weight can never fit, so make_room would
otherwise drain the whole buffer chasing impossible room and still overflow.
Reject it up front (counted as a drop), leaving buffered messages intact.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
post_sync/4 supplies a weight and reports ok | full. On a weighted box,
full means the message would not fit under the count or weight cap (or is
oversized) — a weight-aware generalization of post_sync/3's count-only
signal. On an unweighted box the weight is ignored and the count-based
signal is preserved.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Track weight dropped since the last drain in a drop_weight buffer field
(the weight analogue of the drop counter), accumulated on every insert-time
drop and reset at drain. With the new detailed_mail start option, drained
mail becomes {mail, Box, Msgs, #{count, lost, weight, lost_weight}} instead
of the {mail, Box, Msgs, Count, Lost} tuple; the default (false) shape is
byte-identical to today. buf_filter now returns delivered and lost weight
alongside the counts; on unweighted boxes weight == count.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
resize/2,3 now accept #{max => M, max_weight => MW} alongside the integer
count form. Shrinking either cap drops from the drop-end weight-consistently
via shrink_to_caps/1. Integer resize on a weighted box also routes through
the weight-aware path so the tracked total stays correct (the previous
count-only resize_buf would have dropped {Weight, Msg} elements without
adjusting the total). Unweighted integer resize is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…[A9]

Declare drop_one/1 as an optional pobox_buf callback (alongside push_drop/2)
and add a pobox_weighted_buf sample implementing it. A weighted box on a
{mod,_} buffer now accounts dropped weight through Mod:drop_one/1 (the
dispatch clause landed in A3); this cycle adds the sample fixture and proves
the path drops to fit exactly like the built-in queue. The legacy
pobox_queue_buf (no drop_one/1) is retained for the count-only path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add prop_weighted_never_exceeds_caps: after any sequence of weighted posts
across all buffer types (including the custom {mod, pobox_weighted_buf}),
usage_detailed must report count =< max and weight =< max_weight. Passes at
2000 iterations. Exercises the dual-cap invariant that the deterministic
A3-A9 cases assert pointwise.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Self-review (adversarial) surfaced two real gaps:

E1: weightless post_sync/2,3 on a weighted box decided full/ok by count
    only, so it could reply ok while a weight-driven drop occurred. Add a
    weighted {post, Msg} handle_call clause using fits_after_add(1, Buf).

E2: resize with a map could flip a box between weighted and unweighted
    (infinity <-> finite max_weight), leaving wrapped {W,Msg} and raw Msg
    elements mixed in one buffer -> crash on next post or silent shape
    corruption on drain. Reject such a resize with {error, badarg} via
    weighting_flip/2; resize/2,3 now spec ok | {error, badarg}.

Confirmed clean by the reviewer: weight-accounting has no drift, skip
preserves the buffer, clause ordering keeps the unweighted path identical,
and make_room/shrink_to_caps terminate. 79 CT + 4 properties green; dialyzer clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a "Weighting" section to the README covering the opt-in max_weight cap,
post/3, post_sync/4, usage_detailed, map-form resize, detailed_mail, and
weighted custom buffers; add the 1.3.0 changelog entry and bump the app vsn.
Backward compatible: unweighted boxes are unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@eric-descourtis-thenvoi eric-descourtis-thenvoi left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferd — self-review of the weighting PR (proposal 1 of #15), run as a rigorous multi-pass sweep. This is my own draft PR; posting the consolidated review here so the findings are on the record before it's opened for external eyes.

Warning

Verdict: COMMENT — 4 PR-introduced HIGH (one validation-gap cluster), 2 MED, several test-coverage gaps. The weighting mechanism is sound (weight accounting, drop-to-fit, overflow per type, and both prior E1/E2 fixes all verified live). The blocker is that the new public inputs — max_weight, detailed_mail, and map-form resize — have no validation seam, so malformed input is silently accepted (black-hole / uncapped) or crashes the box and its linked owner. For a library whose entire job is to protect the owner from overload, a bad config that kills the owner is the sharpest possible edge.

Methodology: fresh multi-pass review — 6 parallel lanes on disjoint slices (LLM-hardening, correctness, perf+contract, test-adequacy, a cross-model codex lane, and two general-review "review-the-review" passes on Opus 4.8 + codex). Every HIGH was verified live by compiling the branch tip (7be0b73) in an isolated worktree and reproducing the exact path in an erl shell — no HIGH is asserted from reading alone. Scope-filtered PR-introduced vs pre-existing; siblings #17/#18/#19 are out of scope. Test suite executed: CT 79/79, PropEr 4/4, Dialyzer 0 warnings.


At a glance

# Sev Title file:line Blast radius
🔴 H1 HIGH validate_opts/1 ignores max_weight + detailed_mail → silent black-hole / uncapped / deferred crash pobox.erl:770-786 incident (owner killed on drain)
🔴 H2 HIGH map-resize with bad max_weight crashes the box instead of {error,badarg} pobox.erl:605-606 incident (owner killed)
🔴 H3 HIGH map-resize max key unvalidated → silent count-cap corruption pobox.erl:611-616 contained (silent data loss)
🔴 H4 HIGH weighted {mod,_} without drop_one/1 crashes on first overflow (README overstates as start-time) pobox.erl:562 · README:60-63 incident (owner killed)
🟠 M1 MED exported -type mail() incomplete for detailed_mail boxes pobox.erl:70-71 contained (typed-consumer drift)
🟠 M2 MED "unweighted path unchanged/cheap" claim literally false (2 extra funcalls/post) pobox.erl:332,357,378 cosmetic
🟠 M3 MED changelog "Fully backward compatible" false for map-resize on unweighted boxes README:83-86 contained (misleads upgrader into H2/H3)
🟠 M4 MED post_sync/4 docs: full can mean "stored after drop," not "did not fit" README:36-37 cosmetic (caller mis-reads reply)

Four HIGHs share one root cause and one fix: a validation seam for the new inputs, reused at start and at resize. Two design steers from the general-review passes: (a) the silent black-hole modes (H1 max_weight=0, H3 max=0) are worse than the crashes because they're invisible — so the fix must fail loud at init/resize, not coerce; (b) route every new-input failure through the library's existing {error,badarg} / erlang:error(badarg,...) seam (the one validate_opts/1 already uses), not a bare function_clause.


HIGH (PR-introduced)

H1 — validate_opts/1 never validates the two new start options

pobox.erl:770-786

validate_opts/1 guards max, type, initial_state, owner, name, heir — but not max_weight or detailed_mail. Both the map and proplist start_link/1,2 forms route through it, so four malformed configs start successfully and each fails badly. The -spec says pos_integer() / boolean(), but nothing enforces it — the type is decorative.

Blast radius: incident — detailed_mail => notabool starts clean, then crashes the box with function_clause on the first drain, and (because init/1 links the owner) takes the owner down. max_weight => 0/-5 is a silent black-hole (every post dropped); max_weight => foo silently disables the weight cap.
Scope of cause: this is the class root — H2/H3/H4 are the same missing-validation seam at other entry points.
Fix: extend the validate_opts guard: (MaxWeight =:= infinity orelse (is_integer(MaxWeight) andalso MaxWeight > 0)), is_boolean(DetailedMail).

flowchart TD
    A["start_link(#{max_weight => V})"] --> B["validate_opts/1<br/>guards max/type/state/owner<br/>NOT max_weight / detailed_mail"]
    B --> C{"value of V"}
    C -->|"0 or -5"| D["W &gt; MW always true<br/>every post oversized-dropped<br/>count stuck at 0 (black hole)"]
    C -->|"foo (atom)"| E["Weight+W =&lt; foo<br/>number &lt; atom term order<br/>ALWAYS true → cap disabled"]
    C -->|"1.5 (float)"| F["int weight &gt; 1.5 oversized<br/>all posts dropped"]
    G["start_link(#{detailed_mail => notabool})"] --> H["box starts OK<br/>accepts posts"]
    H --> I["first drain → mail_msg(notabool,...)<br/>function_clause → box dies<br/>LINKED OWNER killed"]
    classDef bad fill:#ffebe9,stroke:#cf222e,color:#1A1A1A;
    classDef warn fill:#fff8c5,stroke:#bf8700,color:#1A1A1A;
    class D,E,F,I bad;
    class B,H warn;
Loading
Evidence & trace

Live-reproduced against 7be0b73 (all four configs start_link{ok, Pid}):

max_weight=0    : STARTED ok ; after 5 weighted posts → #{count => 0, weight => 0}   (black hole)
max_weight=-5   : STARTED ok ; same black hole
max_weight=foo  : STARTED ok ; after 5 posts → #{count => 5, weight => 15, max_weight => foo}  (cap never fires)
max_weight=1.5  : STARTED ok ; all posts dropped
detailed_mail=notabool : STARTED ok ; post OK ; on active/drain →
    error:function_clause  pobox:mail_msg(notabool,[a],1,0,1,0)  (pobox.erl line 508) ; box dead

The proplist form is equally unguarded (start_link([{max_weight,0},...]) → ok). mail_msg/6 (pobox.erl:508-512) has only false/true clauses, so any other detailed_mail value is a latent crash that fires at the first drain, not at start.

start_link(#{max_weight => 0}) → proplist_to_pobox_opt_with_defaults → validate_opts/1 (pobox.erl:770)
  → guard omits max_weight/detailed_mail → returns Opts (accepted)
  → buf_new(queue, 10, 0) → #buf{max_weight=0}
  → post(_,W) → insert(_,W,#buf{max_weight=0}) → W > MW (1 > 0) → oversized clause → dropped (pobox.erl:536)
detailed_mail=>notabool: … → send/1 → mail_msg(notabool,…) → no clause (pobox.erl:508) → function_clause → EXIT → owner killed via link (init/1 pobox.erl:308)

H2 — map-form resize with a malformed max_weight crashes the box

pobox.erl:605-606 (is_weighted/1), reached from :418 (weighting_flip/2)

Both resize/2 and resize/3 (the timeout form) accept any is_map(Map) with no value validation, then call weighting_flip → is_weighted(New). is_weighted/1 is a partial function matching only infinity and MW > 0; any other value (0, negative, float, atom) is a function_clause inside the gen_statem:call, crashing the box. This fires on weighted and unweighted boxes (a max_weight key on an unweighted box still hits is_weighted(0) before the flip comparison). The README and the resize spec both advertise {error, badarg} as the refusal contract — so this is a contract violation, not just a missing guard.

Blast radius: incident — the box dies and the caller's synchronous resize/resize/3 call gets {'EXIT', {{function_clause,...}}}; a linked owner goes down with it.
Scope of cause: same missing-validation seam as H1, at the resize entry point (both arities).
Fix: validate the resize map's max_weight ahead of weighting_flip/2is_weighted/1 is the crash primitive, so a guard added inside resize_buf would be dead code (the process is already gone). Reject with {error, badarg}, mirroring the existing E2 flip refusal.

flowchart TD
    A["resize(Box, #{max_weight => 0})"] --> B["handle_call {resize, Map}<br/>pobox.erl:417"]
    B --> C["weighting_flip(Map, Buf)<br/>pobox.erl:418"]
    C --> D["is_weighted(0)<br/>pobox.erl:605-606"]
    D --> E{"clause?"}
    E -->|"infinity"| F["ok"]
    E -->|"MW &gt; 0"| F
    E -->|"0 / -1 / 3.5 / foo"| G["NO CLAUSE<br/>function_clause<br/>box dies + owner killed"]
    H["contract says:<br/>{error, badarg}"] -.->|"violated"| G
    classDef bad fill:#ffebe9,stroke:#cf222e,color:#1A1A1A;
    classDef ok fill:#dafbe1,stroke:#1a7f37,color:#1A1A1A;
    class G bad;
    class F ok;
Loading
Evidence & trace

Live-reproduced (trap_exit on, box starts unweighted or weighted, then resize with a bad map value):

unweighted resize mw=0   : resize → {'EXIT',{{function_clause,...is_weighted...}}} | box_alive=false
unweighted resize mw=-1  : {'EXIT',{{function_clause,...}}} | box_alive=false
unweighted resize mw=3.5 : {'EXIT',{{function_clause,...}}} | box_alive=false
unweighted resize mw=foo : {'EXIT',{{function_clause,...}}} | box_alive=false
weighted   resize mw=0   : {'EXIT',{{function_clause,...}}} | box_alive=false
resize(Box,#{max_weight=>0}) → gen_statem:call {resize, Map}
  → handle_call({resize,Map}) (pobox.erl:417) → weighting_flip(Map,Buf) (pobox.erl:418)
  → maps:find(max_weight,Map)={ok,0} → is_weighted(0) (pobox.erl:605)
  → 0 =/= infinity AND not (is_integer(0) andalso 0>0) → no clause → function_clause → box EXIT
  → gen_statem:call raises in caller; linked owner killed

H3 — map-form resize max key is also unvalidated → silent count-cap corruption

pobox.erl:611-616 (resize_buf map clause)

The map-resize path does resize_buf(maps:get(max, Map, ...), B1) with no validation of the max value. Unlike H2 this does not crash — it silently installs a corrupt count cap: #{max => 0} returns ok and makes the box a black hole; #{max => foo} returns ok and, via Size < foo term ordering (number < atom), disables the count cap entirely.

Blast radius: contained — silent data loss / silently-disabled cap; no crash, so it's invisible, which is arguably worse than H2's loud failure.
Scope of cause: same seam; the integer resize/2,3 clauses guard is_integer, > 0, but the map path bypasses that guard for the max key.
Fix: validate map max as pos_integer() before resize_buf (same helper as H1/H2).

flowchart TD
    A["resize(Box, #{max => 0 | foo})"] --> B["resize_buf map clause<br/>pobox.erl:611 — no validation"]
    B --> C{"value"}
    C -->|"0"| D["count cap = 0<br/>black hole (drops all)"]
    C -->|"foo (atom)"| E["Size &lt; foo term order<br/>always true → cap disabled"]
    D --> F["returns ok — SILENT<br/>no crash, invisible"]
    E --> F
    classDef bad fill:#ffebe9,stroke:#cf222e,color:#1A1A1A;
    classDef warn fill:#fff8c5,stroke:#bf8700,color:#1A1A1A;
    class D,E bad;
    class F warn;
Loading
Evidence & trace

Live-reproduced:

weighted map max=0   : resize=ok alive=true ; after 3 posts → #{count => 1, max => 0}      (cap corrupt)
weighted map max=-5  : resize=ok alive=true
weighted map max=foo : resize=ok alive=true ; after 5 posts (was max=2) → #{count => 5, max => foo}  (cap disabled)
weighted map empty   : resize=ok alive=true  (benign — no keys)
resize(Box,#{max=>foo}) → handle_call({resize,Map}) → weighting_flip=false (no max_weight key)
  → resize_buf(Map,B0) (pobox.erl:611) → maps:get(max,Map)=foo → resize_buf(foo, B1)
  → weighted clause shrink_to_caps: Size < foo term-order always true → no drop → cap silently disabled

H4 — weighted {mod,_} buffer without drop_one/1 crashes on first overflow

pobox.erl:562 (make_room/2), pobox.erl:582 (drop_one({mod,Mod},...)); README 60-63

drop_one/1 is declared in -optional_callbacks, so a custom buffer lacking it compiles and starts fine even when the box is weighted. The crash is deferred to the first drop, when drop_one({mod,Mod},Data) → Mod:drop_one(Data) hits an undefined function. That drop can be triggered two ways: a weighted overflow via make_room (pobox.erl:562), or a cap-shrink resize via shrink_to_caps (pobox.erl:641) — the latter fires even on a box that never overflowed. The README claims this "will fail" — implying fail-fast at start — but it actually fails lazily, after appearing healthy.

Blast radius: incident — undefined function crash on overflow or resize-shrink kills the box + linked owner, in production, not in a start-time smoke test.
Scope of cause: this call-site; the fix is a start-time assertion.
Fix: at init/buf_new, when max_weight =/= infinity and type = {mod, Mod}, assert erlang:function_exported(Mod, drop_one, 1) and fail cleanly — OR soften the README to "fails on the first weighted overflow."

Evidence & trace

Live-reproduced with a {mod,_} module exporting everything except drop_one/1:

mod-without-drop_one STARTED: true
after post a(50) alive: true
after overflow post (120 > 100) → error: undefined function nodrop_buf:drop_one/1 ; box_alive=false
start_link(#{type=>{mod,nodrop_buf}, max_weight=>100}) → buf_new → OK (drop_one optional, not checked)
  → post a(50), b(40), c(30): 120 > 100 → insert/3 → make_room(30,B) (pobox.erl:562)
  → drop_one({mod,nodrop_buf},Data) (pobox.erl:582) → nodrop_buf:drop_one/1 UNDEFINED → EXIT → owner killed

MED (PR-introduced)

M1 — exported -type mail() is incomplete for detailed_mail boxes

pobox.erl:70-71

The -export_type'd mail() still declares the 5-tuple {'mail', Self, Msgs, Count, Lost}, unchanged from master. But detailed_mail => true delivers a 4-tuple {mail, Box, Msgs, #{count,lost,weight,lost_weight}} (mail_msg/6, pobox.erl:511). A dialyzer-typed owner receiving on mail() gets a type error against a message the library really emits. Runtime-correct; type-contract drift.

Fix: widen the exported type to a union covering both the scalar and the metrics-map tails.

Evidence & trace
-type mail() :: {'mail', pid(), list(), non_neg_integer(), drop()}   (pobox.erl:70, == master)
mail_msg(true, …) → {mail, self(), Msgs, #{count,lost,weight,lost_weight}}   (pobox.erl:511, 4-tuple)
→ typed consumer spec'd on mail() mismatches the emitted detailed tuple → dialyzer error

The README also omits that detailed_mail=true changes the mail tuple arity (5→4) — a doc gap that reinforces this.


M2 — "unweighted path unchanged / hot path stays cheap" is literally false

pobox.erl:332-333, 357-358, 378-379

On master, active_s(cast, {post,Msg}, S) inlined insert(Msg, Buf). Now the weightless cast re-dispatches {post,Msg}{post,Msg,1}insert/3insert/2two extra function calls per default post. No heap tuple is allocated (1 is a literal call arg, not a constructed message), and the delta is almost certainly sub-1% and lost in the noise of gen_statem cast delivery + queue:in. But it's a real, non-zero regression on the exact path the PR singles out as "cheap/unchanged," so the claim should be softened or the path re-inlined.

Fix: have the weightless clause call insert(Msg, Buf) directly instead of round-tripping through {post,Msg,1}insert/3insert/2.

Evidence & trace
master:  active_s(cast,{post,Msg},S) → insert(Msg,Buf)                 [0 extra calls]
HEAD:    active_s(cast,{post,Msg},S) → active_s(cast,{post,Msg,1},S)   [+1 call]
         → insert(Msg,1,#buf{max_weight=infinity}) → insert(Msg,Buf)   [+1 call]

Measurement recipe (idle box, passive, ≥5 reps, median ns/post, master vs HEAD):

{ok,B} = pobox:start_link(#{owner=>self(), max=>1000000, type=>queue, initial_state=>passive}),
timer:tc(fun() -> [pobox:post(B, m) || _ <- lists:seq(1, 5_000_000)] end).

Expect the delta within run-to-run variance; if not, the inline fix closes it.


M3 — changelog/README "Fully backward compatible / strict superset" is false for the new map-resize surface

README.md:83-86 (changelog), README.md:22-23

"A box without max_weight behaves exactly as before" holds only for callers who touch none of the new surface. But an unweighted 1.2 box upgraded to 1.3 that reaches for the newly-documented map-resize form gets a dead box (H2) or a silent black-hole (H3) — the exact "unchanged" configuration the prose promises. The claim actively misleads an upgrader toward the crash. Strike or hedge "Fully backward compatible" until H1–H4 land. (The 1.3.0 minor bump itself is fine — old code is unaffected; it's the prose that oversells.)

Evidence & trace
1.2 user upgrades → existing unweighted box → tries newly-advertised map resize:
  resize(Box, #{max_weight => 5000}) is the documented retune form, BUT
  resize(Box, #{max_weight => 0})  → is_weighted(0) → function_clause → dead box   (H2)
  resize(Box, #{max => 0})         → ok, count cap silently 0 (black hole)          (H3)
→ "behaves exactly as before" is false the moment the new surface is used

M4 — post_sync/4 docs: full can mean "stored after dropping older," not "did not fit"

README.md:36-37, pobox.erl:392-410

The README frames post_sync/4's full as "the message would not fit." That's true for keep_old (reject-new) and for oversized, but for queue/stack a weight-saturated box replies full and still stores the new message after dropping an older element — the count-only post_sync/3 had the same subtlety and the new doc doesn't carry the caveat forward. A caller treating full as "not stored" will be wrong on queue/stack. Same gap the test-adequacy lane flagged (E1 queue/stack reply uncovered).

Evidence & trace
weighted queue, weight-saturated:
  post_sync(Box, Msg, W, T) → handle_call E1/weighted clause (pobox.erl:392/407)
  → fits_after_add=false → reply `full`
  → BUT ?MODULE:StateName(cast,{post,Msg},S) still runs → make_room drops oldest, STORES Msg
→ reply says `full`, message IS in the buffer (unlike keep_old where `full` = rejected)

Fix: extend the doc caveat to say full on queue/stack means "accepted with an overflow drop," matching post_sync/3.


LOW

  • L1 — direct-arg start_link/3,4,5 silently cannot be weightedpobox.erl:116-148. The positional arities never populate max_weight/detailed_mail; only the map/proplist forms expose them. Defensible (keeps tuple arities frozen), but the README never states it, so a user reaching for start_link(Name,Owner,Max,Type) gets a silently unweighted box. Doc gap.
  • L2 — no anonymous weighted postpobox.erl:480,484. handle_info({post,Msg}) supports raw Box ! {post,M}, but there is no {post,Msg,W} info clause, so a raw Box ! {post,M,W} falls to the _Info catch-all and is silently ignored. Only reachable by manual send (post/3 uses cast); degrades gracefully (no crash). API-symmetry note.
  • L3 — stale commentpobox.erl:533. insert/3's comment "(Cap enforcement is added in a later cycle.)" is inaccurate — enforcement (make_room, oversized-reject, keep_old reject) is in the same function body immediately below. Refactor leftover; delete the parenthetical.
  • L4 — named-map start_link(Name, #{...}) README omits the new keysREADME.md:199-208. The options block documents max_weight/detailed_mail for the unnamed map form but the named-form section doesn't list them, despite start_link/2 accepting them.

Runtime-observable behavior (event fan-out)

The diff touches GenServer message handling (gen_statem casts/calls) and the owner-mail send. Enumerated consumers of the new/changed messages:

New / changed message path:line Consumer Verdict
{post, Msg, W} cast pobox.erl:334,359,380 insert/3weighted_push/make_room ✅ weight accounting consistent (verified live incl. skip-first/mid, cascade)
weightless {post,Msg} on weighted box pobox.erl:392 handle_call E1 clause → fits_after_add(1) ✅ replies full/ok correctly (E1 closed)
{resize, Map} call pobox.erl:417 weighting_flipis_weighted 🔴 crashes on bad value (→ H2); silent corrupt on bad max (→ H3)
owner-mail (detailed_mail) pobox.erl:502 OwnerPid ! mail_msg(...) 🔴 function_clause on bad detailed_mail (→ H1); type drift (→ M1)
{mod,_} drop_one/1 pobox.erl:582 Mod:drop_one/1 🔴 undefined-function crash on overflow (→ H4)
anonymous {post,Msg} info pobox.erl:480 routes to count-only cast (weight 1) ✅ unchanged from master; intentional (no weighted anonymous post)

Verified sound (refuted candidate bugs — do not re-flag next round)

  • make_room/2 + shrink_to_caps/1 non-exhaustive {{value,{EW,_}}} = drop_one(...) match — cannot crash: both guard #buf{size=0} before calling drop_one, and the oversized clause (W > MW) intercepts before make_room. Live-probed on count-overflow, weight-overflow, and cascade.
  • fits_after_add strict-< (count) vs =< (weight) — intentional; matches the exactly-at-cap = ok test expectation.
  • Weight-accounting invariant through weighted_push/make_room/shrink_to_caps/wfilter/buf_filter — live-verified consistent, including the skip early-return (buffer + weight preserved on skip-first and skip-mid).
  • Unweighted box byte-identical to 1.2 on all well-formed paths (post/2, post_sync/2,3, default mail tuple, integer resize, drops). The superset claim holds for valid input; it breaks only on malformed new options (H1–H4).
  • record_info/default_opts zip alignment after the new record fields — safe (both derived from the same record definition).
  • E1 / E2 prior fixes — both present at tip and tested; holes closed for valid inputs.

Test execution

CI: upstream runs no CI on this branch (fork PR). Ran locally in an isolated worktree at 7be0b73 with the vendored rebar3:

Suite Result
rebar3 ct 79/79 pass (67 original + 12 new weighting cases)
rebar3 proper 4/4 properties pass (incl. new prop_weighted_never_exceeds_caps)
rebar3 dialyzer 0 warnings (exit 0)

Tests added by the PR (12 CT + 1 property) cover the type×overflow matrix, oversized rejection (queue), both prior fixes, detailed_mail, and map/integer resize. Coverage gaps (all PR-introduced behavior, no assertion — none are blockers, but worth closing):

  • No negative-config tests at all — the entire H1–H4 cluster (bad max_weight/detailed_mail/max, {mod}-without-drop_one) is untested. Add these alongside the fixes.
  • Weighted skip-drain path (wfilter skip return) — zero coverage (behavior is correct, verified live; just unasserted).
  • E1 weightless post_sync full reply covered only for keep_old; queue/stack (reply full while the message is still stored-with-drop) uncovered.
  • post_sync/4 full via the count cap (vs weight cap) not asserted.
  • Oversized reject tested only for queue (stack/keep_old/mod behave correctly live, but unasserted).
  • detailed_mail on an unweighted box (weight==count) uncovered.
  • Integer resize grow (no-drop) not asserted.
  • prop_weighted_never_exceeds_caps only checks final caps hold — never drains, resizes, or checks exact weight-accounting (weight == Σ buffered weights).

(Live-validation of the "verified sound" items above was done in throw-away scratch modules, not committed.)


Recommended action

The weighting mechanism is well-built and the happy paths are solid. The one thing to fix before this is external-review-ready is the validation seam: H1–H4 are a single root cause (new public inputs with no validation), and a single shared helper — validate max_weight (infinity | pos_integer), detailed_mail (boolean), and map-resize max/max_weight — closes all four, plus a start-time drop_one/1 assertion for weighted {mod,_}. Route every one of those failures through the library's existing {error,badarg} / erlang:error(badarg,...) convention (the one validate_opts/1 already models), and fail loud at init/resize — the silent black-hole modes (H1/H3) are worse than the crashes precisely because they're invisible, so coercion is the wrong fix. As a self-review feeding shipping-rigor E-cycles: H1–H4 → E-cycles (RED negative-config test each, then GREEN); M1 fold-in (type widening); M2 optional (re-inline or soften the claim); M3/M4 are doc edits that should land with the fixes. The "strict superset / minor bump" framing is accurate for old code but breaks the moment an upgraded box uses the new map-resize surface — land validation before advertising 1.3.0 as a clean superset.

Approval gate — exactly these, nothing else

  • H1 — validate_opts/1 validates max_weight + detailed_mailpobox.erl:770-786
  • H2 — bad map-resize max_weight replies {error,badarg}, doesn't crash — pobox.erl:417,605
  • H3 — bad map-resize max rejected, not silently installed — pobox.erl:611-616
  • H4 — weighted {mod,_} without drop_one/1 fails at start (or README softened) — pobox.erl:562, README:60-63

Does NOT gate: M1, M2, the test-coverage gaps, and doc polish.

eric-descourtis-thenvoi and others added 3 commits July 6, 2026 17:38
Adversarial self-review found the new public inputs had no validation, so
malformed configs were silently accepted (black-hole / uncapped) or crashed
the box AND its linked owner. Fix, failing loud through the existing
{error,badarg}/erlang:error(badarg) seam:

  H1 validate_opts now guards max_weight (infinity | pos_integer) and
     detailed_mail (boolean) — bad values raise badarg at start instead of
     black-holing every post or crashing on the first drain.
  H2 map-form resize with a malformed max_weight returns {error, badarg}
     (via valid_resize/1, run before weighting_flip) instead of a
     function_clause in is_weighted/1 that killed the box + owner.
  H3 map-form resize validates the max key (pos_integer) instead of silently
     corrupting the count cap.
  H4 a weighted {mod,_} buffer without drop_one/1 fails fast pre-spawn with
     {error, {missing_callback, {Mod, drop_one, 1}}} instead of crashing
     lazily on the first overflow/resize-shrink.

Every case was live-reproduced by the review. 85 CT + 4 PropEr + dialyzer clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…M1,M3,M4]

M1: extend the exported -type mail() with the detailed_mail 4-tuple (map)
    shape so typed consumers don't drift.
M3: the changelog "Fully backward compatible" line overclaimed — restate as
    "existing 1.2 API and unweighted boxes unchanged; new inputs validated".
M4: document that post_sync/4's `full` does not by itself mean YOUR message
    was dropped (queue/stack keep it, dropping an older one); only keep_old /
    oversized means it did not enter — mirroring the post_sync/3 caveat.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
L2: handle_info now accepts a raw `Box ! {post, Msg, Weight}` on a weighted
    box (symmetric with post/3), instead of silently ignoring it via the
    _Info catch-all.
L1: README notes weighting is only exposed by the map/proplist start_link/1,2
    forms; positional start_link/3,4,5 always produce an unweighted box.
L3: drop the stale "(Cap enforcement is added in a later cycle.)" comment on
    insert/3 — enforcement is in the same function.
L4: the named-map start_link(Name, #{...}) options block already lists
    max_weight/detailed_mail (added with the earlier docs commit).

86 CT + 4 PropEr + dialyzer clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@eric-descourtis-thenvoi

Copy link
Copy Markdown
Author

Review addressed ✅

All findings from the review above are fixed (TDD RED→GREEN for the behavioral ones), pushed as 9d14ced + a1883ee. Suite: 86 CT / 4 PropEr / dialyzer clean.

HIGH — validation-gap cluster (all live-reproduced, all fixed):

  • H1 — validate_opts guards max_weight (infinity | pos_integer) + detailed_mail (boolean); bad values raise badarg at start instead of black-holing / crashing the owner on drain.
  • H2 — map-resize with a bad max_weight returns {error, badarg} (via valid_resize/1) instead of a function_clause in is_weighted/1.
  • H3 — map-resize validates the max key (pos_integer) — no silent count-cap corruption.
  • H4 — a weighted {mod,_} without drop_one/1 fails fast pre-spawn with {error, {missing_callback, {Mod, drop_one, 1}}}.

MED:

  • M1 — exported -type mail() extended with the detailed_mail metrics-map shape.
  • M3 — changelog "Fully backward compatible" restated precisely (existing API + unweighted boxes unchanged; new inputs validated).
  • M4 — post_sync/4 full caveat documented (on queue/stack your message is kept; only keep_old/oversized means it didn't enter).
  • M2 — the "unweighted path cheap/unchanged" claim was only in the PR body (no committed assertion); changelog wording is now precise.

LOW:

  • L2 — anonymous weighted post Box ! {post, Msg, W} is now honored (was silently ignored).
  • L1 — README notes weighting is exposed only by the map/proplist start_link/1,2 forms.
  • L3 — stale "(Cap enforcement added in a later cycle.)" comment removed.
  • L4 — named-map start_link(Name, #{...}) options block already lists max_weight/detailed_mail.

@eric-descourtis-thenvoi
eric-descourtis-thenvoi marked this pull request as ready for review July 6, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant