Repository navigation
perf(handoff-report): read selection heads in one snapshot - #2232
Inference1 wants to merge 1 commit into
Conversation
The report resolved each selected Scope's latest Handoff with a separate head read and revision load, so a selection cost about seven statements per Scope. Read all heads in one snapshot and load each selected revision on the same connection; the report still freezes one exact revision per Scope. Measured with mode=all: 17 selected Scopes take 35 statements instead of 115.
There was a problem hiding this comment.
Thanks, the problem is real and the change is well scoped. I reproduced it locally (SQLite, every Scope with a committed Handoff, median of 7 mode=all requests):
| Scopes | before | this PR |
|---|---|---|
| 50 | 353 statements / 190 ms | 103 statements / 47 ms |
| 300 | 2103 statements / 1284 ms | 606 statements / 297 ms |
So even without network round trips this is roughly a 4x latency win, and tests/test_handoff_report.py passes on the branch. I have no blocking concerns. A few optional notes:
-
The guard test name does not match its assertion.
test_handoff_report_statement_count_does_not_grow_with_scope_countassertsstatements <= 2 * selected + 3, which still allows linear growth: each Scope with a Handoff still costs two statements (the artifact row and its lineage), becauseArtifactRepository.get_manyand_load_lineage_manyare bound to a singlescope_id. Either name it after the budget it enforces (for example..._stays_within_two_statements_per_scope), or assert the difference between two sizes so the test protects the property its name claims. -
The
HandoffReportInconsistentErrorcheck inlatest_manycan no longer fire. The revision is loaded by its exact key inside the same snapshot that selected it, and the head-to-artifact foreign key guarantees the row exists, sohandoff.as_ref() != referenceis always false. Consider dropping it, or adding a short comment on why it stays. -
Head semantics now live in one more place. The adapter re-implements
ArtifactRepository._find_headfor many Scopes. This follows existing precedent (runtime/statistics.py,catalog_changes/service.py), so I don't think it should block, but a cross-Scope head read onArtifactRepositorywould keep "current Handoff" defined once. The existing copies already differ: statistics filterslifecycle_state == "active", whilelatest()does not.
I would not push this further toward a constant statement count in this PR. That means changing the shared artifact read path, and the report is already bounded by MAX_HANDOFF_REPORT_BYTES (10 MB, roughly 300 to 1100 Handoffs at 8 to 32 KB each). If Scope counts grow past that, pagination or a summary-only mode would matter more than query batching.
Which issue or RFC does this PR close?
No issue to close. Found while reviewing cross-Scope report reads.
Rationale for this change
HandoffReportApplication.get_reportresolved each selected Scope's Handoff withlatest()(head read plus revision load) and then re-read the revision, so a selection cost about seven statements per Scope.mode=alltherefore grew linearly with the number of Scopes.Measured against the real server with
mode=all(statements counted at the engine level):What changes are included in this PR?
HandoffReadAdapternow exposeslatest_many(scope_ids).RuntimeHandoffReadAdapterreads all selected heads with one query per 100 Scopes inside aconsistent_snapshottransaction and loads each selected revision on the same connection.HandoffReportInconsistentErroris still raised if the repository returns a different revision.RelationalContextsexposeshandoff_artifact_idso composition can build the adapter without reading a private attribute.test_handoff_report_statement_count_does_not_grow_with_scope_count: 16 created Scopes plus the default Scope must stay within2 * selected + 3statements.Are there any user-facing changes?
No. The report payload is unchanged.
How was this change tested?
pytest tests/test_handoff_report.py tests/test_client.py tests/test_mcp.py -q-> 49 passed.pytest tests/test_system_cli.py -k handoff -q-> 8 passed.assert 115 <= 37) and passes with the change (35statements).ruff checkandruff format --checkpass on all changed files.