Skip to content

CBG-4928: Update log messages on changes feed being cancelled #7880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
torcolvin merged 5 commits into from
Nov 21, 2025
Merged

CBG-4928: Update log messages on changes feed being cancelled #7880

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0d33616
CBG-4928 do not log warning for closed blip sender
torcolvin Nov 13, 2025
dd83197
update log messages
RIT3shSapata Nov 14, 2025
ae24539
fixes based on PR comments
RIT3shSapata Nov 19, 2025
51e442e
Merge branch 'CBG-4928-silence-close-err' into CBG-4928
RIT3shSapata Nov 19, 2025
b028e25
fixes based on pr comments
RIT3shSapata Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion base/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ var (
ErrNotFound = &sgError{"Not Found"}
ErrUpdateCancel = &sgError{"Cancel update"}
ErrImportCancelledPurged = HTTPErrorf(http.StatusNotFound, "Import Cancelled Due to Purge")
ErrChannelFeed = &sgError{"Error while building channel feed"}
ErrChannelFeed = &sgError{"Failed to build channel feed"}
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The error message 'Failed to build channel feed' still implies a failure rather than a cancellation, which contradicts the PR's goal of treating these as non-error scenarios. Consider renaming to 'Channel feed cancelled' or similar to better reflect the intended semantics.

Suggested change
ErrChannelFeed = &sgError{"Failed to build channel feed"}
ErrChannelFeed = &sgError{"Channel feed cancelled"}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@torcolvin torcolvin Nov 14, 2025

Choose a reason for hiding this comment

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

I think we can drop this, because I think this error wouldn't show up if we avoid logging it at all.

Copy link
Contributor Author

@RIT3shSapata RIT3shSapata Nov 18, 2025

Choose a reason for hiding this comment

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

I think we cannot remove this, as its being used in multiple other places

Copy link
Collaborator

@torcolvin torcolvin Nov 20, 2025

Choose a reason for hiding this comment

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

I meant, we don't need to change the message. I think "Failure" and "Error" are roughly equivalent in severity and I think there's value to leaving the messages the same to track across releases for supportability.

It is worth changing a message if it downgrades it from an issue that customers/support has to look at, or provides additional clarity.

ErrTimeout = &sgError{"Operation timed out"}
ErrPathNotFound = sgbucket.ErrPathNotFound
ErrPathExists = sgbucket.ErrPathExists
Expand Down
4 changes: 2 additions & 2 deletions db/changes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ func (db *DatabaseCollectionWithUser) changesFeed(ctx context.Context, singleCha
base.TracefCtx(ctx, base.KeyChanges, "Querying channel %q with options: %+v", base.UD(singleChannelCache.ChannelID().Name), paginationOptions)
changes, err := singleChannelCache.GetChanges(ctx, paginationOptions)
if err != nil {
base.WarnfCtx(ctx, "Error retrieving changes for channel %q: %v", base.UD(singleChannelCache.ChannelID().Name), err)
base.InfofCtx(ctx, base.KeyChanges, "Could not retrieve changes for channel %q: %v", base.UD(singleChannelCache.ChannelID().Name), err)
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The error message 'Could not retrieve changes' is vague and doesn't clearly indicate that the changes feed was cancelled. Consider using a more specific message like 'Changes feed cancelled for channel %q: %v' to better reflect the actual cause.

Suggested change
base.InfofCtx(ctx, base.KeyChanges, "Could not retrieve changes for channel %q: %v", base.UD(singleChannelCache.ChannelID().Name), err)
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
base.InfofCtx(ctx, base.KeyChanges, "Changes feed cancelled for channel %q: %v", base.UD(singleChannelCache.ChannelID().Name), err)
} else {
base.InfofCtx(ctx, base.KeyChanges, "Could not retrieve changes for channel %q: %v", base.UD(singleChannelCache.ChannelID().Name), err)
}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@torcolvin torcolvin Nov 14, 2025

Choose a reason for hiding this comment

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

Assuming the context cancellation is passed back in the error message:

								if !errors.Is(err, context.Canceled) && errors.Is(err, context.DeadlineExceeded) {
									base.WarnfCtx(ctx, "Error retrieving changes for channel %q: %v", base.UD(singleChannelCache.ChannelID().Name), err)
								}

However, I think to match the behavior of below if we don't always wrap the context error message:

Suggested change
base.InfofCtx(ctx, base.KeyChanges, "Could not retrieve changes for channel %q: %v", base.UD(singleChannelCache.ChannelID().Name), err)
if options.ChangesCtx.Err() != nil {
base.WarnfCtx(ctx, "Error retrieving changes for channel %q: %v", base.UD(singleChannelCache.ChannelID().Name), err)
}

Copy link
Contributor Author

@RIT3shSapata RIT3shSapata Nov 19, 2025

Choose a reason for hiding this comment

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

A few tests were not being passed because the Querytimeout would just throw an error and no context will be cancelled

change := ChangeEntry{
Err: base.ErrChannelFeed,
}
Expand Down Expand Up @@ -996,7 +996,7 @@ func (col *DatabaseCollectionWithUser) SimpleMultiChangesFeed(ctx context.Contex
} else {
// On feed error, send the error and exit changes processing
if current[i].Err == base.ErrChannelFeed {
base.WarnfCtx(ctx, "MultiChangesFeed got error reading changes feed: %v", current[i].Err)
Comment on lines 998 to -999
Copy link
Collaborator

@torcolvin torcolvin Nov 14, 2025

Choose a reason for hiding this comment

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

Suggested change
if current[i].Err == base.ErrChannelFeed {
base.WarnfCtx(ctx, "MultiChangesFeed got error reading changes feed: %v", current[i].Err)
if !options.ChangesCtx.Err() {
base.WarnfCtx(ctx, "MultiChangesFeed got error reading changes feed: %v", current[i].Err)
}

In this case, it's not clear whether that err from current[i].Err is better than the error returned.

base.InfofCtx(ctx, base.KeyChanges, "Could not read changes feed: %v", current[i].Err)
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The error message 'Could not read changes feed' is ambiguous and doesn't clearly indicate a cancellation scenario. Consider using 'Changes feed cancelled: %v' or 'Changes feed operation cancelled: %v' for clarity.

Suggested change
base.InfofCtx(ctx, base.KeyChanges, "Could not read changes feed: %v", current[i].Err)
base.InfofCtx(ctx, base.KeyChanges, "Changes feed cancelled: %v", current[i].Err)

Copilot uses AI. Check for mistakes.
select {
case <-options.ChangesCtx.Done():
case output <- current[i]:
Expand Down
2 changes: 1 addition & 1 deletion db/channel_cache_single.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ func (c *singleChannelCacheImpl) GetChanges(ctx context.Context, options Changes
// Check whether the changes process has been terminated while we waited for the view lock, to avoid the view
// overhead in that case (and prevent feedback loop on query backlog)
Copy link
Collaborator

@torcolvin torcolvin Nov 14, 2025

Choose a reason for hiding this comment

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

Suggested change
// Check whether the changes process has been terminated while we waited for the view lock, to avoid the view
// overhead in that case (and prevent feedback loop on query backlog)
// Check whether the changes process has been terminated before running a query

if options.ChangesCtx.Err() != nil {
return nil, fmt.Errorf("Changes feed cancelled while waiting for view lock")
return nil, fmt.Errorf("Changes feed cancelled")
Copy link
Collaborator

@torcolvin torcolvin Nov 14, 2025

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("Changes feed cancelled")
return nil, fmt.Errorf("Changes feed cancelled %w", options.ChangesCtx.Err())

This would tell us in downstream while it was canceled, particularly if we are able to determine a cause, this will make the above easier to reason about I think.

}

// Now query the view. We set the max sequence equal to cacheValidFrom, so we'll get one
Expand Down
Loading