coordinator: harden worker payout-lock lifecycle (surface unbind failures, idle GC, bounded load, bulk unbind) - #27
Merged
Conversation
…e GC, bounded load, bulk unbind) - clearPayoutLock now runs the durable DELETE first and surfaces its failure: admin unbind answered ok:true while the row could survive (error was swallowed), silently re-binding the worker after restart. - worker_payout_lock grows one row per worker_id ever seen; add seen_at (backfilled on upgrade, first-bound address never rewritten), idle-GC on attach (HACKME_PAYOUT_LOCK_IDLE_SEC, default 30d, 0 disables). - startup load is bounded newest-first (reuse the dedup memory cap); lockedPayoutAddress already falls back per-worker, so evicted bindings stay enforced. - admin unbind accepts worker_ids[] (cap 256) for fleet retirement. Tests: zz_payout_lock_lifecycle_test.go (red on main for the silent ok:true and the missing bulk path; green + full package green).
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe coordinator now tracks payout-lock activity in durable storage, removes idle lock rows, and bounds in-memory loading. The admin unbind endpoint accepts single-worker or bulk requests and reports durable deletion failures. ChangesPayout-lock lifecycle
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue is established for the payout-lock lifecycle changes; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The changes improve administrative recovery and bound startup loading, but identity enforcement becomes more dependent on successful database access. Database read failures can permit rebinding of unloaded workers, and failed activity updates can leave live bindings eligible for deletion after restart. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoHarden coordinator payout-lock lifecycle and bulk unbind
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…h-on-success, warm-up caps, seen_at index - serialize clearPayoutLock (DELETE + memory clear) against persistPayoutLock with a dedicated mutex, so an in-flight claim cannot re-insert a binding an admin unbind just deleted - lockedPayoutAddress fails closed when the durable store errors (sentinel instead of an empty no-binding read) - the throttled liveness touch advances only after the UPDATE succeeds, so a transient DB failure retries on the next lock read - first read of an evicted binding refreshes its seen_at immediately - warm-up load respects maxWorkers, newest-first; capped bindings fall back to the durable row per read - seen_at index is created after the column upgrade, so old-schema installs migrate cleanly (index-on-missing-column would fail the migration) - log lines quote worker ids (CodeQL log-injection hygiene) Tests: +4 (fail-closed sentinel, evicted-read refresh, retry-on-failed-touch, maxWorkers cap); full package green.
Owner
|
Merged as Payout-lock lifecycle hardening (unbind fail-closed, idle GC, bulk unbind, seen_at) is on main and deploying to the hub coordinator. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Follow-up hardening of the worker_payout_lock durable binding introduced for report #27 - four lifecycle gaps, all in one table code path. Review findings from the first bot pass are addressed in the second commit (5c61110).
What
Admin unbind could answer ok:true while the durable DELETE silently failed. clearPayoutLock swallowed the DELETE error, cleared the in-memory map, and the endpoint always wrote ok:true. If the DELETE fails (e.g. sqlite busy), the binding survives on disk and the worker is silently re-bound to its old address after the next coordinator restart - the coordinator: harden worker payout-lock lifecycle (surface unbind failures, idle GC, bounded load, bulk unbind) #27 recovery path quietly no-ops. The DELETE now runs first and its failure is surfaced: the endpoint answers 500 + ok:false. Unbind (DELETE + memory clear) is serialized against persistPayoutLock with a dedicated mutex, so an in-flight claim cannot re-insert the binding between the two steps.
The table grows forever. One row per worker_id ever seen, no TTL/GC - the sibling dedup tables got a TTL after the multi-million-entry starvation incident, this table got nothing. Adds a seen_at column (old installs are upgraded and backfilled on first boot, so idle-GC never reaps pre-upgrade rows), refreshed on every persist while the first-bound address is never rewritten (report coordinator: harden worker payout-lock lifecycle (surface unbind failures, idle GC, bounded load, bulk unbind) #27 semantics preserved). Idle rows are reaped on attach (default 30d, HACKME_PAYOUT_LOCK_IDLE_SEC, 0 disables), with an index on seen_at for the GC cutoff and the newest-first load (created after the column upgrade so old-schema installs migrate cleanly). Active bindings are kept fresh by a throttled liveness touch (at most 1/h per worker) on the lock reads every submit path already performs - the touch is UPDATE-only and never inserts, so it cannot resurrect a row an in-flight admin unbind just deleted; the in-memory throttle advances only after the UPDATE succeeds, and it is seeded from the durable seen_at at load so a restart does not trigger a per-worker write burst.
Startup loaded the whole table into memory. loadPayoutLocks had no LIMIT. It is now newest-first bounded by the existing dedup memory cap and the configured worker cap - correctness is unchanged because lockedPayoutAddress falls back to the durable row per worker, so evicted bindings stay enforced. That fallback fails closed: a durable-store error returns a sentinel that fails identity checks instead of reading as no binding; the first read of an evicted binding also refreshes its seen_at.
Fleet retirement needed N calls. The unbind endpoint additionally accepts worker_ids[] (cap 256, same per-id validation), returning the count of ids whose live binding was removed plus per-id failures.
Evidence
Notes