Address the review findings on the scoped graph rebuild by alistair3149 · Pull Request #1250 · ProfessionalWiki/NeoWiki

alistair3149 · 2026-08-05T00:21:16Z

Addresses the findings from a review of #1233, one commit per finding. Targets graph-rebuild-scoped so it can be merged into that PR before it goes to master.

The review found 15 issues; one was refuted on validation and one is deferred. What is fixed here:

Behaviour

A run of page-specific failures is no longer an absorbing stop. storeLooksGone() inferred store death from counts alone and rewound the cursor, so a contiguous window of pages a healthy store refuses stopped every attempt at the same place — --resume and a fresh run both walked back into it, and nothing behind it was ever projected. The store is now asked whether it is still there, via the initialize() the plugin contract already offers for exactly that. A store that answers means the pages are at fault, so they are counted, reported and walked past. This also fixes the failed page ids never reaching the log, since a rewound batch reports nothing.
A resumed run records who is driving it. resume() copied the trigger, so refuseWhenStartedByHand() read an operator's resumed run as unattended and a Mapping edit cancelled it mid-walk — via the recovery route the docs endorse (cancel on the page, then --resume).
A stale queued job no longer drives a run the shell resumed. Cancelling deliberately does not reach into the queue because a job for an ended run does nothing — which held until --resume reopened a run id. Only the script resumes, and it never queues, so a queued batch that finds its run being driven from a shell stops.
Each store sharing a projection starts in its own transaction round. Started in one deferred update, every store after the first threw Cannot flush pre-lock snapshot and was left on the old vocabulary.
Protecting a Mapping page no longer restarts its rebuild. Protect, unprotect and expiry changes insert a content-identical null revision; a revision now has to say something new to count as a definition change.
Restoring a Mapping page counts as a change. It queued no rebuild and flipped the store straight back to In sync, so a store rebuilt while the page was gone reported as reconciled.
An extension's store name is held to the rules a configured one meets. addPlugin() applied neither the 255-byte limit nor the case-insensitive neo4j reservation, so a plugin could take a name whose run records it could never find again (truncated silently on production MySQL), or stand as Neo4j beside the bundled backend.
The SPARQL query route registers only when a store will back it. A config whose usable entries are all dropped for their names registered the route with no plugin behind it, answering a query with a 500 instead of a 404.

Tests

The request-timeout half of the executor's catch ( TimeoutException | DBError ) was untested in both phases; dropping the type left the suite green.
Nothing linked GraphStoreName::MAX_LENGTH to the nwrr_store column width. The new check reads the width off the abstract schema, because only MySQL materialises it and CI installs SQLite.

Docs

maintenance.md over-generalised a whole batch failing, and its offline-recovery SQL matched nothing for a run stranded queued — the state the trigger it names actually leaves.
extending.md named two of the three paths that call initialize(), and its throw-timing sentence was false for every background batch after the first.

Two further doc updates follow behaviour changed here, and are folded into the commits that caused them rather than standing alone: staleness and auto-rebuild now name restoring a Mapping page alongside editing and deleting it, and the plugin-facing name rules in extending.md now match the ones installation.md states for configured stores.

Worth a look

The store probe is the one design decision here. Classifying the failures or bounding the rewind were the alternatives; probing needs no schema change, no new state, and no guesses about what a backend throws.
One commit reverses a deliberate, tested decision. MappingPageChangeTimeLookupTest pinned that a restored Mapping 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.
Add frontend core subproject #2 is fenced by trigger rather than by an epoch column. Only the CLI reopens a run id and only the CLI never queues, so the trigger already distinguishes the attempts. An epoch column would be the general fix if background resumption is ever added.

Not addressed

Special:GraphStores emits cdx-button but loads no Codex styles, so the buttons render as plain submit inputs. Left out at @alistair3149's request for a separate pass.
A Mapping page rename was reported and refuted: NS_NEOWIKI_MAPPING is declared "movable": false, so no core move path can rename one.
Scope graph rebuilds per store, batched and resumable, with run records #1233's own description is stale — it lists the Run graph rebuilds in the background with admin, API, and auto triggers #1236 work as deferred and names three "known gaps" that no longer match the code.

Verification

make cs clean, CI green across MW 1.43–master on PHP 8.3–8.5. Every behavioural fix has a test that was confirmed to fail without it; the two test-only commits were confirmed against the mutation they exist to catch. Local full-suite runs hang on this box for environmental reasons (the unmodified branch hangs identically), so per-file runs were used locally and CI covers the whole suite.

_{AI-authored — Claude Code, Opus 5 (1M context); review, validation and implementation in-session; each fix verified against its negative case; not yet human-reviewed.}

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>

alistair3149 · 2026-08-05T18:29:58Z

Landed in graph-rebuild-scoped by fast-forward rather than through this PR, so GitHub closed it as merged. All 13 commits are in #1233 with their history intact.

Folded in first so the rework onto master's new page enumeration (#1226 renamed SubjectPageIdsLookup/SubjectPageRebuilder and changed the scope to every page) happens once over the complete content, instead of resolving the same GraphRebuildExecutor conflicts twice. Review of these fixes continues on #1233.

alistair3149 · 2026-08-05T19:04:19Z

Correction to my earlier comment: these commits are no longer in graph-rebuild-scoped. #1233 has been rewound to its pre-fold state so it stands as an unmodified record of the original design and its review.

The 13 commits live on in #1254, rebased onto master along with the rest of #1233. GitHub still shows this PR as merged because it was, briefly — the diff and discussion here remain the record of the review fixes themselves.

JeroenDeDauw · 2026-08-05T23:12:31Z

+its run records under. Pick a stable one and namespace it to your extension. A name is refused with a warning on the
+`NeoWiki` channel when another backend already holds it, when it is `neo4j` in any casing — reserved for the bundled
+Neo4j backend — or when it is longer than 255 bytes, which is all a run record can hold. A refused backend receives
+no page changes and cannot be rebuilt.


So this is talking about what the code logs? Did not fully verify but looks like there already was clutter before, and now there is even more.

The name is what --store addresses. Pick a stable one and namespace it to your extension.

Seems to be all that is needed. Still horribly worded

Again I did not review the other additions and expect more issues there.

I'm really tired of reviewing this AI text as I deal with it the whole day already and don't want to be constantly reviewing it on other people's PRs. This kind of rot does real damage to the product and should not get auto-merged like seems to be happening consistently here.

Yes it is the failure mode described in the log.
I re-checked the failure path and found that the failure reason wasn't visible to the operator in the default setup.
The doc trim and the logging setup is followed up in #1258

alistair3149 and others added 13 commits August 4, 2026 18:20

alistair3149 force-pushed the graph-rebuild-review-fixes branch from 1e256df to f633528 Compare August 5, 2026 00:29

alistair3149 requested a review from JeroenDeDauw August 5, 2026 00:37

alistair3149 marked this pull request as ready for review August 5, 2026 00:37

alistair3149 merged commit f633528 into graph-rebuild-scoped Aug 5, 2026
14 checks passed

alistair3149 deleted the graph-rebuild-review-fixes branch August 5, 2026 18:29

This was referenced Aug 5, 2026

Scope graph rebuilds per store, on master's page enumeration #1254

Merged

Scope graph rebuilds per store, batched and resumable, with run records #1233

Closed

JeroenDeDauw reviewed Aug 5, 2026

View reviewed changes

alistair3149 mentioned this pull request Aug 6, 2026

Route the NeoWiki log channel, and keep credentials out of it #1258

Open

Navigation Menu

Uh oh!

Address the review findings on the scoped graph rebuild - #1250

Address the review findings on the scoped graph rebuild#1250
alistair3149 merged 13 commits into
graph-rebuild-scopedfrom
graph-rebuild-review-fixes

alistair3149 commented Aug 5, 2026 •

edited

Loading

Uh oh!

Uh oh!

alistair3149 commented Aug 5, 2026

Uh oh!

alistair3149 commented Aug 5, 2026

Uh oh!

JeroenDeDauw Aug 5, 2026

Uh oh!

JeroenDeDauw Aug 5, 2026

Uh oh!

alistair3149 Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

alistair3149 commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Worth a look

Not addressed

Verification

Uh oh!

Uh oh!

alistair3149 commented Aug 5, 2026

Uh oh!

alistair3149 commented Aug 5, 2026

Uh oh!

JeroenDeDauw Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

JeroenDeDauw Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

alistair3149 Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alistair3149 commented Aug 5, 2026 •

edited

Loading