Skip to content

Commit fc1184c

Browse files
committed
feat: add concurrency handling for document fetching and redirect collection
1 parent d17bbc7 commit fc1184c

2 files changed

Lines changed: 225 additions & 36 deletions

File tree

src/utils/docs.functions.ts

Lines changed: 96 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import { isValidRepoPath, MAX_REPO_PATH_LENGTH } from './repo-path'
1717
import { removeLeadingSlash } from './utils'
1818
import type { DocsRedirectManifest } from './docs-redirects'
1919

20-
type DocsTreeNode = {
20+
export type DocsTreeNode = {
2121
path: string
2222
children?: Array<DocsTreeNode>
2323
}
@@ -92,6 +92,33 @@ const docsRedirectInput = v.object({
9292
docsPaths: v.array(v.pipe(v.string(), v.maxLength(512))),
9393
})
9494

95+
// Matches RAW_FETCH_CONCURRENCY in github-example.server.ts.
96+
const DOCS_MANIFEST_FETCH_CONCURRENCY = 6
97+
98+
export async function mapWithConcurrency<T, TResult>(
99+
values: Array<T>,
100+
concurrency: number,
101+
fn: (value: T) => Promise<TResult>,
102+
) {
103+
const results = new Array<TResult>(values.length)
104+
let index = 0
105+
106+
const workers = Array.from(
107+
{ length: Math.min(concurrency, values.length) },
108+
async () => {
109+
while (index < values.length) {
110+
const currentIndex = index
111+
index += 1
112+
results[currentIndex] = await fn(values[currentIndex])
113+
}
114+
},
115+
)
116+
117+
await Promise.all(workers)
118+
119+
return results
120+
}
121+
95122
const temporarilyUnavailableMarkdown = `# Content temporarily unavailable
96123
97124
We are having trouble fetching this document from GitHub right now. Please try again in a minute.`
@@ -146,6 +173,62 @@ function isDocsManifest(value: unknown): value is DocsManifest {
146173
)
147174
}
148175

176+
// Extracted so tests can inject a fake fetchFile without hitting real
177+
// GitHub network/cache.
178+
export async function collectRedirectEntriesForFile(
179+
node: DocsTreeNode,
180+
opts: {
181+
docsRoot: string
182+
fetchFile: (filePath: string) => Promise<string | null>
183+
onCanonicalPath: (canonicalPath: string) => void
184+
},
185+
): Promise<Array<RedirectManifestEntry>> {
186+
const canonicalPath = getCanonicalDocsPath(node.path, opts.docsRoot)
187+
188+
if (canonicalPath === null) {
189+
return []
190+
}
191+
192+
opts.onCanonicalPath(canonicalPath)
193+
194+
let file: string | null
195+
try {
196+
file = await opts.fetchFile(node.path)
197+
} catch (error) {
198+
if (!isRecoverableGitHubContentError(error)) {
199+
throw error
200+
}
201+
202+
return []
203+
}
204+
205+
if (!file) {
206+
return []
207+
}
208+
209+
const frontMatter = extractFrontMatter(file)
210+
const entries: Array<RedirectManifestEntry> = []
211+
212+
for (const redirectFrom of frontMatter.data.redirectFrom ?? []) {
213+
const normalizedRedirect = normalizeDocsRedirectPath(
214+
redirectFrom,
215+
opts.docsRoot,
216+
)
217+
218+
if (!normalizedRedirect || normalizedRedirect === canonicalPath) {
219+
continue
220+
}
221+
222+
entries.push({
223+
from: normalizedRedirect,
224+
to: canonicalPath,
225+
source: node.path,
226+
})
227+
}
228+
229+
return entries
230+
}
231+
149232
async function buildDocsManifest({
150233
repo,
151234
branch,
@@ -165,46 +248,23 @@ async function buildDocsManifest({
165248
node.path.endsWith('.md'),
166249
)
167250
const paths = new Set<string>()
168-
const redirects: Array<RedirectManifestEntry> = []
169-
170-
for (const node of markdownFiles) {
171-
const canonicalPath = getCanonicalDocsPath(node.path, docsRoot)
172-
173-
if (canonicalPath === null) {
174-
continue
175-
}
176-
177-
paths.add(canonicalPath)
178-
179-
const file = await fetchRepoFile(repo, branch, node.path)
180-
181-
if (!file) {
182-
continue
183-
}
184-
185-
const frontMatter = extractFrontMatter(file)
186251

187-
for (const redirectFrom of frontMatter.data.redirectFrom ?? []) {
188-
const normalizedRedirect = normalizeDocsRedirectPath(
189-
redirectFrom,
252+
// A recoverable error on one file must not fail the whole manifest build
253+
// (see collectRedirectEntriesForFile).
254+
const redirectsByFile = await mapWithConcurrency(
255+
markdownFiles,
256+
DOCS_MANIFEST_FETCH_CONCURRENCY,
257+
(node) =>
258+
collectRedirectEntriesForFile(node, {
190259
docsRoot,
191-
)
192-
193-
if (!normalizedRedirect || normalizedRedirect === canonicalPath) {
194-
continue
195-
}
196-
197-
redirects.push({
198-
from: normalizedRedirect,
199-
to: canonicalPath,
200-
source: node.path,
201-
})
202-
}
203-
}
260+
fetchFile: (filePath) => fetchRepoFile(repo, branch, filePath),
261+
onCanonicalPath: (canonicalPath) => paths.add(canonicalPath),
262+
}),
263+
)
204264

205265
return {
206266
paths: Array.from(paths),
207-
redirects: buildRedirectManifest(redirects, {
267+
redirects: buildRedirectManifest(redirectsByFile.flat(), {
208268
label: `docs redirects for ${repo}@${branch}:${docsRoot}`,
209269
}),
210270
}
Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
import assert from 'node:assert/strict'
2+
import test from 'node:test'
3+
import {
4+
collectRedirectEntriesForFile,
5+
mapWithConcurrency,
6+
type DocsTreeNode,
7+
} from '../src/utils/docs.functions'
8+
import { GitHubContentError } from '../src/utils/documents.server'
9+
10+
test('mapWithConcurrency preserves result order regardless of completion order', async () => {
11+
const delays = [30, 5, 20, 1, 15]
12+
13+
const results = await mapWithConcurrency(delays, 3, async (delay) => {
14+
await new Promise((resolve) => setTimeout(resolve, delay))
15+
return delay
16+
})
17+
18+
assert.deepEqual(results, delays)
19+
})
20+
21+
test('mapWithConcurrency bounds concurrency instead of running everything in parallel', async () => {
22+
const items = Array.from({ length: 12 }, (_, index) => index)
23+
let inFlight = 0
24+
let maxInFlight = 0
25+
26+
await mapWithConcurrency(items, 4, async () => {
27+
inFlight += 1
28+
maxInFlight = Math.max(maxInFlight, inFlight)
29+
await new Promise((resolve) => setTimeout(resolve, 10))
30+
inFlight -= 1
31+
})
32+
33+
assert.ok(
34+
maxInFlight <= 4,
35+
`expected at most 4 concurrent calls, saw ${maxInFlight}`,
36+
)
37+
assert.equal(maxInFlight, 4, 'expected concurrency to reach the cap')
38+
})
39+
40+
test('mapWithConcurrency runs in a bounded number of batches, not one call per item', async () => {
41+
const items = Array.from({ length: 18 }, (_, index) => index)
42+
const perItemDelayMs = 20
43+
const concurrency = 6
44+
45+
const start = Date.now()
46+
await mapWithConcurrency(items, concurrency, async () => {
47+
await new Promise((resolve) => setTimeout(resolve, perItemDelayMs))
48+
})
49+
const elapsed = Date.now() - start
50+
51+
const sequentialWorstCase = items.length * perItemDelayMs
52+
assert.ok(
53+
elapsed < sequentialWorstCase / 2,
54+
`expected concurrent execution well under ${sequentialWorstCase}ms, took ${elapsed}ms`,
55+
)
56+
})
57+
58+
console.log('docs manifest concurrency tests passed')
59+
60+
test('collectRedirectEntriesForFile returns no entries (not a rejection) when the file fetch throws a recoverable GitHub error', async () => {
61+
const node: DocsTreeNode = { path: 'docs/guide/old-page.md' }
62+
const paths: Array<string> = []
63+
64+
const entries = await collectRedirectEntriesForFile(node, {
65+
docsRoot: 'docs',
66+
fetchFile: async () => {
67+
throw new GitHubContentError('rate-limit', 'GitHub rate limited this file')
68+
},
69+
onCanonicalPath: (path) => paths.push(path),
70+
})
71+
72+
assert.deepEqual(entries, [])
73+
assert.deepEqual(paths, ['guide/old-page'])
74+
})
75+
76+
test('collectRedirectEntriesForFile rethrows non-recoverable errors instead of silently swallowing them', async () => {
77+
const node: DocsTreeNode = { path: 'docs/guide/old-page.md' }
78+
79+
await assert.rejects(
80+
() =>
81+
collectRedirectEntriesForFile(node, {
82+
docsRoot: 'docs',
83+
fetchFile: async () => {
84+
throw new TypeError('this is a real bug, not a flaky GitHub call')
85+
},
86+
onCanonicalPath: () => {},
87+
}),
88+
TypeError,
89+
)
90+
})
91+
92+
test('a mix of one failing file and several succeeding files still produces every succeeding redirect', async () => {
93+
const nodes: Array<DocsTreeNode> = [
94+
{ path: 'docs/guide/a.md' },
95+
{ path: 'docs/guide/flaky.md' },
96+
{ path: 'docs/guide/b.md' },
97+
]
98+
99+
const fileContents: Record<string, string> = {
100+
'docs/guide/a.md': '---\nredirect_from:\n - guide/old-a\n---\n# A',
101+
'docs/guide/b.md': '---\nredirect_from:\n - guide/old-b\n---\n# B',
102+
}
103+
104+
const paths: Array<string> = []
105+
106+
const results = await mapWithConcurrency(nodes, 3, (node) =>
107+
collectRedirectEntriesForFile(node, {
108+
docsRoot: 'docs',
109+
fetchFile: async (filePath) => {
110+
if (filePath === 'docs/guide/flaky.md') {
111+
throw new GitHubContentError('server', 'GitHub 5xx for this file')
112+
}
113+
114+
return fileContents[filePath] ?? null
115+
},
116+
onCanonicalPath: (path) => paths.push(path),
117+
}),
118+
)
119+
120+
// The whole build did not reject just because one file was flaky.
121+
assert.deepEqual(
122+
results.flat().map((entry) => entry.from),
123+
['guide/old-a', 'guide/old-b'],
124+
)
125+
// All three files' paths were still recorded, including the flaky one.
126+
assert.deepEqual(paths.sort(), ['guide/a', 'guide/b', 'guide/flaky'])
127+
})
128+
129+
console.log('buildDocsManifest per-file fault tolerance tests passed')

0 commit comments

Comments
 (0)