Detect and prevent synchronous database writes on HTTP GET requests

Background

During INC-12449, a burst of package download traffic overloaded the database. The root cause was a pattern where a GET request, which should only read data, also writes to the database on every call. In that incident the write was a download timestamp update. Under high traffic those writes piled up and caused 503 errors across web, API, pgbouncer, and patroni.

The immediate fix (!247205 (merged), merged in 19.3) throttled those timestamp writes and removed about 80% of the write volume on GET requests. But the incident exposed a broader problem: nothing in our codebase or tooling stops a GET endpoint from writing to the database. Any endpoint that does this today, or any new one added tomorrow, can trigger the same failure under load. This issue tracks the corrective action for that systemic risk, migrated from gitlab-com/gl-infra/production-engineering#29434 (closed).

What we want to build

The goal is detection and enforcement: make writes on GET requests visible, and block new ones from being introduced.

Almost all the infrastructure already exists. GitLab has a query analyzer framework (Gitlab::Database::QueryAnalyzer) that runs in production and inspects every SQL query the application executes. We used it before for the cross-database modification campaign, with PreventCrossDatabaseModification: that analyzer started with a long allowlist of known violations, raised an error in test so no new violation could get past CI, and only logged in production so users were never affected. The allowlist was burned down over time and is empty today. We want to repeat that same playbook for writes on GET.

The plan is to add a new analyzer that checks, for every query, whether it modifies data (an INSERT, UPDATE, or DELETE, detected through pg_query, the same parser the existing analyzers use) while the current request is a GET. When that happens, it raises an error in dev and test, which makes the spec fail and blocks the merge request. In production it only logs, behind an ops feature flag, so we can find violations that tests do not cover.

One small piece is missing for this to work: the analyzer needs to know the HTTP verb of the current request. Grape endpoints already expose it through caller_id ("GET /api/..."), but Rails controllers do not. The fix is simple: the analyzer middleware already receives the Rack env, so it can record the request method in the request store. Gitlab::Middleware::ReadOnly already reads the verb this way.

We also document the rule in the database development guidelines, including the reference pattern for GETs that legitimately need to write (date check, exclusive lease, and throttling, as Users::ActivityService does today).

Known offenders

Analysis of production logs shows that after !247205 (merged) about 6M requests per week still write on GET. Most of that comes from two places: viewing a merge request updates its merge_status column (~3.6M requests), and downloading a repository archive inserts audit event rows (~2.4M requests). A smaller one, the dependency proxy, writes on cache miss (~385K requests). These endpoints seed the analyzer's initial allowlist. Fixing them is not in scope here: as the rollout scan confirms the violators, we open backlog issues for the responsible groups.

Proposed steps

Each step can be a separate merge request:

  1. Record the HTTP request method in the request store, from the query analyzer middleware.
  2. Add the PreventWritesOnGet analyzer, seeded with the allowlist of known violators. It combines two existing analyzers: like LogLargeInLists, it logs in production behind an ops feature flag; like PreventCrossDatabaseModification, it raises in dev and test so CI blocks new violations.
  3. Document the constraint in the database development guidelines, with the throttled-write reference pattern for GETs that legitimately need to write.

This corrective action is complete when the analyzer is enforcing in CI and the documentation is merged, with backlog issues opened for the violations found.

References

Edited by Leonardo da Rosa