Repository navigation
Speed up work queue checkpoint verification - #75352
Conversation
Reuse validated canonical prefixes and native Git blob identities, reconstruct checkpoint source hashes without repeated replay, and avoid redundant compressed receipt parsing and cloning. Add bounded synthetic and captured-history benchmarks plus integrity regressions while preserving delivery deadlines and authority checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills... |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Specification Reviewer completed successfully!
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed PR head cbc92af against the work-queue contract, especially docs/src/content/docs/specs/work-queue-specification.md section 7.6 (checkpoint verification and causal Git history).
Blocking finding: the new canonical-prefix shortcut can skip the declared checkpoint boundary when the prior commit is itself a checkpoint root, causing valid nested checkpoint history to be rejected and bypassing the required fresh source replay. See the inline comment on actions/setup/js/work_queue_store.cjs:422.
The benchmark and specification additions otherwise align with the stated optimization intent; no code or specification edits were made by this review.
Generated by PR Specification Reviewer for #75352 · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review-specs to run again
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
What this change does: It avoids replaying validated checkpoint prefixes and adds stricter checkpoint-source checks. It also adds synthetic and captured Git-history benchmarks.
Should fix:
-
Prefix cache stores data the caller never reads (
actions/setup/js/work_queue_store.cjs:L489)- What this is:
canonicalGitPrefixesbuilds a lookup for already validated Git blob IDs. - Problem:
readQueueContentsAtSHAonly tests whetherget(logs[0].sha)is defined, then usesprefixVerified; it never consumes the stored prefix string. A 256-line log therefore retains 256 overlapping copies of the log. - Fix: Store only the blob IDs in a
Set, usehas, and return the verification marker without retaining prefix text. - If we skip it: The fast path keeps needless data alive and makes large-history validation allocate far more than the lookup needs.
- What this is:
Verdict: fix 1 first.
Lean: -1 line possible.
Generated by ✂️ Ponytail Reviewer for #75352 · codex · routed: gpt56 max · ~7 AIC · ⌖ ~1 AIC · ⊞ 118
Comment /ponytail to run again
| .update(`blob ${offset}\0`, "utf8") | ||
| .update(bytes.subarray(0, offset)) | ||
| .digest("hex"); | ||
| prefixes.set(oid, contents.slice(0, end)); |
There was a problem hiding this comment.
Ponytail: This stores a full canonical prefix for every blob ID, but the caller only checks whether the ID exists and uses prefixVerified; keep a Set of IDs instead so large logs do not retain overlapping prefix strings.
|
No code change is needed for this reviewer-start notice. It contains no actionable code-quality finding. |
|
No code change is needed because this notice contains no test improvement request. The |
|
Successful reviewer execution does not establish PR correctness, so no code change is justified by this completion notice alone. The separate checkpoint finding was evaluated and answered in its inline thread: distinct canonical roots prevent the alleged shortcut, and the nested-checkpoint and full-history tests passed. |
|
No code change is needed for this reviewer-start notice. It contains no actionable simplification request. |
|
No code change is needed for this reviewer-start notice. It contains no actionable finding. |
|
No code change is needed because this notice reports no requested action. The |
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Found two actionable performance/resource regressions in the checkpoint verification changes.
Highlights
- The compressed receipt path no longer uses the strict parser limits, allowing oversized request arrays/payloads during queue reads.
- Git prefix object-ID generation rehashes the complete growing history for every prefix, making the new fast path quadratic in payload size.
The specialist pass completed; its additional candidates were adjudicated and not included.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~8 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| const requestsText = inflateRawSync(Buffer.from(wire.requests_compressed, "base64"), { maxOutputLength: 64 << 20 }).toString("utf8"); | ||
| const requests = parseStrictJSON(requestsText); | ||
| // Exact canonical equality also rejects duplicate keys and lossy numbers. | ||
| const requests = JSON.parse(requestsText); |
There was a problem hiding this comment.
Replacing parseStrictJSON with JSON.parse removes the snapshot parser's resource bounds: compressed requests can now contain more than 16 MiB of JSON or arrays larger than 16,384 elements, and canonical does not restore the array-size limit. An untrusted checkpoint can therefore drive substantially more allocation/CPU during every queue read.
💡 Preserve the strict parser limits
Use parseStrictJSON(requestsText, { maxBytes: ... }) (or add equivalent explicit size/member checks before canonicalization) so duplicate-key rejection and the existing depth, integer, and array bounds remain enforced. Keep the canonical-byte equality check for the compressed receipt.
| let end = 0; | ||
| for (const line of lines) { | ||
| offset += Buffer.byteLength(line, "utf8"); | ||
| end += line.length; |
There was a problem hiding this comment.
canonicalGitPrefixes hashes bytes.subarray(0, offset) from the beginning for every line, making prefix construction O(total-history-bytes × number-of-lines) rather than linear. On the large ledgers this optimization is intended to accelerate, it needlessly rehashes the growing payload and can consume the cold-validation budget before Git traversal begins.
💡 Build prefix object IDs incrementally
Maintain a running hash for the blob body and derive each prefix object ID from the accumulated state (or cache/reuse the computed prefix bytes) instead of hashing the entire prior prefix on every iteration. Add a benchmark that isolates this helper for the production-sized history.
There was a problem hiding this comment.
🟡 Changes recommended
Checkpoint parsing can consume excessive resources before structural limits are enforced, and verification can skip source-root ID validation, allowing conflicting records under one durable ID.
3 open issues
What changed in this PR
This PR speeds up gh-aw’s work-queue checkpoint verification, leaving more of the durable completion budget available for independent delivery verification.
Changes:
- Reuses validated canonical history prefixes, including matches established through complete Git blob identities.
- Verifies checkpoint source snapshots directly and reduces receipt parsing and cloning overhead.
- Adds performance benchmarks, malformed-history regressions, and documented local timing budgets.
| File | Description |
|---|---|
docs/src/content/docs/specs/work-queue-specification.md |
Documents prefix reuse, verification boundaries, and benchmark budgets. |
actions/setup/js/work_queue_store.cjs |
Adds canonical-prefix reuse and direct checkpoint-source verification during ancestry traversal. |
actions/setup/js/work_queue_replay.cjs |
Refactors checkpoint reconstruction and receipt parsing to reduce repeated work. |
actions/setup/js/work_queue_live_performance.test.cjs |
Adds a captured-history benchmark using production Git transport. |
actions/setup/js/work_queue_git_transport.test.cjs |
Adds synthetic cold/warm benchmarks and malformed-ancestor regressions. |
actions/setup/js/work_queue_checkpoint_regression.test.cjs |
Tests source-boundary validation and rejection of noncanonical compressed receipts. |
Review effort: Max (auto)
| // Exact canonical equality also rejects duplicate keys and lossy numbers. | ||
| const requests = JSON.parse(requestsText); |
| } | ||
| if (parent === checkpoint.prior_git_sha) { | ||
| try { | ||
| const state = replayCheckpointSource(contents, parent, root); |
| let lines = transactions.map(transaction => canonical(transaction) + "\n"); | ||
| const rootBytes = Buffer.byteLength(lines[0], "utf8"); | ||
| let validatedPrefix = lines.join(""); | ||
| let prefixes = queueGitScope(githubClient) ? canonicalGitPrefixes(lines, head.length) : undefined; |
Cold work queue history validation could exhaust the five-minute durable Completion verification budget before independent delivery verification began. The smoke worker could record its completed finish but fail to publish a verified Result.
Changes
Single-parent ancestry, checkpoint reconstruction, authenticated cache scope, strict transaction parsing, immutable assignment authority and durable delivery deadlines remain enforced.
Validation
make agent-report-progressgate passed.71a8b707c53344d53540482eb0179b5a043a90a0: 135 Work items, 147 Claims and 1,325 retained requests. Full cold Git verification, including local object transfer and integrity checks: 5.769 seconds. Ledger replay alone: 78 ms. Exact head and full replayed projection matched.No new hosted workflow run, production queue mutation or uncertain-effect replay was performed. Hosted delivery verification with this revision has not been rerun.