googleapis / googleapis/google-cloud-cpp

Pub/Sub SubscriptionLeaseManagement self-deadlock: StartRefreshTimer .then() continuation runs inline under mu_ (bigtable #4327 fix never ported)

Abierto
#16,248 1 comentario 0 reacciones 1 asignado Reclamado por @scotthart Ver en GitHub
Lenguaje dominante
C++
Estrellas
659
Forks
462
Merge medio
1 d 2 h
PR fusionados (30 d)
89

Descripción

## Summary

A Pub/Sub streaming-pull subscriber can permanently **self-deadlock** inside `SubscriptionLeaseManagement`. In `google/cloud/pubsub/internal/subscription_lease_management.cc`, `StartRefreshTimer()`'s `.then()` continuation can run **synchronously / inline** when the timer future is already satisfied (the deadline has already passed, or a completion-queue thread wins the satisfaction race) — while `mu_` is still held on the calling stack. The continuation (`OnRefreshTimer` → `RefreshMessageLeases`) then re-acquires the same **non-recursive** `std::mutex mu_` → self-deadlock. The subscription freezes at the lease-management mutex until the process restarts.

## Observed failure (thread state at the deadlock)

- A thread is executing `OnRefreshTimer` **holding** the `SubscriptionLeaseManagement` mutex. It calls `StartRefreshTimer`, which sets up a `.then()` future callback. That future fires **inline** (while the lock is still live on the stack), re-enters `OnRefreshTimer`, and tries to re-acquire the same non-recursive `std::mutex` → it deadlocks itself.
- The ack thread and the `OnRead` thread are both blocked behind that mutex. The TCP connection is alive (`OnRead` has a live message in hand) and the callback thread is running, yet the **entire subscription is frozen** at the lease-management mutex.
- It is **per-subscription** — sibling subscriptions on the same client keep running — and there is **no self-recovery**. Low-traffic / quiet subscriptions are the most exposed (the timer is more likely to be already satisfied when re-armed).

## Affected versions

`subscription_lease_management.cc` is **byte-identical from v2.28.0 through current `main`** (verified by direct diff). Its commit history shows it was **last touched in March 2024**, and no commit has ever addressed the inline-continuation / mutex re-entrancy. The latest release, **v3.6.0 (2026-06-18)**, and all 2026 release notes contain **no** Pub/Sub subscriber / lease / timer fix. So upgrading does not resolve it.

## Precedent — the identical bug was fixed in Bigtable but never ported here

The same re-entrancy class was reported for `bigtable::MutationBatcher` in **#4083** and fixed by **#4327**: `AsyncBulkApply`'s `.then()` continuation could execute synchronously/inline when the operation completed quickly while the `MutationBatcher` mutex was held; the continuation (`OnBulkApplyDone`) re-acquired the same non-recursive mutex → self-deadlock. The fix (quoting the PR) *"schedules the continuation on the completion queue explicitly"* — i.e. the `.then()` body just calls `RunAsync()` so the real work never runs inline under the lock.

That fix was **never ported to Pub/Sub's `SubscriptionLeaseManagement`**, which has the same inline-`.then()`-under-lock shape.

## Proposed fix

Apply the same pattern as #4327: dispatch the refresh-timer continuation via `cq.RunAsync(...)` so `OnRefreshTimer` always runs on a completion-queue thread with **no locks held**, never inline under `mu_`.

We have been running exactly this as a local patch to the v2.28.0 recipe and it resolves the deadlock. Happy to open a PR (under the CLA) if that's welcome.

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.