Repository navigation
Count queue and hotspot totals in SQL instead of over materialized rows - #377
Merged
Merged
Conversation
`_scoped_queue_rows` stripped the caller's limit and `collect_queue_report` recounted the returned rows in Python, so a limit cut what was transferred and not what the server built. `ERDQueue.report_queue_rows` already computes its summary as a COUNT(*) GROUP BY and already accepts a LIMIT; both were discarded. `_scoped_queue_result` returns the queue's own result -- summary and match count included -- so a limit bounds what is returned and never what is counted. Measured on the production queue at 54,205 matched branches, the reports produce byte-identical payloads: queue 2,761 ms to 398 ms, hotspots 2,733 ms to 500 ms. The hotspot report is the starker of the two. It asked SQL to sort the whole queue and kept ten rows, discarding 54,195 that had been normalized into Python dicts on the way. The tree and workers reports keep the unbounded path through `_scoped_queue_rows`: one needs the whole topology in scope, the other a branch-key set to filter workers against. `_collection_summary` is replaced by `_bucketed_collection_summary`, which takes the SQL summary rather than rows. The NULL worker-status bucket moves with it -- SQL counts a branch with no worker dimension under NULL, and a None key cannot be ordered against the strings beside it, so the server's sort_keys encoding fails on the whole report. Closes #366 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00c574f521
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The totals now come from their own aggregate rather than from the rows, which is what lets a limit bound what the server builds. That makes them a second statement, and a swarm moves branches between statuses continuously, so a status-filtered report could count matches the page no longer held -- reporting four queued branches above a page containing none. `report_queue_rows` builds both statements first and then runs them inside one deferred transaction, which takes its read snapshot at the first and holds it across the second. In WAL mode it blocks no writer, and it measures as noise on the production queue: 398 ms to 413 ms at 54,207 matched branches. The transaction is opened only when the connection is not already in one, so a caller that has its own stays in charge of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
Owner
Author
|
Review again @codex |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
1 participant
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.
Summary
Stacked on #365 — the base is
claude/bitmap-sweep-strip-v2, so review the last two commits._scoped_queue_rowsstripped the caller's limit (replace(filters, limit=None)) andcollect_queue_reportthen recounted the returned rows in Python with_collection_summaryandlen(). A limit therefore cut what was transferred and not what the server built.ERDQueue.report_queue_rowsalready computes its summary asCOUNT(*) … GROUP BY branch_status, branch_worker_statusand already accepts aLIMIT. Both were being thrown away by the caller._scoped_queue_resultreturns the queue's own result — summary andmatched_rowsincluded — so a limit bounds what is returned and never what is counted.Measured on the production queue, 54,205 matched branches
Payloads are byte-identical apart from the timestamp (209,604 vs 209,601 B; 54,058 B unchanged), with the same row and match counts.
sort=nodes, limit 10)The hotspot report is the starker case: it asked SQL to sort the whole queue and kept ten rows, having normalized 54,195 discarded rows into Python dicts on the way.
Totals and page read one snapshot
Publishing the aggregate makes the totals a second statement, where before they were measured from the materialized rows and so could not disagree with them. On a live swarm branches move between statuses continuously, so a status-filtered report could count matches the page no longer held — four queued branches reported above a page containing none.
report_queue_rowstherefore builds both statements first and runs them inside oneBEGIN DEFERRED, which takes its read snapshot at the first and holds it across the second. It opens the transaction only when the connection is not already in one, so a caller holding its own stays in charge. In WAL mode it blocks no writer, and it measures as noise: 398 ms to 413 ms.What keeps the unbounded path
The tree and workers reports still go through
_scoped_queue_rows, deliberately — one needs the whole topology in scope, the other a branch-key set to filter workers against. Neither is bounded by a display limit.The NULL bucket moves with the summary
_collection_summaryis replaced by_bucketed_collection_summary, which takes the SQL summary rather than a row list. SQL counts a branch with no worker dimension underNULL, and aNonekey cannot be ordered against the strings beside it, so the server'ssort_keysencoding fails on the whole report — the crash behind the queue report's HTTP 502. The old unit test fed the helper hand-built rows, a shape it will now never see; it is retargeted to the SQL summary shape and joined by an end-to-end test that builds a real finished branch and encodes the whole report withsort_keys.Two stubs in
tests/test_report_model.pyreturned{"rows": [...]}alone, whichreport_queue_rowsnever returns; they now carry the full shape.Tests
Four new guards, each proven to fail against a deliberate break:
test_queue_totals_are_counted_without_materializing_the_rowstest_hotspot_totals_count_matches_rather_than_returned_rowstest_a_queue_holding_a_finished_branch_encodes_with_sorted_keystest_queue_totals_and_rows_come_from_one_snapshot, which fails0 != 4with the snapshot disabledCloses #366. Part of #376.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ