Skip to content

fix: critical correctness and durability bugs from deep audit - #225

Merged
adityamukho merged 5 commits into
mainfrom
fix/net-asserted-facts-valid-time
May 2, 2026
Merged

fix: critical correctness and durability bugs from deep audit#225
adityamukho merged 5 commits into
mainfrom
fix/net-asserted-facts-valid-time

Conversation

@adityamukho

@adityamukho adityamukho commented May 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Fixes 5 critical/high severity bugs identified during a deep spec-vs-implementation audit:

  • net_asserted_facts semantic bug — same EAV triple with different valid-time intervals was collapsed to one, causing incorrect query results for bi-temporal queries (e.g. "what was Alice's salary in 2021?" returned empty when re-asserted later with a different valid-time)
  • Parser panic on integer overflow — 99999999999999999999 in Datalog input caused unwrap() panic (DoS vector for LLM-generated inputs)
  • Single fsync at checkpoint end — partial write on crash could corrupt indexes while header remained valid; now syncs after fact pages and after index pages before writing header
  • Index checksum only covered fact pages — bit-flips in B+tree index pages went undetected; checksum now covers all data pages (backwards-compatible fallback for old files)
  • No file-level locking — two processes opening the same .graph file simultaneously could corrupt data; now uses sidecar .graph.lock with PID-based stale-lock detection

Fixes #211
Fixes #218

Test plan

  • 2 new regression tests for valid-time interval preservation (tests/bitemporal_test.rs)
  • 2 new parser overflow tests (src/query/datalog/parser.rs)
  • All 797 existing tests pass (no regressions)
  • WAL crash-recovery tests pass with file locking (stale lock detection handles mem::forget)
  • cargo fmt --check clean
  • cargo clippy clean

🤖 Generated with Claude Code

adityamukho and others added 5 commits May 2, 2026 14:25
@adityamukho @claude
fix(temporal): preserve multiple valid-time intervals for same EAV tr…
4da4eda 
…iple

net_asserted_facts() previously keyed only on (entity, attribute, value),
collapsing multiple assertions of the same EAV with different valid-time
windows into a single fact (the one with highest tx_count). This caused
queries like :valid-at "2021-06-01" to return empty when the same value
was later re-asserted for a different period (e.g., 2024-2026).

The fix changes the algorithm to:
1. Group facts by (entity, attribute, value)
2. Find the highest-tx_count retraction per group (if any)
3. Keep all assertions with tx_count > that retraction
4. Deduplicate by (valid_from, valid_to) window, keeping highest tx_count

This preserves retraction semantics (retraction cancels all prior assertions)
while allowing multiple non-overlapping valid-time intervals to coexist.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@adityamukho @claude
fix(parser): return error on integer overflow instead of panicking
c6cd895 
The tokenizer used .parse::<i64>().unwrap() and .parse::<f64>().unwrap()
on number literals, causing a panic on inputs like "99999999999999999999".
This is a DoS vector for any code path accepting user-provided Datalog
(especially relevant for AI agent use cases with LLM-generated input).

Replace all four unwrap() sites with .map_err() that returns a
descriptive parse error ("Integer/Float literal out of range: ...").

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@adityamukho @claude
fix(storage): add intermediate fsync during checkpoint to prevent cor…
b81581d 
…ruption

The checkpoint previously performed a single backend.sync() call after
writing fact pages, index pages, AND the header. If a crash occurred
between writing index pages and the final sync, the OS could have flushed
partial index data to disk while the old header still referenced old root
pages — pages now overwritten with new (potentially incomplete) index data.

Add two intermediate sync() calls:
1. After fact pages are written (before index build)
2. After index pages are written (before header write)

This ensures the header write is truly the atomic commit point: all data
referenced by the new header is already durable when the header lands.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@adityamukho @claude
fix(storage): extend index_checksum to cover B+tree index pages
ef696d1 
The index_checksum field previously only checksummed fact data pages,
leaving B+tree index pages unprotected. A bit-flip or partial write
in an index page would go undetected on reopen, causing silent query
corruption.

Now the checksum covers ALL data pages (facts + indexes). On load,
a backwards-compatible fallback tries the old fact-only checksum for
files created before this fix, avoiding spurious rebuilds on upgrade.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@adityamukho @claude
fix(storage): add advisory file locking to prevent multi-process corr…
5d74a8e 
…uption

Implements sidecar .graph.lock file with PID-based stale lock detection.
On open, acquires an exclusive lock; on drop, releases it. If the holder
process is dead (checked via /proc on Linux), the stale lock is removed
automatically. Same-process re-acquisition is allowed for leaked handles
(e.g. mem::forget in crash simulation).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@codecov

codecov Bot commented May 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.64706% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/storage/backend/file.rs 66.66% 10 Missing ⚠️
src/storage/persistent_facts.rs 73.07% 7 Missing ⚠️
src/graph/storage.rs 88.23% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@adityamukho
adityamukho merged commit a51abf7 into main May 2, 2026
36 checks passed
@adityamukho
adityamukho deleted the fix/net-asserted-facts-valid-time branch May 2, 2026 13:07
This was referenced May 13, 2026
Closed
Merged
adityamukho added a commit that referenced this pull request May 17, 2026
@adityamukho @claude
fix(api): revert QueryResult tuple variants for 1.x compat; add curre…
43c3d30 
…nt_tx_count() (#261)

* docs(roadmap): record v2.0 file-format policy and wave completion status

- Add v2.0 section: file format v7 only, v1–v6 migration code removable at 2.0 cut
- List 2.0 milestone scope: #182, #183, #180, #181
- Add Wave 2, 3, 4 entries to timeline (were missing)
- Update Last Updated line to Wave 4 complete

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(api): revert QueryResult::Transacted/Retracted to tuple variants for 1.x compat

The struct-variant form introduced in #225 broke pattern-match compatibility for
any consumer matching QueryResult::Transacted(id) or Retracted(id). Revert both
variants back to the original tuple form.

Expose tx_count (the :as-of counter) via the new Minigraf::current_tx_count() method
instead — a purely additive API addition that existing code doesn't need to change to
use.

850 tests passing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(ffi): update sub-crate QueryResult pattern matches to tuple variants

minigraf-c, minigraf-node, and minigraf-ffi all had the struct-variant form
that was introduced alongside the breaking change. Updated to tuple variant
and dropped tx_count from JSON output (use Minigraf::current_tx_count() instead).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
@adityamukho adityamukho mentioned this pull request May 17, 2026
Merged
2 of 3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ci: enforce core-path panic/unsafe/dead-code audit checks test: add property tests for bitemporal interval and retraction semantics

1 participant

@adityamukho