ETag caching probably broken since 11.5.0

It seems ETag caching don't work anymore since (11.5.0) https://gitlab.com/gitlab-org/gitlab-ce/commit/782badd0a2cd00d2a9cbe591e78b30aca32e252b (introduced in https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2555/diffs to mitigate https://gitlab.com/gitlab-org/gitlab-ce/issues/51423).

This commits adds no-store to the response thus the client cannot send the If-None-Match request header.

If I remove no-store, the 304 are back:

I'm wondering if we should only set no-store for non-XHR requests? I will create an issue.

This would have prevented this regression from happening: https://gitlab.com/gitlab-org/gitlab-ce/issues/49321

/cc @jameslopez

added Create [DEPRECATED] bugperformance devopscreate typebug labels

Yes, I think as the problem was for the back button, having no-store isn't necessary for XHRs.

changed the description

assigned to @rymai

changed milestone to %11.9

created merge request !25400 (merged) to address this issue

mentioned in merge request !25400 (merged)

@dosuken123 I found an interesting case/bug while trying to fix this issue in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25400:

1. In spec/features/merge_request/user_sees_merge_request_pipelines_spec.rb:277 we're looking at a merge request's pipelines when the source project is a fork
2. Currently, since Etag caching is disabled, the test passes
3. With https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25400, we're removing the no-store directive in the Cache-Control header
4. The test now fails since the list of pipelines isn't up-to-date (only 3 pipelines are shown instead of 4) since we're getting 304 responses with stale data
5. I've started investigating and the etag should be updated in ExpirePipelineCacheWorker#each_pipelines_merge_request_path
6. The problem is that pipeline.all_merge_requests returns [] in this method
7. I think this is caused by Pipeline#all_merge_requests not returning merge requests which have a target project different from the pipeline's project (i.e. merge requests from a fork). Btw this case isn't unit tested.
8. In my case the fork pipeline has "project_id"=>2 and "merge_request_id"=>1 but the query to find all the pipeline's merge requests is SELECT "merge_requests".* FROM "merge_requests" WHERE "merge_requests"."target_project_id" = $1 AND "merge_requests"."id" = $2 [["target_project_id", 2], ["id", 1]]
9. This query returns no results, but SELECT "merge_requests".* FROM "merge_requests" WHERE "merge_requests"."source_project_id" = $1 AND "merge_requests"."id" = $2 [["source_project_id", 2], ["id", 1]] would return the correct results in this particular case
10. It looks like the "opposite" method (i.e. MergeRequest#all_pipelines) does return the correct pipelines since it looks in the source project, i.e. source_project.ci_pipelines.for_merge_request(self, source_branch, all_commit_shas)

I tried fixing it with

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index f0ae516a2f8..8921825ae19 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -650,9 +650,9 @@ module Ci
     def all_merge_requests
       @all_merge_requests ||=
         if merge_request?
-          project.merge_requests.where(id: merge_request_id)
+          MergeRequest.where(id: merge_request_id)
         else
-          project.merge_requests.where(source_branch: ref)
+          MergeRequest.where(target_project: [project, project.default_merge_request_target], source_branch: ref)
         end
     end

but the test still fails even though MergeRequest#all_pipelines now returns the correct pipeline and the etag value is updated...

I will get back to it later.

@rymai I think you're right. The fix makes sense to me. MergeRequest.where(target_project: project, source_branch: ref) would be enough as pipelines will be created in forked projects only. My concern is that if there are two forked projects created two merge requests with the same named ref, then this pipeline could return unrelated MRs. This would be a problem we have to resolve aside from the Etag problem.

If you need more hands for debuging the spec, please ping me in the issue.

Thank you for investigating this

@rymai Thanks for the detailed comments. I was able to pick up where you left off to mitigate https://gitlab.com/gitlab-org/gitlab-ce/issues/57487#note_143336895.

Using your patch above and adding the diff below, I see the paths for project group1/project1 invalidated, but the forked project URL /namespace1/project1/merge_requests/1/pipelines.json never gets invalidated:

executing create pipeline here for push, group1/project1
=== updating etag cache for pipeline 1
touching key /group1/project1/pipelines.json, etag is a44bd1b1a0878526215298b69123399b
touching key /group1/project1/pipelines/1.json, etag is 53825ed69dd68403c89330df5472a161
touching key /group1/project1/commit/0b4bc9a49b562e85de7cc9e834518ea6828729b9/pipelines.json, etag is 4a5688a5e765fc0f4bfa1f3ac1cbf529
touching key /group1/project1/merge_requests/new.json, etag is f5bb043a88d58ef14ec1779cf0884ff8
pipeline ID 1: MRs: #<ActiveRecord::Relation []>, 0
touching key /group1/project1/pipelines/1.json, etag is ce13db258a4843433ab2941d12600fe3
touching key /group1/project1/builds/2.json, etag is 5c7c4704efa54022a2ab6fd6d7b23306
touching key /group1/project1/pipelines/1.json, etag is 5a9c3279f9c0712d57a72001df40d06f
touching key /group1/project1/builds/1.json, etag is d7f056b8881f6161de5edd07eeef3ae4
executing create pipeline here for merge_request, group1/project1
=== updating etag cache for pipeline 2
touching key /group1/project1/pipelines.json, etag is 8fd01805f80691870c206a920d79a6a0
touching key /group1/project1/pipelines/2.json, etag is 9eab23c217e12fff5be9a0ea67fd7c4f
touching key /group1/project1/commit/0b4bc9a49b562e85de7cc9e834518ea6828729b9/pipelines.json, etag is 9e72c2b77f1bd6925af4628bc0e7fcc2
touching key /group1/project1/merge_requests/new.json, etag is 0727723879090deca3453496da3866ae
pipeline ID 2: MRs: #<ActiveRecord::Relation [#<MergeRequest id:1 namespace1/project1!1>]>, 1
=== touching path /group1/project1/merge_requests/1/pipelines.json
touching key /group1/project1/merge_requests/1/pipelines.json, etag is 91e9a00e9c068ae57eda73facffdb806
touching key /group1/project1/pipelines/2.json, etag is 0befb9b8105b9a9b7e92a096621201d7
touching key /group1/project1/builds/3.json, etag is e249ad2f9feb6fced216a67f4585135f
executing create pipeline here for push, group1/project1
getting key /namespace1/project1/merge_requests/1/pipelines.json => 445b98579984d75c57b1d09777262177
=== updating etag cache for pipeline 3
touching key /group1/project1/pipelines.json, etag is d2085f8527ffac211c71b0ee2bf4570e
touching key /group1/project1/pipelines/3.json, etag is da9e14268cca5cb3f17c3e9f3607d9a5
touching key /group1/project1/commit/0b4bc9a49b562e85de7cc9e834518ea6828729b9/pipelines.json, etag is ecda7b24489585cfcf10b03bcb9aad96
touching key /group1/project1/merge_requests/new.json, etag is 244be3793dff6a6f3c5a3ab4ec1d7e2b
pipeline ID 3: MRs: #<ActiveRecord::Relation [#<MergeRequest id:1 namespace1/project1!1>]>, 1
=== touching path /group1/project1/merge_requests/1/pipelines.json
touching key /group1/project1/merge_requests/1/pipelines.json, etag is c431749008af3dfff120bc8fc3b6fc07
touching key /group1/project1/pipelines/3.json, etag is 02c00c94b3676dfa60f053d0d5fc19ba
touching key /group1/project1/builds/5.json, etag is 491800fe7ce5eca4eb7187e05bbda8ca
touching key /group1/project1/pipelines/3.json, etag is 717b5e8b6111a9a298cab7fca7e8fde7
touching key /group1/project1/builds/4.json, etag is b81ac01f17d97fab04014085641e4655
executing create pipeline here for merge_request, group1/project1
=== updating etag cache for pipeline 4
touching key /group1/project1/pipelines.json, etag is df92ea141f2fbd7596449a53142a6f71
touching key /group1/project1/pipelines/4.json, etag is 2fdaa797c2a259c168b352839ac2c248
touching key /group1/project1/commit/0b4bc9a49b562e85de7cc9e834518ea6828729b9/pipelines.json, etag is 0891d4fecdbc24aa3b116d8b3125bde2
touching key /group1/project1/merge_requests/new.json, etag is ecd1049b0c80498eee3358a644ca4e4f
pipeline ID 4: MRs: #<ActiveRecord::Relation [#<MergeRequest id:1 namespace1/project1!1>]>, 1
=== touching path /group1/project1/merge_requests/1/pipelines.json
touching key /group1/project1/merge_requests/1/pipelines.json, etag is 685ca2a4e0dd48a57a179a9009594f4a
touching key /group1/project1/pipelines/4.json, etag is 8922897b9616a33821ec7f538dc00def
touching key /group1/project1/builds/6.json, etag is 1eb794bbf892935a9e7a69e14aa9e332
=== starting test here
getting key /namespace1/project1/merge_requests/1/pipelines.json => 445b98579984d75c57b1d09777262177

Notice the duplicate lines:

getting key /namespace1/project1/merge_requests/1/pipelines.json => 445b98579984d75c57b1d09777262177

It looks like we're only invalidating the target project for the merge request, but I think we want to invalidate the source project as well. I'll push a commit.

mentioned in issue #58128 (closed)

added priority3 severity3 labels

closed via merge request !25400 (merged)

mentioned in commit 9b3a0de5

added regression regression:11.5 labels

mentioned in issue #58298 (moved)

mentioned in merge request gitlab!38774 (closed)