Return error instead of using panic() in the Reactor

[vbanos]

Methods ReceiveFeedback and ReceiveInsert already return error.

We call reactor.ReceiveFeedback in the finisher and we terminate when there is an error so the program behavior doesn't change. https://github.com/internetarchive/Zeno/blob/main/internal/pkg/finisher/finisher.go#L122

We call reactor.ReceiveInsert in the pipeline and we terminate when there is an error so the program behavior doesn't change. https://github.com/internetarchive/Zeno/blob/main/internal/pkg/controler/pipeline.go#L161

models.ErroNotASeed is already defined, there is no need to make a new error.

Define ErrReactorItemPresent because there is no relevant reactor error.

Relevant to issue: #473

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.48%. Comparing base (032cbd6) to head (3140077).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/pkg/reactor/reactor.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #475      +/-   ##
==========================================
+ Coverage   56.45%   56.48%   +0.02%     
==========================================
  Files         130      130              
  Lines        8052     8055       +3     
==========================================
+ Hits         4546     4550       +4     
+ Misses       3145     3144       -1     
  Partials      361      361              

Flag	Coverage Δ
e2etests	40.70% <0.00%> (+0.03%)	⬆️
unittests	29.46% <0.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[NGTmeaty]

Nice catch! Looks like a good improvement to me!

[equals215]

this is a serious regression

the reactor is the component that guarantees Zeno is working as expected and if one of those errors that was previously a panic goes uncatched this might cause unhandled chaos

i encourage you to revert this ASAP

panics are here cause they should be, it's not a fantasy, the program should CRASH immediately if a non seed gets into the reactor, it's here to catch a programming error not a runtime error

[vbanos]

@equals215 the program will be terminated with a panic from the component that calls the reactor.

finisher.handleSeed calls reactor.ReceiveFeedback which returns models.ErrNotASeed if the item is not a seed.

finisher.worker terminates with a panic() if finisher.handleSeed returns an error.

https://github.com/internetarchive/Zeno/blob/main/internal/pkg/finisher/finisher.go#L98

[equals215]

it's not the caller responsibility to panic()
cause the way the reactor is used might change in the future, we never know, and maybe someone else might wanna change the way it handles errors ; but the reactor is a component that is designed to work in a given way and provide safety to Zeno
the code is a contract and the panics are the lawyers

[vbanos]

Well, we have a different opinion on this matter.
From my experience, its not a good practice for any program to terminate in random places.
I made this issue to describe it and it was accepted #473
So, then I started making changes to components to reorganize panic usage.
I highlight that with the current code, the program will still panic if an issue occurs. It will just be in a different component.

Anyway, its not for me to decide, its up to the lead contributor. Thank you.