Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: set max number of car cids to resolve #24

Merged
merged 2 commits into from
Mar 2, 2023
Merged

feat: set max number of car cids to resolve #24

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1203d40
feat: set max number of car cids to resolve
vasco-santos Feb 28, 2023
9d4c9ff
chore: apply suggestions from code review
vasco-santos Mar 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/bindings.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export interface Environment {
CARPARK: R2Bucket
DUDEWHERE: R2Bucket
SATNAV: R2Bucket
MAX_CAR_CIDS_TO_RESOLVE: string
}

export interface CarCidsContext extends Context {
Expand Down
7 changes: 7 additions & 0 deletions src/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,13 @@ export function withCarCids (handler) {
throw new HttpError('missing origin CAR CID(s)', { status: 400 })
}

// Cloudflare currently sets a limit of 1000 sub-requests within the worker context
// If we have a given root CID splitted across hundreds of CARs, freeway will hit
// the sub-requests limit and not serve content anyway
if (carCids.length > Number(env.MAX_CAR_CIDS_TO_RESOLVE)) {
throw new HttpError('number CAR CIDs is too large to resolve', { status: 501 })
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move into the while loop above for quicker response?


return handler(request, env, { ...ctx, carCids })
}
}
Expand Down
10 changes: 10 additions & 0 deletions test/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,14 @@ describe('freeway', () => {
const output = new Uint8Array(await res.arrayBuffer())
assert(equals(input[0].content, output))
})

it('should fail when divided into more than 120 CAR files', async () => {
const input = [{ path: 'sargo.tar.xz', content: randomBytes(1218523560) }]
const { dataCid } = await builder.add(input)

const res = await miniflare.dispatchFetch(`http://localhost:8787/ipfs/${dataCid}/${input[0].path}`)

assert(!res.ok)
assert.equal(res.status, 501)
})
})
7 changes: 7 additions & 0 deletions wrangler.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ r2_buckets = [
[env.production.build]
command = "npm run build"

[env.production.vars]
MAX_CAR_CIDS_TO_RESOLVE = "250"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tricky because a DAG might be split across 999 CARs but because of the path you might be able to extract /smol.json with the last sub-request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With our 10MB CAR shards it'll restrict freeway from serving anything that's over 2.5GB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the disadvantage for these use cases. However, at this point looks like the priority should be to reduce R2 ops and avoid potential error states mentioned in issue (break in mid stream without feedback for user, break after worker run out of sub-requests in general). When we have bigger sub-requests limit and better R2 perf we can increase these.

We can tweak more the number if you feel like there is a better compromise. Thoughts?


# Staging!
[env.staging]
account_id = "fffa4b4363a7e5250af8357087263b3a"
Expand All @@ -40,6 +43,9 @@ r2_buckets = [
[env.staging.build]
command = "npm run build"

[env.staging.vars]
MAX_CAR_CIDS_TO_RESOLVE = "250"

# Test!
[env.test]
workers_dev = true
Expand All @@ -51,6 +57,7 @@ r2_buckets = [

[env.test.vars]
DEBUG = "true"
MAX_CAR_CIDS_TO_RESOLVE = "120"

[env.alanshaw]
workers_dev = true
Expand Down