# Implementation Review — Findings, Ranked By Severity

Date: 2026-07-16
Scope: core engine (`engine.rs`, `bfs.rs`, `path_finder.rs`,
`connected_components.rs`, `persistence.rs`, `builder.rs`, `node_store.rs`,
`edge_store.rs`, `safety.rs`, `acl.rs`, `quote.rs`, `config.rs`, SQL facade)
plus the sync findings cross-referenced from `01-sync-architecture-review.md`.
Review only — no code changed.

## Overall Verdict

Nothing is "seriously wrong" with the architecture. The unsafe mmap layer is
genuinely defensive, error/SQLSTATE mapping is disciplined, there are no
`unwrap()`/`expect()`/`panic!` in production hot or unsafe paths, artifact
swaps are atomic, and the PostgreSQL-first write principle (AGENTS.md) is
honored — GQL `CREATE` performs `INSERT INTO … RETURNING` via SPI against the
registered table *before* recording the transaction-local graph delta
(`sql_facade/gql.rs:835-870`, then `tx_delta` at `:347-350`, `:2338`).

The serious findings are about the **security model of reads** and two
**concurrency/correctness seams**, not memory safety.

## HIGH

### H1 — RLS is not enforced on topology reads (confidentiality gap)

`graph.search()` is RLS-safe (re-queries source tables via SPI as the caller,
`sql_search.rs:171-182`). But `graph.traverse()`, `shortest_path()`,
`weighted_shortest_path()`, and `connected_components()` walk the in-memory
CSR and return `node_id` (source PK), depth, paths, and edge labels directly
(`engine.rs:678-777`, `873-953`, `1178-1212`). The only check on that path is
**table-level** `pg_class_aclcheck(ACL_SELECT)` (`acl.rs:52-68`). Row-level
security is never consulted.

The graph artifact is built by whoever runs `graph.build()` — typically an
owner/superuser who bypasses RLS — so it contains every row. A low-privilege
caller holding only table-level SELECT can then enumerate PKs, adjacency,
connectivity, and path existence of rows their RLS policy hides. Hydration
partially masks it (RLS-hidden rows hydrate as `node = NULL`,
`sql_facade/traversal.rs:217-228`) but `node_id` is always emitted, and
in-memory filter predicates (`bfs.rs:583`) let a caller probe hidden rows'
filter-column values.

Options (in increasing effort): document loudly as a boundary ("topology is
builder-scoped; do not grant graph functions to RLS-restricted roles"), add a
guard that refuses topology reads on tables with RLS enabled unless an opt-in
GUC acknowledges the boundary, or build RLS-aware traversal (per-caller
visibility bitmap, expensive). At minimum the first option belongs in
`docs/user_guide/administration-and-security.mdx` and the known-issues
register **now**.

### H2 — Cross-backend durable-ingest race (from sync review)

Durable projection ingest/manifest publication is guarded only by a
process-local `static Mutex<HashSet<PathBuf>>` (`projection/ingest.rs:86-111`)
plus a TOCTOU `final_path.exists()` check (`manifest.rs:225-230`). Two
backends running `graph.apply_sync()` concurrently on the same persisted
mutable graph can publish the same generation id; POSIX rename-replace means
last-writer-wins and one backend's segments are silently dropped. Build and
vacuum already take a per-graph advisory lock (`sql_build.rs:315-330`) —
ingest needs the same. Full detail in `01-sync-architecture-review.md` (W1).

### H3 — Tenant scoping is a filter, not an authorization boundary

For multi-tenant-table graphs (tenant bitmaps, graph not pinned to one
tenant), `resolve_tenant_scope` (`sql_sync.rs:1883-1927`) accepts any
caller-supplied tenant string; `bfs.rs:572-582` just checks bitmap
membership. Any user with table SELECT can read any tenant's subgraph.
Combined with H1, cross-tenant topology is readable. This may be intended
(`graph.enforce_tenant_scope` covers the pinned-graph case) but docs currently
suggest more isolation than exists — must not be marketed as an isolation
guarantee.

## MEDIUM

### M1 — Bidirectional BFS may return a non-minimal "shortest" path

`bidirectional_bfs` breaks at the **first** frontier intersection
(`path_finder.rs:165-172`, `201-208`) instead of finishing the level and
choosing the meeting node minimizing `d_fwd + d_bwd`. Since visited sets span
multiple prior levels, two meeting points found in one expansion can differ in
combined distance — the first found is not guaranteed minimal. Existing tests
only cover single-path chains (`path_finder.rs:516-579`). Likely a real
off-by-one-hop correctness bug on the default bidirectional path. Needs a
targeted regression test with two meeting nodes at different combined depths.

### M2 — Circuit breakers truncate silently

`max_nodes`/`max_frontier` stop traversal and return partial results with no
`truncated` flag (`bfs.rs:304-324`, `416-438`, `640-649`). Callers cannot
distinguish "fully explored" from "capped". Worse: BFS truncation inside
shortest-path search (`path_finder.rs:287-371`) can produce a **false
"no path"** for a reachable target. The result contract needs a
`truncated`/`capped` signal, and path functions should error rather than
answer wrongly when capped.

### M3 — Every traversal deep-clones all tenant state

`traverse_with_filter_ops` clones `tenanted_table_oids` and the entire
`tenant_membership` `HashMap<String, RoaringBitmap>` into `BfsConfig` on every
call, even with no tenant argument (`engine.rs:733-737`). Biggest avoidable
per-query cost in the read path; `BfsConfig` should borrow.

### M4 — Builder interpolates raw catalog table names into dynamic SQL

Hydration resolves names OID-stably via regclass (`catalog.rs:163-167`), but
the builder formats the raw registered name into `SELECT … FROM {}` directly
(`builder.rs:341`, `478-482`). Registration validates via `to_regclass($1)`,
so not classic injection — but the build re-resolves under the *build-time*
`search_path`, so an unqualified registered name can bind to a different
relation than at registration. Use the same catalog-name path as hydration.

### M5 — Artifact + sidecars are not atomically committed as a unit

`write_graph_file_internal` renames `.pggraph` (`persistence.rs:615-620`) and
*then* writes the `.sync` (applied watermark) and `.projection_mode` sidecars
(`:621-622`). A crash in between pairs a new artifact with a stale sync
checkpoint → replay from the wrong watermark on reload. No parent-directory
fsync after the rename either (the projection manifest path does this
correctly — `manifest.rs:233-251`). Fold checkpoint + mode into the artifact
or a single fsynced manifest.

### M6 — Full-file CRC + full content validation on every backend load

`load_graph_file` CRCs the entire mapping (`persistence.rs:773`) and
`validate_persisted_contents` walks every edge/PK offset (`:312-408`) on each
backend's first query — faulting in the whole file and negating mmap laziness.
This is very likely the measured ~4.4 s first-backend-query cost called out in
the roadmap. A per-generation validation cache (validate once per artifact
generation per cluster, not per backend) is the obvious optimization.

## LOW

- **L1 — Endianness**: mmap fast paths read `u32`/`u64` slices in native
  endianness (`edge_store.rs:508-509,717-777`; `node_store.rs:321-445`) while
  the format is little-endian. Silent misread on big-endian hosts (s390x).
  Document LE-only and add a `cfg!(target_endian)` guard.
- **L2 — Frontier pre-allocation scales with the GUC** not the graph
  (`bfs.rs:248,362,510`): `max_frontier` is a `Userset` GUC up to 10M → ~40 MB
  allocation per traversal a user can self-inflict. Clamp to
  `min(max_frontier, node_count)`.
- **L3 — Hand-rolled `errstart`/`errfinish` FFI** for custom `PG0xx` SQLSTATEs
  (`safety.rs:238-281`). Correct today and tested, but bypasses pgrx and rides
  on ereport ABI stability across pg14–18 — a maintenance watch item.
- **L4 — Poison sync rows have no dead-letter path**; `_sync_log.error_message`
  exists but is never written/read (sync review W3).

## What Is Well Done (preserve these)

- Defense-in-depth unsafe layer: pointer/size/bounds/alignment validation
  before every `from_raw_parts` (`node_store.rs:77-151`) — even a bypassed
  layout validator would not cause UB.
- Load-time integrity ordering: CRC → monotonic offsets → target bounds →
  flag domain → UTF-8 PK validation, all before store construction
  (`persistence.rs:312-506`).
- Atomic artifact swap; never in-place mutation; per-graph advisory
  xact locks for build/vacuum (`sql_build.rs:13-38`).
- Panic boundaries on all SQL entry points; overflow-checked arithmetic in
  persistence; edge-type registry capped at 255.
- Sparse traversal metadata mode (`bfs.rs:201-211`) avoiding O(node_count)
  allocations for small-visit queries.
- PostgreSQL-first GQL writes with source-row locking + predicate re-check at
  the write boundary (KI-001 hardening confirmed in code).