utopia-php / utopia-php/monorepo

Consumer::fetch()'s unsubscribe in finally can replace the exception it was added for

Open
#198 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
PHP
Stars
3
Forks
4
Avg merge
12h 25m
Merged PRs (30d)
103

Description

Consumer::fetch() moved its unsubscribe into a finally so the inbox subscription is not leaked on a throw (a real fix — attemptReconnect() re-subscribes every leaked sid, so the leak compounded across a reconnect storm):

// packages/nats/src/JetStream/Consumer.php:59
try {
    $this->conn->publish($requestSubject, $payload, $inbox);
    $this->collectBatch($sub, $messageBatch, $batch, $timeout);
} finally {
    $sub->unsubscribe();
}

Subscription::unsubscribe() reaches Connection::unsubscribe()Connection::send(), which on a dead socket buffers, attempts a reconnect, and raises ConnectionException('Failed to reconnect to any NATS server') once attempts are exhausted.

In PHP an exception thrown from a finally replaces the one already in flight. So on the exact path the finally was added for — the connection died mid-fetch — the useful error (the -ERR, the JetStreamException, or the original ConnectionException naming what actually happened) is swapped for a generic reconnect failure. The subscription is still correctly cleaned up; only the diagnosis is lost, at the moment it matters most.

Fix: suppress a throw from unsubscribe() when an exception is already propagating — the connection is being torn down anyway, so the unsubscribe has nothing left to accomplish — or capture the original and rethrow it.

Found by a review sweep over main while resolving #192; not introduced by it.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in packages/nats/src/JetStream/Consumer.php at Consumer::fetch() and trace Subscription::unsubscribe() through Connection::unsubscribe() to Connection::send(). Reproduce or cover a fetch failure during teardown, then verify the original exception remains visible while cleanup still occurs without replacing it with a reconnect failure.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.