fix(falkordb): make the graph-DB push converge, and add a repo-keyed delta (#3057)

[galshubeli]

Fixes Graphify-Labs#3057.

The defect

push_to_falkordb is MERGE-only. Once graphify global add prunes a repo out of the global graph, a full re-push leaves every one of those nodes in the target permanently — the database diverges from the source and never converges back, no matter how many times you re-push. @Azeem1985 measured it: +1,250 nodes / +1,151 edges surplus, and 25 of 25 sampled pruned ids still present after a full re-push.

While verifying that, a second bug surfaced that isn't in the report and is arguably worse:

OLD (per-row)    4.1s  ->  0 nodes / 0 edges   (reported 4000/4000)
NEW (batched)    0.4s  ->  4000 nodes / 4000 edges

The exporter labelled nodes by file_type only (:Python). Every read path in GraphStore matches :Entity. So the push reported 4000 nodes written and graphify query / serve read back zero — on a branch where FalkorDB is the backend, export falkordb --push produced a graph the product itself cannot open.

What changed

The FalkorDB writer now goes through GraphStore — the same batched UNWIND writer graphify uses for its own graphs. That supplies the :Entity label, the n.id index and the batching for free, and deletes a duplicated, worse writer. Pre-existing nodes are labelled on the way in so the MERGE switch doesn't create a duplicate beside each one. This is also the Graphify-Labs#2258 diagnosis (unindexed edge-endpoint MATCH), fixed by reuse rather than a new code path.

--graph-name. push_to_falkordb has always taken graph_name; the CLI never passed it, so every push through the CLI landed on the graphify key with no way to aim it elsewhere. That is what turned @Azeem1985's mistake into a 1.2M-node deletion.

--prune converges the target — nodes and edges. DETACH DELETE on a stale node takes its edges with it, but an edge dropped between two surviving endpoints needs its own sweep; without it the surplus-edge half of the report survives. Opt-in rather than default, because --graph-name never existed and existing users may have several projects merged into the one graphify key, where a default prune would delete all but the last. That's the contract question from the issue — say the word and I'll flip the default.

Repo-keyed delta (graphify global push, default). Mirrors global_add's own contract instead of inventing one: the repo is the unit of change, keyed on the manifest's per-repo source_hash, which global_add already records and already uses to return skipped=True.

Drift repair. The "what I last pushed" state lives in the target as :GraphifyPushState nodes, cross-checked against the target's own per-repo counts (one indexed aggregate). A ledger alone still reads clean after a wipe or a half-landed run and never repairs it — @Azeem1985's finding #2. Covered by a test that deletes part of a repo behind the push's back and asserts it gets rebuilt.

Cross-repo edges. global_add remaps external-library nodes onto whichever repo first contributed them, so a B -> A edge is owned by neither alone. Pruning A drops it; re-adding only A's internal edges would silently lose cross-repo connectivity on every delta. The delta selects every edge incident to the repo, in either direction.

Delete guards. Capped at 20% of the target unless --allow-shrink — the same "refuse to SILENTLY drop nodes" rule as the Graphify-Labs#479 build guard. Only net removal counts against it; a re-pushed repo is pruned and immediately re-added, so charging it would refuse any delta touching more than 20% of a small global graph. Deletes are paged with the LIMIT inside a WITH, because FalkorDB's LIMIT does not short-circuit an eager DELETE — ... DETACH DELETE n LIMIT $page deletes the whole label. That's finding #1, and the reason is in a comment at the call site.

graphify global push. The global graph is a named FalkorDB graph with no output directory, and export --push resolves its source from a directory's falkordb.json pointer — so the one graph this entire contract is about had no CLI route at all. It does now.

Measured

Local FalkorDB, 20 repos / 20k nodes / 20k edges:

	wall	rows sent
full push, nothing changed	1.46s	39,999
delta, nothing changed	0.01s	0
delta, 1 of 20 repos changed	0.33s	2,001 (5.0%)

Delta output is identical to a from-scratch load on node and edge counts.

Convergence, same corpus:

OLD full re-push  -> 4240 nodes  (ground truth 4000)  converged=False
NEW prune re-push -> 4000 nodes  (ground truth 4000)  converged=True

Tests

13 new integration tests against a live FalkorDB, covering: the :Entity label, add-only default, node convergence, edge-only convergence, prune idempotence, the size guard refusing and not deleting, target isolation by --graph-name, delta skip/re-send/removal, cross-repo edge preservation, drift repair, and a foreign manifest being refused.

Verified they actually catch the bugs: 6 fail on the parent commit, all pass here. Full suite is +13 passing with no new failures (this environment has 390 pre-existing failures from missing tree-sitter grammars, identical before and after).

Deliberately not included

The Neo4j writer is untouched — byte-identical to the parent commit. It has the same defects (per-row, unindexed, never deletes, no database parameter) but verifying a fix needs a Neo4j instance I don't have. The new flags are refused on export neo4j rather than silently ignored, since accepting --prune there would report a converged push that deleted nothing. Happy to do that path in a follow-up.

No CHANGELOG entry — this repo folds those in at release time (chore: bump to X; changelog for #...). Say if you'd rather have one in the PR.

Relationship to Graphify-Labs#2312

Graphify-Labs#2312 proposes a Neo4j source-of-truth backend, which is largely what this branch already is. This isn't competing with it — it's the push-side piece that design needs regardless.

[Azeem1985]

Ran this branch on the box that produced the Graphify-Labs#3057 numbers before answering, so the answers below are informed by the code rather than the description.

Verified here: clean clone of fix/graphdb-push-converge at dc5183c, live falkordb/falkordb:latest (scratch instance, scratch keys) — 15/15 integration tests pass in ~2s. Same test file against the parent commit: 13/15 fail (the two that pass are the pre-existing create/idempotence tests). You counted 6 on the parent — the difference is that for me the prune/delta tests also die on the then-unknown kwargs, which I'd still count as the tests catching the missing API, not as vacuous. I also read the two call sites that worried me most: the LIMIT does sit in a WITH ahead of the DELETE, and the shrink guard refuses before anything is deleted, in both the full-push and delta paths. Both findings from the issue are closed properly.

On the --prune default: keep it opt-in. My reasoning, as the person who deleted the 1.2M nodes: --graph-name did not exist before this PR, so every existing CLI user's pushes have been landing on the one graphify key, and some of those keys genuinely hold several projects merged together. Flip the default and those users get one of two surprises on a routine re-push: a silent deletion (if the foreign share is under the 20% guard) or a hard refusal (if over). Neither is acceptable fallout from a patch upgrade. And the converging path now exists where it belongs — graphify global push, where "make the target match the manifest" is the contract, and which is new, so a converging default there breaks nobody.

One small addition that would close the remaining gap without touching the default: when an add-only full push finishes and the target holds more nodes than the source, print one line — target has N nodes the source does not; --prune converges it. That turns the silent permanent drift from the issue into a visible one-liner, at zero risk.

Changelog: agree, fold at release — that matches the repo's own history (e.g. 1c6b3db).

Neo4j: refusing the new flags on export neo4j is the right call — accepting --prune there and ignoring it would report a converged push that converged nothing, which is the same class of lie the :Entity label bug told. Follow-up when a Neo4j instance is available sounds right.

Nothing else I'd hold it for. Thanks for turning the report into the fix — the drift repair via :GraphifyPushState cross-checked against the target's own counts is exactly the "ledger alone reads clean after a wipe" fix I hoped someone would build.

[galshubeli]

Thanks for running it on the box that produced the original numbers — that is the verification I could not do myself, and independently reproducing 15/15 vs 13/15 on the parent settles it better than my own run did.

On the count difference: you are right and your explanation is the whole of it. My "6 on the parent" was from an earlier, smaller version of the test file, before the prune/delta tests existed. Against the final file the parent fails 13, exactly as you measured. A test dying on an unknown kwarg is the test catching a missing API, so I would count it the same way you do.

Drift notice: done, pushed as c1d0a7e. Implemented the way you framed it — the surplus is computed with the same "not stamped by this push" count the prune path deletes by, so there is one definition of drift rather than two that can disagree:

$ graphify export falkordb --push localhost:6379 --graph-name stg
  note: target has 2 node(s) the source does not; --prune converges it.
Pushed to FalkorDB [stg]: 20 nodes, 0 edges

It is returned as target_surplus from the writer and printed by the CLI, so it is testable rather than a print buried in library code, and a converged target reports 0. Test covers the drifted case, the prune that fixes it, and the 0 afterwards.

--prune default stays opt-in, and your reasoning is better than mine was. I had "users may have several projects merged into the one key"; you have the actual failure split — silent deletion under the guard, hard refusal over it — and "neither is acceptable fallout from a patch upgrade" is the right test to apply. Also agree on where the converging default belongs: graphify global push is new, so nothing breaks, and "make the target match the manifest" is genuinely the contract there rather than a behaviour change.

Changelog folded at release, Neo4j as a follow-up when there is an instance to verify against.

One thing you should know, since it changes where this lands: this PR cannot go to Graphify-Labs as written. Upstream has only v8, which has no graphify/store.py at all — push_to_falkordb there still takes an nx.Graph, and global_graph.py is still NetworkX + graph.json. Every symbol this patch leans on (GraphStore, add_nodes_from, prune_repo, _stream) is absent, so a cherry-pick auto-merges four of five files and then ImportErrors on first use. falkordb-backend is also 236 commits / 19 releases behind v8.

So I am building the v8-targeted version now — same contract, hand-rolled batched UNWIND writer instead of GraphStore, delta bucketed over the in-memory graph. Numbers so far on 20 repos / 20k nodes: old per-row full push 77.2s, batched 2.9s (27x), and the old push writes a graph that reads back as 0 nodes under a single-label query. Will link it here when it is up so the review carries over.

[galshubeli]

v8-targeted PR is up: Graphify-Labs#3069. Same contract, v8-native implementation — batched writer hand-rolled in graphdb.py since there is no store.py there, and the delta buckets the in-memory graph by repo in two passes instead of querying the source. Your review carried over: --prune stays opt-in, the drift notice is in, changelog folded at release, Neo4j refused rather than ignored.

Two things worth reporting from porting it, since both were caught by testing rather than by reading:

The v8 numbers are worse than mine were, which makes the batching case stronger: old per-row full push 79.7s vs 2.9s batched (27x) on 20 repos / 20k nodes, against 4.1s vs 0.4s on the other branch. Same corpus — the difference is that this branch had already moved its own writes to batched UNWIND, so the exporter was the only per-row path left.

And a placement bug that only a CLI smoke test would have found: my global push block landed in the provider subcommand chain instead of global, because v8 has more than one elif subcmd == "remove" and I anchored on the first. graphify provider push <URI> would have run a global push. Caught, moved, provider re-verified intact. Unit tests would never have seen it.

[Azeem1985]

Ran the v8 PR on the same box before answering — full verification posted on Graphify-Labs#3069. Short version: 17/17 on 24c45a0 against a live FalkorDB; the same test file on the parent fails all 15 new tests (one more than your 14 — same counting difference as last time, and I'd still count a kwargs death as the test catching the missing API); and the delta contract holds end-to-end through the CLI, including read-back convergence and the :GraphifyPushState nodes landing in the target.

On the two porting findings: the worse v8 "before" makes sense — your branch had already batched its own writes, so the exporter was the last per-row path standing, which is exactly why the 27x belongs upstream. And the provider-chain placement bug is the best argument for CLI smoke tests I've seen this month; I re-checked what shipped: graphify provider push <URI> refuses with the provider usage line, and global push routes to the global chain. Both intact.

Nothing left open on this thread from my side — the review continues on Graphify-Labs#3069. Thanks for carrying the whole contract over rather than just the diff.

[galshubeli]

Closing this in favour of Graphify-Labs#3069, which you have now verified on the same rig (there). Not abandoned — superseded, and the review you did here is what got it into shape.

The reason this one is redundant rather than parallel is a portability asymmetry I only checked after opening Graphify-Labs#3069, and it went the direction I had assumed it would not: the v8 exporter passes all 16 of this branch's own tests unmodified. GraphStore is nx-shaped — nodes(data=True), edges(data=True), G.nodes[u] all resolve — so the hand-rolled writer runs against it fine. Portability only failed one way: this branch's GraphStore-based version cannot go back to v8 (no store.py there at all), but the v8 version comes forward here without a change. Since falkordb-backend takes periodic v8 merges, Graphify-Labs#3069 arrives on this branch on the next one regardless.

I should have run that check before opening two PRs rather than after. It would have saved you a second review pass.

One thing genuinely lost by closing, worth recording so it is not rediscovered later. This PR routed the push through GraphStore.add_nodes_from — a net −81 lines, one batched writer in the tree. Graphify-Labs#3069 has to hand-roll its own, so once it lands here there will be two batched writers doing the same job, and the exporter will not pick up whatever GraphStore's write path gains later. That is architecture, not correctness — both pass the same tests — but it is a real follow-up on this branch after Graphify-Labs#3069 merges: collapse the exporter back onto the store's writer. Cheap then, and it is the change this PR already contains if anyone wants the diff.

Everything from your review lives on in Graphify-Labs#3069: --prune opt-in, the drift one-liner on the default path, the shrink guard refusing before it deletes, push state in the target rather than a local ledger, and the Neo4j flags refused instead of ignored. Thanks for turning the report into something reviewable and then reviewing it twice.

[Azeem1985]

Ran the portability check on the same rig before closing the loop, since it was the one new claim here: grafted Graphify-Labs#3069's graphify/exporters/graphdb.py (from 24c45a0) over this branch's at c1d0a7e — a 332/314-line swap — and ran this branch's own test_falkordb_integration.py, unmodified, against a scratch FalkorDB. 16/16, identical to the unmodified control (16/16). So "the v8 version comes forward without a change" is now measured from this side too, and superseded-not-parallel is the right call.

Agreed on what closing actually costs, and it's worth the record you made: once Graphify-Labs#3069 lands here on a v8 merge, collapsing the exporter back onto GraphStore's writer is the cheap follow-up, and this PR's diff is the reference for it. Two batched writers passing the same tests is fine to merge and wrong to keep.

Nothing further from my side — the thread continues on Graphify-Labs#3069 if anything moves. Thanks for closing this one with the asymmetry check on the record instead of just the button.