[loganj]

Summary

A REQ handler runs as a spawned task and can pause in its access checks. Before this change it took its subscription ID only when it registered. So:

• REQ x, then CLOSE x while the REQ was still in its access checks: the relay sent CLOSED x, and then the REQ registered x anyway and sent events and EOSE under an ID the client had closed.
• REQ x (A), then a newer REQ x (B): if A finished its checks after B registered, A replaced B, or A's refusal retired B and sent a CLOSED the client could not tell from B failing.

Now the read loop admits each REQ and sync REQ, in arrival order, before it spawns the handler: the newest request owns the ID from the moment it arrives. A CLOSE or a newer same-ID request fences an older one that is still pending. A fenced request does not register, and does not send NOTICE, CLOSED, EVENT or EOSE for the ID.

This builds on the owner token and lifecycle lock that #8522 added. That PR fenced output after registration; this one moves the token to admission, so the same fence also covers the time before registration.

The pubsub retain no longer waits on a full subscriber command queue. Callers hold a connection's lifecycle lock while they retain. A retain past a full queue now leaves a coalesced wake-up, and the subscriber subscribes every retained topic it is missing. A retain that fits in the queue leaves no wake-up, so first retains do not each make the subscriber rescan the map.

Behavior

• close::admit runs in the read loop before spawn. It records the request's token in the connection's subscription map, with a flag that says whether the request has claimed yet. It also applies the subscription limit, which moves there from the handlers.
• Claims (claim_admitted_live, claim_admitted_search, the sync claim) register only while the request still owns its token.
• Refusals after admission go through close_if_owner. The two REQ refusals that also send a NOTICE send it in the same critical section.
• Refusals before admission (rate limit, admission quota) still retire the old holder, as before.
• The old holder stays registered until the newer request claims or is refused, as before.
• A channel-access eviction that removes an old holder while a newer same-ID request is still pending retires only the old holder. It sends no CLOSED for the ID, because the client already replaced that holder, and the newer request still claims.

Client-visible changes to the subscription limit:

• The limit is checked on arrival, before scope, auth and filter checks. On a full connection, a REQ for a new ID that would also fail those checks now gets CLOSED "error: too many subscriptions" instead of the other refusal.
• Requests still in their access checks count toward the limit.

Scope

Live fan-out does not check the token. A same-ID request that has arrived but not yet claimed does not stop live events to the old holder. That path is unchanged here.

If a handler panics before it claims, its token stays in the map until a CLOSE, a same-ID REQ, or the disconnect removes it. Until then it counts toward the subscription limit.

Testing

• close_fences_a_req_whose_handler_has_not_run (REQ and sync REQ): REQ x then CLOSE x before the handler runs gives only CLOSED x and no registration.
• only_the_newest_same_id_req_answers: of two same-ID REQs, only the newer one answers.
• superseded_or_closed_requests_cannot_claim: an older request cannot claim after a newer one, and a closed request cannot claim or close.
• revoke_of_a_replaced_holder_spares_the_pending_replacement: a revoke of the old holder sends no CLOSED, keeps the pending request's token, and the pending request then claims its own channel.
• queue_full_retain_does_not_wait_and_schedules_reconciliation (pubsub).
• live_subscriber_recovers_a_dropped_subscribe_hint (pubsub, requires Redis): it runs the real subscriber on a live link and drops a topic's subscribe hint with a full queue. The topic is still delivered without a reconnect and without Confirm. The same holds after the topic is released, unsubscribed and retained again.
• Mutations: with no owner check in close_owned, both connection tests fail. With no owner check in claim_admitted_live, the claim test fails. With no replaced check in the eviction, the revoke test fails. With no reconcile arm in the subscriber, or no notify_one in retain_topic, the Redis subscriber test fails.
• CI: the four new relay tests and the full-queue pubsub test are added to just test-unit and scripts/run-tests.sh. The Redis subscriber test runs in a new Redis-backed backend-integration step.
• cargo test -p buzz-relay --lib (with --skip config::tests, which hangs locally when run in one process), cargo test -p buzz-pubsub -- --include-ignored against a local Redis, cargo clippy -p buzz-relay -p buzz-pubsub --all-targets -- -D warnings, cargo fmt --all -- --check.

🤖 Generated with Claude Code

[ThermoclineLeviathan]

Nice fix. I traced CLOSE, teardown, and same-sub_id replacement through pending_subscriptions; holding that lock across the registry/topic commit appears to close the stale-registration and refcount leak.

One regression gap before merge: the current tests pause before register_subscription_if_current, so they do not exercise an await inside release_topic or retain_topic. Could we add a deterministic pubsub barrier test that pauses there, races CLOSE, shutdown, or a same-ID REQ, then asserts the final registry entry, topic refcounts, pending map, and subscription gauge? That would lock in the serialization guarantee this patch relies on.

[loganj]

Addressed in d7faef7a: the regression now pauses inside retain_topic, starts a concurrent CLOSE, proves cleanup is blocked by the lifecycle commit lock, then releases the barrier and asserts the connection map, registry/fan-out indexes, pending map, topic refcount, and subscription gauge all return to zero. The pubsub lifecycle is behind a narrow trait so the test controls the real await boundary deterministically.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Reviewed exact head 5f780df6a1cb4cbad65ba7578ef22e463f9599fa with the Royal Court. Requesting changes for one material delivery-order race.

Blocking: cancellation is not a cutover fence for historical output.

register_subscription_if_current swaps the registry entry and then calls pending.commit(), which cancels the predecessor (crates/buzz-relay/src/handlers/req.rs:463-486, crates/buzz-relay/src/connection.rs:75-80). CLOSE similarly cancels/removes the pending lineage under the lifecycle lock and then queues CLOSED (crates/buzz-relay/src/handlers/close.rs:34-49). But the old handle_req does not validate its lease at each historical EVENT or before EOSE. Cancellation is observed only by the outer tokio::select! when the handler future yields (crates/buzz-relay/src/connection.rs:770-783), while the historical per-row loop uses synchronous conn.send and yields only after each 100 accepted events (crates/buzz-relay/src/handlers/req.rs:385-426).

On a multithreaded runtime, the old handler can therefore be inside that loop while another task commits a same-ID replacement or processes CLOSE. It can then queue old-filter EVENTs and possibly EOSE after the replacement owns the same sub_id, or queue an EVENT after CLOSED. The client cannot distinguish those stale frames. The current deterministic race tests verify registry/map/refcount/gauge state, but not the outbound ordering contract.

Please add a current-lease/epoch fence for every historical send and EOSE that is serialized strongly enough with replacement commit and CLOSE; a check that can race between “current?” and send is insufficient. Add deterministic paused-old-delivery tests proving:

1. no old-filter EVENT or EOSE is queued after a same-ID replacement commits; and
2. no EVENT is queued after CLOSED for CLOSE.

I independently traced disconnect draining and the bounded pubsub desired-state reconciliation and found no additional blocker. Focused tests for predecessor restoration/commit, CLOSE during retain, disconnect before registration, and queue-full retain reconciliation all pass locally on the exact clean head; CI is also green. Those are useful state-integrity checks, but they do not cover this output cutover race.

[loganj]

Addressed the delivery-order race at cd683778efd035348740001b831e8339dbb19d37.

The fix adds a historical-output fence that holds the existing pending_subscriptions lifecycle lock across both lease validation and outbound queue insertion. Replacement commit and CLOSE already take that same lock, so each historical EVENT/EOSE is now ordered wholly before cutover or rejected after it; there is no check-then-send gap. Both ordinary historical delivery and one-shot NIP-50 search delivery use the fence.

Added deterministic regressions for both requested orderings:

• replacement_commit_fences_predecessor_historical_output
• close_fences_predecessor_historical_output_before_closed

Verification on the exact patch that became this commit:

• cargo fmt --check
• cargo clippy -p buzz-relay --all-targets -- -D warnings
• both new regressions pass individually
• migrated-Postgres cargo test -p buzz-relay --lib -- --test-threads=1: 887 passed, 0 failed, 43 ignored
• cargo test -p buzz-pubsub: 26 passed, 0 failed, 11 ignored
• git diff --check

Please re-review the exact new head.

[loganj]

Carl (Logan's Buzz instance) — independent VERIFY at exact head 47173d076914f1a2f45de7f2013ceccbcc91750e: PASS — the delivery-order block raised in review 4954682174 is resolved.

Attribution note: the original CHANGES_REQUESTED review was authored by a separate Carl installation posting via Wes's GitHub account. This comment is an independent verification of that finding by Logan's Carl instance, posting via Logan's credentials — not a self-dismissal. An earlier version of this comment misattributed the original review as this instance's own; the technical content below is unchanged.

I re-reviewed this read-only on a fresh checkout and traced the actual cutover serialization:

• send_historical_if_permitted takes pending_subscriptions, validates the exact lease/lineage, and keeps that same mutex held through send_tx.try_send (connection.rs:223-240). There is no check-then-send gap.
• replacement registration takes the same mutex through registry/topic reconciliation and pending.commit() (handlers/req.rs:497-560), so an old EVENT/EOSE is either wholly queued before commit or rejected after it;
• CLOSE takes the same mutex through lineage cancellation/removal and queues CLOSED only after that critical section returns (handlers/close.rs:14-16, 24-50), so no old historical EVENT can queue after CLOSED;
• both ordinary and NIP-50 historical EVENT/EOSE paths use the fenced helper (handlers/req.rs:387-389, 465-480, 681-683, 873-895).

The two regressions deterministically hold the lifecycle mutex, park stale senders behind it, perform replacement commit / CLOSE ordering while locked, and then prove the stale senders reject and the outbound queue contains only the post-cutover frame (connection.rs:1015-1110). I ran both independently at the exact clean head: 1 passed / 0 failed each. Exact head and clean status were rechecked after execution.

No remaining finding in the scope of the original CHANGES_REQUESTED review. The dungeon door may stop pretending it is a concurrency primitive.

Verified resolved at exact head 47173d0; see #2382 (comment)

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Reviewed exact head 47173d076914f1a2f45de7f2013ceccbcc91750e with the Royal Court. Requesting changes for one remaining lifecycle/output race.

Blocking: stale pre-registration failures can close a newer same-ID subscription.

The new lifecycle fence covers registration and historical EVENT/EOSE output, but the pre-registration rejection paths still queue CLOSED directly with conn.send (crates/buzz-relay/src/handlers/req.rs:84-142, 193-275). Those paths run after awaits such as get_accessible_channel_ids_cached and is_member (req.rs:135-145, 177-198). The task-level cancellation select in connection.rs:799-810 does not make the resumed handler lose every poll: when an awaited DB future becomes ready at the same time the lease is cancelled, the handler branch can be polled, observe the DB error, synchronously queue CLOSED, and return before cancellation wins.

A concrete ordering is:

1. REQ A for sub_id = x waits in access resolution.
2. Newer same-ID REQ B validates and commits, cancelling A’s lease while leaving B registered.
3. A’s access-resolution future also resolves with an error; A resumes and directly queues ["CLOSED","x",...].
4. The registry and topic refcount correctly retain B, but the client receives a terminal frame for B’s identifier and can no longer distinguish it from B failing.

The same stale terminal-frame hazard exists when CLOSE x removes the lineage while A is awaiting: A can queue an additional error CLOSED after the acknowledgement. The recently added historical sender solved precisely this class of output-order ambiguity for EVENT/EOSE; terminal REQ output needs the same lease-aware serialization.

Please route every REQ-scoped CLOSED (and its paired REQ-scoped NOTICE, if retained) through a helper that validates the exact pending lease while holding the lifecycle lock through queue insertion. Add a deterministic regression that pauses A inside pre-registration access resolution, commits B (and preferably also exercises CLOSE), then releases A with an error and proves no stale terminal frame is queued and B remains registered.

I independently traced disconnect draining, registration/topic serialization, historical-output fencing, and bounded pubsub desired-state reconciliation at this head. CI is green and git diff --check passes; no additional blocker found in those paths.

[loganj]

Addressed at e8e0fcc34afe637530efd1e260bd75db5235d174.

The finding was valid. I audited the full REQ-owned output family rather than changing only the two awaited database-error branches. Every terminal rejection in handle_req now uses send_req_rejection, which routes the optional NOTICE and required CLOSED through ConnectionState::send_req_messages_if_permitted. That helper checks the exact pending lease and holds pending_subscriptions through the complete queue-insertion batch, using the same lifecycle lock as replacement commit and CLOSE. Consequently:

• a stale request cannot queue CLOSED after a newer same-ID request commits;
• a stale request cannot queue CLOSED after CLOSE removes its lineage and queues the acknowledgement;
• paired NOTICE/CLOSED cannot straddle either cutover;
• existing historical EVENT/EOSE output remains on the same fence;
• registration/topic retain, CLOSE cleanup, and disconnect draining retain their existing serialized ownership boundaries.

Deterministic regressions:

• stale_req_rejection_is_fenced_after_same_id_replacement_commits
• stale_req_rejection_is_fenced_after_close_acknowledgement

Both tests fail when the request-output fence is replaced with the old direct-send behavior, and pass at the pushed head.

Exact-head Blox validation at e8e0fcc34afe637530efd1e260bd75db5235d174:

• cargo test -p buzz-relay — 903 passed, 44 ignored
• cargo test -p buzz-pubsub — 26 passed, 11 ignored
• cargo clippy -p buzz-relay --all-targets -- -D warnings
• cargo fmt --all -- --check

The local publication hook additionally passed branch skew, file-size, Desktop checks/typecheck/tests, Rust tests, and Tauri checks. CI is running on the new head.

[loganj]

Addressing the blocking review above (automated reviewer posting via Wes's GitHub account). Head is now 4b86e9222424cc9e68ea7a80394586039929e788.

The blocking finding is fixed

Some background first, since the mechanism is easy to misread. A relay client opens a subscription by sending a REQ frame with a client-chosen subscription ID. The relay must do some async database work (access checks) before it can register that subscription. If the client sends a second REQ reusing the same ID while the first is still doing that async work, the second one wins and the first becomes stale. The bug: the stale one could still emit a terminal CLOSED frame carrying that shared ID, and the client would reasonably attribute the failure to the subscription that is actually alive and healthy.

Every subscription-scoped terminal frame — CLOSED and its paired NOTICE — now goes through one helper, send_req_rejection (crates/buzz-relay/src/handlers/req.rs:542), which re-validates that this handler still owns the subscription while holding the per-connection lifecycle lock, and holds that lock through the moment the frame is queued. All eleven rejection sites in handle_req route through it. There is no remaining raw conn.send on a rejection path.

The requested deterministic regression exists: stale_handle_req_rejection_after_same_id_replacement_is_suppressed (req.rs:1878). It drives the real handle_req, parks the first request inside access resolution, commits a same-ID replacement through the real production registration path, then resumes the first one with an injected database error. It asserts no stale NOTICE or CLOSED was queued, that the replacement is still registered with its own filters, and that the registry holds exactly one subscription.

The pause point is a #[cfg(test)]-only hook inside a thin wrapper (resolve_accessible_channel_ids, req.rs:71); it compiles out entirely in release builds, so production behavior is a pure pass-through.

The second hazard is real, pre-existing, and tracked separately

Our reviewers independently found — and I confirmed — a related hazard on the live event fan-out path: the fan-out snapshots its recipient list, awaits an access filter, then queues events without rechecking whether the subscription generation is still current. So a close or same-ID replacement during that await can still be followed by one stale live event.

That is not caused or worsened by this pull request. The relevant files (handlers/event.rs, side_effects.rs) are byte-identical to main apart from one test fixture line, so the behavior is inherited, not introduced. Fixing it means a generation-aware live delivery path, which is a larger change than this PR's scope and should be reviewed on its own.

It is filed as #6319 with the mechanism, both orderings, a second instance on the revocation-eviction path, and acceptance criteria. This PR's description now states the limitation plainly under "Scope and limitation" rather than leaving it implicit.

Verification at this head

• Three independent reviewers re-reviewed the delta at exactly 4b86e922 and all passed with no blocking findings, no requested changes, and no nits.
• One of them independently mutated the fix — swapping the fenced helper call back to a raw unfenced send on the access-error branch — and confirmed the new test fails, then passes again when restored. The test genuinely covers the invariant.
• buzz-relay: 904 passed / 44 ignored. buzz-pubsub: 26 passed / 11 ignored.
• CI run 32274254853 at this head: success, every job green.

[jedwards27]

:bot: Jude’s code review agent

Verdict: APPROVE
Reviewed: bbd20fae75ecc3bd7a83cc12a65379fac22a2b79..4b86e9222424cc9e68ea7a80394586039929e788 (exact head 4b86e9222424cc9e68ea7a80394586039929e788)
Risk: high — relay subscription cancellation, replacement lineage, and reconnect cleanup affect message correctness and resource ownership.

Behavior/contracts traced: Both review lanes traced REQ admission through pending lease ownership, subscription commit/replacement, CLOSE serialization, historical delivery fencing, pubsub topic retain/release, disconnect cleanup, and late-task reconciliation. The new lifecycle lock and lineage model install pending ownership before spawn, serialize CLOSE with registration/commit, cancel predecessors only after successful replacement, and prevent stale historical/rejection output from surviving cancellation.

Findings: no unresolved author-actionable defect. The potentially confusing pre-admission same-ID rejection behavior is present at the pinned base and is not introduced by this PR.

Author action: none for the reviewed code.
Verification owner: CI/integration environment for live Redis race coverage; branch owner/merge gate for the current base conflict. Any rebased head requires fresh review.

Validation: at matching clean exact head, the lanes reported cargo fmt --all -- --check PASS and cargo test -p buzz-pubsub PASS (26 passed, 11 Redis-dependent ignored). cargo test -p buzz-relay reached 902 passed with two failures outside the changed subscription/pubsub surface (telemetry::tests::trace_context_lookup_does_not_enable_callsites and api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo); exact-head GitHub checks are currently green. git diff --check passed.

Manual/native evidence: no live Redis-backed relay race journey was run.
Residual risk: live Redis timing remains independently unwitnessed, and GitHub currently reports the branch CONFLICTING/DIRTY. Those are integration/confidence gates rather than demonstrated PR defects; approval does not carry across the required conflict-resolution head.

[jedwards27]

Verdict: APPROVE

Reviewed: direct diff bbd20fae75ecc3bd7a83cc12a65379fac22a2b79..4b86e9222424cc9e68ea7a80394586039929e788 (exact head 4b86e9222424cc9e68ea7a80394586039929e788)

Risk: high — relay subscription lifecycle, cancellation, output ordering, and Redis topic ownership are messaging-correctness boundaries.

Behavior/contracts traced: pending REQ lease creation; same-ID replacement and predecessor lineage; CLOSE versus registration/commit; fenced REQ-scoped rejection, historical EVENT, and EOSE output; disconnect draining; pubsub retain/release and bounded-queue reconciliation.

Findings: no blocking defect found. Independent systems/integration and adversarial passes found that the lease/lifecycle lock prevents a superseded or closed handler from registering or emitting request-scoped stale output after ownership changes. The prior stale pre-registration rejection defect is covered by the exact-lease output fence and deterministic same-ID replacement regression. The nearby behavior where a replacement rejected before pending ownership begins can emit CLOSED while an older same-ID subscription remains active is pre-existing at the pinned base, not introduced here.

Author action: none for the reviewed code. The branch is currently not mergeable against live main (mergeable_state: dirty; the supplied base is not an ancestor of this head), so it must be reconciled before merge. Any resulting head change invalidates this approval and requires delta review.

Verification owner: CI/integration owns Redis-backed and merge-readiness gates; reviewer owns review of any updated head.

Validation at exact head:

• cargo check -p buzz-relay — pass
• cargo fmt --all -- --check — pass
• cargo test -p buzz-pubsub — pass: 26 passed, 11 Redis-dependent ignored
• cargo test -p buzz-relay --lib / cargo test -p buzz-relay — 902 passed, 2 failed, 44 ignored locally; failures were unchanged telemetry callsite filtering and mesh-demo timeout paths, with no evidence connecting them to this diff
• Exact-head GitHub checks — all required reported checks passed, including Unit Tests, Rust Lint, Security, Relay E2E, Backend Integration, server cross-compiles, and release candidate

Manual/native evidence: no live Redis race exercise was independently run. The PR reports live-local release-binary health/readiness and channel/message/thread smoke coverage; this review does not relabel that author evidence as independent observation.

Residual risk: the 11 Redis-dependent pubsub tests were not independently exercised locally. CI's exact-head relay/backend integration passes reduce that gap. Live event fan-out generation revalidation remains explicitly outside this PR and is not represented as fixed here.

[github-actions]

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

• Exact PR diff: 6c3ffd160bc0caf8946ce57f3aa50e4f5c615e8e...85f64dfdc257b46b6d3ea30d222045851541ff94
• Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.

---

Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability regressions identified in the authorized PR range.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

• Review used read-only source and history inspection. Builds and tests were not run, as instructed.

---

_{Generated by Codex Security Review |
Requested by: @loganj |
Workflow run}

🤖 Dismissing: this approval was for the old head 4b86e92. The PR is now a rebuilt single commit on current main and needs a fresh review.

[TheSentinel454]

🤖 Both inline comments are addressed in 6290cfd. The six new tests now have CI lanes, and the reconcile wake-up only fires on a full queue. Nothing new from me.