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.

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!: return allocated bytes in store/add receipt #1213

Merged
merged 10 commits into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 25 additions & 16 deletions packages/capabilities/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -438,18 +438,34 @@ export type StoreRemove = InferInvokedCapability<typeof StoreCaps.remove>
export type StoreList = InferInvokedCapability<typeof StoreCaps.list>

export type StoreAddSuccess = StoreAddSuccessDone | StoreAddSuccessUpload
export interface StoreAddSuccessDone {
status: 'done'

export type StoreAddSuccessStatusUpload = 'upload'
export type StoreAddSuccessStatusDone = 'done'

export interface StoreAddSuccessResult {
/**
* Status of the item to store. A "done" status indicates that it is not
* necessary to upload the item. An "upload" status indicates that the item
* should be uploaded to the provided URL.
*/
status: StoreAddSuccessStatusUpload | StoreAddSuccessStatusDone
/**
* Total bytes allocated in the space to accommodate this stored item.
* May be zero if the item is _already_ stored in _this_ space.
*/
allocated: number
Copy link
Contributor

@gobengo gobengo Dec 4, 2023

Choose a reason for hiding this comment

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

We may want to advertise how much we can ensure consistency of this, which I imagine might be 'not very much' unless we have transactions in our underlying db

e.g. let's say cid A is not stored in the space. Two store/add invocations get created with different invocations CIDs (e.g. different nonce/expirty) to add cid A to the space. Then those two invocations are submitted to us at the exact same second. iiuc it may be the case that both of those responses come back with allocated: size(A) (because we dont have transactions, and in this scenario both invocations calls carStoreBucket.has(link) here would have returned false.

my main concern

  • someone might use this allocated response value in some business logic without being aware of the eventual consistency lack-of-transactionality in generating this value. i.e. this could be nonzero even if technically the a different store/add invocation added the CID first (in the race condition describes above).
  • in that race condition, the same CID may have multiple store/add receipts whose results have allocated set to the size of the referent of the CID.

If we want to return this information in the result of the write operation (store/add), we probably need something like transactions to do it in a way that isn't subject to these race conditions.
But if possible it may be nice to avoid returning this value in the result of the write operation (because it almost always increases the cost of the write operation to add return values that. must also be consistent)

alternatively we ignore the above concerns but maybe it's a good idea to at least put in the jsdoc here that there a nonzero response does not necessarily indicate that the store/add is the authoritative addition of the CID to the space.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point, in hindsight this is maybe not the right solution because we want to use it in our business logic...

Copy link
Contributor

Choose a reason for hiding this comment

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

good point, so maybe we want the insert later on to have a result of being conflict or not. Only one Put should happen in dynamo successfully

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, on reflection I think we can make allocated in the response be authoratitive about the addition of the CID to the space. This is what we can do:

  • We don't need TransactWriteItems since we're not writing multiple items!
  • We can use conditional expressions to ensure writes fail:
    • Change store/add PutItem so that it fails if not a new record
    • Change store/remove DeleteItem so that it fails if record does not exist

/** DID of the space this item will be stored in. */
with: DID
/** CID of the item. */
link: UnknownLink
url?: undefined
headers?: undefined
}

export interface StoreAddSuccessUpload {
status: 'upload'
with: DID
link: UnknownLink
export interface StoreAddSuccessDone extends StoreAddSuccessResult {
status: StoreAddSuccessStatusDone
}

export interface StoreAddSuccessUpload extends StoreAddSuccessResult {
status: StoreAddSuccessStatusUpload
url: ToString<URL>
headers: Record<string, string>
}
Expand Down Expand Up @@ -497,14 +513,7 @@ export type UploadAddSuccess = Omit<UploadListItem, 'insertedAt' | 'updatedAt'>

export type UploadGetSuccess = UploadListItem

export type UploadRemoveSuccess = UploadDidRemove | UploadDidNotRemove

export interface UploadDidRemove extends UploadAddSuccess {}

export interface UploadDidNotRemove {
root?: undefined
shards?: undefined
}
export type UploadRemoveSuccess = UploadAddSuccess

export interface UploadListSuccess extends ListResponse<UploadListItem> {}

Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-config-w3up/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ module.exports = {
EXPERIMENTAL_useProjectService: true,
},
rules: {
"@typescript-eslint/no-floating-promises": "error",
'@typescript-eslint/no-floating-promises': 'error',
'@typescript-eslint/no-unused-vars': 'off',
'@typescript-eslint/ban-ts-comment': 'off',

Expand Down
2 changes: 1 addition & 1 deletion packages/filecoin-api/test/events/aggregator.js
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,7 @@ export const test = {
minPieceInsertedAt: new Date().toISOString(),
}
const putAggregateRes = await context.aggregateStore.put(
aggregateRecord,
aggregateRecord
)
assert.ok(putAggregateRes.ok)

Expand Down
4 changes: 1 addition & 3 deletions packages/upload-api/src/admin/store/inspect.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ const inspect = async ({ capability }, context) => {
return { error: new UnknownProvider(capability.with) }
}

return {
ok: await context.storeTable.inspect(capability.nb.link),
}
return await context.storeTable.inspect(capability.nb.link)
}

class UnknownProvider extends Provider.Failure {
Expand Down
4 changes: 1 addition & 3 deletions packages/upload-api/src/admin/upload/inspect.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ const inspect = async ({ capability }, context) => {
return { error: new UnknownProvider(capability.with) }
}

return {
ok: await context.uploadTable.inspect(capability.nb.root),
}
return await context.uploadTable.inspect(capability.nb.root)
}

class UnknownProvider extends Provider.Failure {
Expand Down
31 changes: 20 additions & 11 deletions packages/upload-api/src/store/add.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export function storeAddProvider(context) {
Server.DID.parse(capability.with).did()
)
const issuer = invocation.issuer.did()
const [allocated, carIsLinkedToAccount, carExists] = await Promise.all([
const [allocated, carExists] = await Promise.all([
allocate(
{
capability: {
Expand All @@ -33,7 +33,6 @@ export function storeAddProvider(context) {
},
context
),
storeTable.exists(space, link),
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, we drop a request :)

carStoreBucket.has(link),
])

Expand All @@ -42,21 +41,30 @@ export function storeAddProvider(context) {
return allocated
}

if (!carIsLinkedToAccount) {
await storeTable.insert({
space,
link,
size,
origin,
issuer,
invocation: invocation.cid,
})
let allocatedSize = size
const res = await storeTable.insert({
space,
link,
size,
origin,
issuer,
invocation: invocation.cid,
})
if (res.error) {
// if the insert failed with conflict then this item has already been
// added to the space and there is no allocation change.
if (res.error.name === 'RecordKeyConflict') {
allocatedSize = 0
} else {
return res
}
}

if (carExists) {
return {
ok: {
status: 'done',
allocated: allocatedSize,
with: space,
link,
},
Expand All @@ -67,6 +75,7 @@ export function storeAddProvider(context) {
return {
ok: {
status: 'upload',
allocated: allocatedSize,
with: space,
link,
url: url.toString(),
Expand Down
14 changes: 4 additions & 10 deletions packages/upload-api/src/store/get.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as Server from '@ucanto/server'
import * as Store from '@web3-storage/capabilities/store'
import * as API from '../types.js'
import { StoreItemNotFound } from './lib.js'

/**
* @param {API.StoreServiceContext} context
Expand All @@ -14,16 +15,9 @@ export function storeGetProvider(context) {
}
const space = Server.DID.parse(capability.with).did()
const res = await context.storeTable.get(space, link)
if (!res) {
return {
error: {
name: 'StoreItemNotFound',
message: 'Store item not found',
},
}
}
return {
ok: res,
if (res.error && res.error.name === 'RecordNotFound') {
return Server.error(new StoreItemNotFound(space, link))
vasco-santos marked this conversation as resolved.
Show resolved Hide resolved
}
return res
})
}
29 changes: 29 additions & 0 deletions packages/upload-api/src/store/lib.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { Failure } from '@ucanto/server'

export class StoreItemNotFound extends Failure {
/**
* @param {import('@ucanto/interface').DID} space
* @param {import('@ucanto/interface').UnknownLink} link
*/
constructor(space, link) {
super()
this.space = space
this.link = link
}

get name() {
return 'StoreItemNotFound'
}

describe() {
return `${this.link} not found in ${this.space}`
}

toJSON() {
return {
...super.toJSON(),
space: this.space,
link: { '/': this.link.toString() },
}
}
}
9 changes: 1 addition & 8 deletions packages/upload-api/src/store/list.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,6 @@ export function storeListProvider(context) {
return Server.provide(Store.list, async ({ capability }) => {
const { cursor, size, pre } = capability.nb
const space = Server.DID.parse(capability.with).did()

return {
ok: await context.storeTable.list(space, {
size,
cursor,
pre,
}),
}
return await context.storeTable.list(space, { size, cursor, pre })
})
}
37 changes: 4 additions & 33 deletions packages/upload-api/src/store/remove.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as Server from '@ucanto/server'
import * as Store from '@web3-storage/capabilities/store'
import * as API from '../types.js'
import { StoreItemNotFound } from './lib.js'

/**
* @param {API.StoreServiceContext} context
Expand All @@ -11,41 +12,11 @@ export function storeRemoveProvider(context) {
const { link } = capability.nb
const space = Server.DID.parse(capability.with).did()

const item = await context.storeTable.get(space, link)
if (!item) {
const res = await context.storeTable.remove(space, link)
if (res.error && res.error.name === 'RecordNotFound') {
return Server.error(new StoreItemNotFound(space, link))
}

await context.storeTable.remove(space, link)

return Server.ok({ size: item.size })
return res
})
}

class StoreItemNotFound extends Server.Failure {
/**
* @param {import('@ucanto/interface').DID} space
* @param {import('@ucanto/interface').UnknownLink} link
*/
constructor(space, link) {
super()
this.space = space
this.link = link
}

get name() {
return 'StoreItemNotFound'
}

describe() {
return `${this.link} not found in ${this.space}`
}

toJSON() {
return {
...super.toJSON(),
space: this.space,
link: { '/': this.link.toString() },
}
}
}
60 changes: 48 additions & 12 deletions packages/upload-api/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -422,28 +422,64 @@ export interface DudewhereBucket {
put: (dataCid: string, carCid: string) => Promise<void>
}

/**
* Indicates the requested record was not present in the table.
*/
export interface RecordNotFound extends Failure {
name: 'RecordNotFound'
}

/**
* Indicates the inserted record key conflicts with an existing key of a record
* that already exists in the table.
*/
export interface RecordKeyConflict extends Failure {
name: 'RecordKeyConflict'
}

export interface StoreTable {
inspect: (link: UnknownLink) => Promise<StoreInspectSuccess>
exists: (space: DID, link: UnknownLink) => Promise<boolean>
get: (space: DID, link: UnknownLink) => Promise<StoreGetSuccess | undefined>
insert: (item: StoreAddInput) => Promise<StoreAddOutput>
remove: (space: DID, link: UnknownLink) => Promise<void>
inspect: (link: UnknownLink) => Promise<Result<StoreInspectSuccess, Failure>>
exists: (space: DID, link: UnknownLink) => Promise<Result<boolean, Failure>>
get: (
space: DID,
link: UnknownLink
) => Promise<Result<StoreGetSuccess, RecordNotFound>>
/** Inserts an item in the table if it does not already exist. */
insert: (
item: StoreAddInput
) => Promise<Result<StoreAddOutput, RecordKeyConflict>>
/** Removes an item from the table but fails if the item does not exist. */
remove: (
space: DID,
link: UnknownLink
) => Promise<Result<StoreRemoveSuccess, RecordNotFound>>
list: (
space: DID,
options?: ListOptions
) => Promise<ListResponse<StoreListItem>>
) => Promise<Result<ListResponse<StoreListItem>, Failure>>
}

export interface UploadTable {
inspect: (link: UnknownLink) => Promise<UploadInspectSuccess>
exists: (space: DID, root: UnknownLink) => Promise<boolean>
get: (space: DID, link: UnknownLink) => Promise<UploadGetSuccess | undefined>
insert: (item: UploadAddInput) => Promise<UploadAddSuccess>
remove: (space: DID, root: UnknownLink) => Promise<UploadRemoveSuccess | null>
inspect: (link: UnknownLink) => Promise<Result<UploadInspectSuccess, Failure>>
exists: (space: DID, root: UnknownLink) => Promise<Result<boolean, Failure>>
get: (
space: DID,
link: UnknownLink
) => Promise<Result<UploadGetSuccess, RecordNotFound>>
/**
* Inserts an item in the table if it does not already exist or updates an
* existing item if it does exist.
*/
upsert: (item: UploadAddInput) => Promise<Result<UploadAddSuccess, Failure>>
/** Removes an item from the table but fails if the item does not exist. */
remove: (
space: DID,
root: UnknownLink
) => Promise<Result<UploadRemoveSuccess, RecordNotFound>>
list: (
space: DID,
options?: ListOptions
) => Promise<ListResponse<UploadListItem>>
) => Promise<Result<ListResponse<UploadListItem>, Failure>>
}

export type SpaceInfoSuccess = {
Expand Down
4 changes: 2 additions & 2 deletions packages/upload-api/src/upload/add.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export function uploadAddProvider(context) {

const [res] = await Promise.all([
// Store in Database
uploadTable.insert({
uploadTable.upsert({
space,
root,
shards,
Expand All @@ -40,7 +40,7 @@ export function uploadAddProvider(context) {
writeDataCidToCarCidsMapping(dudewhereBucket, root, shards),
])

return { ok: res }
return res
})
}

Expand Down
Loading
Loading