LAB-411: fix lock acquire wire contract — snake_case, not camelCase - #34
LAB-411: fix lock acquire wire contract — snake_case, not camelCase#3427Bslash6 wants to merge 1 commit into
Conversation
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>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
… 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>
…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>
Closes LAB-411.
Problem
CachekitIO::acquire_lockcould never succeed against a spec-compliant CacheKit SaaS server. BothLockAcquireRequestandLockAcquireResponsewere annotated#[serde(rename_all = "camelCase")], but the wire contract (protocol/spec/saas-api.mdLock Endpoints) is snake_case:{"timeoutMs": N}; the server readsbody.timeout_ms→undefined→NaNlock expiry.{"lock_id": "<uuid>"}(acquired) or{"lock_id": null}(contested, protocol#22), always HTTP 200. serde expectedlockId, anddeny_unknown_fieldsturned 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
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.deny_unknown_fieldson the response (correct once the field name matches).Some(uuid), contested →None, and the literal serialized request body{"timeout_ms":5000}. These fail loudly if a rename ever reappears.Verification
unknown field 'lock_id', expected 'lockId'; body{"timeoutMs":5000}), pass after the fix.cargo testgreen,cargo clippy --all-targetsclean,cargo fmt --checkclean.Found during LAB-240 SDK verification (contested-lock spec amendment, cachekit-io/protocol#22). Related: cachekit-ts#70.