refactor(networkx): make the recovery reload a process-local flag - #3874
refactor(networkx): make the recovery reload a process-local flag#3874danielaskdd wants to merge 11 commits into
Conversation
After a failed save whose recovery reload also failed, the in-memory graph holds a mutation the file does not have. That divergence was armed by setting `storage_updated` (and, since the fingerprint fence landed, by invalidating `_loaded_fingerprint`) so a later `_get_graph` would reload out of it. Both are cross-process channels that answer "did a peer commit?"; this fact is the opposite one -- the file did NOT move, memory is what is wrong -- and it is purely process-local. Replace both arms with `_recovery_reload_pending`, a plain bool tested ahead of both channels in `_get_graph` and in `index_done_callback`'s decline block, cleared by `_reload_locked` as its third post-condition and by `drop`. Three things this fixes beyond accuracy: * `_missed_notification_reloads` no longer counts a failed save as a lost notification. Invalidating the fingerprint made the file channel report a peer commit that never happened whenever the flag write also failed -- i.e. precisely in the deployment where someone would be reading that counter to decide whether the `os.utime` monotonicity option is needed. * Arming no longer needs a Manager RPC to the process whose outage may be why the reload just failed, so its dedicated failure path, best-effort log and ordering argument all go away. * Single-process mode is covered by the same mechanism. The file channel is gated off there, so the flag had been the only thing arming recovery. The fingerprint is deliberately left alone when recovery is armed: the save failed, so the file is untouched and the recorded value still describes it. Also record, next to the code, the two deferred remedies #3854 had left only in its issue thread: the writer-side `os.utime` mtime monotonicity option for the timestamp-tick collision, and an explicit generation key (or atomic set publication) for the multi-file completeness test. Tests: five new cases in the NetworkX fence suite covering recovery in single-process mode, the decline on the next commit, the clean counter, the absence of any manager write, and `drop` clearing the sticky flag. Each was verified to fail against a mutation of the behaviour it pins. Closes #3854 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8577747143
ℹ️ 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".
`_worker_with_a_failed_save` undid the caller's `monkeypatch` fixture to restore `write_nx_graph` / `load_nx_graph`. pytest hands the same fixture instance to the test and to every fixture it requested, so that undo also reverted the `multiprocess` fixture's patch of `file_fingerprint.is_multiprocess_mode`. Three tests that ask for the fence therefore ran their assertions with it disabled. `test_a_recovery_reload_is_not_counted_as_a_lost_notification` was the worst of them: `_peer_commit_detected()` is unconditionally False in single-process mode, so both of its assertions on that call were vacuous. Apply the breakage through a `pytest.MonkeyPatch` instance of the helper's own and undo only that, and assert `fence_enabled()` in the test that depends on it so the same mistake cannot return silently. Verified by re-introducing the shared undo: the new precondition fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6c16c4e51
ℹ️ 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".
The recovery flag is tested ahead of both fence channels and one reload discharges all of them, so a peer commit that arrived unannounced WHILE recovery was pending was handled correctly and never counted: `_reload_locked` adopts the file, and nothing afterwards can tell it happened. The same precedence, and the same blind spot, existed in `index_done_callback`'s decline block. `_missed_notification_reloads` is the evidence the writer-side `os.utime` monotonicity decision waits on, so undercounting it is as much a defect as the overcounting this branch's predecessor produced. Both recovery branches now call `_count_unannounced_peer_commit_locked` BEFORE reloading, testing exactly what the channel branches test: the flag never fired, and the file is not the one this process recorded. In the ordinary recovery case -- a failed save with no peer -- the file is untouched, so it counts nothing. Corrects the class docstring's "clean evidence" claim accordingly: the process-local test cannot overcount, but it could undercount, and that is what the new classifier exists for. Tests: one case per site, each verified to fail with the classifier removed, and the `_get_graph` one also with the classifier moved after the reload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41e6787b1e
ℹ️ 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".
Counting at detection re-counts the same peer commit on every later call when the reload does not land: a `_reload_locked` that raises leaves `_loaded_fingerprint` and `_recovery_reload_pending` exactly as they were, so the divergence is re-detected and re-counted, without bound. This is not specific to the recovery branch the previous commit added. The same shape landed in #3861: a reproduction against the pre-existing `_get_graph` fingerprint branch, with no recovery flag involved, counted 5 for one peer commit across 5 failed reloads. Fixing only the new site would leave the counter inflatable through the old one, so all four counting sites now route through `_count_unannounced_peer_commit_locked`, which becomes the single increment site and deduplicates on the file's `(st_mtime_ns, st_size)`. Counting stays at detection rather than moving after a successful reload: the window occurred whether or not this process could reload out of it, and a file that never becomes readable would otherwise erase the evidence entirely. A genuinely second commit landing while reloads keep failing has a different fingerprint and is counted; two commits sharing one timestamp tick are not distinguished, which is the residue the class docstring already documents, inherited rather than introduced. An unreadable stat between the detection and the sample counts nothing: it cannot say which state it would be counting, so it could neither be deduplicated nor trusted. The next call counts it. Tests: the same peer commit across five failed reloads counts once at both the channel branch and the recovery branch; a second distinct commit during failing reloads counts twice. Verified against dropping the dedupe (all three fail) and against never recording the counted fingerprint (all three fail). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d867fadfc
ℹ️ 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".
The note this PR added to `file_fingerprint` designates the `_missed_notification_reloads` counters "that each storage logs" as the evidence the writer-side `os.utime` remedy waits on. After the previous commit that was true of NetworkX only: `NanoVectorDBStorage` and `FaissVectorDBStorage` still increment before their load and leave `_loaded_fingerprint` in place when it raises, so one peer commit inflates either counter without bound — the same shape, from #3867. Rather than narrowing the claim to one backend, make it true. The tick collision the remedy addresses is a property of all three, so evidence from one of them is the weaker half of the decision. `counts_as_a_new_lost_notification` moves the predicate into `file_fingerprint`, where the module docstring already argues that this fence's hazards are details rather than shape and that three copies is how it rots. Each storage keeps its own `_counted_peer_fingerprint` and consults the one predicate; NetworkX is migrated onto it, dropping its local UNREADABLE special case. The vector backends already sample the fingerprint before their read, so the counting now reuses that sample: the dedupe costs them no extra stat, and the sample it deduplicates on is exactly the one the reload will adopt. Tests: one case per vector backend — the same peer commit across five failed loads counts once, a genuinely second commit during the outage counts twice, and the count stays put once the load finally lands. Verified against two mutations of the shared predicate (never deduplicate; always suppress), each turning the new cases red in all three backends. Note for local runs: the FAISS suite needs `faiss-cpu`, which the `api`/`test` extras do not bring; CI syncs `--extra offline-storage` and does run it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b02fbac8f5
ℹ️ 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".
`_counted_peer_fingerprint` was set when counting and never cleared, so it outlived the reload it existed for and suppressed any on-disk state that RECURS. The reachable case is a repeated drop: a peer drop whose notification is lost (counted, marker = the "absent" fingerprint), a recreation this process IS notified about, then a second drop whose notification is also lost — same absent fingerprint, genuinely a second lost notification, silently uncounted. Not the same-tick collision residue, which is about two commits indistinguishable from each other, not one state recurring after another was adopted in between. Cleared in each backend's `_adopt_fingerprint`, which is the single place a new state is recorded and is reached only after a load or commit has landed. That is exactly the boundary the marker needs: it survives failed reload attempts, because the raising load never reaches adoption, and ends the moment one succeeds — from there `_loaded_fingerprint` IS that state, so any later divergence is new by construction. Tests: one per backend, the drop / notified recreation / drop sequence, asserting the counter reaches 2. Each verified to fail with the clear removed. The two earlier mutations of the shared predicate (never deduplicate; always suppress) were re-run against the corrected code and still turn the dedupe cases red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10f3dd883b
ℹ️ 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".
`_count_unannounced_peer_commit_locked` took its own `stat` and `_reload_locked` took another. A transient `UNREADABLE` on the counting one alone then lost the event permanently: the helper skipped the increment, while the reload's own successful sample adopted the peer state and erased the divergence a later call would have counted. The docstring's promise that "the next call counts it" only holds if nothing adopts in between. The caller now samples once and hands the same value to both. An unreadable sample therefore adopts no fingerprint (`adopted(UNREADABLE)` is `None`), so the divergence still stands and the next call counts it — the two outcomes are tied rather than independent. This is NetworkX-only. The vector backends were already written this way in the commit that deduplicated them: they sample once before their read and that same value drives the counting and the adoption. `_reload_locked` gains an optional `fingerprint`; `None` is unambiguous as "not given" because `_stat_fingerprint` returns a tuple or `UNREADABLE`, never `None`. The two branches that do not count (the flag branch at each site) still sample for themselves. Tests: shadow `_stat_fingerprint` for exactly one call — `_peer_commit_detected` reaches `file_fingerprint.sample` directly, so only the hoisted sample is hit — and assert the event is not counted on that call but IS on the next, with no fingerprint adopted in between. Verified against both halves of the defect: the helper sampling independently, and the reload sampling independently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
|
@codex review |
`_missed_notification_reloads` reads like a log line and is not one: it is the instrument the `os.utime` remedy waits on, so a bias in it silently decides whether that work is ever judged necessary. Review found it wrong in five distinct ways on this branch, each fixed in a different file or method: arming a cross-process channel for a process-local recovery (overcount); a recovery branch outranking both channels and discharging them in one reload (undercount); counting attempts rather than states, unbounded across failed reloads (overcount); a dedupe marker outliving its reload and suppressing a recurring state (undercount); and counting and adopting from two independent samples, which loses the event outright. No single site shows that contract, and each is a fragment on its own. State it once in the module docstring, with the five failure modes indexed to where each is kept, and name the one blind spot that remains by design: the tick collision the deferred remedy would remove. Documentation only; every claim in the index was checked against the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 456a8120ee
ℹ️ 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".
The previous commit tied counting and adoption to one observation and left the divergence test inside the counting helper taking a third `stat`. Same loss, one step earlier: that recheck can return UNREADABLE while the caller's sample succeeded, so the count is skipped and `_reload_locked( sampled)` still adopts the good sample, erasing the divergence a later call would have counted. `file_fingerprint.divergence_detected` is `peer_commit_detected`'s decision without the sampling — same gate, same UNREADABLE handling, same multi-file completeness test — and `peer_commit_detected` now calls it after sampling for itself. `NetworkXStorage._peer_commit_detected` takes an optional sample and the counting helper passes the one it was given. The rule this makes explicit, corrected in the module docstring: once a call is committed to adopting, it must not observe the file again. An observation BEFORE that point is fine, and the vector backends use one — theirs gates the whole function and returns early, adopting nothing, so a failure there costs a retry rather than the event. Their count and adopt already share a sample, so they need no change. Test: allow the branch condition's observation and the hoisted sample, then make every further one unreadable, and assert the file is observed exactly twice and the event is counted. Targets `file_fingerprint.sample` rather than `os.stat` so the load's own `os.path.exists` stays out of it. Verified to fail with the helper re-observing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 865de7a8c5
ℹ️ 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".
`_adopt_fingerprint` cleared `_counted_peer_fingerprint` unconditionally, in all three backends. `adopted(UNREADABLE)` is `None`, which is not a state -- it means "nothing recorded", and `peer_commit_detected` reports a change against it for ANY state. So a retry whose pre-read `stat` fails while the load itself succeeds recorded `None`, forgot which commit had been counted, and counted the same peer commit again on the next call. Reproduced before fixing: one peer commit, a failed reload, then a retry with an unreadable sample, gave `_missed_notification_reloads == 2`. The clear is now conditional on the adoption recording a concrete state. The post-drop fingerprint is `(None,)` -- a real state, distinct from `None` -- so the recurring-drop case that motivated the clear still works. Tests: one per backend, and they bracket the boundary from both sides together with the recurring-state tests. Clearing unconditionally fails the three new ones; never clearing fails the three recurring-state ones. Corrects index item 4 in `file_fingerprint`, which stated the clear without its precondition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Description
Two related pieces of work. The first is what the PR was opened for; the second is what eight rounds of review turned it into.
1. The recovery reload becomes a process-local flag. When a
NetworkXStoragesave fails and the recovery reload that should have discarded the unpersisted mutation fails too,self._graphholds a mutation the file does not have. That divergence has to be visible to every later call, orutils_graph's deletion retry reads it as durable state and sweeps a live object's tracking row.It was armed by setting
storage_updated— and, since #3861, also by invalidating_loaded_fingerprint. Both are cross-process channels answering "did a peer commit?". The fact here is the opposite one: the file did not move (the save failed), memory is what is wrong. It is also purely process-local._recovery_reload_pending, a plainbool, replaces both arms and is tested ahead of both channels at the two sites they are tested at.2.
_missed_notification_reloadsbecomes trustworthy. That counter is the evidence #3854 designates for deciding whether the writer-sideos.utimeremedy is ever needed — so a bias in it silently decides whether that work is judged necessary. Review found it wrong in five ways, in both directions, some of them pre-existing. Its contract is now stated once, infile_fingerprint's module docstring, with the five failure modes indexed to where each is kept:Related Issues
Closes #3854. The reload fence itself landed in #3861 (NetworkX) and #3867 (Nano + FAISS); this is the remaining cleanup that issue listed, and with the two deferred-remedy notes below nothing it tracks is left undocumented in the code.
Changes Made
lightrag/kg/networkx_impl.py— the recovery flag_recovery_reload_pending, armed inindex_done_callback's failed-save handler, tested first in_get_graph(discard the divergent graph) and inindex_done_callback's decline block (decline rather than publish mutations from a batch already reported as failed)._reload_lockedclears it as its third post-condition;dropclears it too.lightrag/kg/{networkx,nano_vector_db,faiss}_impl.py— the counterThe five failure modes, each fixed in a different place:
_reload_lockedthat raises leaves the recorded fingerprint untouched, so the same commit is re-detected — and re-counted — by every later call, without bound. Reproduced against the_get_graphfingerprint branch from fix(kg): give NetworkX's reload fence a channel the manager cannot lose #3861 with no recovery flag involved: one commit, five failed reloads, counter== 5. All four counting sites now route through one increment site and deduplicate on(st_mtime_ns, st_size). The two vector backends had the same shape from fix(kg): extend the reload fence to the vector backends #3867 and are fixed too. Unbounded overcount._adopt_fingerprint, but only when a concrete state is recorded:adopted(UNREADABLE)isNone, which means "nothing recorded" and against which any state reads as a change, so clearing there would count the same commit twice. The post-drop fingerprint is(None,), a real state, so a drop still clears. Both directions.UNREADABLEon any one of them loses the event outright: that step is skipped while the reload's own good sample adopts the peer state, erasing the divergence a later call would have counted. Counting callers now take one sample and pass it todivergence_detected,counts_as_a_new_lost_notificationand the reload alike. Loss, not merely undercount.This changes behaviour that shipped in #3861 and #3867 — items 3 and 4 — the counter only, never a reload or decline decision.
lightrag/kg/file_fingerprint.pycounts_as_a_new_lost_notificationanddivergence_detected, shared by all three backends, where the module docstring already argues this fence's hazards are details rather than shape and three copies is how it rots.os.utimemtime-monotonicity option (and why it is not done: no good bump value on a 1 s / 2 s-granularity filesystem), and an explicit generation key or atomic set publication for the multi-file completeness test (a storage-format change for what is currently development/test storage).What could break, and why it does not
drop's clear is load-bearing. The recovery flag is sticky and is tested byindex_done_callback, so adropleaving it set would make the first commit after a clear decline and discard fresh work._reload_lockedclears it last, after the load returned — a load that raises leaves it armed, which is what keeps the divergence visible.Falselike the two channel declines, so_commit_graph_or_raiseand_flush_storages's_flush_oneturn it into a caller-visible failure (rule 5 of the issue): the document goes FAILED and reprocessing re-extracts the work.(st_mtime_ns, st_size), which is the residue theos.utimeremedy would remove.Checklist
file_fingerprintmodule docstring; no file underdocs/references this fenceAdditional Notes
Tests. 18 cases across the three backends' fence suites, replacing the now-obsolete
test_failed_save_arms_the_file_channel_even_if_the_flag_write_fails. Every one was verified to fail against a mutation of the behaviour it pins, then restored — 18 mutations in total, including both directions of each two-sided rule:..._an_unreadable_adoption_keeps_the_dedupe_markertests fail; never clearing it → the three..._a_recurring_state_is_counted_again_after_a_notified_reloadtests fail;Test runs:
tests/kg/networkx_impl,tests/kg/nano_impl,tests/kg/faiss_impl,tests/pipeline— 1086 passed, on the current head withmainmerged in.Two notes for anyone reproducing locally: the FAISS fence suite
importorskipsfaiss, which theapi/testextras do not bring, so it silently skips unlessfaiss-cpuis installed as well (CI syncs--extra offline-storageand does run it; it was run locally too rather than left to CI). Andtests/kgcannot be collected whole in this sandbox — optional backends likeasyncpgare not installable here, and the same collection errors occur on an unmodified checkout. CI covers them and is green.🤖 Generated with Claude Code
https://claude.ai/code/session_016iCWxLGmRUKJyRnsC9kpMd