Skip to content

Conversation

@mkysel
Copy link
Collaborator

@mkysel mkysel commented Oct 14, 2025

Acquire a high-availability generator worker advisory lock by adding db.LockKindGeneratorWorker with value 0x04 and invoking payerreport.workers.GeneratorWorker.GenerateReports to take the lock before report generation in generator.go

This change introduces a new advisory lock kind and methods to acquire it, and updates the generator worker to obtain the lock before generating reports. It adds the db.LockKindGeneratorWorker constant with value 0x04, extends the advisory locker interfaces and implementations to support LockGeneratorWorker, and modifies payerreport.workers.GeneratorWorker.GenerateReports to acquire and release the transaction-scoped lock around its work.

  • Update advisory lock constants and APIs in advisory_lock.go by adding db.LockKindGeneratorWorker (0x04) and implementing db.AdvisoryLocker.LockGeneratorWorker and db.TransactionScopedAdvisoryLocker.LockGeneratorWorker using queries.AdvisoryLockWithKey.
  • Modify generator.go to retrieve a transaction-scoped advisory locker via w.store.GetAdvisoryLocker(w.ctx), defer Release, and call LockGeneratorWorker() before fetching nodes and iterating.

📍Where to Start

Start with the payerreport.workers.GeneratorWorker.GenerateReports method in generator.go, then review the lock kind and locker methods in advisory_lock.go.


📊 Macroscope summarized 07f4943. 2 files reviewed, 3 issues evaluated, 3 issues filtered, 0 comments posted

🗂️ Filtered Issues

pkg/db/advisory_lock.go — 0 comments posted, 1 evaluated, 1 filtered
  • line 99: TransactionScopedAdvisoryLocker.Release() always calls tx.Rollback() even on successful paths. While this releases a transaction-scoped advisory lock, if any writes are accidentally performed within the same transaction in future changes, they would be silently discarded. Given the current code confines the transaction to advisory lock acquisition, this works, but it couples correctness to a fragile assumption. Consider constraining the transaction strictly to advisory lock acquisition (and avoid using it for any other operations) or using a connection-level/session-level advisory lock to decouple from transaction semantics. At minimum, document that the transaction must not be used for any writes. [ Low confidence ]
pkg/payerreport/workers/generator.go — 0 comments posted, 2 evaluated, 2 filtered
  • line 96: GenerateReports holds a transaction-scoped advisory lock (and thus an open database transaction/connection) across non-database work, including registry access and per-node processing. The lock is acquired at the start via GetAdvisoryLocker and LockGeneratorWorker, and only released at function end via deferred Release() (rollback). This keeps a DB connection checked out for the entire duration of registry.GetNodes() and the loop over allNodes calling maybeGenerateReport. If these operations are slow or the node count is large, it can starve the DB connection pool and block other operations, causing timeouts or deadlocks. Advisory locks that are transaction-scoped should be held for the minimal critical section necessary to make a decision; otherwise, use a dedicated session-scoped advisory lock on a separate connection or restructure to acquire/release around a shorter critical section. [ Low confidence ]
  • line 101: Errors from Release() are ignored in GenerateReports (defer func(){ _ = haLock.Release() }()). If the rollback fails (e.g., connection error) the transaction might not be closed cleanly, potentially leaking resources or leaving the lock state uncertain. At minimum, log the error; ideally, ensure the transaction is deterministically closed and the error is surfaced or handled. While sql.Tx.Rollback() commonly returns sql.ErrTxDone if already completed, ignoring all errors obscures real failures. [ Low confidence ]

@mkysel mkysel requested a review from a team as a code owner October 14, 2025 17:43
@graphite-app
Copy link

graphite-app bot commented Oct 14, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • Queue - adds this PR to the back of the merge queue
  • Hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

LockKindIdentityUpdateInsert LockKind = 0x00
LockKindAttestationWorker LockKind = 0x01
LockKindSubmitterWorker LockKind = 0x02
LockKindGeneratorWorker LockKind = 0x04
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

0x03 is the settlement worker

@mkysel mkysel merged commit 3b00d01 into main Oct 14, 2025
11 checks passed
@mkysel mkysel deleted the mkysel/generator-lock branch October 14, 2025 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants