-
Notifications
You must be signed in to change notification settings - Fork 130
[Coordinator] Break prover client interface for creating requests #1957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| return findRequestFileIfAlreadyInFileSystem(requestFileName) | ||
| .thenCompose { requestFileFound: String? -> | ||
| responsesWaiting.incrementAndGet() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Counter leak when createProofRequest called independently
The responsesWaiting counter is incremented in createProofRequest but only decremented in requestProof. Since createProofRequest is now exposed as a public interface method via ProverProofRequestCreator, callers can invoke it independently without going through requestProof. In that scenario, the counter increments but never decrements, causing the metric to grow unbounded and report incorrect values via the Supplier<Number> interface.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If invoked independently, this behaviour is expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createProofRequest will be used by conflation backtesting which will use a different metrics facade so the prod and backtesting metrics do not interfere with each other.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1957 +/- ##
============================================
+ Coverage 62.80% 62.81% +0.01%
- Complexity 1536 1538 +2
============================================
Files 410 410
Lines 14978 14980 +2
Branches 1582 1582
============================================
+ Hits 9407 9410 +3
+ Misses 4952 4951 -1
Partials 619 619
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
| return proverB | ||
| } else { | ||
| proverA.findProofResponse(proofRequestId) | ||
| return proverA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I prefer moving return out of the if
This PR implements issue(s) #1740
Checklist
Note
Introduce
createProofRequestvia newProverProofRequestCreatorand refactor file-based prover client and router to delegate and reuse request creation.coordinator/core/.../ProverClient.kt):ProverProofRequestCreatorwithcreateProofRequest.ProverClientto extendProverProofRequestCreator.GenericFileBasedProverClient):createProofRequest; extract request file creation logic fromrequestProof.requestProofto callcreateProofRequestthen wait/parse response.ABProverClientRouter):getProverhelper and delegatefindProofResponse,requestProof, and newcreateProofRequestbased on predicate.Written by Cursor Bugbot for commit 8b55a66. This will update automatically on new commits. Configure here.