Skip to content

Commit 97047a5

Browse files
committed
refactor(runtime): share HMGET projection zipping via HashProjections
Extract the private mapHashFields helper from RedisReservationRepository into a shared data.util.HashProjections, and switch the expiry event hydration from positional HMGET reads (fields.get(0)..get(6)) to name-keyed reads over a declared EXPIRED_EVENT_FIELDS projection. Reordering or inserting a projected field can no longer silently shift every later column in the expired-event payload. No wire or behavior change: the HMGET commands issued are byte-identical. Review follow-up (PR #237 round 2, minor carryover). Unit tests added for the shared helper; full mvn verify green with the 95% JaCoCo gate.
1 parent cc3cd0b commit 97047a5

6 files changed

Lines changed: 124 additions & 32 deletions

File tree

AUDIT.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,10 @@ every `HGETALL`. Mutation scripts also omit snapshot JSON when invoked without
5252
an idempotency key. The response-state marker remains available for the
5353
evidence finalization state machine, so this optimization does not weaken its
5454
race guarantees. Detail uses a static projection and both list paths construct
55-
their projection array once per request rather than once per Redis key.
55+
their projection array once per request rather than once per Redis key. All
56+
`HMGET` replies are zipped to field names by one shared `HashProjections`
57+
helper; the expiry event path now reads fields by name rather than positional
58+
index, so reordering a projection cannot silently shift columns.
5659

5760
**Clear replay compatibility and metrics.** The Lua commit/release replay
5861
branches now do only payload-hash validation and return the immutable snapshot

CHANGELOG.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,9 @@ called out but are not breaking to API clients.
5555
now use explicit `HMGET` projections. Immutable response snapshots and other
5656
unrelated hash fields are no longer transferred and parsed on every read.
5757
Projection arrays are built once per operation rather than once per hydrated
58-
reservation. Lua also omits snapshot JSON when invoked without an
59-
idempotency key.
58+
reservation, and all projection replies are zipped to field names by a shared
59+
`HashProjections` helper (name-keyed reads, no positional indexing). Lua also
60+
omits snapshot JSON when invoked without an idempotency key.
6061

6162
### Compatibility
6263

cycles-protocol-service/cycles-protocol-service-data/src/main/java/io/runcycles/protocol/data/repository/RedisReservationRepository.java

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import io.runcycles.protocol.data.service.EvidenceEmitter;
1010
import io.runcycles.protocol.data.service.LuaScriptRegistry;
1111
import io.runcycles.protocol.data.service.ScopeDerivationService;
12+
import io.runcycles.protocol.data.util.HashProjections;
1213
import io.runcycles.protocol.data.util.LogSanitizer;
1314
import io.runcycles.protocol.model.*;
1415
import io.runcycles.protocol.model.audit.AuditLogEntry;
@@ -1507,7 +1508,7 @@ public ReservationExtendResponse extendReservation(String reservationId, Reserva
15071508
public ReservationDetail getReservationById(String reservationId) {
15081509
try (Jedis jedis = jedisPool.getResource()) {
15091510
String key = "reservation:res_" + reservationId;
1510-
Map<String, String> fields = mapHashFields(
1511+
Map<String, String> fields = HashProjections.mapHashFields(
15111512
RESERVATION_DETAIL_FIELDS, jedis.hmget(key, RESERVATION_DETAIL_FIELD_ARRAY));
15121513
if (fields == null || fields.isEmpty()) {
15131514
throw CyclesProtocolException.notFound(reservationId);
@@ -1624,7 +1625,7 @@ public ReservationListResponse listReservations(String tenant, String idempotenc
16241625
for (int i = startIndex; i < keys.size(); i++) {
16251626
String key = keys.get(i);
16261627
try {
1627-
Map<String, String> fields = mapHashFields(projection, responses.get(key).get());
1628+
Map<String, String> fields = HashProjections.mapHashFields(projection, responses.get(key).get());
16281629
if (fields.isEmpty()) continue;
16291630
if (!tenant.equals(fields.get("tenant"))) continue;
16301631
if (status != null && !status.equals(fields.get("state"))) continue;
@@ -1735,7 +1736,7 @@ private ReservationListResponse listReservationsSorted(
17351736

17361737
for (String key : keys) {
17371738
try {
1738-
Map<String, String> fields = mapHashFields(projection, responses.get(key).get());
1739+
Map<String, String> fields = HashProjections.mapHashFields(projection, responses.get(key).get());
17391740
if (fields.isEmpty()) continue;
17401741
ReservationSummary summary = matchingReservation(fields, tenant,
17411742
idempotencyKey, status, workspaceSegment, appSegment,
@@ -2468,21 +2469,6 @@ private static List<String> reservationProjection(Set<ReservationInclude> includ
24682469
return fields;
24692470
}
24702471

2471-
private static Map<String, String> mapHashFields(List<String> names,
2472-
List<String> values) {
2473-
if (values == null) {
2474-
return Collections.emptyMap();
2475-
}
2476-
Map<String, String> fields = new HashMap<>();
2477-
for (int i = 0; i < names.size() && i < values.size(); i++) {
2478-
String value = values.get(i);
2479-
if (value != null) {
2480-
fields.put(names.get(i), value);
2481-
}
2482-
}
2483-
return fields;
2484-
}
2485-
24862472
private ReservationDetail buildReservationSummary(Map<String, String> fields) throws Exception {
24872473
String estimateUnitStr = fields.get("estimate_unit");
24882474
String estimateAmountStr = fields.get("estimate_amount");

cycles-protocol-service/cycles-protocol-service-data/src/main/java/io/runcycles/protocol/data/service/ReservationExpiryService.java

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import com.fasterxml.jackson.databind.JsonNode;
44
import com.fasterxml.jackson.databind.ObjectMapper;
55
import io.runcycles.protocol.data.metrics.CyclesMetrics;
6+
import io.runcycles.protocol.data.util.HashProjections;
67
import io.runcycles.protocol.data.util.LogSanitizer;
78
import io.runcycles.protocol.data.util.TraceContext;
89
import io.runcycles.protocol.data.util.TraceIdGenerator;
@@ -18,6 +19,7 @@
1819

1920
import java.time.Instant;
2021
import java.util.List;
22+
import java.util.Map;
2123

2224
/**
2325
* Cycles Protocol v0.1.25 - Background job that marks expired reservations.
@@ -42,6 +44,11 @@ public class ReservationExpiryService {
4244
/** Max candidates per sweep to avoid OOM after prolonged outages. */
4345
private static final int SWEEP_BATCH_SIZE = 1000;
4446

47+
/** HMGET projection for the reservation.expired event payload. */
48+
private static final List<String> EXPIRED_EVENT_FIELDS = List.of(
49+
"tenant", "scope_path", "estimate_unit", "estimate_amount",
50+
"created_at", "expires_at", "extension_count");
51+
4552
@Scheduled(fixedDelayString = "${cycles.expiry.interval-ms:5000}",
4653
initialDelayString = "${cycles.expiry.initial-delay-ms:5000}")
4754
public void expireReservations() {
@@ -110,24 +117,24 @@ private void emitExpiredEvent(Jedis jedis, String reservationId, JsonNode luaRes
110117
// previously wrong (missing prefix) — dormant because HMGET on a non-existent key
111118
// returns null fields rather than throwing, so the method just silently no-op'd.
112119
// Surfaced by the new cycles.reservations.expired counter test in v0.1.25.10.
113-
List<String> fields = jedis.hmget("reservation:res_" + reservationId,
114-
"tenant", "scope_path", "estimate_unit", "estimate_amount",
115-
"created_at", "expires_at", "extension_count");
116-
if (fields == null || fields.isEmpty()) return;
120+
Map<String, String> fields = HashProjections.mapHashFields(EXPIRED_EVENT_FIELDS,
121+
jedis.hmget("reservation:res_" + reservationId,
122+
EXPIRED_EVENT_FIELDS.toArray(String[]::new)));
123+
if (fields.isEmpty()) return;
117124

118-
tenantId = fields.get(0);
125+
tenantId = fields.get("tenant");
119126
if (tenantId == null) return;
120127

121128
// Counter is bumped once per actual EXPIRED transition (SKIP results from
122129
// still-in-grace or already-finalised reservations are filtered out above).
123130
metrics.recordExpired(tenantId);
124131

125-
scopePath = fields.get(1);
126-
String unit = fields.get(2);
127-
Long estimateAmount = parseLong(fields.get(3));
128-
Long createdAtMs = parseLong(fields.get(4));
129-
Long expiresAtMs = parseLong(fields.get(5));
130-
Integer extensionCount = parseInt(fields.get(6));
132+
scopePath = fields.get("scope_path");
133+
String unit = fields.get("estimate_unit");
134+
Long estimateAmount = parseLong(fields.get("estimate_amount"));
135+
Long createdAtMs = parseLong(fields.get("created_at"));
136+
Long expiresAtMs = parseLong(fields.get("expires_at"));
137+
Integer extensionCount = parseInt(fields.get("extension_count"));
131138
Integer ttlMs = (createdAtMs != null && expiresAtMs != null)
132139
? (int) (expiresAtMs - createdAtMs) : null;
133140

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
package io.runcycles.protocol.data.util;
2+
3+
import java.util.Collections;
4+
import java.util.HashMap;
5+
import java.util.List;
6+
import java.util.Map;
7+
8+
/**
9+
* Zips an {@code HMGET} projection (the requested field names) with its
10+
* positional Redis reply into a name-keyed map, so consumers read
11+
* {@code fields.get("tenant")} instead of {@code values.get(0)} — inserting or
12+
* reordering a projected field can no longer silently shift every later
13+
* column. Shared by the reservation repository's detail/list projections and
14+
* the expiry event hydration.
15+
*
16+
* <p>{@code HMGET} returns one entry per requested name with {@code null} for
17+
* absent fields; nulls are dropped, so a missing key (all-null reply) yields
18+
* an empty map, which callers already treat as "hash not found".
19+
*/
20+
public final class HashProjections {
21+
22+
private HashProjections() {
23+
}
24+
25+
/**
26+
* @param names the field names passed to {@code HMGET}, in call order
27+
* @param values the positional {@code HMGET} reply ({@code null} tolerated)
28+
* @return name→value for every non-null reply entry; empty when
29+
* {@code values} is null or every value is null
30+
*/
31+
public static Map<String, String> mapHashFields(List<String> names, List<String> values) {
32+
if (values == null) {
33+
return Collections.emptyMap();
34+
}
35+
Map<String, String> fields = new HashMap<>();
36+
for (int i = 0; i < names.size() && i < values.size(); i++) {
37+
String value = values.get(i);
38+
if (value != null) {
39+
fields.put(names.get(i), value);
40+
}
41+
}
42+
return fields;
43+
}
44+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
package io.runcycles.protocol.data.util;
2+
3+
import org.junit.jupiter.api.DisplayName;
4+
import org.junit.jupiter.api.Test;
5+
6+
import java.util.Arrays;
7+
import java.util.List;
8+
9+
import static org.assertj.core.api.Assertions.assertThat;
10+
11+
@DisplayName("HashProjections")
12+
class HashProjectionsTest {
13+
14+
private static final List<String> NAMES = List.of("tenant", "state", "estimate_amount");
15+
16+
@Test
17+
void zipsNamesToValuesInCallOrder() {
18+
assertThat(HashProjections.mapHashFields(NAMES, List.of("acme", "ACTIVE", "5000")))
19+
.containsOnly(
20+
java.util.Map.entry("tenant", "acme"),
21+
java.util.Map.entry("state", "ACTIVE"),
22+
java.util.Map.entry("estimate_amount", "5000"));
23+
}
24+
25+
@Test
26+
@DisplayName("null HMGET values (absent hash fields) are dropped, not stored as nulls")
27+
void dropsNullValues() {
28+
assertThat(HashProjections.mapHashFields(NAMES, Arrays.asList("acme", null, null)))
29+
.containsOnly(java.util.Map.entry("tenant", "acme"));
30+
}
31+
32+
@Test
33+
@DisplayName("missing key (all-null reply) yields an empty map — callers treat as not-found")
34+
void allNullReplyYieldsEmptyMap() {
35+
assertThat(HashProjections.mapHashFields(NAMES, Arrays.asList(null, null, null))).isEmpty();
36+
}
37+
38+
@Test
39+
void nullReplyYieldsEmptyMap() {
40+
assertThat(HashProjections.mapHashFields(NAMES, null)).isEmpty();
41+
}
42+
43+
@Test
44+
@DisplayName("length mismatch is bounded by the shorter list (defensive against driver quirks)")
45+
void lengthMismatchIsBounded() {
46+
assertThat(HashProjections.mapHashFields(NAMES, List.of("acme")))
47+
.containsOnly(java.util.Map.entry("tenant", "acme"));
48+
assertThat(HashProjections.mapHashFields(List.of("tenant"), List.of("acme", "extra")))
49+
.containsOnly(java.util.Map.entry("tenant", "acme"));
50+
}
51+
}

0 commit comments

Comments
 (0)