Skip to content

Commit 4f02f7a

Browse files
committed
manifest cache: Make branching on the cache state structurally total
Replace per-caller case expressions over get_manifest/1 with with_manifest/2, a fold whose handler map has mandatory keys for all three cache states (resolved, pending, absent). A call site that fails to consider a state no longer compiles a quiet fallback into place; it fails to match at the call site, and when a state is ever added every caller crashes loudly instead of inheriting a neighbor's behavior. This is the structural successor to the no-catch-alls convention: the boot-window read bug existed because each consumer branched on the bare accessor with its own clauses and a catch-all arm absorbed the state it had not considered. get_manifest/1 remains for non-branching uses (diagnostics, tests). get_range/1 and get_manifest_and_epoch/1 keep their documented single-value projections. Migrated: the log reader's four resolution clauses and resolve_first, the retention-counter hook, GC's still_dangling re-check, the remote reader's iterator refresh, and the replica reader's manifest resolution and reconciler re-seed. No behavior change; the full suite set from the pending-state change stays green, including the single-node boot-window regression.
1 parent a581897 commit 4f02f7a

7 files changed

Lines changed: 294 additions & 239 deletions

docs/conventions.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ The chunk header is not a fixed 48 bytes. It includes a bloom filter whose size
2828

2929
Any state derived from the manifest (the per-node manifest cache, the osiris first-offset counters, iterator snapshots) can be missing or not yet established, and "missing" is never the same value as "empty". When branching on such state, write one clause per state and no catch-all: a `_ ->` arm over cache states is how a cache miss gets silently collapsed into "no remote tier", which is the boot-window read bug (see the Cached state section of [invariants.md](./invariants.md)). If a new state is added, every consumer should fail to compile or crash loudly, not inherit an arbitrary neighbor's behavior.
3030

31+
For the manifest cache this is enforced structurally, not by convention: branching goes through `rabbitmq_stream_s3_manifest_replica:with_manifest/2`, whose handler map has mandatory keys for all three states (`resolved`, `pending`, `absent`). A call site that fails to consider a state does not compile a quiet fallback into place; it fails to match. `get_manifest/1` remains only for non-branching uses (diagnostics, tests). New derived-state APIs should follow the same shape: expose a total fold, not a bare accessor that invites per-caller `case` expressions.
32+
3133
The direction of each explicit miss clause matters: consumers must fail in the direction that cannot lose data. Readers fail closed (error, the consumer retries), retention deletes nothing, GC skips, resolution goes to the authoritative store. A fallback that degrades silently reads as robustness in review and is exactly where correctness leaks out.
3234

3335
## Cold-state test dimension

src/rabbitmq_stream_s3_gc.erl

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -286,8 +286,8 @@ still_dangling(#{reason := no_anchor}) ->
286286
%% the absence is permanent, so there is nothing to re-validate.
287287
true;
288288
still_dangling(#{reason := below_first_offset, stream_id := StreamId, key := Key}) ->
289-
case rabbitmq_stream_s3_manifest_replica:get_manifest(StreamId) of
290-
#manifest{first_offset = FirstOffset} = Manifest ->
289+
rabbitmq_stream_s3_manifest_replica:with_manifest(StreamId, #{
290+
resolved => fun(#manifest{first_offset = FirstOffset} = Manifest) ->
291291
case parse_key(Key) of
292292
{data, _StreamId, Offset} ->
293293
Offset < FirstOffset;
@@ -296,14 +296,14 @@ still_dangling(#{reason := below_first_offset, stream_id := StreamId, key := Key
296296
not live_leading_group(StreamId, Key, Manifest);
297297
_ ->
298298
false
299-
end;
300-
Unresolved when Unresolved =:= undefined; Unresolved =:= pending ->
301-
%% No live manifest to compare against (missing row, or a pending
302-
%% marker whose manifest has not resolved yet): do not delete (a
303-
%% later sweep reclaims a genuine orphan once a floor is known
304-
%% again).
305-
false
306-
end.
299+
end
300+
end,
301+
%% No live manifest to compare against (a pending marker whose manifest
302+
%% has not resolved yet, or a missing row): do not delete (a later
303+
%% sweep reclaims a genuine orphan once a floor is known again).
304+
pending => fun() -> false end,
305+
absent => fun() -> false end
306+
}).
307307

308308
%% Whether the group object Key is protected by the LIVE manifest's leading-group
309309
%% carve-out: it is the live referenced leading group, or the live manifest is in

src/rabbitmq_stream_s3_hooks.erl

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -151,20 +151,22 @@ on_retention_evaluated(Cnt, #{name := Name}) ->
151151
%% of -1, which must never reach the counter. get_range/1 returns `empty`
152152
%% in that case, so the override only runs when the remote tier actually
153153
%% holds data and its first_timestamp is a real timestamp.
154-
case rabbitmq_stream_s3_manifest_replica:get_manifest(StreamId) of
155-
#manifest{entries = <<>>} ->
156-
ok;
157-
#manifest{first_offset = RemoteFirst, first_timestamp = RemoteFirstTs} ->
158-
LocalFirst = counters:get(Cnt, ?C_OSIRIS_LOG_FIRST_OFFSET),
159-
counters:put(Cnt, ?C_OSIRIS_LOG_FIRST_OFFSET, min(LocalFirst, RemoteFirst)),
160-
LocalFirstTs = counters:get(Cnt, ?C_OSIRIS_LOG_FIRST_TIMESTAMP),
161-
counters:put(Cnt, ?C_OSIRIS_LOG_FIRST_TIMESTAMP, min(LocalFirstTs, RemoteFirstTs));
162-
pending ->
163-
%% Not yet resolved: nothing known to override the counters with.
164-
ok;
165-
undefined ->
166-
ok
167-
end.
154+
rabbitmq_stream_s3_manifest_replica:with_manifest(StreamId, #{
155+
resolved => fun
156+
(#manifest{entries = <<>>}) ->
157+
ok;
158+
(#manifest{first_offset = RemoteFirst, first_timestamp = RemoteFirstTs}) ->
159+
LocalFirst = counters:get(Cnt, ?C_OSIRIS_LOG_FIRST_OFFSET),
160+
counters:put(Cnt, ?C_OSIRIS_LOG_FIRST_OFFSET, min(LocalFirst, RemoteFirst)),
161+
LocalFirstTs = counters:get(Cnt, ?C_OSIRIS_LOG_FIRST_TIMESTAMP),
162+
counters:put(
163+
Cnt, ?C_OSIRIS_LOG_FIRST_TIMESTAMP, min(LocalFirstTs, RemoteFirstTs)
164+
)
165+
end,
166+
%% Not yet resolved: nothing known to override the counters with.
167+
pending => fun() -> ok end,
168+
absent => fun() -> ok end
169+
}).
168170

169171
-doc """
170172
Discover existing osiris writers and replicas on this node and attach

0 commit comments

Comments
 (0)