statiqdev / Statiq.Web

Statiq Web is a flexible static site generator written in .NET.
https://statiq.dev/web
Other
1.64k stars 234 forks source link

Deploy pipeline failure results in exit code 0 #972

Closed SteveDesmond-ca closed 2 years ago

SteveDesmond-ca commented 2 years ago

Steps to reproduce:

(I'll try to find a more minimal repro, but for now here's the most recent time I've hit this)

Expected result:

Observed result:

Notes:

SteveDesmond-ca commented 2 years ago

Minimal repro is just about as minimal as it can get!

Program.cs:

using Statiq.App;
using Statiq.Web;

await Bootstrapper.Factory
    .CreateDefaultWithout(args, DefaultFeatures.Pipelines)
    .AddWeb()
    .DeployToGitHubPages("test", "test", "test")
    .RunAsync();

Deployment fails but exit code of dotnet run -- deploy is 0

daveaglick commented 2 years ago

Interesting. I think there are two things going on (both are bad!):

Stay tuned - I'll drop a note here when a new release is out (hopefully shortly). Hopefully with some better logging we can figure out why the deployment is failing in addition to knowing that it's failing.

SteveDesmond-ca commented 2 years ago

I found it -- the async/awaits inside of github.ThrottleAsync() in DeployGitHubPages.ExecuteConfigAsync() (link) are swallowing the exception. Removing them (like is done at the bottom of the file) fixes this issue.

PR incoming later this afternoon for review.

daveaglick commented 2 years ago

Interesting! Now I've got to wrap my head around why that would be the case. The ThrottleAsync() method takes a Task-returning function, so when we omit the async/await it's passing in the actual Task from the called method, which then gets awaited inside the ThrottleAsync() method. However, when we pass in an async lambda like in the problematic code, we await the lambda itself within the ThrottleAsync() method, which then awaits the called method. My intuition is that shouldn't impact exception catching other than adding the extra lambda to the call stack.

I'm clearly missing something! Now I've been successfully nerd snipped and can't rest until I fully understand what's going on here.

daveaglick commented 2 years ago

Hmm...they both seem to throw correctly as I'd expect:

image

image

SteveDesmond-ca commented 2 years ago

lol it's like we're pairing and finding the same issues simultaneously...indeed, I think the problem may be in my code

SteveDesmond-ca commented 2 years ago

Yep, there's even a corresponding problem in my "minimal" repro -- it's a little too minimal:

Program.cs:

using Statiq.App;
using Statiq.Web;

await Bootstrapper.Factory
    .CreateDefaultWithout(args, DefaultFeatures.Pipelines)
    .AddWeb()
    .DeployToGitHubPages("test", "test", "test")
    .RunAsync();

Process finished with exit code 0.

vs

Program.cs:

using Statiq.App;
using Statiq.Web;

return await Bootstrapper.Factory
    .CreateDefaultWithout(args, DefaultFeatures.Pipelines)
    .AddWeb()
    .DeployToGitHubPages("test", "test", "test")
    .RunAsync();

Process finished with exit code 1.

Apologies for the rabbit hole!

daveaglick commented 2 years ago

I think the problem may be in my code

I'm not really seeing anything in the project that jumps out as suspect (unless this only started with some uncommitted code or code in a branch?). It all looks pretty straightforward, I.e. nothing exotic with changing inbox pipelines or anything.

daveaglick commented 2 years ago

Oh my gosh! Haha - so it just wasn't returning the exit code. It's always the blindingly obvious things :)

daveaglick commented 2 years ago

Apologies for the rabbit hole!

No worries at all - async/await is tough enough to get right that validating assumptions and experimenting with it is an absolutely good exercise every so often.

SteveDesmond-ca commented 2 years ago

Plus if nothing else more supporting evidence for why I'm against top-level statements :grimacing: