Detail Bug Report

app.detail.dev/org_0f605249-55f3-46eb-880c-20ae3e45ae99/…/bug_962c2d96-ee5f-4a4d-8887-2748c8b9ff6a

Introduced in #80 by @ripgrim on Oct 23, 2026

Summary

• Context: listRepoReviewers (a read-only server function backing the workflow editor's reviewer picker) queues a background reviewer sync when the cached set is older than 24 hours; the sync timestamp is derived from the repo_reviewers rows themselves.
• Bug: When a sync produces zero reviewers, replaceRepoReviewers deletes all rows and inserts none, so the sync never records its timestamp — the repo remains "never synced" (syncedAt: null) forever, and every read re-queues a sync, defeating the 24-hour staleness throttle.
• Actual vs. expected: A successful sync — even one finding zero eligible reviewers — should make the repo look fresh for 24 hours; instead a repo with no syncable reviewers is treated as permanently stale and every open of the reviewer panel enqueues a new GitHub sync.
• Impact: For any repo whose collaborator list yields zero push-or-above collaborators (or where GitHub reads are disabled), each user opening the panel keeps the read loop queueing syncs for up to a 30-second pending window (2-second query polling sends ~15 GETs, but sends are rejected while a job is queued, so at most one new job gets queued per poll cycle after the previous one completes, each job costing exactly 1 GitHub /collaborators API call since pagination stops after the first empty page), while the UI shows a "Refreshing"/"Loading reviewers" state for up to 30 seconds on every open.

Code with Bug

// packages/db/src/services/reviewers.ts
export async function replaceRepoReviewers(
	db: Db,
	repoId: string,
	reviewers: readonly RepoReviewer[],
	syncedAt: Date = new Date(),
): Promise<void> {
	const unique = new Map(
		reviewers.map((reviewer) => {
			const parsed = repoReviewerSchema.parse(reviewer);
			return [parsed.id, parsed];
		}),
	);
	await db.transaction(async (tx) => {
		await tx.delete(repoReviewers).where(eq(repoReviewers.repoId, repoId));
		if (unique.size === 0) {
			return; // <-- BUG 🔴 zero reviewers: sync time is never recorded
		}
		await tx.insert(repoReviewers).values(
			[...unique.values()].map((reviewer) => ({ /* ... */, syncedAt })),
		);
	});
}

// packages/db/src/services/reviewers.ts
const syncedAt = rows.reduce<Date | null>(
	(latest, row) => (latest && latest > row.syncedAt ? latest : row.syncedAt),
	null,
); // <-- BUG 🔴 zero rows ⇒ null, regardless of how often we synced

// apps/web/src/lib/reviewers.functions.ts
const syncing = reviewerServices.reviewersNeedSync(
	list.syncedAt,
	new Date(),
)
	? await queueReviewerSync(data.repoId) // <-- BUG 🔴 fires on every GET when syncedAt stays null
	: false;

// apps/worker/src/jobs/sync-repo-reviewers.ts
if (!http) {
	logger.warn({ repoId: repo.id }, "reviewer sync skipped: github reads disabled");
	return; // <-- BUG 🔴 completes without ever stamping the sync
}

Explanation

• Reviewers freshness is inferred from the repo_reviewers rows (max(row.syncedAt)), so an empty reviewer set means there are no rows and syncedAt computes to null.
• replaceRepoReviewers deletes existing rows and returns early when the new set is empty, so it never persists any timestamp indicating “we successfully synced, but there are zero reviewers.”
• The read path treats syncedAt === null as “never synced,” so listRepoReviewers keeps enqueueing a sync on every read, defeating the intended 24-hour throttle.
• The worker also returns early without stamping when GitHub reads are disabled (and similarly for non-GitHub repos), producing the same “permanent null” behavior.

Codebase Inconsistency

• upsertRepoSuggestions in packages/db/src/services/repos.ts stamps updatedAt even when the cached suggestion list is empty, so a “successful empty sync” becomes fresh for the configured stale window. Reviewers (and similarly structured labels) deviate from this pattern by tying freshness solely to per-item rows.

Failing Test

bun test packages/db/src/services/reviewers-empty-sync.repro.test.ts
# observed: two empty syncs leave syncedAt null and always stale
#   listRepoReviewers(...).syncedAt is null after two empty syncs a day apart   → PASS
#   reviewersNeedSync(syncedAt, secondSync) true immediately after success      → PASS
#   reviewersNeedSync(syncedAt, secondSync + 25h) true                          → PASS
# control: nonempty sync records the time and the throttle engages             → PASS
# 2 pass, 0 fail

Recommended Fix

Record the sync time even when the result set is empty, using a per-repo stamp that survives an empty result (e.g., a dedicated stamp row/table or a synced_at column on the repo) written unconditionally by the sync job, and have listRepoReviewers read from that stamp (or max(stamp, max(row.synced_at))).

History

This bug was introduced in commit 211e4dcb (feat(workflows): load reviewers on their own when the panel opens, Refs #67). That commit added the read-path staleness throttle: listRepoReviewers began calling the new reviewersNeedSync(syncedAt, now) and queuing a sync whenever syncedAt === null or older than 24 hours, with the stated intent that "reading a repo's reviewers queues a sync when it never synced or the last sync is a day old". It did not touch the sync writer, however: replaceRepoReviewers (from 274de6d1) derives the sync stamp solely from repo_reviewers rows and silently drops the timestamp for an empty result — harmless under the original design where syncs only ran from worker jobs on install/refresh, but fatal once reads started gating on the stamp. Consequently a repo that syncs to zero reviewers can never transition out of the "never synced" (null) state, so the 24-hour throttle introduced by 211e4dcb never engages and every read re-queues a sync — turning the pre-existing empty-sync data-model quirk into the per-read re-queue loop reported here. (d86e480f later only refactored reviewersNeedSync into syncIsStale without changing semantics.)