Conversation
Pull request dashboard statusClosed · refreshed 2026-09-16 19:41 UTC Status above doesn't look right?
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-process lease timing and CAS interactions warrant final human validation despite strong targeted coverage.
Pull request overview
Adds a publisher-aware write barrier to prevent state updates from starving dashboard publication.
Changes:
- Waits for publisher leases before each state CAS attempt.
- Applies a shared 40-minute queue lock-wait deadline.
- Adds tests and updates concurrency documentation.
File summaries
| File | Description |
|---|---|
state_branch.py |
Implements publisher barriers and shared deadlines. |
dashboard.py |
Enables barriers for dashboard state writers. |
process_queue_batch.py |
Bounds queue-wide lock waiting. |
test_state_branch.py |
Tests barrier and deadline behavior. |
test_dashboard.py |
Verifies writers respect publisher locks. |
test_process_queue_batch.py |
Tests queue deadline environment handling. |
RATIONALE.md |
Documents lease/CAS coordination. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The distributed lease, CAS, and durable queue interactions warrant final human validation despite strong test coverage.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Make dashboard state writers wait for active publisher leases before each CAS attempt, and bound queue-worker lock waits below the job timeout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c4a7acc-3827-4097-9d75-2d054cf9fc3f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c4a7acc-3827-4097-9d75-2d054cf9fc3f
Review finding: This PR makes the scheduled dashboard-queue-recover function the normal restart path for work it defers, and raises its frequency from hourly to every five minutes for exactly that reason. RATIONALE.md now says the claims become runnable after the delay and a recovery scan runs every five minutes to start their next drain. WEBHOOK_SETUP.md still tells operators the opposite: it says a new event normally starts the singleton drain immediately and that scheduled recovery is only a failure backstop. After this change a publisher-lock deferral sets notBefore in the future, finishDispatcher reports nothing runnable, and no new event restarts the drain, so the scheduled scan is the ordinary path for that work. Fix: update the WEBHOOK_SETUP.md paragraph so it also names the scheduled scan as the path that restarts deferred queue work, instead of describing it purely as a failure backstop. Analysis: A publisher-lock deferral acknowledges a claim as a retry with a five-minute delay, which sets notBefore in the future. claimWave and hasRunnableItems both skip an item until notBefore passes, so finishDispatcher finds nothing runnable and requests no successor drain. Nothing then restarts that work until either the scheduled dashboard-queue-recover scan sees the item as runnable and requests a dispatcher, or a later webhook event happens to arrive after the delay has elapsed. That is why this PR changed the function's schedule from hourly to every five minutes. WEBHOOK_SETUP.md is the operator-facing setup document, and its claim that scheduled recovery is only a failure backstop was accurate before this change, because production acknowledgments supplied no nonzero retry delay. This PR makes that sentence wrong and leaves an operator with no explanation for the new schedule. The replacement paragraph states the schedule, names both restart paths, and keeps the dead-letter sentence unchanged. It avoids the evaluator's stronger wording that would have dropped the still-true fact that a new event normally starts the drain immediately. Upsides: The setup document now matches the queue's behavior and explains why the recovery function runs every five minutes. An operator reading it will not treat a scan that starts a deferred drain as a sign that something failed. Downsides: No material downside identified. The change is documentation only and touches no file this PR already changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
trask
force-pushed
the
trask-fix-dashboard-publisher-contention
branch
from
September 15, 2026 15:19
d44904b to
14cf2a9
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The refreshed lease ref can permit a stale force-push that discards publisher state.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+451
to
+456
| if respect_publisher_lock: | ||
| wait_for_publisher_unlock( | ||
| state_dir, | ||
| state_branch, | ||
| wait_seconds=publisher_lock_wait_seconds, | ||
| ) |
Member
Author
|
Superseded by #383 |
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
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.
Prevents concurrent dashboard state updates from starving a publisher. State writers respect the repository publisher lease, while
--force-with-leasehandles races that begin before the lease commit.Direct workflows wait for the publisher. Queue workers return every claim for a busy repository and retry after five minutes without using the processing-failure budget. Targeted updates and head-SHA claim resolution check the lease before GitHub API or Copilot work.
Fixes #341