Scope graph rebuilds per store, on master's page enumeration - #1254
Merged
Conversation
This was referenced Aug 5, 2026
alistair3149
marked this pull request as ready for review
August 5, 2026 20:23
alistair3149
marked this pull request as draft
August 5, 2026 20:23
For #1230 Groundwork for running graph rebuilds in the background. A rebuild rebuilt every configured backend in one pass, in one unbatched loop, with no way to scope, resume, or find out afterwards what happened: one unreachable store aborted the whole thing at `initialize()`, the page ids came back in a single array, and the script always exited 0. A rebuild is now always of one store, and rebuilding everything is one run per store, in sequence. That is what makes each store independently recoverable: an unreachable store costs only its own run, and only its own run has to be redone. Every backend has a name, since a scoped rebuild has to be addressed by something. Neo4j is `neo4j`. A `$wgNeoWikiSparqlStores` entry takes an optional `name`, defaulting to its `projection`; an entry repeating a name already taken is skipped with a warning, the way a missing `updateUrl` already is. Extensions register under a name too, via `NeoWikiRegistrar::addGraphDatabasePlugin()`. The names are a composition-level map, so `GraphDatabasePlugin` stays name-free and the composites over it stay unchanged. The save path composes from the same map and keeps its order and its per-plugin isolation. `GraphRebuildCoordinator` starts and resumes runs; `GraphRebuildExecutor` walks the wiki for one of them. The maintenance script is a shell over the coordinator, which is what the background jobs will call. Each run walks the subject pages in batches, projects each into its one store, then removes the pages MediaWiki no longer has, recording its cursor and counters after every batch. `--resume` picks a store's last unfinished run back up from that cursor, reopening the row so one interrupted rebuild stays one record. Failures are separated by what they say about the rest of the run. A page that will not project is logged on the `NeoWiki` channel and counted, and the run carries on. A store that cannot be reached — including one whose `initialize()` throws — ends that run, with the cursor to resume from. Only one run of a store may be going at a time; a second is refused. There is no supersede or cancel yet: `cancelled` is in the status enum because the jobs will write it, and resuming already accepts it. `neowiki_rebuild_runs`, NeoWiki's first own table, declared as an abstract schema with the generated per-DBMS SQL committed alongside (`composer dbschema` regenerates it; a test fails if the two drift) and registered on `LoadExtensionSchemaUpdates`. `--store <name>` scopes to one store, `--batch-size <n>` sets the batch (200 by default), `--resume` continues. With no arguments it still rebuilds every store, so `make rebuild-graph-databases` and the existing docs keep working. Output is per batch rather than per page, and the script exits non-zero whenever anything was left unreconciled. Background jobs and queueing, REST endpoints, an admin Special page, staleness computation, supersede and cancel, and parallelism within a store. They are the follow-up PR; this one is the service and the records they need. Co-Authored-By: Claude Opus <noreply@anthropic.com>
Since 1.46, Maintenance::fatalError() only throws instead of exiting when the script is flagged as being under test, so the tests covering the rebuild's non-zero exits exited the PHPUnit process instead. Flag the script where the property exists; the older supported versions have no such flag and go by the MW_PHPUNIT_TEST constant alone. Also states in RebuildRun that only failed() leaves an error behind. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…ndings Reporting failure with fatalError() ended the whole PHPUnit process on MediaWiki 1.46, which since that version only converts it into an exception for scripts flagged as under test. Returning false from execute() is what MaintenanceRunner turns into the exit status anyway, and it is testable on every supported version without a shim. Behaviour, from the review: * Resuming every store no longer fails over the stores whose last rebuild finished; asking for one store by name and finding nothing to resume still does. * An Error thrown mid-run is recorded as the run's end rather than leaving it recorded as still going, which would block every later rebuild of that store. * A SPARQL store may no longer take the name 'neo4j', which silently replaced the Neo4j backend in both the rebuild map and the save path. * An extension plugin whose name is already taken is warned about rather than dropped in silence, matching what a configured store repeating a name does. * A store name that is numeric no longer breaks rebuilding every store. * Skipped pages are logged, so a page a lagging replica hid leaves a trace. * An over-long store error no longer fails the write that records the failure. Documentation now says what the code does: the walk over the wiki is what is checkpointed and resumable, a store that dies mid-run is not recognised as such, only one *started* run per store is refused, and a rebuild killed outright leaves a row to clear. Co-Authored-By: Claude Opus <noreply@anthropic.com>
Three of them recorded what the spy already recorded and differed only in what they refused, so the spy now takes the pages it will not save, whether it refuses deletions, and what it throws — the one thing a rebuild reads, since a wiki-database error ends the run where a rejected page does not. Names the deletion observer's running total for what it is, and moves the null observer to the test doubles it has always been the only user of. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A client reports an unreachable server by quoting the connection URI it tried, credentials and all, and a SPARQL store quotes the endpoint URL it posted to. Those messages were stored verbatim in the run records and logged verbatim per failed page, where update.php's report already redacted them. Lifts that redaction out of the update hook into the graph-database domain, where both paths reach it. Ending a run now also logs on the NeoWiki channel, which the script and the docs already promised: the run record cannot hold the exception class or its backtrace, and until now nothing did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Building it can fail on its own — a backend whose configuration will not resolve — and it was built as an argument to the executor, so after the run row already said the store was rebuilding. The throw escaped the executor's Throwable net, leaving that row Running for good, and both starting and resuming a rebuild refuse while one is on: the store was wedged. A factory failure now stops a run being recorded at all, which is what it is: nothing ran, so there is nothing to resume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resuming every store passes over the ones with nothing to continue, which conflated three states: a store already reconciled, one added since the last rebuild and never built, and one whose last run finished with pages it could not reconcile. The last two exited zero, so a scheduled --resume left a store holding none of the wiki and still reported success. The exception now carries the run it looked at, and only a run that succeeded with nothing failed counts as in sync — the same test the ordinary path already applied to a run it started itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An extension plugin naming itself after a configured store was dropped where the two sets were combined, without a word — while a plugin colliding with another plugin, and a configured store colliding with another store, both warned. The registry is now told the bundled names up front, so every collision comes out of the one path that reports it. A store called "neo4j" got the collision message meant for two entries colliding with each other, which told the operator to give one of them an explicit name — no help when the other one is a bundled backend. It now says what it is, and rejects the name in any casing, since a store called "Neo4j" reads as that backend wherever a name is written. A name too long for the run records to file the store under is refused the same way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Maintenance turns anything unparseable into 0, and the script rounded that up to 1, so a typo silently walked the wiki a page at a time. It now says what is wrong and rebuilds nothing, and the coordinator refuses an empty batch itself rather than trusting a parameter type only its analysed callers are bound by. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The closing line claimed not every page was reconciled whenever any store was left behind, including for failures that never reached a page — an unknown store name, a rebuild refused because one was already going. It now names the stores instead, which is what the operator acts on one at a time. A store's failure goes to stderr, so a scheduled rebuild reaches whoever reads that rather than only the log its output went to. And a page count is not something anyone can act on, so the failing page ids are named too, the first few of them, with the log channel holding the rest and the reasons. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each of these survived the suite unchanged: the cursor not advancing past a page that failed (which loops the executor over the same batch for good), a page skipped for carrying no Subject, an oversized error cut mid-character, the store being prepared only after the pages were projected, the two guards on resume(), and the wiki-database rethrow in the removal phase. Replaces an assertion that a resumed run carries no error — true after any transition, so it could not fail — with a RebuildRun test that says which transitions clear it and which keeps it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Uses core's batch-cursor idiom in the subject-page walk, which drops the repo-wide PHPStan ignore this PR added for every use of expr(), including the four already covered by the baseline. - Reads an unrecognised status or trigger back as no run, so a row written by a newer version cannot fatal in the middle of a rebuild. - Keys the SPARQL plugins by the store name they already carry, so the name comes from one place. - Drops exception properties nobody reads, hides the two lookups nothing outside the extension builds, and shares the one run repository the coordinator and its executor both need. - Adds a `make dbschema` target for regenerating the per-DBMS SQL, which the schema test now names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The extension guide never mentioned the name a graph database backend must now register under, or what happens to one that takes a name already held. Its failure paragraph still described a rebuild of everything at once. - The recovery from a killed rebuild was "clear the run's row", which named neither the table nor what to write, and throwing the row away would take the cursor --resume continues from with it. - --resume continues a run that failed; a run that finished having left pages behind is re-run, not resumed. The docs said only "if a run ends". - The single-store configuration example set a name, which the paragraph under it advises against. Also settles the ops docs on "graph store" where they mixed it with "backend", and gives both the same command form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cuts restatement and derivation from the prose the per-store rebuild added, without changing what any of it claims: - The store-naming sentence now reads in one direction instead of three. - The failure-reporting paragraph named the NeoWiki channel twice for the same fact. - The extension-facing failure contract dropped the clause explaining why the two failure kinds differ. - Backfilling a new SPARQL entry links to the command's own section rather than repeating it inline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rebuild rework renamed newSubjectPageRebuilder() away, leaving the recipe in docs/extending/extending.md calling a method that no longer exists. The import variant already had the semantics an extension-triggered refresh needs (every store, hook-path failure isolation), so it takes the general name back and both callers share it. The refresh's failure contract is now documented as isolating, matching what it does. Also closes the text-review flags on the rebuild docs: the kill -9 recovery statement is now executable as written (store-scoped, no run-id hunt), the mirrored-projection note asks for one explicit name rather than two, the 255-byte name cap is stated, and the duration bullet names --resume and --store as the remedies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rs (#1236) * Make a rebuild batch the unit of work, and close three gaps A rebuild ran as one uninterruptible walk driven by the maintenance script. Splitting a batch out as its own step is what lets the same code be driven a batch at a time from elsewhere, and it closes the three things the scoped rebuild left open: - The removals are checkpointed like the projections. Both phases now walk by keyset cursor, and the run records which phase that cursor belongs to, so a rebuild interrupted between them continues with the removals instead of reprojecting the whole wiki first. - Starting a run takes the store's advisory database lock across the check for an active run and the row that check guards, so two callers starting at once produce one run and one refusal. - A batch of two or more pages that fails in its entirety is read as the store having gone rather than as a wiki of unprojectable pages: the run ends with its cursor left at that batch, so resuming retries it. A batch of one is exempt, or one broken page would wedge resume in a retry loop. The status and trigger enums gain the values the surfaces in the commits after this one file runs under, and the run carries the times the record already held. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Run graph rebuilds on the job queue A rebuild could only be run by someone sitting at a shell for as long as it took. This files one as work instead: a job per batch, each queueing the next until the run is done, so a rebuild can be asked for from a web request and outlive it. The run record is the state, not the queue. A job carries only which run to advance, and reads that record before doing anything, so a job for a run that has since been cancelled or finished does nothing — which is what lets cancelling be a single write, with no reach into the queue, and what makes a retried job safe. Rebuilds started here and run in a maintenance script are the same runs, sharing one record per store, so cancelling reaches a rebuild wherever it is running and only one may be under way at a time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Report how far each graph store is from the wiki Nothing said whether a store's contents still matched what the wiki described. Editing a Mapping page is the case that matters: it changes what every mapped page's graph should contain, and nothing reprojects those pages, so the store keeps serving the old vocabulary with no sign that it is doing so. Each store is now reported as never built, stale, or in sync, derived from the run records and the wiki rather than stored: anything stored would be one more thing to keep true, and would be wrong exactly when it mattered. Staleness is measured against when the last finished rebuild started, not when it ended, because a Mapping edited mid-rebuild leaves the pages that rebuild had already passed projected under the old rules. Only a store holding an ontology projection can go stale this way. The native projection and a backend holding no RDF have no editable definition, so once rebuilt they stay as current as the per-edit projection keeps them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Report and rebuild graph stores over the REST API A rebuild could only be asked for from a shell, and how far a store was from the wiki could only be worked out by reading run rows. Three endpoints now answer both, so a rebuild is something the wiki can be asked for rather than something only its host can do. They are gated on a new neowiki-admin right, granted to administrators. What they report and change is the installation's own machinery, so no page's permissions could stand in for it, and no OAuth grant maps to it — this is not something a delegated application should acquire by asking for a category of access. A store is described by what it holds and how far that is from the wiki, never by how it is reached: the endpoint URL and access token it is configured with stay out of every response, because being allowed to rebuild a store is not being allowed to read the credentials for it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add Special:GraphStores for reporting and rebuilding stores Rebuilding a graph store, or finding out whether one needed it, meant shell access to the server. An administrator can now see and do both from the wiki. Rendered on the server and read from the run records, so what it shows is what a rebuild has actually recorded rather than a copy of it kept in the browser. A rebuild started here runs on the job queue, so the page comes back at once and progress appears on reload; there is no auto-refresh, so watching one costs a wiki nothing. Every action posts and redirects back, which is what makes reloading to watch a rebuild advance safe: it repeats a read rather than the submission that started the rebuild. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Optionally rebuild a store when its Mapping changes Editing a Mapping page changes what every mapped page's graph should contain, and reprojects nothing: the store keeps serving the old vocabulary. That is the gap this whole thread exists to close, and $wgNeoWikiAutoRebuildOnMappingChange closes it without anyone having to notice. Off by default. Such a rebuild reprojects every page carrying a Subject into that store, which on a large wiki is substantial work an administrator should choose to spend; while it is off, the store shows up as stale on Special:GraphStores with the rebuild a click away. A store already rebuilding is replaced rather than left to finish: a run begun under the old Mapping has projected part of the wiki under rules that no longer apply, so finishing it would leave the store half in each vocabulary with nothing recording that it had. Ending the old run and filing the new one happen under the store's start lock, so nothing can slip a third rebuild in between. The work is deferred past the change's own transaction. A rebuild cannot be started inside one — taking the start lock flushes the connection's snapshot, which a transaction with writes pending may not do — and an edit must not wait on a lock or a queue to be saved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Document background rebuilds and the right they need Covers what an administrator now has: the page and the endpoints, the job-runner caveat that decides whether a large rebuild finishes this week or next, and that rebuilds started anywhere are the same runs. Also replaces the manual SQL for releasing a killed rebuild, which cancelling on the page now does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Stop a batch writing over the run it no longer owns A batch read the run, spent minutes projecting, then wrote back what it got through — carrying the status it had read. A cancellation landing in that window was written straight back over: the admin was told the rebuild had stopped and it carried on. Worse, an automatic rebuild restarting a store mid-batch left the old run resurrected alongside the new one, both projecting into the store the design exists to give one rebuild at a time. A batch's write now only lands while the records still have the run going; one ended in the meantime keeps the status that ended it, and the batch's work is dropped rather than written over it. Alongside it, from reviewing the same code: - Timestamps are read back as MediaWiki timestamps rather than in whatever format the database stores them in, so staleness is not decided by comparing two different formats as strings. - A batch is read as store death only when every page the store was actually offered failed. One page the wiki dropped between the walk and the batch used to spare a dead store its verdict. - The batch totals reach the observer as something to call rather than as numbers, so a background rebuild no longer counts the whole wiki twice per batch to tell a reporter that reports nothing. - A batch is queued after the run row it names commits, not before. - A continuation that cannot be queued ends its run, like the first batch already did, rather than leaving it recorded as going with nothing going. - Special:GraphStores checks read-only mode, catches every failure its message claims to cover, and logs the reason that message points at. - Losing the start lock is a 409 rather than a 500, and a run's error is no longer serialized into REST responses — it can quote the endpoint a backend could not reach, which is for whoever reads the records. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Assert the start lock is let go of without lockIsFree MediaWiki removed that method after 1.46, and it was the only thing these two tests used it for. What they are really about is that a store whose last rebuild was filed, or refused, can start another — which is a thing the lock's callers can observe. Also records why the restriction is passed to the SpecialPage constructor, deprecated on 1.46, rather than declared the way that release suggests: on 1.43 the permission check reads the property the constructor sets and not getRestriction(), so overriding that would leave the page open to everyone there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Stop a stale read resurrecting a cancelled rebuild A batch's conditional write reported success by reading the row back, but that read is served from the batch's own transaction snapshot — which a job runner opens before the batch, and which therefore predates the cancellation the check exists to notice. The write was correctly refused and reported as landed anyway. Whether it landed now comes from the row count the write matched, and what ended the run is read under a lock so another connection's commit is visible. Taking a queued run up was an unconditional write on top of that, so a cancellation committed before the first batch was written straight back over, resurrecting a run the admin was told had stopped. It goes through the same conditional path, and a run something else ended stops there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Stop Special:GraphStores running what its query string carries The store name it reports back was substituted into the outcome message as wikitext, which the message transform then ran: a link handed to an administrator executed whatever it carried, on a GET. It is now substituted as plain text, after the transform. The outcome itself decided a message key, so the query string chose which message the page showed. Only the outcomes this page redirects with are reported now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Recognise a dead store in both phases of a rebuild, and only there A store refusing every removal ended its run Succeeded: the removal phase had no notion of a store that had gone, so the store was reported in sync while every page the wiki deleted stayed queryable in it, out of reach of --resume. Removals now report failure the way projections do, and a whole batch of them failing ends the run with its cursor rewound to that batch. The test the heuristic never had also showed it reading two things as a dead store that are not one. A short last batch is where the walk's permanently unprojectable pages collect, and a batch almost all of whose pages the wiki has since dropped tells the store's answer from one page. Both left --resume retrying pages that will never work, so a batch now has to be full and to have reached the store with at least two pages. The pages of a batch the run ends on are no longer logged or reported as pages that failed. That batch is retried from where it began, so nothing should be left recording them as settled failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Stop counting the whole wiki once per rebuild batch Every batch handed its observer a way to count how many pages there are in total, and the one observer that wanted a denominator called it — turning a rebuild into a full-wiki count per batch. The script counts each total once, for the store it is about to rebuild, and the observer is left reporting what a batch did rather than how much there is of it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cancel a rebuild without reading it first Cancelling read the store's active run and then wrote it back cancelled. A run that reached the end of the wiki in between had that recorded over: the rebuild had reconciled the wiki, and the store was then reported as never built. The records now end the run in one conditional write, and say whether there was one to end. The cancelled run also keeps whatever progress the records hold rather than whatever the caller last read, so a batch that advanced it meanwhile is not rolled back. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Leave a rebuild somebody started out of the automatic one's reach An edit to a Mapping page cancelled whatever rebuild its store had going and queued its own. With automatic rebuilds enabled, that let anyone who may edit a Mapping take away a rebuild an administrator had started and was waiting on, and start their wait over from the beginning of the wiki. An automatic rebuild now replaces only another automatic one. A run started from the script, the page or the API is left to finish, and said so on the log. The store is reported stale once it ends, so the changed Mapping is not lost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Report a store whose Mapping was deleted as stale Deleting the Mapping page that defines a projection left every page projected under it carrying that vocabulary, with nothing reprojecting them — but the store read as in sync, because the last change was looked for on a page that no longer exists. The deletion log now says when, for a page that is gone. A projection nothing ever defined still reads as never changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Refuse to start or cancel a rebuild while the wiki is read only Both endpoints write — a run record, and the jobs that carry it — and neither asked whether the wiki was accepting writes. They answer 503 with the reason now, rather than leaving the write to fail somewhere further down with a half-filed rebuild behind it. Special:GraphStores already checked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Say what the batch-push catches actually reach Both comments read as though every failed push were caught there. A web request's push is deferred past the response, and what runs it logs failures rather than raising them, so what these see is a job the queue refuses before the deferral plus everything pushed under the command line. A continuation the queue will not take now has the test the first push already had: it ends the run rather than leaving it recorded as going with nothing carrying it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Redact the credentials a store carries outside its connection URI Only the userinfo of a connection URI was removed, which is how a Bolt driver quotes credentials. A SPARQL store carries them in the query string or in an Authorization header instead, and those messages are kept just as long — on a terminal, in deployment logs, and in the rebuild run records. Access tokens, API keys, passwords and bearer tokens are removed too now, leaving what names them so the message still says how the store was being authenticated to. Special:GraphStores also declares that it writes, so MediaWiki routes it to the primary database rather than a replica. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Agree with the script on what a store left out of sync looks like A rebuild that ran to the end but could not reconcile every page left the maintenance script exiting non-zero and the page saying "In sync" about the same records. The store holds a copy of the wiki with holes in it, which is where a store nothing has ever rebuilt stands, so it is reported the same way. Rebuild batches are also filed as deduplicable. The queue only drops a batch matching one still waiting to be claimed, so a run's serial chain is unaffected and what is dropped is the second copy of a batch that got run twice — which is where a run would otherwise fork into two chains advancing one cursor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Close the test gaps the graph-store surfaces were left with The refusals asserted only their status code, so a handler that refused and then went on to write would have passed; they check the records now too. The CSRF path was never driven at all, only stubbed past — the real validator refuses both mutating endpoints in a test. A batch filed for a run nothing exists under asserted the absence of a run nothing had created, which no change could break; it asserts what the batch projected and what it said instead. Stale is the one store state an operator reaches without a rebuild having failed, and neither the page nor the REST serializer had ever rendered it. Both do now, over a Mapping edited after the rebuild that read it. A resume that picks up partway through the removals also has a test: the removals already made are not made again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Tidy the edges the graph-rebuild work left - A page called Mapping:Native no longer sets an automatic rebuild going. The native projection is defined by NeoWiki's own code, which is already why a store holding it never reads as stale. - A rebuild batch carries only the run it advances. The store came along in the job parameters as a second copy of what the run record already says, which a job filed for one store could have used to advance another's run. - A job whose parameters are not what this version files reads as run 0 rather than fatalling on a missing key. - The log tells a rebuild started from the wiki to rebuild the store again, rather than naming a maintenance-script option whoever started it cannot reach. - A Mapping edit on a wiki that has not asked for automatic rebuilds registers no deferred update at all, and one that has cannot take the edit's deferred work down with an unresolvable backend. - Removed the stray blank line in extension.json. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Take out what the graph-rebuild work does not use - GraphStoreStatusLookup::getStatus() and RebuildProgress::getCursor() had no production caller. - The graph-store endpoints share an access rule and an error shape; building a serializer is not something they share, only something each of them does. - The rebuild coordinator's batch size no longer defaults on the factory, so a production caller says which size it means rather than the parameter being there for tests to override. - The start lock is a port the rebuild use case depends on, so it and its exception sit with the use case rather than in Persistence, giving the REST handler one namespace to catch its refusals from. The implementation stays. - NeoWikiExtension::getStoreProjections() is private; what a Mapping change looks at is its own accessor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Share the test helpers the rebuild suites were each keeping Creating pages that carry a Subject, reading back what a store was given, deleting a page and flattening a log buffer were written out once per test class. They are on the shared base class now. Two suites keep their own page deletion, overriding rather than repeating it: theirs also runs the deferred updates the projection they watch happens in. The one that returns the status its test asserts on is named for that instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Say in the docs what a rebuild now does about its own failures - `--resume` continues in whichever of the rebuild's two phases it stopped in. - A store that stops answering ends the run, with the size at which that stops being recognisable. - Cancelling on the page or over the API also stops a rebuild the script is running. - A rebuild queued on a wiki whose job runner does not load NeoWiki stays queued, blocking the next one, until it is cancelled. - A deleted Mapping page makes its stores stale, as an edited one does. - The rebuild endpoints answer 503 while the wiki is read only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Tighten the prose the background-rebuild work added - The rebuild's two halves are stated once, where `--resume` needs them, rather than restating what the section opener already said. - Cut the reasoning behind a whole failed batch ending the run, and behind one rebuild per store blocking the next: both restate the contract above them. - The maintenance page links the REST reference for the graph-store endpoints rather than repeating paths nothing checks for drift. - `neowiki-admin` allows viewing rather than reporting, and a Mapping edit rather than "it" is what cannot take a hand-started rebuild away. - The stale state reads "changed at <time>", and a rebuild that could not be queued is "a" rather than "the" rebuild. - qqq: deleting a Mapping page makes its stores stale as editing one does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make the graph-store UI text match what the page can do The cancelled-outcome message promised a resume only the maintenance script offers; it now states what actually happens. The state label for a store no run ever fully reconciled read "Never built" beside a listed last rebuild; it now reads "Not reconciled", which covers both ways of getting there. Also: the Stale docblock said finished where the code compares against started, the auto-rebuild docs state what happens to a Mapping edit during a hand-started run, and the REST doc gives the rebuild endpoints' refusal order straight. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Refusal orders, HTTP codes conventions already imply, security assurances addressed to a reviewer, tiny-wiki edge cases, and restatements of adjacent sentences all go. The graph-store gating note shrinks to its one fact; CSRF was already covered file-wide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A text-review pass over the reader-facing rebuild docs: drop what the named reader does not act on — convention-implied status codes, UI and job-queue narration whose home is elsewhere, derivation clauses, and a cross-file restatement of the neowiki-admin default, now a link. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A batch every page of which failed was read as the store having gone, and the run was rewound to that batch so resuming retried it. A store that is up and holding a run of pages it will not take — a family of bulk-imported pages too large for it, say — refuses a batch exactly the same way, and counting the failures cannot tell the two apart. Read that way, such a window is absorbing: the cursor is rewound to the batch, so --resume walks back into the same pages and stops there, and a fresh run from cursor 0 reaches it and stops too. Nothing behind the window is ever projected. Only a --batch-size wide enough to hold the whole window alongside one page the store takes gets past it, which is the maintenance script only: every background path passes a fixed batch size. The batch now decides nothing on its own. When every page the store was offered failed, the store is asked whether it is still there, which is what the plugin contract already offers — initialize() is idempotent, callers are asked to be free to repeat it, and a rebuild already reads a store whose initialize() throws as one it cannot reach. A store that answers means the pages are at fault, so they are counted and reported and the walk goes on; one that cannot be opened ends the run as before. It costs one round trip per wholly failed batch, which has just cost one failed round trip a page. Reporting the pages follows from the same change: a rewound batch still records nothing, so before this the page ids of a window like that reached neither the log nor the observer, and an operator got no lead on which pages were at fault. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
resume() reopened the run with started(), which copies every field but the status — including the trigger that says what filed it. An operator picking up a rebuild from a shell therefore left it recorded as the automatic run it started life as. That field is what refuseWhenStartedByHand() reads to decide whether an automatic restart may take a rebuild away from someone. Read on a resumed run it answers about how the run began rather than about who is driving it now, so a Mapping edit cancelled the operator's walk mid-run and filed a replacement, and the script exited non-zero reporting a cancellation. The route into it is the one the docs endorse: cancel on Special:GraphStores, then --resume. resume() now takes the trigger driving it and stamps it on the reopened run, and the batch writes carry the trigger so it survives them. describeHowToContinue() reads the same field, so a resumed run now also names --resume rather than telling a shell operator to go to Special:GraphStores. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"A whole batch failing ends the run, and --resume retries that batch" is only half the story: a batch shorter than --batch-size — the tail of any walk, and the whole of a wiki smaller than 200 pages — is read as failing pages instead, so the run ends succeeded and --resume is refused. The page said what to do about that five lines earlier without ever connecting the two. The paragraph now names both outcomes the exit code covers and which recovery each takes, and follows the store-reachability check the rebuild now makes before reading a whole failed batch as the store having gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The documented statement filters on nwrr_status = 'running', so it matched nothing for a run stranded as queued — which blocks the store exactly as hard, with no reaper or TTL to release it, leaving the store unrebuildable for good. Queued is not an exotic state to be stranded in. A background rebuild commits its run row in the request's transaction and pushes the first batch in a post-send deferred update, so the machine going down between the two — the trigger this paragraph already names — leaves one, as does the queue losing the job. Cancelling from Special:GraphStores already covered both; only the offline route did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The plugin contract listed update.php and the rebuild script and said "both paths call it every time". A rebuild started from the wiki runs a job per batch and opens the store on each one, and the rebuild now also opens it to ask whether a store that refused a whole batch is still there — so an initialize that is merely idempotent is no longer enough, and "ends the run before a page is read" is false for every batch after the first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A store name is how a scoped rebuild is addressed and what its run records are filed under, so the same two rules have to hold whether the name comes from NeoWikiSparqlStores or from an extension. Only the config path applied them. A plugin could therefore register a name longer than the run records can hold. MySQL cuts it to fit on the insert — silently, since strict warnings are off by default outside development — while every lookup passes the uncut name, so the store's rebuilds never find their own records: the concurrent-run guard, cancel, resume and the Special:GraphStores state all read as if nothing had ever run. It could also take a bundled backend's name in another casing. The config path reserves "neo4j" however it is cased, because a store called "Neo4j" reads as the bundled backend wherever a name is written or reported; the registry compared as array keys, so it accepted one and stood it beside the bundled backend. Both rules move to GraphStoreName, which the two paths now share. Taking a reserved name and repeating a taken one no longer give the same warning, since the ways out differ. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GraphStoreName::MAX_LENGTH and the nwrr_store column width were two declarations of one number, with nothing holding them together: narrowing the column alone would leave names the rules accept but the records cannot hold whole, and every lookup for such a store would then match nothing. The check reads the width off the abstract schema rather than off a generated per-DBMS file, because only MySQL materialises it — postgres declares TEXT and sqlite BLOB — and CI installs sqlite, so a test that round-tripped a long name through the database would pass there whatever the JSON said. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both executor phases re-throw a request timeout and a wiki-database error by the same clause, but only the wiki-database half was ever exercised. Dropping TimeoutException from either union left the whole suite green, so nothing recorded that a run must end where a request runs out of time rather than count the page and walk on into pages that would fail the same way. Two tests mirroring the wiki-database pair, one per phase. Both fail with that type removed from the clause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The automatic rebuild fired on every revision of a Mapping page, and protecting one — or unprotecting it, or changing when that expires — inserts a revision carrying the content of the one before it. Nothing about the projection had changed, but the store's rebuild was cancelled and a replacement filed from the first page, so an admin protecting a Mapping mid-rebuild threw away the whole walk to reach the graph the wiki already had. With no rebuild in flight it queued a needless full one instead. A revision now has to say something new to count as a definition change, which is its content differing from its parent's. Page moves already were genuine changes: that hook fires with the destination page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t is Deleting a Mapping page was a definition change; putting it back was not. So a store rebuilt while the page was gone — built with no projection for it at all — went straight back to reporting In sync the moment the page returned, and the automatic rebuild that a save or a delete queues did not fire either. The one signal that something was wrong was cleared by the action that made it repairable. Two halves, both about the same event. PageUndeleteComplete now queues the rebuild that a save or a delete does. And a projection's last change is now the later of its page's last revision and the last time that page was deleted or restored, because a restored revision keeps its original timestamp: read off the page alone, a projection put back after its stores were rebuilt without it looks untouched since before the deletion. This reverses what MappingPageChangeTimeLookupTest pinned deliberately — that a restored page reads as changed when it was last edited. That holds only for a store rebuilt before the deletion; one rebuilt during it is as far from the wiki as a store rebuilt before an edit, which is the case the staleness report exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two stores can hold the same projection — one endpoint mirroring another, or two vocabularies of one Mapping — and the automatic rebuild started them all inside a single deferred update, so they shared one transaction round on one connection. Starting a rebuild takes the store's advisory lock, and taking a lock flushes the connection's snapshot, which a connection still holding the previous store's writes may not do. Every store after the first therefore threw "Cannot flush pre-lock snapshot", was caught and logged, and was left on the old vocabulary while the first rebuilt. Each store now gets its own deferred update. The rebuilder says which stores hold a projection; the hook, which is where the deferral decision already lives, files one update per store. The log line names the store it could not rebuild rather than the whole set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The registration-time gate accepts any entry carrying a usable updateUrl, which mirrored the factory until the factory learned to drop an entry for its name as well. A config whose only usable entries are dropped as reserved or over-long therefore registered the route with no plugin behind it, and a query reached requireFirstSparqlPlugin() and came back a JSON 500 instead of the 404 the gate exists to produce. The gate now derives each entry's name the way the factory does and applies the same two rules. A duplicate name is not among them: the first entry claiming a name keeps it, so a duplicate can never empty the stores. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cancelling a background rebuild deliberately does not reach into the job queue, because a job for an ended run does nothing. That held while nothing reopened a run id — until --resume did. So: cancel a background rebuild on Special:GraphStores, resume it from a shell as the docs say to, and a batch still queued from before the cancel finds the run going again and advances it alongside the script. Neither knows about the other, and every batch is a read-modify-write conditioned only on the run being active, so they overwrite each other's phase, cursor and counters — the wiki gets walked twice, and a stale write landing over the deletion phase sends the whole walk back to the start. Only the maintenance script resumes, and it is also the only caller that never queues a batch, so a queued batch that finds its run being driven from a shell has nothing left to do. Recording who is driving a resumed run is what makes that answerable, which the trigger now carries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebase repair, not new behaviour. Master's #1226 renamed SubjectPageRebuilder to PageRebuilder and moved undelete handling from RevisionUndeleted to PageUndeleteComplete, which this branch had also added a handler for. The two handlers are merged: the page is reprojected and, when it is a Mapping page, the stores holding its projection are rebuilt. The graph rebuild still walks its own subject-page enumeration; moving it onto master's every-page lookups is the next commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#1226 made every page a projected page and replaced the enumeration this rebuild was written against: SubjectPageIdsLookup and its deleted twin gave way to PageIdsLookup and DeletedPageIdsLookup, which walk the whole wiki. Two enumerations of the same wiki cannot both be right, and the rebuild was reconciling a subset of what every other path projects — so a rebuilt graph no longer matched a saved one. The rebuild now walks master's lookups, and its Subject-scoped duplicates are gone. Both contracts gain what a resumable rebuild needs and an unbounded walk does not: a bounded read that returns one batch, so the run can record how far it got and continue there rather than part-way through a generator, and a count to report progress against. The deleted-page read is the one place the two designs genuinely disagree. master's getDeletedPageIds() pages by ar_id because the archive has no index on ar_page_id, and sorting it per batch is what that avoids. A rebuild resuming from a page-id cursor cannot start from an archive id, so the bounded read orders by page id and pays that sort — the cost of being resumable, and the same cost this branch's own deleted-subject lookup paid. The unbounded reader keeps its faster walk. Tests for the widened scope are not updated yet: a test that creates one page and asserts on everything projected now also sees the pages its fixture made. master bounds those assertions with a helper in its own rebuild test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every page is projected now, so "it carries no Subject to project" is not why a rebuild skips one — a page without Subjects is projected like any other. master's PageRefreshOutcome distinguishes a missing revision from unreadable Subjects from unreadable page properties, and carries the reason for each; the rebuild's log now says which of them it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch moved several helpers onto NeoWikiIntegrationTestCase; master has tests that declare their own with the same names, and PHP will not let a subclass narrow an inherited method. Every one of these is fatal at load, so the whole suite dies before a single test runs. deletePageByName does what the inherited one does, so the copy goes. The other two are different things that happen to share a name — one takes no JSON, the other requires a main Subject — so they keep their behaviour under names of their own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rebuild reconciles the whole wiki now, so a test that creates one page and asserts on everything projected also sees the Schema page its Subjects need — created first, so it sorts ahead of the pages the test names and fills the first batch alongside them. Counts gain FIXTURE_PAGES and lists are bounded by the first page the test made, both from the shared base class. Where a test needed a wholly refused *full* batch to exercise the store-death rewind, the refused pages move up one so the fixture's page is not in it. Two tests changed meaning rather than arithmetic: - A page carrying no Subject is projected like any other, so it is no longer skipped and no longer proves the walk steps past a page it did nothing with. It asserts what now happens instead. - Saving a Mapping page projects it into stores the test points at unreachable endpoints, so the write path logs errors that say nothing about whether a rebuild was left alone. That assertion now looks only at errors about the rebuild. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
master renamed DatabaseDeletedSubjectPageIdsLookupTest to DatabaseDeletedPageIdsLookupTest, so git merged the two versions into one file and the rebase resolution kept this branch's tests without master's newLookup(), leaving every one of them erroring on an undefined method. Its remaining Subject-scoped assertion goes too: the lookup reports every page the wiki lost, which is what the test's own name says it checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rework moved the rebuild onto the whole-wiki enumeration but not the prose describing it: the script's --help regressed to master's pre-#1226 "every Subject" description, the executor's class docblock and a dangling storeLooksGone reference kept the old scope, the auto-rebuild setting's description said "every page carrying a Subject" and omitted restoring, the Stale stores paragraph said the same while its own page's opening said every page, and the script's totals were still named totalSubjectPages. An operator with subjectless page-property pages would read all of it as those pages being outside rebuild coverage. The test base class also carried a doubled property docblock whose stale first half declared the pre-rework list type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The executor's bounded getDeletedPageIdsAfter() replaced the maintenance script's removeDeletedPages() as the deletion walk, leaving master's unbounded ar_id-paged generator with no production caller — only its interface slot, implementation, test double and tests. Keeping a second enumeration of the same set invites the next caller to pick the one whose ordering the rebuild's cursor cannot resume from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fixture-page shift was applied to the store-gone tests but not the store-alive ones, and the fixture's Schema page — lowest page id, so first into the first batch — succeeded inside the batches they refuse. No batch wholly failed, the probe was never consulted, and all three passed against an executor that failed the run without asking the store: dropping the probe kept the whole suite green. The refused pages move up one so the probed batch is full again, and the short-batch test now uses a store that would fail the probe, so only the full-batch guard keeps its run succeeding. Verified against both mutations. The rewrite of the no-Subject test also deleted the only path driving the executor through a rebuilder that returns a skip outcome. A stubbed rebuilder returning SkippedMissingRevision pins that branch: nothing processed, nothing failed, nothing saved, the reason logged, and the walk moving on to the deletion phase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Protecting or unprotecting a Mapping page inserts a revision carrying the content of the one before it, with a fresh timestamp. The auto-rebuild hook already ignores those, but the staleness display read the page's latest revision time, so Special:GraphStores reported every store holding the projection as stale over an action that changed nothing — found live in a browser test. The lookup now walks back past revisions whose content hash matches their parent's, in practice a few protection entries at most. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alistair3149
force-pushed
the
graph-rebuild-scoped-v2
branch
from
August 5, 2026 21:17
3a3b255 to
4d40d09
Compare
alistair3149
marked this pull request as ready for review
August 5, 2026 21:48
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #1233, rebased onto master and reworked onto the page enumeration #1226 introduced. #1233 stays open as the record of the original design and its review; close it once this is accepted.
CI is green: 2,670 tests across MW 1.43–master, PHP 8.3–8.5.
Why this exists rather than a rebase of #1233
#1233 predicted this: "whichever merges second needs rework, not conflict resolution." #1226 merged first, and it did not just rename things — it changed what a rebuild reconciles:
PageIdsLookup::getPageIds( $afterPageId ): iterableSubjectPageIdsLookup::getSubjectPageIdsAfter( $after, $limit ): arrayDeletedPageIdsLookup::getDeletedPageIds(): iterablegetDeletedSubjectPageIdsAfter( $after, $limit ): arrayPageRebuilderSubjectPageRebuilderResolved mechanically, the tree would have held two enumerations of the same wiki and a rebuild that reconciled a subset of what every other path projects — so a rebuilt graph would no longer match a saved one.
What changed
The rebuild walks master's lookups; the four
Subject*duplicates are gone. Both of master's contracts gain what a resumable rebuild needs and an unbounded walk does not: a bounded read returning one batch, so a run can record how far it got and continue there rather than part-way through a generator, plus a count to report progress against.This PR also carries the 13 review-fix commits that were #1250, folded in before the rework so the same conflicts were resolved once rather than twice.
Decisions worth a look
getDeletedPageIds()pages byar_idbecausearchivehas no index onar_page_id, and sorting it per batch is exactly what that avoids. A run resuming from a page-id cursor cannot start from an archive id, so the new bounded read orders by page id and pays that sort — the cost of being resumable, and the same cost Scope graph rebuilds per store, batched and resumable, with run records #1233's own deleted-subject lookup already paid. master's unbounded reader keeps its faster walk. The alternative is threading a second cursor concept throughRebuildProgressand the run record.PageUndeleteCompletehad two handlers — master added one for reprojection, Address the review findings on the scoped graph rebuild #1250 added one for Mapping changes. Merged into one that does both.Refreshedoutcome, which is now false: such a page is projected like any other. It reportsPageRefreshOutcome::skipReason()instead.What the tests needed
The scope change reached further into the suite than the production code did, and all of it was rebase damage rather than defects in the original work:
FIXTURE_PAGES; lists are bounded by the first page the test made, via a helper on the shared base class.testAPageWithNoSubjectLeftToProjectIsSkippedAndWalkedPastlost its premise and now asserts what happens instead. And saving a Mapping page projects it into stores the test points at unreachable endpoints, so an assertion that the error log was empty now filters to errors about the rebuild.NeoWikiIntegrationTestCasethat master's tests also declare, and PHP will not let a subclass narrow an inherited method — the suite died before a single test ran. One was a true duplicate; two were different things sharing a name.DatabaseDeletedSubjectPageIdsLookupTest, so git merged both versions and the resolution kept this branch's tests without master'snewLookup(). Its remaining Subject-scoped assertion went too: the lookup reports every page the wiki lost, which is what the test's own name says it checks.Still open for review
Two semantics questions the suite passing does not settle:
wasOfferedToTheStoreunder every-page scope. It exists so a page the walk found but could not offer does not count toward "every offered page failed" in the store-death probe. With no-Subject pages now offered, the unoffered set shrinks to unreadable pages, soMIN_BATCH_SIZE_FOR_STORE_DEATHand the probe's guard deserve a fresh read.SkippedUnreadableSubjects— the exact case the hook path warns about, telling the operator to run this script) is skipped at info level, not counted, and the run reports "in sync" with exit 0 while the store provably lacks that page. Counting unreadable skips as failed, or carrying a skip count on the run record, are the options; both change semantics, so this is a design call rather than a patch.Verification
make csclean; CI green across MW 1.43–master on PHP 8.3–8.5 (2,670 tests, 7,197 assertions).AI-authored — Claude Code,
Opus 5 (1M context); rebase, rework and review fixes in-session; not yet human-reviewed.