Skip to content
This repository was archived by the owner on Jun 30, 2025. It is now read-only.

Conversation

@juliangruber
Copy link
Member

The previous patch wasn't sufficient, core-fly got stuck after this:

Screenshot 2024-04-02 at 16 49 16

When we threw an error, no clean up happened. This is now fixed. And also, we now let execa kill all child processes through the AbortSignal, which helps tighten up the clean up further.

@juliangruber juliangruber requested a review from bajtos April 2, 2024 14:49
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit concerned about the fragility of this orchestration layer. Is it perhaps time to improve zinniad to be finally able to run multiple modules, as it was meant to from the beginning?

Other than that, I don't see any obvious problems in these changes.

It would be great to document how to (manually?) test this orchestration logic so that we don't accidentally introduce regressions in the future.

@juliangruber
Copy link
Member Author

The hard thing is that I haven't been able to reproduce any of these issues. They happen after a while, on Fly.io.

I agree that this logic is showing to be a bit brittle, and that a more fundamental change is needed. Adding multi module support to zinniad will help for sure! There's also complexity in updating the source files, but with just one zinnia process it's less likely to get stuck.

@juliangruber juliangruber merged commit 514cf33 into main Apr 2, 2024
@juliangruber juliangruber deleted the fix/more-zinnia-loop-issues branch April 2, 2024 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants