PCBC-1058: Add Cluster::disconnect() by avsej · Pull Request #269 · couchbase/couchbase-php-client

avsej · 2026-10-30T21:09:44Z

Problem

A cached connection cannot be closed from PHP. It stays open until the shutdown sweep expires it, or until the process exits. With the default ini settings the sweep never expires it.

Depends on #268, which introduces the detach path.

Change

Cluster::disconnect() closes the connection the cluster holds and removes it from the cache. It acts on the resource, not on a key, because a connection detached by setAuthenticator() is no longer under its key.
The core cluster closes; the handle stays until nothing references it. Every request on the closed cluster completes with cluster_closed. The handle, and the io_context the core cluster runs on, are freed by the sweep once no PHP value references the resource. Transactions and ScanResults hold that resource, so the io_context cannot be destroyed under a live transaction or scan. The ticket proposed refusing when GC_REFCOUNT > 0. That would always refuse, because the calling cluster and every bucket and collection derived from it hold a reference.

Every later use throws. Extension functions fetch the handle through one helper, which now throws CouchbaseException (errc::network::cluster_closed) for a closed connection:

if (handle != nullptr && handle->closed()) {
  couchbase_throw_exception({ couchbase::errc::network::cluster_closed,
                              ERROR_LOCATION,
                              "the connection has been closed by Cluster::disconnect()" });
  return nullptr;
}

closeConnection() does nothing for a closed connection. Fork notification still reaches it, because its io_context keeps running until the handle is freed.

A range scan started before disconnect() throws instead of ending early. Closing the cluster cancels the scan, which then reports range_scan_completed, the same signal as a finished scan. scan_result_resource now checks for a closed cluster before each fetch:

/* closing the cluster cancels the scan, which then reports range_scan_completed */
if (auto [ec, origin] = cluster_.origin(); ec == couchbase::errc::network::cluster_closed) {
  return { {}, { ec, ERROR_LOCATION, "Unable to fetch scan item" } };
}

disconnect() is declared on ClusterInterface. The couchbase2:// cluster throws UnsupportedOperationException, as its setAuthenticator() does.
testDisconnectClosesConnectionForEveryHolder disconnects a cluster, then checks that a collection opened before, another cluster sharing the connection and the cluster itself throw. It then runs fork notification and a second disconnect(), and checks that a new cluster with the same arguments bootstraps again. testDisconnectFailsScanStartedBefore and testDisconnectFailsTransactionsCreatedBefore check that a scan and a Transactions object obtained before disconnect() fail with cluster_closed. testForkAfterDisconnect forks with a disconnected cluster alive and checks that the child exits cleanly.

Behaviour change for other callers

Classes outside the SDK that implement ClusterInterface must add disconnect().

Copilot

🟡 Changes recommended

Mutating the original ClusterOptions before calling setAuthenticator() can cache an existing connection under settings never applied to it, causing later clusters to reuse the wrong configuration.

1 open issue

Snapshot bootstrap options to avoid re-keying with mutated settings · New

What changed in this PR

Adds Cluster::disconnect() to explicitly close a shared native connection and remove it from cache lookup while preserving PHP resource references for safe cleanup.

Changes:

Exposes disconnect through the cluster interface; Protostellar reports it as unsupported.
Adds closed-connection errors, cleanup handling, and fork-notification guards.
Includes cache-key and authenticator re-keying changes, with tests for sharing and disconnect behavior.

File	Description
`tests/ConnectionCacheTest.php`	Tests cache sharing, credential re-keying, disconnect, and reconnection.
`src/wrapper/persistent_connections_cache.hxx`	Declares cache move, close, and updated cleanup functions.
`src/wrapper/persistent_connections_cache.cxx`	Implements re-keying, connection closure, and safe cleanup.
`src/wrapper/connection_handle.hxx`	Tracks detached state and allows cache-key updates.
`src/php_couchbase.cxx`	Exposes native close operations and rejects closed-connection use.
`Couchbase/Protostellar/Cluster.php`	Explicitly rejects unsupported disconnect calls.
`Couchbase/ClusterOptions.php`	Removes the authenticator-only hash helper.
`Couchbase/ClusterInterface.php`	Adds the disconnect contract.
`Couchbase/Cluster.php`	Adds disconnect and derives cache keys from exported options.

_{Review effort: Balanced}

Motivation ---------- The persistent connection cache keyed a connection on the connection string, the authenticator and the core tracing, core metrics and span buffering flags. Clusters that differed in any other option received the same cached connection. Examples are a timeout, TLS verification, the network and DNS-SRV. The second cluster ran with the options of the first. Modifications ------------- The cache key is the SHA-256 of the connection string and the serialized ClusterOptions::export(). export() carries every option passed to the extension, the authenticator included. Its keys are in a fixed order, so the key does not depend on the order the setters were called in. serialize() is used rather than json_encode(), which fails on strings that are not valid UTF-8. The internal ClusterOptions::authenticatorHash() is removed, since the exported options already cover the authenticator. Results ------- Clusters share a connection only when their connection strings and all exported options are equal.

Copilot

🟡 Changes recommended

Re-keying from the caller’s mutable options can cache an existing connection under timeout settings it never applied, causing later clusters to reuse a connection with the wrong configuration.

2 open issues

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

Native Zend lifetime changes require human validation of retained scan/transaction cleanup and request-shutdown behavior because the relevant core dependencies and runtime verification are unavailable.

No open issues 🎉

2 resolved since last review

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

The review remains incomplete because one or more reviewers were stopped by a runtime budget limit.

No open issues 🎉

_{Review effort: Balanced}

Copilot

🟡 Changes recommended

Fork handling skips disconnected handles that still own running I/O state, risking hangs or failures in child cleanup, and the scan test can fail because its fixture is not durably persisted.

2 open issues

_{Review effort: Balanced}

Copilot

🟡 Changes recommended

Unresolved lifecycle defects can hang transactions after close, destroy an I/O context during asynchronous teardown, and reopen closed connections after a fork.

3 open issues

2 resolved since last review

_{Review effort: Balanced}

+  impl_->core_api().close([barrier]() {
+    barrier->set_value();
+  });
+  f.wait();


+    /* a closed handle still runs its io_context, which must be quiesced and restarted too */
    connection->notify_fork(event);


Copilot

🟡 Changes recommended

Connection lifecycle flaws can hang existing transactions after closure and allow native transaction teardown to access a reclaimed connection, risking use-after-free.

5 open issues

_{Review effort: Balanced}

Copilot

🟡 Changes recommended

Unresolved connection-lifecycle issues can destroy I/O contexts during native teardown, hang retained transactions after disconnect, and reopen disconnected connections in forked children.

5 open issues

1 resolved since last review

Terminate and reap the child when the shutdown test times out

_{Review effort: Balanced}

Motivation ---------- Cluster::setAuthenticator() replaced the credentials of the cached connection but left it under the key derived from the previous credentials. A cluster connected later with the previous credentials received that connection and ran with the new credentials. Sockets opened before the change stay authenticated with the previous credentials, so the connection cannot serve either set of credentials. Modifications ------------- After the credentials are applied, the extension moves the cache entry to a key that no lookup produces and marks it detached. The cluster that called setAuthenticator() keeps using the connection. Removing the old entry bypasses the persistent list destructor, which would free the resource while PHP values still reference it. After each request has released its variables, detached connections that nothing references are freed, whatever the couchbase.persistent_timeout and couchbase.max_persistent settings are. The expiry rule still applies only when those settings enable it, and is evaluated after that sweep. Creating or detaching a connection frees them as well, so a long-running request does not keep one open per setAuthenticator() call. Detached connections are kept in a per-thread list, so that sweep visits only them. phpinfo() reports the number of persistent connections as couchbase_persistent_connections. Transactions and ScanResults keep a reference to the connection resource. They use the connection's core cluster, so freeing the connection while either is alive would close it under them. Their native objects are destroyed on separate threads, so these also hold a token that keeps the connection from being freed until that teardown has finished. Results ------- A cluster connected after setAuthenticator() bootstraps its own connection and authenticates with its own credentials, whichever they are.

Motivation ---------- A cached connection could not be closed from PHP. It stayed open until the persistent connection sweep expired it, or until the PHP process exited. With the default ini settings the sweep never expires it. Modifications ------------- Cluster::disconnect() closes the cluster's connection and removes it from the cache. It acts on the connection the cluster holds rather than on a cache key, because a connection detached by setAuthenticator() is no longer under its key. The connection is detached from its cache key and its core cluster is closed, so every request on it completes with cluster_closed. The handle and the io_context behind the core cluster stay until no PHP value references the resource; the sweep frees them then. Transactions and range scans run on the core cluster, and destroying the io_context while one of them is alive would free memory they still use. Every extension function that fetches the connection handle throws CouchbaseException carrying errc::network::cluster_closed for a closed connection. closeConnection() does nothing for one. Fork notification still reaches it, because its io_context keeps running until the handle is freed. A range scan checks for a closed cluster before fetching each item. Closing the cluster cancels the scan, which then reports range_scan_completed, so iteration ended early without an error. disconnect() is declared on ClusterInterface. The couchbase2 cluster throws UnsupportedOperationException, as its setAuthenticator() does. Results ------- After disconnect(), the next operation throws on the cluster, on every bucket and collection opened from it, and on every other cluster sharing the connection. Transactions and scans obtained before fail with cluster_closed. A cluster created afterwards with the same arguments bootstraps a new connection. A second disconnect() does nothing.

Copilot

🔵 Needs a closer look

The C++ core dependency is absent from the snapshot, leaving repeated disconnect, asynchronous transaction/scan teardown, and fork/shutdown safety unverified without integration testing against the pinned core.

2 open issues

3 resolved since last review

_{Review effort: Balanced}

avsej requested a balanced review from Copilot October 30, 2026 21:29

Copilot started reviewing on behalf of avsej October 30, 2026 21:29

View session