Skip to content

WIP: Remove issues, merge requests, and epic notes from ETag caching

Nick Thomas requested to merge 209784-fix-notes-caching into master

What does this MR do?

GitLab has an etag-based caching system for certain endpoints. This is found in lib/gitlab/etag_caching/ (and EE equivalents). Among other things, this is used to cache the .../noteable/:type/:iid/notes endpoints.

These are meant to return a list of notes created since a given X-Last-Fetched-At value, provided in the request headers. However, this interacts badly with the ETag-based caching. If X-Last-Fetched-At needs to move on, it is prevented from doing so until the ETag cache is invalidated for that noteable.

This is usually not a problem, since we return all notes between X-Last-Fetched-At and now, and invalidate the cache when a new note is added. However, with pagination, we might return a single page of notes, and ask the frontend to poll again with an X-Last-Fetched-At from the distant past. When this happens, we end up stuck on that page of notes, since the ETag-based caching kicks in and returns the same page in response to every request, until the cache is invalidated for some other reason (a new note is added, say).

I didn't spot this while developing the backend because I routinely disable all caching in my browser 😬

So, what to do? One option is to skip adding an ETag if we are returning a partial page of notes. This initially seemed quite tempting to me, but then I dug a bit deeper. All the caching is saving us from doing is calculating and returning this response body:

{"last_fetched_at":1600085609861410,"more":false,"notes":[]}

With the caching disabled, we return this every 5 seconds, with the last_fetched_at incrementing each time. It equates to the usual HTTP request processing, plus a query that verifies no new notes have been added since last time. It's more expensive than the cache, but is it that much more expensive to justify the complexity?

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Part of #209784 (closed)

Edited by Nick Thomas

Merge request reports