feat: rate-limit-aware, resumable GitHub sync with webhook race guard (#24) - #36
Merged
chonilius merged 9 commits intoJul 22, 2026
Conversation
…ergeFi#24) Adds @octokit/plugin-retry and @octokit/plugin-throttling (compatible with the already-installed @octokit/core@7 that @octokit/rest@22 resolves to) - the official plugins for backing off on transient 5xx/network errors and primary/secondary GitHub rate limits. Both plugins, like @octokit/rest itself, ship ESM-only with no CJS build. Jest's default config only transforms non-node_modules code, so anything that imports the GitHub sync service (even transitively) fails to parse under ts-jest with "Cannot use import statement outside a module". Widens transformIgnorePatterns to also transform @octokit/* and its ESM-only sub-dependencies (before-after-hook, universal-user-agent). Real Node (20.19+/22.12+, and CI's Node 24) already handles require()-ing these ESM packages natively, so this is a test-tooling-only fix, not a production runtime concern.
createGithubOctokit builds an Octokit client with both plugins wired in behind a GITHUB_OCTOKIT DI token (so tests can inject a mock instead of a real Octokit): - retry.doNotRetry explicitly adds 429 on top of the plugin's default list. plugin-retry would otherwise retry primary rate limits too, racing plugin-throttling's own retry-after-aware handling of the exact same error - 429 is deliberately left to throttling alone. - throttle.onRateLimit/onSecondaryRateLimit share one handler factory (makeLimitHandler) that logs the hit and enforces a hard ceiling (GITHUB_MAX_RETRIES = 3) so a persistent outage or bad token can't retry forever. Not wired into the module/service yet - that's the next commit.
…eFi#24) Tracks GitHub's own updated_at for the issue, as of the last write that touched it from either sync or a webhook. The next commit adds the guard itself (GithubSyncService.upsertIssueRecord); this is just the column both write paths will read/write.
Companion migration for the previous commit's entity change, matching this repo's existing pattern of a reviewable migration file alongside every schema change (see MergeFi#27's CreateIdempotencyKeys/ EscrowFkIntegrityAndSponsorId for precedent).
Addresses the core failure modes from MergeFi#24: - syncIssues switches from octokit.paginate() (which buffers every page before returning) to octokit.paginate.iterator(), persisting each page as it's fetched. A rate-limit/network failure on page N no longer discards pages 1..N-1 - those are already durably saved. - A failure now surfaces as GithubSyncInterruptedError, carrying owner/ repo/issuesSyncedBeforeFailure/cause, instead of an opaque Octokit error - "resumable failure with a clear signal", not a silent partial sync. Re-running is always safe: every write goes through the idempotent, optimistic-concurrency-guarded upsertIssueRecord below, so nothing is duplicated and nothing is permanently skipped. - upsertIssueRecord centralizes the per-issue upsert (previously inlined in syncIssues) and adds the guard itself: a write is skipped if the locally stored githubUpdatedAt is already newer than the incoming data. Public and reusable so the webhook path (next commit) can share it rather than duplicating the comparison. - findRepositoryByGithubId is a small public lookup the webhook path needs to resolve a payload's repository.id to a local Repository row. - logRateLimitStatus logs the core rate-limit budget before and after syncRepository, so a mid-sync interruption shows up as an obviously low "before" number rather than a mystery failure. Best-effort: a failed lookup never blocks the sync itself. Now depends on GITHUB_OCTOKIT (previous commits) instead of constructing its own bare Octokit client; wired into GithubModule's providers here.
…ergeFi#24) Adds an "issues" event handler (opened/edited/closed/reopened/...) that resolves the payload's repository and calls GithubSyncService.upsertIssueRecord - the exact same optimistic-concurrency-guarded write path syncIssues uses. This is what makes the ordering-independence guarantee real: whichever of sync or a webhook has the freshest GitHub-side updated_at always wins, regardless of which one runs, or commits, second. Before this, no webhook path wrote to Issue's own fields at all, so the race MergeFi#24 describes had no real webhook-side write to guard.
…ndency (MergeFi#24) Updates the existing spec's TestingModule with a GithubSyncService mock (now a required constructor dependency) and adds coverage for the three issues-event paths: delegates to upsertIssueRecord for a tracked repository, ignores events for an untracked one without erroring, and still marks the webhook event PROCESSED when the upsert itself rejects the write as stale.
Tests makeLimitHandler directly rather than trying to simulate real Octokit HTTP retry timing: retries while under GITHUB_MAX_RETRIES for both primary and secondary limits, stops once the ceiling is reached (so a persistent outage can't retry forever), and logs enough of the request (method/url/retryAfter) to trace an interruption. Also smoke -tests createGithubOctokit with and without a configured token.
…ging (MergeFi#24) Matches MergeFi#24's acceptance criteria directly: - A simulated 429 mid-pagination: the page fetched before the failure is durably persisted (issueRepo.save called for it), and syncIssues rejects with GithubSyncInterruptedError carrying owner/repo/ issuesSyncedBeforeFailure and a message confirming re-running is safe - never a silently incomplete sync with no signal. - upsertIssueRecord ordering-independence: a stale write is rejected whether it arrives as a "sync payload landing after a webhook" or a "delayed webhook retry landing after a sync" - same guard, same result, regardless of which caller runs second. - syncRepository logs rate-limit budget both before and after, and a failed rate-limit lookup doesn't block the sync itself.
|
@boluwacodes is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
4 tasks
Contributor
|
good fix |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #24.
GithubSyncServicebuilt a plain Octokit client with no retry/backoffand paginated via
octokit.paginate(), which buffers every pagebefore returning. Investigated the concrete failure modes the issue
asked about:
call (not "half-synced" —
paginate()never returns anything untilevery page succeeds, so a failure on page N discards pages 1..N-1
too), and propagates as an unhandled, opaque Octokit error.
inspection, no webhook path even wrote to
Issue's own fields atall yet (only read it, to find a linked bounty), so the "sync clobbers
a webhook update" race had no real webhook-side write to guard against.
Changes
octokit.provider.ts, new): Octokitconfigured with
@octokit/plugin-retry+@octokit/plugin-throttlingbehind a
GITHUB_OCTOKITDI token.retry.doNotRetryexplicitly adds429 on top of the plugin's default list so it doesn't race
throttling's own retry-after-aware handling of the same error.onRateLimit/onSecondaryRateLimitshare one handler that logs thehit and enforces a hard ceiling (
GITHUB_MAX_RETRIES = 3) so apersistent outage can't retry forever.
syncIssuesswitched fromoctokit.paginate()tooctokit.paginate.iterator(), persisting each page as it's fetched.A failure now surfaces as
GithubSyncInterruptedError(owner, repo,issuesSyncedBeforeFailure, cause) instead of an opaque error —"resumable failure with a clear signal," never a silent partial sync.
Re-running is always safe: every write goes through the idempotent,
guarded upsert below.
Issue.githubUpdatedAt(migration included) and
GithubSyncService.upsertIssueRecord, whichskips a write if the stored
githubUpdatedAtis already newer thanthe incoming data. Added a real
issueswebhook event handler(opened/edited/closed/reopened/...) that calls the same guarded
upsert sync uses — this is what makes the ordering-independence
guarantee real, rather than guarding a write path that didn't exist.
syncRepositorylogs the corerate-limit budget before and after via
octokit.rest.rateLimit.get(),best-effort (a failed lookup never blocks the sync).
@octokit/rest/plugin-retry/plugin-throttlingare ESM-only with no CJS build. Modern Node (20.19+/22.12+, incl. CI's
Node 24)
require()s them fine natively, but Jest's default configonly transforms non-
node_modulescode, so anything importing thesync service failed to parse under
ts-jest. WidenedtransformIgnorePatternsto cover@octokit/*and its ESM-onlydeps — test-tooling only, confirmed no production runtime impact by
requiring the actual compiled
dist/output directly under Node.Test plan
npm run lint— cleannpx tsc --noEmit— cleannpm run build— succeeds; manually verified the compileddist/github/*.jsload correctly under real Node (confirms theJest transform fix is test-tooling-only, not masking a runtime issue)
npm test— 114/121 passing; the 7 failures are all inescrow-fk-integrity.integration.spec.ts, a pre-existing testrequiring a live Postgres connection I don't have locally (CI
provides one via
services.postgres) — confirmed pre-existingand unrelated by reproducing the identical failure on
mainbeforeany of my changes
persisted,
GithubSyncInterruptedErrorthrown with accuratecounts), ordering-independence for the concurrency guard in both
directions, rate-limit logging before/after sync, and the retry/
throttle handlers' ceiling behavior