Skip to content

Fix try_shrink not freeing back to pool#20382

Merged
mbutrovich merged 1 commit intoapache:mainfrom
pydantic:fix_try_shrink
Feb 17, 2026
Merged

Fix try_shrink not freeing back to pool#20382
mbutrovich merged 1 commit intoapache:mainfrom
pydantic:fix_try_shrink

Conversation

@cetra3
Copy link
Contributor

@cetra3 cetra3 commented Feb 16, 2026

Which issue does this PR close?

No issue raised

Rationale for this change

try_shrink never released any memory from the pool, and so would cause a "memory leak"

What changes are included in this PR?

A few changes to MemoryReservation

Are these changes tested?

Yes & new test has been added

Are there any user-facing changes?

Nope

@github-actions github-actions bot added the execution Related to the execution crate label Feb 16, 2026
@comphead
Copy link
Contributor

@andygrove FYI

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Thanks @cetra3 that looks like a really nice catch, I checked the test currently failing on main for other mem pools as well.

@mbutrovich mbutrovich requested review from mbutrovich and removed request for mbutrovich February 17, 2026 14:24
@mbutrovich mbutrovich added this pull request to the merge queue Feb 17, 2026
Merged via the queue into apache:main with commit 6798dff Feb 17, 2026
28 checks passed
@hareshkh
Copy link
Contributor

hareshkh commented Feb 17, 2026

@cetra3 @mbutrovich @comphead : Any chance we could get this in a patch to branch-51? I can run the backports but would be interested to have this fix there. (I can backport to branch-52 as well)

@comphead
Copy link
Contributor

@cetra3 @mbutrovich @comphead : Any chance we could get this in a patch to branch-51? I can run the backports but would be interested to have this fix there. (I can backport to branch-52 as well)

We should prob wait for DF 53 release which should be pretty soon
#19692

@mbutrovich
Copy link
Contributor

@cetra3 @mbutrovich @comphead : Any chance we could get this in a patch to branch-51? I can run the backports but would be interested to have this fix there. (I can backport to branch-52 as well)

I would be interested in a backport to DF52. I will bring it here: #20287. If you open the backport PR for 52, could you add it to that tracking issue? I am not sure if we are planning another 51 patch release.

@hareshkh
Copy link
Contributor

@mbutrovich @comphead : Sorry, my bad here. This PR only fixes a bug introduced by #19759 so branch-51 and branch-52 (which use usize instead of AtomicUsize) do not have the bug.
In branch-52, try_shrink:

pub fn try_shrink(&mut self, capacity: usize) -> Result<usize> {
    if let Some(new_size) = self.size.checked_sub(capacity) {
        self.registration.pool.shrink(self, capacity);
        self.size = new_size;
        Ok(new_size)
    } else {
        internal_err!(
            "Cannot free the capacity {capacity} out of allocated size {}",
            self.size
        )
    }
}

Which already calls pool.shrink() correctly.

I was debugging another issue I am running into and I thought this would be the fix for branch-51/branch-52 as well but that is not the case :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

execution Related to the execution crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments