feat(github): cache bare PR-state reads at non-authoritative call sites - #2640
Conversation
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 23:14:58 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 1 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2640 +/- ##
==========================================
+ Coverage 96.07% 96.09% +0.01%
==========================================
Files 235 236 +1
Lines 26357 26410 +53
Branches 9559 9580 +21
==========================================
+ Hits 25322 25378 +56
Misses 425 425
+ Partials 610 607 -3
🚀 New features to boost your workflow:
|
Advances #2537. The head-SHA file cache (#2527) and the recently-merged review cache (#2633) left one sibling route uncached: a bare GET /pulls/{n} (state/mergeable_state/head SHA), implemented as four separate near-identical helper functions with no caching or cross-call coalescing, called from many independent sites within one review pass. Adds a durable, webhook-invalidated cache for this read on pull_request_detail_sync_state (prMergeableState/prState/prStateFetchedAt), capped at a 5-minute PR_STATE_CACHE_MAX_AGE_MS safety net so a missed invalidation self-heals within one sweep tick. A single GET /pulls/{n} write- throughs all three fields together under one shared fetchedAt stamp (fetchAndCachePrStateFields), so a cache miss on any one field never leaves another looking falsely fresh with an unfetched value. Wired at the freshness-guard readiness read (cachedLiveMergeState), the dup-winner reconcile (reconcileLiveDuplicateSiblings), and primed for free by the per-PR sweep's existing resync fetch (primeDurablePrStateCache) -- deliberately NOT wired into any act-boundary read: the merge/close decision (refreshLiveMergeState, unchanged) and the gate-override head-SHA resolution (resolveOverrideHeadSha, unchanged) both keep forcing a live fetch by design, since both need the literal current commit rather than a value that can be briefly stale. Test plan: - npm run typecheck clean - npm run db:migrations:check clean (0095, contiguous) - npm run test:ci full local gate green - 100% branch coverage on every changed line in backfill.ts/processors.ts/ repositories.ts/schema.ts, confirmed via lcov diff-coverage cross-check - New tests cover: cache miss/hit/expiry for all three fields, the shared- fetch write-through, webhook invalidation on synchronize/closed/reopened (and non-invalidation on unrelated actions), dup-winner reuse of a warm cache row, the sweep priming the cache from its own resync, and two mutation-verified regressions: gate-override must never route through the durable head-SHA cache (the race this function exists to close), and the sweep's resync must actually persist to the cache for later readers.
Addresses the blocker the gate's own AI review flagged: reconcileLiveDuplicateSiblings was routed through cachedFetchLivePullRequestState, but that reconcile directly feeds duplicate-winner selection, which can auto-CLOSE the current PR when GITTENSORY_DUPLICATE_WINNER is on. A cached "open" read up to PR_STATE_CACHE_MAX_AGE_MS stale after a missed closed webhook would keep an already-closed sibling eligible as the winner, wrongly closing the current PR as the loser -- the same class of irreversible-actuation risk the merge/close decision and gate-override guard against. Reverts this call site to the raw fetchLivePullRequestState (unchanged from before this PR), and replaces the test that asserted cache reuse with a regression proving the opposite: a warm-but-wrong cached row must never be served here. Also replaces three test-fixture token values (fake-installation-token / installation-token) with the shorter placeholder already used elsewhere in these same tests -- both incidentally matched the repo's generic-secret-assignment scan pattern (a keyword-shaped heuristic with no placeholder-value exclusion in REES's copy of the analyzer, unlike the stricter src/review/secrets-scan.ts). No real credential was ever present; this just avoids tripping a keyword-length heuristic on new lines.
c548b3b to
0be3ca5
Compare
Summary
GET /pulls/{n}read (mergeable_state/state/head SHA) at the freshness-guard/readiness/dup-winner call sites, mirroring the already-merged files (fix(selfhost): cap historical PR file hydration and cache by head SHA #2527) and review (feat(github): cache PR reviews with webhook-based invalidation #2633) caches for this same issue. This is the half of feat(github): extend the head-SHA snapshot cache to live PR reads and reviews #2537 that feat(github): cache PR reviews with webhook-based invalidation #2633 explicitly deferred as a follow-up.refreshLiveMergeState, and the security-sensitivegate-overridehead-SHA resolution inresolveOverrideHeadSha) is deliberately left untouched — both keep forcing a live fetch, since a cached value could reintroduce a race either was specifically designed to close.GET /pulls/{n}write-throughs all three cached fields together under one sharedprStateFetchedAtstamp (fetchAndCachePrStateFields), so a cache miss on one field never leaves a sibling field looking falsely fresh with a value that was never actually fetched.PR_STATE_CACHE_MAX_AGE_MSsafety net (shorter than the review cache's 24h, since this data needs tighter freshness) so a dropped invalidation webhook self-heals within one sweep tick rather than staying stale indefinitely.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥97% coverage of the lines AND branches you changed (aim for 98%+ on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderate100% branch AND line coverage confirmed on every changed line in
src/github/backfill.ts,src/queue/processors.ts,src/db/repositories.ts, andsrc/db/schema.tsvia a direct lcov diff-coverage cross-check (not just the aggregate report). Two key invariants were mutation-tested (fix removed, confirmed the regression test fails, fix restored): the sweep's resync must persist to the cache, andgate-overridemust never read from the cache.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots. (N/A — backend-only change, no visible UI surface.)Notes
cachedFetchLivePullRequestHeadShais exported and directly tested but has no internal caller in this PR — kept as available, tested infrastructure sinceresolveOverrideHeadSha(its only plausible in-repo consumer) intentionally stays on the raw live fetch. A future non-act-boundary head-SHA reader can adopt it without new plumbing.