Answer Dr. CI auth failures with a status instead of an unhandled 500

[izaitsevfb]

✴️ iz2: POST /api/drci/drci?prNumber=N returns HTTP 500 to trymerge.py, and has since somewhere between 2026-08-29 22:23 UTC and 2026-09-02 23:51 UTC. trymerge reads any failure of this endpoint as "no classifications" and falls back to the Dr. CI check-run summary, so nothing reported it — it has been classifying off a stale snapshot for about three weeks, and the AI_NOT_RELATED category, which trymerge deliberately drops from that fallback, never reaches the merge gate at all.

The handler's own try/catch answers 400, and the auth branches answer 403/429, so a 500 means something threw outside that try. The only code there is the auth block and the req.body destructure — and the destructure is excluded, because update-drci-comments.yml posts the same form-encoded body to the same route every 15 minutes and its pytorch/pytorch job is green. That leaves the user-token branch, which is reached only when the Authorization header does not equal the deployed DRCI_BOT_KEY, and whose users.getAuthenticated() throws uncaught.

This PR does not restore suppression. It cannot: the remaining half is a credential, and which side of that comparison changed is not something the code can tell you. What it does is make this class of failure say so, in one merge cycle, instead of hiding for three weeks.

What changes

• A GitHub 401 answers 401 (with WWW-Authenticate); anything else — unreachable GitHub, a ClickHouse fault in the rate limiter — answers 503. Conflating the two would point callers and monitoring at credentials when the problem is ours. Messages are fixed strings; exception text can name backends and queries, so it stays in the log.
• The rate-limit increment is awaited. It was neither awaited nor caught, so its rejection escaped. That is not theoretical: reverting the await in a mutation test does not fail the test, it kills the Node worker — which is how an unhandled rejection here becomes a 500.
• A failed increment refuses (503) rather than serving. That insert is the ten-per-hour limit. Reads and writes use separate ClickHouse credentials, so a lost INSERT permission can persist while the read keeps answering, and every authenticated user would drive unbounded work under the service's own bot credentials without consuming quota. The bot-key path never reaches this limiter, so the scheduled comment updates are unaffected.
• DRCI_BOT_KEY must be non-empty to match. authorization == process.env.DRCI_BOT_KEY is undefined == undefined for a caller that sends no header, so a deployment that lost the variable admitted every anonymous request as the bot.

Test plan

New torchci/test/drciAuth.test.ts, thirteen cases over every branch: GitHub refusal → 401 + WWW-Authenticate; non-refusal GitHub fault → 503; factory failure → 503; upstream 403 → 503, not 401; rate-limit read failure → 503, asserting the backend hostname is absent from the body; rate-limit write failure → 503 and the body never runs; authenticated under-limit → reaches the body; over-limit → 429 with no increment; user token without prNumber → 403; no header → 403; unset and empty DRCI_BOT_KEY → 403; bot key → skips the user path.

test/nextApiMocks.ts gains an end() recorder, because the shared mock had only status/json and this route ends 403/429 responses. The 403 and 429 cases assert _ended, so deleting .end() cannot leave them green while real requests hang.

Mutation-tested, restored byte-identical and re-run green after: reverting the bot-key guard, always-401, never-401, and re-swallowing the increment all fail; dropping the await kills the worker.

next lint, tsc and jest all pass; prettier --check clean. crcrResults, greenlightPrState and drci suites — the other mockRes consumers — pass.

Still open after this

Someone with access needs to compare pytorch/pytorch's DRCI_BOT_KEY secret against the value the deployment serves. Once this lands, a trymerge log will name the reason instead of HTTP Error 500.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated
torchci	Ignored		Sep 18, 2026 9:41pm UTC

[jeanschmidt]

Review summary

🟡 1 minor · ⚪ 1 nit

• 🟡 greenlight's drci_poke comment still states that a Dr. CI auth failure answers 500 because the auth branch sits outside the try/catch — torchci/pages/api/drci/drci.ts:133
• ⚪ The new test file's header says a throw in the authorization block leaves no response and answers 500, which is what the change it tests stops happening — torchci/test/drciAuth.test.ts:5

Extended analysis (ai-generated section)

What this changes

The Dr. CI endpoint's user-token branch in torchci/pages/api/drci/drci.ts gains its own try/catch. A GitHub error carrying status 401 answers 401 with a WWW-Authenticate: Bearer header; anything else answers 503. incrementDrCIRateLimit is now awaited and a failed increment refuses with 503 rather than serving. The bot-key comparison becomes botKey && authorization === botKey, so an unset DRCI_BOT_KEY no longer matches a header-less request. A new torchci/test/drciAuth.test.ts covers the branches, and the shared mockRes gains an end() recorder.

What it gets right

Awaiting the increment is the real fix, not the status codes: a rejected floating promise is what left the function with no response. The 401/503 split is drawn in the right place — a GitHub 403 for secondary rate limiting is not a bad credential, and a test pins that it answers 503. Response bodies are fixed strings with the exception kept in the log, and one test asserts a backend hostname never reaches the body. The bot path stays outside the limiter, so the new 503 cannot touch the scheduled sweep. The _ended recorder closes a real gap: a status-only assertion cannot tell a terminated 429 from a hanging one.

Where the risk is

Everything concentrates in two added lines: the guard at drci.ts:117 and the catch at drci.ts:157-166. Several independent reviews landed on the same WWW-Authenticate: Bearer line (cluster c-004), which is signal, though they do not agree on the mechanism. The durable part is that the bot-key comparison is an exact === against a raw secret, so a caller honouring the advertised scheme cannot match it; the further claim that a Bearer-prefixed user token also fails at GitHub has evidence against it, since Octokit strips the prefix. One edit — dropping the setHeader and the assertion at drciAuth.test.ts:111 — settles the whole cluster.

A theme runs through the rest: the change improves legibility and then mislabels the result in three places. The single catch spans GitHub and ClickHouse, so a rate-limiter fault is reported as "Authentication service unavailable" (c-005). Two greenlight comments still state the old contract, that an auth failure answers 500 and the status cannot classify it, at the only in-repo consumer that receives the new statuses (c-003, one edit for both). The new test file's header states the removed 500 behaviour in the present tense (c-006).

Before merge, and after

The guard is the one item that can take something down: "If the deployed DRCI_BOT_KEY is unset, the new botKey && guard makes update-drci-comments.yml's header-less curl 403". The code is correct; the check is operational and cannot be settled from the repository. Confirm the deployment variable is set first. The WWW-Authenticate deletion is one line and one assertion — cheap enough to take now.

Follow-up: the greenlight comment drift in drci_poke.py and test_drci_poke.py; splitting the catch so the limiter does not inherit the auth label; setting _ended in json(); the docstring tense. "Relabels the failure status but adds no detection" is a design decision about monitoring, not a fix to this diff.

[jeanschmidt]

this is the kind of thing that gets me puzzled, should we update this with the same change, given it is the same repo?

---

🟡 greenlight's drci_poke comment still states that a Dr. CI auth failure answers 500 because the auth branch sits outside the try/catch (ai-generated section)

This change wraps the Dr. CI authorization branch in its own try/catch and answers 401 or 503 instead of falling through to the platform's 500. Two comments elsewhere in this repository assert the opposite and are not updated: greenlight/src/greenlight/drci_poke.py:127-128 reads "An auth failure answers 500, not 403 -- the endpoint's auth branch sits outside its try/catch -- so the status code cannot classify the failure. Log it and move on.", and greenlight/tests/test_drci_poke.py:180 repeats "An auth failure answers 500, so a non-2xx cannot be classified -- but it must never raise." Both clauses are now false: the branch is inside a try/catch (this line), and a GitHub refusal answers 401 (drci.ts:160) while a dependency fault answers 503 (drci.ts:164). greenlight/src/greenlight/drci_poke.py:107 POSTs to DRCI_ENDPOINT = https​://hud.pytorch.org/api/drci/drci (greenlight/src/greenlight/constants.py:66), i.e. exactly the route this diff changes, and logs the non-2xx status at drci_poke.py:129 under the comment quoted above. The diff adds try { at drci.ts:133 around the whole auth block and adds res.status(401) at drci.ts:160 and res.status(503) at drci.ts:164, so the status code now does classify the failure for precisely the case the comment says it cannot. A maintainer debugging a failed poke reads drci_poke.py:127, concludes the logged status is uninformative, and ignores a 401 or 503 that now names the cause -- the exact misdiagnosis this change exists to prevent. The stale sentence also blocks the obvious follow-up of classifying the status in poke, since the comment documents it as impossible. Replace drci_poke.py:127-128 with something like # The endpoint distinguishes its failures: 401 means the token was refused, 503 means Dr. CI could not reach GitHub or ClickHouse. The poke still swallows all of them -- log the code and move on. and update the matching sentence at greenlight/tests/test_drci_poke.py:180 the same way.

_{Reviewed by claude-opus-5[1m] at max effort, against 6cab1a9.}

[jeanschmidt]

⚪ The new test file's header says a throw in the authorization block leaves no response and answers 500, which is what the change it tests stops happening (ai-generated section)

The module docstring describes "The Dr. CI endpoint's authorization block" and then states in the present tense that a throw there leaves the function with no response and the platform answers 500. After this change the block is wrapped in a try/catch (torchci/pages/api/drci/drci.ts:133) that answers 401 or 503, and the thirteen tests below this docstring assert exactly that. The paragraph reads as a description of current behaviour rather than of the failure mode the change removed. drciAuth.test.ts:5-6 says "A throw here leaves the function with no response and the platform answers 500." The first test at drciAuth.test.ts:96-112 asserts res._status is 401 for a throw in that same block, and drciAuth.test.ts:122-125 asserts 503 for another. A reader who opens the file for the first time is told the block 500s and then reads twelve tests proving it does not, and has to reconcile the two before trusting either. Put the sentence in the past or conditional: "Before this handling existed, a throw here left the function with no response and the platform answered 500.".

_{Reviewed by claude-opus-5[1m] at max effort, against 6cab1a9.}