Consensys / armlet

a MythX API client wrapper
MIT License
17 stars 7 forks source link

When the backend hits an error state, it is not noticed in armlet. #29

Open rocky opened 5 years ago

rocky commented 5 years ago

On staging right now, there is some sort of bug in the API backend.

$ ./analyses.sh  sample-json/PublicArray.json 
Issuing HTTP POST https://staging.api.mythx.io/v1/analyses
  (with MYTHRIL_ACCESS_TOKEN on file sample-json/PublicArray.json)

curl completed sucessfully. Output follows...
HTTP/2 200 
{
  "apiVersion": "v1.3.0-41-ge2a3895",
  "mythrilVersion": "0.19.11",
  "maruVersion": "0.3.1",
  "queueTime": 3,
  "runTime": 0,
  "status": "Queued",
  "submittedAt": "2019-01-16T22:08:46.991Z",
  "submittedBy": "5bf808544b0aa00010a9c096",
  "uuid": "693f8236-b9d1-47ae-8231-0ab6db86f6f0"
}
$ ./analyses-status.sh 693f8236-b9d1-47ae-8231-0ab6db86f6f0
Issuing HTTP GET https://staging.api.mythx.io/v1/analyses/693f8236-b9d1-47ae-8231-0ab6db86f6f0
  (with MYTHRIL_ACCESS_TOKEN)
curl completed sucessfully. Output follows...
HTTP/2 200 
{
  "apiVersion": "v1.3.0-41-ge2a3895",
  "error": "Error processing task: json: cannot unmarshal string into Go struct field AnalyzeInput.sources of type tasks.SourceContent",
  "mythrilVersion": "0.19.11",
  "maruVersion": "0.3.1",
  "queueTime": 506,
  "runTime": 196,
  "status": "Error",
  "submittedAt": "2019-01-16T22:08:46.991Z",
  "submittedBy": "5bf808544b0aa00010a9c096",
  "uuid": "693f8236-b9d1-47ae-8231-0ab6db86f6f0"
}

Note in the above output the lines:

  "error": "Error processing task: json: cannot unmarshal string into Go struct field AnalyzeInput.sources 
  "status": "Error",

Armlet doesn't notice these or report it.

Please work with @fgimenez and fix. Thanks.

daniyarchambylov commented 5 years ago

@rocky @fgimenez submitted PR https://github.com/ConsenSys/armlet/pull/30

birdofpreyru commented 5 years ago

@daniyarchambylov @rocky

Note that in this case API intentionally replies with 200 OK status, because the operation Get results of a past analysis is successful in this case: it can successfully return analysis result. Though, the analysis result itself says that some of the tools failed to do the analysis, that's reflected in the payload's status.

So, the correct check that analysis completed successfully is to check both HTTP status is 200, and payload's status is finished.

fgimenez commented 5 years ago

@rocky @daniyarchambylov the problem i see is that /analyses-status.sh access the /analyses/${uuid} endpoint, and armlet polls the issues endpoint https://github.com/ConsenSys/armlet/blob/master/lib/poller.js#L25

For the failed status described above, the API in the issues endpoint currently returns a 200 status code and an empty response, which leads to the SyntaxError: Unexpected end of JSON input message. A possible solution could be to poll the /analyses/${uuid} endpoint instead, parse the response and, when the results are available, do an additional request to the issues endpoint, this involves just one more request per analysis, WDYT?

birdofpreyru commented 5 years ago

@fgimenez @rocky Oh... if this is the case, I think we should update the API so that /analyses/{uuid}/issues responds 404 for failed analyses.

fgimenez commented 5 years ago

@birdofpreyru hmm not sure, currently /analyses/{uuid}/issues returns 404 for not finished analyses, now we have a way to at least distinguish between finished and not finished in the issues endpoint. Do you think returning the same status code for both situations (ongoing and failed) improves the situation?

In any case, even if we do this change on the API side (or to be more precise, with more reason if we put this change in place), the client will need to poll the /analyses/${uuid} endpoint to know when the analysis is done.

birdofpreyru commented 5 years ago

@fgimenez well, from the viewpoint of REST API, both situation are 404: you are fetching a resource that does not exist, either because the analysis is still in progress and it will be created later, or because the analysis failed. If these two scenarios need to be distinguished, then your comment above is correct: one should hit /analyses/{uuid} first to check analysis state.

rocky commented 5 years ago

from @fgimenez regarding lib/poller.js:

If you want to get additional information on the JSON parse error we get for the issues endpoint I'm afraid that it won't have any effect because we are returning an empty string. I think that instead of polling the issues endpoint we should poll the analysis endpoint and parse the status.