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: integrate agent store for idempotence & invocation/receipt persistence #1444

Merged
merged 49 commits into from
May 16, 2024

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented May 13, 2024

This is a draft that I could not finished on Friday, but I thought I'd share in case @vasco-santos or @alanshaw are looking into related stuff on Monday.

PR introduces AgentStore that is used for writing ucanto AgentMessage's from which receipts and invocations get indexed. This store is then consulted by a service during invocations to provide idempotence or more simply to return receipt without executing a task if we already have one.

Main goal here is to take out custom logic out of w3infra and to remove need for manual receipt / invocation storing in the handlers.

⚠️ It is worth calling out that this is significantly changes how things work. In the past sending same invocation used to re-run the handler, with this change that is no longer the case and you'll get a same receipt back (which is a neat way to lookup receipts actually). However that also means that bunch of our tests were affected. I don't expect it would affect real world cases however, because same invocation send second later will have unique CID (due to how we generate expiry) however it does affect tests because we don't wait a second between calls. That is why nonce's were added in a lot of places, but that is not something I expect users would have to do.

@Gozala Gozala marked this pull request as ready for review May 15, 2024 01:28
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Overall looks good to me!

Not approved essentially because agent store tests should be expanded as reviewed inline

Also worth mentioning, there is a big refactor here on blob/add that would be much easier to be a separate PR instead of intertwined with a lot of other changes. The code seems way better documented for folks without big prior knowledge, so I am happy with these changes still. But made reviewing much more complicated and time consuming

// in order to issue blob/accept.
const [, allocation] = /** @type {API.UCANAwait} */ (put.nb.url)['ucan/await']
const result = await context.agentStore.invocations.get(allocation)
// If could not find blob/allocate invocation there is something wrong in
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would be great to send this into a DLQ where we could be alerted. Probably an issue for now would be good, as we would also need to do the setup for DLQ monitoring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably set this up as capability handler and dispatch to it, that way we would be able to throw here which will end up triggering catch handler of the server and get logged instead.

Does that sound like a better option ?

packages/upload-api/test/helpers/blob.js Outdated Show resolved Hide resolved
packages/upload-api/src/blob/add.js Show resolved Hide resolved
Comment on lines +49 to +53
// Verify blob is within the max upload size.
if (capability.nb.blob.size > context.maxUploadSize) {
// While blob may exceed current maxUploadSize limit it could be that limit
// was higher in the past and user had this blob uploaded already in which
// case we should not error.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why this moved here instead of blob/add. Would you expect that write targets could have different max sizes? Otherwise, I don't understand the change as it results in way more processing for same end result

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 think you are making some good points on this. That said, I also think that I also think there should be no blob/add handler it should be a static workflow like one illustrated here storacha/RFC#25 (comment)

Perhaps what we miss is a prior step that checks that blob size is below the max limit (and that step can even run locally on the invoking agent (like http/put) as opposed to service.

Overall sounds like spec needs to address this by calling out the verification step that needs to take place. In the meantime I suggest we keep this here and we can address this as part of router design which will actually deal with multiple allocation sites. Does this sound reasonable ?

If you feel very strongly about it I can move this back into the blob/add handler, however please note that allocation site would need to have this kind of check anyway as they may have different constraints to work with.

// that were awaiting in the background. In the future task scheduler is
// expected to handle coordination of tasks and their dependencies. In the
// meantime we poll `blob/allocate` tasks that were awaiting for the
// `http/put` receipt.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if we need to add further triggers like this in the future, we would need to decouple this into where it is. There is why I would check on it, and I still think it is better to do

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'm not sure I follow what you are saying here. Are you saying what if we need to resume some other blocked task ? If so I think answer there would to add something like Scheduler.poll(....) which would go and poll each task we need to poll that way logic around individual tasks lives where those task are defined as opposed to been entangled with conclude handler with a branched logic

Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM after mentioned test in

@Gozala Gozala merged commit c9bf33e into main May 16, 2024
15 checks passed
@Gozala Gozala deleted the feat/agent-store branch May 16, 2024 21:13
vasco-santos pushed a commit that referenced this pull request May 23, 2024
🤖 I have created a release *beep* *boop*
---


##
[7.0.0](filecoin-api-v6.0.1...filecoin-api-v7.0.0)
(2024-05-23)


### ⚠ BREAKING CHANGES

* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))

### Features

* add blob protocol to upload-client
([#1425](#1425))
([49aef56](49aef56))
* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))
([c9bf33e](c9bf33e))


### Fixes

* check service did in w3filecoin
([#1476](#1476))
([11b00bf](11b00bf))


### Other Changes

* appease linter
([782c6d0](782c6d0))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
vasco-santos pushed a commit that referenced this pull request May 29, 2024
🤖 I have created a release *beep* *boop*
---


##
[16.0.0](upload-client-v15.0.0...upload-client-v16.0.0)
(2024-05-23)


### ⚠ BREAKING CHANGES

* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))

### Features

* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))
([c9bf33e](c9bf33e))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
vasco-santos pushed a commit that referenced this pull request May 30, 2024
🤖 I have created a release *beep* *boop*
---


##
[20.0.0](access-v19.0.0...access-v20.0.0)
(2024-05-30)


### ⚠ BREAKING CHANGES

* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))

### Features

* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))
([c9bf33e](c9bf33e))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
vasco-santos added a commit that referenced this pull request May 30, 2024
🤖 I have created a release *beep* *boop*
---


##
[17.0.0](upload-api-v16.0.0...upload-api-v17.0.0)
(2024-05-30)


### ⚠ BREAKING CHANGES

* updates agent-store api to unblock integration with w3infra
([#1479](#1479))
* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))

### Features

* updates agent-store api to unblock integration with w3infra
([#1479](#1479))
([2998a93](2998a93))
* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))
([c9bf33e](c9bf33e))
* use digest in `blob/accept` location commitment
([#1480](#1480))
([ade45eb](ade45eb))


### Fixes

* rename blob and index client capabilities
([#1478](#1478))
([17e3a31](17e3a31))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Vasco Santos <[email protected]>
vasco-santos added a commit that referenced this pull request Jun 4, 2024
🤖 I have created a release *beep* *boop*
---


##
[14.0.0](w3up-client-v13.1.1...w3up-client-v14.0.0)
(2024-06-04)


### ⚠ BREAKING CHANGES

* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))
* delegated capabilities required to use `uploadFile`, `uploadDirectory`
and `uploadCAR` have changed. In order to use these methods your agent
will now need to be delegated `blob/add`, `index/add`, `filecoin/offer`
and `upload/add` capabilities. Note: no code changes are required.

### Features

* generate sharded DAG index on client and invoke w `index/add`
([#1451](#1451))
([a6d9026](a6d9026))
* **upload-api:** integrate agent store for idempotence &
invocation/receipt persistence
([#1444](#1444))
([c9bf33e](c9bf33e))


### Fixes

* check for blob/accept receipts before blob/add is concluded
([#1459](#1459))
([462518c](462518c))
* export blob client
([#1485](#1485))
([7944077](7944077))
* rename blob and index client capabilities
([#1478](#1478))
([17e3a31](17e3a31))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Vasco Santos <[email protected]>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants