Skip to content

fix(subscriptions): stop a subscription delete from disconnecting its subscribers - #20

Merged
yordis merged 13 commits into
mainfrom
yordis/fix-delete-subscription-with-subscribers
Sep 22, 2026
Merged

yordis merged 13 commits into
mainfrom
yordis/fix-delete-subscription-with-subscribers

Conversation

@yordis

@yordis yordis commented Sep 21, 2026 •

Copy link
Copy Markdown
Member
  • Deleting a subscription tore down the process that every one of its subscribers shares, so a caller holding none of it could destroy a live subscription and erase the checkpoint it was working through. The subscribers found out only from the resulting :DOWN, which is indistinguishable from the event store going away.
  • A name is the only thing a caller needs to delete a subscription, and names are shared: concurrent subscribers all connect to one, and an unrelated process can pick a name someone else already holds. Deletion has to be refused while anyone is still attached, otherwise holding a subscription guarantees nothing.
  • A delete that used to answer :ok while subscribers were connected now answers {:error, :subscription_has_subscribers}, which the callback already declared as possible. Callers that relied on it to force a disconnect must unsubscribe first.

@cursor

cursor Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes subscription lifecycle and delete API semantics (:subscribers_connected), with unbounded waits during stop/checkpoint that could affect callers expecting immediate :ok on delete.

Overview
Deleting a persistent subscription no longer kills the shared subscription process while subscribers are attached. delete_subscription / delete_all_streams_subscription now return {:error, :subscribers_connected} when any subscriber is still connected; callers must unsubscribe first. Stopping goes through a new cooperative Subscription.stop/1 (infinite timeout) instead of DynamicSupervisor.terminate_child, and the supervisor waits for the process to exit so a final checkpoint in terminate/2 can finish before the DB row is removed.

Subscription checkpoint writes are now observable via telemetry. Each persisted checkpoint emits [:eventstore, :subscription_checkpoint, ...] with stream_uuid, subscription_name, and last_seen; subscribe passes telemetry_metadata into the subscription process. Usage guide and tests cover delete races, checkpoint timing, and telemetry for refused deletes and checkpoints.

Reviewed by Cursor Bugbot for commit 2aa0d16. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b537daaf-22e1-453d-b720-366cc6c12396

📥 Commits

Reviewing files that changed from the base of the PR and between e75ad72 and f4517a1.

📒 Files selected for processing (3)
  • lib/event_store.ex
  • lib/event_store/subscriptions/subscription.ex
  • test/subscriptions/subscribe_to_stream_test.exs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The change updates subscription deletion to stop a subscription only when no subscribers remain. It reports {:error, :still_subscribed} for connected subscribers and adds tests for termination, cleanup, and connection races.

Changes

Subscription deletion coordination

Layer / File(s) Summary
Termination contract and callback results
lib/event_store.ex, lib/event_store/subscriptions/subscription.ex
Callback specifications use {:error, :still_subscribed}. The subscription process stops only when its subscriber map is empty.
Coordinated subscription termination
lib/event_store/subscriptions/supervisor.ex
stop_subscription/3 monitors the process, requests conditional termination, handles already-terminating processes, and waits for the process to exit.
Deletion behavior validation
test/subscriptions/subscribe_to_stream_test.exs, test/telemetry_test.exs
Tests cover connected and concurrent subscribers, empty and terminating subscriptions, storage cleanup, connection races, helper setup, and telemetry deletion setup.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant EventStore
  participant Supervisor
  participant Subscription
  EventStore->>Supervisor: stop_subscription/3
  Supervisor->>Subscription: stop_unless_subscribed
  Subscription-->>Supervisor: termination result
  Supervisor-->>EventStore: deletion result
Loading

Merge Risk: ⚪ Minimal · up to f4517

The deletion result is consistent with the intentional public contract, and deletion waits for shutdown before checking process liveness. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: subscription deletion no longer disconnects active subscribers.
Description check ✅ Passed The description explains why deletion must be refused while subscribers remain connected and describes the resulting behavior change. It uses an outdated error atom compared with the changeset, but it…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.

Comment @coderabbitai help to get the list of available commands.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread lib/event_store/subscriptions/supervisor.ex

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread lib/event_store/subscriptions/supervisor.ex
… subscribers

Deleting a subscription tore down the process that every one of its subscribers shares, so a caller holding none of it could destroy a live subscription and erase the checkpoint it was working through, with the subscribers learning about it only from the resulting `:DOWN`.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Asking whether a subscription has subscribers and then stopping it were two separate calls, so a subscriber could connect in the gap between them and still be torn down by the very delete its presence should have refused.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ady going down

Unsubscribing answers before the subscription it stops has terminated, so a delete that follows it could reach a process that no longer answers calls and take the caller down with it.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis
yordis force-pushed the yordis/fix-delete-subscription-with-subscribers branch from 57494c2 to 886302f Compare September 22, 2026 01:00
@yordis yordis changed the title fix(subscriptions)!: stop a subscription delete from disconnecting its subscribers fix(subscriptions): stop a subscription delete from disconnecting its subscribers Sep 22, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…isconnect

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…refused

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
… one is in flight

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
… of assuming it

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread lib/event_store/subscriptions/subscription.ex
…s the row

A checkpoint deferred past the reply would have its stale last_seen land on whichever subscription holds the name next, and nothing else declares that it cannot be deferred.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 739df40. Configure here.

Comment thread lib/event_store/subscriptions/subscription_fsm.ex Outdated
… has to wait for

A subscription is given fifteen seconds to write a checkpoint and the call asking it to stop waited five, so a slow write reached the caller as a crash rather than as the wait it is.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…t wrote it

A named instance reported its own name as the event store, so its checkpoints could not be lined up against any other operation it emitted.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Comment thread lib/event_store/subscriptions/subscription.ex
…ts for a checkpoint

A deadline the caller cannot see or set is a deadline they cannot reason about, and the event store already takes a :timeout for exactly this.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis
yordis merged commit 43ac0b7 into main Sep 22, 2026
7 checks passed
@yordis
yordis deleted the yordis/fix-delete-subscription-with-subscribers branch September 22, 2026 19:40
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