Skip to content

Conversation

absurdlylongusername
Copy link

@absurdlylongusername absurdlylongusername commented Jul 30, 2025

Improve logging and error handling for IIS ASP.NET Core module shadow copy

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)
Improve shadow copy logging and error handling

Description

Goal of this PR: if shadow copy fails due to lack of permissions to the shadow copy directory

  1. The application will return a useful 500 error instead of an empty
  2. Useful logs will be printed to debug log output
  3. Useful information will be logged in event viewer

Laundry list of changes

  • In CreateApplication, log exception message for all types of exception and not just ConfigurationLoadException
  • When exception is thrown in CreateApplication, return useful HTTP 500 with exception message instead of an empty one
  • I moved the code that checks if shadow copy is enabled up from HandleShadowCopy to TryCreateApplication
  • TryCreateApplication will now return failed HRESULT if shadow copy fails
  • HandleShadowCopy now takes an ErrorContext that will provide error information when it fails
  • Added error handling in Environment::CopyToDirectory for when it fails; it also now throws exception
  • Added two tests in ShadowCopyTests that test shadow copy failure behaviour when there's no permissions to the shadow copy directory, and when there's no write permissions
  • Tiny refactor in IISDeployer.cs to use TryGetValue
  • Some refactoring in ShadowCopyTests of variable renaming and adding Arrange Act Assert comments for clarity (can undo this if it's not wanted)

Fixes #62148

Bare in mind I am well aware this is not production ready. However, I am not a C++ enjoyer (yet), so I am opening this PR with a reasonable working solution to get it reviewed instead of trying to perfect it myself.

I have left some TODOs in here which are mostly questions for the people who are going to review it. Once they are addressed I will remove them.

The code displays a new 500 error about shadow copying: what's the protocol on sub error codes for these? Is reusing existing ones okay or should a new one be made? I presume existing documentation elsewhere would also need to be updated?

@github-actions github-actions bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jul 30, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 30, 2025
@absurdlylongusername absurdlylongusername changed the title Fix/shadowcopy62148 Improve logging and error handling for IIS ASP.NET Core module shadow copy Jul 30, 2025
@absurdlylongusername
Copy link
Author

@dotnet-policy-service agree

Copy link
Contributor

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Aug 6, 2025
@absurdlylongusername
Copy link
Author

@dotnet-policy-service agree

Add logging for when no permissions to shadow copy directory
Add tests for when no permissions and when only write permissions
Add error handling to CopyToDirectory
Minor refactoring in ShadowCopyTests for clarity
… directory fails

Add ConfigurationLoadException to exception helper logging
@absurdlylongusername
Copy link
Author

@halter73 @BrennanConroy @JamesNK Hi all, gentle reminder that this PR is open for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shadow Copy - unclear and/or misleading errors
1 participant