Skip to content

Commit

Permalink
fix(subscriber): correct retain logic (#447)
Browse files Browse the repository at this point in the history
The current logic present in `IdData::drop_closed` marks an item (task,
resource, and async op stats) to be dropped in the case that the item
**is** dirty and there **are** watchers: `(dirty && has_watchers)`.

This causes a case where if an item is first received and then completes
in between the aggregator push cycle, it will be discarded immediately
and never sent.

This logic has been in place since the concepts of watchers and dirty
items was introduced in #77. However since an item that is created and
then dropped within a single update cycle isn't likely to be missed in
the UI, it may never have been noticed.

Instead the logic should be to **retain** an item if **any** of the
following is true:
* there are watchers and the item is dirty: `(dirty && has_watchers)`
* item has been dropped less time than the retention period:
  `dropped_for <= retention`.
  • Loading branch information
hds authored and hawkw committed Sep 29, 2023
1 parent 132ed4e commit 36ffc51
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions console-subscriber/src/aggregator/id_data.rs
Expand Up @@ -104,18 +104,18 @@ impl<T: Unsent> IdData<T> {
if let Some(dropped_at) = stats.dropped_at() {
let dropped_for = now.checked_duration_since(dropped_at).unwrap_or_default();
let dirty = stats.is_unsent();
let should_drop =
let should_retain =
// if there are any clients watching, retain all dirty tasks regardless of age
(dirty && has_watchers)
|| dropped_for > retention;
|| dropped_for <= retention;
tracing::trace!(
stats.id = ?id,
stats.dropped_at = ?dropped_at,
stats.dropped_for = ?dropped_for,
stats.dirty = dirty,
should_drop,
should_retain,
);
return !should_drop;
return should_retain;
}

true
Expand Down

0 comments on commit 36ffc51

Please sign in to comment.