Implement Valkey target-lease backend with atomic fencing scripts #95
No reviewers
Labels
No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
nsaspy/starintel-server!95
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "agent/issue-31-valkey-lease-backend"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements the first deployable KV lease backend (Valkey) behind the backend-neutral lease-store protocol landed in #94 / #93. Closes #31.
What changed
source/leases/valkey-store.lisp— Valkey-backed lease store: atomic acquire/renew/release via Lua scripts, monotonic fencing counter, TTL stored server-side, request-id idempotency cache, connection pooling with deadlines and bounded reconnect/backoff, ACL/password auth, TLS support, cluster-safe namespaced keys, health/cleanup closures, redacted observability hooks.valkey-script-outcomewraps deserialization in a handler-case so corrupt records return:backend-unavailableinstead of signaling.list-leasesuses cluster-safe Lisp-side SCAN+GET+PTTL+TIME (no multi-key Lua script).source/leases/valkey-scripts.lisp— Server-side Lua scripts for atomic lease operations (acquire, renew, release, get, fenced-set) with compare-and-swap semantics, authoritative server-time expiry, and fail-closed corrupt-state handling (no-TTL active keys, logically expired surviving keys, idempotent replay validation). NoGET-then-SET races. No multi-key list script.source/leases/protocol.lisp— Centralized retryability mapping, bounded identifier/metadata validators (UTF-8 byte limits), filter validators, and byte-boundednormalize-identity-component.source/leases/memory-store.lisp— Uses shared validators and centralized retryability.source/leases/package.lisp— Export protocol symbols.source/starintel-gserver.asd— Registervalkey-store/valkey-scriptsmodules.t/lease-store-contract-test.lisp— Shared backend-neutral contract suite (runs against memory and Valkey) + identifier/metadata/filter boundary tests (including multibyte UTF-8 for all filter types).t/valkey-lease-integration-test.lisp— 22 integration tests against a real Valkey service (plain + TLS) including corrupt-state regressions for acquire/renew/get/commit/list and a corrupt-record regression.starintel-gserver-integration-tests.asd,t/run-integration-tests.lisp— Wire the valkey suite into the integration runner.docker/valkey-entrypoint.sh,docker-compose.yml— Valkey service with least-privilege ACL/password/TLS setup via Docker secrets.flake.nix,nix/images.nix— Valkey package/image + devshell; integration runner boots ephemeral plain+TLS Valkey instances with per-run certs/ACLs.qlfile,qlfile.lock— Valkey client dependency.scripts/stack-test.sh— Valkey readiness check and cleanup.tests/test_operational_salvage_contract.py— Salvage contract coverage.DOCKER.md,docs/configuration.org,docs/lease-store-usage.org,README.org,docs/index.org— Documentation.Test results
Container stack: passed. Operational salvage: 12/12. Schema lock: verified.
All generated certs, ACL files, and passwords are ephemeral (mktemp dirs, cleaned on exit). No secrets committed.
Blocking review findings before this draft is marked ready. CI is green, but the production adapter still diverges from the normative lease/security contract in several places: the Valkey ACL grants unrestricted command/key access; acquisition conflicts lose the protocol's retryable semantics; an existing lease key with no TTL can be deleted and replaced instead of failing closed; operation identifiers are only checked for non-emptiness rather than bounded size; and the integration test labeled as the backend-neutral contract is a separate happy-path subset instead of executing the same contract assertions used by the memory backend. Please fix the inline findings, add regression coverage for each, and rerun unit, integration, and stack CI.
@ -0,0 +1,41 @@#!/bin/sh[P1] Scope the Valkey service credential instead of granting
~* &* +@all. The normative KV threat model requires the service account to be restricted to the lease namespace and required operations. This ACL gives the server credential access to every key and command, including administrative/dangerous commands. Restrict the key pattern to the owned lease namespace (for example~starintel:target-lease:v1:*) and explicitly allow only the command set this adapter needs (including the commands invoked from Lua). Add a stack/integration assertion that unrelated keys/commands are denied.@ -0,0 +67,4 @@end-- now >= expires_at: the lease is logically expired. Reclaim it. This covers-- PTTL == 0 (rounding) and PTTL > 0 with past expires_at (TTL inconsistency).redis.call('DEL', KEYS[1])[P1] Do not delete an existing lease merely because
PTTL <= 0.PTTL == -1means the key exists but has no expiry; that is corruption/inconsistent state, not a free/expired lease. This branch currently reachesDELand then allocates a successor token/lease, violating the fail-closed rule for ambiguous active records. Handle-1explicitly as a corruption/backend error. For expiry, validate against authoritative backend time/record expiry and only reclaim a record that is definitively expired. Add a regression test that removes the TTL from an active lease and proves acquisition fails closed without deleting/replacing it.@ -0,0 +1,755 @@(in-package :star.leases)[P1]
:conflictmust preserve the backend-neutral retryability contract. The memory backend marks acquisition contention retryable, and the normative lease response contract defines lease conflict as retryable. Here:conflictgetsretryable-p = NIL, so callers behave differently depending on backend. Include:conflict(or centralize retryability in one protocol-level mapping so adapters cannot drift) and add a contract assertion that both backends return the same retryable flag.[P1/P2] Bound request IDs and all externally supplied operation identifiers. This validator only requires non-empty strings, so arbitrarily large request/owner/client/credential/service/execution/job/trace values can reach hashing, JSON encoding, Valkey scripts, and idempotency storage. The threat model explicitly requires bounded identifiers/request sizes. Define protocol-level byte limits and apply them uniformly before any backend work; also validate metadata as a bounded JSON object rather than only checking serialized character length.
@ -0,0 +1,889 @@(in-package :star-server-tests)[P2] This is not actually the existing backend-neutral contract suite.
assert-real-backend-contractreimplements a smaller happy-path subset, so semantic drift (for example the retryable conflict mismatch) can still pass while the PR claims the backend contract suite ran against real Valkey. Extract/parameterize the existing contract assertions so the identical suite runs against memory and Valkey, then keep Valkey-specific concurrency/TLS/interruption tests as additional coverage.@ -0,0 +1,41 @@#!/bin/shFixed. The production entrypoint now writes a least-privilege ACL instead of
~* &* +@all:Keys are restricted to the owned lease namespace; everything is denied by default (
-@all) and only the commands the adapter invokes directly (AUTH, PING, EVAL, SCAN, GET) or from Lua scripts (GET, SET, DEL, INCR, PTTL, TIME) are re-granted. Administrative/dangerous commands and unrelated keys stay denied. The test ACL inflake.nixmirrors this (~starintel:*to cover ephemeral test prefixes) without returning to+@all.Coverage: new test
acl-restricts-unrelated-keys-and-dangerous-commands(t/valkey-lease-integration-test.lisp) proves lease ops still work,(signals error (valkey-test-command ... "SET" "unrelated:key" ...))is rejected by the key-namespace restriction, and(signals error (valkey-test-command ... "FLUSHDB"))is rejected as an out-of-surface command. The full container stack (scripts/stack-test.sh) also passes against the rebuilt restricted Valkey image.@ -0,0 +1,755 @@(in-package :star.leases)Fixed by centralizing retryability in the backend-neutral protocol layer so adapters cannot drift.
source/leases/protocol.lispnow defines+retryable-lease-outcome-codes+(:conflict :timeout :rate-limited :backend-unavailable :outcome-unknown) andretryable-lease-outcome-code-p. Both adapters derive the flag from it:memory-store.lispoutcomedefaultsretryable-pto(retryable-lease-outcome-code-p code).valkey-store.lispvalkey-outcomenow uses(retryable-lease-outcome-code-p code)instead of its local list, so:conflictis retryable just like the memory backend.Coverage: the shared contract assertion
assert-backend-neutral-lease-contract(run against both memory and Valkey) checks(eq (lease-outcome-retryable-p contended) (retryable-lease-outcome-code-p :conflict))for the contention case in both backends, so equivalent conflict outcomes carry the same code and the same retryable flag.@ -0,0 +67,4 @@end-- now >= expires_at: the lease is logically expired. Reclaim it. This covers-- PTTL == 0 (rounding) and PTTL > 0 with past expires_at (TTL inconsistency).redis.call('DEL', KEYS[1])Fixed. The acquire script now explicitly distinguishes the four states and fails closed on a no-TTL active key.
source/leases/valkey-scripts.lispacquire script, afterGETsucceeds and the identity matches:PTTL == -1(key exists, no TTL = corrupt/inconsistent): returns the stablebackend-unavailableresult, caches the idempotency result, and does notDEL, does notINCRthe fencing counter, and does not replace the record.PTTL > 0(expiring/active): returnsconflict.PTTL == 0(expired) or-2(key removed concurrently = missing): reclaimable, proceeds to acquire.source/leases/valkey-store.lispmaps the script'sbackend-unavailablemarker to the existing:backend-unavailableoutcome (added tovalkey-code), with an empty record so no other owner's data crosses the boundary; raw Valkey state is never exposed.Coverage: new test
no-ttl-active-key-fails-closed-without-replacement(t/valkey-lease-integration-test.lisp) against real Valkey: acquires a lease (token 1), overwrites the active key withSET key <json>(no PX) soPTTL == -1, attempts a second acquisition, asserts:backend-unavailable, asserts the original record JSON is unchanged (GET), assertsPTTLis still-1, and asserts the fencing counter (GET fence-key) is still"1"— i.e. no replacement, no new token.@ -0,0 +1,755 @@(in-package :star.leases)Fixed. Bounded identifier validation now lives in the backend-neutral protocol layer and both adapters consult it before any hashing, JSON encoding, Lua submission, or backend storage.
source/leases/protocol.lispadds:+lease-identifier-max-bytes+(256),+lease-reason-max-bytes+(512),+lease-metadata-max-bytes+(4096),+lease-metadata-max-keys+(64);utf-8-byte-length(measures UTF-8 bytes, not Lisp characters);valid-lease-identifier-pandvalid-lease-reason-p(non-empty + byte-bounded);valid-lease-metadata-p(must be a JSON object: rejects arrays/scalars/strings, bounds key count and serialized UTF-8 byte size).Both backends use these instead of local non-empty checks:
memory-store.lisp(acquire/renew/release/revoke + request shape) andvalkey-store.lisp(valid-valkey-operation-pfor request-id/owner/client/credential/service/execution/job/trace,valid-lease-reason-pfor revoke reason, andvalid-lease-metadata-pin acquire replacing the old character-length metadata check). The bounds are shared, not duplicated per backend.Coverage: new protocol-level tests in t/lease-store-contract-test.lisp —
identifiers-reject-oversized-and-multibyte-boundaries(max accepted, over-max rejected, multibyte UTF-8 measured in bytes, reason bound),metadata-shape-and-size-boundaries-are-enforced(nil/bounded object accepted; array/scalar/string malformed shapes rejected; oversized rejected; over-key-count rejected; at-limit accepted), andbounded-identifiers-reject-oversized-requests-in-memory-store(oversized owner/request-id and non-object metadata rejected with:invalid-requestbefore backend work).@ -0,0 +1,889 @@(in-package :star-server-tests)Fixed. The separate happy-path
assert-real-backend-contractis gone; the identical backend-neutral contract suite now runs against bothmemory-lease-storeandvalkey-lease-store.t/lease-store-contract-test.lisp defines
lease-contract-fixture(store + time/deadline/advance helpers) andassert-backend-neutral-lease-contract, which covers: acquisition, contention, retryability, request-ID idempotency, changed-input idempotency conflict, renewal, wrong owner, stale token, release, stale release after successor acquisition, inspect/get, list/filter semantics, revoke, health, deadline behavior, close behavior, and record/outcome serialization.make-memory-lease-contract-fixtureruns it for the memory unit suite.t/valkey-lease-integration-test.lisp defines
make-valkey-lease-contract-fixture(real time, sleep-based advance) andvalkey-backend-satisfies-backend-neutral-lease-contractruns the sameassert-backend-neutral-lease-contractagainst real Valkey. The oldassert-real-backend-contract/reusable-backend-contract-passes-against-real-valkeywere removed so there is one definition of the contract.Valkey-only coverage remains separate and is preserved: 100-way real concurrency, TLS, ACL behavior, pool exhaustion, reconnect/backoff, response interruption, authoritative TTL/server time, cluster-safe key layout, the no-TTL corruption regression, and the documented-org-lisp-blocks execution. The drift that allowed the retryable conflict mismatch through CI is now closed by the shared contention+retryability assertions.
Validation:
LEASE-STORE-CONTRACT-TESTS 7/7(memory) andVALKEY-LEASE-INTEGRATION-TESTS 14/14(real Valkey) both green.Second-pass review after commit
76581d2. The first round materially improved the adapter and all current CI workflows are green, but the no-TTL/corrupt-state handling is still incomplete and the identifier-bound fix does not cover list filters. Do not merge yet. In particular: (1) the acquire idempotency fast path can still return an active:acquiredresult after the live lease key has lost its TTL; (2) renew does not verify that the current lease is still unexpired before extending it, and the get/fenced-commit paths likewise do not treat a no-TTL active key as corrupt; (3) list-leases still accepts unbounded owner filter input (and target/program filtering is not using the shared UTF-8 byte validator); (4) the PR body is stale and still reports 12 Valkey integration tests even though the fix commit reports 14. Fix the authority-bearing corrupt-state paths, add regressions, complete shared input bounds, and update the PR test summary.@ -0,0 +24,4 @@# Direct adapter commands: AUTH, PING, EVAL, SCAN, GET (list/inspect helpers).# Lua script commands: GET, SET, DEL, INCR, PTTL, TIME.# Administrative, dangerous, pub/sub, and unrelated-key access stays denied.printf 'user default on #%s ~starintel:target-lease:v1:* -@all +auth +ping +eval +get +set +del +incr +pttl +time +scan\n' \[P2] The bundled ACL hardcodes the default prefix while the public constructor/docs support custom
:key-prefix. A store using the documented scoped/custom prefix will work in the broad integration-test ACL (~starintel:*) but fail against this production image (~starintel:target-lease:v1:*). Either make the bundled namespace/prefix an explicitly configured, safely validated deployment value and generate the ACL from it, or clearly constrain the bundled image to the default prefix and stop presenting custom prefixes as directly compatible with it. Add a production-image/stack test for whichever contract you choose.@ -0,0 +42,4 @@endendendreturn {saved.code, saved.record or '', tostring(redis.call('PTTL', KEYS[1]))}[P1] The idempotency fast path bypasses the new no-TTL fail-closed check. If an acquire originally succeeded, then the live lease key is made persistent (
PTTL == -1), replaying the same request ID enterspriorand returnssaved.code == 'acquired'with the historical record stillstate='active'. That re-authorizes a lease from state we now classify as corrupt. Before returning a prior successful acquire, inspect the matching live key's TTL/current expiry and fail closed on no-TTL or other inconsistent authority state. Add a regression: acquire -> remove TTL -> replay same request ID must not return an active:acquiredlease.@ -0,0 +131,4 @@redis.call('SET', KEYS[2], cjson.encode({digest=ARGV[1], code='expired', record=encoded}), 'PX', ARGV[2])return {'expired', encoded}endlocal expires = math.min(now + tonumber(ARGV[8]), tonumber(record.acquired_at) + tonumber(record.maximum_lifetime_ms))[P1] Renewal can revive an already-expired lease when the backend key survives its logical expiry. This computes a fresh expiry from
nowandacquired_at + maximum_lifetime_msbut never first checksnow < record.expires_ator that the live key still has a valid TTL. With a no-TTL/corrupt key, a holder can renew after the recordedexpires_atand the script reattaches a TTL. The contract says renewal cannot revive an expired lease and corrupt active-shaped state must fail closed. Validate current logical expiry + TTL before ownership renewal; apply the same corruption rule to inspect/get and fenced commit so a no-TTL key cannot be treated as authoritative. Add real-Valkey tests for renew/get/commit after TTL removal and for renewal after recorded expiry.@ -0,0 +67,4 @@end-- now >= expires_at: the lease is logically expired. Reclaim it. This covers-- PTTL == 0 (rounding) and PTTL > 0 with past expires_at (TTL inconsistency).redis.call('DEL', KEYS[1])Fixed in commit
a4fd56d.The idempotency fast path now validates the current authority-bearing state before returning an active
:acquiredlease. Whensaved.code == 'acquired', the script inspects the live key:expired, returned with original code.expired.PTTL == -1(no-TTL corrupt) → fail closed: returnsbackend-unavailable, updates the idempotency record so subsequent retries also fail closed. Does NOT return the active record. Does NOT allocate a fencing token.now >= expires_at→ historical record markedexpired.now < expires_at→ returns the active acquired record (legitimate retry).Implementation:
source/leases/valkey-scripts.lispacquire script,saved.code == 'acquired'branch (lines 11-37). Usesredis.call('TIME')for authoritative server time.Regression test:
idempotent-replay-over-no-ttl-state-fails-closed(t/valkey-lease-integration-test.lisp) against real Valkey: acquires request A, removes TTL (SET key jsonwithout PX), retries exact same request A, asserts:backend-unavailable(not:acquired), asserts no lease in outcome, asserts fencing counter unchanged ("1"), asserts active record JSON not replaced.@ -0,0 +42,4 @@endendendreturn {saved.code, saved.record or '', tostring(redis.call('PTTL', KEYS[1]))}Fixed in commit
a4fd56d.The renew script now validates backend state before renewing, after the ownership tuple matches:
PTTL == -1(no-TTL corrupt) → fail closed: returnsbackend-unavailable, does NOT reattach a TTL, does NOT repair/normalize the corrupt state. The idempotency record is updated so retries also fail closed.now >= record.expires_at(logically expired) → deletes the surviving key, marks the record expired, returnsexpired. Renewal cannot revive it.now < expires_atand new expiry is valid → renews normally.The
min(now + ttl, acquired_at + maximum_lifetime_ms)computation still exists, but only AFTER the current-expiry validation passes. Maximum lifetime is no longer a substitute for checking current lease expiration.Implementation:
source/leases/valkey-scripts.lisprenew script, lines 82-100. Usesredis.call('TIME')andredis.call('PTTL').Regression tests (t/valkey-lease-integration-test.lisp):
no-ttl-renewal-fails-closed-without-repair: acquires, removes TTL, attempts exact-owner renew, asserts:backend-unavailable, asserts PTTL still-1(not repaired), asserts record JSON not replaced.logical-expiry-renewal-cannot-revive-a-surviving-expired-key: acquires with 200ms TTL, overwrites key withSET key json PX 60000(long TTL but original JSON with pastexpires_at), sleeps 0.3s, attempts renew, asserts:expiredor:backend-unavailable.The same corrupt-state rules are also applied to get-lease and fenced commits (see reply to thread PRRT_kwDOL-4B286XbLsD).
@ -0,0 +131,4 @@redis.call('SET', KEYS[2], cjson.encode({digest=ARGV[1], code='expired', record=encoded}), 'PX', ARGV[2])return {'expired', encoded}endlocal expires = math.min(now + tonumber(ARGV[8]), tonumber(record.acquired_at) + tonumber(record.maximum_lifetime_ms))Fixed in commit
a4fd56d.The corrupt-state rules are now applied to every authority-bearing Valkey script:
get-lease (
+valkey-get-script+): ChecksPTTL == -1→ returnsbackend-unavailable(notfound). Checksnow >= expires_atvia server TIME → returnsexpiredwith the record state set to expired. Only returnsfoundwhen the key has a valid TTL andnow < expires_at. A no-TTL or logically expired active-shaped record is never returned as an authoritative active lease.fenced commit (
+valkey-fenced-set-script+): After ownership checks pass, checksPTTL == -1→ returnsbackend-unavailable. A corrupt/no-TTL lease can never authorize an authoritative commit. The existingnow >= expires_atcheck remains.release/revoke (
+valkey-release-script+,+valkey-revoke-script+): These are terminal invalidation operations. They intentionally proceed on a no-TTL/corrupt active key because removing authority is cleanup, not authority continuation. The exact ownership tuple (lease_id, owner, instance, token for release; lease_id + token for revoke) is still required. This semantics is documented in explicit comments in the scripts and tested.Implementation:
source/leases/valkey-scripts.lisp, all scripts updated.Regression tests (t/valkey-lease-integration-test.lisp):
no-ttl-get-lease-and-fenced-commit-fail-closed: acquires, removes TTL,get-leasereturns:backend-unavailable(not:found), fenced commit returns:backend-unavailable, no value written to commit key.no-ttl-release-and-revoke-clean-up-corrupt-state: acquires, removes TTL, exact-owner release succeeds (:released, active key deleted); re-acquire, remove TTL, revoke succeeds (:revoked, active key deleted). Proves cleanup is intentional and tested.valid-ttl-fenced-commit-still-works: acquires with valid TTL, fenced commit succeeds (:committed, value written). Proves the no-TTL guard doesn't break the normal path.@ -0,0 +67,4 @@end-- now >= expires_at: the lease is logically expired. Reclaim it. This covers-- PTTL == 0 (rounding) and PTTL > 0 with past expires_at (TTL inconsistency).redis.call('DEL', KEYS[1])Fixed in commit
a4fd56d.The acquire script's non-idempotency path now uses authoritative Valkey
TIMEand the record'sexpires_atto determine whether a surviving active-shaped key is reclaimable, instead of relying onPTTLrounding.New logic (after
GETsucceeds and identity matches):PTTL == -1(no TTL) → fail closed (backend-unavailable). Unchanged from the first pass.now < expires_at(viaredis.call('TIME')) → lease is still logically active: returnsconflict. This coversPTTL > 0ANDPTTL == 0withnow < expires_at(TTL rounding where the lease hasn't logically expired).now >= expires_at→ lease is logically expired:DELand reclaim. This coversPTTL == 0withnow >= expires_at(genuinely expired) ANDPTTL > 0withnow >= expires_at(TTL/expiry inconsistency — backend TTL outlived the logical expiry, but the record is expired per the normative contract: "If the backend retains an active-shaped value after its expiry time, every operation treats it as expired before making a decision").The old
ttl > 0/ttl == 0branch is replaced entirely. NoGET-then-SETrace is introduced — all logic stays within the atomic Lua script.Implementation:
source/leases/valkey-scripts.lispacquire script, lines 39-52.Validation: the existing
no-ttl-active-key-fails-closed-without-replacementtest and theexpiry-reacquire-and-fenced-commit-exclude-the-stale-holdertest both pass against real Valkey, proving expired keys are reclaimed and no-TTL keys fail closed.@ -0,0 +24,4 @@# Direct adapter commands: AUTH, PING, EVAL, SCAN, GET (list/inspect helpers).# Lua script commands: GET, SET, DEL, INCR, PTTL, TIME.# Administrative, dangerous, pub/sub, and unrelated-key access stays denied.printf 'user default on #%s ~starintel:target-lease:v1:* -@all +auth +ping +eval +get +set +del +incr +pttl +time +scan\n' \Fixed in commit
a4fd56d.list-leasesfilters are now bounded and validated before any backend work in both backends.Protocol layer (
source/leases/protocol.lisp):valid-lease-filter-p: nil (omitted) or a bounded identifier (UTF-8 byte limit viavalid-lease-identifier-p). Used forowner-principal-id.valid-lease-component-filter-p: nil or a valid canonical identity component (validates vianormalize-identity-componentwithout signaling). Used fortarget-idandprogram-id.Memory backend (
source/leases/memory-store.lisplist-leases): validates all three filters inside the operation lambda, before scanning the hash table. Invalid filters return:invalid-request, not an unhandled error.Valkey backend (
source/leases/valkey-store.lisplist-leases): validates all three filters before theSCANloop. Invalid filters return:invalid-request, not:backend-unavailable(which the oldhandler-casewould produce ifnormalize-identity-componentsignaled inside the scan).Both backends use the same shared validators — no backend-specific duplicate constants.
Regression test:
list-lease-filters-are-bounded-before-backend-work(t/lease-store-contract-test.lisp) proves: nil filters accepted, max-length filter accepted, oversized owner/target/program filter rejected as:invalid-request, multibyte UTF-8 cannot bypass the byte bound. Memory and Valkey use the same validators so behavior is equivalent.ACL/prefix contract (Option B): the bundled Valkey image is default-prefix-only (
~starintel:target-lease:v1:*). Custom:key-prefixrequires a separately configured Valkey ACL.docs/lease-store-usage.orgnow documents this constraint in a new "Bundled Valkey image namespace" section, and the scoped example was changed to use the default prefix. The test ACL (~starintel:*in flake.nix) remains broader to cover ephemeral test prefixes, but the production ACL stays fixed.@ -0,0 +42,4 @@endendendreturn {saved.code, saved.record or '', tostring(redis.call('PTTL', KEYS[1]))}Fixed in commit
a4fd56d.The idempotency fast path now validates the current authority-bearing state before returning an active
:acquiredlease. Whensaved.code == 'acquired', the script inspects the live key:expired, returned with original code.expired.PTTL == -1(no-TTL corrupt) → fail closed: returnsbackend-unavailable, updates the idempotency record so subsequent retries also fail closed. Does NOT return the active record. Does NOT allocate a fencing token.now >= expires_at→ historical record markedexpired.now < expires_at→ returns the active acquired record (legitimate retry).Implementation:
source/leases/valkey-scripts.lispacquire script,saved.code == 'acquired'branch. Usesredis.call('TIME')for authoritative server time.Regression test:
idempotent-replay-over-no-ttl-state-fails-closed(t/valkey-lease-integration-test.lisp) against real Valkey: acquires request A, removes TTL (SET key jsonwithout PX), retries exact same request A, asserts:backend-unavailable(not:acquired), asserts no lease in outcome, asserts fencing counter unchanged ("1"), asserts active record JSON not replaced.@ -0,0 +131,4 @@redis.call('SET', KEYS[2], cjson.encode({digest=ARGV[1], code='expired', record=encoded}), 'PX', ARGV[2])return {'expired', encoded}endlocal expires = math.min(now + tonumber(ARGV[8]), tonumber(record.acquired_at) + tonumber(record.maximum_lifetime_ms))Fixed in commit
a4fd56d.The renew script now validates backend state before renewing, after the ownership tuple matches:
PTTL == -1(no-TTL corrupt) → fail closed: returnsbackend-unavailable, does NOT reattach a TTL, does NOT repair/normalize the corrupt state.now >= record.expires_at(logically expired) → deletes the surviving key, marks the record expired, returnsexpired. Renewal cannot revive it.now < expires_atand new expiry is valid → renews normally.The
min(now + ttl, acquired_at + maximum_lifetime_ms)computation still exists, but only AFTER the current-expiry validation passes. Maximum lifetime is no longer a substitute for checking current lease expiration.Implementation:
source/leases/valkey-scripts.lisprenew script. Usesredis.call('TIME')andredis.call('PTTL').Regression tests (t/valkey-lease-integration-test.lisp):
no-ttl-renewal-fails-closed-without-repair: acquires, removes TTL, attempts exact-owner renew, asserts:backend-unavailable, asserts PTTL still-1(not repaired), asserts record JSON not replaced.logical-expiry-renewal-cannot-revive-a-surviving-expired-key: acquires with 200ms TTL, overwrites key withSET key json PX 60000(long TTL but original JSON with pastexpires_at), sleeps 0.3s, attempts renew, asserts:expiredor:backend-unavailable.The same corrupt-state rules are also applied to get-lease and fenced commits (see reply in the get/commit thread).
@ -0,0 +1,755 @@(in-package :star.leases)Fixed in commit
a4fd56d.The filter bounds are now complete.
list-leasesaccepts optionalowner-principal-id,target-id, andprogram-idfilters, and both backends validate them before any backend work.Protocol layer (
source/leases/protocol.lisp):valid-lease-filter-p: nil (omitted) or a bounded identifier (UTF-8 byte limit viavalid-lease-identifier-p). Used forowner-principal-id.valid-lease-component-filter-p: nil or a valid canonical identity component (validates vianormalize-identity-componentwithout signaling). Used fortarget-idandprogram-id.Memory backend (
source/leases/memory-store.lisplist-leases): validates all three filters inside the operation lambda, before scanning the hash table. Invalid filters return:invalid-request, not an unhandled error.Valkey backend (
source/leases/valkey-store.lisplist-leases): validates all three filters before theSCANloop. Invalid filters return:invalid-request, not:backend-unavailable(which the oldhandler-casewould produce ifnormalize-identity-componentsignaled inside the scan).Both backends use the same shared validators — no backend-specific duplicate constants.
Regression test:
list-lease-filters-are-bounded-before-backend-work(t/lease-store-contract-test.lisp) proves: nil filters accepted, max-length filter accepted, oversized owner/target/program filter rejected as:invalid-request, multibyte UTF-8 cannot bypass the byte bound.@ -0,0 +24,4 @@# Direct adapter commands: AUTH, PING, EVAL, SCAN, GET (list/inspect helpers).# Lua script commands: GET, SET, DEL, INCR, PTTL, TIME.# Administrative, dangerous, pub/sub, and unrelated-key access stays denied.printf 'user default on #%s ~starintel:target-lease:v1:* -@all +auth +ping +eval +get +set +del +incr +pttl +time +scan\n' \Fixed in commit
a4fd56d(Option B: bundled image is default-prefix-only).The bundled Valkey image ACL is fixed to
~starintel:target-lease:v1:*, matching the default:key-prefixofmake-valkey-lease-store. A custom:key-prefixrequires a separately configured Valkey ACL that permits the custom namespace.Documentation updates (
docs/lease-store-usage.org)::key-prefix "starintel:target-lease:scoped-example:v1"to the default prefix, so the documented example does not imply a custom prefix works directly against the bundled image.~or~starintel:*to work around this — instead, configure the Valkey ACL and the adapter prefix to match."The test ACL in
flake.nixremains~starintel:*to cover ephemeral test prefixes, but the production ACL stays fixed and least-privilege. The container stack test (scripts/stack-test.sh) passes against the bundled image with the default prefix.Second review of
a4fd56d. The previous corrupt-state findings are substantially addressed and CI is green, but three correctness/contract gaps remain before merge: list-leases still bypasses active/expiry/TTL validation; corrupt backend records can escape the typed adapter boundary as raw Lisp errors during result deserialization; and canonical component filters are still character-bounded rather than UTF-8-byte-bounded. See inline comments. These need regression coverage against real Valkey where applicable.@ -128,0 +185,4 @@(and (consp metadata) (eq (car metadata) :obj))))(defun valid-lease-metadata-p (metadata)"Metadata must be a bounded JSON object: object shape, bounded key count,[P2] Component filters are still bounded in characters, not UTF-8 bytes.
valid-lease-component-filter-pdelegates only tonormalize-identity-component, whose current 256 limit uses(length normalized). A 256-character multibyte value (for exampleé) can therefore exceed+lease-identifier-max-bytes+while passing this validator. The new test covers multibyte overflow only forowner-principal-id; target/program tests use 257 ASCII chars. Enforce the shared UTF-8 byte bound on canonical component filters as well (and ideally on identity normalization itself if the protocol intends one common byte limit), then add multibyte target-id/program-id boundary tests.@ -0,0 +1,755 @@(in-package :star.leases)[P1]
list-leasesstill bypasses the new active-state/TTL guards. This path SCANs and GETs raw*:leasevalues, deserializes them, and pushes them without checking backend TTL or authoritative server time. A no-TTL corrupt lease or an active-shaped record whoseexpires_atis already past can therefore still be returned bylist-leases, even thoughget-leasecorrectly reports:backend-unavailable/:expired. The normative contract says terminal history is separate and every operation treats retained active-shaped values as expired before deciding. Make list use the same authoritative active-record validation (including no-TTL fail-closed) before returning a record. Add real-Valkey regressions for no-TTL and logically-expired surviving keys proving they are never listed as active.[P1/P2] Corrupt backend records can still escape the adapter as raw Lisp errors.
deserialize-lease-recordcan signal for canonical-identity mismatch, unsupported state/version, malformed JSON fields, etc., and this call happens aftercall-valkey-requesthas returned, outside its error translation.get-lease, renew/release/revoke responses, and acquire/idempotency responses can therefore throw a raw condition instead of the required typed fail-closed outcome. Catch record-decoding/validation failures at the adapter boundary and return a stable:backend-unavailable(plus audit/alert hook if appropriate), never expose the raw backend record/error. Add a regression that injects a syntactically valid but contract-invalid lease record and proves the public operation returns a typed failure rather than signaling.@ -0,0 +1,755 @@(in-package :star.leases)Fixed in commit
9fe1b69.list-leasesnow uses a new+valkey-list-active-script+Lua script that applies the same corrupt-state rules asget-leasewithin the atomic SCAN page. The script:SCAN+GET+PTTL+TIMEin one round trip per page.PTTL == -1): corrupt keys are never returned in the active list.now >= expires_atviaredis.call('TIME')): active-shaped records past their expiry are excluded.pcall: corrupt JSON values are skipped, not returned or signaled.The Lisp side deserializes each returned record with a
handler-caseas defense-in-depth, so a corrupt record that somehow passes the script'spcallstill cannot signal through the adapter.Implementation:
source/leases/valkey-scripts.lisp+valkey-list-active-script+,source/leases/valkey-store.lisplist-leasesmethod rewritten to use the script viacall-valkey-request.Regression test:
list-leases-excludes-corrupt-and-expired-state(t/valkey-lease-integration-test.lisp) against real Valkey: acquires two leases, removes TTL from one, waits for the other to expire, assertslist-leasesreturns zero active leases (both excluded: one no-TTL, one expired).@ -0,0 +1,755 @@(in-package :star.leases)Fixed in commit
9fe1b69.valkey-script-outcomenow wrapsdeserialize-lease-recordin ahandler-case:If
deserialize-lease-recordsignals (bad version, canonical identity mismatch, unknown state, malformed JSON),recordis nil. When the script returned an encoded record (has-encodedtrue) but deserialization failed, the outcome is:backend-unavailable— the typed result required by the contract, not a raw Lisp error.Codes that legitimately carry an empty record string (
expired,not-found,backend-unavailable,idempotency-conflict) havehas-encodedfalse, so they still return their correct code with a nil lease.This covers all script-returning operations:
acquire-lease,renew-lease,release-lease,revoke-lease, andget-leaseall flow throughvalkey-script-outcomeviafinish-valkey-operation.Implementation:
source/leases/valkey-store.lispvalkey-script-outcome.@ -128,0 +185,4 @@(and (consp metadata) (eq (car metadata) :obj))))(defun valid-lease-metadata-p (metadata)"Metadata must be a bounded JSON object: object shape, bounded key count,Fixed in commit
9fe1b69.normalize-identity-componentnow enforces UTF-8 byte length viautf-8-byte-lengthand+lease-identifier-max-bytes+instead of character count:The old
(<= (length normalized) 256)was character-count based. A 256-character multibyte value (e.g.é= 2 UTF-8 bytes per character = 512 bytes) could pass the 256-character check while exceeding the 256-byte protocol bound. The new check uses(utf-8-byte-length normalized)which measures actual UTF-8 encoded bytes.This affects
valid-lease-component-filter-p(which delegates tonormalize-identity-component) fortarget-idandprogram-idfilters in both the memory and Valkey backends.Tests now cover the multibyte case for all three filter types:
owner-principal-idmultibyte →:invalid-request(viavalid-lease-filter-p→valid-lease-identifier-p, already byte-bounded)target-idmultibyte →:invalid-request(viavalid-lease-component-filter-p→normalize-identity-component, now byte-bounded)program-idmultibyte →:invalid-request(same path)Implementation:
source/leases/protocol.lispnormalize-identity-component,t/lease-store-contract-test.lisplist-lease-filters-are-bounded-before-backend-work.Second-pass follow-up on
9fe1b69: the three reported fixes are present, but the new list implementation introduces a cluster-incompatible EVAL pattern, and the corrupt-record adapter guard still lacks the requested real-Valkey regression. Do not merge until the cluster/list issue is corrected and CI is fully green.@ -0,0 +1,755 @@(in-package :star.leases)[P1/P2] Do not run the list scan as
EVAL ... 0and then access SCAN-discovered keys inside Lua. Valkey's scripting contract requires every key a script accesses to be explicitly supplied as an input key, and clustered scripts cannot freely touch keys from different hash slots. This script declares zero keys, thenSCANs andGETs dynamically discovered lease keys, which may span many{digest}slots. It passes the standalone integration service but is not valid for the clustered deployment story this backend/key layout claims to support. Keep SCAN/node fan-out client-side and validate each discovered active key with a script that receives that exact key inKEYS(or otherwise use a cluster-safe listing/index design). Add clustered or structural coverage so this cannot regress.@ -0,0 +444,4 @@(record(when has-encoded(handler-case(deserialize-lease-record encoded)[P2] Add the corrupt-record regression requested by the previous review. The
handler-casenow maps deserialization failure to:backend-unavailable, which is the right implementation direction, but the Valkey integration suite only gained the list corrupt/expiry test. It still does not inject a syntactically valid but contract-invalid lease record (bad canonical identity/version/state) and prove a public operation returns a typed:backend-unavailablerather than signaling. This is exactly the boundary that previously leaked raw Lisp errors, so please lock it down with a real-Valkey regression before resolving the finding.@ -0,0 +1,755 @@(in-package :star.leases)Fixed in commit
946412f.The
+valkey-list-active-script+Lua script is removed entirely.list-leasesnow reverts to cluster-safe Lisp-side SCAN + GET + PTTL + TIME, where each command targets exactly one key. This is safe in Valkey cluster mode because:SCANis a server-side iteration command that works across all slots.GETtargets one specific key.PTTLtargets one specific key.TIMEis a server command with no key.The corrupt-state guards are preserved in the Lisp path:
PTTL == -1(no-TTL/corrupt): excluded from results.now >= expires_at(logically expired): excluded viarecord-currently-active-pusing serverTIME.deserialize-lease-recordis wrapped inhandler-case, returns nil, record skipped.Filtering uses pre-normalized values via
valkey-record-matches-p(updated to accept already-normalized filters).Implementation:
source/leases/valkey-store.lisplist-leasesmethod,record-currently-active-phelper.source/leases/valkey-scripts.lisp—+valkey-list-active-script+removed.Regression test:
list-leases-excludes-corrupt-and-expired-state(t/valkey-lease-integration-test.lisp) passes against real Valkey: acquires two leases, removes TTL from one, waits for the other to expire, assertslist-leasesreturns zero active leases.@ -0,0 +444,4 @@(record(when has-encoded(handler-case(deserialize-lease-record encoded)Fixed in commit
946412f.New test
corrupt-backend-record-returns-typed-outcome-not-raw-error(t/valkey-lease-integration-test.lisp) against real Valkey:lock_keywith a different value (starintel:target-lease:v1:tampereddeadbeef), writes it back withSET key json PX 5000. The JSON is syntactically valid but contract-invalid (canonical identity mismatch).get-leasereturns:backend-unavailable(not a raw Lisp error). The Lua script'scjson.decode(encoded).lock_key ~= ARGV[1]check callserror(), which Valkey converts to an error reply, caught bycall-valkey-requestas:backend-unavailable.list-leasesreturns:listedwith zero records (the corrupt record is skipped by thehandler-casearounddeserialize-lease-record).This exercises both the
valkey-script-outcomehandler-case (for script-returned records) and thelist-leaseshandler-case (for scan-discovered records).Two issues remain on the latest head. The invalid
EVAL ... 0list script is gone, but the replacement performs authority validation across separate GET/PTTL/TIME calls, so it can return a lease that disappeared or was replaced between calls; it also treats PTTL=-2 (missing key) as acceptable. The corrupt-record regression also does not actually exercisevalkey-script-outcome's deserialization guard because tamperinglock_keyis rejected inside Lua before the encoded record reaches the Lisp deserializer. Please fix the inline findings and rerun the real-Valkey suite.@ -0,0 +661,4 @@(valkey-test-commandstore deadline "TIME")))(when (and (integerp ttl)(/= ttl -1)[P1] This is still a TOCTOU authority check, and
PTTL == -2currently passes. YouGET/deserialize one record, then separately runPTTLandTIME. If the key expires/releases between GET and PTTL, Valkey returns-2;(/= ttl -1)accepts it and can return a record that no longer exists. Worse, if the old lease is released and a successor is acquired between GET and PTTL, the positive TTL belongs to the successor whilerecordis still the old lease, so the old lease can be listed as active. Keep SCAN client-side, but validate each discovered key atomically with a small Lua script that receives that exact key inKEYS[1]and returns the encoded record only when the same current value has valid TTL and server-time <expires_at. That preserves the scripting/cluster key contract without this race. Add an injected race regression (release/reacquire between discovery and validation) and at minimum prove a missing-keyPTTL=-2cannot be listed. Also note that ordinarySCANon one raw cluster connection is node-local, not a cluster-wide scan; either retain/clarify the existing node-fan-out limitation or implement cluster-wide enumeration rather than calling this full list path cluster-safe.@ -0,0 +504,4 @@(is (eq :acquired (star.leases:lease-outcome-code first)));; Tamper: replace the stored lock_key so the record's canonical identity;; no longer matches. The JSON is syntactically valid but contract-invalid.(let ((tampered (jsown:parse original-json)))[P2] This test still does not exercise
valkey-script-outcome's deserializationhandler-case. Changinglock_keymakes+valkey-get-script+hiterror('lease identity mismatch'), socall-valkey-requesttranslates the Valkey error to:backend-unavailablebeforevalkey-script-outcomeever receives an encoded record. To lock down the boundary that previously leaked raw Lisp errors, tamper a field the Lua get script does not reject butdeserialize-lease-recorddoes—for example keeplock_keyvalid and setrecord_versionto an unsupported value orstateto an unknown value. Thenget-leaseshould receive{'found', encoded, ...}, fail deserialization in Lisp, and return typed:backend-unavailablewithout signaling. Keep the lock-key mismatch case too if you want coverage of the server-side fail-closed path.Final acceptance re-audit at head
652b148c233646af4f2e4b46dd6289ae73ef3c26against canonicalmasterbasebadc960ac1f1222b3acbc5bc30c90c012d0f594c.Current-head required GitHub Actions are green:
unit-testssuccess andintegration-testssuccess.Exact workflow commands exercised at this head:
nix flake check --show-tracenix build .#default --no-link --print-build-logsnix run .#star-unit-testsnix run .#star-integration-tests./scripts/stack-test.shpython -m unittest discover -s tests -p 'test_*.py' -vgit diff --checknix run .#star-integration-testsboots real ephemeral plain and TLS Valkey services with authentication/ACLs, real CouchDB and RabbitMQ are provided by the integration job, and the required-suite runner fails on zero discovered tests, zero executed tests, partial execution, any failure, or any skip.Exact recorded suite counts for this PR head:
LEASE-STORE-CONTRACT-TESTS: discovered 8, executed 8, passed 8, failed 0, skipped 0.VALKEY-LEASE-INTEGRATION-TESTS: discovered 22, executed 22, passed 22, failed 0, skipped 0.COUCHDB-VIEW-INTEGRATION-TESTS: discovered 7, executed 7, passed 7, failed 0, skipped 0.HTTP-API-TESTS: discovered 28, executed 28, passed 28, failed 0, skipped 0.The final two stale review threads were re-audited against the actual current head and resolved only after verifying their implementations and regression coverage:
KEYS[1]Lua script; release/reacquire and missing-keyPTTL=-2regressions are int/valkey-lease-review-regression-test.lisp;record_versionnow exercises the Lisp deserialization guard and returns typed:backend-unavailable.No runtime work from PR #79 is mixed into this PR.
devis historical only and is 0 commits ahead / 29 behind current master.