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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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.

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)
}

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)
Comment on lines 413 to 414
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