perf(sync): encode only what changed, and detect when the compressors move - #117
Merged
Conversation
Preparation compressed every asset on every sync, then the diff discovered
most of them were already in place — so a re-deploy paid full brotli q11 to
learn it had nothing to upload, and a no-op deploy paid it to learn nothing
at all. On text-heavy sites that pass is a third of deploy wall time.
Split preparation in two. `plan_project` scans, resolves media types and
headers, and hashes each asset's *uncompressed* bytes — a read and a SHA-256.
`PlannedAsset::encode` is the expensive half, and `sync-core` now calls it
only for assets `current_encodings` can't prove the canister already holds.
That inference is sound because compression is deterministic: for a given
build, every compressed encoding is a pure function of the identity bytes. So
a matching identity hash under a matching content_type implies matching
compressed encodings. It requires the encoding set to match *exactly* — a
missing compressed encoding is ambiguous ("didn't shrink" vs "an earlier
deploy died") and the two are indistinguishable without compressing, so any
mismatch re-encodes. It also requires preparation parameters to be frozen
within a release series; the version lock pairs code, not the provenance of
stored bytes, so that invariant is now written down in the project-stage rule.
Skipping applies to encoding only, never to diffing: `_headers` can change
while content doesn't, so headers and content_type are still compared for
every asset.
The deploy can no longer derive the state hash locally, since skipped assets'
compressed hashes are never computed. Rather than spend an extra consensus
round trip on `state_hash()`, `execute_operations` now returns it on the
`is_final` call. The summary reports it as canister-reported — a
self-consistency read, which is all the deploy-printed hash ever was; real
verification is still `state-hash <dist>` against reproduced source.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lazy encoding infers that an unchanged uncompressed hash implies unchanged
compressed encodings. That holds only while the compressors behave identically,
and nothing guarantees they will: compressed output is not part of a crate's
published interface, so a semver-compatible brotli bump may re-tune its
heuristics, flate2's DEFLATE bytes depend on which backend feature unification
picked, and neither is pinned by a spec — RFC 7932 and RFC 1951 define decoders.
Left alone, that inference fails silently and permanently. The identity hash
keeps matching, so stale compressed bytes are never re-uploaded and the
canister's state hash stays unreproducible with no error anywhere. The previous
eager pipeline could not fail this way: it compressed everything, compared, and
re-uploaded what differed, so the canister always converged.
So record what the compressors actually did rather than freezing them by
convention. `asset-prep::canary` hashes a frozen, deliberately varied input
through this build's compressors; the canister stores the value written
alongside its assets (a new stable cell on an unused MemoryId, so an upgraded
canister simply reads "unknown"). A mismatch makes the sync re-prepare every
asset and record the new fingerprint — the old converging behaviour, restored
automatically. This is detection, not proof: it samples encoder behaviour at one
point. What it catches reliably is what actually happens — a version bump or a
backend swap, both of which change essentially every output — and a miss leaves
the sync exactly where it would be with no canary at all.
That removes the freeze requirement, so the project-stage rule is downgraded
from a correctness constraint to a cost warning.
Also adds `Compression::{Enabled, Disabled}`, threaded to `sync-agent::SyncOpts`.
Disabled stores only the uncompressed copy: no compressor runs and ~20-25% fewer
bytes go over the wire, since one copy is uploaded instead of three. It is for a
platform deploying builds nobody browses, and is deliberately unreachable from
`icp deploy` — the saving goes to whoever runs the deploy while the 5-6x larger
transfers go to whoever visits the site, so it is only sound when the deployer
knows nobody will. Switching modes needs no bookkeeping: the diff derives the
desired encoding set from the mode, so enabling uploads the missing encodings
and disabling unsets them.
`state-hash` now prints both hashes, since the two modes hash differently and a
verifier cannot be expected to know which was used. No flags.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The canary detects compressor drift at deploy time, but nothing catches it before a release ships, and nothing checked that the plugin's wasm-compiled compressors agree with the verifier's native ones. Four gaps, closed together. Golden vectors pin gzip's and brotli's output bytes — length and sha256 — for a fixed structured input, plus the canary fingerprint they feed. These fire at PR time and say which compressor moved; the canary fires at deploy time, including for downstream builds CI never sees. Neither is redundant. `brotli` and `flate2` move into their own group in the workspace manifest, under one comment explaining why their output bytes matter. Neither is exact-pinned: a version pin cannot see a `flate2` backend swap (feature unification picks the DEFLATE implementation), and the golden vectors fail on any drift regardless, so pinning would add friction without adding detection. What does keep a build reproducible is the lockfile, so CI and the wasm builds now pass `--locked`: a stale lock fails the build instead of being silently updated. The lock is what pins the compressors whose bytes deployed canisters store. Finally, an e2e test does what `docs/verifying-contents.md` tells a third party to do: deploy, compute the hash from the same `dist/`, compare. It is the only test whose two sides are built for different targets — the plugin prepared the assets as wasm32-wasip2 under wasmtime, `asset-prep` runs natively here — so it is the one that would catch compressors disagreeing across targets, which would break verification for every canister while leaving every other test green. A second case re-deploys after editing one file, covering the skip path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three gaps found reviewing the branch. `benches.rs` matched `ComputationStatus::Done(())`, which stopped compiling when that arm gained the state hash. It sits behind the off-by-default `canbench-rs` feature, so neither the default test run nor `clippy --all-targets` builds it — only CI's canbench job would have caught it, and only after filing. Nothing checked that the compression canary survives an upgrade. It lives in stable memory precisely so it does; if it didn't, every patch release would look like a compressor change to the next sync and re-upload every asset. Also covers the unknown-canary default and the rejection of a wrong-length value, since a silently truncated canary would make a client trust encodings it shouldn't. The canary op's placement was argued for but not asserted: it must be the last operation so it rides in the `is_final` group, leaving a partially-failed sync with the old value. `SyncMock` now records the operations it executes, which is what that assertion needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Human review recommended
It changes canister wire behavior, stable storage, and sync semantics across multiple crates, so it warrants final human review despite good test updates.
Pull request overview
This PR optimizes the deploy/sync pipeline by avoiding unnecessary recompression on re-deploys, while adding a compressor-behavior “canary” to safely detect when compressed output could drift across builds/targets. It also introduces a Compression::{Enabled,Disabled} mode so platform integrators can choose to store only identity encodings for low-traffic/preview deployments, and updates verification tooling/docs to reflect the new hashing modes.
Changes:
- Split asset preparation into planning (hash uncompressed bytes) vs encoding, and encode only assets that are stale on the canister (guarded by a compressor fingerprint canary).
- Add
preparation_canarystate/API + wire types, record it at the end of full re-prepares, and return canister-recomputedstate_hashfrom the finalexecute_operationscall. - Update
state-hashCLI and docs to emit/describe bothcompressedanduncompressedhashes; enforce--lockedbuilds/tests to prevent silent compressor drift.
File summaries
| File | Description |
|---|---|
| Makefile | Build canister/plugin wasm with --locked to avoid silent dependency drift. |
| docs/verifying-contents.md | Document dual hashes (compressed/uncompressed) and clarify verification semantics. |
| docs/how-it-works.md | Explain lazy encoding, canary-based fallback, and compression on/off behavior. |
| crates/wire-types/src/lib.rs | Add SetPreparationCanaryArguments + Operation::SetPreparationCanary. |
| crates/sync-plugin/src/lib.rs | Force Compression::Enabled for icp deploy syncs. |
| crates/sync-core/tests/call_pattern.rs | Update call patterns for new preparation_canary query and new execute return type. |
| crates/sync-core/src/sync.rs | Implement lazy encoding via planning+diff; add canary gating and return state hash from finalize. |
| crates/sync-core/src/lib.rs | Re-export asset_prep::Compression. |
| crates/sync-core/src/canister.rs | Add preparation_canary() call; change execute_operations() to return opt blob. |
| crates/sync-agent/src/lib.rs | Add SyncOpts.compression and pass it through to sync-core. |
| crates/state-hash-cli/src/main.rs | Print both compressed/uncompressed hashes for the same dist/. |
| crates/e2e/tests/sync.rs | Assert deploy output includes “canister reports state hash”; assert no-op sync encodes 0 assets. |
| crates/e2e/tests/state_hash.rs | New e2e tests asserting verifier hash equals canister hash across targets and after partial redeploy. |
| crates/e2e/tests/latency.rs | Update SyncOpts construction to use defaults with new fields. |
| crates/e2e/src/lib.rs | Add helper to fetch state_hash from the canister for tests. |
| crates/e2e/Cargo.toml | Add asset-prep + serde_bytes deps for new tests. |
| crates/canister/src/lib.rs | Expose preparation_canary query; return opt blob from execute_operations. |
| crates/canister-core/src/store/mod.rs | Add stable cell for PreparationCanary at new MemoryId. |
| crates/canister-core/src/state/tests.rs | Add upgrade persistence + validation tests for preparation canary; update hash tests for new signatures. |
| crates/canister-core/src/state/sync.rs | Handle SetPreparationCanary op; return state hash on finalization. |
| crates/canister-core/src/state/hashing.rs | Add getters/setters for preparation canary in state layer. |
| crates/canister-core/src/lib.rs | Return state hash from final execute_operations; expose preparation_canary() API. |
| crates/canister-core/src/benches.rs | Adjust benches for new execute_operations return type. |
| crates/asset-prep/src/prepare.rs | Split into plan_project + PlannedAsset::encode; introduce Compression mode; update state hash helper signature. |
| crates/asset-prep/src/lib.rs | Export canary module and new planning/compression types. |
| crates/asset-prep/src/content.rs | Add golden-vector tests pinning compressor output bytes. |
| crates/asset-prep/src/canary.rs | New compressor fingerprint implementation + pinned input/fingerprint tests. |
| certified-assets.did | Add preparation_canary query; add SetPreparationCanary; change execute_operations return to opt blob. |
| Cargo.toml | Centralize compressor deps and document why they’re sensitive to drift. |
| Cargo.lock | Record new deps (e2e additions). |
| .github/workflows/ci.yml | Run tests/clippy with --locked; document rationale. |
| .claude/rules/testing.md | Document new canary + golden vector test expectations. |
| .claude/rules/project-stage.md | Document why prep/compressor changes are costly and should be deliberate. |
Review details
- Files reviewed: 32/33 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The `current_encodings` comment justified the strict encoding-set rule by claiming an asset whose compressed form isn't smaller is "tiny by definition". That is false: a large, high-entropy asset with a compressible content type — natively, or via a `_headers` Content-Type override — also fails to shrink. Measured, a 4 MB one costs ~1.5 s of brotli, and under this rule it pays that on every sync rather than once. The claim was hiding a real repeated cost. Says so plainly instead, and pins the case in a test so the comment can't drift back. Also records the second job the strict rule does, which the old comment omitted: it is what makes a Compression mode switch converge. A subset rule would fix the ambiguity this comment is about and silently break that. Behaviour is unchanged — removing the cost would need per-asset state recording which encodings were attempted, which isn't worth it for how rare the shape is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The concurrency ratio was assumed to cancel out machine speed, since it compares two runs done back-to-back on one machine. Measured, it doesn't: identical code gives 2.59x locally and 1.41x on a CI macos-15 runner. A 1.5x threshold sits inside that spread, so it flakes by construction — and it did, costing an 8-minute re-run. The delay is a replica-side wait that survives CPU contention, but it isn't the whole cost: the concurrent run also pays real transfer and polling for ~13 MB, and on a loaded runner that floor dominates (9s of the macOS run). That is why the ratio is machine-dependent. Lowers the floor to 1.2x, chosen against the failure this guards rather than picked to make CI quiet: silently serialized uploads drive the ratio to ~1.0, so a 20% floor still trips on that while clearing the worst observed run. Records both measurements in the module doc so the number isn't re-guessed later. Raising `artificial-delay-ms` was considered instead — rejected because it lifts the mean without touching the macOS transfer floor and taxes every run. Shrinking the payload isn't available: each file must exceed half the 1.9 MB chunk budget or files share upload calls and the call count collapses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Three related changes to what a deploy spends locally.
Lazy encoding. Preparation compressed every asset on every sync, and only then did the diff discover most were already in place — so a re-deploy paid full brotli q11 to learn it had nothing to upload. It now hashes uncompressed bytes first and compresses only what the canister doesn't already hold.
Compressor canary. That skip infers "unchanged uncompressed hash ⇒ unchanged compressed encodings", which holds only while the compressors behave identically — and nothing guarantees that: compressed output isn't part of a crate's published API, and RFCs 7932/1951 specify decoders. Left alone it fails silently and permanently. So each sync records a fingerprint of how its compressors actually behaved; a mismatch re-prepares everything, restoring the pre-lazy-encoding converging behaviour automatically. Golden vectors pin the same bytes at PR time, and CI now builds
--locked.Compression::{Enabled, Disabled}onsync-agent::SyncOpts.Disabledstores identity only — no compressor runs, and ~20–25% fewer bytes upload since one copy goes over the wire instead of three. Deliberately unreachable fromicp deploy: the saving goes to whoever runs the deploy, the 5–6× larger transfers go to whoever visits the site.Notes for the 0.3.2 release
MemoryId, sopost_upgradeneeds no migration.state-hashnow prints two hashes (compressed/uncompressed), since the two modes hash the samedist/differently.Not included
state-hash ./diststill rejects relative paths (pre-existingscan::absolute_root, correct for the WASI preopen and wrong natively). Separate PR.