Skip to content

fix: Catch exceptions at Circle::getInitiator during mountpoint setup#2314

Open
mejo- wants to merge 1 commit intomainfrom
fix/mountpoint_catch_exceptions
Open

fix: Catch exceptions at Circle::getInitiator during mountpoint setup#2314
mejo- wants to merge 1 commit intomainfrom
fix/mountpoint_catch_exceptions

Conversation

@mejo-
Copy link
Member

@mejo- mejo- commented Mar 5, 2026

Extracted from #2229 and adapted

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits

@mejo- mejo- requested a review from juliusknorr March 5, 2026 11:55
@mejo- mejo- self-assigned this Mar 5, 2026
@mejo- mejo- added bug Something isn't working 3. to review labels Mar 5, 2026
@mejo- mejo- mentioned this pull request Mar 5, 2026
5 tasks
@max-nextcloud
Copy link
Collaborator

I'm not sure I understand well enough what you are trying to do. So let me try and write it down so you can check if it's matching:

  • Some code inside getCollectivesForUser may throw an exception.
  • As this is during mount point setup that will break a lot of requests - basically all requiring a filesystem to be setup.
  • Since the code in question may live in the circles app and may be a bug / syntax error we cannot rely on the exception types we usually have.
  • Therefore we catch and log all exceptions in this critical code path.

Assuming that's your goal I agree with it. However this generic catching of exceptions will also catch errors in our own code - even syntax errors and only report them as log entries. I'm not a fan of simply logging unexpected errors - as they may remain unnoticed for quite a while. 500 responses scream - there's something wrong on the server - look at the logs. Log entries alone are quiet in many contexts (for example our current CI).

Instead I'd prefer to keep the scope of generic error handling as small as possible. I.e. catching generic exceptions thrown by circles, rethrowing them as our own Exception - for example CircleException and only adding that to the exceptions caught in the MountProvider.

@mejo-
Copy link
Member Author

mejo- commented Mar 5, 2026

Thanks for your thoughts @max-nextcloud

Assuming that's your goal I agree with it.

Yes, you nailed it 😆

I'm not a fan of simply logging unexpected errors - as they may remain unnoticed for quite a while.

Indeed, I agree.

Instead I'd prefer to keep the scope of generic error handling as small as possible. I.e. catching generic exceptions thrown by circles, rethrowing them as our own Exception - for example CircleException and only adding that to the exceptions caught in the MountProvider.

This sounds like a good way forward to me 👍

@mejo- mejo- force-pushed the fix/mountpoint_catch_exceptions branch from 5a5b23b to 51083b7 Compare March 9, 2026 09:02
@mejo- mejo- changed the title fix: Catch generic exceptions during mountpoint setup fix: Catch exceptions at Circle::getInitiator during mountpoint setup Mar 9, 2026
@mejo- mejo- force-pushed the fix/mountpoint_catch_exceptions branch from 51083b7 to 9ea513f Compare March 9, 2026 09:04
@mejo- mejo- force-pushed the fix/mountpoint_catch_exceptions branch from 9ea513f to 5992662 Compare March 9, 2026 09:06
@mejo-
Copy link
Member Author

mejo- commented Mar 9, 2026

@max-nextcloud I refactored the change now to only catch generic exceptions at Circle::getInitiator() for now as this was the original problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants