P2 [CRF-1] The new "out is unbuffered" comment documents a design that contradicts the live SubscriberChannelSize option, which is exported, defaulted to 16, validated in New, and wired to nothing. (Hisoka P2, Meruem P2, Mafuuu P2, Zoro P2)

Verified: grep SubscriberChannelSize finds only the const (line 36), the Options field (line 85), the default-fill in New (lines 166-167), and one test arg (message_part_buffer_test.go:294). The only subscriber channel is out: make(chan Part) (line 291), always unbuffered. Hisoka: "a caller who sets SubscriberChannelSize: 16 to get backpressure headroom gets a strictly unbuffered channel guarded by a 10s hard send timeout instead." The one test that sets it (SubscriberChannelSize: 1) passes identically for any value because the consumer reads synchronously, so it proves nothing about the option.

This is a public, exported contract that silently no-ops, and the new comment now asserts the opposite intent 200 lines below the option. The documented intent (unbuffered backpressure) and the option are mutually exclusive, so the consistent fix is to delete the field, its const, and the New default block (and the test arg). If buffered delivery is actually wanted, wire out: make(chan Part, b.opts.SubscriberChannelSize) instead, but that contradicts the new comment.

🤖

P2 [CRF-2] The CreateEpisode comment claims creation "starts delivering parts to those early subscribers," but CreateEpisode never wakes any subscriber. (Gon P2, Knov P2, Mafu-san P2)

Verified: CreateEpisode (lines 186-199) calls only episode.markCreated(), which sets created = true and nothing else. The only notify sites are AddPart (line 233) and CloseEpisode (line 253). An early subscriber is parked in deliverSubscriber's select on notifyCh (line 452) and is woken by the first AddPart (or CloseEpisode), not by creation, and there are zero parts to deliver at creation time anyway. TestBuffer_SubscribeBeforeCreateReturnsAndWaitsWithoutNotFound only receives after the AddPart that follows CreateEpisode.

This is the removed createdCh design described as if it were still present. No runtime bug today, but the hazard is latent: Komugi notes a future maintainer who trusts this comment could drop the AddPart notify believing creation owns the wakeup, and every early subscriber would hang. Reword to attribute delivery to the first AddPart after creation, e.g. "Creating the episode makes it eligible to receive parts; the first AddPart wakes early subscribers."

🤖

P2 [CRF-3] The SubscribeToEpisode comment lists "creation" as an event that ends an early subscriber's idle wait, but creation does not wake the delivery goroutine. (Razor P2, Mafu-san P2, Pariston P2, Gon P2)

Same root cause as CRF-2 (line 184). The comment says the subscription "stays idle until creation, closure, cancellation, or buffer shutdown." deliverSubscriber only resumes on notifyCh, stopCh, ctx.Done(), or b.done (lines 451-459); notifyCh is signaled by AddPart and CloseEpisode, never by CreateEpisode. The accurate wakeup set is "the first part added, closure, cancellation, or buffer shutdown." Fix both comments together so the package does not document a non-existent creation-time wakeup in two places.

🤖

P3 [CRF-4] The package doc attributes never-created-placeholder removal to the cleanup loop, but the cleanup loop never touches placeholders; removeSubscriber does. (Leorio P2, Razor P3, Mafu-san P3, Pariston P3, Knov P3)

The sentence "The cleanup loop removes closed episodes after the retention window, and removes never-created placeholders when the last early subscriber leaves" carries "The cleanup loop" as the subject of both clauses. The cleanup loop (startCleanupLoop -> gcClosedEpisodesLocked, lines 331-385) only walks the closedEpisodes heap, which queueClosedEpisodeLocked populates with closed episodes only; a never-created placeholder is never queued there. The placeholder delete(b.episodes, key) lives in removeSubscriber (lines 505-511), triggered when the last subscriber's delivery goroutine exits.

Kept at P3 (one reviewer argued P2), but the consequence Leorio names is worth stating: a reader hunting a placeholder map leak follows this comment into the cleanup loop, finds nothing, and may conclude removeSubscriber's delete is redundant and remove it, leaking one map entry per early-subscribe-then-abandon for the buffer's lifetime. Split the clause so placeholder removal is attributed to subscriber teardown.

🤖

Nit [CRF-5] AddPart still inlines the subscriber-notify loop instead of calling the new notifySubscribers() helper, so the extraction covers only one of its two call sites. (Ryosuke, Zoro)

The PR added episodeState.notifySubscribers() (line 541) and used it in CloseEpisode (line 253), but AddPart keeps the identical inline for subscriber := range episode.subscribers { notifySubscriber(subscriber) } (lines 232-234). This is the repeated pattern #26109 (r3380010948) asked to remove, left half-done. Replace the loop with episode.notifySubscribers().

🤖

Nit [CRF-7] getEpisodeLocked (this PR's new helper, called here by AddPart) and the existing episodeLocked are a confusable pair with opposite semantics and no name signal. (Gon)

episodeLocked (line 396) is get-or-create-placeholder; getEpisodeLocked (line 406) is get-or-ErrEpisodeNotFound. Nothing in the names tells a caller which inserts a placeholder and which rejects, so a maintainer reaching for "the lookup" can pick the wrong one and either leak a placeholder or get a spurious not-found. Both are in-package only; consider renaming the get-or-create one to getOrCreateEpisodeLocked, leaving getEpisodeLocked as the strict lookup.

🤖

Nit [CRF-8] episodeState.close returns an undocumented bool whose meaning (transitioned vs already-closed) is invisible at the call site. (Gon)

The sole caller writes if !episode.close(...) (line 249) to mean "already closed, nothing to do," but the signature close(now time.Time) bool gives no hint. Add a one-line doc: "close marks the episode closed and returns false if it was already closed."

🤖

Note [CRF-11] append([]Part(nil), episode.parts...) is the pre-1.21 clone idiom; slices.Clone(episode.parts) says the same in one call. (Ging-go)

The package is on a modern Go toolchain, so slices.Clone is available. The same idiom also sits at line 430. Identical behavior, so this is a breadcrumb, but a cleanup pass on this package is the natural moment to drop it.

🤖