diff --git a/docs/design-decisions/051-timeline-actor-type-for-bot-classification.md b/docs/design-decisions/051-timeline-actor-type-for-bot-classification.md new file mode 100644 index 00000000..c90ccd9a --- /dev/null +++ b/docs/design-decisions/051-timeline-actor-type-for-bot-classification.md @@ -0,0 +1,483 @@ +# Timeline Actor Type for Bot Classification + +> Status: **Accepted** — implemented in PR #194 (`doc/051-timeline-actor-type`). +> The production drain, the first post-drain export, and the downstream switch +> in `qb-notebook` are still outstanding; see Operational Notes. +> Converted from a living implementation plan to a decision record on +> 2026-08-23 (plan history is in git). + +## Context + +- `PRTimelineEvent.actor_login` stored *who* acted, but not *what kind of + account* acted, and not a rename-stable identity for that account. Every + downstream consumer that needed "was this a human?" therefore hardcoded a + list of bot logins — and **login is the wrong key**, which is the root cause: + any identity change silently invalidates the list. +- GitHub already told us, we already asked, and we threw it away. All three + live timeline queries selected the actor's `__typename` + (`pr_bundle.graphql`, `timeline_page.graphql`, `timeline_page_back.graphql`); + `_login_or_empty()` in `syncer/services/sub/timeline_sync.py` kept only + `login` and dropped it. +- **Motivating failure.** `qb-notebook`'s `DEFAULT_BOT_ACTORS` + (`qb_notebook/review_states.py`) finds each PR's first *human* review touch. + The mathlib bots changed identity on **2026-02-03** and the list went stale + unnoticed. Effect on "open → first review" for a last-90-days cohort + (snapshot 2026-08-10): **15.7 % of first touches were bot events**, and the + median moved 0.261 d → 0.467 d once filtered — a reported ~41 % latency + improvement that was mostly an artifact. Six months of a wrong number from a + change nobody downstream could have seen. +- **What the changeover actually was** (resolved live 2026-08-15, correcting + the original "rename" reading): + + | login | kind | node id | + | --- | --- | --- | + | `mathlib4-merge-conflict-bot` | `User` | `U_kgDODVl3LA` | + | `mathlib-merge-conflicts` | **`Bot`** | `BOT_kgDOD2_IkQ` | + | `mathlib4-dependent-issues-bot` | `User` | `U_kgDOCsITAQ` | + | `mathlib-dependent-issues` | **`Bot`** | `BOT_kgDOD2_cBQ` | + + The machine-user bots were **replaced by GitHub Apps** — not renamed. The old + logins still resolve to their original accounts. Consequences: `actor_type` + alone catches the replacements and any future App with no list to update; no + key, node id included, could have bridged the substitution, so + `actor_node_id` is justified by the ordinary renames it *does* survive and by + giving the residual machine-user list a stable key. That residual list is the + old, frozen accounts (`U_kgDODVl3LA`, `U_kgDOCsITAQ`, and + `leanprover-community-bot-assistant` = `U_kgDOBcsTTQ`), plus + `leanprover-radar` (`User`, `U_kgDOCG88RQ`); `mathlib-triage` is a `Bot` + (`BOT_kgDOD2_uYQ`). +- Automation accounts with no API-side bot signal at all, found in the same + sweep: `mathlib-auto-merge`, `mathlib-splicebot`, `leanprover-bot`, + `mergify`, `downstream-reports-automation`, `botbaki-review`, + `copilot-pull-request-reviewer`, `copilot-swe-agent`. +- No usable substitute existed in the export: + - `core_user.github_node_id` encodes the kind in its prefix, but `core_user` + is a PR-**author** table: 60 event actors accounting for **111,864 touch + events (~41 %)** have no row there, because most bots never open PRs. + - The GitHub App `[bot]` login suffix is not available: **0** occurrences + anywhere in the export. GraphQL's `Bot.login` never includes it (REST's + `/users/[bot]` does), so this is not something we strip. + - `PRTimelineEvent.extra` is populated only for `REVIEW_DISMISSED` rows and + arrives in parquet as a JSON *string*; not a substitute for a typed column. +- Precedent inside the same function: the `ReviewRequested` / + `ReviewRequestRemoved` branch already read `requestedReviewer.__typename` to + route `User`/`Bot`/`Mannequin` vs `Team`, and the top-level `author` + selections already requested `id`. The timeline actor unions just never did. + +## Decision + +- **Columns.** `PRTimelineEvent` gains `actor_type` (`PRActorType` choices + `User` / `Bot` / `Mannequin`, GitHub's exact wire casing) and + `actor_node_id`, both nullable — migration + `syncer/migrations/0054_prtimelineevent_actor_type_and_node_id.py`. `NULL` + means *unknown*, never `User`. + - **No index on either.** `actor_type` is three-valued, the "first human + touch per PR" shape is already served by `syncer_prtimeline_pr_time_idx`, + and the analytics consumer reads a `SELECT *` parquet. Add one when an + in-repo query needs it. + - **No CHECK constraint.** Unlike `before_sha` / `label_name` / + `requested_*`, these are meaningful on every event type, so there is + nothing to type-scope. In particular, do not encode "`actor_type` set ⟹ + `actor_login` non-empty": it holds today but is a GitHub-side invariant we + do not control. +- **Wire.** `id` added to the 12 timeline `actor` unions, the `IssueComment` / + `PullRequestReview` `author` unions, and `ReviewDismissedEvent`'s + `review { author }` in all three live queries. Adding fields to an existing + selection does not change GraphQL point cost. The inline-comment `author` + under `PullRequestReview.comments.nodes` is deliberately left `login`-only + (see the non-goal below). +- **Extraction.** `actor_type_or_none` / `actor_node_id_or_none` (public, + because the backfill command imports them) plus a thin `_actor_identity` + pair-builder in `timeline_sync.py`, applied in every branch of + `_extract_event_fields` that sets `actor_login` — both `actor` idioms and the + `author` branches. The allowed typename set is derived from + `PRActorType.values` so the helper cannot drift from the model. An + *unmodelled* typename (a hypothetical `Organization`) yields + `actor_type = NULL` but still stores `actor_node_id`: the node id is exact + regardless of whether we model the kind. + `_extract_event_fields` is the single funnel for all five + `sync_timeline_events` call sites, so one extraction change covers every + ingest path, archive import included. +- **Synthesized dismissed-review parents.** `REVIEW_DISMISSED` rows now + denormalize `dismissed_review_author_type` / `_node_id` into `extra`, and + `_synthesize_dismissed_review_parent` reads them back — otherwise those rows + (which are exactly the review events the motivating metric reads) would be + `NULL` even under fresh ingestion. Rows whose `extra` predates the new keys + stay `NULL` and are healed by the backfill. +- **Backfill: targeted `nodes(ids:)` resolution, not a rewalk.** Every row + already stores the timeline item's own `github_node_id`, and GitHub + re-resolves that node's actor on demand. + `syncer/queries/actor_types_by_node_ids.graphql` (registered in + `scripts/validate_github_graphql.py`, which uses an explicit list, not a + glob) plus `manage.py backfill_timeline_actor_types`. This is *exact*, not + heuristic: it resolves the actual actor object attached to each specific + event, so renamed accounts resolve correctly and login reuse cannot mis-type + anything. Measured live: `rateLimit.cost` is **1 at the full 100-id cap**, + confirmed again by a 2 000-row probe on production (20 calls, 20 points), so + ~608 k rows ≈ **~6.1 k points** — roughly 1/20th of a schema-version rewalk + wave, with no reset migration and no interaction with the upgrader chain. + Note the installation's GraphQL budget is **~9 700 points/hour**, not the + 5 000 a PAT gets (GitHub App limits scale with repos and users), so the whole + drain fits inside a single rate window if the live syncer leaves room. + - **Dead node ids are dropped by name, not by bisection.** Deleted comments + and reviews are common enough to matter: one unresolvable id makes GitHub + reject the whole call, and bisecting a 100-id batch to rediscover which id + was bad costs 13 calls. GitHub already names it in the error message + (`…global id of 'IC_…'`), so the command parses the named ids out, drops + them, and retries the remainder in one more call. At 0.25 % deleted rows + over ~608 k that is the difference between ~18 k wasted points and ~1.5 k. + Halving survives only as the fallback for error messages with no id to + parse. Dropped ids are absent from the result, which is what makes them + land in `unresolved` — a fact about the row, not about the call. + - A named `ActorIdentity` fragment keeps the 13 inline fragments readable; + `... on Comment` is what covers `IssueComment`, `PullRequestReview`, and + the synthesized dismissed-review parents (whose stored node id *is* the + review's). + - Per-repository clients and an outer repo loop, because a single cross-repo + client would bypass GitHub App operation-token resolution that every other + syncer entry point goes through. +- **Archive-row `actor_login` healing, in the same pass.** The legacy archive + fragment omitted the `actor` field entirely for eight event types, so those + rows have no attribution at all — not merely no typename. The same + `nodes(ids:)` response carries `login`, so the drain fills it, guarded + **fill-only**: write only when the resolved login is non-empty *and* the + stored value is empty, with the predicate covering both `NULL` and `''` + because the two extraction idioms disagree. A non-empty stored login is the + login *as of ingest time*; clobbering it with today's login would destroy the + rename history `actor_node_id` exists to expose. (Nothing to fix in + `src/queueboard/queries/pr_info.graphql`: it is no longer a live fetch path, + so the backfill is the only available remedy.) +- **Fill-empty allowlist extended.** `actor_type` and `actor_node_id` are added + to the explicit column tuple `sync_timeline_events` iterates when updating + existing rows. The drain writes via `bulk_update` and does not depend on + this, but every ordinary rewalk does — including the continuous timeline + backfill for PRs not yet `timeline_backfill_done`. **This is the single + easiest thing to break in any future change here**; omit it and those paths + silently never populate the columns. +- **Export and sanitization unchanged.** + `EXPORT_TABLE_QUERIES["syncer_prtimelineevent"]` is `SELECT *`, so the + columns flow through automatically; `scripts/sanitize_backup.py` references + neither the table nor any `actor_*` column. `actor_type` is a three-valued + enum and `actor_node_id` an opaque GitHub identifier already exported for + authors via `core_user.github_node_id`. +- **Progress is monitored, not eyeballed.** `syncer.collect_convergence` + records two per-repo counters on `SyncerConvergenceSnapshot` (migration + `0055`), following the precedent of `archive_resync_remaining` for the + doc-043 drain: + - `timeline_events_missing_actor_type` — the backfill command's exact target + set (`actor_type IS NULL AND github_node_id IS NOT NULL`), so the admin + page and the command's own output agree. It **plateaus** at the null-actor + floor rather than reaching 0. + - `timeline_events_untyped_with_login` — the same rows narrowed to those + carrying a login. A row with a login demonstrably had an actor, so this is + typeable work: it converges to ~0 and then **stays** there. That makes it + the standing regression canary — if it climbs later, ingestion stopped + typing actors, most likely because the fill-empty allowlist was dropped. + + Both are `COUNT(*)` over `syncer_prtimelineevent` per active repo every + `ANALYTICS_CONVERGENCE_PERIOD_SECONDS` (900 s). With no index on + `actor_type` that is a sequential scan, which is acceptable at this table + size and cadence; a partial index `WHERE actor_type IS NULL` is the escape + hatch if the collector ever gets slow. +- **Not in scope: `PRReviewInlineComment.author_login`.** The mechanism would + work verbatim (the wire already carries `author { __typename … }`, + `github_node_id` is unique, the table is exported), but it carries **no new + information**: an inline comment's author is the parent review's author. + Sampled 30 recent merged mathlib4 PRs (2026-08-13): 33 inline comments, **0 + author mismatches** against their parent review — 3 were bot-authored, so + bots do post inline comments, they just do it as their own review. So + `author_type` is derivable by joining `review_node_id` → + `PRTimelineEvent.github_node_id`. Treat the sample as absence-of- + counterexample, not proof. Residual gap: comments whose parent + `PRTimelineEvent` row does not exist (the documented null + `parent_review_event` case) have nothing to join to. What would justify + revisiting: per-comment review-effort metrics that count inline comments per + author *without* joining to the parent, where + `copilot-pull-request-reviewer` / `copilot-swe-agent` would inflate human + review volume. Denormalization for convenience, not correctness — so it + should be driven by a real downstream query, not by symmetry with this + change. + +## Consequences + +- **`__typename == "Bot"` identifies GitHub *Apps*, not all automation.** + Machine accounts that are ordinary user accounts report `User` — confirmed + for `leanprover-community-bot-assistant`, `leanprover-radar`, and both + retired mathlib bots. `actor_type` is therefore **necessary but not + sufficient**: the downstream machine-user list shrinks and stops being + rename-fragile, but does not disappear. Key that residual list on + `actor_node_id`. +- **`actor_type IS NULL` is a permanent, non-trivial population.** GitHub + returns a null actor for a real share of events: 12 of the first 100 real + mathlib4 nodes probed, concentrated in workflow-driven label events + (`delegated`, `ready-to-merge`). No backfill route can type these. Analysts + must read `NULL` as *unknown*, never as `User` — this repo has no export + README, so that documentation lives in `qb-notebook` / `analytics-datasets`. +- **A login list is still required as the fallback for untyped rows, and + dropping it would silently reclassify bots as humans.** Measured after the + drain: **678 events carry a known automation login but `actor_type IS NULL`** + — 83 % of the 812 unresolvable node ids, concentrated in + `mathlib-dependent-issues` (564), whose comments the bot itself deletes and + reposts. Deleted comments are exactly the rows `nodes(ids:)` can never + re-resolve. Those are `ISSUE_COMMENTED` rows, i.e. a *first-touch* event + type, so a downstream predicate of `actor_type == 'Bot'` alone would count + them as human review touches — reintroducing a smaller version of the bug + this whole change exists to fix. The correct downstream shape is the union of + three tests: `actor_type == 'Bot'`, **or** `actor_node_id` in the frozen + machine-user set, **or** `actor_login` in the residual list. The list stops + being load-bearing for typed rows; it does not stop existing. +- The set of `(actor_login, actor_node_id)` pairs is a free rename history — + and read the other way, a *changed* node id under a similar-looking login is + the signature of an account replacement like the 2026-02-03 one. +- The update path is fill-only (`new_val and not getattr(obj, col)`), so a + rewalk never overwrites an existing `actor_type`. That makes the drain + idempotent and order-independent with respect to the live syncer. +- Synthesis uses `get_or_create`, so a synthesized parent row that already + exists from a pre-051 ingest is **not** retyped by a later dismiss-event + re-ingest. Those rows are healed by the ordinary update path if the + `PullRequestReview` node itself is walked, and by the drain otherwise. Left + as-is rather than special-cased: the fill-only convention stays uniform. +- **Parquet dtype drift.** `scripts/export_for_analysis.py` goes + Postgres → `COPY … CSV` → `pd.read_csv` → `to_parquet`. An export run while a + column is entirely NULL infers all-NaN **float64** and lands as `double`; a + later export lands as `object`. `requested_team_slug` in the current artifact + is `double` for exactly this reason. Hence the export must be sequenced + *after* the drain, and the check is on dtype, not just column presence. +- Width cost: ~40 B × ~600 k rows on two columns, in the table and in the + export. `actor_node_id` is the separable half if that is ever unwelcome — + dropping it would mean keeping the rename fragility downstream. +- Tunables are CLI args plus module constants, deliberately not settings: a + one-shot operator command must not introduce a `getattr(settings, …)` with no + matching `os.getenv` in `base.py` (root `AGENTS.md`). If the drain ever + becomes beat-driven, wire it through `base.py` *and* `.env.example` then. + +## Operational Notes + +### Status + +- Landed in PR #194: columns + migration `0054` + admin + (`actor_type` in `list_display`/`list_filter`, `actor_node_id` in + `search_fields`, both in `readonly_fields`), extraction, and the backfill + command with 22 + 23 tests, and the two convergence counters (migration + `0055`) with 3 tests. Full `syncer` suite green (527 tests) on + host-against-dockerized-Postgres; `scripts/validate_github_graphql.py` and + `scripts/validate_backup_policy.py` pass; CI `checks` + `docker` (which runs + `scripts/repo_check_compose.sh`) green. +- Validated against the live API, not only fakes: 154 real mathlib4 node ids + resolved through the new query, every actor typed as expected, 12 null + actors in the first 100, 0 unresolvable, and cost 1 at the 100-id cap. +- Production probe, 2026-08-24 (`--dry-run --limit 2000` on mathlib4, 607 558 + rows then untyped): 20 calls, **20 points**, 1 994 rows typed — `User` 1 340, + **`Bot` 654 (33 %)**, 6 null actors, 0 unresolved, 0 call failures. The token + came from the GitHub App path (`github_app_token_minted`), and the run left + `remaining=9621 used=79`, which is how the ~9 700/hr installation budget + above was measured. + - The null-actor rate here is **0.3 %**, against 12 % in the earlier + recent-timeline probe. The id-ordered head of the table predates the + workflow-label automation that produces most null actors, so the eventual + plateau lands somewhere between the two — a rising `null_actor` share as + the drain reaches newer rows is expected, not a fault. + - `logins=0` is likewise an artifact of id order: archive rows were imported + later and carry higher ids, so the `actor_login` healing shows up in the + back half of the drain. + - The first live drain (2026-08-24) then hit dense runs of deleted + `IssueComment` ids, which is what surfaced the bisection cost above. The + probe's id-head slice had none, so the dry run could not have predicted + it — worth remembering that a 2 000-row head-of-table probe does not + characterise the whole id space. +- **Drain complete for mathlib4, 2026-08-24.** 607 558 untyped rows at the + start; **565 269 typed**, leaving **42 289 (7.0 %)** that no route can ever + type — 41 477 where GitHub reports a null actor and 812 whose node id no + longer resolves. That remainder is exactly the final pass's + `null_actor + unresolved`, which is what proves the drain finished rather + than stalled. Of the 267 542 rows typed in the second pass, **39.7 % were + `Bot`** — the population a login list could not see. + - Cost: 3 099 points for that pass, exactly 1 per 100-id batch. The 347 + batches containing a dead id cost **nothing** — GitHub does not charge for + a rejected query — so `api_calls` (3 446) exceeds `points` by precisely the + rejection count. `retries=0` and `call_failed=0` across ~3.4 k calls. + - Dead ids were 0.26 % of rows (812 across 347 batches, 2.34 per affected + batch), not the ~1 % a dense log window suggested mid-run. Correcting the + projection that prompted the name-parsing fix: bisection's *failing* + sub-calls are also free, so the penalty was ~6 extra charged points per + affected batch (~2 k points here), and mostly a **wall-clock** cost of ~12 + extra throttled calls per affected batch. The fix was still worth + deploying, but the "several rate windows" scenario was an artifact of + extrapolating from a dense pocket. + - **`logins=0` — the archive-row `actor_login` healing recovered nothing.** + Not a failure of the mechanism: across the whole id space, every row the + drain saw with an empty login had no actor on GitHub either. So the gap was + already closed by ordinary rewalks (`actor_login` has always been in the + fill-empty allowlist), or it is a subset of the 42 289 floor — those events + are permanently unattributable, not merely unattributed. Measured after the + drain: **10** archive-imported rows still have an empty `actor_login`, so + the gap this scope item existed to close had already closed itself. Keeping + the fill in the command cost nothing and is still correct; it simply had no + work left to do. +- **Post-drain typing confirmed on production, 2026-08-24.** The doc's central + claim holds at scale: the replacements are Apps and the retired accounts are + machine users. + + | login | actor_type | events | + | --- | --- | --- | + | `github-actions` | `Bot` | 109 579 | + | `mathlib-bors` | `Bot` | 72 535 | + | `mathlib-triage` | `Bot` | 14 711 | + | `mathlib-merge-conflicts` | `Bot` | 7 117 | + | `mathlib-dependent-issues` | `Bot` | 3 853 | + | `leanprover-community-bot-assistant` | `User` | 10 499 | + | `mathlib4-dependent-issues-bot` | `User` | 8 103 | + | `mathlib4-merge-conflict-bot` | `User` | 7 564 | + | `leanprover-radar` | `User` | 1 713 | + + So **207 795** automation events are now caught by `actor_type` alone with no + list to maintain, against **27 879** from machine users that still need one — + keyed on `actor_node_id`, since those four accounts are historical and frozen. + mathlib4 was the only repo with untyped rows left (42 289); every other repo + is fully typed. +- Outstanding: the first post-drain export, and the `qb-notebook` switch. + +### No new configuration + +The command adds **no settings and no env vars**. It uses the same token path +as the live syncer — `GitHubClient(operation="syncer_pr_read", owner=…, +repo=…)`, i.e. the GitHub App installation token, falling back to +`GH_TOKEN`/`GITHUB_TOKEN` — and reads the rate snapshot from the Redis behind +`CELERY_BROKER_URL`, which the worker already uses. If Redis is unreachable the +snapshot is `None`, the rate gate silently does nothing, and the drain instead +stops on GitHub's own rejection (below). Request pacing comes from the existing +`SYNCER_GH_THROTTLE_MS` (250 ms), shared cross-process with the live syncer. + +### Runbook + +1. **Deploy.** Merge and push; the Procfile `release` phase runs `migrate`. + Both columns are nullable with no default, so `0054` is a metadata-only + `ALTER TABLE` — no rewrite and no long lock on ~600 k rows. From here the + live syncer types new events, and rewalks heal old rows through the + fill-empty allowlist. +2. **Dry run, to see the distribution before writing anything.** It resolves + for real and only skips the writes, so it **spends the same GraphQL points** + as a live run of the same size — which is what makes it a usable cost probe. + Heroku consumes `--flags` before passthrough, so keep the `--`, and set + `PYTHONPATH` the way the Procfile does: + + ```bash + heroku run --app queueboard-backend --no-tty -- \ + sh -c 'export PYTHONPATH=$PWD/qb_site:$PWD${PYTHONPATH:+:$PYTHONPATH}; \ + exec python qb_site/manage.py backfill_timeline_actor_types \ + --repo leanprover-community/mathlib4 --dry-run --limit 2000' + ``` +3. **Drain.** ~6.1 k points against a measured ~9 700/hr budget *shared with + the live syncer*, so the binding constraint is wall clock, not points: + ~6 080 calls at 250 ms of shared throttle each is ~25 minutes of throttle + alone, realistically an hour or so end to end. Expect no sleep, or one if + the syncer is busy. Use a detached dyno so a dropped connection does not + kill it: + + ```bash + heroku run:detached --app queueboard-backend --size=standard-1x -- \ + sh -c 'export PYTHONPATH=$PWD/qb_site:$PWD${PYTHONPATH:+:$PYTHONPATH}; \ + exec python qb_site/manage.py backfill_timeline_actor_types --wait-for-rate' + heroku logs --app queueboard-backend --dyno run.NNNN --tail + ``` + + The default `--min-rate-remaining` is 2 500, matching the floor the doc-043 + forced-resync drain settled on: the live syncer stops itself only at + `SYNCER_RATE_REMAINING_MIN` (200), so that headroom keeps it fed while the + drain runs. Without `--wait-for-rate` the drain stops cleanly at the floor + and is resumable — the target set is + just `actor_type IS NULL`. `--limit` / `--batch-size` / `--repo` drip-feed it + under supervision. +4. **Read the counters.** Per repo and in total: + `scanned`, `written`, `typed`, `node_ids`, `logins`, then the four ways a row + can come back untyped, which mean different things and must not be conflated: + - `null_actor` — GitHub says there is no actor. Permanent; this is the floor + the drain plateaus at. + - `unresolved` — the node id no longer resolves (hard-deleted comment or + review). Also permanent. + - `call_failed` — the call failed for a reason unrelated to the row + (transport error, or a GraphQL error not attributable to an id). Retried + up to 3 times with backoff first; whatever is left is **not** a fact about + the row, and a later run picks it up. + - `unmodelled` — a typename outside `PRActorType`. The node id is still + stored. + + `points` is the summed `rateLimit.cost` from the responses themselves — + measured spend, not `api_calls` × an assumed price — and the closing `rate:` + line reports the `remaining` / `used` / `resetAt` the run leaves behind. A + small `--limit` run is therefore a direct cost probe: extrapolate from its + `points` / `scanned` ratio before committing to the full drain. + + A rate-limit rejection from GitHub unwinds to the same resumable stop as the + floor rather than retrying or splitting, since both would only spend a + budget that is already gone. `retries` counts re-attempts, so a run fighting + a flaky API is visible. +5. **Watch the canaries** (per the syncer `AGENTS.md` ingestion checklist). + The first two are on the `SyncerConvergenceSnapshot` admin change list, + refreshed every 15 minutes, so the drain can be followed there rather than + by hand — but the collector only starts recording them from the deploy that + carries `0055`, so the first snapshot is the baseline: + - `timeline_events_missing_actor_type` per repo. Should fall steeply, then + plateau at the genuinely-null-actor population. **A plateau at the + *starting* value means the fill-empty allowlist regressed.** Equivalent + SQL: `SELECT count(*) FROM syncer_prtimelineevent WHERE actor_type IS + NULL AND github_node_id IS NOT NULL`. + - `timeline_events_untyped_with_login` per repo — the converging half. + Should approach 0; whatever remains is unresolved nodes and unmodelled + typenames, which the command's counters name explicitly. + - `SELECT count(*) FROM syncer_prtimelineevent WHERE archive_imported_at IS + NOT NULL AND coalesce(actor_login, '') = ''`. On mathlib4 the drain filled + **zero** logins, so this should come back no larger than the 42 289 floor + and consist entirely of rows GitHub reports with a null actor — i.e. it + bottoms out wherever it already was, and that is the answer, not a miss. + - `SELECT actor_login, actor_type, count(*) … GROUP BY 1,2` should show + `Bot` for `github-actions`, `mathlib-bors`, `mathlib-dependent-issues`, + `mathlib-merge-conflicts`, `mathlib-triage`, and `User` for the machine + accounts — **including** the two retired `mathlib4-*-bot` logins, which + are machine users, not Apps. Do not expect those to come back as `Bot`. + - The `(actor_login, actor_node_id)` pairs should reproduce 2026-02-03 as a + replacement: retired `mathlib4-*` logins keep their `U_…` ids and stop + after 2026-02-02; `mathlib-*` logins appear from 2026-02-03 with fresh + `BOT_…` ids. +6. **Only then export** (`upload_backup.yaml`, daily 06:00 UTC or on demand), + so the first parquet is written with values present and pandas infers + `object`. Verify dtype, not just presence. +7. **Only then switch downstream.** In `qb-notebook`, replace the login-only + filter with the three-part union recorded under Consequences — + `actor_type == 'Bot'` **or** `actor_node_id` in the frozen machine-user set + (`U_kgDOBcsTTQ`, `U_kgDOCsITAQ`, `U_kgDODVl3LA`, `U_kgDOCG88RQ`) **or** + `actor_login` in the residual list, which must stay for the 678 untyped + automation events. Document the `NULL` ≠ `User` invariant and the + machine-user caveat in `docs/schema-notes.md`, and keep the fallback path + working against exports that predate these columns. + +### Fallback + +If `nodes(ids:)` ever proves unable to resolve some class of stored node id, the +schema-version wave remains available and cheap to write +(`CURRENT_SYNC_SCHEMA_VERSION = 4`, `UpgradeToV4` subclassing `UpgradeToV3`, a +reset migration mirroring `0045`); v3 drained in under 24 h. Its one genuine +advantage is that it re-ingests *everything*, so it would also pick up any +other field the legacy archive fragment omitted. + +## Alternatives + +- **Login → type map resolved from the API.** Rejected: the mechanism does not + exist. GraphQL has no `bot(login:)` root field, and `user(login:)` / + `repositoryOwner(login:)` cannot return a `Bot` — confirmed live, where + `repositoryOwner(login: "mathlib-merge-conflicts")` returns `null` precisely + *because* that account is a `Bot`. "Bot" could only be inferred from lookup + failure, conflating bots with deleted accounts, organizations, and + mannequins. REST could do it, but `github_client.py` is GraphQL-only. Fatally, + a map resolved *today* is blind to the retired logins that motivated the + exercise. +- **Schema-version wave.** Correct and exact, but ~20× the rate budget, needs a + reset migration, and stalls behind the upgrader chain. Kept in reserve + (above). +- **A normalized actor-directory table** (login or node id → type) instead of + denormalizing onto ~600 k rows. Attractive, since account kind really is a + property of the account. Rejected because the historical rows still need + per-event resolution to be typed at all — the retired logins are only + recoverable through the events that reference them — so the directory would + not avoid the expensive part, and a denormalized column needs no join in the + parquet export, which is the only consumer. diff --git a/qb_site/syncer/AGENTS.md b/qb_site/syncer/AGENTS.md index 8eb082fa..3f9960e4 100644 --- a/qb_site/syncer/AGENTS.md +++ b/qb_site/syncer/AGENTS.md @@ -50,6 +50,21 @@ docker compose exec -T web python qb_site/manage.py bootstrap_archive_worklist \ docker compose exec -T web python qb_site/manage.py resync_archive_touched_prs \ --repo leanprover-community/mathlib4 --apply --limit 1000 +# Timeline actor-type backfill (design doc 051). Re-resolves each stored +# PRTimelineEvent.github_node_id through GitHub's nodes(ids:) root field to +# fill actor_type / actor_node_id, and fills archive rows' missing +# actor_login in the same pass (fill-only — never overwrites a stored login). +# A full 100-id call costs 1 GraphQL point, so the whole table is ~6k points. +# Resumable and idempotent (target set is `actor_type IS NULL`); repeat runs +# plateau at the genuinely-null-actor population rather than reaching zero. +# Watch progress per repo on the SyncerConvergenceSnapshot admin page: +# `timeline_events_missing_actor_type` (the command's own target set) and +# `timeline_events_untyped_with_login` (converges to ~0). +docker compose exec -T web python qb_site/manage.py backfill_timeline_actor_types \ + --repo leanprover-community/mathlib4 --dry-run --limit 500 +# Unattended drain (sleeps until resetAt instead of stopping at the floor): +docker compose exec -T web python qb_site/manage.py backfill_timeline_actor_types --wait-for-rate + # App tests docker compose exec -T web env DJANGO_SETTINGS_MODULE=qb_site.settings.ci python qb_site/manage.py test syncer ``` @@ -130,7 +145,7 @@ front and expensive to recover from when skipped. - `syncer.harvest_commit_history` / `syncer.harvest_commit_history_sweep` (optional), - `syncer.archive_import_tick` → `syncer.archive_import_pr_item` — beat-driven worklist drain for the archive backfill importer (design doc 043). Tick runs every `ARCHIVE_IMPORT_TICK_SECONDS` (default 60s) and gates on `ARCHIVE_IMPORT_ENABLED` so operators can toggle activity without restarting beat. Status surface: `python manage.py archive_import_status [--repo OWNER/NAME] [--errors N]`. - `syncer.resync_archive_touched_tick` — beat-driven drain for the doc-043 forced-resync remediation. Every `ARCHIVE_RESYNC_TICK_SECONDS` (default 600s) it enqueues up to `ARCHIVE_RESYNC_PER_TICK` (default 0 = disabled) `sync_pr(force=True)` tasks from `archive_touched_resync_targets` (open first, stalest `last_synced_at` first, healed PRs excluded), skipping the tick when the cached GraphQL budget is below `ARCHIVE_RESYNC_MIN_RATE_REMAINING` and deduping against still-queued sync_pr enqueues. Self-completing: returns `status=drained` once the target set is empty; `remaining` in the task result tracks progress, and `syncer.collect_convergence` records the same count as `archive_resync_remaining` on `SyncerConvergenceSnapshot` for admin monitoring. - - `syncer.collect_convergence` — records syncer convergence metrics, + - `syncer.collect_convergence` — records syncer convergence metrics. Includes the doc-051 actor-typing counters: `timeline_events_missing_actor_type` is the `backfill_timeline_actor_types` target set and plateaus at the genuinely-null-actor population, while `timeline_events_untyped_with_login` counts only rows known to have had an actor, so it converges to ~0 and is the standing canary for the fill-empty column allowlist in `sync_timeline_events`, - `syncer.collect_metrics` — records sync throughput/lag metrics. - Keep task behavior idempotent and retry-safe; prefer explicit status/reason payloads in return dicts. diff --git a/qb_site/syncer/admin.py b/qb_site/syncer/admin.py index c5d0a322..a67d30b5 100644 --- a/qb_site/syncer/admin.py +++ b/qb_site/syncer/admin.py @@ -904,6 +904,7 @@ def short_after_sha(self, obj: PRTimelineEvent) -> str: # pragma: no cover - si "type", "occurred_at", "actor_login", + "actor_type", "label_name", "assignee_login", "requested_reviewer_login", @@ -911,13 +912,14 @@ def short_after_sha(self, obj: PRTimelineEvent) -> str: # pragma: no cover - si "short_before_sha", "short_after_sha", ) - list_filter = ("pull_request__repository", "type") + list_filter = ("pull_request__repository", "type", "actor_type") search_fields = ( "label_name", "pull_request__number", "before_sha", "after_sha", "actor_login", + "actor_node_id", "assignee_login", "requested_reviewer_login", "requested_team_slug", @@ -933,6 +935,8 @@ def short_after_sha(self, obj: PRTimelineEvent) -> str: # pragma: no cover - si "label_name", "assignee_login", "actor_login", + "actor_type", + "actor_node_id", "before_sha", "after_sha", "extra", @@ -1322,6 +1326,8 @@ class SyncerConvergenceSnapshotAdmin(ReadOnlyAdmin): "archive_completed", "archive_failed_permanent", "archive_resync_remaining", + "timeline_events_missing_actor_type", + "timeline_events_untyped_with_login", ) list_filter = ("repository", "history_cursor_completed", "discovery_continuation_active") date_hierarchy = "collected_at" @@ -1350,6 +1356,8 @@ class SyncerConvergenceSnapshotAdmin(ReadOnlyAdmin): "archive_completed", "archive_failed_permanent", "archive_resync_remaining", + "timeline_events_missing_actor_type", + "timeline_events_untyped_with_login", "created_at", ) diff --git a/qb_site/syncer/management/commands/backfill_timeline_actor_types.py b/qb_site/syncer/management/commands/backfill_timeline_actor_types.py new file mode 100644 index 00000000..d2d6ec94 --- /dev/null +++ b/qb_site/syncer/management/commands/backfill_timeline_actor_types.py @@ -0,0 +1,527 @@ +"""Backfill ``PRTimelineEvent.actor_type`` / ``actor_node_id`` (design doc 051). + +Resolves each stored timeline item by its own ``github_node_id`` through +GitHub's ``nodes(ids:)`` root field, which returns the same actor union the +timeline queries do. This is exact rather than heuristic — renamed accounts +resolve correctly and login reuse cannot mis-type anything — and it is roughly +1/20th the rate cost of a full schema-version rewalk wave. + +The same response carries ``login``, so archive-imported rows (whose legacy +fragment omitted the actor entirely) get their missing ``actor_login`` filled +in the same pass. That fill is guarded fill-only: a stored non-empty login is +the login *as of ingest time*, and clobbering it with today's login would +destroy the rename history ``actor_node_id`` exists to expose. +""" + +from __future__ import annotations + +import logging +import re +import time +from collections import Counter +from dataclasses import dataclass, field +from datetime import datetime, timezone as _tz +from typing import Any, Dict, List, Optional, Sequence + +import requests +from django.core.management.base import BaseCommand, CommandError + +from core.models import Repository +from syncer.models import PRTimelineEvent +from syncer.services.github_client import GitHubClient +from syncer.services.rate_budget import get_rate_snapshot +from syncer.services.sub.timeline_sync import actor_node_id_or_none, actor_type_or_none + +logger = logging.getLogger(__name__) + +# Tunables live here and on the argument parser rather than in settings: this +# is a one-shot operator command, and a `getattr(settings, ...)` with no +# matching os.getenv line in base.py would be a phantom setting (root AGENTS.md). +DEFAULT_BATCH_SIZE = 100 +# Headroom left for the live syncer, which shares this budget and only stops +# itself at SYNCER_RATE_REMAINING_MIN (200). 2500 matches the floor the doc-043 +# forced-resync drain settled on (ARCHIVE_RESYNC_MIN_RATE_REMAINING). +DEFAULT_MIN_RATE_REMAINING = 2500 +# Slack added to `resetAt` when sleeping, so we wake up after the window rolls. +RATE_RESET_SLACK_SECONDS = 15 +# Bounded retry for transient transport failures (5xx, connection resets, +# timeouts). A full drain is ~6 k calls, so meeting one is near-certain, and +# letting it propagate would abort the run before it printed its counters. +CALL_RETRY_ATTEMPTS = 3 +CALL_RETRY_BACKOFF_SECONDS = 2 + +# Marks an id whose call failed for a reason that says nothing about the id +# itself. Those rows stay untyped and a later run retries them; keeping them out +# of `unresolved` is what stops that count from reading as "these nodes are gone +# from GitHub" when it actually means "we never got an answer". +_CALL_FAILED = object() + + +def _is_rate_limit_error(message: str) -> bool: + """True for the GraphQL rejection GitHub sends once the budget is spent.""" + low = message.lower() + return "rate limit" in low or "rate_limited" in low + + +def _is_missing_node_error(message: str) -> bool: + """True when GitHub is telling us one specific id does not resolve.""" + low = message.lower() + return "could not resolve to a node" in low or "not a valid global id" in low + + +# GitHub names the offending id in the message: +# Could not resolve to a node with the global id of 'IC_kwDOFcwZ1c7vKdns'. +# Reading it back is what lets us drop the dead id and retry the rest, instead +# of bisecting the batch to rediscover what the error already told us. +_MISSING_ID_RE = re.compile(r"global id of '([^']+)'") + + +def named_missing_ids(message: str, candidates: Sequence[str]) -> List[str]: + """Return the ids in ``candidates`` that ``message`` names as unresolvable.""" + named = set(_MISSING_ID_RE.findall(message)) + return [nid for nid in candidates if nid in named] + + +@dataclass +class BackfillStats: + """Per-run counters. Every one of these is reported; none is decorative.""" + + scanned: int = 0 + typed: int = 0 + node_ids_filled: int = 0 + logins_filled: int = 0 + rows_written: int = 0 + # GitHub returned `actor: null` — permanent, no backfill route can type these. + null_actor: int = 0 + # The node id no longer resolves (hard-deleted comment/review, or a bad id). + unresolved: int = 0 + # The call carrying the row failed for a reason unrelated to the row itself, + # so we never learned anything about it. A later run retries these. + call_failed: int = 0 + # Retried calls. `api_calls` counts every attempt; this counts the re-tries + # among them, so a run fighting a flaky API is visible in the report. + retries: int = 0 + # Actor reported a typename outside PRActorType (e.g. Organization). The + # node id is still stored; actor_type stays NULL. + unmodelled_type: int = 0 + api_calls: int = 0 + # Sum of `rateLimit.cost` across responses — what the run actually spent, + # rather than api_calls x an assumed cost of 1. + points_spent: int = 0 + distribution: Counter = field(default_factory=Counter) + + def merge(self, other: "BackfillStats") -> None: + self.scanned += other.scanned + self.typed += other.typed + self.node_ids_filled += other.node_ids_filled + self.logins_filled += other.logins_filled + self.rows_written += other.rows_written + self.null_actor += other.null_actor + self.unresolved += other.unresolved + self.call_failed += other.call_failed + self.retries += other.retries + self.unmodelled_type += other.unmodelled_type + self.api_calls += other.api_calls + self.points_spent += other.points_spent + self.distribution.update(other.distribution) + + +def _node_actor(node: Any) -> Optional[Dict[str, Any]]: + """Return the acting account from one resolved node. + + Timeline event types carry ``actor``; ``IssueComment`` and + ``PullRequestReview`` carry ``author``. Either may be null. + """ + if not isinstance(node, dict): + return None + actor = node.get("actor") + if actor is None: + actor = node.get("author") + return actor if isinstance(actor, dict) else None + + +def _rate_snapshot_remaining(client: GitHubClient) -> tuple[Optional[int], Optional[str]]: + snap = get_rate_snapshot(getattr(client, "token_id", None)) or {} + remaining = snap.get("remaining") + reset_at = snap.get("resetAt") + return (remaining if isinstance(remaining, int) else None, reset_at if isinstance(reset_at, str) else None) + + +def _seconds_until(reset_at_iso: Optional[str]) -> int: + if not reset_at_iso: + return 60 + try: + reset_dt = datetime.fromisoformat(reset_at_iso.replace("Z", "+00:00")) + except ValueError: + return 60 + if reset_dt.tzinfo is None: + reset_dt = reset_dt.replace(tzinfo=_tz.utc) + delta = (reset_dt - datetime.now(_tz.utc)).total_seconds() + return max(1, int(delta) + RATE_RESET_SLACK_SECONDS) + + +class RateBudgetExhausted(Exception): + """Raised to unwind the drain when the cached rate snapshot is too low.""" + + +class Command(BaseCommand): + help = ( + "Backfill PRTimelineEvent.actor_type / actor_node_id by re-resolving each row's " + "stored github_node_id through GitHub's nodes(ids:) root field (design doc 051). " + "Also fills archive-imported rows' missing actor_login, guarded fill-only so " + "ingest-time logins and the rename history survive. " + "Resumable and idempotent: the target set is 'actor_type IS NULL', so an " + "interrupted run simply continues where it stopped. Note that rows whose actor " + "GitHub reports as null can never be typed, so repeat runs plateau at that " + "population rather than reaching zero — the reported null_actor count is that " + "floor. Rows whose call failed outright (transport error, or a GraphQL error we " + "cannot pin on an id) are reported as call_failed and left for a later run, so " + "one flaky call never ends the drain. Drip-feed with --limit, or use " + "--wait-for-rate for an unattended drain." + ) + + def add_arguments(self, parser): # type: ignore[override] + parser.add_argument("--repo", help="Limit to a single repository in owner/name format") + parser.add_argument( + "--batch-size", + type=int, + default=DEFAULT_BATCH_SIZE, + help=f"Node ids per nodes(ids:) call (default {DEFAULT_BATCH_SIZE}; GitHub caps this at {GitHubClient.NODES_IDS_MAX})", + ) + parser.add_argument( + "--limit", + type=int, + default=0, + help="Cap the number of rows resolved across all repositories (0 = no cap)", + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Resolve and report the actor-type distribution without writing (still spends GraphQL points)", + ) + parser.add_argument( + "--min-rate-remaining", + type=int, + default=DEFAULT_MIN_RATE_REMAINING, + help=( + f"Pause when the cached GraphQL rate snapshot drops below this (default {DEFAULT_MIN_RATE_REMAINING}). " + "The drain shares its budget with the live syncer." + ), + ) + parser.add_argument( + "--wait-for-rate", + action="store_true", + help="Sleep until resetAt instead of stopping when the rate floor is hit", + ) + + def handle(self, *args, **opts): # type: ignore[override] + # Last `rateLimit` block seen, so the run can report measured cost and + # the budget it leaves behind rather than an assumed per-call price. + self._last_rate: Optional[Dict[str, Any]] = None + repo_filter = self._resolve_repo(opts.get("repo")) + batch_size = max(1, min(int(opts.get("batch_size") or DEFAULT_BATCH_SIZE), GitHubClient.NODES_IDS_MAX)) + limit = max(0, int(opts.get("limit") or 0)) + dry_run = bool(opts.get("dry_run")) + min_rate = max(0, int(opts.get("min_rate_remaining") or 0)) + wait_for_rate = bool(opts.get("wait_for_rate")) + + # Rows we can never reach this way. Counted and reported rather than + # assumed to be zero; _extract_event_fields drops node-id-less events, + # so a non-zero count means something else wrote them. + no_node_id = self._base_queryset(repo_filter).filter(github_node_id__isnull=True).count() + if no_node_id: + self.stdout.write( + self.style.WARNING(f"{no_node_id} untyped row(s) have no github_node_id and cannot be backfilled this way.") + ) + + repos = self._target_repositories(repo_filter) + if not repos: + self.stdout.write(self.style.SUCCESS("Nothing to backfill.")) + return + + totals = BackfillStats() + stopped_on_rate = False + for repo in repos: + if limit and totals.scanned >= limit: + break + remaining_budget = (limit - totals.scanned) if limit else 0 + self.stdout.write(f"→ {repo.owner}/{repo.name}") + stats = BackfillStats() + try: + self._drain_repo( + repo, + stats, + batch_size=batch_size, + limit=remaining_budget, + dry_run=dry_run, + min_rate=min_rate, + wait_for_rate=wait_for_rate, + ) + except RateBudgetExhausted: + stopped_on_rate = True + finally: + # Merge unconditionally: a run cut short by the rate floor has + # already written its earlier batches, so its counters are real. + totals.merge(stats) + self._report(stats, prefix=" ", dry_run=dry_run) + if stopped_on_rate: + break + + self.stdout.write("") + self._report(totals, prefix="TOTAL ", dry_run=dry_run) + if self._last_rate: + self.stdout.write( + f"rate: remaining={self._last_rate.get('remaining')} used={self._last_rate.get('used')} " + f"resetAt={self._last_rate.get('resetAt')}" + ) + remaining = self._base_queryset(repo_filter).count() + self.stdout.write(f"{remaining} row(s) still have actor_type IS NULL.") + if stopped_on_rate: + hint = "" if wait_for_rate else " Pass --wait-for-rate for an unattended drain." + self.stdout.write( + self.style.WARNING( + "Stopped early: GraphQL rate budget exhausted (floor reached, or GitHub rejected the call). " + f"Re-run to continue.{hint}" + ) + ) + elif dry_run: + self.stdout.write(self.style.WARNING("Dry run: nothing was written.")) + + # ---- selection ------------------------------------------------------- + + def _resolve_repo(self, repo_opt: Optional[str]) -> Optional[Repository]: + if not repo_opt: + return None + if "/" not in repo_opt: + raise CommandError("--repo must be in the form owner/name") + owner, name = repo_opt.split("/", 1) + repo = Repository.objects.filter(owner=owner, name=name).first() + if repo is None: + raise CommandError(f"Repository not found: {repo_opt}") + return repo + + def _base_queryset(self, repo: Optional[Repository]): + qs = PRTimelineEvent.objects.filter(actor_type__isnull=True) + if repo is not None: + qs = qs.filter(pull_request__repository=repo) + return qs + + def _target_repositories(self, repo: Optional[Repository]) -> List[Repository]: + """Repositories with at least one backfillable row. + + Filtering on actual work matters even for an explicit ``--repo``: + constructing a client requires a token, so a no-op run would otherwise + fail with "GitHub token not found" instead of reporting nothing to do. + """ + if repo is not None: + has_work = self._base_queryset(repo).filter(github_node_id__isnull=False).exists() + return [repo] if has_work else [] + repo_ids = ( + self._base_queryset(None) + .filter(github_node_id__isnull=False) + .values_list("pull_request__repository_id", flat=True) + .distinct() + ) + return list(Repository.objects.filter(id__in=list(repo_ids)).order_by("owner", "name")) + + # ---- drain ----------------------------------------------------------- + + def _drain_repo( + self, + repo: Repository, + stats: BackfillStats, + *, + batch_size: int, + limit: int, + dry_run: bool, + min_rate: int, + wait_for_rate: bool, + ) -> None: + # Per-repo client so GitHub App operation tokens resolve correctly. + client = GitHubClient(operation="syncer_pr_read", owner=repo.owner, repo=repo.name) + qs = self._base_queryset(repo).filter(github_node_id__isnull=False).order_by("id") + + last_id = 0 + while True: + if limit and stats.scanned >= limit: + break + take = batch_size + if limit: + take = min(take, limit - stats.scanned) + rows = list(qs.filter(id__gt=last_id)[:take]) + if not rows: + break + last_id = rows[-1].pk + + self._gate_on_rate(client, min_rate=min_rate, wait_for_rate=wait_for_rate, stats=stats) + + resolved = self._resolve_ids(client, [r.github_node_id for r in rows if r.github_node_id], stats) + self._apply(rows, resolved, stats, dry_run=dry_run) + + def _gate_on_rate(self, client: GitHubClient, *, min_rate: int, wait_for_rate: bool, stats: BackfillStats) -> None: + if min_rate <= 0: + return + remaining, reset_at = _rate_snapshot_remaining(client) + if remaining is None or remaining >= min_rate: + return + if not wait_for_rate: + raise RateBudgetExhausted() + delay = _seconds_until(reset_at) + self.stdout.write(self.style.WARNING(f" rate remaining={remaining} < {min_rate}; sleeping {delay}s until reset")) + time.sleep(delay) + + def _resolve_ids(self, client: GitHubClient, ids: Sequence[str], stats: BackfillStats) -> Dict[str, Any]: + """Return ``{node_id: node}`` for ``ids``, or ``_CALL_FAILED`` per unasked id. + + Three failure shapes, handled differently because they mean different + things: + + - **Rate-limit rejection.** The cached snapshot the gate reads lied — + Redis unavailable, or the live syncer spent the window between our + check and this call. Retrying or splitting would only spend more, so + unwind to the resumable stop. + - **Transport failure** (5xx, reset, timeout). Retried with backoff; + if it persists, the batch is recorded as unasked and the drain moves + on to the next one. Deliberately *not* split: an outage would + otherwise fan one batch out into hundreds of sleeping calls. + - **A dead node id.** One unresolvable id makes GitHub reject the whole + call, poisoning a 100-id batch. GitHub names the id, so drop the + named ones and retry the remainder: 2 calls, against the 13 that + bisecting a 100-id batch spends to rediscover the same fact. Dropped + ids are simply absent from the result, which is what makes ``_apply`` + count them as unresolved. + - **Any other GraphQL error.** Nothing to parse, so fall back to + halving and recursing — every good id survives in at most log2(n) + extra calls and the bad one is isolated. + """ + if not ids: + return {} + node_ids = list(ids) + graphql_error: Optional[str] = None + transport_error: Optional[str] = None + + for attempt in range(1, CALL_RETRY_ATTEMPTS + 1): + try: + stats.api_calls += 1 + payload = client.get_timeline_actors_by_node_ids(ids=node_ids) + except RuntimeError as exc: + message = str(exc) + if _is_rate_limit_error(message): + raise RateBudgetExhausted() from exc + # A query-level error answers the same way next time; go split. + graphql_error = message + break + except requests.RequestException as exc: + transport_error = str(exc) + if attempt < CALL_RETRY_ATTEMPTS: + stats.retries += 1 + logger.warning("backfill_actor_types.transport_retry attempt=%s ids=%s error=%s", attempt, len(node_ids), exc) + time.sleep(CALL_RETRY_BACKOFF_SECONDS * attempt) + continue + else: + data = payload.get("data") or {} + rate = data.get("rateLimit") + if isinstance(rate, dict): + cost = rate.get("cost") + if isinstance(cost, int): + stats.points_spent += cost + self._last_rate = rate + nodes = data.get("nodes") or [] + return {n["id"]: n for n in nodes if isinstance(n, dict) and n.get("id")} + + if graphql_error is None: + logger.warning("backfill_actor_types.call_failed ids=%s error=%s", len(node_ids), transport_error) + return {nid: _CALL_FAILED for nid in node_ids} + + dead = named_missing_ids(graphql_error, node_ids) + if dead: + # One log line per batch, not per id: a dense run of deleted + # comments would otherwise bury everything else in the dyno log. + logger.warning( + "backfill_actor_types.unresolvable_node_ids count=%s of=%s ids=%s", + len(dead), + len(node_ids), + ",".join(dead[:5]) + ("…" if len(dead) > 5 else ""), + ) + survivors = [nid for nid in node_ids if nid not in set(dead)] + # The dead ids stay out of the returned mapping, so they land in the + # `unresolved` count exactly as if we had asked about them alone. + return self._resolve_ids(client, survivors, stats) + + if len(node_ids) > 1: + mid = len(node_ids) // 2 + left = self._resolve_ids(client, node_ids[:mid], stats) + left.update(self._resolve_ids(client, node_ids[mid:], stats)) + return left + + logger.warning("backfill_actor_types.node_id_rejected id=%s error=%s", node_ids[0], graphql_error) + # "Could not resolve" is a fact about the row: that node is gone. Any + # other query-level error is a fact about the call, so keep the two + # apart instead of reporting both as unresolved nodes. + if _is_missing_node_error(graphql_error): + return {} + return {node_ids[0]: _CALL_FAILED} + + def _apply(self, rows: List[PRTimelineEvent], resolved: Dict[str, Any], stats: BackfillStats, *, dry_run: bool) -> None: + to_update: List[PRTimelineEvent] = [] + for row in rows: + stats.scanned += 1 + node = resolved.get(row.github_node_id or "") + if node is _CALL_FAILED: + stats.call_failed += 1 + stats.distribution["(call failed)"] += 1 + continue + if node is None: + stats.unresolved += 1 + stats.distribution["(unresolved node)"] += 1 + continue + + actor = _node_actor(node) + if actor is None: + stats.null_actor += 1 + stats.distribution["(null actor)"] += 1 + continue + + a_type = actor_type_or_none(actor) + a_node_id = actor_node_id_or_none(actor) + login = actor.get("login") + stats.distribution[a_type or f"(unmodelled: {actor.get('__typename')})"] += 1 + if a_type is None: + stats.unmodelled_type += 1 + + changed = False + if a_type and row.actor_type is None: + row.actor_type = a_type + stats.typed += 1 + changed = True + if a_node_id and not row.actor_node_id: + row.actor_node_id = a_node_id + stats.node_ids_filled += 1 + changed = True + # Fill-only, and the predicate must cover both empties: the two + # extraction idioms in timeline_sync disagree on NULL vs "". + if login and not (row.actor_login or ""): + row.actor_login = str(login) + stats.logins_filled += 1 + changed = True + if changed: + to_update.append(row) + + stats.rows_written += len(to_update) + if to_update and not dry_run: + PRTimelineEvent.objects.bulk_update(to_update, ["actor_type", "actor_node_id", "actor_login"]) + + # ---- reporting ------------------------------------------------------- + + def _report(self, stats: BackfillStats, *, prefix: str, dry_run: bool = False) -> None: + wrote = "would_write" if dry_run else "written" + self.stdout.write( + f"{prefix}scanned={stats.scanned} {wrote}={stats.rows_written} typed={stats.typed} " + f"node_ids={stats.node_ids_filled} logins={stats.logins_filled} " + f"null_actor={stats.null_actor} unresolved={stats.unresolved} " + f"call_failed={stats.call_failed} unmodelled={stats.unmodelled_type} " + f"api_calls={stats.api_calls} retries={stats.retries} points={stats.points_spent}" + ) + if stats.distribution: + parts = ", ".join(f"{k}={v}" for k, v in sorted(stats.distribution.items(), key=lambda kv: (-kv[1], kv[0]))) + self.stdout.write(f"{prefix}distribution: {parts}") diff --git a/qb_site/syncer/migrations/0054_prtimelineevent_actor_type_and_node_id.py b/qb_site/syncer/migrations/0054_prtimelineevent_actor_type_and_node_id.py new file mode 100644 index 00000000..d022f977 --- /dev/null +++ b/qb_site/syncer/migrations/0054_prtimelineevent_actor_type_and_node_id.py @@ -0,0 +1,24 @@ +# Generated by Django 5.2.6 on 2026-08-15 05:33 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("syncer", "0053_syncerconvergencesnapshot_archive_resync_remaining"), + ] + + operations = [ + migrations.AddField( + model_name="prtimelineevent", + name="actor_node_id", + field=models.CharField(blank=True, max_length=255, null=True), + ), + migrations.AddField( + model_name="prtimelineevent", + name="actor_type", + field=models.CharField( + blank=True, choices=[("User", "user"), ("Bot", "bot"), ("Mannequin", "mannequin")], max_length=16, null=True + ), + ), + ] diff --git a/qb_site/syncer/migrations/0055_syncerconvergencesnapshot_actor_type_counters.py b/qb_site/syncer/migrations/0055_syncerconvergencesnapshot_actor_type_counters.py new file mode 100644 index 00000000..cfca2625 --- /dev/null +++ b/qb_site/syncer/migrations/0055_syncerconvergencesnapshot_actor_type_counters.py @@ -0,0 +1,22 @@ +# Generated by Django 5.2.6 on 2026-08-24 02:31 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("syncer", "0054_prtimelineevent_actor_type_and_node_id"), + ] + + operations = [ + migrations.AddField( + model_name="syncerconvergencesnapshot", + name="timeline_events_missing_actor_type", + field=models.IntegerField(default=0), + ), + migrations.AddField( + model_name="syncerconvergencesnapshot", + name="timeline_events_untyped_with_login", + field=models.IntegerField(default=0), + ), + ] diff --git a/qb_site/syncer/models/__init__.py b/qb_site/syncer/models/__init__.py index 2790bf6d..566d919f 100644 --- a/qb_site/syncer/models/__init__.py +++ b/qb_site/syncer/models/__init__.py @@ -3,7 +3,7 @@ from .pull_request import PullRequest, PullRequestState # noqa: F401 from .label_def import LabelDef # noqa: F401 from .pr_label import PRLabel # noqa: F401 -from .pr_timeline_event import PRTimelineEvent, PRTimelineEventType # noqa: F401 +from .pr_timeline_event import PRActorType, PRTimelineEvent, PRTimelineEventType # noqa: F401 from .pr_review_inline_comment import PRReviewInlineComment, PRReviewInlineCommentBackfill # noqa: F401 from .commit_check_run import CommitCheckRun # noqa: F401 from .commit_status_context import CommitStatusContext # noqa: F401 diff --git a/qb_site/syncer/models/convergence_snapshot.py b/qb_site/syncer/models/convergence_snapshot.py index 1eaf70e5..91ec3dac 100644 --- a/qb_site/syncer/models/convergence_snapshot.py +++ b/qb_site/syncer/models/convergence_snapshot.py @@ -60,6 +60,20 @@ class SyncerConvergenceSnapshot(models.Model): # enabled means ticks are being rate-skipped or the worker is backlogged. archive_resync_remaining = models.IntegerField(default=0) + # Actor typing drain (design doc 051), counted in timeline *events*, not PRs. + # ``timeline_events_missing_actor_type`` is the drain's exact target set — a + # resolvable node id and no ``actor_type`` — so it lines up with the + # backfill command's own per-repo output. It does **not** reach 0: GitHub + # returns a null actor for a real share of events, so it plateaus at that + # floor. Watch the trend, not the absolute value. + # ``timeline_events_untyped_with_login`` is the converging companion: a row + # carrying a login must have had an actor, so whatever is left here is + # typeable work. It should fall to ~0 during the drain and stay there. A + # later climb means ingestion stopped typing actors — most likely the + # fill-empty column allowlist in ``sync_timeline_events`` was dropped. + timeline_events_missing_actor_type = models.IntegerField(default=0) + timeline_events_untyped_with_login = models.IntegerField(default=0) + created_at = models.DateTimeField(auto_now_add=True) class Meta: diff --git a/qb_site/syncer/models/pr_timeline_event.py b/qb_site/syncer/models/pr_timeline_event.py index abb15047..caf76aee 100644 --- a/qb_site/syncer/models/pr_timeline_event.py +++ b/qb_site/syncer/models/pr_timeline_event.py @@ -26,6 +26,19 @@ class PRTimelineEventType(models.TextChoices): REVIEW_REQUEST_REMOVED = "REVIEW_REQUEST_REMOVED", "review_request_removed" +class PRActorType(models.TextChoices): + """GraphQL ``__typename`` of a timeline actor account. + + Values are GitHub's exact wire casing so they compare directly against + ``__typename`` (the ``requested_*`` routing in ``timeline_sync`` already + compares raw typenames). + """ + + USER = "User", "user" + BOT = "Bot", "bot" + MANNEQUIN = "Mannequin", "mannequin" + + class PRTimelineEvent(TimestampedModel): """Key timeline events for a PR used in status evolution analytics. @@ -52,6 +65,19 @@ class PRTimelineEvent(TimestampedModel): assignee_login = models.CharField(max_length=255, null=True, blank=True) # Present for ASSIGNED/UNASSIGNED events when available. actor_login = models.CharField(max_length=255, null=True, blank=True) + # GraphQL __typename of the acting account, as returned by the timeline + # queries. NULL means *unknown*, never "User": rows ingested before this + # column existed, archive-imported rows whose legacy fragment omits the + # actor entirely, and events where GitHub itself returns a null actor + # (workflow-driven label events routinely do) all land here. + # Note "Bot" identifies a GitHub App; machine accounts that are ordinary + # user accounts report "User", so this is necessary but not sufficient for + # "was this automation?". + actor_type = models.CharField(max_length=16, choices=PRActorType.choices, null=True, blank=True) + # GraphQL node id of the acting account. Stable across login renames, so + # downstream automation lists should key on this rather than actor_login. + # NULL under the same conditions as actor_type. + actor_node_id = models.CharField(max_length=255, null=True, blank=True) # Present only for HEAD_FORCE_PUSHED events; Git commit SHAs (40 chars) before_sha = models.CharField(max_length=40, null=True, blank=True) after_sha = models.CharField(max_length=40, null=True, blank=True) diff --git a/qb_site/syncer/queries/actor_types_by_node_ids.graphql b/qb_site/syncer/queries/actor_types_by_node_ids.graphql new file mode 100644 index 00000000..b5e119f2 --- /dev/null +++ b/qb_site/syncer/queries/actor_types_by_node_ids.graphql @@ -0,0 +1,44 @@ +# Re-resolve the acting account for stored timeline items, by node id. +# Variables: +# $ids: [ID!]! (GitHub caps `nodes(ids:)` at 100 per call) +# +# Used by the `backfill_timeline_actor_types` management command (design doc +# 051) to fill `PRTimelineEvent.actor_type` / `actor_node_id` — and the +# archive-imported rows' missing `actor_login` — on rows ingested before those +# columns existed. This is exact rather than heuristic: it resolves the actual +# actor object attached to each specific event, so renamed accounts resolve +# correctly and login reuse cannot mis-type anything. +# +# `nodes` takes a heterogeneous id list and returns `null` in place of any id +# that no longer resolves, so a hard-deleted comment does not fail the batch. + +query TimelineActorsByNodeIds($ids: [ID!]!) { + rateLimit { cost remaining resetAt used } + nodes(ids: $ids) { + __typename + id + # IssueComment and PullRequestReview expose `author` (via the Comment + # interface) rather than `actor`. This branch also covers the synthesized + # dismissed-review parent rows, whose stored node id *is* the review's. + ... on Comment { author { ...ActorIdentity } } + ... on LabeledEvent { actor { ...ActorIdentity } } + ... on UnlabeledEvent { actor { ...ActorIdentity } } + ... on AssignedEvent { actor { ...ActorIdentity } } + ... on UnassignedEvent { actor { ...ActorIdentity } } + ... on ReadyForReviewEvent { actor { ...ActorIdentity } } + ... on ConvertToDraftEvent { actor { ...ActorIdentity } } + ... on ReopenedEvent { actor { ...ActorIdentity } } + ... on ClosedEvent { actor { ...ActorIdentity } } + ... on HeadRefForcePushedEvent { actor { ...ActorIdentity } } + ... on ReviewDismissedEvent { actor { ...ActorIdentity } } + ... on ReviewRequestedEvent { actor { ...ActorIdentity } } + ... on ReviewRequestRemovedEvent { actor { ...ActorIdentity } } + } +} + +fragment ActorIdentity on Actor { + __typename + ... on User { id login } + ... on Bot { id login } + ... on Mannequin { id login } +} diff --git a/qb_site/syncer/queries/pr_bundle.graphql b/qb_site/syncer/queries/pr_bundle.graphql index 68a69d6d..58572633 100644 --- a/qb_site/syncer/queries/pr_bundle.graphql +++ b/qb_site/syncer/queries/pr_bundle.graphql @@ -128,64 +128,64 @@ query PRBundle($owner: String!, $name: String!, $number: Int!, $timelineK: Int!, ... on LabeledEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } label { name } } ... on UnlabeledEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } label { name } } ... on AssignedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } assignee { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } } ... on UnassignedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } assignee { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } } ... on ReadyForReviewEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on ConvertToDraftEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on ReopenedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on ClosedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on HeadRefForcePushedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } beforeCommit { oid } afterCommit { oid } } ... on IssueComment { id createdAt - author { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + author { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on PullRequestReview { id submittedAt state - author { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + author { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } comments(first: $inlineCommentsPerReview) { totalCount pageInfo { hasNextPage } @@ -204,13 +204,13 @@ query PRBundle($owner: String!, $name: String!, $number: Int!, $timelineK: Int!, id createdAt previousReviewState - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } - review { id submittedAt author { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } + review { id submittedAt author { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } } ... on ReviewRequestedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } requestedReviewer { __typename ... on User { login } @@ -222,7 +222,7 @@ query PRBundle($owner: String!, $name: String!, $number: Int!, $timelineK: Int!, ... on ReviewRequestRemovedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } requestedReviewer { __typename ... on User { login } diff --git a/qb_site/syncer/queries/timeline_page.graphql b/qb_site/syncer/queries/timeline_page.graphql index 80306326..d8153d37 100644 --- a/qb_site/syncer/queries/timeline_page.graphql +++ b/qb_site/syncer/queries/timeline_page.graphql @@ -36,64 +36,64 @@ query PRTimelinePage($owner: String!, $name: String!, $number: Int!, $first: Int ... on LabeledEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } label { name } } ... on UnlabeledEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } label { name } } ... on AssignedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } assignee { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } } ... on UnassignedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } assignee { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } } ... on ReadyForReviewEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on ConvertToDraftEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on ReopenedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on ClosedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on HeadRefForcePushedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } beforeCommit { oid } afterCommit { oid } } ... on IssueComment { id createdAt - author { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + author { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on PullRequestReview { id submittedAt state - author { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + author { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } comments(first: $inlineCommentsPerReview) { totalCount pageInfo { hasNextPage } @@ -112,13 +112,13 @@ query PRTimelinePage($owner: String!, $name: String!, $number: Int!, $first: Int id createdAt previousReviewState - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } - review { id submittedAt author { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } + review { id submittedAt author { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } } ... on ReviewRequestedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } requestedReviewer { __typename ... on User { login } @@ -130,7 +130,7 @@ query PRTimelinePage($owner: String!, $name: String!, $number: Int!, $first: Int ... on ReviewRequestRemovedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } requestedReviewer { __typename ... on User { login } diff --git a/qb_site/syncer/queries/timeline_page_back.graphql b/qb_site/syncer/queries/timeline_page_back.graphql index 422c9e04..ba49b4fe 100644 --- a/qb_site/syncer/queries/timeline_page_back.graphql +++ b/qb_site/syncer/queries/timeline_page_back.graphql @@ -34,64 +34,64 @@ query PRTimelinePageBack($owner: String!, $name: String!, $number: Int!, $last: ... on LabeledEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } label { name } } ... on UnlabeledEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } label { name } } ... on AssignedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } assignee { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } } ... on UnassignedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } assignee { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } } ... on ReadyForReviewEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on ConvertToDraftEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on ReopenedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on ClosedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on HeadRefForcePushedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } beforeCommit { oid } afterCommit { oid } } ... on IssueComment { id createdAt - author { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + author { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } ... on PullRequestReview { id submittedAt state - author { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + author { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } comments(first: $inlineCommentsPerReview) { totalCount pageInfo { hasNextPage } @@ -110,13 +110,13 @@ query PRTimelinePageBack($owner: String!, $name: String!, $number: Int!, $last: id createdAt previousReviewState - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } - review { id submittedAt author { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } + review { id submittedAt author { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } } } ... on ReviewRequestedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } requestedReviewer { __typename ... on User { login } @@ -128,7 +128,7 @@ query PRTimelinePageBack($owner: String!, $name: String!, $number: Int!, $last: ... on ReviewRequestRemovedEvent { id createdAt - actor { __typename ... on User { login } ... on Bot { login } ... on Mannequin { login } } + actor { __typename ... on User { id login } ... on Bot { id login } ... on Mannequin { id login } } requestedReviewer { __typename ... on User { login } diff --git a/qb_site/syncer/services/github_client.py b/qb_site/syncer/services/github_client.py index a066dd31..7ff9e6d9 100644 --- a/qb_site/syncer/services/github_client.py +++ b/qb_site/syncer/services/github_client.py @@ -195,6 +195,26 @@ def get_timeline_page_back( } return self.execute(query, variables) + NODES_IDS_MAX = 100 # GitHub's hard cap on `nodes(ids: [...])` + + def get_timeline_actors_by_node_ids( + self, + *, + ids: Sequence[str], + query_path: str = "qb_site/syncer/queries/actor_types_by_node_ids.graphql", + ) -> Dict[str, Any]: + """Re-resolve the acting account for stored timeline item node ids. + + Callers read ``data.nodes[]``, each carrying ``__typename``, ``id`` and + either ``actor`` or ``author`` (``null`` for ids that no longer + resolve). Used by the actor-type backfill (design doc 051). + """ + query = self._read_file(query_path) + node_ids = list(ids) + if len(node_ids) > self.NODES_IDS_MAX: + raise ValueError(f"nodes(ids:) accepts at most {self.NODES_IDS_MAX} ids, got {len(node_ids)}") + return self.execute(query, {"ids": node_ids}) + def get_last_rate_limit(self) -> Optional[Dict[str, Any]]: """Return the last seen rateLimit snapshot (if any).""" return self._last_rate_limit diff --git a/qb_site/syncer/services/sub/timeline_sync.py b/qb_site/syncer/services/sub/timeline_sync.py index 4b55cb20..aec5bdae 100644 --- a/qb_site/syncer/services/sub/timeline_sync.py +++ b/qb_site/syncer/services/sub/timeline_sync.py @@ -9,7 +9,7 @@ from analyzer.services.revisions import mark_pr_revision_dirty_if_earlier from syncer.models.pr_review_inline_comment import PRReviewInlineComment -from syncer.models.pr_timeline_event import PRTimelineEvent, PRTimelineEventType +from syncer.models.pr_timeline_event import PRActorType, PRTimelineEvent, PRTimelineEventType from syncer.models.pull_request import PullRequest logger = logging.getLogger(__name__) @@ -58,6 +58,41 @@ def _login_or_empty(actor: Any) -> str: return str(login) if login else "" +def actor_type_or_none(actor: Any) -> Optional[str]: + """Return ``actor.__typename`` when it is a known account kind, else ``None``. + + The allowed set is derived from ``PRActorType.values`` so the helper cannot + drift from the model's choices. An unmodelled typename (a future + ``Organization``, say) is dropped rather than stored raw: ``None`` means + "unknown", and that is exactly what an unrecognized kind is. + """ + if not isinstance(actor, dict): + return None + tn = actor.get("__typename") + return tn if tn in PRActorType.values else None + + +def actor_node_id_or_none(actor: Any) -> Optional[str]: + """Return the actor's GraphQL node id, or ``None`` when absent.""" + if not isinstance(actor, dict): + return None + nid = actor.get("id") + return str(nid) if nid else None + + +def _actor_identity(actor: Any) -> Dict[str, Optional[str]]: + """Return the ``actor_type`` / ``actor_node_id`` pair for one actor/author. + + Both are ``None`` for a null or absent actor. GitHub returns a null actor + for a real share of events (workflow-driven label changes especially), so + this is a normal outcome, not an error path. + """ + return { + "actor_type": actor_type_or_none(actor), + "actor_node_id": actor_node_id_or_none(actor), + } + + def _extract_event_fields(ev: Dict[str, Any]) -> Optional[Dict[str, Any]]: """Translate one ``timelineItems.nodes[]`` entry into row fields. @@ -104,6 +139,7 @@ def _extract_event_fields(ev: Dict[str, Any]) -> Optional[Dict[str, Any]]: occurred_at=occurred_at, actor_login=_login_or_empty(ev.get("author")), inline_comment_total_count=int(total) if isinstance(total, int) else 0, + **_actor_identity(ev.get("author")), ) return fields @@ -119,11 +155,14 @@ def _extract_event_fields(ev: Dict[str, Any]) -> Optional[Dict[str, Any]]: if typename in ("LabeledEvent", "UnlabeledEvent"): fields["label_name"] = (ev.get("label") or {}).get("name") fields["actor_login"] = (ev.get("actor") or {}).get("login") + fields.update(_actor_identity(ev.get("actor"))) elif typename in ("AssignedEvent", "UnassignedEvent"): fields["assignee_login"] = (ev.get("assignee") or {}).get("login") fields["actor_login"] = (ev.get("actor") or {}).get("login") + fields.update(_actor_identity(ev.get("actor"))) elif typename in ("ReadyForReviewEvent", "ConvertToDraftEvent", "ReopenedEvent", "ClosedEvent"): fields["actor_login"] = (ev.get("actor") or {}).get("login") + fields.update(_actor_identity(ev.get("actor"))) elif typename == "HeadRefForcePushedEvent": before_sha = (ev.get("beforeCommit") or {}).get("oid") after_sha = (ev.get("afterCommit") or {}).get("oid") @@ -138,22 +177,31 @@ def _extract_event_fields(ev: Dict[str, Any]) -> Optional[Dict[str, Any]]: fields["before_sha"] = before_sha fields["after_sha"] = after_sha fields["actor_login"] = (ev.get("actor") or {}).get("login") + fields.update(_actor_identity(ev.get("actor"))) elif typename == "IssueComment": fields["actor_login"] = _login_or_empty(ev.get("author")) + fields.update(_actor_identity(ev.get("author"))) elif typename == "ReviewDismissedEvent": # Actor is the dismisser, NEVER the dismissed review's author. Review # may be null when the underlying review has been hard-deleted; in # that case omit the dismissed_review_* fields. fields["actor_login"] = _login_or_empty(ev.get("actor")) + fields.update(_actor_identity(ev.get("actor"))) review = ev.get("review") if isinstance(ev.get("review"), dict) else None extra: Dict[str, Any] = {"previous_review_state": ev.get("previousReviewState")} if review is not None: extra["dismissed_review_node_id"] = review.get("id") extra["dismissed_review_author"] = _login_or_empty(review.get("author")) extra["dismissed_review_submitted_at"] = review.get("submittedAt") + # Denormalized so _synthesize_dismissed_review_parent can type the + # row it materializes; there is no other source for it, since the + # synthesized row is built entirely from this extra blob. + extra["dismissed_review_author_type"] = actor_type_or_none(review.get("author")) + extra["dismissed_review_author_node_id"] = actor_node_id_or_none(review.get("author")) fields["extra"] = extra elif typename in ("ReviewRequestedEvent", "ReviewRequestRemovedEvent"): fields["actor_login"] = _login_or_empty(ev.get("actor")) + fields.update(_actor_identity(ev.get("actor"))) rr = ev.get("requestedReviewer") or {} rr_typename = rr.get("__typename") if isinstance(rr, dict) else None if rr_typename == "Team": @@ -198,6 +246,12 @@ def _synthesize_dismissed_review_parent(pr: PullRequest, dismiss_extra: Dict[str previous_state = dismiss_extra.get("previous_review_state") submitted_at_iso = dismiss_extra.get("dismissed_review_submitted_at") author = dismiss_extra.get("dismissed_review_author") or "" + # Present only when the dismiss event was ingested under code that + # denormalizes them (design doc 051). Rows whose `extra` predates those + # keys synthesize with a null type/node id and are healed by the + # nodes(ids:) backfill instead. + author_type = dismiss_extra.get("dismissed_review_author_type") or None + author_node_id = dismiss_extra.get("dismissed_review_author_node_id") or None if not review_node_id or not previous_state or not submitted_at_iso: return (None, False) @@ -232,6 +286,8 @@ def _synthesize_dismissed_review_parent(pr: PullRequest, dismiss_extra: Dict[str "type": ev_type, "occurred_at": occurred_at, "actor_login": str(author), + "actor_type": author_type if author_type in PRActorType.values else None, + "actor_node_id": str(author_node_id) if author_node_id else None, # inline_comment_total_count starts NULL: we don't know the count # without seeing the actual PullRequestReview node. The CHECK # constraint allows null on review-submission types. If a later @@ -337,6 +393,8 @@ def sync_timeline_events( "label_name", "assignee_login", "actor_login", + "actor_type", + "actor_node_id", "before_sha", "after_sha", "requested_reviewer_login", diff --git a/qb_site/syncer/tasks/collect_convergence.py b/qb_site/syncer/tasks/collect_convergence.py index 5a004ce2..520a944a 100644 --- a/qb_site/syncer/tasks/collect_convergence.py +++ b/qb_site/syncer/tasks/collect_convergence.py @@ -8,6 +8,7 @@ from syncer.models import ( ArchiveImportItem, ArchiveImportItemStatus, + PRTimelineEvent, PullRequest, CommitHistoryHarvest, RepoBackfillCursor, @@ -105,6 +106,14 @@ def collect_syncer_convergence_task(self) -> dict: # type: ignore[no-redef] inconsistent_open = inconsistent_open_prs_queryset(repo).count() + # Doc-051 actor typing. The first count is the backfill command's target + # set and plateaus at the null-actor floor; the second only counts rows + # we know had an actor, so it converges and doubles as the standing + # canary for the fill-empty allowlist. + events = PRTimelineEvent.objects.filter(pull_request__repository=repo, actor_type__isnull=True) + events_missing_actor_type = events.filter(github_node_id__isnull=False).count() + events_untyped_with_login = events.exclude(actor_login__isnull=True).exclude(actor_login="").count() + SyncerConvergenceSnapshot.objects.create( repository=repo, collected_at=collected_at, @@ -128,6 +137,8 @@ def collect_syncer_convergence_task(self) -> dict: # type: ignore[no-redef] archive_completed=archive_completed, archive_failed_permanent=archive_failed_permanent, archive_resync_remaining=archive_resync_remaining, + timeline_events_missing_actor_type=events_missing_actor_type, + timeline_events_untyped_with_login=events_untyped_with_login, ) rows += 1 per_repo.append( @@ -160,6 +171,8 @@ def collect_syncer_convergence_task(self) -> dict: # type: ignore[no-redef] "archive_completed": archive_completed, "archive_failed_permanent": archive_failed_permanent, "archive_resync_remaining": archive_resync_remaining, + "timeline_events_missing_actor_type": events_missing_actor_type, + "timeline_events_untyped_with_login": events_untyped_with_login, } ) return {"repos": len(repos), "rows_created": rows, "per_repo": per_repo, "request_meta": request_meta} diff --git a/qb_site/syncer/tests/management/test_backfill_timeline_actor_types_cmd.py b/qb_site/syncer/tests/management/test_backfill_timeline_actor_types_cmd.py new file mode 100644 index 00000000..10646322 --- /dev/null +++ b/qb_site/syncer/tests/management/test_backfill_timeline_actor_types_cmd.py @@ -0,0 +1,496 @@ +"""Tests for the ``backfill_timeline_actor_types`` command (design doc 051).""" + +from __future__ import annotations + +from io import StringIO +from typing import Any, Dict, List, Sequence +from unittest import mock + +import requests +from django.core.management import call_command +from django.test import TestCase + +from syncer.models import PRActorType, PRTimelineEvent, PRTimelineEventType +from syncer.tests.factories import make_pr, make_repo + +CMD = "syncer.management.commands.backfill_timeline_actor_types" + + +def _actor(typename: str, node_id: str, login: str) -> Dict[str, Any]: + return {"__typename": typename, "id": node_id, "login": login} + + +class FakeClient: + """Stands in for GitHubClient; records every batch it is handed.""" + + NODES_IDS_MAX = 100 + + # Set by each test: node_id -> node dict (or None to make it unresolvable). + responses: Dict[str, Any] = {} + # node ids that make the whole call fail, forcing the batch-splitting path. + poison: set = set() + # GraphQL message the poison raises when `name_poison_ids` is off — the + # shape of GitHub's message minus the id, i.e. nothing to parse. + poison_message: str = "GraphQL error(s): Could not resolve to a node with the global id" + # When on, the error names each dead id the way GitHub really does. + name_poison_ids: bool = False + # Number of leading calls that die at the transport layer (5xx / reset). + transport_failures: int = 0 + # Make every call come back as GitHub's rate-limit rejection. + rate_limited: bool = False + + def __init__(self, **kwargs: Any) -> None: + self.token_id = "fake-token" + FakeClient.calls.append(kwargs) + + calls: List[Dict[str, Any]] = [] + batches: List[List[str]] = [] + + @classmethod + def reset(cls) -> None: + cls.responses = {} + cls.poison = set() + cls.poison_message = "GraphQL error(s): Could not resolve to a node with the global id" + cls.name_poison_ids = False + cls.transport_failures = 0 + cls.rate_limited = False + cls.calls = [] + cls.batches = [] + + def get_timeline_actors_by_node_ids(self, *, ids: Sequence[str]) -> Dict[str, Any]: + ids = list(ids) + FakeClient.batches.append(ids) + if len(ids) > self.NODES_IDS_MAX: + raise ValueError("batch exceeded the nodes(ids:) cap") + if FakeClient.rate_limited: + raise RuntimeError("GraphQL error(s): API rate limit exceeded for user ID 1") + if FakeClient.transport_failures > 0: + FakeClient.transport_failures -= 1 + raise requests.ConnectionError("502 Server Error: Bad Gateway") + dead = FakeClient.poison & set(ids) + if dead: + if FakeClient.name_poison_ids: + named = "; ".join(f"Could not resolve to a node with the global id of '{i}'." for i in ids if i in dead) + raise RuntimeError(f"GraphQL error(s): {named}") + raise RuntimeError(FakeClient.poison_message) + nodes = [FakeClient.responses.get(i) for i in ids] + return {"data": {"rateLimit": {"cost": 1, "remaining": 4999}, "nodes": nodes}} + + +class BackfillCommandTestBase(TestCase): + def setUp(self) -> None: + FakeClient.reset() + self.repo = make_repo(owner="leanprover-community", name="mathlib4") + self.pr = make_pr(self.repo, 1) + + def _row(self, node_id: str, **kwargs: Any) -> PRTimelineEvent: + defaults: Dict[str, Any] = { + "pull_request": self.pr, + "github_node_id": node_id, + "type": PRTimelineEventType.LABELED, + "occurred_at": "2025-01-01T00:00:00Z", + } + defaults.update(kwargs) + return PRTimelineEvent.objects.create(**defaults) + + def _run(self, **opts: Any) -> str: + out = StringIO() + with mock.patch(f"{CMD}.GitHubClient", FakeClient): + call_command("backfill_timeline_actor_types", stdout=out, **opts) + return out.getvalue() + + +class TestBackfillResolution(BackfillCommandTestBase): + def test_types_rows_from_resolved_actors(self) -> None: + self._row("TL_BOT", actor_login="mathlib-merge-conflicts") + self._row("TL_USER", actor_login="alice") + FakeClient.responses = { + "TL_BOT": { + "__typename": "LabeledEvent", + "id": "TL_BOT", + "actor": _actor("Bot", "BOT_kgDOD2_IkQ", "mathlib-merge-conflicts"), + }, + "TL_USER": { + "__typename": "LabeledEvent", + "id": "TL_USER", + "actor": _actor("User", "U_kgDOAlice", "alice"), + }, + } + self._run() + + bot = PRTimelineEvent.objects.get(github_node_id="TL_BOT") + self.assertEqual(bot.actor_type, PRActorType.BOT) + self.assertEqual(bot.actor_node_id, "BOT_kgDOD2_IkQ") + user = PRTimelineEvent.objects.get(github_node_id="TL_USER") + self.assertEqual(user.actor_type, PRActorType.USER) + self.assertEqual(user.actor_node_id, "U_kgDOAlice") + + def test_author_bearing_nodes_are_read_too(self) -> None: + # IssueComment / PullRequestReview carry `author`, not `actor`. The + # synthesized dismissed-review parents land here as well, since their + # stored node id is the review's. + self._row("REV_1", type=PRTimelineEventType.REVIEW_APPROVED, actor_login="alice") + FakeClient.responses = { + "REV_1": { + "__typename": "PullRequestReview", + "id": "REV_1", + "author": _actor("User", "U_kgDOAlice", "alice"), + } + } + self._run() + self.assertEqual(PRTimelineEvent.objects.get(github_node_id="REV_1").actor_type, PRActorType.USER) + + def test_null_actor_stays_untyped(self) -> None: + # Workflow-driven label events genuinely have no actor. This is the + # permanent floor the drain plateaus at, not a failure. + self._row("TL_NULL") + FakeClient.responses = {"TL_NULL": {"__typename": "LabeledEvent", "id": "TL_NULL", "actor": None}} + out = self._run() + row = PRTimelineEvent.objects.get(github_node_id="TL_NULL") + self.assertIsNone(row.actor_type) + self.assertIsNone(row.actor_node_id) + self.assertIn("null_actor=1", out) + + def test_unresolvable_node_is_counted_not_crashed(self) -> None: + self._row("TL_GONE") + FakeClient.responses = {"TL_GONE": None} + out = self._run() + self.assertIsNone(PRTimelineEvent.objects.get(github_node_id="TL_GONE").actor_type) + self.assertIn("unresolved=1", out) + + def test_unmodelled_actor_type_stores_node_id_only(self) -> None: + self._row("TL_ORG") + FakeClient.responses = { + "TL_ORG": { + "__typename": "ClosedEvent", + "id": "TL_ORG", + "actor": _actor("Organization", "O_1", "leanprover-community"), + } + } + out = self._run() + row = PRTimelineEvent.objects.get(github_node_id="TL_ORG") + self.assertIsNone(row.actor_type) + self.assertEqual(row.actor_node_id, "O_1") + self.assertIn("unmodelled=1", out) + + def test_rows_without_node_id_are_reported(self) -> None: + self._row("TL_OK") + PRTimelineEvent.objects.create( + pull_request=self.pr, + github_node_id=None, + type=PRTimelineEventType.LABELED, + occurred_at="2025-01-01T00:00:00Z", + ) + FakeClient.responses = {"TL_OK": {"__typename": "LabeledEvent", "id": "TL_OK", "actor": _actor("User", "U_1", "alice")}} + out = self._run() + self.assertIn("1 untyped row(s) have no github_node_id", out) + + +class TestBackfillWriteGuards(BackfillCommandTestBase): + def test_already_typed_rows_are_not_re_resolved(self) -> None: + self._row("TL_DONE", actor_login="alice", actor_type=PRActorType.USER, actor_node_id="U_1") + out = self._run() + # Not in the target set at all: no client and no batch were ever made. + self.assertEqual(FakeClient.batches, []) + self.assertEqual(FakeClient.calls, []) + self.assertIn("Nothing to backfill", out) + + def test_is_idempotent(self) -> None: + self._row("TL_1", actor_login="alice") + FakeClient.responses = {"TL_1": {"__typename": "LabeledEvent", "id": "TL_1", "actor": _actor("User", "U_1", "alice")}} + self._run() + first = PRTimelineEvent.objects.get(github_node_id="TL_1") + batches_after_first = len(FakeClient.batches) + + self._run() + second = PRTimelineEvent.objects.get(github_node_id="TL_1") + self.assertEqual( + (first.actor_type, first.actor_node_id, first.actor_login), + (second.actor_type, second.actor_node_id, second.actor_login), + ) + # The second run had nothing to do. + self.assertEqual(len(FakeClient.batches), batches_after_first) + + def test_fills_missing_actor_login_for_null_and_empty(self) -> None: + # Archive-imported rows have NULL actor_login; the _login_or_empty + # idiom writes "". Both are "missing" and both must be filled. + self._row("TL_NULL_LOGIN", actor_login=None) + self._row("TL_EMPTY_LOGIN", actor_login="") + FakeClient.responses = { + "TL_NULL_LOGIN": { + "__typename": "LabeledEvent", + "id": "TL_NULL_LOGIN", + "actor": _actor("Bot", "BOT_1", "mathlib-bors"), + }, + "TL_EMPTY_LOGIN": { + "__typename": "LabeledEvent", + "id": "TL_EMPTY_LOGIN", + "actor": _actor("Bot", "BOT_1", "mathlib-bors"), + }, + } + out = self._run() + self.assertEqual(PRTimelineEvent.objects.get(github_node_id="TL_NULL_LOGIN").actor_login, "mathlib-bors") + self.assertEqual(PRTimelineEvent.objects.get(github_node_id="TL_EMPTY_LOGIN").actor_login, "mathlib-bors") + self.assertIn("logins=2", out) + + def test_never_overwrites_an_existing_login_even_after_a_rename(self) -> None: + # The stored login is the login as of ingest time. Clobbering it with + # today's login would destroy exactly the history actor_node_id exists + # to expose. + self._row("TL_RENAMED", actor_login="mathlib4-merge-conflict-bot") + FakeClient.responses = { + "TL_RENAMED": { + "__typename": "LabeledEvent", + "id": "TL_RENAMED", + "actor": _actor("User", "U_kgDODVl3LA", "some-new-login"), + } + } + self._run() + row = PRTimelineEvent.objects.get(github_node_id="TL_RENAMED") + self.assertEqual(row.actor_login, "mathlib4-merge-conflict-bot") + self.assertEqual(row.actor_node_id, "U_kgDODVl3LA") + self.assertEqual(row.actor_type, PRActorType.USER) + + def test_dry_run_labels_its_write_count_as_hypothetical(self) -> None: + # These reports get pasted into runbooks; "written" on a dry run reads + # as a claim that rows changed. + self._row("TL_DRY_LABEL", actor_login="alice") + FakeClient.responses = { + "TL_DRY_LABEL": {"__typename": "LabeledEvent", "id": "TL_DRY_LABEL", "actor": _actor("User", "U_1", "alice")} + } + out = self._run(dry_run=True) + self.assertIn("would_write=1", out) + self.assertNotIn("written=", out) + + def test_dry_run_resolves_but_writes_nothing(self) -> None: + self._row("TL_DRY") + FakeClient.responses = { + "TL_DRY": {"__typename": "LabeledEvent", "id": "TL_DRY", "actor": _actor("Bot", "BOT_1", "mathlib-bors")} + } + out = self._run(dry_run=True) + row = PRTimelineEvent.objects.get(github_node_id="TL_DRY") + self.assertIsNone(row.actor_type) + self.assertIsNone(row.actor_node_id) + self.assertIn("Bot=1", out) + self.assertIn("Dry run", out) + + +class TestBackfillBatching(BackfillCommandTestBase): + def test_batches_at_the_hundred_id_cap(self) -> None: + for i in range(250): + node_id = f"TL_{i:03d}" + self._row(node_id) + FakeClient.responses[node_id] = { + "__typename": "LabeledEvent", + "id": node_id, + "actor": _actor("User", f"U_{i}", "alice"), + } + self._run() + self.assertEqual([len(b) for b in FakeClient.batches], [100, 100, 50]) + self.assertEqual(PRTimelineEvent.objects.filter(actor_type__isnull=True).count(), 0) + + def test_limit_caps_rows_resolved(self) -> None: + for i in range(20): + node_id = f"TL_{i:03d}" + self._row(node_id) + FakeClient.responses[node_id] = { + "__typename": "LabeledEvent", + "id": node_id, + "actor": _actor("User", f"U_{i}", "alice"), + } + self._run(limit=5, batch_size=3) + self.assertEqual([len(b) for b in FakeClient.batches], [3, 2]) + self.assertEqual(PRTimelineEvent.objects.filter(actor_type__isnull=False).count(), 5) + + def test_named_dead_ids_are_dropped_without_bisecting(self) -> None: + # GitHub names the unresolvable id, so re-deriving it by halving the + # batch costs 13 calls where 2 will do. On a 600 k-row drain with even + # 0.25 % deleted comments that difference is ~18 k GraphQL points. + for i in range(4): + node_id = f"TL_{i}" + self._row(node_id) + FakeClient.responses[node_id] = { + "__typename": "IssueComment", + "id": node_id, + "author": _actor("User", f"U_{i}", "alice"), + } + FakeClient.poison = {"TL_2"} + FakeClient.name_poison_ids = True + + out = self._run(batch_size=4) + self.assertEqual(PRTimelineEvent.objects.filter(actor_type__isnull=False).count(), 3) + self.assertIsNone(PRTimelineEvent.objects.get(github_node_id="TL_2").actor_type) + # One rejected call naming TL_2, then one clean call for the other three. + self.assertEqual([len(b) for b in FakeClient.batches], [4, 3]) + # Dropped, not merely unasked: it is a fact about the row. + self.assertIn("unresolved=1", out) + self.assertIn("call_failed=0", out) + + def test_all_named_dead_ids_are_dropped_in_one_retry(self) -> None: + for i in range(5): + node_id = f"TL_{i}" + self._row(node_id) + FakeClient.responses[node_id] = { + "__typename": "LabeledEvent", + "id": node_id, + "actor": _actor("Bot", f"BOT_{i}", "mathlib-bors"), + } + FakeClient.poison = {"TL_1", "TL_3"} + FakeClient.name_poison_ids = True + + out = self._run(batch_size=5) + self.assertEqual([len(b) for b in FakeClient.batches], [5, 3]) + self.assertIn("unresolved=2", out) + + def test_unparseable_graphql_error_falls_back_to_splitting(self) -> None: + for i in range(4): + node_id = f"TL_{i}" + self._row(node_id) + FakeClient.responses[node_id] = { + "__typename": "LabeledEvent", + "id": node_id, + "actor": _actor("User", f"U_{i}", "alice"), + } + FakeClient.poison = {"TL_2"} + + self._run(batch_size=4) + # The three good ids still resolved; only the poisoned one was dropped. + self.assertEqual(PRTimelineEvent.objects.filter(actor_type__isnull=False).count(), 3) + self.assertIsNone(PRTimelineEvent.objects.get(github_node_id="TL_2").actor_type) + # 1 failed call of 4, then halves (2 + 2), then the failing half's singles. + self.assertEqual([len(b) for b in FakeClient.batches], [4, 2, 2, 1, 1]) + + +class TestBackfillRateGating(BackfillCommandTestBase): + def test_stops_when_rate_snapshot_is_below_the_floor(self) -> None: + self._row("TL_RATE") + FakeClient.responses = { + "TL_RATE": {"__typename": "LabeledEvent", "id": "TL_RATE", "actor": _actor("User", "U_1", "alice")} + } + with mock.patch(f"{CMD}.get_rate_snapshot", return_value={"remaining": 10, "resetAt": None}): + out = self._run(min_rate_remaining=500) + self.assertEqual(FakeClient.batches, []) + self.assertIsNone(PRTimelineEvent.objects.get(github_node_id="TL_RATE").actor_type) + self.assertIn("rate budget exhausted", out) + + def test_runs_when_snapshot_is_healthy(self) -> None: + self._row("TL_RATE_OK") + FakeClient.responses = { + "TL_RATE_OK": {"__typename": "LabeledEvent", "id": "TL_RATE_OK", "actor": _actor("User", "U_1", "alice")} + } + with mock.patch(f"{CMD}.get_rate_snapshot", return_value={"remaining": 4000, "resetAt": None}): + self._run(min_rate_remaining=500) + self.assertEqual(PRTimelineEvent.objects.get(github_node_id="TL_RATE_OK").actor_type, PRActorType.USER) + + +class TestBackfillCallFailures(BackfillCommandTestBase): + """A ~6 k-call drain meets a flaky API; none of it may end the run.""" + + def _row_with_response(self, node_id: str) -> None: + self._row(node_id) + FakeClient.responses[node_id] = { + "__typename": "LabeledEvent", + "id": node_id, + "actor": _actor("User", f"U_{node_id}", "alice"), + } + + def test_transport_failure_is_retried_and_then_succeeds(self) -> None: + self._row_with_response("TL_FLAKY") + FakeClient.transport_failures = 1 + with mock.patch(f"{CMD}.time.sleep") as sleep: + out = self._run() + self.assertEqual(PRTimelineEvent.objects.get(github_node_id="TL_FLAKY").actor_type, PRActorType.USER) + self.assertEqual([len(b) for b in FakeClient.batches], [1, 1]) + self.assertIn("retries=1", out) + sleep.assert_called_once() + + def test_persistent_transport_failure_is_counted_and_never_split(self) -> None: + # Splitting an outage would fan one batch out into hundreds of + # sleeping calls, so the batch is recorded unasked and the drain + # moves on. The rows stay untyped for a later run. + for i in range(4): + self._row_with_response(f"TL_DOWN_{i}") + FakeClient.transport_failures = 99 + with mock.patch(f"{CMD}.time.sleep"): + out = self._run(batch_size=4) + self.assertEqual(PRTimelineEvent.objects.filter(actor_type__isnull=False).count(), 0) + # Three attempts at the same batch of 4 — no halving. + self.assertEqual([len(b) for b in FakeClient.batches], [4, 4, 4]) + self.assertIn("call_failed=4", out) + # And not misreported as nodes GitHub no longer has. + self.assertIn("unresolved=0", out) + + def test_api_rate_limit_rejection_stops_the_drain(self) -> None: + # The gate reads a cached snapshot, so it can be stale. A live + # rejection must unwind rather than retry or split — both would only + # spend more of a budget that is already gone. + self._row_with_response("TL_LIMITED") + FakeClient.rate_limited = True + out = self._run() + self.assertEqual([len(b) for b in FakeClient.batches], [1]) + self.assertIsNone(PRTimelineEvent.objects.get(github_node_id="TL_LIMITED").actor_type) + self.assertIn("rate budget exhausted", out) + + def test_unattributable_graphql_error_is_not_counted_as_unresolved(self) -> None: + # GitHub's "something went wrong" is a fact about the call, not about + # the row; only "could not resolve" means the node is really gone. + self._row_with_response("TL_SHRUG") + FakeClient.poison = {"TL_SHRUG"} + FakeClient.poison_message = "GraphQL error(s): Something went wrong while executing your query" + out = self._run() + self.assertIsNone(PRTimelineEvent.objects.get(github_node_id="TL_SHRUG").actor_type) + self.assertIn("call_failed=1", out) + self.assertIn("unresolved=0", out) + + +class TestBackfillRepoScoping(BackfillCommandTestBase): + def test_repo_filter_limits_the_target_set(self) -> None: + other_repo = make_repo(owner="leanprover-community", name="batteries") + other_pr = make_pr(other_repo, 7) + self._row("TL_MATHLIB") + PRTimelineEvent.objects.create( + pull_request=other_pr, + github_node_id="TL_OTHER", + type=PRTimelineEventType.LABELED, + occurred_at="2025-01-01T00:00:00Z", + ) + FakeClient.responses = { + "TL_MATHLIB": { + "__typename": "LabeledEvent", + "id": "TL_MATHLIB", + "actor": _actor("User", "U_1", "alice"), + }, + "TL_OTHER": {"__typename": "LabeledEvent", "id": "TL_OTHER", "actor": _actor("User", "U_2", "bob")}, + } + self._run(repo="leanprover-community/mathlib4") + self.assertEqual(PRTimelineEvent.objects.get(github_node_id="TL_MATHLIB").actor_type, PRActorType.USER) + self.assertIsNone(PRTimelineEvent.objects.get(github_node_id="TL_OTHER").actor_type) + + def test_explicit_repo_with_no_work_builds_no_client(self) -> None: + # Constructing a client needs a token, so a no-op run must not reach + # for one — it should just say there is nothing to do. + self._row("TL_DONE", actor_type=PRActorType.USER, actor_node_id="U_1") + out = self._run(repo="leanprover-community/mathlib4") + self.assertEqual(FakeClient.calls, []) + self.assertIn("Nothing to backfill", out) + + def test_reports_measured_point_cost_and_remaining_budget(self) -> None: + # The whole point of a --limit test run is to see what it cost, so the + # spend comes from each response's rateLimit block rather than from + # api_calls times an assumed price. + self._row("TL_COST") + FakeClient.responses = { + "TL_COST": {"__typename": "LabeledEvent", "id": "TL_COST", "actor": _actor("User", "U_1", "alice")} + } + out = self._run() + self.assertIn("points=1", out) + self.assertIn("rate: remaining=4999", out) + + def test_client_is_constructed_per_repository(self) -> None: + self._row("TL_A") + FakeClient.responses = {"TL_A": {"__typename": "LabeledEvent", "id": "TL_A", "actor": _actor("User", "U_1", "alice")}} + self._run() + self.assertEqual( + FakeClient.calls, + [{"operation": "syncer_pr_read", "owner": "leanprover-community", "repo": "mathlib4"}], + ) diff --git a/qb_site/syncer/tests/services/test_timeline_actor_type.py b/qb_site/syncer/tests/services/test_timeline_actor_type.py new file mode 100644 index 00000000..19411a84 --- /dev/null +++ b/qb_site/syncer/tests/services/test_timeline_actor_type.py @@ -0,0 +1,267 @@ +"""Timeline actor typing: ``actor_type`` / ``actor_node_id`` (design doc 051). + +Covers the extraction helpers, one case per ``__typename`` branch that sets +``actor_login``, the fill-empty update path, and the synthesized +dismissed-review parent. +""" + +from __future__ import annotations + +from django.test import TestCase + +from syncer.models import PRActorType, PRTimelineEvent, PRTimelineEventType +from syncer.services.sub.timeline_sync import ( + actor_node_id_or_none, + actor_type_or_none, + _extract_event_fields, + _synthesize_dismissed_review_parent, + sync_timeline_events, +) +from syncer.tests.factories import make_pr, make_repo + +USER = {"__typename": "User", "id": "U_kgDOAlice", "login": "alice"} +BOT = {"__typename": "Bot", "id": "BOT_kgDOBors", "login": "mathlib-bors"} +MANNEQUIN = {"__typename": "Mannequin", "id": "MNQ_kgDOGhost", "login": "ghost"} + + +def _ev(typename: str, node_id: str, **extra): + base = {"__typename": typename, "id": node_id, "createdAt": "2025-01-01T00:00:00Z"} + base.update(extra) + return base + + +class TestActorHelpers(TestCase): + def test_known_typenames_pass_through(self) -> None: + self.assertEqual(actor_type_or_none(USER), "User") + self.assertEqual(actor_type_or_none(BOT), "Bot") + self.assertEqual(actor_type_or_none(MANNEQUIN), "Mannequin") + + def test_unknown_typename_is_dropped_not_stored_raw(self) -> None: + # A future union member (Organization, EnterpriseUserAccount, …) must + # not land in the column: NULL means unknown, and that is what it is. + self.assertIsNone(actor_type_or_none({"__typename": "Organization", "id": "O_1", "login": "org"})) + + def test_null_and_non_dict_actors(self) -> None: + for actor in (None, "", [], {}): + self.assertIsNone(actor_type_or_none(actor)) + self.assertIsNone(actor_node_id_or_none(actor)) + + def test_node_id_is_stringified_and_empty_is_none(self) -> None: + self.assertEqual(actor_node_id_or_none({"__typename": "User", "id": 12345}), "12345") + self.assertIsNone(actor_node_id_or_none({"__typename": "User", "id": None})) + self.assertIsNone(actor_node_id_or_none({"__typename": "User"})) + + +class TestExtractEventFieldsActorIdentity(TestCase): + """One case per branch of ``_extract_event_fields`` that sets actor_login.""" + + def _assert_identity(self, ev: dict, *, login: str, actor_type: str, node_id: str) -> None: + fields = _extract_event_fields(ev) + assert fields is not None + self.assertEqual(fields["actor_login"], login) + self.assertEqual(fields["actor_type"], actor_type) + self.assertEqual(fields["actor_node_id"], node_id) + + def test_labeled_and_unlabeled(self) -> None: + for tn in ("LabeledEvent", "UnlabeledEvent"): + self._assert_identity( + _ev(tn, "N1", actor=BOT, label={"name": "ready-to-merge"}), + login="mathlib-bors", + actor_type="Bot", + node_id="BOT_kgDOBors", + ) + + def test_assigned_and_unassigned(self) -> None: + for tn in ("AssignedEvent", "UnassignedEvent"): + self._assert_identity( + _ev(tn, "N2", actor=USER, assignee=USER), + login="alice", + actor_type="User", + node_id="U_kgDOAlice", + ) + + def test_draft_and_state_flips(self) -> None: + for tn in ("ReadyForReviewEvent", "ConvertToDraftEvent", "ReopenedEvent", "ClosedEvent"): + self._assert_identity(_ev(tn, "N3", actor=MANNEQUIN), login="ghost", actor_type="Mannequin", node_id="MNQ_kgDOGhost") + + def test_force_push(self) -> None: + self._assert_identity( + _ev( + "HeadRefForcePushedEvent", + "N4", + actor=USER, + beforeCommit={"oid": "a" * 40}, + afterCommit={"oid": "b" * 40}, + ), + login="alice", + actor_type="User", + node_id="U_kgDOAlice", + ) + + def test_issue_comment_uses_author(self) -> None: + self._assert_identity( + _ev("IssueComment", "N5", author=BOT), login="mathlib-bors", actor_type="Bot", node_id="BOT_kgDOBors" + ) + + def test_pull_request_review_uses_author(self) -> None: + ev = { + "__typename": "PullRequestReview", + "id": "N6", + "state": "APPROVED", + "submittedAt": "2025-01-01T00:00:00Z", + "author": USER, + "comments": {"totalCount": 0}, + } + self._assert_identity(ev, login="alice", actor_type="User", node_id="U_kgDOAlice") + + def test_review_dismissed_types_the_dismisser(self) -> None: + ev = _ev( + "ReviewDismissedEvent", + "N7", + previousReviewState="CHANGES_REQUESTED", + actor=USER, + review={"id": "REV_1", "submittedAt": "2024-12-31T00:00:00Z", "author": BOT}, + ) + self._assert_identity(ev, login="alice", actor_type="User", node_id="U_kgDOAlice") + fields = _extract_event_fields(ev) + assert fields is not None + # …and denormalizes the *dismissed review's* author separately. + self.assertEqual(fields["extra"]["dismissed_review_author"], "mathlib-bors") + self.assertEqual(fields["extra"]["dismissed_review_author_type"], "Bot") + self.assertEqual(fields["extra"]["dismissed_review_author_node_id"], "BOT_kgDOBors") + + def test_review_requested_and_removed(self) -> None: + for tn in ("ReviewRequestedEvent", "ReviewRequestRemovedEvent"): + self._assert_identity( + _ev(tn, "N8", actor=USER, requestedReviewer={"__typename": "User", "login": "bob"}), + login="alice", + actor_type="User", + node_id="U_kgDOAlice", + ) + + def test_null_actor_yields_none_not_user(self) -> None: + # GitHub returns actor: null for workflow-driven label events. This is + # a permanent population, not a transient gap. + fields = _extract_event_fields(_ev("LabeledEvent", "N9", actor=None, label={"name": "delegated"})) + assert fields is not None + self.assertIsNone(fields["actor_type"]) + self.assertIsNone(fields["actor_node_id"]) + + def test_absent_actor_field_yields_none(self) -> None: + # The legacy archive fragment omits `actor` entirely. + fields = _extract_event_fields(_ev("LabeledEvent", "N10", label={"name": "WIP"})) + assert fields is not None + self.assertIsNone(fields["actor_type"]) + self.assertIsNone(fields["actor_node_id"]) + + def test_unknown_actor_typename_is_not_stored(self) -> None: + actor = {"__typename": "Organization", "id": "O_1", "login": "leanprover-community"} + fields = _extract_event_fields(_ev("ClosedEvent", "N11", actor=actor)) + assert fields is not None + self.assertEqual(fields["actor_login"], "leanprover-community") + self.assertIsNone(fields["actor_type"]) + # The node id is still exact and useful even when the kind is unmodelled. + self.assertEqual(fields["actor_node_id"], "O_1") + + +class TestTimelineSyncActorIdentityPersistence(TestCase): + def setUp(self) -> None: + self.repo = make_repo() + self.pr = make_pr(self.repo, 1) + + def test_create_persists_identity(self) -> None: + sync_timeline_events(self.pr, [_ev("LabeledEvent", "TL1", actor=BOT, label={"name": "CI"})]) + row = PRTimelineEvent.objects.get(github_node_id="TL1") + self.assertEqual(row.actor_type, PRActorType.BOT) + self.assertEqual(row.actor_node_id, "BOT_kgDOBors") + + def test_rewalk_fills_previously_empty_identity(self) -> None: + # Simulates a row ingested before the columns existed (or an archive row). + sync_timeline_events(self.pr, [_ev("LabeledEvent", "TL2", label={"name": "CI"})]) + row = PRTimelineEvent.objects.get(github_node_id="TL2") + self.assertIsNone(row.actor_type) + + res = sync_timeline_events(self.pr, [_ev("LabeledEvent", "TL2", actor=BOT, label={"name": "CI"})]) + self.assertEqual(res.updated, 1) + row.refresh_from_db() + self.assertEqual(row.actor_type, PRActorType.BOT) + self.assertEqual(row.actor_node_id, "BOT_kgDOBors") + self.assertEqual(row.actor_login, "mathlib-bors") + + def test_rewalk_never_overwrites_an_existing_identity(self) -> None: + sync_timeline_events(self.pr, [_ev("ClosedEvent", "TL3", actor=USER)]) + # A later walk reporting a different account must not clobber the + # ingest-time attribution — that history is the point of the node id. + sync_timeline_events(self.pr, [_ev("ClosedEvent", "TL3", actor=BOT)]) + row = PRTimelineEvent.objects.get(github_node_id="TL3") + self.assertEqual(row.actor_type, PRActorType.USER) + self.assertEqual(row.actor_node_id, "U_kgDOAlice") + self.assertEqual(row.actor_login, "alice") + + def test_archive_mode_rows_ingest_untyped(self) -> None: + res = sync_timeline_events( + self.pr, + [_ev("LabeledEvent", "TL4", label={"name": "WIP"})], + archive_mode=True, + ) + self.assertEqual(res.created, 1) + row = PRTimelineEvent.objects.get(github_node_id="TL4") + self.assertIsNone(row.actor_type) + self.assertIsNone(row.actor_node_id) + self.assertIsNone(row.actor_login) + + +class TestSynthesizedDismissedReviewParentIdentity(TestCase): + def setUp(self) -> None: + self.repo = make_repo() + self.pr = make_pr(self.repo, 1) + + def _dismiss_event(self, *, review_author: dict | None) -> dict: + return _ev( + "ReviewDismissedEvent", + "TL_DIS", + previousReviewState="CHANGES_REQUESTED", + actor=USER, + review={"id": "REV_1", "submittedAt": "2024-12-31T00:00:00Z", "author": review_author}, + ) + + def test_synthesized_parent_carries_review_author_identity(self) -> None: + sync_timeline_events(self.pr, [self._dismiss_event(review_author=BOT)]) + parent = PRTimelineEvent.objects.get(github_node_id="REV_1") + self.assertEqual(parent.type, PRTimelineEventType.REVIEW_CHANGES_REQUESTED) + self.assertEqual(parent.actor_login, "mathlib-bors") + self.assertEqual(parent.actor_type, PRActorType.BOT) + self.assertEqual(parent.actor_node_id, "BOT_kgDOBors") + + def test_synthesized_parent_stays_untyped_for_legacy_extra(self) -> None: + # A REVIEW_DISMISSED row ingested before doc 051 has no + # dismissed_review_author_type key in `extra`; synthesis must not + # invent one. The nodes(ids:) backfill heals these. + legacy_extra = { + "previous_review_state": "APPROVED", + "dismissed_review_node_id": "REV_LEGACY", + "dismissed_review_author": "alice", + "dismissed_review_submitted_at": "2024-12-31T00:00:00Z", + } + parent, created = _synthesize_dismissed_review_parent(self.pr, legacy_extra) + self.assertTrue(created) + assert parent is not None + self.assertEqual(parent.actor_login, "alice") + self.assertIsNone(parent.actor_type) + self.assertIsNone(parent.actor_node_id) + + def test_synthesized_parent_drops_unmodelled_author_type(self) -> None: + parent, _ = _synthesize_dismissed_review_parent( + self.pr, + { + "previous_review_state": "APPROVED", + "dismissed_review_node_id": "REV_ODD", + "dismissed_review_author": "leanprover-community", + "dismissed_review_submitted_at": "2024-12-31T00:00:00Z", + "dismissed_review_author_type": "Organization", + "dismissed_review_author_node_id": "O_1", + }, + ) + assert parent is not None + self.assertIsNone(parent.actor_type) + self.assertEqual(parent.actor_node_id, "O_1") diff --git a/qb_site/syncer/tests/tasks/test_collect_convergence_actor_type.py b/qb_site/syncer/tests/tasks/test_collect_convergence_actor_type.py new file mode 100644 index 00000000..7b2677f1 --- /dev/null +++ b/qb_site/syncer/tests/tasks/test_collect_convergence_actor_type.py @@ -0,0 +1,86 @@ +"""Convergence counters for the actor-typing drain (design doc 051).""" + +from __future__ import annotations + +from django.test import TestCase + +from syncer.models import PRActorType, PRTimelineEvent, PRTimelineEventType, SyncerConvergenceSnapshot +from syncer.tasks.collect_convergence import collect_syncer_convergence_task +from syncer.tests.factories import make_pr, make_repo + + +class TestCollectSyncerConvergenceActorTypeCounters(TestCase): + def setUp(self) -> None: + self.repo = make_repo(owner="leanprover-community", name="mathlib4") + self.pr = make_pr(self.repo, 1) + self.seq = 0 + + def _event(self, **kwargs: object) -> PRTimelineEvent: + self.seq += 1 + defaults: dict = { + "pull_request": self.pr, + "github_node_id": f"TL_{self.seq}", + "type": PRTimelineEventType.LABELED, + "occurred_at": "2025-01-01T00:00:00Z", + } + defaults.update(kwargs) + return PRTimelineEvent.objects.create(**defaults) + + def _latest(self) -> SyncerConvergenceSnapshot: + collect_syncer_convergence_task() + return SyncerConvergenceSnapshot.objects.filter(repository=self.repo).latest("collected_at") + + def test_counts_the_drain_target_set_and_the_typeable_subset(self) -> None: + # Untyped with a login: typeable work, counted by both. + self._event(actor_login="alice") + # Untyped with no login at all (archive-imported shape) and with the + # empty-string spelling the other extraction idiom writes: still in the + # drain's target set, but not known to have had an actor. + self._event(actor_login=None) + self._event(actor_login="") + # Already typed: counted by neither. + self._event(actor_login="bob", actor_type=PRActorType.USER, actor_node_id="U_1") + # No node id, so the drain cannot reach it; excluded from its target set + # but still typeable work the metric should not hide. + PRTimelineEvent.objects.create( + pull_request=self.pr, + github_node_id=None, + type=PRTimelineEventType.LABELED, + occurred_at="2025-01-01T00:00:00Z", + actor_login="carol", + ) + + snap = self._latest() + self.assertEqual(snap.timeline_events_missing_actor_type, 3) + self.assertEqual(snap.timeline_events_untyped_with_login, 2) + + def test_both_counters_are_zero_once_everything_resolvable_is_typed(self) -> None: + # The floor the drain plateaus at: GitHub reports no actor, so there is + # no login and nothing to type. `missing_actor_type` stays non-zero + # forever; only the login-bearing counter converges. + self._event(actor_login=None) + self._event(actor_login="alice", actor_type=PRActorType.BOT, actor_node_id="BOT_1") + + snap = self._latest() + self.assertEqual(snap.timeline_events_missing_actor_type, 1) + self.assertEqual(snap.timeline_events_untyped_with_login, 0) + + def test_counters_are_scoped_per_repository(self) -> None: + other_repo = make_repo(owner="leanprover-community", name="batteries") + other_pr = make_pr(other_repo, 7) + self._event(actor_login="alice") + PRTimelineEvent.objects.create( + pull_request=other_pr, + github_node_id="TL_OTHER", + type=PRTimelineEventType.LABELED, + occurred_at="2025-01-01T00:00:00Z", + actor_login="bob", + ) + + collect_syncer_convergence_task() + mine = SyncerConvergenceSnapshot.objects.filter(repository=self.repo).latest("collected_at") + theirs = SyncerConvergenceSnapshot.objects.filter(repository=other_repo).latest("collected_at") + self.assertEqual(mine.timeline_events_untyped_with_login, 1) + self.assertEqual(theirs.timeline_events_untyped_with_login, 1) + self.assertEqual(mine.timeline_events_missing_actor_type, 1) + self.assertEqual(theirs.timeline_events_missing_actor_type, 1) diff --git a/scripts/validate_github_graphql.py b/scripts/validate_github_graphql.py index 499834bf..9245b380 100644 --- a/scripts/validate_github_graphql.py +++ b/scripts/validate_github_graphql.py @@ -188,14 +188,21 @@ def main() -> int: timeline_payload = _post_graphql(token, timeline_query, timeline_vars) _require_no_errors("timeline_page.graphql", timeline_payload) - start_cursor = ( - (timeline_payload.get("data") or {}) - .get("repository", {}) - .get("pullRequest", {}) - .get("timelineItems", {}) - .get("pageInfo", {}) - .get("startCursor") - ) + timeline_items = (timeline_payload.get("data") or {}).get("repository", {}).get("pullRequest", {}).get("timelineItems", {}) + + # actor_types_by_node_ids.graphql re-resolves stored timeline items by node + # id (design doc 051). Validate it against ids we just fetched, so the + # check exercises the real union rather than a synthetic id. + node_ids = [n.get("id") for n in (timeline_items.get("nodes") or []) if isinstance(n, dict) and n.get("id")] + if node_ids: + actor_types_query = _load_query(Path("qb_site/syncer/queries/actor_types_by_node_ids.graphql")) + print("Validating actor_types_by_node_ids.graphql...") + actor_types_payload = _post_graphql(token, actor_types_query, {"ids": node_ids}) + _require_no_errors("actor_types_by_node_ids.graphql", actor_types_payload) + else: + print("Skipping actor_types_by_node_ids.graphql (no timeline node ids available).") + + start_cursor = timeline_items.get("pageInfo", {}).get("startCursor") if not start_cursor: print("Skipping timeline_page_back.graphql (no startCursor available).") return 0