Implement missing _delete() method of filesystem.Channel#2470
Implement missing _delete() method of filesystem.Channel#2470WojciechMigda wants to merge 19 commits intocelery:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements the missing filesystem transport Channel._delete() behavior so that deleting a queue also removes its binding from the exchange’s persisted routing table (control files), preventing bindings from leaking across test cases and runs.
Changes:
- Add
Channel._delete()to updatecontrol_folder/<exchange>.exchangewhen queues are deleted. - Expand filesystem transport unit tests to validate exchange-table updates after
Queue.delete(). - Isolate tests via per-test temporary
control_folderand explicit cleanup of temp folders/bindings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
kombu/transport/filesystem.py |
Adds _delete() to remove bindings from the persisted exchange routing table file. |
t/unit/transport/test_filesystem.py |
Adds assertions around binding-table contents and improves test isolation/cleanup for filesystem transport. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I'll take care of this. |
Duplicated temporary folder removal code was extracted to WithJanitorMixin class. Safe queue deletion was taken care of by creating managed_consumer function, decorated with contextlib.contextmanager.
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2470 +/- ##
==========================================
- Coverage 82.44% 82.42% -0.03%
==========================================
Files 79 79
Lines 10142 10167 +25
Branches 1165 1167 +2
==========================================
+ Hits 8362 8380 +18
- Misses 1579 1583 +4
- Partials 201 204 +3 ☔ View full report in Codecov by Sentry. |
auvipy
left a comment
auvipy
left a comment
There was a problem hiding this comment.
would you mind improving the test coverage, Please?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sure thing. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def setup_method(self): | ||
| self.channels = set() | ||
| self.channels: set[Channel] = set() | ||
| try: | ||
| data_folder_in = tempfile.mkdtemp() | ||
| data_folder_out = tempfile.mkdtemp() | ||
| self.data_folder_in = tempfile.mkdtemp() | ||
| self.data_folder_out = tempfile.mkdtemp() | ||
| self.control_folder = tempfile.mkdtemp() |
There was a problem hiding this comment.
Filesystem transport uses a class-level BrokerState (Transport.global_state) shared across tests. Since this setup now creates a fresh temporary control_folder each test and teardown removes it, stale bindings from prior tests can cause queue_bind to early-return (state.has_binding==True) and skip writing the exchange table into the new control_folder, breaking routing/hanging these tests. Consider clearing kombu.transport.filesystem.Transport.global_state in setup/teardown, or make exchange/queue names unique per test so bindings are re-declared for the new folders.
There was a problem hiding this comment.
@WojciechMigda would you mind cross checking this, please?
There was a problem hiding this comment.
There was an issue of shared global mutable state leaking between existing tests and part of the changes I introduced aimed at resolving this. Namely, exchange file contents from one test was not being torn down and subsequent test not only saw that but its code was written around it.
In my experience, the way to approach risks involved with shared mutable global state in tests is to put sufficient safeguards into proper test cleanup and not add more global mutable state (shared control folder). Here, as I understand it, as long as we clean up all queues associated with consumers (managed_consumer) we are safe in that respect.
There was a problem hiding this comment.
To satisfy LLM we could insert assertions which check that there are no stale bindings left when the test is completed. I am not sure though, if there is a public, or otherwise accessible API allowing us to do that.
As of now, Channel class of filesystem transport does not have a _delete method. It prevents queues from being removed from an associated exchange. This PR intents to remedy that.