feat(nameservice): add NameServiceLookup::heads for cheap head reads - #1668
Conversation
Add LedgerHeads { commit, index } and a heads(ledger_id) method on
NameServiceLookup, default-implemented over two get_ref calls, with
single-round-trip overrides:
- DynamoDB: one consistent projected Query over the head..=index sort-key
range (vs a GetItem per ref or the full 4-item lookup query)
- File / StorageNameService: main + index file via a shared merge_heads,
skipping config/context/status parsing
- Memory / Raft: one lock; Raft tombstones retracted branches like get_ref
- NotifyingNameService, CompositeNameService, Arc<T>, NameServiceMode forward
ProxyNameService::get_ref was an Err stub; it and heads now project lookup().
LedgerHeads converts to NsRecordSnapshot so rollback capture can use it.
aaj3f
left a comment
There was a problem hiding this comment.
This is carefully built and makes sense, @bplatz. The three-read-depth surface is a clean generalization of the existing RefLookup/NameServiceLookup split rather than a new orthogonal construct, and the per-backend overrides each exploit their backend honestly (I ran the LocalStack test against real DynamoDB and the single projected Query agrees with lookup() and get_ref(); the raft override is line-for-line parity with its get_ref; merge_heads matches load_record's >= rule and graph-source guard on both file backends).
The one thing I'd spotlight before this gains callers is the proxy projection's treatment of retracted branches. The upgraded get_ref/heads happily resolve head refs for branches a raft-backed remote would tombstone, which is exactly the data-plane scenario raft's own comment warns about; it's a one-line filter (or a one-sentence doc) either way.
The other notes are all optional, though if you agree I'd push for folding them in now rather than filling up the issue backlog: unit tests for the untested proxy projections, an equal-t case to pin merge_heads (a >=→> mutation currently survives all 160 tests), and the pre-existing file get_ref(IndexHead) unconditional-index-file quirk.
Just keep in mind the CI didn't run on these as it's a stacked PR. I ran CI locally and all seems to pass though.
Adherence to repo commitments:
- Patterns/abstractions: ✔ Extends
NameServiceLookupwith a default-implemented method; every one of the 10 production implementors overrides or forwards; no parallel construct. - Performance (speed first, memory second): ✔ No query-engine path touched; the new read is strictly cheaper on every backend (1 consistent Query vs 4-item lookup on DynamoDB; skips config/context/status parsing on file/S3; one lock on raft/memory).
- Testing:
⚠️ Good coverage for trait default, file, storage, memory, raft, and LocalStack DynamoDB (all confirmed running by name locally) — but the proxy projections ship untested, themerge_headsequal-t boundary is mutation-unpinned, and none of it has run in CI yet because the stacked base gets no CI. - Conventions: ✔ Multi-line commit body, self-describing
feat(nameservice):title, fmt/clippy clean,docs/concepts/ledgers-and-nameservice.mdupdated with the read-depth table.
Verified locally at branch HEAD (5694eb1a5): fluree-db-nameservice 160/160 · fluree-db-nameservice-sync 81/81 · fluree-db-storage-aws 30/30 · raft heads test green under --features raft · LocalStack nameservice_ref_publisher green against Docker · fmt + clippy (-D warnings) clean · merge_heads mutation check performed and reverted.
Approving so you can merge when ready — but let's settle the proxy-retraction question first
| _kind: fluree_db_nameservice::RefKind, | ||
| ledger_id: &str, | ||
| kind: fluree_db_nameservice::RefKind, | ||
| ) -> Result<Option<fluree_db_nameservice::RefValue>> { |
There was a problem hiding this comment.
Question — and the one thing I'd like settled before callers appear.
Neither the upgraded get_ref nor the new heads filters retracted, and the server endpoint they project (storage_proxy.rs:get_ns_record, which calls lookup()) serves retracted records with the flag intact — I confirmed NsRecordResponse carries retracted on both sides of the wire. Raft's get_ref deliberately tombstones retracted branches, with a comment that says exactly why: "Without this pairing the data-plane query path would happily resolve a head ref for a branch the operator soft-deleted." So a peer reading through the proxy from a raft-backed transaction server now resolves head refs raft itself would report as gone.
I can see the counterargument — file/dynamo get_ref don't tombstone either, so the proxy is arguably just matching the majority — but the proxy's remote is the one backend that does, and before this PR the stub failed loudly rather than silently resurrecting soft-deleted branches.
Minimally this is a one-liner in both projections (.filter(|r| !r.retracted) on the lookup result, or map retracted → Ok(None)); if you'd rather keep the lookup-faithful behavior on purpose, a sentence in the method docs saying the proxy does NOT tombstone (unlike raft) would do it. Since nothing calls these paths yet I'm not treating it as a shipped regression, but it's much cheaper to decide now than after a data-plane caller bakes in the current semantics.
There was a problem hiding this comment.
Tombstoned — both projections now return None for a retracted branch, and lookup stays faithful so the flag is still visible to admin tooling.
Your framing was right, and checking the counterargument made it stronger rather than weaker: raft is the only backend that tombstones at all. File, DynamoDB and memory get_ref all resolve head refs for retracted branches. So there was no majority contract to match — the proxy was agreeing with three backends that have no explicit position and diverging from the one that documented its reasoning.
A product report that landed while this was open settled the direction: an app on the DynamoDB nameservice deleting ledgers and recreating them under the same name. That turned out to be a different retraction defect on the write side — soft drop reserves the alias permanently and there is no un-retract path anywhere in the tree, so init's attribute_not_exists refuses the recreate. Reproduced on both the file backend and real DynamoDB.
Both that and the cross-backend read inconsistency are in #1670. I deliberately did not touch file/DynamoDB/memory get_ref here — that is a behavior change to shipped surfaces with live callers, and it wants deciding once, on purpose, rather than riding along on a PR about read depth.
| } | ||
| } | ||
|
|
||
| async fn heads(&self, ledger_id: &str) -> Result<Option<fluree_db_nameservice::LedgerHeads>> { |
There was a problem hiding this comment.
Optional. The projection change and new heads ship with no tests, while the file right next to them has a nice pattern for exactly this (test_ns_record_conversion builds an NsRecordResponse and asserts the conversion). A couple of unit tests asserting get_ref/heads project the record correctly — including a retracted: true record once the tombstone question is decided — would pin whichever semantics you pick.
There was a problem hiding this comment.
Added, and the projections are now pure project_ref / project_heads functions so the retraction rule lives in one place per read and is testable without standing up an HTTP mock.
Three tests: carried heads, the retracted tombstone, and unknown-vs-unborn. The first asserts heads equals the two single-ref reads rather than just checking its fields, so the two projections cannot drift apart; the last exists because None and a zeroed RefValue mean different things on this surface and nothing was pinning the difference.
| }; | ||
| if let Some(f) = index_file { | ||
| if f.index.t >= index.t { | ||
| index = RefValue { |
There was a problem hiding this comment.
Optional, found by a mutation check. Flipping merge_heads's f.index.t >= index.t to > leaves all 160 fluree-db-nameservice tests green, so the equal-t boundary of the "same read-time merge rule as load_record" claim is unpinned.
The divergence window is admittedly narrow (a separate index file and an inline main.index at the same t with different cids), but since the whole point of merge_heads is byte-identical agreement with load_record, a small unit test constructing that equal-t case and asserting the index-file cid wins (in both merge_heads and load_record) would regression-proof the shared rule.
There was a problem hiding this comment.
Reproduced your mutation before fixing it — >= to > does pass all 160.
Pinned on both sides, since a pure test alone would not catch a drift in load_record: a unit test in ns_format.rs asserting the separate index file wins at equal t, plus a file-backend test asserting heads equals LedgerHeads::from_record(lookup()) at that boundary. The second one has to write both ns files directly — publish_index only ever writes the separate file, so the equal-t divergence is not reachable through the public API.
Added a third case covering the other two directions (stale separate file, ahead separate file, and neither present), since the mutation showed how little of that rule was actually held down.
|
|
||
| /// Head pointers only: same files and merge rule as `load_record`, minus | ||
| /// the config/context/status fields. | ||
| async fn load_heads(&self, ledger_name: &str, branch: &str) -> Result<Option<LedgerHeads>> { |
There was a problem hiding this comment.
Pre-existing, not introduced by this PR. File get_ref(RefKind::IndexHead) (file.rs:1282-1297) prefers the separate index file unconditionally, while load_record (and now merge_heads here) apply the >= merge rule — so in the stale-index-file edge (separate file t lower than main's inline index t, e.g. leftover state), get_ref and heads/lookup can disagree on the same backend.
Your heads sides with lookup, which seems like the right side to be on, but it does mean the doc line "None follows get_ref" is about existence only, not values.
(Commenting here because file.rs:1282 is not in this diff.)
There was a problem hiding this comment.
Left as-is, but you are right that it makes my doc line wrong, so the docs changed.
heads now states that the get_ref correspondence is about existence, not values; names the divergence (file get_ref(IndexHead) prefers the separate index file unconditionally, where heads and lookup apply the read-time >= merge, so a stale index file makes them disagree); and says explicitly that heads sides with lookup.
Fixing get_ref itself is a value change on a shipped surface, so it belongs with the other retraction/merge inconsistencies in #1670 rather than here.
| .client | ||
| .query() | ||
| .table_name(&self.table_name) | ||
| .key_condition_expression("#pk = :pk AND #sk BETWEEN :head AND :index") |
There was a problem hiding this comment.
Praise. The range read is well-guarded: I enumerated the table's full sk vocabulary (commit#<padded> < config < head < index < meta < status), and exactly the two target items fall in BETWEEN 'head' AND 'index'; even if a future sk lands lexically between them, find_item_by_sk exact-matches so the result can't be corrupted — worst case is wasted RCU. A one-line comment in schema.rs warning future sk additions that heads() range-scans head..=index would make that invariant self-documenting, but that's a nit.
There was a problem hiding this comment.
Added the note in schema.rs.
I kept your framing that the worst case is wasted RCU rather than a wrong answer — the comment says find_item_by_sk matches exactly so the result cannot be corrupted, and asks for new keys outside the range only "where practical". Otherwise the next person reads it as a correctness constraint and designs around it harder than the invariant deserves.
Thanks for enumerating the full sk vocabulary to check the range — that is the part I had not verified myself.
| Ok(record_from_state(&state, &name, &branch)) | ||
| } | ||
|
|
||
| /// One lock, two field reads. Mirrors `get_ref`: retracted branches are |
There was a problem hiding this comment.
Praise. The raft override mirrors get_ref exactly — existence check, tombstone, unborn RefValue construction — and the doc comment states the lookup-vs-active-read split plainly. This is the parity the other backends should aspire to (see the proxy question on proxy_nameservice.rs).
…ad surface The proxy projects the remote's lookup endpoint, which serves retracted records with the flag intact, so get_ref and heads resolved head refs for branches a raft-backed origin reports as gone — the data-plane resurrection raft's own get_ref comment warns about. Both projections now report None for a retracted branch; lookup stays faithful so admin tooling still sees the flag. Retraction remains inconsistent across backends (raft tombstones on this surface, file/DynamoDB/memory do not); that is tracked in #1670 rather than changed here, since altering shipped get_ref semantics is a behavior change with its own oracle. The projections move into pure project_ref / project_heads functions so the rule lives in one place per read and is directly testable: three tests cover carried heads (with heads agreeing with the two single-ref reads), the retracted tombstone, and unknown-vs-unborn. Also pins the merge_heads equal-t boundary, which nothing covered — flipping its >= to > passed all 160 tests. Unit tests assert the separate index file wins at equal t, plus a file-backend test asserting heads and lookup agree there, since the claim merge_heads makes is byte-identical agreement with load_record. Documents that the heads/get_ref correspondence is about existence, not values: file get_ref(IndexHead) prefers the separate index file unconditionally while heads and lookup apply the >= merge, so a stale index file makes them disagree; heads sides with lookup. And warns in the DynamoDB schema that heads range-scans head..=index, so future sort keys landing in that range cost RCU on every head read.
|
All five folded in, @aaj3f — merged Proxy retraction — settled by tombstoningBoth projections now report Worth recording what I found while checking the counterargument, because it is worse than "the proxy is matching the majority": raft is the only backend that tombstones at all. File, DynamoDB and memory I did not change the other three here. That is a behavior change to shipped surfaces with live callers and it deserves its own oracle, so it is filed as #1670 along with a second, distinct retraction defect on the write side (soft drop reserves the alias permanently, with no un-retract path anywhere in the tree, so delete-then-recreate fails on Proxy testsThe projections are now pure
|
Summary
External callers (e.g. a Lambda query backend over DynamoDB, or a standalone deployment over the file nameservice) want the cheapest possible "where is this ledger" read without depending on which backend is configured.
lookup()returns the fullNsRecord— on DynamoDB that is a 4-item query plus record assembly; on file/S3 it is main + index file plus config/context/status parsing.This adds a middle read depth on
NameServiceLookup, so every backend now offers three levels behind the same&dyn NameServiceLookup:get_ref(id, RefKind)RefValue { id, t }heads(id)LedgerHeads { commit, index }lookup(id)NsRecordChanges
LedgerHeads { commit: RefValue, index: RefValue }+NameServiceLookup::heads(ledger_id), default-implemented over twoget_refcalls.From<&LedgerHeads> for NsRecordSnapshotso rollback capture can use it.Queryoversk BETWEEN head AND index, projected to the four ref attributes (1 RCU vs two GetItems or the 4-item lookup query).ns_format::merge_heads(same read-time merge rule asload_record, graph-source guard preserved).get_refdoes.NotifyingNameService,CompositeNameService,Arc<T>, and api'sNameServiceMode.ProxyNameService::get_refwas anErr("not supported")stub; it andheadsnow projectlookup()(still one HTTP round trip), so the proxy backend honors the full read surface.docs/concepts/ledgers-and-nameservice.mdgains the read-depth table.Semantics
Nonefromheadsfollowsget_refon the same backend: unknown ledger, and on Raft also retracted. Onlylookupsurfacesretracted: true. The two refs are read together but not atomically on file backends, soindex.t > commit.tremains a transient state callers must tolerate — same asNsRecord.Testing
None; merge agrees withlookupandget_ref), StorageNameService, Raft.heads()assertions added to the LocalStack testnameservice_ref_publisherinfluree-db-connection/tests/aws_testcontainers_test.rs(requires Docker;--features aws-testcontainers).cargo clippy --all --all-features --all-targets -- -D warningsclean.