Greg Arndt [:garndt] Reporter
	Description • 7 years ago

Some errors that are caught when trying to upload an artifact result in the worker to panic.  The queue has a few scenarios where an error (such as 400) is returned because of input errors (such as artifact expiration > task expiration) where it would probably be better to log that to the task log and fail the task.

One such example is here:
https://queue.taskcluster.net/v1/task/V-qWmL-PSzaogy8YTcOqaQ

It looks like the situation is handled here:
https://github.com/taskcluster/generic-worker/blob/master/artifacts.go#L372-L378

In the situation where the task log shows that the task it was successful (like for this task), but was resolved with worker-shutdown it can seem that the automatic retrigger for this exception will fix the issue, but really it's a problem with the task definition to begin with, and subsequent retriggers will fail as well.

	Pete Moore [:pmoore][:pete]
	Comment 1 • 7 years ago

(In reply to Greg Arndt [:garndt] from comment #0)
> Some errors that are caught when trying to upload an artifact result in the
> worker to panic.  The queue has a few scenarios where an error (such as 400)
> is returned because of input errors (such as artifact expiration > task
> expiration) where it would probably be better to log that to the task log
> and fail the task.

That particular check is handled up front, when the payload is validated, so does not cause a worker panic.

> One such example is here:
> https://queue.taskcluster.net/v1/task/V-qWmL-PSzaogy8YTcOqaQ
> 
> It looks like the situation is handled here:
> https://github.com/taskcluster/generic-worker/blob/master/artifacts.go#L372-
> L378
> 
> In the situation where the task log shows that the task it was successful
> (like for this task), but was resolved with worker-shutdown it can seem that
> the automatic retrigger for this exception will fix the issue, but really
> it's a problem with the task definition to begin with, and subsequent
> retriggers will fail as well.

I suspect the problem was that https://github.com/taskcluster/generic-worker/blob/bf89d92b6fec8eac4eacbcc11affcea3b128e5db/artifacts.go#L178-L180 (incorrectly) assuming that `putResp` is not nil. I have a fix for that I'll add here.

I'm not sure papertrail still has the logs for that particular event, but I'll see if I can get the logs, to confirm.

	[github robot]
	Comment 2 • 7 years ago

Commit pushed to master at https://github.com/taskcluster/generic-worker

https://github.com/taskcluster/generic-worker/commit/f385a636807bf8ce549bad38fcb5b7da1639597f
Bug 1356800 - avoid nil pointer exception when uploading artifacts (#47)

	Pete Moore [:pmoore][:pete]
	Comment 3 • 7 years ago

(In reply to Pete Moore [:pmoore][:pete] from comment #1)
> (In reply to Greg Arndt [:garndt] from comment #0)
> > Some errors that are caught when trying to upload an artifact result in the
> > worker to panic.  The queue has a few scenarios where an error (such as 400)
> > is returned because of input errors (such as artifact expiration > task
> > expiration) where it would probably be better to log that to the task log
> > and fail the task.
> 
> That particular check is handled up front, when the payload is validated, so
> does not cause a worker panic.

I was lying! So it turns out, I was confusing the check with another one. The worker was already checking that artifacts do not expire before task deadline, but were not checking that they do not expire after task expiry.

This came up in bug 1380978 and I made a fix for it (and added some tests) in:
  * https://github.com/taskcluster/generic-worker/commit/e4a56acf286dd972ed328789dbc1c6473e3b4486

Lastly, I also improved the logging (to show queue error messages) in:
  * https://github.com/taskcluster/generic-worker/commit/57b5079d50951558c737b592dee59e8c6396aa0b

Note, this depending on some changes in taskcluster-client-go (such that the errors returned from API methods retain the http request/response when an API call is not successful).

	Pete Moore [:pmoore][:pete]
	Comment 4 • 7 years ago

Included in generic-worker 10.1.4