Skip to content

Release driver-side references in PublisherModel before publishing - #261

Merged
timkpaine merged 1 commit into
mainfrom
fix/publisher-model-release-references
Aug 24, 2026
Merged

Release driver-side references in PublisherModel before publishing#261
timkpaine merged 1 commit into
mainfrom
fix/publisher-model-release-references

Conversation

@ptomecek

Copy link
Copy Markdown
Collaborator

Summary

PublisherModel.__call__ held live references to the source result (data) and the extracted pub_data for the entire duration of the publisher's execution. When a publisher writes a large frame to a sink, it can build a sole-reference ownership carrier internally so the frame is freed before the terminal write drains — a peak-memory contract. But the outer method retaining data/pub_data defeated that ownership transfer from the outside, so the frame's refcount never dropped to zero during publisher(). Peak memory was roughly twice the frame size during the terminal write for any return_data=False sink-write publisher.

Fix

When return_data=False, the driver-side references are deleted before invoking the publisher, so the publisher can own the sole reference and free the frame during the write:

publisher.data = pub_data
if self.return_data:
    out = publisher()
    return data
del data, pub_data
out = publisher()
return self.result_type(value=out)

When return_data=True, references are retained because the source result must be returned. Behavior is otherwise unchanged.

Testing

Added regression tests using weakref + gc that verify the frame is collected during publisher() when return_data=False, and retained when return_data=True. The new return_data=False test was confirmed to fail on the pre-fix source. Full ccflow/tests/models suite passes (81 tests).

PublisherModel.__call__ held live references to the source result (data)
and the extracted pub_data for the entire duration of the publisher's
execution. When a publisher writes a large frame to a sink, it can build
a sole-reference ownership carrier internally so the frame is freed before
the terminal write drains. The outer method retaining data/pub_data
defeated that transfer, leaving peak memory at roughly twice the frame
size during writes for return_data=False sink-write publishers.

When return_data=False, delete the driver-side references before invoking
the publisher so it can own the sole reference and free the frame during
the write. When return_data=True the references are retained, since the
source result must be returned.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Pascal Tomecek <pascal.tomecek@cubistsystematic.com>
@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Test Results

    1 files  ±0      1 suites  ±0   3m 8s ⏱️ +8s
1 330 tests +2  1 328 ✅ +2  2 💤 ±0  0 ❌ ±0 
1 336 runs  +2  1 334 ✅ +2  2 💤 ±0  0 ❌ ±0 

Results for commit a8b125d. ± Comparison against base commit 6a788a6.

♻️ This comment has been updated with latest results.

@codecov

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.51%. Comparing base (6a788a6) to head (a8b125d).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
+ Coverage   93.48%   93.51%   +0.03%     
==========================================
  Files         176      176              
  Lines       20327    20361      +34     
  Branches     1350     1350              
==========================================
+ Hits        19002    19040      +38     
+ Misses       1052     1050       -2     
+ Partials      273      271       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ptomecek
ptomecek marked this pull request as ready for review August 24, 2026 12:12
@timkpaine
timkpaine merged commit d8f632d into main Aug 24, 2026
20 checks passed
@timkpaine
timkpaine deleted the fix/publisher-model-release-references branch August 24, 2026 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants