Colossus bug fixes for 3.10.0 release by mnaamani · Pull Request #5016 · Joystream/joystream

mnaamani · 2023-12-28T12:51:36Z

Multiple bug fixes
Refactored and improved pending objects service
Allow re-upload of already "accepted" objects via files api endpoint, in-case they are lost.
Fixed issue in batch extrinsic sender which was not returning correct failed calls.

closes:

Colossus: In memory local data objects cache improvements #4991 - removed async lock, added some assertions
[Colossus] storage failing to query data objects #5002 - fixed bug in files api that was injecting undefined key into idCache map
[Colossus] storage node crashed on upload #5003
- timeout issue sending extrinsic (which caused the next issue) - with use of latest improvements this should no longer happen.
- failure to re-upload - support now added.
- basic 404 file note found. Not a problem really, but maybe it shouldn't be logged as an error?

…aders request handlers Fix for: Joystream#5002

…-fix-objectid-cache-bug

zeeshanakram3

Thanks for investigating and fixing the issue. Wouldn't it be much simpler to store data object ID (req.params.id) at the start of the both GET/HEAD handlers, before the try block starts. This way the data object ID in the finally block would still be valid and we won't have to separately do the unpinning at multiple different places. For example, following change fixes the bug

zeeshanakram3 · 2023-12-28T15:40:24Z

+  if (!(await getDataObjectIdFromCache(dataObjectId))) {
+    sendResponseWithError(res, next, new WebApiError('File Not Found', 404), 'files')
+    return
+  }


Wouldn't this be redundant, since now pinDataObjectIdToCache ensures that ID exists before pinning it? If this is added to return a better error message in the response, then we can provide this error in as an argument of asset in pinDataObjectIdToCache function itself. e.g.

assert(idCache.has(dataObjectId), new WebApiError('File Not Found', 404))

We can also pass the Error as a second optional argument to the pinDataObjectIdToCache function

Remember assertions don't throw and it is intended to fail the process completely. Meaning the caller needs to make sure they are calling at the right time. So if these new assertions fail it means our code is not correct.

Are you suggesting we should be returning/throwing an error/exception instead of these assertions?

I picked assertions because I was treating the user of the idCache as "our own" code. So its not like a library.
With reference to https://softwareengineering.stackexchange.com/questions/15515/when-to-use-assertions-and-when-to-use-exceptions#15518

Let me know what you think.

Remember assertions don't throw and it is intended to fail the process completely.

Maybe I am misunderstanding, but In javascript, the assert module throws the Error of type AssertionError, and we can actually pass a custom error too, that should be thrown in case assertion fails.

You are absolutely correct. And given that is true I think I might be introducing more problems.
Let me think this through and come back to it.

mnaamani · 2023-12-28T16:36:52Z

Thanks for investigating and fixing the issue. Wouldn't it be much simpler to store data object ID (req.params.id) at the start of the both GET/HEAD handlers, before the try block starts. This way the data object ID in the finally block would still be valid and we won't have to separately do the unpinning at multiple different places.

Yes that was one of the reasons I moved the data object id instantiation outside of the try block, but then realized that I had to get rid of the finally clause because it can be reached before the stream ends. So we would be unpinning too early, defeating the whole point.

It is easy to miss such cases when mixing async/await code with traditional node event and steam based paradigm in the same function.

…ker availability

zeeshanakram3 · 2023-12-30T08:31:08Z

+  // Skipping this check to allow re-upload of "lost" objects
  if (dataObject.accepted.valueOf()) {
-    throw new WebApiError(`Data object ${dataObjectId} has already been accepted by storage node`, 400)
+    // throw new WebApiError(`Data object ${dataObjectId} has already been accepted by storage node`, 400)


Please remove this commented code.

zeeshanakram3 · 2023-12-30T08:34:02Z

+  // assert(!idCache.has(dataObjectId))
+  // existence check to avoid resetting pin count


Isn't it better to have this assertion?

zeeshanakram3 · 2023-12-30T08:35:19Z

+    if (!flags.pendingFolder) {
+      logger.warn(
+        'You did not specify a path to the pending directory. ' +
+          'A pending folder under the uploads folder willl be used. ' +


Suggested change

'A pending folder under the uploads folder willl be used. ' +

'A pending folder under the uploads folder will be used. ' +

zeeshanakram3 · 2023-12-30T08:40:28Z

+    pendingFolder: flags.string({
+      description:
+        'Directory to store pending files which are uploaded upload (absolute path).\nIf not specified a subfolder under the uploads directory will be used.',
+    }),


I think making the temp folder configurable does not create any risks, but in the case of the pending folder what would happen if the operator specifies a different pendingFolder in different node runs? Would this lead to considering the objects in the previous pending folder as effectively lost (unless they were re-uploaded)

Correct there is a risk of that happening. The operator can after start node with a new path for pending folder, simply copy the files to the new location and they will be picked up. But I think it is a necessary step towards making the uploads folder separate in preparation for external object storage back-end.

zeeshanakram3 · 2023-12-30T09:07:43Z

+    (req: express.Request, res: express.Response<unknown, AppConfig>, next: NextFunction) => {
+      if (req.path === '/api/v1/files' && (req.method === 'GET' || req.method === 'HEAD')) {
+        validateDataObjectId(req, res)
+          .then(next)
+          .catch((error) => sendResponseWithError(res, next, error, 'upload'))
+      } else {
+        next()
+      }
+    },


This middleware handler would never be invoked since req.path would always be /api/v1/files/{id}, i.e. it would include the data object ID as part of the path.

Also, it's it better to do this validation at the openApi schema level (e.g. example)?

Good catch. Yeah I was surprised actually that the schema was for a string. Maybe it could be of type number rather than regex pattern on a string? Was a string perhaps chosen to support data object ids larger than javascript max number ?

Even that regex expression could allow numbers too large for BN to fail.
With this custom function we could also do checks like if the id outside the range of valid dataobject ids?

zeeshanakram3 · 2023-12-30T09:19:30Z

-
+export function deleteDataObjectIdFromCache(dataObjectId: string): void {
+  assert(typeof dataObjectId === 'string')
+  // assert(idCache.has(dataObjectId))


Isn't it better to have this assertion?

zeeshanakram3 · 2023-12-30T09:44:24Z

    sendResponseWithError(res, next, err, 'files')
-  } finally {
-    await unpinDataObjectIdFromCache(req.params.id)
+    // we assume that if stream.pipe throws, then stream.on('end') will never fire


I think the comment should not be here?

The reason for the comment at both lines where we are invoking the unpin call is to explain that it is not a mistake (based on the assumption), and we should never actually get the result where unpin is invoked twice.
I suppose I could have a locally scoped function that will only even call unpinDataObjectIdFromCache() once and log a warning if it is every invoked multiple times as a signal that this assumption is false without causing a possible corrupt state or unpin throwing the assertion error.

About this whole approach of pinning to prevent the cleanup service deleting a file being served, I may have overlooked that on linux file systems at least, it may not be necessary at all. I should have considered that when reviewing first PR that introduced the clean up service.

I do think we can safely get rid of this.

On top of that, I can't find the comment in the PR but I do remember at one point we were serving content from pending folder if objects was not yet moved to uploads, and I suggested that it was a bad idea as we didn't know what would happen if files was being moved while being served.

I think we should re-introduce ability to serve from pending folder. It will become more important when we move to object storage, if there is a delay in transferring the pending file from local file system to object storage (compared with what we do currently which is just renaming on the save disk volume).

What do you think?

On top of that, I can't find the comment in the PR but I do remember at one point we were serving content from pending folder if objects was not yet moved to uploads, and I suggested that it was a bad idea as we didn't know what would happen if files was being being while being served.

This is the PR where you suggested and mage the change #4989

Made unpinning less error prone and guaranteed to only happen once after pinning in getFile handler.
I read that end event on stream may/or may not fire if error event is fired, so there are potentially 3 places where we would need to unpin..

Done in d5ec0ec

Made unpinning less error prone and guaranteed to only happen once after pinning in getFile handler.

Yeah, this is a good change actually.

zeeshanakram3 · 2023-12-30T10:18:26Z

+    })().catch((err) => {
+      logger.error(`Accept pending objects service died: ${err}`)
+    })


Suggested change

})().catch((err) => {

logger.error(`Accept pending objects service died: ${err}`)

})

})()

Since the async function is in try...catch, I don't think any promise rejections will ever be caught in .catch function. WDYT?

You are correct, I think I was forced to add a catch handler because typescript complained. I'll try to use the previous code style to make this whole function cleaner. The reason I ended up with this was this was the point where I was debugging the issue which ended up being the invalid spread operator on the extrinsic result.

zeeshanakram3 · 2023-12-30T10:22:33Z

+          logger.debug(
            `Data object ${dataObject.id} in pending directory is no longer assigned to any of the upload buckets: ${this.uploadBuckets}.`
          )
+          await fsPromises.unlink(path.join(this.pendingDataObjectsDir, dataObject.id))


I think we discussed that doing this is risky, and should probably by handled by pruning service (#4971 (comment))

So better to move such objects to the uploads folder, and pruning service can handle from there?

Ah yes now I remember. That is a good idea actually.

mnaamani · 2023-12-30T17:18:04Z

+    await Promise.allSettled(
      pendingDataObjects.map(async (dataObject) => {
-        const storageBucket = dataObject.storageBag.storageBuckets.find(({ id }) => this.uploadBuckets.includes(id))
+        // verify the ipfshash of the object matches against runtime!


@zeeshanakram3 what are your thoughts about doing this additional check here?

This check is already being done after the file upload completes and before it is moved to the pending dir. So I think doing that would be redundant?

It is of course redundant for objects that just came in through the http api, but not for files that might have been copied by operator directly. I'm thinking of scenario where operator maybe recovering their setup, moving files from other location after chaing pending folder path. Maybe somehow recovering "lost" objects from a backup/archive and placing them into the pending folder for processing..

Yeah, in those cases it makes sense to have this validation. So maybe just add a comment, briefly explaining why are we doing this? In which scenarios it would be beneficial? As you described above

done in 0cdd273

zeeshanakram3 · 2023-12-30T19:11:49Z

+    (req: express.Request, res: express.Response<unknown, AppConfig>, next: NextFunction) => {
+      if (req.path.startsWith('/api/v1/files/') && (req.method === 'GET' || req.method === 'HEAD')) {
+        validateDataObjectId(req, res)
+          .then(next)
+          .catch((error) => sendResponseWithError(res, next, error, 'upload'))
+      } else {
+        next()
+      }
+    },


Actually, this still won't work correctly as req.params inside validateDataObjectId is empty object ({}). This is because the express does not know about the structure of the URL yet (i.e. whether 1 in /api/v1/files/1 is parameter or part of the URL itself), and it will only know about the params after the next middleware (OpenApiValidator.middleware) is executed.

So unfortunately I think there is no way then doing this validation at openapi schema level

Ah thanks for checking this.
So I'll drop it, and update the handlers to not assume the id has been validated fully, and at least update the schema validation.

Done in 0cdd273

zeeshanakram3

LGTM.

mnaamani added 4 commits December 28, 2023 15:54

storage-node: assertions around idCache and fix getFile and getFileHe…

f383e8b

…aders request handlers Fix for: Joystream#5002

Merge branch 'master' of github.com:Joystream/joystream into colossus…

50c3dc8

…-fix-objectid-cache-bug

storage-node: additional assertions on idCache

727d41e

storage-node: cleaner code in getFile and getFileHeaders handlers

43f6c41

mnaamani requested a review from zeeshanakram3 December 28, 2023 14:22

zeeshanakram3 reviewed Dec 28, 2023

View reviewed changes

mnaamani commented Dec 28, 2023

View reviewed changes

Comment thread storage-node/src/services/caching/localDataObjects.ts Outdated

mnaamani added 2 commits December 29, 2023 01:39

storage-node: await-lock not required for idCache

501581c

storage-node: fix some function docs

69aed2e

mnaamani marked this pull request as draft December 29, 2023 10:14

storage-node: refactor pending objects service and allow re-uploads

1983731

mnaamani force-pushed the colossus-fix-objectid-cache-bug branch from 987aa0d to 1983731 Compare December 29, 2023 20:36

mnaamani marked this pull request as ready for review December 29, 2023 21:52

mnaamani requested a review from zeeshanakram3 December 29, 2023 22:00

mnaamani added 3 commits December 30, 2023 02:31

storage-node: move new uploads to pending and uploads folder for quic…

fe34034

…ker availability

storage-node: revert copying new upload to both uploads and pending

675fc89

storage-node: --pendingFolder optional argument

0001eef

zeeshanakram3 requested changes Dec 30, 2023

View reviewed changes

storage-node: address review comments

3af466c

mnaamani commented Dec 30, 2023

View reviewed changes

Comment thread storage-node/src/services/webApi/controllers/filesApi.ts Outdated

mnaamani requested a review from zeeshanakram3 December 30, 2023 17:21

zeeshanakram3 reviewed Dec 30, 2023

View reviewed changes

mnaamani added 3 commits December 30, 2023 23:40

storage-node: do data object id validation in openapi schema

0cdd273

storage-node: unpin once guaranteed in getFile handler

d5ec0ec

storage-node: validate object ipfshash when loaded from pending folder

ff19297

zeeshanakram3 approved these changes Dec 30, 2023

View reviewed changes

mnaamani changed the title ~~Colossus fix objectid cache bug~~ Colossus bug fixes for 3.10.0 release Dec 30, 2023

mnaamani merged commit 132bcfb into Joystream:master Dec 30, 2023

This was referenced Jan 15, 2024

Colossus: In memory local data objects cache improvements #4991

Closed

[Colossus] storage failing to query data objects #5002

Closed

[Colossus] storage node crashed on upload #5003

Closed

mnaamani deleted the colossus-fix-objectid-cache-bug branch March 12, 2024 10:36

		// assert(!idCache.has(dataObjectId))
		// existence check to avoid resetting pin count

	'A pending folder under the uploads folder willl be used. ' +
	'A pending folder under the uploads folder will be used. ' +

Conversation

mnaamani commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeeshanakram3 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!

zeeshanakram3 Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaamani commented Dec 28, 2023

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!

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!

mnaamani Dec 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zeeshanakram3 Dec 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zeeshanakram3 Dec 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

mnaamani commented Dec 28, 2023 •

edited

Loading

zeeshanakram3 Dec 28, 2023 •

edited

Loading

mnaamani Dec 30, 2023 •

edited

Loading

zeeshanakram3 Dec 30, 2023 •

edited

Loading

zeeshanakram3 Dec 30, 2023 •

edited

Loading