Retry or cancel a non-latest pipeline from the list doesn't expire ETag cache

---

Updated: Looks like this only happens on non-latest pipeline?

---

Summary

Retry or cancel a pipeline from the list doesn't expire ETag cache. It would show the original status for a long while, but if we click on the pipeline directly, or use a private tab to check the list, we could see the updated status.

Steps to reproduce

0. Find a merge request having failed or running pipelines
1. Check the pipeline lists for that merge request
2. Click on retry or cancel button for one of the pipeline
3. Wait for the pipeline to update its status
4. Check the pipeline status by clicking on pipeline directly; Or check the list by a private tab

What is the current bug behavior?

Pipeline list is cached via ETag which was not properly expired after the pipeline status changed

What is the expected correct behavior?

Checking the pipeline page directly, or checking with a private tab, should show the same status for the pipeline.

Relevant logs and/or screenshots

I'll attach one when I found another merge request for this.

/cc @to1ne @adamniedzielski

mentioned in issue #30864 (closed)

Interestingly this only happens on non-latest pipelines?

I confirmed that this only happens on non-latest pipeline.

changed title from Retry or cancel a pipeline from the list doesn't expire ETag cache to Retry or cancel a non-latest pipeline from the list doesn't expire ETag cache

Logged in list:

Private tab list:

assigned to @to1ne

@godfat You are aware polling is paused when a browser tab page is no longer visible? (if I'm not mistaken) cc @filipa

@to1ne I am half aware of that (could feel that, but not completely sure). But I think it's irrelevant about polling because refreshing this several times can't fix this at all. I would try complete refresh next time. (cmd+shift+r) I assume a complete refresh could fix it because it would try to remove ETag cache from browsers.

@godfat OK, I've found it. It seems Ci::Pipeline#merge_requests only returns the Merge Requests for which the pipeline is the latest commit.

    # Merge requests for which the current pipeline is running against
    # the merge request's latest commit.
    def merge_requests
      @merge_requests ||= project.merge_requests
        .where(source_branch: self.ref)
        .select { |merge_request| merge_request.head_pipeline.try(:id) == self.id }
    end

So, before I start changing anything. I guess this implementation exists for some reason? After we clear this out, I'll work on a fix.

@to1ne It's mostly due to performance reason. If it's trying to return all merge requests, it would be very slow. We probably can't change this without checking the performance implication. I am not sure if we could really fix this then. I am not familiar with current ETag implementation. Is it possible to fix this issue without touching Ci::Pipeline#merge_requests?

Expected behaviour is: when we receive a successful response from the retry or cancel endpoint, we call pipelines endpoint again to refresh data.

When we introduced polling we add a verification that we would only fetch data if we were not currently doing a request already (polling).

Should we change this? cc @ayufan

@filipa I think this is completely irrelevant to polling, because even a refresh can't fix this. The problem is that there's new data, but the browser would think that's there's no new data because ETag is not properly invalidated.

I think this is a purely backend issue.

@godfat At the moment the Ci::ExpirePipelineCacheService is used to invalidate the Etag cache. If we would use a worker instead, we might do some long-taking queries. Using a worker might be a good idea after all, because I assume the current query in Ci::Pipeline#merge_requests isn't really fast too.

But then there is still the question what impact the long-running query (to get all MRs for the pipeline) would have on the load of the server.

@to1ne Oh, I think I understand the issue now. Then I think this is also blocked by the old friend which we don't have a relation from pipeline to merge request.

Oh why haven't we already done this? This is blocking so many issues.

Thanks for explaining @godfat :)

I am really so bad at finding issues. I do think we have an issue somewhere talking about adding the relation..... or was that only my idea, which had never being an issue?

@to1ne @godfat I'm not sure, I would have to take a look how it behaves. I think that we expect to receive only one merge request.

I think that it should be fine to run that to all merge requests.

So we are looking at something like that:

@merge_requests ||= project.merge_requests.where(source_branch: self.ref)

I found the issue #23708 (closed), it's closed in favour of #23902 (moved).

@ayufan Before changing that, we need to think of the implication of https://gitlab.com/gitlab-org/gitlab-ce/issues/27687#note_22861234

Changing it might be either making it very slow, or very inaccurate.

Or, we could start introducing Pipeline#all_merge_requests....

Short term wise, I think we could add Pipeline#all_merge_requests (name to be decided), and use it in ExpirePipelineCacheService.

This would only impact people re-using the branch name over and over, which I am not sure if it's common.

mentioned in merge request !10770 (merged)

@godfat & @ayufan I've created !10770 (merged). It does what it supposed to do.

added Next Patch Release ~12980 labels

mentioned in issue #31127 (closed)

@godfat This would be targeted change. We don't want to change everything.

closed via merge request !10770 (merged)

mentioned in commit d35515cb

mentioned in commit 24020e69