Skip to content

Commit dfd3bc2

Browse files
authored
fix(registry): complete large repository source refresh (#575)
1 parent 5a9beb3 commit dfd3bc2

7 files changed

Lines changed: 169 additions & 18 deletions

File tree

‎layers/registry/server/api/repos/[owner]/[repo].get.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,11 @@ export default defineCachedEventHandler(async (event) => {
143143
if (cachedScan)
144144
return profileFrom(meta, cachedScan) satisfies RepoSourceProfile
145145

146-
const treeRes = await getTree(meta.owner.login, meta.name, treeSha ?? meta.default_branch, bindings, { timeoutMs: GITHUB_PAGE_READ_TIMEOUT_MS })
146+
const treeRes = await getTree(meta.owner.login, meta.name, treeSha ?? meta.default_branch, bindings, {
147+
timeoutMs: GITHUB_PAGE_READ_TIMEOUT_MS,
148+
// Repository pages can show a partial listing. Durable sync owns expansion.
149+
expandTruncated: false,
150+
})
147151
.catch((error: unknown) => githubReadFailed('repo-source-profile-tree', error))
148152
const scan = treeRes.data ? scanTree(treeRes.data) : null
149153
if (scan && treeSha)

‎layers/registry/server/utils/github-client.ts‎

Lines changed: 50 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,7 @@ export interface TreeEntry {
175175
path: string
176176
type: 'blob' | 'tree' | 'commit'
177177
sha: string
178+
size?: number
178179
}
179180

180181
export interface TreeResponse {
@@ -529,13 +530,59 @@ export async function getTree(
529530
repo: string,
530531
ref: string,
531532
bindings: GithubBindings,
532-
options?: GithubReadOptions,
533+
options?: GithubReadOptions & { expandTruncated?: boolean },
533534
): Promise<FetchOutcome<TreeResponse>> {
534-
return ghRequest<TreeResponse>(
535+
// A page deadline covers the entire fallback, including credential reads.
536+
const init = readInit(options)
537+
const initial = await ghRequest<TreeResponse>(
535538
`${API_BASE}/repos/${owner}/${repo}/git/trees/${ref}?recursive=1`,
536539
bindings,
537-
readInit(options),
540+
init,
538541
)
542+
if (!initial.data?.truncated || options?.expandTruncated !== true)
543+
return initial
544+
545+
// GitHub caps recursive trees at 100,000 entries or 7 MB. Resolve each
546+
// truncated subtree from its immutable SHA. Never merge a partial response
547+
// into the inventory: sync uses absent paths to retire Skills.
548+
const rootSha = initial.data.sha
549+
const incomplete: TreeResponse = { sha: rootSha, tree: [], truncated: true }
550+
const pending = [{ sha: rootSha, prefix: '', recursive: false }]
551+
const entries: TreeEntry[] = []
552+
let rateLimit = initial.rateLimit
553+
let requests = 1
554+
while (pending.length > 0) {
555+
// Bound quota and isolate memory. Larger trees retain their verdict and
556+
// retry on the general refresh clock instead of yielding a false snapshot.
557+
if (requests >= 64)
558+
return { ...initial, data: incomplete, rateLimit, notModified: false }
559+
const next = pending.pop()!
560+
requests++
561+
const result = await ghRequest<TreeResponse>(
562+
`${API_BASE}/repos/${owner}/${repo}/git/trees/${next.sha}${next.recursive ? '?recursive=1' : ''}`,
563+
bindings,
564+
init,
565+
)
566+
if (!result.data)
567+
return result
568+
if (result.rateLimit && (!rateLimit || result.rateLimit.remaining < rateLimit.remaining))
569+
rateLimit = result.rateLimit
570+
if (result.data.truncated) {
571+
if (!next.recursive)
572+
return { ...result, data: incomplete, rateLimit, notModified: false }
573+
pending.push({ ...next, recursive: false })
574+
continue
575+
}
576+
if (entries.length + result.data.tree.length > 250_000)
577+
return { ...result, data: incomplete, rateLimit, notModified: false }
578+
for (const entry of result.data.tree) {
579+
const path = next.prefix ? `${next.prefix}/${entry.path}` : entry.path
580+
entries.push({ path, type: entry.type, sha: entry.sha, size: entry.size })
581+
if (!next.recursive && entry.type === 'tree')
582+
pending.push({ sha: entry.sha, prefix: path, recursive: true })
583+
}
584+
}
585+
return { ...initial, data: { sha: rootSha, tree: entries, truncated: false }, rateLimit, notModified: false }
539586
}
540587

541588
export interface GetCommitsOpts {

‎layers/registry/server/utils/sync-candidates.ts‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ interface HistoricalDiscoveryStageOptions extends RepoSyncPriorityOptions {
1717
/**
1818
* Subscribed repos get the short freshness window supplied by the caller.
1919
* Grouping collapses multiple subscribers watching the same repo. Repos with
20-
* a recorded too-large tree verdict stay out: GitHub will truncate their tree
21-
* response on every attempt, so re-picking them only burns rate limit.
20+
* a recorded too-large tree verdict use the slower general refresh pool.
21+
* Their bounded subtree fallback costs more reads than a normal tree fetch.
2222
*/
2323
export const SUBSCRIBED_SYNC_CANDIDATES_SQL = `
2424
SELECT r.owner, r.repo, r.repo_meta_synced_at AS ls,
@@ -47,8 +47,9 @@ export const SUBSCRIBED_SYNC_CANDIDATES_SQL = `
4747
* forever. EXISTS keeps empty/retired repo rows out without grouping skills.
4848
* A repo whose every row is `path_missing` has no Skills left: its tree was
4949
* readable but held none, so it is not broken and not refreshed either.
50-
* Repos with a recorded too-large tree verdict stay out for the same reason
51-
* as the subscribed sweep above.
50+
* Truncated repositories retry on this slower clock. The tree reader now
51+
* resolves truncated responses through bounded immutable subtree reads.
52+
* Public references without a source commit lead this pool until repaired.
5253
*/
5354
export const GENERAL_SYNC_CANDIDATES_SQL = `
5455
SELECT r.owner, r.repo, r.repo_meta_synced_at AS ls,
@@ -57,14 +58,19 @@ export const GENERAL_SYNC_CANDIDATES_SQL = `
5758
LEFT JOIN discovery_candidates dc
5859
ON dc.owner = r.owner AND dc.repo = r.repo
5960
WHERE r.broken_since IS NULL
60-
AND r.tree_truncated_at IS NULL
6161
AND (r.repo_meta_synced_at IS NULL OR r.repo_meta_synced_at < ?1)
6262
AND EXISTS (
6363
SELECT 1 FROM skills s
6464
WHERE s.owner = r.owner AND s.repo = r.repo
6565
AND COALESCE(s.sync_status, '') != 'path_missing'
6666
)
67-
ORDER BY r.repo_meta_synced_at IS NULL DESC, r.repo_meta_synced_at ASC
67+
ORDER BY EXISTS (
68+
SELECT 1 FROM skills s
69+
WHERE s.owner = r.owner AND s.repo = r.repo
70+
AND s.seo_indexable = 1 AND s.references_count > 0
71+
AND s.rendered_commit_sha IS NULL
72+
AND COALESCE(s.sync_status, '') != 'path_missing'
73+
) DESC, r.repo_meta_synced_at IS NULL DESC, r.repo_meta_synced_at ASC
6874
LIMIT ?2`
6975

7076
/**

‎layers/registry/server/utils/sync-repo.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,7 @@ export async function refreshRepoAssets(
373373
SET source_owner = ?, source_repo = ?
374374
WHERE owner = ? AND repo = ?
375375
`).bind(sourceOwner, sourceRepo, owner, repo).run()
376-
const treeRes = await getTree(sourceOwner, sourceRepo, repoRes.data.headTreeSha ?? branch, bindings)
376+
const treeRes = await getTree(sourceOwner, sourceRepo, repoRes.data.headTreeSha ?? branch, bindings, { expandTruncated: true })
377377
logRateLimit(`asset-backfill tree ${owner}/${repo}`, treeRes.rateLimit)
378378
const rateLimitRemaining = Math.min(
379379
rate.rateLimitRemaining ?? Number.POSITIVE_INFINITY,
@@ -895,7 +895,7 @@ export async function syncRepo(
895895
// The head tree SHA names the exact tree the summary saw. A branch name
896896
// can move between the two reads, and a SHA URL is immutable, so its ETag
897897
// cache entry answers 304 for as long as the tree stands.
898-
const treeRes = await getTree(sourceOwner, sourceRepo, headTreeSha ?? branch, bindings)
898+
const treeRes = await getTree(sourceOwner, sourceRepo, headTreeSha ?? branch, bindings, { expandTruncated: true })
899899
logRateLimit(`tree ${owner}/${repo}`, treeRes.rateLimit)
900900
trackRateLimit(treeRes.rateLimit)
901901

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { afterEach, describe, expect, it, vi } from 'vitest'
2+
import { getTree } from '../../layers/registry/server/utils/github-client'
3+
4+
afterEach(() => vi.unstubAllGlobals())
5+
6+
describe('getTree', () => {
7+
it('reconstructs truncated trees from immutable subtree SHAs', async () => {
8+
const answers = new Map([
9+
['root?recursive=1', { sha: 'root', truncated: true, tree: [{ path: 'partial', type: 'blob', sha: 'discard' }] }],
10+
['root', { sha: 'root', tree: [{ path: 'skills', type: 'tree', sha: 'skills-tree' }] }],
11+
['skills-tree?recursive=1', { sha: 'skills-tree', truncated: true, tree: [] }],
12+
['skills-tree', { sha: 'skills-tree', tree: [{ path: 'one', type: 'tree', sha: 'one-tree' }] }],
13+
['one-tree?recursive=1', { sha: 'one-tree', tree: [{ path: 'SKILL.md', type: 'blob', sha: 'skill', size: 120 }, { path: 'references/a.md', type: 'blob', sha: 'reference', size: 42 }] }],
14+
])
15+
const reads: string[] = []
16+
vi.stubGlobal('fetch', vi.fn(async (url: string) => {
17+
const key = url.split('/git/trees/')[1]!
18+
reads.push(key)
19+
const answer = answers.get(key)
20+
if (!answer)
21+
throw new Error(`Unexpected tree read: ${key}`)
22+
return Response.json(answer)
23+
}))
24+
25+
const result = await getTree('acme', 'large', 'root', {}, { expandTruncated: true })
26+
27+
expect(result.data).toEqual({ sha: 'root', truncated: false, tree: [
28+
{ path: 'skills', type: 'tree', sha: 'skills-tree' },
29+
{ path: 'skills/one', type: 'tree', sha: 'one-tree' },
30+
{ path: 'skills/one/SKILL.md', type: 'blob', sha: 'skill', size: 120 },
31+
{ path: 'skills/one/references/a.md', type: 'blob', sha: 'reference', size: 42 },
32+
] })
33+
expect(reads).toEqual([...answers.keys()])
34+
})
35+
36+
it('returns a partial tree with one request unless durable refresh opts in', async () => {
37+
const tree = { sha: 'root', truncated: true, tree: [{ path: 'SKILL.md', type: 'blob', sha: 'skill', size: 120 }] }
38+
const fetchMock = vi.fn(async () => Response.json(tree))
39+
vi.stubGlobal('fetch', fetchMock)
40+
41+
const result = await getTree('acme', 'large', 'root', {})
42+
43+
expect(result.data).toEqual(tree)
44+
expect(fetchMock).toHaveBeenCalledTimes(1)
45+
})
46+
47+
it('propagates a subtree read failure without presenting a partial tree', async () => {
48+
let reads = 0
49+
vi.stubGlobal('fetch', vi.fn(async () => {
50+
reads++
51+
return reads === 1
52+
? Response.json({ sha: 'root', truncated: true, tree: [] })
53+
: new Response('', { status: 429 })
54+
}))
55+
56+
const result = await getTree('acme', 'large', 'root', {}, { expandTruncated: true })
57+
58+
expect(result.status).toBe(429)
59+
expect(result.data).toBeNull()
60+
})
61+
62+
it('stops bounded traversal without accepting an incomplete inventory', async () => {
63+
const fetchMock = vi.fn(async (url: string) => Response.json({
64+
sha: url.split('/git/trees/')[1]!.split('?')[0],
65+
truncated: url.includes('?'),
66+
tree: url.includes('?') ? [] : [{ path: 'nested', type: 'tree', sha: 'next' }],
67+
}))
68+
vi.stubGlobal('fetch', fetchMock)
69+
70+
const result = await getTree('acme', 'large', 'root', {}, { expandTruncated: true })
71+
72+
expect(result.data?.truncated).toBe(true)
73+
expect(fetchMock.mock.calls.length).toBeGreaterThan(1)
74+
expect(fetchMock.mock.calls.length).toBeLessThanOrEqual(64)
75+
})
76+
})

‎test/unit/sync-candidates.test.ts‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ describe('github sync candidate selection', () => {
3030
repo TEXT NOT NULL,
3131
name TEXT NOT NULL,
3232
sync_status TEXT,
33+
seo_indexable INTEGER DEFAULT 0,
34+
references_count INTEGER DEFAULT 0,
35+
rendered_commit_sha TEXT,
3336
PRIMARY KEY (owner, repo, name)
3437
);
3538
CREATE TABLE skill_subscriptions (
@@ -120,7 +123,7 @@ describe('github sync candidate selection', () => {
120123
.prepare(GENERAL_SYNC_CANDIDATES_SQL)
121124
.all({ 1: 1_000_000 - 36 * 3600, 2: 250 }) as Array<{ owner: string, repo: string }>
122125

123-
expect(rows.map(row => row.repo)).toEqual(['never-checked', 'general-due'])
126+
expect(rows.map(row => row.repo)).toEqual(['never-checked', 'general-due', 'too-large'])
124127
expect(rows.map(row => row.repo)).not.toContain('general-recent')
125128
expect(rows.map(row => row.repo)).not.toContain('broken')
126129
expect(rows.map(row => row.repo)).not.toContain('empty-retired')
@@ -143,20 +146,35 @@ describe('github sync candidate selection', () => {
143146
.all({ 1: 1_000_000 - 36 * 3600, 2: 250 }) as Array<{ repo: string }>
144147

145148
expect(subscribed.map(row => row.repo)).not.toContain('all-retired')
146-
expect(general.map(row => row.repo)).toEqual(['never-checked', 'general-due'])
149+
expect(general.map(row => row.repo)).toEqual(['never-checked', 'general-due', 'too-large'])
147150
})
148151

149-
it('excludes repos with a recorded too-large verdict from both sweeps', () => {
152+
it('retries truncated repositories on the general refresh clock', () => {
150153
const subscribed = sqlite
151154
.prepare(SUBSCRIBED_SYNC_CANDIDATES_SQL)
152155
.all({ 1: 1_000_000 - 3600, 2: 250 }) as Array<{ repo: string }>
153156
const general = sqlite
154157
.prepare(GENERAL_SYNC_CANDIDATES_SQL)
155158
.all({ 1: 1_000_000 - 36 * 3600, 2: 250 }) as Array<{ repo: string }>
156159

157-
// acme/too-large is due on both clocks, so only the verdict keeps it out.
158160
expect(subscribed.map(row => row.repo)).not.toContain('too-large')
159-
expect(general.map(row => row.repo)).not.toContain('too-large')
161+
expect(general.map(row => row.repo)).toContain('too-large')
162+
sqlite.exec('UPDATE repos SET repo_meta_synced_at = 999000 WHERE repo = \'too-large\'')
163+
const rechecked = sqlite.prepare(GENERAL_SYNC_CANDIDATES_SQL)
164+
.all({ 1: 1_000_000 - 36 * 3600, 2: 250 }) as Array<{ repo: string }>
165+
expect(rechecked.map(row => row.repo)).not.toContain('too-large')
166+
})
167+
168+
it('prioritizes public references without a source commit over older ordinary work', () => {
169+
sqlite.exec(`
170+
UPDATE skills SET seo_indexable = 1, references_count = 2 WHERE repo = 'too-large';
171+
UPDATE skills SET seo_indexable = 1, references_count = 2, rendered_commit_sha = 'known' WHERE repo = 'general-due';
172+
INSERT INTO skills (owner, repo, name, seo_indexable, references_count, sync_status)
173+
VALUES ('acme', 'never-checked', 'retired', 1, 2, 'path_missing');
174+
`)
175+
const rows = sqlite.prepare(GENERAL_SYNC_CANDIDATES_SQL)
176+
.all({ 1: 1_000_000 - 36 * 3600, 2: 1 }) as Array<{ repo: string }>
177+
expect(rows.map(row => row.repo)).toEqual(['too-large'])
160178
})
161179

162180
it('walks the due-work partial index', () => {

‎test/unit/sync-repo-freshness.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ describe('syncRepo freshness cursor', () => {
217217
FROM repos WHERE owner = 'acme' AND repo = 'skills'
218218
`).get()
219219

220-
expect(github.getTree).toHaveBeenCalledWith('acme', 'skills', 'moved-tree', {})
220+
expect(github.getTree.mock.calls[0]?.[2]).toBe('moved-tree')
221221
expect(result.status).toBe('failed')
222222
expect(result.reason).toBe('tree_truncated')
223223
expect(row).toEqual({

0 commit comments

Comments
 (0)