Log fetch reports per worker #1237
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Log fetch reports per worker by moving
payerreport.Store.FetchReportsto Debug level and adding Debug logs inpayerreport.workers.AttestationWorker,payerreport.workers.GeneratorWorker, andpayerreport.workers.SubmitterWorkermethodsThis change adjusts logging around report fetching across the payer report workers. It lowers the log level of the store fetch to Debug and adds new Debug logs with contextual fields in worker methods.
payerreport.Store.FetchReportsfrom Info to Debug and update message casing in store.gopayerreport.workers.AttestationWorker.findReportsNeedingAttestationand before fetching previous reports withoriginator_node_idandstart_sequence_idinpayerreport.workers.AttestationWorker.getPreviousReportin attestation.gopayerreport.workers.GeneratorWorker.maybeGenerateReportwithnode_idandexisting_end_sequence_id, and inpayerreport.workers.GeneratorWorker.getLastSubmittedReportwithnode_idin generator.gopayerreport.workers.SubmitterWorker.SubmitReportsin submitter.go📍Where to Start
Start with the log level change in
payerreport.Store.FetchReportsin store.go, then review the added Debug logs in worker methods beginning withpayerreport.workers.AttestationWorker.findReportsNeedingAttestationin attestation.go.📊 Macroscope summarized 36d6a60. 4 files reviewed, 4 issues evaluated, 4 issues filtered, 0 comments posted
🗂️ Filtered Issues
pkg/payerreport/workers/generator.go — 0 comments posted, 1 evaluated, 1 filtered
maybeGenerateReport. Between fetchinglastSubmittedReportand subsequently fetchingexistingReportsfiltered bySubmissionPending, a concurrent submitter can transition a report from pending to submitted. This makes the pending check miss the just-submitted report and allowsgenerateReportto run, creating a duplicate report for the same range. The code even notes this hazard in comments. To fix, enforce a stronger atomicity: e.g., perform both checks and the potential creation within a single DB transaction using appropriate locks (SELECT ... FOR UPDATE) and/or a unique constraint on the report interval per originator, and handle uniqueness violations by re-reading; or take an advisory/row lock around the sequence window. [ Out of scope ]pkg/payerreport/workers/submitter.go — 0 comments posted, 3 evaluated, 3 filtered
SubmitReportsinconsistently uses contexts: it acquires the advisory locker withw.payerReportStore.GetAdvisoryLocker(w.ctx)using the worker’s long-livedw.ctx, but uses the passedctxfor store operations (FetchReports,SetReportSubmissionRejected,SetReportSubmitted). This can lead to cancellation and timeout mismatches where the advisory lock acquisition/holding ignores the caller’s cancellation, or store operations cancel while the lock continues to be held. This inconsistency can cause prolonged lock holding or partial work whenctxis canceled. The function should use a consistent context, typically the passedctx, for all operations, or document and intentionally separate lifetimes with explicit handling. [ Low confidence ]reportsare non-nil, but no guard enforces this. IfFetchReportsreturns a slice containing a nil*PayerReportWithStatus, thenpayerreport.AddReportLogFields(w.log, &report.PayerReport)will dereferencereportand panic. Subsequent uses likereport.ID.String()and passingreporttow.submitReport(report)will also panic. You should add a nil check per element before dereferencing or using the report. [ Low confidence ]SetReportSubmittedare only logged atWarnlevel and not propagated via the function return (latestErris not set). This means the worker can returnnileven when the report’s submitted state failed to persist, leading to inconsistent state and silent failure from the caller’s perspective (Startonly logs returned errors). While the comment mentions a race the store should handle, failures unrelated to the race (e.g., DB errors) are suppressed. Consider including the error in the log (zap.Error(err)) and propagating it vialatestErrto ensure visibility and retries/alerts behave correctly. [ Low confidence ]