add catch to deploy entity (#886)

* add catch to deploy entity * more logs * more logs * more logs * more logs * fix test * fix test * fix tests * fix tests * add extra checks * print more information * print error * auto start queue * fix * chore: bump `content-validator` version (#887) * extra log * more logs * add fallback to not fail * remove flaky code * fix tests * comments Co-authored-by: menduz <github@menduz.com> Co-authored-by: guidota <guido@decentraland.org>
decentraland · Jan 29, 2022 · 39df832 · 39df832
1 parent 36b7d5a
commit 39df832
Show file tree

Hide file tree

Showing 28 changed files with 345 additions and 207 deletions.
diff --git a/.vscode/launch.json b/.vscode/launch.json
@@ -4,7 +4,6 @@
   // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
   "version": "0.2.0",
   "configurations": [
-
     {
       "type": "node",
       "request": "attach",
@@ -28,6 +27,21 @@
       "env": {
         "CI": "true"
       }
+    },
+    {
+      "name": "Debug Jest Tests - Content server",
+      "type": "node",
+      "request": "launch",
+      "runtimeArgs": [
+        "--inspect-brk",
+        "${workspaceRoot}/node_modules/.bin/jest",
+        "--runInBand",
+        "test/integration/service/concurrent-deployments.spec.ts"
+      ],
+      "cwd": "${workspaceFolder}/content",
+      "console": "integratedTerminal",
+      "internalConsoleOptions": "neverOpen",
+      "port": 9229
     }
   ]
 }
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -25,6 +25,8 @@
   "files.insertFinalNewline": true,
   "files.trimTrailingWhitespace": true,
   "files.trimFinalNewlines": true,
-  "jestrunner.runOptions": ["--forceExit"],
+  "jestrunner.runOptions": [
+    "--forceExit"
+  ],
   "typescript.preferences.importModuleSpecifier": "relative",
 }
diff --git a/content/jest.config.js b/content/jest.config.js
@@ -7,11 +7,21 @@ module.exports = {
   verbose: true,
   projects: [
     {
+      globals: {
+        'ts-jest': {
+          tsconfig: 'test/tsconfig.json'
+        }
+      },
       displayName: 'unit',
       testMatch: ['**/test/unit/**/*.spec.(ts)'],
       preset: 'ts-jest'
     },
     {
+      globals: {
+        'ts-jest': {
+          tsconfig: 'test/tsconfig.json'
+        }
+      },
       displayName: 'integration',
       testMatch: ['**/test/integration/**/*.spec.(ts)'],
       globalSetup: './jest.globalSetup.ts',

diff --git a/content/package.json b/content/package.json
@@ -17,7 +17,7 @@
     "@catalyst/commons": "0.1.0",
     "@catalyst/contracts": "0.1.0",
     "@dcl/catalyst-api-specs": "1.0.1",
-    "@dcl/content-validator": "1.0.0",
+    "@dcl/content-validator": "1.0.1-20220128144241.commit-9e37746",
     "@dcl/schemas": "3.6.0",
     "@dcl/snapshots-fetcher": "2.0.0",
     "@dcl/urn-resolver": "1.3.0",

diff --git a/content/src/components.ts b/content/src/components.ts
@@ -71,7 +71,7 @@ export async function initComponentsWithEnv(env: Environment): Promise<AppCompon
   const pointerManager = new PointerManager()
 
   const failedDeploymentsCache = createFailedDeploymentsCache()
-  const validator = createValidator({ storage, authenticator, catalystFetcher, env })
+  const validator = createValidator({ storage, authenticator, catalystFetcher, env, logs })
   const serverValidator = createServerValidator({ failedDeploymentsCache })
 
   const deployedEntitiesFilter = createBloomFilterComponent({

diff --git a/content/src/controller/Controller.ts b/content/src/controller/Controller.ts
@@ -25,7 +25,12 @@ import { ContentAuthenticator } from '../service/auth/Authenticator'
 import { DeploymentOptions } from '../service/deployments/types'
 import { getPointerChanges } from '../service/pointers/pointers'
 import { DeploymentPointerChanges, PointerChangesFilters } from '../service/pointers/types'
-import { DeploymentContext, isSuccessfulDeployment, LocalDeploymentAuditInfo } from '../service/Service'
+import {
+  DeploymentContext,
+  isInvalidDeployment,
+  isSuccessfulDeployment,
+  LocalDeploymentAuditInfo
+} from '../service/Service'
 import { ContentItem, RawContent } from '../storage/ContentStorage'
 import { AppComponents } from '../types'
 import { ControllerDeploymentFactory } from './ControllerDeploymentFactory'
@@ -134,14 +139,23 @@ export class Controller {
       if (isSuccessfulDeployment(deploymentResult)) {
         this.components.metrics.increment('dcl_deployments_endpoint_counter', { kind: 'success' })
         res.send({ creationTimestamp: deploymentResult })
-      } else {
+      } else if (isInvalidDeployment(deploymentResult)) {
         this.components.metrics.increment('dcl_deployments_endpoint_counter', { kind: 'validation_error' })
-        Controller.LOGGER.error(`POST /entities - Returning error '${deploymentResult.errors.join('\n')}'`)
+        Controller.LOGGER.error(`POST /entities - Deployment failed (${deploymentResult.errors.join(',')})`)
         res.status(400).send({ errors: deploymentResult.errors }).end()
+      } else {
+        Controller.LOGGER.error(`deploymentResult is invalid ${JSON.stringify(deploymentResult)}`)
+        throw new Error('deploymentResult is invalid')
       }
     } catch (error) {
       this.components.metrics.increment('dcl_deployments_endpoint_counter', { kind: 'error' })
-      Controller.LOGGER.error(`POST /entities - returning error '${error.message}'`)
+      Controller.LOGGER.error(`POST /entities - Internal server error '${error}'`, {
+        entityId,
+        authChain: JSON.stringify(authChain),
+        ethAddress,
+        signature
+      })
+      Controller.LOGGER.error(error)
       res.status(500).end()
     } finally {
       await this.deleteUploadedFiles(deployFiles)

diff --git a/content/src/denylist/DenylistServiceDecorator.ts b/content/src/denylist/DenylistServiceDecorator.ts
@@ -11,21 +11,17 @@ import { AuthChain } from 'dcl-crypto'
 import { DenylistRepository } from '../repository/extensions/DenylistRepository'
 import { Repository } from '../repository/Repository'
 import { DB_REQUEST_PRIORITY } from '../repository/RepositoryQueue'
-import { ContentAuthenticator } from '../service/auth/Authenticator'
 import { DeploymentOptions } from '../service/deployments/types'
-import { EntityFactory } from '../service/EntityFactory'
 import {
   DeploymentContext,
   DeploymentFiles,
   DeploymentResult,
   LocalDeploymentAuditInfo,
   MetaverseContentService
 } from '../service/Service'
-import { ServiceImpl } from '../service/ServiceImpl'
 import { ContentItem } from '../storage/ContentStorage'
 import { Denylist } from './Denylist'
 import {
-  buildAddressTarget,
   buildContentTarget,
   buildEntityTarget,
   buildPointerTarget,
@@ -102,16 +98,8 @@ export class DenylistServiceDecorator implements MetaverseContentService {
     auditInfo: LocalDeploymentAuditInfo,
     context: DeploymentContext
   ): Promise<DeploymentResult> {
-    return this.repository.task(
-      async (task) => {
-        // Validate the deployment
-        const hashedFiles = await this.validateDeployment(task.denylist, files, entityId, auditInfo)
-
-        // If all validations passed, then deploy the entity
-        return this.service.deployEntity(hashedFiles, entityId, auditInfo, context, task)
-      },
-      { priority: DB_REQUEST_PRIORITY.HIGH }
-    )
+    // TODO: validations were removed in PR #866
+    return this.service.deployEntity(files, entityId, auditInfo, context)
   }
 
   async getDeployments(options?: DeploymentOptions): Promise<PartialDeploymentHistory<Deployment>> {
@@ -244,42 +232,42 @@ export class DenylistServiceDecorator implements MetaverseContentService {
     return this.service.getActiveDeploymentsByContentHash(hash)
   }
 
-  private async validateDeployment(
-    denylistRepo: DenylistRepository,
-    files: DeploymentFiles,
-    entityId: EntityId,
-    auditInfo: LocalDeploymentAuditInfo
-  ) {
-    // No deployments from denylisted eth addresses are allowed
-    const ownerAddress = ContentAuthenticator.ownerAddress(auditInfo.authChain)
-    if (await this.areDenylisted(denylistRepo, buildAddressTarget(ownerAddress))) {
-      throw new Error(`Can't allow a deployment from address '${ownerAddress}' since it was denylisted.`)
-    }
-
-    // Find the entity file
-    const hashes = await ServiceImpl.hashFiles(files, entityId)
-    const entityFile = hashes.get(entityId)
-    if (!entityFile) {
-      throw new Error(`Failed to find the entity file.`)
-    }
-
-    // Parse entity file into an Entity
-    const entity: Entity = EntityFactory.fromBufferWithId(entityFile, entityId)
-
-    // No deployments with denylisted hash are allowed
-    const contentTargets: DenylistTarget[] = (entity.content ?? []).map((item) => buildContentTarget(item.hash))
-    if (await this.areDenylisted(denylistRepo, ...contentTargets)) {
-      throw new Error(`Can't allow the deployment since the entity contains a denylisted content.`)
-    }
-
-    // No deployments on denylisted pointers are allowed
-    const pointerTargets: DenylistTarget[] = entity.pointers.map((pointer) => buildPointerTarget(entity.type, pointer))
-    if (await this.areDenylisted(denylistRepo, ...pointerTargets)) {
-      throw new Error(`Can't allow the deployment since the entity contains a denylisted pointer.`)
-    }
-
-    return hashes
-  }
+  // private async validateDeployment(
+  //   denylistRepo: DenylistRepository,
+  //   files: DeploymentFiles,
+  //   entityId: EntityId,
+  //   auditInfo: LocalDeploymentAuditInfo
+  // ) {
+  //   // No deployments from denylisted eth addresses are allowed
+  //   const ownerAddress = ContentAuthenticator.ownerAddress(auditInfo.authChain)
+  //   if (await this.areDenylisted(denylistRepo, buildAddressTarget(ownerAddress))) {
+  //     throw new Error(`Can't allow a deployment from address '${ownerAddress}' since it was denylisted.`)
+  //   }
+
+  //   // Find the entity file
+  //   const hashes = await ServiceImpl.hashFiles(files, entityId)
+  //   const entityFile = hashes.get(entityId)
+  //   if (!entityFile) {
+  //     throw new Error(`Failed to find the entity file.`)
+  //   }
+
+  //   // Parse entity file into an Entity
+  //   const entity: Entity = EntityFactory.fromBufferWithId(entityFile, entityId)
+
+  //   // No deployments with denylisted hash are allowed
+  //   const contentTargets: DenylistTarget[] = (entity.content ?? []).map((item) => buildContentTarget(item.hash))
+  //   if (await this.areDenylisted(denylistRepo, ...contentTargets)) {
+  //     throw new Error(`Can't allow the deployment since the entity contains a denylisted content.`)
+  //   }
+
+  //   // No deployments on denylisted pointers are allowed
+  //   const pointerTargets: DenylistTarget[] = entity.pointers.map((pointer) => buildPointerTarget(entity.type, pointer))
+  //   if (await this.areDenylisted(denylistRepo, ...pointerTargets)) {
+  //     throw new Error(`Can't allow the deployment since the entity contains a denylisted pointer.`)
+  //   }
+
+  //   return hashes
+  // }
 
   /** Since entity ids are also file hashes, we need to check for all possible targets */
   private getHashTargets(fileHash: string): DenylistTarget[] {

diff --git a/content/src/logic/deployments.ts b/content/src/logic/deployments.ts
@@ -2,6 +2,7 @@ import { ILoggerComponent } from '@well-known-components/interfaces'
 import { FailedDeployment } from '../ports/failedDeploymentsCache'
 import { DeploymentContext } from '../service/Service'
 import { deployEntityFromRemoteServer } from '../service/synchronization/deployRemoteEntity'
+import { IGNORING_FIX_ERROR } from '../service/validations/server'
 import { AppComponents } from '../types'
 import { deploymentExists } from './database-queries/deployments-queries'
 
@@ -52,10 +53,14 @@ export async function retryFailedDeploymentExecution(
         )
       } catch (error) {
         // it failed again, override failed deployment error description
-        const errorDescription = error.message
-        components.failedDeploymentsCache.reportFailure({ ...failedDeployment, errorDescription })
+        const errorDescription = error.message + ''
 
-        logger.info(`Failed to fix deployment of entity`, { entityId, entityType })
+        // TODO [mendez] this condition has no test coverage
+        if (!errorDescription.startsWith(IGNORING_FIX_ERROR)) {
+          components.failedDeploymentsCache.reportFailure({ ...failedDeployment, errorDescription })
+        }
+
+        logger.error(`Failed to fix deployment of entity`, { entityId, entityType, errorDescription })
         logger.error(error)
       }
     } else {

diff --git a/content/src/repository/RepositoryQueue.ts b/content/src/repository/RepositoryQueue.ts
@@ -23,7 +23,7 @@ export class RepositoryQueue {
       timeout: RepositoryQueue.DEFAULT_TIMEOUT,
       ...withoutUndefined
     }
-    this.queue = new PQueue({ concurrency: maxConcurrency, timeout: ms(timeout) })
+    this.queue = new PQueue({ concurrency: maxConcurrency, timeout: ms(timeout), autoStart: true })
     this.maxQueued = maxQueued
   }
 

diff --git a/content/src/service/Service.ts b/content/src/service/Service.ts
@@ -52,6 +52,9 @@ export type DeploymentEvent = {
 }
 
 export type InvalidResult = { errors: string[] }
+export function InvalidResult(val: InvalidResult): InvalidResult {
+  return val
+}
 
 export type DeploymentResult = Timestamp | InvalidResult
 
@@ -61,8 +64,12 @@ export function isSuccessfulDeployment(deploymentResult: DeploymentResult): depl
   return typeof deploymentResult === 'number'
 }
 
-export function isInvalidDeployment(deploymentResult: DeploymentResult): deploymentResult is InvalidResult {
-  return !isSuccessfulDeployment(deploymentResult)
+export function isInvalidDeployment(deploymentResult: any): deploymentResult is InvalidResult {
+  if (deploymentResult && typeof deploymentResult === 'object' && Array.isArray(deploymentResult['errors'])) {
+    return true
+  }
+
+  return false
 }
 
 export enum DeploymentContext {