Openwhisk: sequence invoked with "error" parameter automatically fails

[starpit]

when i pass an "error" parameter to a sequence, it fails with that parameter as a return value. note that the sequence fails, without even invoking any of the components. also note that this only affects sequences, not individual actions.

this breaks slack oauth flows: when the user clicks Cancel in an oauth flow, slack calls my redirect_uri with an error parameter. but the issue seems to be unrelated to web-export. the undesirable behavior occurs even with conventional invocations.

here is an example:

```
echo 'function main(params) { return { code: 302, headers: { location: "https://google.com" }}}' > /tmp/foo.js

wsk action update foo /tmp/foo.js
wsk action update --sequence fooseq foo,foo

wsk action invoke fooseq -p error weird -br
{
"error": "weird"
}
wsk action invoke fooseq -p error weird -b 2>&1 | grep status
"status": "application error",
"statusCode": 0,

wsk action invoke fooseq -p error_nope weird -br
{
"code": 302,
"headers": {
"location": "https://google.com"
}
}```

[sjfink]

So we currently have a convention, obviously, that any action that returns a result with an "error" attribute is considered an error.

Seems clear that reserving the keyword "error" is a mistake. Easy fix: use something like __OPENWHISK_ABNORMAL_EXIT instead.

[starpit]

the problem here is that none of my component actions were even invoked... the sequence just prematurely fails without even invoking any of the components

i.e. invoking one of my plain actions with -p error gives the expected result:

wsk action invoke foo -p error weird -br
{
    "code": 302,
    "headers": {
        "location": "https://google.com"
    }
}

also: i purposefully did the 302. if you add -a web-export true to the two action creation steps above, and hit the urls in the browser, you'll see that either you get redirected (if you don't pass -p error xxx), or you'll see garbagey json (if you do)

[rabbah]

Not to mention the action isn't returning an error property.

[rabbah]

@ioana-blue assigned to you speculatively. This looks suspicious: https://github.com/openwhisk/openwhisk/blob/master/core/controller/src/main/scala/whisk/core/controller/actions/SequenceActions.scala#L322

[sjfink]

ok ... so the check to abort a sequence based on "error" arguably should not apply to the first method in the outermost sequence .... although I could argue the other way and claim this is working as designed.

I still propose changing the keyword as a better fix.

[rabbah]

Should not apply to the incoming invoke parameters - they are not the result of an action. Why do we need to change the convention to abort the sequence? This strikes me as completely orthogonal.

[sjfink]

it's orthogonal, ok -- but the current convention sucks so let's fix it as long as we're touching that code?

[rabbah]

Well if we change it it seems quite a change. Now actions might return error or some other property if they are stand alone vs composed in a sequence?

Edit: if you're just suggesting that instead of "error" to indicate a failed activation and instead some other property name, and keeping the rest of the semantics then ok that's not as bad as I was thinking. But I don't see why error is a bad idea or warrants a change yet. But I think I misunderstood initially.

[starpit]

of minor note: if we had a catch mechanism #1793, then i could have protected myself from this situation, by redirecting the user back to the origin page of my app.

[ioana-blue]

This works as designed. There is even a test for it @rabbah see this:
https://github.com/openwhisk/openwhisk/blob/master/tests/src/system/basic/WskSequenceTests.scala#L107

Note zero actions being executed in the sequence is tested in this line (so works as designed) @starpit
https://github.com/openwhisk/openwhisk/blob/master/tests/src/system/basic/WskSequenceTests.scala#L113

I understand we need to change this. Some of the options discussed in this thread:

1. simply not apply the 'error' convention to the params of the topmost/outtermost action in the sequence
2. change the reserved keyword (while keeping the current behavior of checking whether the keyword is part of the incoming params)
3. change the reserved keyword and also do 1 above

[rabbah]

Thanks for pointing out the test.

Changing the name of the error property really is a different point. As in if the (initial) invoke parameters contained the property, whatever it is called, should the sequence really not run?

I think this behavior is wrong or at the very least surprising - since it applies only to sequences. We should consider the two points separately (behavior if invoke time parameters specify the property for error, and name of the error property). Changing the name of the property to something less common is just hiding the behavior/making it less likely to happen in practice. It doesn't address the sequence vs non-sequence invocation semantic difference. I suppose we could disallow incoming parameters from specifying this property, as option 4... maybe?

Option 5 as alluded to by @starpit: provide catch and leave the rest as is - but also doesn't address semantic gap vs non-sequences.

[ioana-blue]

I vaguely remember why we settled on this behavior/convention. I think we didn't want to treat sequences differently depending on where they appear wrt to other sequences/actions

E.g., s -> a, x , where a is an atomic action and x is a sequence. Under the current behavior, if the error property is received by s or x, no actions from those sequences get executed.

In the new behavior under discussion, it makes a difference whether the error property is received by initial parameter payload or is received as payload from a return from a previous action in a sequence.

[rabbah]

in the new behavior under discussion, it makes a difference whether the error property is received by initial parameter payload or is received as payload from a return from a previous action in a sequence.

Or to put it differently, if the invoke time parameters are received by a sequence vs non-sequence (atomic) action. Since sequences are actions, should the semantics apply to all actions?

[starpit]

I can understand some kind of retval interpretation, but why would we ever have an inval squasher? In AX if A returns error then fine kill the sequence at that point.

[ioana-blue]

@starpit I can imagine scenarios where the incoming params come from triggers through rules and then you can think of inval as retval.

[starpit]

Triggers don't have return values so I think I'm OK with that. If we insist they do, I persist: any retval of a trigger is just it's inval. And so we are back to squashing. Added on input, which we cannot do, because inputs may come from a world that does not obey our conventions.

And why isn't rejecting a promise sufficient for indication of error and thus shirt circuiting. Why would we ever need to invent a convention for this.

[ioana-blue]

any retval of a trigger is just it's inval

That's how I think of triggers.

And why isn't rejecting a promise sufficient for indication of error and thus shirt circuiting. Why would we ever need to invent a convention for this.

Not sure I get this part. It may hold in a js only world, but how does it generalize to polyglot world?

[starpit]

We are agreed at least to fix this, so that failures no longer occur as a function of input values?

[ioana-blue]

After all this discussion, I think I agree with @rabbah 's point of view, i.e., 1. the name of the error property is orthogonal to the way the error property is treated when received as parameter - by either topmost sequences or actions; and 2. the semantics for sequences should be the same as for actions since sequences are actions.

As such, I suggest for now I change the way the params with an error property received by a topmost sequence behave (i.e., the sequence will start execution and the error property short-circuit applies only to returned values).

I perceive the renaming of the error property a much more intrusive change because, to keep semantics the same for actions and sequences, we would need to rename the error property for atomic/simple actions as well. This could potentially cause headaches for active users and right now I don't see any benefit.

Any comments/opinions/etc? @rabbah @starpit @sjfink

[sjfink]

I think Nick's example shows that it is likely that external services may use "error" property -- and so we will run into this again, when someone wants to use such a service in a sequence.

So, it seems to me we will have to change the convention sooner or later ... sooner seems better than later.

[ioana-blue]

@sjfink what's your take on actions and sequences behaving the same? do you mean to change the error property name for atomic actions as well to stay consistent across all types of actions?

Also, Nick's example and external services affect inval of actions not retval (to use Nick's terminology), so I don't immediately see your point.

[sjfink]

Yes, I think actions and sequences should behave the same, so I propose changing the name for atomic actions as well.

Nick has an example of an external service producing a value with an "error" property. It seems reasonable to me that someday, someone will wrap a REST call to a similar service and use it in a sequence.

If you want a concrete example, let's suppose tomorrow Watson conversation changes their API so that one of their API calls returns an object with an "error" property. All of a sudden, with no notice, all sequences using this will just break.

I think it's not a question of if this will happen, but when.

[ioana-blue]

After some more thinking, the treatment of error property is different for the very first action in a sequence, irrespective of whether that action is a sequence or not (nothing to do with topmost sequence).

Consider the following example (assume s, x are sequences and a, b are atomic actions and none of the actions return error):

   s-> a, x
   s -> a
   s -> x   (*)
   x -> b

Assume all these sequences get a payload that contains an error field, they will all execute fine under the new behavior.

(*) note x executes even when not topmost

[perryibm]

@ioana-blue In your proposed fixed, you said

_As such, I suggest for now I change the way the params with an error property received by a topmost sequence behave (i.e., the sequence will start execution and the error property short-circuit applies only to returned values)._

The error property should apply _also_ based on whether some status code indicates a whisk error?

[ioana-blue]

@perryibm I think that happens automatically by current design; whenever there is a status code that indicates an error, the activation response that is constructed contains the error property. We have a test for such a case

https://github.com/openwhisk/openwhisk/blob/master/tests/src/system/basic/WskSequenceTests.scala#L304

[ioana-blue]

@rabbah curious about your take on this.

If I were to play devil's advocate, in this example
https://github.com/openwhisk/openwhisk/issues/1822#issuecomment-278355909

if a service changes anything in its API, I'd rather know about it than everything in the OW world behave as if nothing happened. Also, you'd think any API breaking changes would be advertised in advance, so the REST wrapper action could be updated to properly handle the new field addition. If any service provider adds an error field, I'd like to know and have control over it.

Not to add that if I were a current OW user and something that was naturally used so far (the error field to signify error) would be changed to something cryptic like __OW__**** and I would have to change all the code in my actions to deal with this change, I would be annoyed.

This being said, I can argue both ways. It's fuzzy. (That's one reason why I liked Nick's inval/retval distinction. The REST api wrapper example is not an inval (at least not in my mind).)

[rabbah]

We tend to state that invoking an action is Dictionary → Dictionary but more formally it is Dictionary → Try[Dictionary]. The Dictionary must be treated opaquely and if so, then there are no grounds to squash/not invoke an action that receives a dictionary because it defines an error property, regardless of the property name. Such is the case of an argument received from the REST API to activate an action whether directly or via a trigger.

Once running inside of the _openwhisk runtime_, the semantics are such that an action may return a Success[Dictionary] or Failure[Dictionary], and under composition, if the result of an action is the former, the dictionary is unboxed and passed to the next action in the sequence. If it's the latter, the execution terminates.

The runtime encodes Failure[Dictionary] with the presence of an error property in the dictionary - while within the openwhisk runtime, it is fair to apply this uniformly since any action must adhere to this protocol and the runtime _enforces_ this.

(To relate this to the catch handler, when an action result is Failure[Dictionary] and there is a catch handler in scope for the action, then the action is invoked and receives the unboxed Dictionary.)

[starpit]

i think that's a good summary. the general flow is then:

given Seq(X,Y,Z), where X resolves to Success(D1), and Y resolves to Failure(D2), and Y.catch is W such that W(D2) yields Success(D3), the result is the following data flow

D0 -> X => Success(D1) -> D1 => Y => Failure(D2) -> D2 -> W => Success(D3) -> D3

where -> flows are managed by openwhisk, and => flows are managed by the application/actions

note that openwhisk only inspects Try[Dictionary] values when managing the flows?

[rabbah]

Closed by https://github.com/openwhisk/openwhisk/commit/a1dfc1dc449348533cfcfc76b1a89944730c716e.