Skip to content

LAB-411: fix lock acquire wire contract — snake_case, not camelCase - #34

Open
27Bslash6 wants to merge 1 commit into
mainfrom
lab-411-lock-wire-snake-case
Open

LAB-411: fix lock acquire wire contract — snake_case, not camelCase#34
27Bslash6 wants to merge 1 commit into
mainfrom
lab-411-lock-wire-snake-case

Conversation

@27Bslash6

Copy link
Copy Markdown
Contributor

Closes LAB-411.

Problem

CachekitIO::acquire_lock could never succeed against a spec-compliant CacheKit SaaS server. Both LockAcquireRequest and LockAcquireResponse were annotated #[serde(rename_all = "camelCase")], but the wire contract (protocol/spec/saas-api.md Lock Endpoints) is snake_case:

  • Request — rs sent {"timeoutMs": N}; the server reads body.timeout_msundefinedNaN lock expiry.
  • Response — the server returns {"lock_id": "<uuid>"} (acquired) or {"lock_id": null} (contested, protocol#22), always HTTP 200. serde expected lockId, and deny_unknown_fields turned the mismatch into a hard parse error — so every acquire failed, acquired or contested alike.

cachekit-py and cachekit-ts both handle snake_case correctly; rs was the lone outlier. No existing test exercised the acquire JSON round-trip, which is how it slipped through.

Fix

  • Drop rename_all = "camelCase" from both structs — the Rust field names (timeout_ms, lock_id) are the wire names, so no rename attribute is needed at all.
  • Keep deny_unknown_fields on the response (correct once the field name matches).
  • Pin the contract with round-trip tests: acquired → Some(uuid), contested → None, and the literal serialized request body {"timeout_ms":5000}. These fail loudly if a rename ever reappears.

Verification

  • New tests fail on the old code with exactly the diagnosed errors (unknown field 'lock_id', expected 'lockId'; body {"timeoutMs":5000}), pass after the fix.
  • Full cargo test green, cargo clippy --all-targets clean, cargo fmt --check clean.

Found during LAB-240 SDK verification (contested-lock spec amendment, cachekit-io/protocol#22). Related: cachekit-ts#70.

The wire contract (spec/saas-api.md Lock Endpoints) is snake_case:
{"timeout_ms": N} in, {"lock_id": ...} out. Both structs were annotated
rename_all = "camelCase", so the server read timeout_ms as undefined and
deny_unknown_fields made every response a hard parse error — acquire_lock
failed unconditionally against a spec-compliant server.

Drop the renames (Rust field names are the wire names) and pin the
contract with round-trip tests: acquired ({"lock_id":"<uuid>"} -> Some),
contested ({"lock_id":null} -> None, protocol#22), and the literal
serialized request body.

Co-authored-by: multica-agent <github@multica.ai>
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 46 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 78d841ae-7226-4f03-8e50-9338ef5fcb67

📥 Commits

Reviewing files that changed from the base of the PR and between e86172b and 5b368df.

📒 Files selected for processing (1)
  • crates/cachekit/src/backend/cachekitio_lock.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lab-411-lock-wire-snake-case

Comment @coderabbitai help to get the list of available commands.

27Bslash6 added a commit that referenced this pull request Jul 23, 2026
… checkout creds, README wording

- Register cachekit-lean + cachekit self-hosted labels in
  .github/actionlint.yaml so actionlint (CodeRabbit runs it) recognises
  runs-on values.
- Pin the redis:7 service image to its OCI index digest (resolved from
  Docker Hub 2026-07-23) — matches the SHA-pinned actions posture.
- persist-credentials: false on the redis-lock job checkout; it never
  pushes and the runner is self-hosted (zizmor artipacked).
- README: describe Workers lock/TTL support without claiming parity
  with the native SaaS backend while its lock wire format is still
  camelCase-broken pending #34 (LAB-411).

CodeRabbit-Resolved: .github/workflows/ci.yml:66:Unknown cachekit runner label
CodeRabbit-Resolved: .github/workflows/ci.yml:77:Pin the redis:7 service image
CodeRabbit-Resolved: .github/workflows/ci.yml:79:Set persist-credentials false
CodeRabbit-Resolved: README.md:216:Avoid claiming full parity
Co-authored-by: multica-agent <github@multica.ai>
27Bslash6 added a commit that referenced this pull request Jul 23, 2026
…26) (#37)

* feat(backend): Redis distributed locking + Workers lock/TTL capability impls

Backend capability parity per the LAB-273 audit:

- RedisBackend gains LockableBackend: SET NX PX acquire with a UUID
  capability token, atomic Lua compare-and-delete release. Callers pass
  the bare cache key; the backend derives <key>:lock on the wire,
  byte-identical to cachekit-py's Redis lock namespace so py and rs
  workloads contend on the same lock. Needs fred's i-scripts feature
  for EVAL.
- WorkersCachekitIO gains LockableBackend + TtlInspectable against the
  same SaaS endpoints the native CachekitIO impls call. The lock token
  travels in X-CacheKit-Lock-Id, never the query string (CWE-532).
  Contested acquire is 200 {"lock_id": null} — branched on the body,
  never on a 409 status (protocol#22 / LAB-240).
- SaaS lock/TTL JSON bodies live in backend/saas_wire.rs with literal
  wire-JSON round-trip tests compiled under cfg(test) unconditionally,
  so the snake_case contract is enforced in native CI (guards against
  the LAB-411 camelCase serde regression).
- Live Redis lock semantics covered by an env-gated integration test
  (CACHEKIT_TEST_REDIS_URL): acquire, contested None, foreign-token
  release refusal, double release, TTL self-expiry, wire key shape.

Co-authored-by: multica-agent <github@multica.ai>

* ci: exercise redis lock test against a live redis on the cachekit runner (LAB-426)

Co-authored-by: multica-agent <github@multica.ai>

* refactor(workers): route get/set/delete/health errors through error_from_response

Co-authored-by: multica-agent <github@multica.ai>

* fix: address coderabbit review — actionlint labels, redis digest pin, checkout creds, README wording

- Register cachekit-lean + cachekit self-hosted labels in
  .github/actionlint.yaml so actionlint (CodeRabbit runs it) recognises
  runs-on values.
- Pin the redis:7 service image to its OCI index digest (resolved from
  Docker Hub 2026-07-23) — matches the SHA-pinned actions posture.
- persist-credentials: false on the redis-lock job checkout; it never
  pushes and the runner is self-hosted (zizmor artipacked).
- README: describe Workers lock/TTL support without claiming parity
  with the native SaaS backend while its lock wire format is still
  camelCase-broken pending #34 (LAB-411).

CodeRabbit-Resolved: .github/workflows/ci.yml:66:Unknown cachekit runner label
CodeRabbit-Resolved: .github/workflows/ci.yml:77:Pin the redis:7 service image
CodeRabbit-Resolved: .github/workflows/ci.yml:79:Set persist-credentials false
CodeRabbit-Resolved: README.md:216:Avoid claiming full parity
Co-authored-by: multica-agent <github@multica.ai>

---------

Co-authored-by: multica-agent <github@multica.ai>
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.

1 participant