Skip to content

fix: reduce checkpoint packing I/O before publication - #293

Merged
vincentkoc merged 1 commit into
mainfrom
fix/checkpoint-pack-stream-hashes-20261018
Oct 17, 2026
Merged

vincentkoc merged 1 commit into
mainfrom
fix/checkpoint-pack-stream-hashes-20261018

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

What Problem This Solves

Checkpoint packing rereads the entire database and every compressed part solely to compute hashes, consuming time needed for verified publication.

User Impact

Packing avoids those redundant reads while preserving the compressed format, authenticated restored-file hashes and full SQLite checks. The existing publication reserve and deadline remain unchanged. Sufficient production headroom still needs a measured end-to-end run.

Why This Change Was Made

The packing owner now carries the database hash from its existing input traversal and compressed hashes from successful output writes, including gzip headers and closing trailers. Incomplete writes, hash failures and close failures stop before upload. Fixed phase timings distinguish download, reconstruction, disk hashing and SQLite verification.

This follows the checkpoint transport introduced in #287 and its shared deadline in #291. Recovery, workflow triggers, credentials, retention and scheduling are unchanged.

Production LOC: +62/-32 (net +30) for bounded stream accounting and phase attribution. Tests: +131/-3.

Evidence

  • All 95 Python checkpoint tests passed, including exact gzip byte parity across empty, exact-boundary and multipart inputs; short/invalid writes; flush/trailer accounting; hash and close failures; and unchanged authenticated readback checks.
  • Both independent source reviews and fresh P0-P3 autoreview passed on 3c3f7feef8cd622b72c38a73d72a6377352186b7 (scoped-clean, no findings).
  • git diff --check passed. All 18 source checks passed, including CI 39056366959, Docker and CodeQL; later conditional bot-dispatch checks were skipped.
  • Exact-head ClawSweeper review reported no findings or before-merge requirements. Its optional Rank-up move—measure the shipped save path with representative state and authenticated readback—is deferred to the separately authorized postmerge manual qualification. Production publication headroom remains unproven; this PR does not activate workflows or schedules.

@clawsweeper

clawsweeper Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 17, 2026
@clawsweeper

clawsweeper Bot commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR computes checkpoint hashes during packing and adds separate transfer and verification timings.

Example: Packing a 1,073,741,824-byte SQLite checkpoint into two raw chunks.

  • Before: Packing separately rereads the database and both compressed files to compute hashes.
  • After: Hashes come from the existing input and output streams; downloaded files still receive independent verification.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Focused and source-correct, with meaningful regression coverage and successful checks; behavior evidence remains limited to an in-process setup.
Proof confidence 🦐 gold shrimp (3/6) This maintainer-authored optimization is outside the ordinary live-proof gate; supplied evidence exercises real packing and SQLite with a fake GitHub transport, and explicitly leaves production publication headroom unqualified.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Performance · Worth it: Yes · Fix scope: Complete
User problem: Redundant disk reads consume the checkpoint save budget available for verified publication.
Reason: The maintainer-authored change has clear bounded value, preserves the existing product contract, and adds no configuration or public API.

Merge readiness

✅ Ready for maintainer review

Keep this maintainer-authored draft open: it provides a focused optimization absent from main, with no introduced correctness or security defect found.

Priority: P2
Reviewed head: 3c3f7feef8cd622b72c38a73d72a6377352186b7

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: No shipped save run demonstrates measured packing savings and successful publication within the unchanged deadline; the supplied regressions were inspected, not rerun.
Agent review details

How this fits together

The private checkpoint helper snapshots native SQLite state, packs and uploads release assets, then authenticates a complete restore before publishing the checkpoint.

flowchart LR
A[Native SQLite state] --> B[Pinned snapshot]
B --> C[Compression and stream hashes]
C --> D[Private draft assets]
D --> E[Authenticated download]
E --> F[Disk and SQLite verification]
F --> G[Checkpoint promotion]
Loading

Technical review

Best possible solution:

Land the focused stream-hashing optimization while retaining the existing separate production qualification before workflow activation.

Do we have a high-confidence way to reproduce the issue?

Main visibly hashes the standalone database separately and rereads each compressed part; this establishes redundant I/O, but does not establish a production timeout.

Is this the best way to solve the issue?

Computing hashes during existing traversals addresses the redundant reads directly while preserving the independent authenticated restore and publication checks.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 95d1154f8972.

Provenance checked

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Review metrics

Metric Value Why it matters
Implementation and regression coverage Production +62/-32 lines; tests +131/-3 lines Growth covers bounded stream accounting, phase attribution, byte parity, and failure handling within the existing packing owner.

Labels

Label changes:

  • add P2: Removing redundant checkpoint I/O is a useful, limited-scope performance improvement without evidence of an urgent regression.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P2: Removing redundant checkpoint I/O is a useful, limited-scope performance improvement without evidence of an urgent regression.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Measure the shipped checkpoint save path with representative state and authenticated readback before claiming production publication headroom.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 17, 2026, 2:15 PM ET / 18:15 UTC.

@vincentkoc
vincentkoc marked this pull request as ready for review October 17, 2026 18:16
@vincentkoc
vincentkoc merged commit 5a092bd into main Oct 17, 2026
25 checks passed
@vincentkoc
vincentkoc deleted the fix/checkpoint-pack-stream-hashes-20261018 branch October 17, 2026 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant