Fix dispatch dict passed to MergeStrategy not getting updated for future fetch's

[Marenz]

• We are also switching to use the same now while evaluating other dispatches in the MergeStrategy, but this is just an enhancement and not a fix
• We still used logging without "path" in some places, that was fixed

[llucax]

As usual, the PR and commit title and description could use some improvement. It is hard to review when they say so little. Could you please please please put a bit more thought into those? Please 🙏 ? For the PR description just copying the copilot review summary is a big improvement.

For example in this PR it is mostly irrelevant that you are using started_at, that tells me almost nothing. What is important is you are fixing a bug caused by not having a consistent now used in different path of the code to decide on how to merge dispatches.

[Marenz]

commit title and description

tbh, I disagree. The commit titles and descriptions are all perfectly fine imo.

PR, yeah, okay, that grew a bit and needed updating.

I also don't think a required dependency update necessarily needs to be mentioned if it is so minor as here, and the fact that its part of that particular commit is in itself a documentation of its need, at least that was my perspective....

I don't really see how any of the other commit titles & content is lacking anything to make them understandable.

[Marenz]

• Updated to print all other active dispatch ids
• Updated to mention the need of the version update in the according PR description.
• Updated another commit description to be even more clear

[llucax]

Without wanting to make a huge thing about this, I will just comment on this one, as I think it can bring some clarity:

I also don't think a required dependency update necessarily needs to be mentioned if it is so minor as here, and the fact that its part of that particular commit is in itself a documentation of its need, at least that was my perspective....

I've seen many times people that sneak an unrelated change into a commit just because it was there. And I don't even think that's obscenely wrong, I can live with that when there is intention. The problem is when it was a mistake, and you were working on something else that needed the bump, but this PR doesn't really need it, and that dependency bump can have real world impact.

So what it is important for me is declaring intention. If the bump was needed by the commit, that's good to know, and good to know why, even if it could be inferred from the diff, just as an extra verification. If the bump was just done because one is lazy about putting it in a separate commit, and the bump is harmless, that should also be explicitly stated, so I can actually do a review, which is about detecting issues early.

This is my main point. I want to be able to tell between mistakes and intended changes.

[llucax]

LGTM, but one thing to check and one small suggestion.

[Copilot]

Pull Request Overview

This pull request fixes an issue where the dispatch dictionary passed to MergeStrategy was not getting updated for future fetches, causing the merge filter to work with stale data. The fix ensures the dispatch cache is properly maintained and updated during fetch operations.

Key changes:

• Fixed cache update logic in _fetch() to properly update the _dispatches dictionary with new data
• Enhanced consistency by using the same now timestamp when evaluating multiple dispatches in the merge strategy
• Improved logging throughout the codebase to use proper logger paths and provide better debugging information

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/frequenz/dispatch/_bg_service.py	Fixed dispatch cache update logic and improved error handling for deleted dispatches
src/frequenz/dispatch/_merge_strategies.py	Updated to use consistent timestamp and proper logger path
src/frequenz/dispatch/_dispatch.py	Added started_at() method to accept external timestamp for consistency
src/frequenz/dispatch/_actor_dispatcher.py	Improved logging messages for actor lifecycle events
pyproject.toml	Updated dependency version for frequenz-client-dispatch
RELEASE_NOTES.md	Updated release notes to reflect bug fixes

Comments suppressed due to low confidence (2)

src/frequenz/dispatch/_merge_strategies.py:61

• Variable name collision: dispatch is used both as the parameter to the filter method and as the iteration variable in this generator expression. This shadows the outer dispatch variable and could cause confusion. Consider renaming the iteration variable to running_dispatch.

            dispatch.started_at(now) for dispatch in running_dispatch_list

src/frequenz/dispatch/_merge_strategies.py:74

• Variable name collision: dispatch is used both as the parameter to the filter method and as the iteration variable in this generator expression. This shadows the outer dispatch variable and could cause confusion. Consider renaming the iteration variable to running_dispatch.

                list(dispatch.id for dispatch in running_dispatch_list),

[llucax]

LGTM, just one more suggestion, removing auto-merge in case you want to change it.

[llucax]

Suggested change

old_dispatches = set(self._dispatches.keys())

[llucax]

It is usually recommended to narrow the try/except to only the code you expect to fail, otherwise you could catch other failures that maybe are not OK to ignore. For example if some code in await self._lifecycle_events_tx.send(Deleted(dispatch=dispatch)) (suppose send() uses some internal cache and there is a bug that also make it raise a KeyError).

Also if old_dispatches == self._dispatches.keys(), maybe it makes more sense to use self._dispatches.keys() directly now, I see in the new code old_dispatches is only being used here. This would make the code more obvious and actually guarantee the pop() can't fail, old_dispatches and self._dispatches can't get out of sync.

So I suggest removing old_dispatches and just do:

Suggested change

	for dispatch_id in old_dispatches - new_dispatches.keys():
	try:
	dispatch = self._dispatches.pop(dispatch_id)
	_logger.debug("Deleted dispatch: %s", dispatch)
	await self._update_dispatch_schedule_and_notify(None, dispatch, timer)
	# Set deleted only here as it influences the result of dispatch.started
	# which is used in the func call above ^
	dispatch._set_deleted() # pylint: disable=protected-access
	await self._lifecycle_events_tx.send(Deleted(dispatch=dispatch))
	except KeyError:
	_logger.warning(
	"Inconsistency in cache detected. Tried to delete non-existing dispatch %s",
	dispatch_id,
	)
	# We make a copy because we mutate self._dispatch.keys() inside the loop
	for dispatch_id in frozenset(self._dispatches.keys() - new_dispatches.keys()):
	dispatch = self._dispatches.pop(dispatch_id)
	_logger.debug("Deleted dispatch: %s", dispatch)
	await self._update_dispatch_schedule_and_notify(None, dispatch, timer)
	# Set deleted only here as it influences the result of dispatch.started
	# which is used in the func call above ^
	dispatch._set_deleted() # pylint: disable=protected-access
	await self._lifecycle_events_tx.send(Deleted(dispatch=dispatch))

[llucax]

Not sure if the copy is necessary, as we are using the - which I guess produces a copy in itself, but I'm not 100% sure. Also making it a frozenset guarantees our items being iterated will never change, so it makes everything more explicit.