fullhistory: hardening + contract notes from the pre-existing-code review (closes #840)#847
Conversation
chowbao
force-pushed
the
chore/fullhistory-hardening-840
branch
from
13cc252 to
f1873e1
Compare
|
Edit: Done |
chowbao
force-pushed
the
chore/fullhistory-hardening-840
branch
from
f1873e1 to
8a10b76
Compare
…view (#840) Storage roots: pairwise distinct-and-non-nested validation (Paths.ValidateRoots) before LockRoots; correct config_lock.go's fail-fast claim (exact-path only, nested roots not detected). Snapshot-distribution guards, all at load time: offsets total vs trailer item count; non-empty index.pack/index.hash bound to each other (MPHF keys == pack records) and to the chunk's emptiness; Trailer().Format verified for both eventstore packs. Close now drains the raw MPHF wait, so eventless-chunk teardown does no events.pack metadata I/O — validation lives on the lookup path. Point fixes: InsertEvents' deferred close-error join reaches the caller (named return); the warmup tamper tripwire no longer wraps at eventID==MaxUint32 (uint64 bound); TermsForBytes hard-fails a future ContractEvent body version to match the SQLite backend instead of silently indexing contractID-only; the MaxEvents-before-postFilter paging contract is documented for the #772 wiring; FetchRange's doc matches the count==0 no-op both implementations have.
chowbao
force-pushed
the
chore/fullhistory-hardening-840
branch
from
8a10b76 to
604a021
Compare
|
@codex[agent] review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 604a021492
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
isAncestorDir built the ancestor prefix as outer+separator, which for outer "/" produces "//" and matches nothing — ledgers = "/" plus any sibling root passed ValidateRoots despite the outer tree containing every other store. Only append the separator when the cleaned path does not already end in one (true only for the filesystem root).
| // holds an exclusive OS lock on a LOCK file under EVERY independently configurable | ||
| // storage root — the catalog, each immutable artifact tree, AND the | ||
| // hot-storage tree. A second daemon touching any shared root fails fast. | ||
| // hot-storage tree. A second daemon pointing at the SAME root path fails fast. |
There was a problem hiding this comment.
Do we need the per-root flocks? RocksDB already takes an exclusive lock when catalog.Open opens the catalog read-write, so a second daemon on the same catalog already fails to start on its own. The flocks only add coverage for the cold artifact trees, and those are write-once: two daemons freezing the same chunk write identical bytes and the atomic rename resolves it, so a shared cold tree is redundant work, not corruption. Suggest dropping them and leaning on the catalog lock.
Same for ValidateRoots: the prune is per-chunk (eligiblePruneOps deletes {root}/{chunkID}/, never a whole root), so a nested root is harmless. The only config that loses data is two roots at the same path, so a one-line distinct-roots check is enough and isAncestorDir/rootNames/the nesting cases can go.
There was a problem hiding this comment.
Done in 7cd5416. Verified both premises before cutting: the catalog is RocksDB (catalog.go:42) so a second daemon fails at catalog.Open on its exclusive LOCK, and every whole-directory deletion is per-chunk (BeginHotCreate/DiscardHotChunk remove HotChunkPath(chunkID) only) — nothing ever removes a root. Flocks are gone; LockRoots' load-bearing other half (create missing roots + fsync the new dir chain, the fresh-deploy crash-safety step) survives as PrepareRoots, and RootsToLock is renamed Roots. ValidateRoots is now the one-line distinct-roots check (still on resolved absolute paths so unclean-spelling duplicates are caught); nesting cases, isAncestorDir, and rootNames are deleted, and the daemon contention test now pins the RocksDB-LOCK failure instead of ErrRootLocked.
There was a problem hiding this comment.
PrepareRoots seems good for creating new dirs/roots on startup
| ch <- mphfResult{idx: m, err: err} | ||
| }() | ||
| c.waitMPHF = sync.OnceValues(func() (*mphf, error) { | ||
| c.waitMPHFRaw = sync.OnceValues(func() (*mphf, error) { |
There was a problem hiding this comment.
The raw/validated split works, but the leak is avoided by convention: waitMPHF returns (nil, err) on a validation failure, so Close has to remember to use waitMPHFRaw or it would skip m.Close() and leak the handle. Cleaner to separate the handle from the verdict: one load accessor (waitMPHF, no validation) plus an error-only gate (validateMPHF). Then Close always gets the real handle and a nil can't reach it.
There was a problem hiding this comment.
Done in 14603d1: waitMPHF is now the validation-free load accessor and validateMPHF is a sync.OnceValue error-only gate; Close always gets the real handle, and LookupKeys runs the gate before using it.
waitMPHF returned (nil, err) on a cross-check failure, so Close had to remember to drain the raw variant or it would leak the handle. Split instead into one validation-free load accessor (waitMPHF) and an error-only gate (validateMPHF): Close always gets the real handle, and a validation failure can no longer hand it a nil.
catalog.Open already holds RocksDB's exclusive LOCK, so a second daemon on the same catalog fails to start without any help. The extra flocks only covered the cold artifact trees — write-once behind the one-write protocol, where a shared tree means redundant work, not corruption — and each hot chunk DB carries its own RocksDB LOCK. LockRoots' other job survives as PrepareRoots: create missing roots and fsync the new directory chain, so a fresh-deploy crash cannot lose a tree the synced catalog already references. RootsToLock is renamed Roots to match. ValidateRoots shrinks to a distinct-roots check: prune and discard delete per-chunk paths only, never a whole root, so nested roots lose nothing and the nesting detection (isAncestorDir, rootNames) goes.
tamirms
left a comment
tamirms
left a comment
There was a problem hiding this comment.
LGTM. Verified locally: the tree builds, go vet is clean, and the config, eventstore, daemon, and full e2e suites pass, including the new second-daemon test confirming the RocksDB catalog lock preserves single-process enforcement. One minor nit inline.
| if err != nil { | ||
| return fmt.Errorf("resolve storage root %q: %w", root, err) | ||
| } | ||
| if _, dup := seen[abs]; dup { |
There was a problem hiding this comment.
PrepareRoots's seen dedup is redundant: ValidateRoots runs first in runDaemonWith and already rejects any two roots resolving to the same path, so this loop never sees a duplicate. MkdirAll is idempotent anyway, so a repeat would be harmless. Could drop it, minor.
There was a problem hiding this comment.
Dropped in 4652e37 — the loop is now a plain range with the empty-root skip; the test's repeated-root case stays as a harmless-no-op check.
ValidateRoots runs first and rejects duplicate roots, and MkdirAll + FsyncNewDirs are idempotent anyway — the seen map guarded nothing.
2 participants
Closes #840. Hardening from the pre-existing-code review: cheap load-time guards, corrected doc claims, and contracts written down before #772 builds on them. No schema or on-disk format changes.
Storage roots
catalog.Open's own exclusive RocksDB LOCK — a second daemon on the same catalog fails to start. Cold artifact trees are write-once behind the one-write protocol (a shared tree is redundant work, not corruption) and each hot chunk DB carries its own RocksDB LOCK.LockRoots' load-bearing other half survives asPrepareRoots: create missing roots and fsync the new directory chain, so a fresh-deploy crash cannot lose a tree the synced catalog already references.RootsToLock→Roots.Paths.ValidateRoots(): distinct roots only — two roots at the same path would write into each other's trees (resolved viafilepath.Abs, so unclean-spelling duplicates are caught). Nesting is deliberately allowed: prune and discard delete per-chunk paths, never a whole root.Snapshot-distribution guards (cold trees copied between machines)
One load-time cross-check each, mirroring the chunk-ID binding events.pack already had:
loadMetacross-checksoffsets.TotalEvents()against the trailer item count — a mispaired offsets blob otherwise silently truncates tail events off every per-ledger range.Trailer().Formatat open —cold_format.goclaimed readers dispatch on it, but only the ledger store checked; a mis-pointed pack now fails at open instead of mid-query with an opaque zstd error.waitMPHFloads,validateMPHFis an error-only gate the lookup path runs —Closealways gets the real handle to release, and an eventless chunk's teardown (the common case across early history) does no events.pack metadata I/O.Contracts and point fixes
Query'sMaxEvents-before-postFiltercontract documented atQueryOptionsand the cap site: a short page does not mean exhaustion, so the Cut over to RPC v2: remove SQLite ingestion/query, serve from v2 stores #772 getEvents wiring must not uselen < limit ⇒ done. Documented rather than a top-up loop — the consumer doesn't exist yet.InsertEvents: named error return so the deferred close-error join reaches the caller (previously assigned a dead local).eventID == MaxUint32: the index upper bound is uint64, so the exact adversarial input the check exists for can't wrapmax+1to 0 and slip past.TermsForBytesnow hard-fails a non-V0 ContractEvent body, matching the SQLite backend, instead of silently indexing contractID-only (topic queries would miss real events with no signal). Flagging for review — the old behavior was defensible as forward-compat; this picks the loud option before a protocol bump picks for us.FetchRange's doc matches both implementations:count == 0is a no-op regardless ofstart.Tests:
ValidateRoots+PrepareRootstables, the five cold-reader guard rejections, the MaxUint32 tripwire row, the body-version hard-fail (raw-XDR discriminant patch — the SDK can't marshal a body the protocol doesn't have yet), and the daemon contention test now pins the catalog's RocksDB-LOCK failure.