test(ledger): a soft-dropped ledger reads as absent from exists() - #1739
test(ledger): a soft-dropped ledger reads as absent from exists()#1739aaj3f wants to merge 2 commits into
Conversation
43d7586 made ledger_exists treat a retracted record as not-found on the query path, matching LedgerState::load: a dropped ledger must not load or serve queries, so it must not report as existing either. The assertion here still encoded the older contract -- exists() as "is there a record" -- and has been red on main since that merge. Updates the assertion and the comment to state the current contract and why the record still exists on a tombstoning nameservice while exists() answers false.
bplatz
left a comment
There was a problem hiding this comment.
Approving — merge this quickly, it's unblocking every open PR's CI (I hit it independently on #1727, #1730 and #1736).
Your reading is the right one, and there's a piece of evidence for it that also corrects 43d758610's own message. That commit says "Backends that hide the record outright (file, DynamoDB, memory) never showed this." The file backend does not hide it — retract flips status to "retracted" and keeps the file (only purge removes it), and lookup returns the retracted record unfiltered. That's exactly why a file-storage test went red.
Which means the bug #1716 fixed — a dropped ledger still loading and serving queries — was live on the default embedded/CLI backend too, not just raft. So the test was pinning the buggy contract on the very backend where the bug was real. The new assertion is also consistent with list_branches, which already filtered retracted before any of this.
One thing worth adding to #1670, since you're right that it's the place for it — the concrete symptom rather than the abstract question. exists and create now contradict each other after a soft drop, on both backends:
| call | answer |
|---|---|
ledger_exists("x:main") |
false |
create_ledger("x:main") |
LedgerAlreadyExists |
- raft:
retract_ledgerinserts intostate.retractedbut doesn't remove the branch fromledger.branches, andcreate_ledgerrejects onbranches.contains(&branch). - file:
initusesstorage.insert(create-if-absent) and the retracted file is still there.
So a user is told the ledger doesn't exist and simultaneously can't create it — reachable from the CLI, not only raft. Live on main today either way, so it's no reason to hold this.
Also: writing this up as the smaller of two readings and naming the call as someone else's was the right way to handle a test that contradicts a deliberate change.
| // tombstoning nameservice so admin tooling can read the retracted | ||
| // flag, but `exists` is a query-path question: a dropped ledger must | ||
| // not load or serve queries, so it must not report as existing | ||
| // either. See 43d758610. |
There was a problem hiding this comment.
Nit: a bare SHA is awkward to chase from a code comment — worth adding "(#1716)" alongside so the next reader gets the discussion, not just the diff.
|
Closing as superseded — Thanks for the review regardless — the file-backend correction ("the file backend does not hide the record; |
main'stestjob has been red since #1716 merged, on a single assertion.43d758610madeledger_existstreat a retracted record as not-found on the query path — matchingLedgerState::load, so that a dropped ledger can neither load nor serve queries.it_ledger_lifecycle::ledger_exists_on_file_storage:219still encoded the older contract, thatexistsanswers "is there a record" rather than "is it live":This flips the assertion and rewrites the comment to say why both halves are true at once: the record genuinely survives on a tombstoning nameservice so admin tooling can read the retracted flag, and
existsanswers false because it's a query-path question. It cites the commit, so the next reader gets the reasoning rather than re-deriving it.Nothing else in the test changes — the create/commit/unknown-name/malformed-id assertions above it are untouched, and they're the reason the test exists.
Worth knowing this is currently inherited by every open PR whose CI has run since 01:35, since CI merges
maininto each head. It's the only failure in those runs.I've written this as the smaller of the two readings — that the test was stale and the change was intended. If the older contract was the right one and
existsshould keep reporting retracted records, then this should be closed and43d758610revisited instead; that's a call for @bplatz rather than me.One related note: this settles a sub-question of #1670, which is still open and carries the wider delete-semantics decision — init-reclaims versus reject, whether an un-retract path exists, and branch-scoped hard purge. Worth recording there that the
existshalf is now decided, so whoever picks up the rest doesn't re-derive it and the two don't drift apart, which is what that issue is about in the first place.