Log any cat-log failure when listing log files

[MetRonnie]

When trying to set up cylc hub, Dave encountered cat-log problems which were swallowed by the UIS. This PR ensures any such errors are logged.

Check List

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Tests are not needed
• No changelog entry needed as minor
• No docs needed
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[MetRonnie]

I'm inclined to say no automated tests are needed for this

[oliver-sanders]

I don't think raising an exception is the right way to go here. This leaves traceback (that we don't want) in the UIS stderr, however, there is no record of this in the UIS log (exceptions don't go to the log by default).

Suggest logging this with log.error.

[hjoliver]

OK, I agree with that.

[MetRonnie]

Good spot on the exception not being printed in the log file.

However I think raising an exception is the way to go. In order to return this error to the UI so the user can get the feedback that something has gone wrong (via an error snackbar - cylc/cylc-ui#1842), the only other choice is to add a field on the schema (e.g. called error), which seems like an abuse of graphQL?

cylc-uiserver/cylc/uiserver/schema.py

Lines 555 to 565 in 3395e8b

	class UISQueries(Queries):
	class LogFiles(graphene.ObjectType):
	# Example GraphiQL query:
	# {
	# logFiles(workflowID: "<workflow_id>", task: "<task_id>") {
	# files
	# }
	# }
	files = graphene.List(graphene.String)

Unfortunately Graphene seems to lack any documentation on error handling.

In terms of preventing GraphQLLocatedError traceback from appearing in stderr, I think this would be a separate issue as it happens any time an uncaught exception occurs

[oliver-sanders]

the only other choice is to add a field on the schema (e.g. called error), which seems like an abuse of graphQL?

Using the error field is the correct way to send errors back to the client (it is not an abuse). We already do this for other functionality, e.g:

cylc-uiserver/cylc/uiserver/resolvers.py

Line 396 in f6a05fc

yield {'error': msg}

Note, data and error are standard GraphQL return fields, check out the GraphQL docs rather than Graphene.

it happens any time an uncaught exception occurs

Uncaught errors are bugs which should be reported. Tracebacks should not occur under normal operation.

[MetRonnie]

data and error are standard GraphQL return fields

data and/or errors are indeed the GraphQL protocol for the JSON response. But these should not be confused with fields within the data response.

There is no apparent way (that I am aware of) of responding with errors without causing a GraphQLLocatedError traceback. If you capture any exceptions and return them in the resolver function, it still leads to the same outcome.

What you have linked to in the log file contents resolver is returned within the data response; it is included as a field called error in the schema (but could be called anything we like)

cylc-uiserver/cylc/uiserver/schema.py

Lines 618 to 622 in f6a05fc

	class Logs(graphene.ObjectType):
	lines = graphene.List(graphene.String)
	connected = graphene.Boolean()
	path = graphene.String()
	error = graphene.String()

{
  "data": {
    "logs": {
      "lines": [
        "2024-07-08T12:39:47+01:00 INFO - [20240709T0500Z/p/01:running] => succeeded\n",
        "2024-07-08T12:39:47+01:00 INFO - [20240709T0500Z/q/01:running] => succeeded\n"
      ],
      "connected": null,
      "path": null,
      "error": null,
      "__typename": "Logs"
    }
  }
}

I think it makes sense to do it this way, because error is mainly used as a result for when the log has been deleted, which we expect to occur frequently.

I guess we could do a similar thing for listing the log files, even if we don't expect this to fail in the normal course of things. Luckily we wouldn't have to worry about backward compatibility in the schema because cylc-flow is not involved here.

[MetRonnie]

I realised that there are a few ways cat-log will exit non-zero when navigating in the UI (e.g. navigating to /log/non-exist), so have settled for just logging the stderr and returning an empty list as before