feat(changes): commit entity diffs and a durable change feed - #519
Conversation
A manifest-pinned Lance version that cleanup reclaimed previously surfaced as a generic storage string from the shared dataset opener. Historical reads and the upcoming change surfaces need to distinguish retention from arbitrary I/O failure without parsing error text, so the opener now classifies Lance VersionNotFound/DatasetNotFound/NotFound under an exact version pin as the typed HistoricalVersionReclaimed error. The server maps it to an internal error for now; change surfaces will rewrite it into their own typed gap.
Introduce the deliberate SDK types for per-commit entity changes and the change feed: one cause per block, node/edge kind, opaque graph type identity plus name, logical entity id, operation, and exact before/after images with edge endpoints inside each image. Updates carry both images by contract. Physical vocabulary (table keys, incarnations, Lance versions, row addresses) is structurally absent from these types. The module also owns the server-owned page/feed ceilings and the engine mirror of the five reserved Lance virtual column names, pinned by a unit test so exact-name matching replaces prefix heuristics that would hide legal user properties such as _row_custom. The internal net-diff types are untouched and remain unserialized.
Three payload kinds share one checksummed base64url envelope: a commit page token continues one bounded commit diff, a feed cursor is the caller-owned durable position after a complete commit, and a feed page token continues one interrupted poll at its captured cut. Kind and version tags make every cross-use, corruption, or unsupported-version case a typed rejection with a reason that names both sides, and the server maps the typed rejection to a 400 with the stable 'change cursor rejected: ' prefix. The module also owns the opaque graph type-id projection (a hash of the persisted schema identity domain plus immutable table identity: rename-stable, drop/re-add-fresh, numbers never exposed) and the canonical filter digest that binds continuations to their scope, collapsing explicit match-all selections to the same digest as an absent filter.
Derive one graph commit's entity changes against its first parent by an ordered-by-id merge of every changed table lifetime's two pinned endpoints. Logical operation is defined only by the two graph-visible states; physical row lineage never becomes graph identity. Inserts carry the exact child after-image, deletes the exact parent before-image, and updates BOTH exact images — the merge keeps both popped rows and reads each image from its own pinned handle. Row equality is typed and structural: non-Blob user columns compare by Arrow logical equality on one-row slices (null, empty string, and empty list stay distinct — no display-string join), while Blob columns compare payload-free by physical descriptor identity with an exact payload byte-compare only on a descriptor tie, so compaction cannot surface phantom updates and unchanged Blob siblings cost zero payload I/O. Images decode with the commit-era physical schema, never the live catalog, and only the five exact reserved Lance virtual columns are excluded. A strict schema gate runs over every changed interval before anything is emitted: paired lifetimes must share one user schema fingerprint and a non-empty added or removed lifetime is a typed boundary refusal — schema evolution is never synthesized into entity changes. The parentless genesis commit is a typed refusal, reclaimed pinned versions become the typed feed gap, and page continuation is an opaque checksummed token bound to the graph, commit, and filter scope. Emission order is frozen as (nodes first, opaque type identity, id, operation rank). The cost instrument pins bounded opens (at most two per changed interval) and Blob laziness flat, and pins the exact ordered-merge full-table scan term as a growing tripwire: candidate pruning over the Lance row-version columns is the sanctioned follow-up that must flip it to a flat assertion.
One poll captures a coherent branch cut — lineage head, Lance-native branch incarnation witness, genesis, and the commit projection from a single branch-pinned coordinator open — then walks the first-parent chain from the caller's position toward that cut, emitting one block per commit through the shared enumerator. Adjacent edges share their snapshot endpoint, so a poll costs one manifest snapshot resolution per commit examined plus one. The durable cursor advances only over complete commits: a page ending inside a block carries only a page token (a partial block can never be checkpointed), a page ending at a boundary carries the cursor with a caught-up flag, empty blocks for physical-only commits advance the position, and a page token keeps its originally captured cut so commits landing mid-poll stay outside it. Start modes are explicit (now, after an exact on-chain commit, or the beginning including inherited history on a named branch); a missing cursor is never an implicit beginning. The server holds no consumer state: any handle or process resumes from the caller-owned cursor. Cursor scope is validated in a fixed order — graph identity, graph-history genesis, branch name, branch incarnation witness (fencing same-name delete/recreate), and filter digest — each mismatch a distinct typed rejection. An unreadable first commit surfaces the typed feed gap or schema boundary; a later one ends the page atomically at the previous block boundary so the next poll surfaces the same typed error deterministically. Commits examined per poll are bounded (128 default, 512 ceiling); the cost instrument pins the caught-up poll data-flat and the backlog walk's per-commit terms honestly.
Recovery from a feed gap is one atomic handshake, never a bare head ID a caller must race an export against. Under the export-cut permit — held from before head capture until the last byte, so cleanup, schema apply, branch replacement, and root deletion cannot remove the selected coordinates mid-stream — the engine captures the branch's change cut coherently, streams the data-only entity snapshot pinned AT that head commit, and mints the cursor that resumes the feed immediately after it, bound to the same branch incarnation and filter scope. The snapshot honors the scope's kind and type-name dimensions; operation filters bind only the resume cursor's feed scope, since a snapshot has no operations. A commit landing after the capture is outside the snapshot and is the first block the resumed feed yields; a failed export returns an error, so a usable cursor structurally cannot outlive a broken snapshot.
Add the graph-vocabulary wire contract for commit entity diffs and the change
feed: cause stated once per block (authored_at is the authorship time,
deliberately not labeled a commit time; authored_branch is the branch the
commit originally landed on), entity changes with opaque graph type identity
plus name, logical id, operation, and exact before/after images with edge
endpoints inside each image. The finite diff and the feed carry separate
continuations: a page token continues one bounded response, the durable
cursor appears only on a terminal page together with a caught_up flag. The
baseline handshake speaks a request body plus a terminal
{"baseline": {snapshot_commit_id, resume_cursor}} stream record.
No wire field carries a table key, table or incarnation id, physical or
manifest version, row address, part marker, completeness flag, change index,
or caller byte limit — physical vocabulary is structurally absent from the
contract. ErrorOutput gains two additive rolling-safe details: a 410 feed gap
(cursor plus first unreadable commit) and a 409 diff refusal (parentless
commit or schema boundary); ErrorCode stays closed. Mapping from engine types
lives in one place here, shared by the server and the CLI's embedded arm so
the two transports translate identically.
GET /graphs/{graph_id}/commits/{commit_id}/changes returns one bounded page
of the entity changes a commit made relative to its first parent, in graph
vocabulary with exact before/after images and the cause stated once. Large
commits continue via the opaque page token.
Because the response carries row images, read authorization binds to the
branch the commit landed on — deliberately stricter than commit-show's
branch-free read. The route parses its query with a strict allow-list over
repeatable kind/type/op filters: an unknown parameter is a 400, so a caller
byte limit or physical vocabulary can never silently ride the surface.
Typed failures map onto the transport: rejected continuations are stable
'change cursor rejected: ' 400s, parentless-genesis and schema-boundary
refusals are 409s with the structured refusal detail, reclaimed history is
the server's first 410 with the structured feed-gap detail, and above-ceiling
limits stay 413.
GET /graphs/{graph_id}/changes polls one branch's first-parent change feed:
complete blocks in order, the durable cursor only on a terminal page with a
caught_up flag, and a page token when a bounded page ends inside a block so a
partial block can never be checkpointed. Position selection is explicit —
cursor, start (now | beginning | after:<commit_id>), and page_token are
mutually exclusive, and a missing position means now, never an implicit
replay of all history. Read authorization binds to the polled branch; the
strict query allow-list from the commit-diff route applies unchanged.
POST /graphs/{graph_id}/changes/baseline streams the data-only entity
snapshot pinned at one coherently captured head over the bounded export
transport (reserve before capture, two-chunk backpressure, disconnect
cancellation), then emits exactly one terminal
{"baseline": {snapshot_commit_id, resume_cursor}} record — sent only after
every snapshot record succeeded, so an interrupted stream structurally
carries no usable cursor. A baseline is a full data export and requires the
export action.
The engine side reuses the one move-only export-cut type: the served baseline
capture returns the pre-minted handshake beside a cut pinned at the captured
commit, and the source guard now registers exactly these two cut-capture
surfaces.
omnigraph commit changes <commit_id> lists one commit's entity changes with the cause header stated once; without --page-token it auto-paginates the whole block, with it it fetches exactly one page and prints the next token. omnigraph changes poll follows a branch's feed from --cursor or --start (now | beginning | after:<commit_id>), always consuming page tokens internally and stitching split blocks back together so only the terminal durable cursor is ever printed. omnigraph changes baseline streams the snapshot to --out and prints the handshake; the remote arm holds back the final NDJSON line and refuses a stream that ends without the terminal record, so an interrupted transfer yields no usable cursor. Both arms of every verb share the api-types filter translation, keeping embedded and served behavior identical by construction. The commands are ordinary data-plane verbs in the capability matrix.
Two vocabulary gates make the graph-only wire contract structural: a data-route walk over real commit-diff, feed, and baseline-terminal responses rejects every physical storage key (table keys, incarnations, manifest and table versions, row addresses, part markers, completeness flags, change indexes, byte limits), and a spec-side walk over every change schema plus the change routes' parameters enforces the same set in the OpenAPI contract. The auth matrix pins bearer requirements on all three routes and the policy split: a read-scoped actor polls the feed and diffs commits but cannot capture a baseline, which is a full data export. Parity rows drive commit changes (with internal page-token walking), filtered diffs, a beginning-replay poll, and the baseline handshake through both the embedded and served arms of the CLI — on the twin fixture they must agree exactly, cursors included.
The changes guide gains the three public surfaces: exact per-commit entity diffs (cause once, before/after images, frozen order, empty physical-only blocks, typed genesis and schema-boundary refusals), the durable first-parent change feed (explicit start modes, cut pinning, block-boundary-only cursors, at-least-once consumption, typed scope rejections), and the baseline handshake as the sole reset after a retention gap. The server reference adds the three endpoint rows with their auth actions and the pagination note; the CLI reference adds commit changes and the changes poll/baseline commands; the test map records the new engine, server, and parity owners; and the design document records the shipped contract details (exact-merge-only v1 with its growing cost tripwire, strict schema gate, continuation bindings, feed stop rules, baseline framing, and the manifest-history retention dependency).
The two additive change-surface detail fields pushed ApiError past clippy's 128-byte Err ceiling, flagging every non-async function that returns it. Shrink the type back under the ceiling instead of allowing the lint: the message becomes a boxed str and the merge-conflict list a boxed slice, both converted at the one IntoResponse boundary. Also settle two test-side lints (a redundant mutable binding and a filter/next_back chain) and keep the CLI modules' items ahead of their trailing test modules.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Within one kind, blocks must emit types in lexicographic order of the opaque entity_type.id the caller actually sees — the published continuation key — and a bounded paged walk must resume across type boundaries on that same key. The current implementation orders by an internal numeric identity whose projection into the published id is a hash, so this six-type fixture fails (719/720 of runs; the identity domain is random per graph, so a coincidental match of the two orders is possible but vanishing). Verified failing with the order-mismatch symptom before the fix.
![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: dd5f8c4d42
ℹ️ 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".
…tity Block emission and continuation resume now share one key: the PUBLISHED opaque type identity. Interval plans compute the projection once, sort by (kind rank, opaque id) so multi-type commits emit in the exact lexicographic order a caller can reproduce from the response, and the continuation key carries that same string. The commit and feed page-token payloads drop their numeric stable-table and incarnation fields entirely — a token's decodable JSON never exposes physical identity components, closing the gap between the continuation contract's wording and the payload bytes (the SHA-256 tail is integrity, not encryption). Pre-release wire format; no compatibility shim.
A page-token resume whose first remaining change exceeds the poll's byte budget must surface ResourceLimitExceeded immediately: the resumed poll's budget is fresh, so an empty-page exhaustion already proves the change cannot fit any page of this size. Verified failing before the fix with the predicted symptom — the poll returns Ok with an empty block-boundary page whose cursor is still parked before the split commit, deferring the same typed error to the following poll.
An empty-page exhaustion means the budget is untouched regardless of whether the poll started fresh or resumed a page token, so the single change provably exceeds this poll's own byte ceiling. Return the typed resource limit immediately instead of an empty boundary page that only deferred the same error to the following poll.
A failed 'changes baseline' run must not destroy the previous good snapshot at --out or leave partial-file residue beside it. Verified failing before the fix with the predicted symptom: the destination is truncated to empty because the file is created before capture begins.
Stream the snapshot into '<out>.partial' beside the destination and rename it over --out only after the terminal handshake completes. Rename within one directory is atomic on the same filesystem, so a failed or interrupted capture leaves the previous snapshot untouched and removes its own partial file instead of truncating the destination before capture even starts.
The schema gate proves the two endpoints share one user schema with a name-keyed, order-insensitive fingerprint, but the comparator zipped blob signatures positionally — a physical column reorder within one lifetime would pass the gate and then mark every blob-bearing row a phantom update. Sort the signatures by column name at batch preparation so the zip aligns by name. Defense in depth: no supported writer reorders columns within a lifetime today, so there is no integration reproduction; the sort simply makes the comparator honor the same order-insensitivity the gate does.
The design document's continuation section lists enforced bounds among a page token's bindings; the shipped v1 deliberately binds identity, scope, and position only — row limits stay per-request and byte ceilings server-owned — so replay is position-stable rather than page-size-stable. Record that deviation with its rationale, and note that continuation positions are now keyed by the published opaque type identity so token payloads carry no numeric physical components.
Keying resume and ordering by the published opaque identity left the raw table interval unread on the plan; keep only the fields the enumeration consumes.
|
Review findings from the automated pass have been dispositioned; nine follow-up commits are on the branch: Fixed
Resolved by documentation
|
The RFC-030 §4.4 obligation to prove the ordering path bounded is not discharged. Verified against pinned Lance 10.0.0 source: an ordered-by-id scan plans as a single-partition SortExec with no fetch, backed by an UnboundedMemoryPool with spill structurally disabled (no Scanner knob, no env override), so resident memory is O(table projected width) — embeddings included. Shared by branch merge, the change enumerator, the commit diff, and export; it is the mechanism behind the recorded branch_merge OOM. Sharpen merge-complexity.md from 'high memory/spill risk' to the precise fact, add the open obligation to RFC-030 §14, and name the instance in the invariants resource-bounds gap.
…ow_ props The cross-branch net diff compared rows by a display-string signature that rendered null and "" identically and skipped every _row_-prefixed column. Two regression cells prove a null->"" flip and a change to a legal _row_notes property are both silently dropped (the change set comes back empty). RED against the current comparator; the next commit turns them green.
commit_snapshot re-proves each commit's manifest head, but plan_intervals then opens the per-table datasets SEPARATELY, by (branch path, numeric version) using the warm session. A named branch deleted and recreated at the same path and version AFTER the head proof but before the table open retargeted the open to the replacement branch's rows, which the feed emitted under the captured commit's label — a second ABA window the earlier commit_snapshot fix (first window) did not cover. Add TableStore::open_at_entry_verified: for a named-branch entry it opens cache-bypassing (so a stale warm handle cannot mask the retarget) and requires the opened dataset's manifest e_tag to match the entry's recorded incarnation, failing closed with the same typed refusal otherwise. Main entries cannot ABA and keep the warm path. plan_intervals uses it, guarded by a new CHANGE_FEED_PRE_TABLE_OPEN failpoint. The new failpoint cell isolates a single commit (start after its parent) so only the table open — not a later commit's commit_snapshot — can catch the recreation. Verified RED: with the unverified open the poll emits the replacement branch's "new" row as the feature commit's change; GREEN after. Residual: if version_metadata.e_tag() is ever absent, only the manifest-head proof covers the window; the e_tag path closes it on local FS (synthetic inode/mtime/size e_tag) and object stores.
The engine scope carries only `type_names` and the HTTP `type` parameter is interpreted only as a current type name, but §3.3 read as if an opaque type identity were also a filter dimension. Supplying an emitted opaque type ID therefore matched nothing silently. Amend the RFC to say type filtering is by-name-only and that the opaque identity on emitted changes is a display/join key, not a filter input — closing the over-promise rather than building an ID filter dimension across engine, cursor digest, HTTP, CLI, and baseline (F6).
Human `commit changes`/`changes poll` output was lossy in two ways: an endpoint-moving edge update printed only `after.or(before)` endpoints, hiding the old pair, and `render_change_value` rendered a JSON null identically to the literal string "null" while a missing key and an empty string both rendered empty. Render an endpoint move as `old_from -> old_to => new_from -> new_to` (insert, delete, and endpoint-preserving updates keep the single pair), render JSON null as the sentinel `<null>` (so the string "null" stays `null`), and render an absent key in a diff as `<absent>` — so null, "null", empty, and absent never collide. `--json` output is unchanged and remains the exact form. Unit tests pin the four ambiguous states and every endpoint case (F7).
The baseline install fsync'd the temp file and atomically persisted it over `--out`, then tried to fsync the parent directory with `if let Ok(dir) = File::open(..)` / `let _ = dir.sync_all()` — swallowing both the open failure and the sync failure. A resume cursor could then print for a rename that never reached disk, exactly the acknowledged-before-durable shape the fsync barrier exists to prevent. Extract a `sync_dir` helper that on Unix opens the directory and fsyncs it, propagating any failure so the caller's `?` aborts before the cursor prints; on non-Unix it is a documented no-op (a directory is not a file-fsync durability primitive there and opening one as a file fails). A unit test proves `sync_dir` returns an error instead of swallowing it (F5).
The backlog cost test measured only manifest and data reads, so it could not see the CPU/allocation term: a poll clones the whole backlog (cut head → cursor) into its first-parent chain via `chain_after`, which grows with the backlog even when the page ceiling is small — invisible to the IO counters. Add a `feed_commits_visited` probe following the existing `graph_build_count` idiom (no-op in production, task-local unset), record the chain length at the poll, surface it through the shared cost harness `IoCounts`, and assert in the backlog test that it equals the backlog and grows with it. This makes the bounded-visit fix (a forward-child projection, tracked as the B1 follow-up) measurable rather than a silent regression surface (F4).
Correct RFC-030 §14's managed-Blob identity bullet (the fragment-id qualifier was superseded by the immutable data-file path), add a second-review-round amendment covering the data-file identity (P1-A), the shared typed merge comparator (P1-B), the per-table-open incarnation re-prove (P1-C), the feed_commits_visited counter (F4), and the F5-F7 polish. Extend the testing.md map: the Overwrite same-length Blob CDC cells, the nested-list merge cell, the second-window feed ABA failpoint, and the feed_commits_visited backlog pin.
Second review round — addressedA second review pass found three correctness gaps and four polish items, all now fixed on this branch (full failpoint-superset workspace suite green: 76 test binaries, ~2195 tests). Correctness
Polish
|
Current-head re-review 1/2 — correctness and authorizationReviewed exact head
These are current-code findings even where a bot thread is marked resolved. |
Current-head re-review 2/2 — boundedness, durability, and shipped statusThe public graph-vocabulary direction is good, but the implementation and RFC status still overstate the evidence:
CI is nominally green at this head, but the canonical workspace/failpoint suite, RustFS integration, and V5↔V6 fence were skipped. I would fix the authorization and snapshot-coherence defects first, remove the “accepted/shipped” claim while the mandatory gates are open, and then re-review C2/C3 after ordered scanning, history traversal, and client output are genuinely bounded. |
The second-window ABA witness was the manifest e_tag alone, which is not a sufficient native-branch incarnation witness: a store may persist no table e_tag (the check silently skipped when the stored value was absent), and content equality is a heuristic, not identity — the repository's own release notes state a manifest e-tag is not a substitute for an incarnation witness. Make the witness logical and store-independent: after plan_intervals opens the per-table datasets, every named branch it opened is re-proven by a fresh, cache-bypassing manifest snapshot at the same pinned version, which must still report the graph_head the enumeration's captured snapshot carries. A recreated fork's manifest at that version carries different lineage commit ids (or lacks the version), so the poll fails closed on every store; the per-open e_tag comparison is retained as defense-in-depth. Main pays no extra resolution. A new CHANGE_FEED_SKIP_ETAG_WITNESS behavior seam (boolean is_enabled failpoint, not an error injection) simulates an e_tag-less store; the new failpoint cell combines it with the pre-table-open park plus a branch delete/recreate and requires the typed refusal. Verified RED with the re-prove disabled: the poll emitted the replacement branch's row under the captured commit's label; GREEN with it.
A poll cloned the complete head-to-cursor backlog into a Vec before the max_commits ceiling was consulted, and walked that chain a second time for on-chain cursor validation (resolve_position -> is_on_chain repeated the same head-down traversal). A max_commits=1 poll over a large backlog therefore performed roughly two full backlog walks and a backlog-sized allocation, and the feed_commits_visited counter recorded only the second walk. Build a forward first-parent child index during cut construction (the same walk that finds genesis, so no extra asymptotic cost), make on-chain validation O(1) membership, and walk the chain FORWARD from the cursor collecting at most max_commits plus one sentinel element that proves more commits remain — the boundary/caught_up semantics are unchanged. The counter now records the complete chain-walk term. New cost cell pins a max_commits=1 poll flat across backlog depths (visited, manifest reads, and data opens all bounded by the ceiling); the existing backlog cell keeps the unbounded-ceiling walk pinned at exactly the backlog.
Every poll rebuilt the cut from scratch: clone the full lineage projection into a map, walk to genesis, and (now) build the child index — O(total history) CPU and allocation per poll even when caught up, invisible to the manifest-IO counters the warm-cut work made flat. A cut is a pure projection of __manifest, so it is exactly valid while the manifest incarnation (version + e_tag/timestamp witness) is unchanged — the same probe the warm poll already pays. Cache one Arc'd cut per handle keyed by that incarnation: a caught-up same-branch poll now reuses it at O(1), any publish/refresh misses and rebuilds, and invalidate_read_caches clears it alongside the other read caches (the incarnation key carries correctness; the clear is the same hygiene the handle caches get). Cross-branch polls keep the uncached capture path.
…e guard Two baseline-install gaps: (1) on non-Unix platforms sync_dir was a silent no-op while the printed resume cursor still claimed the rename was durable — and std's rename is not write-through there, so a crash could lose the namespace replacement after the cursor printed; (2) two processes capturing to the same --out could each atomically replace it and print their own cursor, so one could print a cursor for a snapshot the other had already replaced — a consumer restoring that file and resuming from the printed cursor would skip the changes between the two snapshots. Fail closed at command entry on non-Unix (before any capture work) with a typed message naming the missing durability barrier; a durable/write-through replace is the sanctioned future lift. After the install barrier, re-read the installed file's terminal baseline record (bounded tail read) and refuse to print a cursor that does not match it — the file's own terminal record remains the authoritative pairing for any later reader.
The change routes' spec referenced the generic error envelope, whose reachable schema graph carries the write-path conflict shapes with physical storage keys — so generated clients saw storage vocabulary the runtime projection never emits, and the existing vocabulary gate missed it because it scanned only Change*-prefixed component names. Add ChangeErrorOutput: a wire-compatible projection of the error envelope restricted to what the read-only change routes can produce (code, message, resource_limit, change_feed_gap, change_diff_refusal). Point all three change routes' error responses at it, and give the baseline's NDJSON success response a schema (the terminal ChangeBaselineRecord handshake) so clients can discover the cursor contract. A new reachability gate walks every schema transitively referenced by the change operations and requires storage properties absent and the generic envelope unreachable — closing the name-prefix blind spot.
The RFC header claimed C0-C3 shipped without qualification while two mandatory obligations remain open; name them in the status (the §4.4 ordered-scan memory bound and bounded client auto-pagination) and annotate the C2 phasing row. Record name-only type filtering as a deliberate v1 scope decision with the sanctioned ID-filter extension path. testing.md maps the e_tag-less second- window ABA cell (logical post-open witness), the small-ceiling bounded-poll cost cell, and the OpenAPI reachability gate.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6361039. Configure here.
Third review round — all findings addressedTwo further review passes (one implementation review, one current-head re-review) surfaced four correctness/consistency findings and six status/boundedness findings. All were validated against the code and the pinned Lance 10.0.0 source; every confirmed defect is fixed on this branch (full failpoint-superset workspace suite green: 76 binaries / 2207 tests here, 81 / 2287 on the follow-up branch). Correctness
Durability / client honesty
Spec / status
Still open, by design and now recorded in the RFC header: the §4.4 bounded ordered read and streaming client auto-pagination — both tracked follow-ups rather than silent claims. |
The concurrent-capture guard expected the terminal handshake record inside the snapshot file, but the baseline file contract is deliberately snapshot-only — the handshake goes to stdout. Every successful capture therefore bailed after persisting and never printed a resume cursor. No test caught it: the parity cell compares the two arms to each other (two identical failures count as parity) and the only direct CLI cell asserts the failure path. Replace the content probe with witnesses that match the contract: an exclusive flock on a stable .lock sibling serializes cooperating captures of the same --out from before staging through cursor delivery (closing the validate-then-replaced-before-print window between omnigraph processes), and a file-identity check (device+inode of the File persist() returned versus the path) refuses to print a cursor when any other writer replaced --out after the install. Both are Unix-only, like the rest of the install barrier. New cell pins the success path end-to-end — exit 0, a non-empty paired handshake on stdout, snapshot rows in the file, and no handshake record inside it — so a guard that breaks successful captures can never pass the suite again.
…e gaps Three defects from the same review batch, each proven red first: Warm refresh paired a new head with stale lineage. refresh_for_live_read took the state-only fast path whenever the branch head row merely existed, so after another handle committed, the refreshed head was absent from this handle's lineage projection and every later poll failed with 'lineage chain is missing commit' — permanently. The fast path now requires the durable head to be a commit the projection already contains; otherwise lineage is re-read atomically with the state. Red test: a second handle's commit broke the first handle's next poll with exactly that error; green: the poll emits the foreign commit and stays healthy. Snapshot-by-commit resolution could retarget after branch delete/recreate. The shared Snapshot resolution reopened (branch, version) and echoed the request's commit id without proving the reopened manifest still names it as that branch's head — the same structural proof the feed's commit_snapshot performs, now applied at the one shared resolution point so the finite commit diff and every snapshot-by-commit consumer inherit it. The two existing branching ABA cells now exercise this earlier structural refusal (their exact-message pins widened to the shared incarnation-witness phrase; the Blob-level witness remains for the windows commit resolution cannot see). An exhausted byte budget leaked one oversized change per commit. The solo-oversized forward-progress rule is scoped per enumerate call, and the poll loop only checked remaining rows — so once bytes hit zero, every later commit force-emitted its first change. The loop now stops at the block boundary on zero remaining bytes. Red test: three oversized commits arrived as three solo blocks in one page; green: one per page across three polls.
The feed and baseline handlers authorized the caller's raw branch string while the engine trims it later, so a padded spelling like ' main ' was classified by Cedar as an unprotected named branch and then resolved to protected main — a branch-scope policy bypass. Normalize once (trim; empty-after-trim is a 400) before both authorization and the engine call, so Cedar and the engine judge the same identity. The export route carried the identical pattern and gets the same normalization (baselines are exports of the same data). New policy cell: under an unprotected-only read/export rule with main protected, branch=%20main%20 on the feed and a padded baseline body are both 403 exactly like plain main; before the fix the padded baseline streamed main's full export. Residual, deliberately out of scope: several pre-existing routes (query, change, load, snapshot, branch controls) authorize the same raw shape and deserve one audited normalization pass with per-route policy tests as a follow-up.
Fourth round — re-review 1/2 dispositions (all five validated; four fixed, one already closed)All five findings from the correctness/authorization re-review are addressed on this branch, plus one functional repair the bots caught in my own previous fix. Full failpoint-superset workspace suite green (76 binaries / 2211 tests), each defect proven red first.
Also this round: the bots caught that my previous concurrent-capture guard expected the baseline handshake inside the snapshot file — which the file contract deliberately never carries — so every successful capture failed after persisting. Repaired with contract-true witnesses (an exclusive lock on a |
2 participants
What & why
Implements RFC-030 C0–C3: exact first-parent commit diffs, a stateless durable change feed with caller-owned cursors, and an exact snapshot-plus-cursor baseline reset flow.
The embedded SDK, HTTP API, and CLI share the same graph-level change semantics. Public results use node/edge vocabulary and logical values; no Lance paths, table versions, fragments, or other physical storage details are exposed.
Backing issue / RFC
Checklist
docs/dev/invariants.mdWhat changed
BranchIdentifier, closing same-name delete/recreate ABA windows.main, retaining the branch-control, Blob-test, and net-zero merge work while keeping CDC and merge comparison logic unified.Contract notes
Verification
cargo test --workspace --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints— passescargo clippy --workspace --all-targets --locked -- -D warnings -W clippy::dbg_macro— passescargo clippy --workspace --all-targets --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints -- -D warnings -W clippy::dbg_macro— passescargo fmt --all --check,git diff --check,scripts/check-agents-md.sh— passchanges45/45,changes_cost6/6, forbidden API guards 19/19,branching41/41,composite_flow3/3 on the default stack, warm-read cost/ABA 16/16, net-zero merge 11/11, and server OpenAPI 98/98.Greptile Summary
Implements graph-level commit diffs, a durable caller-cursor change feed, and snapshot-plus-cursor baseline recovery across the engine, HTTP API, CLI, and shared wire types.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Consumer participant Surface as SDK / HTTP / CLI participant Feed as Change feed participant Manifest Consumer->>Surface: Poll with start or durable cursor Surface->>Feed: Resolve branch-scoped position Feed->>Manifest: Capture first-parent cut and snapshots Manifest-->>Feed: Commit lineage and historical table versions Feed-->>Surface: Complete blocks or page token Surface-->>Consumer: Page token for partial commit Consumer->>Surface: Continue with page token Surface->>Feed: Resume captured cut Feed-->>Surface: Terminal page and durable cursor Surface-->>Consumer: Cursor advances past complete commits onlyReviews (18): Last reviewed commit: "Merge main into change-feed" | Re-trigger Greptile
Context used (3)