When there is a transaction error, we try to rollback before re-raising the error. When the error is re-raised, the transaction's __exit__ is called, which attempts to commit. This commit fails with a very cryptic "Transaction is invalid" error. We should not attempt to commit if there has been an error.

bug 740360 is also a problem with the AUSTransaction code. I might fix this there, or I may rip out AUSTransaction altogether.

Turns out this happens in cases where there's a view that eats IntegrityError exceptions, and possibly other SQLAlchemyExceptions. For example, this View catches *all* Exceptions: https://github.com/mozilla/balrog/blob/master/auslib/web/views/permissions.py#L74 If any of the queries it does raise an IntegrityError it will get caught in the final exception handler, a Response object will be returned, and then the context manager setup here: https://github.com/mozilla/balrog/blob/master/auslib/web/views/base.py#L44 will never see the exception. Then, in __exit__, this code will think everything went OK: https://github.com/mozilla/balrog/blob/master/auslib/db.py#L62 and will continue down further and try to commit(). So, I don't think we should be catching generic "Exception" inside of views, and in fact I think I had started to make that change as part of bug 672918 and forgot to finish doing it. Eg, releases.py doesn't do it, but permissions.py does. This also makes me wonder if we should raise 400 responses instead of return, so that __exit__ will rollback() instead of trying to commit.

Comment on attachment 612575 [details] [diff] [review] stop catching errors only to convert them to a 500 Response Review of attachment 612575 [details] [diff] [review]: ----------------------------------------------------------------- What will the behaviour be in production if we hit an unhandled exception? Will we end up displaying the error to the user? If so, is this a problem? Assuming the resulting behaviour is what we want, looks good to me.

Comment on attachment 612575 [details] [diff] [review] stop catching errors only to convert them to a 500 Response In production, the wsgi script will return non-zero and Apache will display a generic ISA 500 message.

Hmm, might be helpful to know what Firefox does with response codes. If it ignores any data for non-200 responses then this sounds OK.

(In reply to Nick Thomas [:nthomas] from comment #6) > Hmm, might be helpful to know what Firefox does with response codes. If it > ignores any data for non-200 responses then this sounds OK. Rob, do you know the answer to this?

(In reply to Ben Hearsum [:bhearsum] from comment #7) > (In reply to Nick Thomas [:nthomas] from comment #6) > > Hmm, might be helpful to know what Firefox does with response codes. If it > > ignores any data for non-200 responses then this sounds OK. > > Rob, do you know the answer to this? No idea... app update uses custom handling of response codes.

Thanks to khuey, I found http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2455, which doesn't seem to do much at all. I'm not sure what this._callback ends up being, though.

It wasn't clear to me from the comments that you are interested in the update service's response codes for an update check and not the Firefox response codes. That is the listener for an update check. The listener can be internal to the update service code in the case of a background check or external in the case of code external to the update service performing a check for updates. http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsIUpdateService.idl#357 and it is set here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2335 The update service doesn't special case handling errors returned from XMLHttpRequest which is used for the update check.

Comment on attachment 612575 [details] [diff] [review] stop catching errors only to convert them to a 500 Response Sorry, dunno why I thought endusers would be hitting 500's.

Jenkins run is here: https://jenkins.mozilla.org/job/Balrog/80/