feat: skip ads where entries cannot be retrieved #23

bajtos · 2024-09-24T12:35:18Z

While running the indexer in production, I noticed that many index providers return advertisemnt but then they return "404 cid not found" when we ask for the (payload) entries.

Let's recover from that error by ignoring the advertisement and continuing the walk.

Links:

Lightweight DDO Compatibility: Spark Piece Indexer roadmap#143

While running the indexer in production, I noticed that many index providers return advertisemnt but then they return "404 cid not found" when we ask for the (payload) entries. Let's recover from that error by ignoring the advertisement and continuing the walk. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

indexer/lib/typings.d.ts

indexer/lib/advertisement-walker.js

bajtos · 2024-09-24T15:19:41Z

indexer/lib/advertisement-walker.js

+        /** @type {any} */(err).serverMessage ?? '<not found>'
+      )
+      return {
+        entriesFetchError: true,


This is not the best property name, can you suggest a better one? Or may I should set the value to a string instead of an error?

Suggested change

entriesFetchError: true,

entriesFetchError: String(err.statusCode),

I was just looking up this property, as was assuming it's one of these

an Error

an array of entries for which there was a fetch error

the count of entries for which there was a fetch error

What about entriesFetchError: err? Or otherwise hasEntriesFetchError: true

indexer/test/helpers/http-server.js

indexer/test/advertisement-walker.test.js

juliangruber · 2024-09-25T07:43:31Z

indexer/lib/advertisement-walker.js

+        /** @type {any} */(err).serverMessage ?? '<not found>'
+      )
+      return {
+        entriesFetchError: true,


I was just looking up this property, as was assuming it's one of these

an Error

an array of entries for which there was a fetch error

the count of entries for which there was a fetch error

What about entriesFetchError: err? Or otherwise hasEntriesFetchError: true

juliangruber · 2024-09-25T07:43:59Z

indexer/lib/advertisement-walker.js


  try {
-    const { previousAdvertisementCid, ...entry } = await fetchAdvertisedPayload(
+    const { previousAdvertisementCid, entriesFetchError, ...entry } = await fetchAdvertisedPayload(


why is entriesFetchError plural and entry singular?

Let me explain how this works:

First, we fetch the advertisement object. This object contains a CID linking to another object containing (payload block) entries.

Second, we fetch the object with the entries.

Finally, we take the first entry and discard the rest.

So, the error happens when we are fetching entries, but then we return only a single entry.

indexer/lib/advertisement-walker.js

juliangruber · 2024-09-25T07:47:30Z

indexer/lib/advertisement-walker.js

+        err.statusCode,
+        /** @type {any} */(err).serverMessage ?? '<not found>'
+      )
+      return {


Why are we returning an object with { error } instead of throwing it?

Yeah, this can be confusing, and I am thinking about redesigning my implementation.

What I wanted to accomplish in this PR:

If the request for entries returns 404, we want to skip the advertisement and walk to the next advertisement in the chain. This is handled by returning { error }

If the request for entries fails with a different error, we want to retry it. This is handled by (re)throwing the error.

After thinking about this some more, I am leaning towards the following design:

If the request for entries returns 404, we skip the advertisement. (No change from the current proposal.)

If the request for entries fails differently, we retry it several times. If all attempts fail, then we skip the advertisement, too.

That way, we optimise for finishing the walk of all advertisements so that we can process new advertisements and avoid repeating the same failing request forever.

Co-authored-by: Julian Gruber <julian@juliangruber.com>

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

bajtos · 2024-10-01T12:09:54Z

@juliangruber I reworked my PR as follows:

Inside fetchAdvertisedPayload:

I added pRetry to retry when the server returns a 5xx error. (It does not make sense to retry after an error like 404 because the next request is almost certainly going to fail too.)
I changed the return value to either contains entry object or an error string code.
I recognise two errors - CANNOT_FETCH_ENTRIES and CANNOT_DETERMINE_PIECE_CID
In particular, this function no longer throws when we cannot fetch the content for the Entries CID link.

Inside processNextAdvertisement, I added code to distinguish those two error codes and added a new property state.adsMissingPieceCID.

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

juliangruber

Some suggestions but nothing blocking the PR from being merged

juliangruber · 2024-10-02T08:23:41Z

indexer/lib/advertisement-walker.js

-    const indexEntry = entry.pieceCid ? entry : undefined
+    if (error === 'CANNOT_FETCH_ENTRIES') {
+      state.entriesNotRetrievable = (state.entriesNotRetrievable ?? 0) + 1
+    } else if (error === 'CANNOT_DETERMINE_PIECE_CID') {


Suggested change

} else if (error === 'CANNOT_DETERMINE_PIECE_CID') {

} else if (error === 'MISSING_PIECE_CID') {

To match the state property

I don't know what state property do you refer to. Do you mean the text in the debug logs?

I don't have a strong opinion about the error codes, I will apply your suggestions.

juliangruber · 2024-10-02T08:24:04Z

indexer/lib/advertisement-walker.js

    }

-    const indexEntry = entry.pieceCid ? entry : undefined
+    if (error === 'CANNOT_FETCH_ENTRIES') {


Suggested change

if (error === 'CANNOT_FETCH_ENTRIES') {

if (error === 'ENTRY_NOT_RETRIEVABLE') {

To match the state property

I'll use ENTRIES_NOT_RETRIEVABLE. We cannot retrieve the list of entries. We don't try to retrieve the entry itself.

juliangruber · 2024-10-02T08:24:53Z

indexer/lib/advertisement-walker.js

-}} AdvertisedPayload */
+/**
+ * @param {unknown} err
+ * @param {string} providerAddress


Suggested change

* @param {string} providerAddress

* @param {string} providerAddress

* @returns {string}

TypeScript infers the return type.

But I can see how it can be helpful to see the return type in the JSDoc comment. I'll add @returns {string | undefined}

juliangruber · 2024-10-02T08:25:31Z

indexer/lib/advertisement-walker.js

+  if (!pieceCid) {
+    debug('advertisement %s has no PieceCID in metadata: %j', advertisementCid, meta.deal)
+    return {
+      error: /** @type {const} */('CANNOT_DETERMINE_PIECE_CID'),


what is this type cast for?

Without this cast, the error would have the type string and auto-completion in VS Code won't work.

With the cast, the error has an enum type 'CANNOT_DETERMINE_PIECE_CID' | 'CANNOT_FETCH_ENTRIES'.

Then, when you start writing a condition like if (error === ', VSCode can auto-complete the valid values.

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

bajtos requested a review from juliangruber September 24, 2024 12:35

bajtos commented Sep 24, 2024

View reviewed changes

indexer/lib/typings.d.ts Show resolved Hide resolved

bajtos commented Sep 24, 2024

View reviewed changes

indexer/lib/advertisement-walker.js Outdated Show resolved Hide resolved

bajtos commented Sep 24, 2024

View reviewed changes

juliangruber requested changes Sep 25, 2024

View reviewed changes

bajtos and others added 7 commits September 30, 2024 15:25

Update indexer/test/helpers/http-server.js

bc75a24

Co-authored-by: Julian Gruber <julian@juliangruber.com>

Update indexer/lib/advertisement-walker.js

4311a10

Co-authored-by: Julian Gruber <julian@juliangruber.com>

revert bc75a24 and add comment to explain

a80f971

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

pipe web stream to node response stream

737f284

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

refactor: introduce pRetry and error codes

b83d774

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

fixup! tests

d9707c4

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

Merge branch 'main' into handle-ad-errors

8c27540

bajtos requested a review from juliangruber October 1, 2024 12:10

fixup! improve code formatting

1f06f37

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

juliangruber approved these changes Oct 2, 2024

View reviewed changes

bajtos added 2 commits October 2, 2024 13:33

fixup! rename error codes

a4d7f3a

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

fixup! jsdoc for return type

efd9da6

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>

bajtos merged commit d99d54e into main Oct 2, 2024
10 checks passed

bajtos deleted the handle-ad-errors branch October 2, 2024 11:42

This was referenced Oct 2, 2024

fix: don't log 404 error when fetching entries #29

Merged

feat: expose adsMissingPieceCID and entriesNotRetrievable #30

Merged

	entriesFetchError: true,
	entriesFetchError: String(err.statusCode),

	} else if (error === 'CANNOT_DETERMINE_PIECE_CID') {
	} else if (error === 'MISSING_PIECE_CID') {

	if (error === 'CANNOT_FETCH_ENTRIES') {
	if (error === 'ENTRY_NOT_RETRIEVABLE') {

	* @param {string} providerAddress
	* @param {string} providerAddress
	* @returns {string}

feat: skip ads where entries cannot be retrieved #23

feat: skip ads where entries cannot be retrieved #23

Uh oh!

Conversation

bajtos commented Sep 24, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Oct 1, 2024

Uh oh!

juliangruber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants