cloud_io: release client lease before retry backoff sleep#30177
Conversation
Acquire the client lease inside the retry loop and explicitly release it after shutdown, before sleeping for backoff. Previously most methods acquired the lease once before the loop and held it across all retries, starving the pool while sleeping. As a side effect, the retry permit check (which tests the caller's abort source) now happens before attempting to acquire a client from the pool.
There was a problem hiding this comment.
Pull request overview
Updates cloud_io::remote retry loops to avoid starving the cloud_storage_clients::client_pool by not holding a client lease across backoff sleeps, improving concurrency/fairness for other operations.
Changes:
- Move client lease acquisition inside retry loops for several remote operations (download/head/delete/list) to avoid holding leases while sleeping.
- Explicitly drop the lease after
shutdown()before executing retry backoff sleeps. - Remove now-unneeded
ssx:semaphore/ssx:watchdogbuild dependencies from thecloud_io:remoteBazel target.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/v/cloud_io/remote.cc |
Refactors retry loops to acquire/release client leases per-attempt and release before backoff sleep. |
src/v/cloud_io/BUILD |
Drops unused implementation deps from the remote library target. |
CI test resultstest results on build#83192
|
|
|
||
| lease.client->shutdown(); | ||
| { | ||
| auto _ = std::move(lease); |
There was a problem hiding this comment.
std::ignore = std::move(lease);?
|
/backport v26.1.x |
|
|
||
| lease.client->shutdown(); | ||
| { | ||
| auto _ = std::move(lease); |
|
Moving to std::ignore is noop and doesn’t achieve the goal.
…On Mon, 20 Apr 2026 at 20:10, Noah Watkins ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/v/cloud_io/remote.cc
<#30177?email_source=notifications&email_token=AAEETWN3YTF25E5AOM7NOH34WZYYZA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJUGI3TEMZWGMYKM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJL3QOJPXEZLWNFSXOX3DNRUWG2Y#discussion_r3113039374>
:
> @@ -1159,6 +1171,9 @@ ss::future<list_result> remote::list_objects(
}
lease.client->shutdown();
+ {
+ auto _ = std::move(lease);
use std:ignore =
—
Reply to this email directly, view it on GitHub
<#30177?email_source=notifications&email_token=AAEETWI4AUUA6ATLATUZOBL4WZYYZA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJUGI3TEMZWGMYKM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJPLQOJPXEZLWNFSXOX3ON52GSZTJMNQXI2LPNZZV6Y3MNFRWW#pullrequestreview-4142723630>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEETWL4Q545AFZATUGUMVL4WZYYZAVCNFSM6AAAAACX2T7PLKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DCNBSG4ZDGNRTGA>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
I don't think so. Pretty sure a valid use case for |
|
… On Wed, 22 Apr 2026 at 21:54, Noah Watkins ***@***.***> wrote:
*dotnwat* left a comment (redpanda-data/redpanda#30177)
<#30177?email_source=notifications&email_token=AAEETWPGDXFAFCKOIX5FYLD4XEWO3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRZHE4TAMZYHEYKM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4299903890>
Moving to std::ignore is noop and doesn’t achieve the goal.
I don't think so. Pretty sure a valid use case for std::ignore = is as a
replacement for casting to void, or assigning to _ or unused or any other
throwaway name, all of which don't evaluation of the expression, which IIUC
is all you're trying to achieve. Is there something else special about
std::ignore i'm missing?
—
Reply to this email directly, view it on GitHub
<#30177?email_source=notifications&email_token=AAEETWPGDXFAFCKOIX5FYLD4XEWO3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRZHE4TAMZYHEYKM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4299903890>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEETWM3KT5QDW32GYHGDTL4XEWO3AVCNFSM6AAAAACX2T7PLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEOJZHEYDGOBZGA>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Yeh, exactly you need to make sure that the statement does something. std::move is just a cast, so you need something like std::exchange. |
|
Not following anymore. What’s your proposal?
…On Thu, 23 Apr 2026 at 00:20, Noah Watkins ***@***.***> wrote:
*dotnwat* left a comment (redpanda-data/redpanda#30177)
<#30177?email_source=notifications&email_token=AAEETWNTIMOWD4I2YGQFPHL4XFHUFA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMZQGA2TOMBSG44KM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4300570278>
https://godbolt.org/z/xdxzecoYG
… <#m_-5804177704652785510_>
On Wed, 22 Apr 2026 at 21:54, Noah Watkins *@*.*> wrote: dotnwat left a
comment (redpanda-data/redpanda#30177
<#30177>) <#30177
<#30177>?email_source=notifications&email_token=AAEETWPGDXFAFCKOIX5FYLD4XEWO3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRZHE4TAMZYHEYKM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4299903890>
Moving to std::ignore is noop and doesn’t achieve the goal. I don't think
so. Pretty sure a valid use case for std::ignore = is as a replacement for
casting to void, or assigning to _ or unused or any other throwaway name,
all of which don't evaluation of the expression, which IIUC is all you're
trying to achieve. Is there something else special about std::ignore i'm
missing? — Reply to this email directly, view it on GitHub <#30177
<#30177>?email_source=notifications&email_token=AAEETWPGDXFAFCKOIX5FYLD4XEWO3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRZHE4TAMZYHEYKM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4299903890>,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAEETWM3KT5QDW32GYHGDTL4XEWO3AVCNFSM6AAAAACX2T7PLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEOJZHEYDGOBZGA
<https://github.com/notifications/unsubscribe-auth/AAEETWM3KT5QDW32GYHGDTL4XEWO3AVCNFSM6AAAAACX2T7PLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEOJZHEYDGOBZGA>
. You are receiving this because you modified the open/close state.Message
ID: @.*>
Yeh, exactly you need to make sure that the statement does something.
std::move is just a cast, so you need something like std::exchange.
—
Reply to this email directly, view it on GitHub
<#30177?email_source=notifications&email_token=AAEETWNTIMOWD4I2YGQFPHL4XFHUFA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMZQGA2TOMBSG44KM4TFMFZW63VMON2GC5DFL5RWQYLOM5S2KZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4300570278>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEETWN4RDX6X5KCJ3LRTRD4XFHUFAVCNFSM6AAAAACX2T7PLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGMBQGU3TAMRXHA>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Single underscore name as placeholder pretty sure isn't standard until c++26? |
Most methods in
cloud_io::remoteacquired a client lease from the poolbefore the retry loop and held it for the entire duration — including
across retry backoff sleeps. This starves the client pool: other
operations on the same shard block waiting for a client while the
retrying operation is just sleeping.
Move lease acquisition inside the retry loop for the six methods that had
it outside (
download_stream,download_object,object_exists,delete_object,delete_object_batch,list_objects), matching thepattern already used by
upload_object. Additionally, explicitly releasethe lease after
shutdown()and beforesleep_abortable()in all eightmethods so the client is returned to the pool during backoff.
As a side effect, the retry permit check (which tests the caller's abort
source) now happens before attempting to acquire a client from the pool.
Backports Required
Release Notes