Skip to content

refactor: single source of truth for SYNC_BATCH_LIMIT (closes #207)#369

Merged
intendednull merged 2 commits into
mainfrom
claude/audit-draft-issue-207-sync-batch-config
Apr 25, 2026
Merged

refactor: single source of truth for SYNC_BATCH_LIMIT (closes #207)#369
intendednull merged 2 commits into
mainfrom
claude/audit-draft-issue-207-sync-batch-config

Conversation

@intendednull
Copy link
Copy Markdown
Owner

@intendednull intendednull commented Apr 25, 2026

Hoists the sync-batch cap into willow-common::SYNC_BATCH_LIMIT and deletes the two mirror constants in storage and client. Behaviour unchanged (still 10_000); the change is purely "single source of truth, ship".

Placement

Constant lives in crates/common/src/lib.rs. willow-common is the workspace-wide leaf that already owns WireMessage::SyncBatch (the wire type the cap bounds) and is already a dep of willow-client. Adding it as a dep of willow-storage is a one-line cost; both crates now import the same pub const from the same place.

What changed

  • crates/common/src/lib.rs — new pub const SYNC_BATCH_LIMIT: usize = 10_000; with a doc comment that names both call sites and explains why they MUST agree.
  • crates/storage/src/store.rs — drop the associated const SYNC_BATCH_LIMIT on StorageEventStore, drop the // TODO(#207) mirror-warning comment, switch Self::SYNC_BATCH_LIMIT / StorageEventStore::SYNC_BATCH_LIMIT to the imported free constant.
  • crates/client/src/listeners.rs — drop the local const MAX_SYNC_BATCH_SIZE, use SYNC_BATCH_LIMIT from common (also unifies the name).
  • crates/storage/Cargo.toml — add willow-common dep.

Tradeoffs considered

  • Workspace [env] var or build-script knob. Rejected — not a real Rust constant, requires option_env! parsing, and the cap is a protocol invariant (producer must not exceed validator), not a deploy knob.
  • New willow-limits crate. Rejected — one constant doesn't justify a crate; willow-common is already the right home.
  • Promote to ClientConfig. Rejected for this PR — both producer (storage) and consumer (client) need to agree at compile time, so runtime tunability would require a coordinated wire-level negotiation. That's a separate feature; this PR closes the "drift hazard" bug today.

Verification

  • cargo check -p willow-common -p willow-storage -p willow-client — clean.
  • cargo clippy -p willow-common -p willow-storage -p willow-client --all-targets -- -D warnings — clean.
  • cargo test -p willow-storage — 33/33 pass, including history_caps_caller_limit_to_sync_batch_limit which directly exercises the cap.

Closes #207.


Generated by Claude Code

@intendednull
Copy link
Copy Markdown
Owner Author

single source of truth please, the best way you see fit

Single source of truth for the sync-batch cap, replacing the two
mirror constants that had drifted apart only in name (storage's
`SYNC_BATCH_LIMIT` vs the client listener's `MAX_SYNC_BATCH_SIZE`,
both literally `10_000`).

Placement: `willow-common`. It's the workspace-wide leaf where the
shared `WireMessage::SyncBatch` already lives, and is already a dep
of `willow-client`. Adding it to `willow-storage` (which previously
only depended on willow-state and willow-worker) is a one-line cost
and keeps the cap colocated with the wire type it bounds.

Tradeoffs considered:
- workspace const in `Cargo.toml` env var: rejected — not a real
  Rust constant, requires `option_env!` parsing, and the cap is a
  protocol invariant, not a deploy knob.
- new `willow-limits` crate: rejected — one constant doesn't
  justify a crate; willow-common is already the right home.
- promoting to `ClientConfig`: rejected for now — both producer
  (storage) and consumer (client) need to agree at compile time;
  runtime tunability is a separate feature behind its own PR.

Behaviour unchanged: same numeric value (10_000), same enforcement
points. Removes the "keep these in sync" comment hazard.
@intendednull intendednull changed the title [draft] feat(storage): configurable sync batch limits (refs #207) refactor: single source of truth for SYNC_BATCH_LIMIT (closes #207) Apr 25, 2026
@intendednull intendednull marked this pull request as ready for review April 25, 2026 18:51
Copy link
Copy Markdown
Owner Author

Done. Picked willow-common as the home: it already owns WireMessage::SyncBatch (the wire type this cap bounds) and was already a dep of willow-client, so adding it to willow-storage (one line) keeps the constant colocated with the type it constrains.

  • crates/common/src/lib.rspub const SYNC_BATCH_LIMIT: usize = 10_000;
  • crates/storage/src/store.rs — dropped the associated const + TODO mirror-warning, imports from common
  • crates/client/src/listeners.rs — dropped MAX_SYNC_BATCH_SIZE, imports from common (also unifies the name)

Behaviour unchanged. cargo check, clippy -D warnings, and cargo test -p willow-storage (33/33) all clean. Title and body updated, draft flag flipped, PR is ready for review.

Tradeoffs in the PR body — short version: rejected option_env! workspace var (not a real const, and this is a protocol invariant not a deploy knob), rejected a new willow-limits crate (overkill for one constant), and intentionally deferred ClientConfig promotion to a separate PR since runtime tunability needs a coordinated wire-level handshake between producer and validator.


Generated by Claude Code

@intendednull intendednull merged commit 5f33f2c into main Apr 25, 2026
5 checks passed
@intendednull intendednull deleted the claude/audit-draft-issue-207-sync-batch-config branch April 25, 2026 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[tech-debt] Hardcoded sync batch limit of 500 events lacks configuration and blocks protocol migration

2 participants