[#157] Load and save the XML file only when it has changed

[maximthomas]

Problem

XMLConnector is not poolable, so the framework runs init(), one operation and dispose() per call. ConcurrentXMLHandler parses the whole file when the number of invokers goes from 0 to 1 and writes it back when it goes from 1 to 0, whatever the operation was. A search, an authenticate, a test and a schema call therefore parse and rewrite the file although nothing in it changed, and the cost is proportional to the file size. #176 made the save linear; this PR removes the save, and most parses, from the calls that do not need them.

Related defects found on the way:

• The old collision check (version != lastModified && isExternallyModified()) could never be true, because version and lastModified were always assigned together, so an outside edit was overwritten without a log line.
• Xerces' deferred DOM builds a node on its first read, and NodeList.item(i) changes the document-wide NodeListCache. search and authenticate run in parallel under the read lock, so the connector's own reads wrote to the shared DOM. This PR removes those writes. It does not remove all of them: Saxon's XQuery walks behind search and getEntry (so authenticate too) still call NodeList.item(i), which updates the document's node list cache under the read lock. Part 2 replaces the lookups by identifier and the unfiltered search; searches with other filters stay on XQuery.
• XMLConnector.init() held the class monitor while it loaded a file, or while it waited for another call to finish saving one, so one slow file blocked connectors on every other file.

Lookups (the Saxon XQuery behind search) are not changed here and stay as slow as they are; part 2 of #157 replaces the lookups by identifier and the unfiltered search, and searches with other filters stay on XQuery.

Change

• Read-only operations do not write the file. dispose() returns early (Exit serialize: nothing to save) unless the document holds a user change (dirty) or is a new document that has not been saved yet (unsavedNewFile). dirty is reset after a successful write.
• Reload by stamp. New FileStamp (package-private) records the modification time, size and file key, or that the file does not exist. init() parses again only when the stamp differs: the modification time is compared for inequality (an older time counts, because cp -p sets one), so is the size, so is the file key (a replaced file). Under a racy stamp the CRC-32 of the file is compared with the CRC-32 of the bytes last read or written. The racy flag is computed when the stamp is taken: the modification time was less than 2 s before the clock, or after it. That is stricter than the rule in the issue, which misses a write in the same tick. After the connector's own save the stamp is always racy, so without the content check the next init() would always parse again.
• loadDocument() parses the file once, through a CheckedInputStream, so the CRC-32 is of the bytes parsed and no copy of the file stays in memory through the parse. The source has the file's system id, xmlFile.toURI().toASCIIString() as DocumentBuilder.parse(File) used (a relative DTD still resolves, also under a non-ASCII path: the raw non-ASCII URI made Xerces 2.6.2 fail with MalformedURLException: no protocol: store.dtd). Right after the parse it calls XmlDocumentWriter.normalizeText(loaded). This PR keeps a loaded document in memory and saves only on change, so without it a hand-written or generated store would be served un-normalised for as long as only reads happen (a CDATA value missing or cut short, a whitespace-only value as " "); before this PR every save rewrote the file normalised and the next call reparsed it. The normalisation does not write the file and does not mark the document dirty. The stamp, the checksum and the document are recorded only after a successful parse, so a malformed file keeps failing until it is fixed.
• The bundle IT creates a store and loads an outside edit. Once loads became conditional, [#157] Save the XML document in linear time #176's XMLConnectorBundleIT no longer parsed a file inside the bundle (the first call created the store and no later call reloaded it). It still starts without a file, so the first call creates a new document under the bundle's child-first loader; after its changes it rewrites the file from outside (carol, mtime an hour back) and reads it through the facade, so a load runs under that loader too.
• The dirty flag.
  • markDirty() runs before the mutation in create and update: a mutation that fails midway is still saved, so the file agrees with memory.
  • In delete it runs after removeChild: that removal is all-or-nothing, so a delete that fails marks nothing.
  • A failed save keeps the change in memory, and the next dispose() retries it. init() never reloads over an unsaved change to a loaded document.
  • A new document is not a user change. The empty document created when the file is missing (createFileIfNotExists) has its own flag, unsavedNewFile. A store file that appears before the new document is saved wins over it, whether it appears after a failed first save, during the first call, or while the new document is being created (the new document starts from FileStamp.MISSING, what buildDocument()'s check found, not from a second read): the connector never loaded that file, so a save would replace a whole store. The new document is dropped together with any changes made to it, which an ERROR line reports (<file> appeared before the new document was saved; keeping the file and dropping the changes made to the new document); the next call loads the file, or starts a new document if the file is gone by then. Before this PR such changes were lost as well when the file appeared between calls (every call reloaded), and a file that appeared during the first call was overwritten without a log line. The partial write of the connector's own failed save is not such a file (the save takes the stamp again; if that stamp cannot be read, the file is still taken for the partial write, because only a save leaves a new document with an unreadable stamp), and neither is a directory in the file's place, so those saves are retried. For a new document a file that appears is the only reason to reload: the content check behind a racy stamp applies to loaded documents only, so an unchanged new document does not parse its own partial write either.
• The collision check now fires. dispose() compares the stamp taken at the last load or save attempt with the file's current stamp and logs UPDATE COLLISION: <file> has changed since it was loaded or saved; overwriting it with the data in memory. The stamp is refreshed in a finally after every save attempt, so the retry of a failed, partly written save does not report its own write. The line is not logged where the path holds no file, because nothing is overwritten there (the retry after a save failed on a directory that has since gone).
• A DOM without deferred expansion. The parser runs with http://apache.org/xml/features/dom/defer-node-expansion set to false, so every node exists after the parse and the connector's own reads do not write to the document. (Saxon's XQuery lookups still update the node list cache under the read lock; part 2 replaces the lookups by identifier and the unfiltered search, and searches with other filters keep them.) A parser that rejects the attribute is logged at WARN and loading goes on. ConnectorObjectCreator.createConnectorObject(Node) reads an entry through sibling pointers and never through a NodeList.
• The narrowed monitor. XMLConnector.init() keeps the handler cache lookup and creation inside synchronized (XMLConnector.class) and calls handler.init() (the load) outside it. XMLHandlerCache is package-private and ConcurrentXMLHandler has a package-private constructor taking an XMLHandler, both only so a test can block init on one file and watch another.

Known limits

• The collision check compares only the stamp, without a CRC: a same-size write in the same tick is overwritten without a log line.
• The racy flag is computed when the stamp is taken, so a save that crosses a 2-second tick boundary on FAT can yield a non-racy stamp.
• With createFileIfNotExists=false, a repeated save of a dirty document recreates a deleted file.
• A store file deleted during a call that changes it is recreated with the data in memory, without an UPDATE COLLISION line.
• If the stamp after a new document's failed save cannot be read (an IOException other than NoSuchFileException), any file at the path is taken for the connector's own partial write: the stamp cannot tell whose write the file holds. The retry then logs UPDATE COLLISION against that partial write. A store that someone else wrote there before the retry is overwritten, with the same line; before d10cd2a that store was kept and the new document was dropped. The connector's side is taken because losing a store this way needs both failures and an outside writer in between, while giving way made every later call parse the connector's own partial write and fail (V4).
• A store shared by several connector servers, for example over NFS, is not supported. The stamp comes from the file's attributes, and an NFS client may answer those from its attribute cache (up to acregmax, 60 s by default) where an open would revalidate, so another host's write can go unseen and be overwritten by the next save. Before this PR every call opened and parsed the file, but nothing locked it across hosts either.

Measurements

Time through ConnectorFacade, one call per measurement (init(), the operation, dispose()), after one warm-up size (1,000 entries, not shown). The harness is the issue's XmlPerf with -Dreps=3: ri:__ACCOUNT__ entries with __UID__, __NAME__ and a few more fields, and a facade built over the generated file. "Before" is #176 at d37978d (classes), "after" is this branch at 8334844, before its rebases onto 7a1a5eb, bd2fe56, faf5d15 and, once #176 was merged, master at dda8961 (the #176 commits added since d37978d change tests and Javadoc, plus #176's review fixes (its A6 and A7 rows), of which only a cheaper normalizeText touches these timings; the tables were not measured again). Each cell is the median of the three runs' medians (each run is one JVM with 3 repetitions per cell), with the smallest and the largest of the three runs in parentheses. The six runs alternated before/after on an otherwise idle machine (16:56 to 17:09, all exited 0). JDK 26 (Zulu 26.28), Intel Core i7-4850HQ, 8 logical CPUs, 16 GB, macOS; all numbers from one developer machine, so read them as orders of magnitude.

Entries	search __NAME__ before	after	authenticate before	after	update before	after
1,000	28 ms (23-30)	9 ms (8-9)	19 ms (16-24)	5 ms (5-6)	22 ms (19-29)	15 ms (12-18)
10,000	467 ms (408-536)	155 ms (150-302)	272 ms (208-507)	45 ms (41-56)	295 ms (292-342)	147 ms (143-181)
20,000	1,487 ms (1,285-1,496)	597 ms (561-721)	562 ms (510-793)	159 ms (147-160)	869 ms (817-922)	423 ms (409-443)
40,000	5,075 ms (4,970-5,110)	2,138 ms (1,972-2,206)	1,615 ms (1,556-1,623)	597 ms (561-763)	2,781 ms (2,663-3,216)	1,346 ms (1,295-1,375)

Entries	create before	after	search all before	after
1,000	24 ms (21-30)	16 ms (12-19)	47 ms (46-55)	26 ms (22-28)
10,000	422 ms (398-424)	251 ms (238-274)	538 ms (512-658)	334 ms (334-340)
20,000	1,358 ms (1,312-1,376)	683 ms (664-698)	1,540 ms (1,517-1,574)	732 ms (687-746)
40,000	5,022 ms (4,989-5,084)	2,283 ms (2,274-2,353)	5,895 ms (5,245-6,161)	2,459 ms (2,400-2,578)

DOM pinned is what the same operations cost on a document that is already in memory, with no reload and no rewrite (the lower bound for a lookup that still goes through XQuery). At 40,000 entries (median of the three runs), before and after: search 4,483 and 2,164 ms, update 2,195 and 1,110 ms, create 4,455 and 2,093 ms.

Did a search rewrite the file? Before: yes, at every size, in all three runs. After: no, at every size, in all three runs (file rewritten by a search: false). That is the part of the change that does not depend on the machine.

What the numbers show:

• "After" is faster than "before" in every cell of both tables, and at 40,000 entries the ranges of the three runs do not overlap in any column (for example authenticate 1,556-1,623 ms before and 561-763 ms after; update 2,663-3,216 ms and 1,295-1,375 ms).
• authenticate at 40,000 entries is 1,615 ms before and 597 ms after. It no longer writes the file or parses it again. Its remaining cost is the XQuery lookup, which this PR does not touch.
• A search by __NAME__ at 40,000 entries is 5,075 ms before and 2,138 ms after, and it sits at the DOM pinned search (2,164 ms): what is left is the lookup, which is quadratic and is part 2. At 10,000 entries the three "after" runs scatter most for this operation (150 to 302 ms, the median is 155 ms). The XQuery lookup is not changed by this PR, and its cost depends on the state that earlier walks left in Xerces' node list cache (the issue's probe shows one cache object on a free list that points to itself); the numbers here do not show more than that the scatter is in the search and not in the load or the save. authenticate before the change scatters the same way at 10,000 entries (208 to 507 ms).
• update and create at 40,000 entries are about 2.1 times faster (2,781 to 1,346 ms, 5,022 to 2,283 ms); both still pay for a save and for the XQuery lookup of the entry, and create sits near its DOM pinned value (2,093 ms).
• The numbers in the issue (13.5 s for a search at 40,000 entries) were taken on master, before [#157] Save the XML document in linear time #176; [#157] Save the XML document in linear time #176 alone ("before" above) is already at 5.1 s on this machine.

Tests

mvn -o -pl OpenICF-xml-connector verify, whole module, at 0db055fe, on JDK 26 and on JDK 11: surefire 196 run, 0 failed, 0 skipped; failsafe 1 run, 0 failed, 0 skipped (XMLConnectorBundleIT, which creates a store and then loads an outside edit). (master at dda89610, which has #176: surefire 142.)

New test classes and cases:

• FileStampTests (11): an unchanged file matches; an older mtime, a size change under the same mtime, a replaced file, a deleted file and an unreadable path (a symbolic link to itself) do not match; a missing file matches MISSING; the racy window, on both sides of the clock (an mtime an hour ahead, and one 500 ms old); the checksum; which stamps are known (readableAndMissingFilesAreKnown).
• XMLHandlerReloadTests (41): read-only calls do not write the file; a change is in the file when the last user leaves (create, update, delete); a failed update, a failed delete of a nested entry, a failed save and its retry; an unchanged file is served from memory; an outside copy with an older mtime, a same-size edit in the racy window, and a malformed edit; a racy load is not parsed again; an unreadable file behind a racy stamp; a relative DTD; the UPDATE COLLISION line and its absence on an own save, on a recreated deleted file, on the retry of a partial write, and on the retry that creates the file; an own save is not parsed again; a new file, a saved new file, and an unsaved new document that gives way to a file that appears; the DOM is fully expanded; ConnectorObjectCreator uses no node lists; a parser without the defer attribute still loads and is reported; the CDATA value of a loaded file is read (cdataValueOfALoadedFileIsRead); a relative DTD is resolved under a directory with a non-ASCII name (relativeDtdIsResolvedUnderANonAsciiDirectory); a store file that appears before a new document is saved wins over it, after a failed save of a change and during the first call with and without a change, while a freed path and the connector's own partial write are saved again; a whitespace-only value of a loaded file is absent (whitespaceOnlyValueOfALoadedFileIsAbsent); an unchanged new document whose save failed midway does not parse its partial write and saves again (ownPartialWriteOfAnUnchangedNewDocumentIsNotLoaded); the changes dropped for a file that appears stay dropped when the file goes (changesDroppedForAFileThatAppearsStayDroppedWhenItGoes); a file that appears while a new document is created, and a loaded store moved back before the new document is saved, are kept (fileThatAppearsAsTheNewDocumentIsCreatedIsNotOverwritten, loadedStoreMovedBackBeforeTheNewDocumentIsSavedIsKept); the connector's own partial write behind an unreadable stamp is saved again (ownPartialWriteBehindAnUnreadableStampIsSavedAgain); a racy load with 100 KB after the root is not parsed again (racyLoadWithMarkupAfterTheRootIsNotParsedAgain).
• XMLConnectorTests gains readOnlyOperationsDoNotRewriteTheFile and initOnOneFileDoesNotWaitForAnotherFile; disposeNeverUsesNodeLists is rewritten (see B2 row 28).
• Two tests map to no arm row: FileStampTests#deletedFileIsAChange (every stamp of an existing file has an mtime and MISSING has none, so it dies only on the first row's return true; kept as a readable case) and XMLHandlerReloadTests#externalEditWithANewerMtimeIsLoaded (row 8's mutant dies on externalCopyWithAnOlderMtimeIsLoaded as well; kept as the plain case).

Test strength

Each row is an arm of the production code, the case that reaches it, what only that arm produces, and a mutant that only that case kills. "red at" is the commit where the case was red before its arm existed (B1: on d37978d2, #176 before its review commits; B2 rows 1-23 and 28: on 93329684; B3: on 2e6c4c7a; B4: on 6eaed53f; the final review fixes: on the commit named in their rows; the second review's fixes: bd73b5eb, or mutant only where the case was already green there; the third review's fixes: 6533361b, likewise; the fourth review's fixes: the commit before each, or mutant only). Rows marked unpinnable are arms that no test can tell from their absence; the reason is given as found. Line numbers are at 0db055fe. The reds were observed before this branch was rebased onto 7a1a5eb5, then onto bd2fe56f, then onto faf5d158, then, once #176 was merged, onto master at dda89610. The first, the third and the fourth rebase left every patch unchanged. The second left every patch unchanged except 2e6c4c7a: its dispose() takes the document from getDocument() without the document monitor, as #176 now does, and it turns #176's new saveWithoutADocumentThrowsConnectorException into saveWithoutADocumentSavesNothing, because here dispose() with nothing to save returns before it touches the document. The #176 commits added underneath since d37978d2 (squashed into dda89610) change tests and Javadoc, plus #176's review fixes (its A6 and A7 rows); the other commits master gained since (#169, #170, #173) touch no file of this module.

B1: FileStamp (9332968)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	FileStamp.java:63-69,102-104 — read, sameState — mtime, size and file key are recorded; an unchanged file matches	unchangedFileMatches	two reads of one unchanged file match	FileStamp not defined (afterwards: FileTime or file key compared with ==)	d37978d2
2	FileStamp.java:102 — sameState — Objects.equals(lastModified, other.lastModified)	olderMtimeIsAChange	an older mtime does not match	sameState without the mtime (returns true)	d37978d2
3	FileStamp.java:103 — sameState — size == other.size	sizeChangeWithTheSameMtimeIsAChange	a size change under the same mtime does not match	no size comparison	d37978d2
4	FileStamp.java:104 — sameState — Objects.equals(fileKey, other.fileKey)	replacedFileIsAChange (SKIP where the file system has no file keys)	a replaced file with the same size and mtime does not match	no file key comparison	d37978d2
5	FileStamp.java:72-73,99-101 — read, sameState — catch (IOException e) returns UNKNOWN, and this == UNKNOWN || other == UNKNOWN returns false (one road: UNKNOWN differs from MISSING only through this guard)	unreadableStampMatchesNothing (a symbolic link to itself: ELOOP; SKIP where links cannot be made, e.g. Windows without the privilege)	the stamp of an unreadable path matches neither itself nor MISSING, in either direction	catch (IOException e) { return MISSING; }; also killed here: the guard dropped, the guard on this only, on other only, && for || (verified)	d37978d2
6	FileStamp.java:70-71 — read — catch (NoSuchFileException e) returns MISSING	missingFileMatchesMissing	a missing file matches MISSING	every IOException returns UNKNOWN	d37978d2
7	FileStamp.java:69 — read — racy: now - modified.toMillis() < RACY_WINDOW_MILLIS	stampIsRacyOnlyNearTheClock; stampIsRacyJustAfterAWrite (37b4a3e)	an mtime an hour ahead is racy, an hour old is not; an mtime 500 ms old is racy, 3 s old is not	racy always false; only the second case kills < 0 (only future mtimes racy) and a window of 400 ms; a window of 4 s dies on it and on stampConfirmedByContentStopsBeingRacy	d37978d2; the second case mutant only (green at 6533361b)
8	FileStamp.java:78-86 — checksum — CRC-32 of the content	checksumFollowsTheContent	equals the CRC-32 of abc, changes with the content	checksum not defined	d37978d2

UNKNOWN's racy = true is never read (fileHasChanged asks isRacy() only after sameState returned true); it is an equivalent mutant and has no row. The first draft had an exists field; it is gone, because an existing file always has a non-null lastModifiedTime() and exists == other.exists could never decide anything the mtime comparison did not. Rows 4 and 5 are SKIPPED where their condition does not hold, so those arms are unpinned on such runners (Windows CI).

B2: reload and save only when needed (2e6c4c7)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	XMLHandlerImpl.java:421-424 — dispose — keep side of the guard !dirty && !unsavedNewFile: nothing to save, return	readOnlyCallsDoNotWriteTheFile	mtime and bytes unchanged after search and authenticate	the unconditional save (A2's dispose)	93329684
2	XMLHandlerImpl.java:543-545,421 — createDocument, dispose — the document made for a missing file is saved by the first call (written as dirty = true first; row 21 replaced it with unsavedNewFile)	newFileIsCreatedByTheFirstCall	the file exists after one init/dispose	nothing marks a new document (row 1's guard skips its save)	93329684
3	XMLHandlerImpl.java:277-278 — create — markDirty() before getDocument().getDocumentElement().appendChild(objElement)	changeIsInTheFileWhenTheLastUserLeaves	file names [alice, bob]	the call deleted	93329684
4	XMLHandlerImpl.java:333-334 — update — markDirty() before removeChildrenFromElement(entry, …), the entry's first change	failedUpdateLeavesTheFileInAgreementWithMemory	after an update that throws past the removal, the file's last names equal memory's [null]	the call deleted; the call moved after the attribute's appends or after the loop (verified red). The mutant "markDirty() one statement down, right after removeChildrenFromElement" survives this case: that call throws midway only on an XSD-invalid store whose entry holds a same-named element below another child (getElementsByTagName searches descendants), and no case builds such a store. The failure this case uses, values removed before the single-valued check, is a defect master has too, filed as #178; once it is fixed, this row needs another mid-mutation failure	93329684
5	XMLHandlerImpl.java:372-373 — delete — markDirty() after getDocument().getDocumentElement().removeChild(elementToRemove)	deleteIsInTheFileWhenTheLastUserLeaves	file names [alice]	the call deleted	93329684
6	XMLHandlerImpl.java:440 — dispose — dirty = false after a successful write	XMLConnectorTests#readOnlyOperationsDoNotRewriteTheFile	mtime and bytes unchanged by read-only facade calls after a create	the reset deleted: every later dispose writes again (verified on the final code too; no other case kills it)	93329684
7	XMLHandlerImpl.java:126 — init — keep side: a loaded document is not parsed again	unchangedFileIsServedFromMemory	[alice] after a same-size, same-mtime edit under a non-racy stamp	init() calls buildDocument() every time	93329684
8	XMLHandlerImpl.java:157-158,589 — fileHasChanged, loadDocument — !stamp.sameState(current) reloads; loadDocument records the stamp	externalCopyWithAnOlderMtimeIsLoaded	[carol] after a copy with an older mtime	init reloads only when document == null	93329684
9	XMLHandlerImpl.java:587-591 — loadDocument — stamp and document recorded only after a successful parse	malformedEditFailsEveryCallUntilFixed	the second init() on the malformed file throws too	the stamp recorded before the parse	93329684
10	XMLHandlerImpl.java:163-166 — fileHasChanged — racy stamp, different CRC-32 → reload	sameSizeEditInsideTheRacyWindowIsLoaded	[bobby] after a same-size edit under a racy stamp	fileHasChanged = !stamp.sameState(current) only	93329684
11	XMLHandlerImpl.java:164,170-171,577-584,590 — fileHasChanged, loadDocument — racy stamp, same CRC-32 → no reparse; loadDocument records the CRC-32 of the bytes it parses (of a byte array until 19962a6, of the parser's stream since)	racyLoadIsNotParsedAgain	the second call logs no Loading XML document from (the first call's log has it, which proves the literal)	racy → always reload	93329684
12	XMLHandlerImpl.java:167-168 — fileHasChanged — catch (IOException e) returns true: a file that cannot be read behind a racy stamp is reloaded (and the load fails), not served from memory	unreadableFileBehindARacyStampFails (SKIP as root or where permissions cannot deny reads, e.g. Windows)	init() throws ConnectorException	catch … { return false; } (row 11's stub)	93329684
13	XMLHandlerImpl.java:170 — fileHasChanged — stamp = current after the content matched	stampConfirmedByContentStopsBeingRacy (same SKIP, and SKIP when the first load falls outside the racy window: a slow load, or a file system with second-granularity mtimes)	the third call serves [alice] without reading the now unreadable file	the refresh deleted: every later init reads and checksums the whole file	93329684, re-run after review
14	XMLHandlerImpl.java:582 — loadDocument — source.setSystemId(xmlFile.toURI().toASCIIString())	relativeDtdIsResolvedAgainstTheFile	a DTD named relative to the store file resolves	no system id: parsing from bytes resolves against the working directory	93329684
15	XMLHandlerImpl.java:432-434 — dispose — ERROR UPDATE COLLISION: {0} has changed since it was loaded or saved; overwriting it with the data in memory.	outsideEditDuringAChangeIsReportedAndOverwritten	System.err has UPDATE COLLISION: <file> has changed since it was loaded or saved, and the file then holds [alice, bob]	the old check (version != lastModified, never true)	93329684
16	XMLHandlerImpl.java:432 — dispose — keep side of !stamp.sameState(FileStamp.read(xmlFile))	ownChangeIsSavedWithoutACollision	no UPDATE COLLISION on an ordinary save	the line logged on every save	93329684
17	XMLHandlerImpl.java:543 — createDocument — the stamp is set (read again until 03753ae, FileStamp.MISSING since)	deletedFileIsRecreatedWithoutACollision; since 3c4cfc17 (T3) the collision check skips a path with no file, so this case no longer kills the mutant, and V3 pins the arm	no UPDATE COLLISION when a deleted file is recreated empty	the write deleted (the deleted file's stamp stays)	93329684
18	XMLHandlerImpl.java:446-448 — dispose — finally { stamp = FileStamp.read(xmlFile); }: the stamp follows every save attempt	retryAfterAFailedSaveDoesNotReportItsOwnPartialWrite	no UPDATE COLLISION when a save that failed mid-write (unpaired surrogate) is retried	the read deleted, or done after a successful write only	93329684
19	XMLHandlerImpl.java:439 — dispose — checksum = XmlDocumentWriter.write(…)	ownSaveIsNotParsedAgain	no Loading XML document from after the connector's own save	the assignment deleted	93329684
20	XMLHandlerImpl.java:126 — init — !dirty &&: never reload over an unsaved change to a loaded document (R1 is the exception for a new document)	failedSaveKeepsTheChangeInMemoryAndRetriesIt	[alice, bob] in memory and then in the file after a failed save and an outside file	document == null || fileHasChanged()	93329684
21	XMLHandlerImpl.java:545,421 — createDocument, dispose — unsavedNewFile = true instead of row 2's dirty = true; the guard !dirty && !unsavedNewFile	unsavedNewDocumentGivesWayToAFileThatAppears (its names() assertion)	[alice] from a file that appeared after the new document's save failed	a new document marked dirty (the issue's "treat a new document as dirty")	93329684
22	XMLHandlerImpl.java:441 — dispose — unsavedNewFile = false after a successful write	savedNewFileIsNotWrittenAgain	the second call logs Exit serialize: nothing to save	the reset deleted (every later dispose writes again)	93329684
23	XMLHandlerImpl.java:591 — loadDocument — unsavedNewFile = false	unsavedNewDocumentGivesWayToAFileThatAppears (its bytes assertion)	the file that appeared keeps its bytes	the reset deleted: the loaded file is written back, reformatted	93329684
24	XMLHandlerImpl.java:277-278 — create — placement of markDirty() before appendChild	—	unpinnable by construction: appending a detached new element to the document element cannot throw, so "before" and "after" are the same state; moving the call after appendChild keeps every case green (verified). Row 3 pins the call itself	—	—
25	XMLHandlerImpl.java:372-373 — delete — markDirty() after removeChild: the removal is all-or-nothing, so a delete that fails marks nothing	failedDeleteOfANestedEntryLeavesTheFileAlone	an entry nested below another element (an XSD-invalid hand edit that getEntry's descendant query finds and removeChild on the document element rejects with NOT_FOUND_ERR): after the failed delete the file's bytes and mtime are unchanged	markDirty() before removeChild: the failed delete marks the document dirty and dispose() rewrites the file	red in the review round, on this branch's commit before the fix (the first draft had declared this row unpinnable; its reason, "cannot throw", was false)
26	XMLHandlerImpl.java:576-580 — loadDocument — FileStamp.read before the file is opened (before Files.readAllBytes until 19962a6)	—	unpinnable by construction: only a write that lands between the two statements tells the orders apart, and a test has no hook there	—	—
27	XMLHandlerImpl.java:577-584 — loadDocument — checksum the bytes that are parsed, not the file a second time (since 19962a6 the stream the parser reads)	—	unpinnable by construction: only a write between two reads of the file tells them apart; no hook	—	—
28	XMLHandlerImpl.java:435-439 — dispose — the save road (A2 row 1: XmlDocumentWriter.normalizeText/write instead of Saxon's //text() XPath and DOMSender), which row 1's guard now skips for a loaded, unchanged document	XMLConnectorTests#disposeNeverUsesNodeLists, rewritten	the file exists after dispose(), and XercesNodeLists.used(getDocument()) is false	a NodeList walk on the save road: document.getDocumentElement().getChildNodes().getLength(); inserted right after the if (!dirty && !unsavedNewFile) { … } guard in dispose() (stands for A2 row 1's mutant, the old Saxon walks)	93329684

Row 28 is an addition made after row 1 landed: A2's disposeNeverUsesNodeLists loaded a two-entry file and called init() + dispose(), which now returns "nothing to save", so it would pass whatever the save road does (checked: A2's version stays green with the mutant line injected). A new document with only its root does not help either, because Xerces 2.6.2 allocates no node list cache for a parent with fewer than two children. The rewritten case makes a new document, appends two entries through getDocument() with DOM calls that allocate no cache, and lets dispose() save it; with the mutant it is red.

B3: the connector's own reads do not write to the DOM (6eaed53)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	XMLHandlerImpl.java:568 — loadDocument — docBuilderFactory.setAttribute(DEFER_NODE_EXPANSION, Boolean.FALSE)	loadedDocumentIsFullyExpanded	getDocument() is not a DeferredDocumentImpl	the attribute not set	2e6c4c7a
2	ConnectorObjectCreator.java:79 — addAllAttributesToBuilder(Node entry) — attributes by getFirstChild()/getNextSibling() (with its one call site, XMLHandlerImpl.java:401 in search(String, …), which the signature change forces; the existing XMLHandlerTests value assertions pin what search returns)	connectorObjectIsReadWithoutNodeLists	uid, name and last name read, and XercesNodeLists.used still false afterwards	the walk through entry.getChildNodes().item(i)	2e6c4c7a
3	XMLHandlerImpl.java:569 — loadDocument — catch (IllegalArgumentException ex) around setAttribute: loading goes on	parserWithoutTheDeferAttributeStillLoads: the JVM-wide setting javax.xml.parsers.DocumentBuilderFactory names a JAXP parser whose setAttribute throws IllegalArgumentException, which is what the JAXP contract specifies for an attribute the parser does not recognise	[alice] loaded	no catch: IllegalArgumentException: Not supported: http://apache.org/xml/features/dom/defer-node-expansion out of init()	2e6c4c7a
4	XMLHandlerImpl.java:570 — loadDocument — the catch's WARN The XML parser {0} does not support {1}	parserWithoutTheDeferAttributeIsReported (same fixture)	System.out has The XML parser <factory class> does not support http://apache.org/xml/features/dom/defer-node-expansion	the catch without the WARN	2e6c4c7a

B4: the class monitor does not cover the load (0111d43)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	XMLConnector.java:97 — init — handler.init() runs after the synchronized (XMLConnector.class) block	XMLConnectorTests#initOnOneFileDoesNotWaitForAnotherFile	init on another file returns within 10 s while a BlockingHandler holds init on the first file	handler.init() inside the class monitor (TimeoutException at other.get)	6eaed53f
2	XMLConnector.java:84-90 — init — lookup and creation in XMLHandlerCache stay inside the class monitor	—	unpinnable by construction: dropping the monitor matters only when two init() calls on one new path interleave between get and put, and a test cannot force that interleaving	—	—

Final review fixes (84df879, d0efc9c, bd73b5e)

#	arm (file:line — function — condition)	case	observable	mutant	red at
F1	XMLHandlerImpl.java:586 — loadDocument — XmlDocumentWriter.normalizeText(loaded) right after the parse	XMLHandlerReloadTests#cdataValueOfALoadedFileIsRead	a store whose lastname is a CDATA section, mtime an hour back: two read-only calls each return Last-alice	the call absent: the first call returns [null]	red at 0111d437
F2	XMLHandlerImpl.java:582 — loadDocument — setSystemId(xmlFile.toURI().toASCIIString())	XMLHandlerReloadTests#relativeDtdIsResolvedUnderANonAsciiDirectory (SKIP where a directory with a non-ASCII name cannot be created; since 1b76414 the directory is named after the test's store file, so one left by a killed run cannot cause the SKIP)	a DTD next to a store in a directory named with Cyrillic and accented letters resolves; names() is [alice]	toURI().toString(): MalformedURLException: no protocol: store.dtd	red at 84df8799
F3	XMLHandlerImpl.java:561-591 — loadDocument under the bundle's child-first loader (xml-apis 1.3.04, the bundle's Xerces)	XMLConnectorBundleIT#bundleSavesLoadsAndFindsEntries	after the facade's own changes ([alice] in the file), an outside edit (carol, mtime an hour back) comes back from getObject; until 9661a0c the store was written before the first call	DocumentBuilderFactory.newDefaultInstance() in loadDocument (absent from xml-apis 1.3.04: green under surefire, NoSuchMethodError only in the bundle; the IT before this commit stayed green with it, with no Loading XML document from line)	red at d0efc9c0

Second review fixes (529d3c6, 9661a0c, e30c0ae, 4625e87)

#	arm (file:line — function — condition)	case	observable	mutant	red at
R1	XMLHandlerImpl.java:123-124,139-141 — init — fileAppearedOverNewDocument(): a store file that appeared after the failed first save of a changed new document is loaded	XMLHandlerReloadTests#changedNewDocumentGivesWayToAFileThatAppears	the next search returns the file's [alice, carol], and the file's bytes are unchanged	init() without the call: the search returns [bob], and the next save replaces the store	bd73b5eb
R2	XMLHandlerImpl.java:426-430 — dispose — the same check: a file that appeared during the call is kept	fileThatAppearsDuringAChangeToANewDocumentIsKept (with a change), fileThatAppearsDuringTheFirstCallIsNotOverwritten (an empty new document)	the file's bytes are unchanged after dispose(), and the next search returns its entries	dispose() without the check: UPDATE COLLISION, and the file is overwritten	bd73b5eb
R3	XMLHandlerImpl.java:141 — fileAppearedOverNewDocument — !unsavedNewFile: only a new document gives way	failedSaveKeepsTheChangeInMemoryAndRetriesIt, outsideEditDuringAChangeIsReportedAndOverwritten	a loaded document's change still overwrites an outside edit, with UPDATE COLLISION	the condition dropped: a loaded document gives way as well	mutant only (green at bd73b5eb)
R4	XMLHandlerImpl.java:141 — fileAppearedOverNewDocument — !xmlFile.isFile(): a directory in the file's place, or no file at all, is not a file that appeared	changeToANewDocumentIsSavedOnceThePathIsFree, changedNewDocumentGivesWayToAFileThatAppears, unsavedNewDocumentGivesWayToAFileThatAppears	after a save failed on a directory in the file's place and the directory is gone, the next call saves [bob]	the condition dropped: the change is dropped although no file appeared; exists() for isFile(): the save onto the directory is skipped instead of failing	mutant only (green at bd73b5eb)
R5	XMLHandlerImpl.java:141 — fileAppearedOverNewDocument — stamp.sameState(...): the partial write of the connector's own failed save is not a file that appeared	ownPartialWriteOfANewDocumentIsSavedAgain	the second dispose() tries the save again (and fails again: the value cannot be encoded)	the comparison dropped: the second dispose() drops the change and returns	mutant only (green at bd73b5eb)
R6	XMLHandlerImpl.java:146-148 — dropNewDocument (in fileAppearedOverNewDocument until 5934cdd) — the ERROR line, and dirty = false	changedNewDocumentGivesWayToAFileThatAppears, fileThatAppearsDuringAChangeToANewDocumentIsKept	stderr has <file> appeared before the new document was saved; after the reload the file is not written	the line removed: no ERROR; dirty kept: the reloaded document is saved over the file	mutant only
R7	XMLHandlerImpl.java:586 — loadDocument — F1's normalizeText, its whitespace-only half	whitespaceOnlyValueOfALoadedFileIsAbsent	a lastname of one space comes back absent, as after a save and a reload	setCoalescing(true) on the factory instead of normalizeText (which fixes CDATA only): the value comes back as " "	mutant only (green at bd73b5eb)
R8	XMLHandlerImpl.java:511 — createDocument under the bundle's child-first loader	XMLConnectorBundleIT#bundleSavesLoadsAndFindsEntries	the first facade call creates the store inside the bundle (Creating new xml storage file), and a later one loads the outside edit (Loading XML document from)	DocumentBuilderFactory.newDefaultInstance() in createDocument: NoSuchMethodError in the bundle only; the IT of bd73b5eb, which started from an existing store, stayed green with it	mutant only (green at bd73b5eb)

Third review fixes (40104db, bf79edd, 3c4cfc1)

#	arm (file:line — function — condition)	case	observable	mutant	red at
T1	XMLHandlerImpl.java:126 — init — !unsavedNewFile &&: a new document reloads only for a file that appears, not for its own partial write	ownPartialWriteOfAnUnchangedNewDocumentIsNotLoaded	a store is loaded and then deleted, so the new document keeps the loaded file's checksum; its save fails midway with no user change (an unpaired surrogate added to the DOM); once that node is removed, the next init() succeeds and dispose() writes an empty store	the condition dropped: init() parses the partial file and throws SAXParseException: Premature end of file, on every later call too	6533361b
T2	XMLHandlerImpl.java:150 — dropNewDocument (in fileAppearedOverNewDocument until 5934cdd) — document = null: the new document goes with the changes the ERROR line drops	changesDroppedForAFileThatAppearsStayDroppedWhenItGoes	a store file appears during a call that created bob in a new document and is deleted afterwards: the next search returns [], and the file that call writes holds no entry	the line removed: the search returns [bob] and the save writes it	6533361b
T3	XMLHandlerImpl.java:432 — dispose — xmlFile.isFile() &&: no UPDATE COLLISION where no file is overwritten	changeToANewDocumentIsSavedOnceThePathIsFree (its new stderr assertion)	after a save failed on a directory in the file's place and the directory is gone, the retry that creates the file logs no UPDATE COLLISION	the condition dropped: the retry logs UPDATE COLLISION against the directory's stamp	6533361b

Fourth review fixes (5934cdd, 03753ae, d10cd2a, 19962a6, 0736d45, a3d352d)

#	arm (file:line — function — condition)	case	observable	mutant	red at
V1	XMLHandlerImpl.java:123-124,426-427 — init, dispose — dropNewDocument() at each call site; fileAppearedOverNewDocument() (:139-142) is a plain check	changedNewDocumentGivesWayToAFileThatAppears, unsavedNewDocumentGivesWayToAFileThatAppears (init); changesDroppedForAFileThatAppearsStayDroppedWhenItGoes, fileThatAppearsDuringAChangeToANewDocumentIsKept (dispose)	as in R1, R2 and T2	the call removed from init(): the two init cases fail; removed from dispose(): the two dispose cases fail	mutant only (no behaviour change: all four were green before 5934cdd)
V2	XMLHandlerImpl.java:543 — createDocument — stamp = FileStamp.MISSING, not a second read: a file that appears between buildDocument()'s exists() and the stamp is not taken for the connector's	fileThatAppearsAsTheNewDocumentIsCreatedIsNotOverwritten (a store path whose exists() writes a store and returns false)	the store's bytes are unchanged after dispose(), and the next search returns [alice]	FileStamp.read(config.getXmlFilePath()) as before: the first dispose() writes the empty document over the store, with no log line	5934cdd3
V3	XMLHandlerImpl.java:543 — createDocument — the assignment itself (B2 row 17's arm)	loadedStoreMovedBackBeforeTheNewDocumentIsSavedIsKept	a loaded store is renamed away, a call starts a new document, and the store is renamed back (same file key, size and mtime) before dispose(): the file still holds [alice]	the assignment removed: the new document keeps the loaded file's stamp, takes the store for its own and overwrites it, with no log line (the whole module was green with this mutant before the case)	mutant only (green at 3c4cfc17, whose second read returned MISSING here)
V4	XMLHandlerImpl.java:141 — fileAppearedOverNewDocument — stamp.isKnown() &&: a file behind an unreadable stamp of the new document's own save is its partial write	ownPartialWriteBehindAnUnreadableStampIsSavedAgain (the stamp is read through a link to itself while the save fails; SKIP where links cannot be made or the file system does not report the loop as an error)	after the save that failed midway, the next init() keeps the new document and dispose() writes [bob], with no ERROR about an appeared file	the condition dropped: init() drops the document and throws SAXParseException: Premature end of file	03753ae3
V5	FileStamp.java:93-96 — isKnown — this != UNKNOWN	unreadableStampMatchesNothing (its new assertion), readableAndMissingFilesAreKnown	the stamp of an unreadable path is not known; those of an existing and of a missing file are	return true: unreadableStampMatchesNothing and V4's case fail; return false: readableAndMissingFilesAreKnown and six handler cases fail (those of R1, R2, B2 row 21, V2 and V3)	mutant only (new in d10cd2a)
V6	XMLHandlerImpl.java:577-584,590 — loadDocument — the CRC-32 comes from the stream the parser reads (CheckedInputStream), not from a byte array kept through the parse	racyLoadIsNotParsedAgain (B2 row 11), racyLoadWithMarkupAfterTheRootIsNotParsedAgain (100 KB of comment and a processing instruction after the root)	the second call logs no Loading XML document from	the stream without the file's last byte; the CRC-32 taken into a CRC32 that is not recorded: both cases fail	mutant only (behaviour unchanged: both green at d10cd2a1)

Notes on the rows: B1 has 8 rows, none unpinnable; B2 has 28 rows, 3 unpinnable (24, 26, 27); B3 has 4 rows; B4 has 2 rows, 1 unpinnable; the final review fixes have 3 rows; the second review's fixes have 8 rows, none unpinnable; the third review's fixes have 3 rows, none unpinnable; the fourth review's fixes have 6 rows, none unpinnable. The fifth review (of a3d352d9) found no code defect. It asked for the second consequence of the UNKNOWN rule under Known limits, and for StatFailingFile's Javadoc to name the callers it diverts (0db055f): with a writer that opened toPath(), V4's case fails at assertTrue(file.exists()) (observed). The fourth review (of de0ab50a) found that an unreadable stamp after a new document's failed save made the next call drop the document and parse its partial write (V4), and that fileAppearedOverNewDocument() changed state inside init()'s || chain (V1); it also suggested the streamed load (V6). Answering it found that a file that appeared while a new document was created was overwritten (V2), and that B2 row 17 had lost its pin with T3 (V3). Each V mutant was run alone at a3d352d9 against XMLHandlerReloadTests and FileStampTests (V3's also against the whole module) and failed only the cases named in its row; the R3-R6 and T1 mutants were run again at a3d352d9 and still fail their cases. Without document != null && in init() every case stays green, and that mutant is equivalent: with no document dirty is always false, so the drop changes nothing, and the guard only saves a stat. 0736d45 changes only V4's case: its SKIP asked isKnown(), so V5's return true mutant made the case a SKIP instead of a failure. V6's second case pins what no code mutant can change, that the parser reads to the end of the file. Xerces 2.6.2 does: in a probe of 48 files (with and without a BOM, ISO-8859-1, 100 KB of trailing white space, comments and a processing instruction after the root), the CRC-32 of the bytes it read equalled the file's in every one. A parser that stopped earlier would cost reloads inside the racy window, not a missed change, since a different CRC-32 reads as a change. The third review (of 6533361b) found that an unchanged new document whose save failed midway parsed its own partial write on every later call (T1), that the changes the ERROR line drops stayed in the document and came back once the appeared file was gone (T2), and a false UPDATE COLLISION on the retry that creates the file (T3); it also found that B1 row 7 pinned only the future half of the racy window. The T1-T3 mutants and B1 row 7's < 0 and 400 ms mutants were each run alone, and each failed only its case among XMLHandlerReloadTests and FileStampTests. The second review (of bd73b5eb) found that a changed new document overwrote a store file that appeared after its first save failed (R1; R2 also covers a file that appears during the first call, the earlier review's M2) and that the IT no longer created a store inside the bundle (R8); R3-R7 pin conditions with mutants that were each run alone. The final review found I1 (the bundle IT no longer parsed a file, F3) and I2 (a loaded file was served un-normalised, F1), both pinned above; it also led to F2 and to the narrower reader claim. Platform SKIPs: B1 row 4 (no file keys), B1 row 5 and V4 (symbolic links cannot be made), B2 rows 12 and 13 (root, or Windows where permissions cannot deny reads), and B2 row 13 also when the first load falls outside the racy window. Those arms are unpinned on such runners, which includes the Windows CI job; the module run above had none of them skipped (0 skipped on JDK 26 and JDK 11). B2 row 4's surviving mutant and B2 row 25 are described in the rows: row 25 started as unpinnable in the first draft, a review showed that reason was false, markDirty() moved after removeChild, and the case was added. The red for B2 row 13 was re-run after the review added its second SKIP. The reds were observed by running each case on the tree without its arm (B1: on d37978d2; B2: rows built in order on 93329684).

Part of #157

[vharseko]

Review of the PR's own 17 commits (from 000bba1b to de0ab50a), checked against the description and the three earlier review rounds. I found no new correctness bug in the stamp, racy-window or dirty logic. Points the description already settles (dropping a new document's changes when a file appears, the stamp-only collision check, the package-private XMLHandlerCache) are not raised again.

1. Pre-existing: update() validates after it removes the old values (XMLHandlerImpl.java:328-336)

update() checks the single-valued rule (!isMultiValued() && values.size() > 1) only after removeChildrenFromElement() has deleted the entry's existing values. This is not a regression: master has the same order (lines 280-286), and its dispose() saved on every call, so the loss already reached the disk there. This PR keeps that on purpose ("file agrees with memory"), and failedUpdateLeavesTheFileInAgreementWithMemory uses the bug as its failure scenario.

Scenario: update(alice, lastname=["A","B"]) on a single-valued attribute throws IllegalArgumentException, but alice's lastname is already gone from memory and then from the file.

Suggestion: a separate issue, not a change in this PR. Compute values and run the size check before the removal, as create() validates before appendChild. Once that is fixed, B2 row 4 needs a different mid-mutation failure to pin markDirty().

2. A stamp of UNKNOWN after a failed save drops the new document (XMLHandlerImpl.java:137, :441)

If FileStamp.read() in dispose()'s finally hits an IOException other than NoSuchFileException, stamp becomes UNKNOWN. sameState(UNKNOWN) is always false, so fileAppearedOverNewDocument() takes the connector's own partial write for a foreign file.

Scenario: a new document's first save fails midway and leaves a partial file, and readAttributes in finally fails transiently. On the next init(), isFile() is true and the stamp does not match, so the change is dropped with the ERROR "appeared before the new document was saved", and the truncated file is parsed (SAXParseException until someone fixes it). This takes two failures in a row, so it is unlikely. A cheap guard: in finally, keep the previous stamp when the new read returns UNKNOWN, or do not treat UNKNOWN as "appeared" after the connector's own failed save.

3. Optional: loadDocument keeps the whole file as a byte[] during the parse (XMLHandlerImpl.java:570-573)

Files.readAllBytes holds a file-sized buffer through the parse, and the CRC-32 is computed inline, apart from FileStamp.checksum. Passing a CheckedInputStream over Files.newInputStream as the InputSource byte stream keeps the property from rows 26-27 (the parsed bytes are the checksummed bytes) without the buffer, and gives one CRC path. The saving is small next to the DOM, so this is a minor point. Caveat: the CRC matches FileStamp.checksum only if the parser reads to EOF. Xerces does, because it scans trailing Misc, but a test should cover it.

4. Optional: a predicate with side effects inside || (XMLHandlerImpl.java:122, :421)

fileAppearedOverNewDocument() returns a boolean but also nulls the document, clears dirty and logs an ERROR, and it is called from init()'s || chain and from dispose()'s guard. Whether the state changes depends on where the call sits in the short-circuit: reordering the arms of init(), or one more call for logging, silently changes behaviour. A pure fileAppeared() check plus an explicit dropNewDocument() at each of the two call sites would be clearer. This is a style point.

[maximthomas]

Answers to review 5471660670 (of de0ab50a). The branch is now at ef7e35b0: six commits on top of 946df85c, which is de0ab50a rebased onto #176's faf5d158 with no patch of this PR changed.

1. update() validates after the removal. Agreed: this is not from this PR and stays out of it. Filed as #178. Moving the size check up is not enough by itself. update() checks each attribute just before it replaces it, so a later attribute that is rejected (not supported, not updatable, wrong type) leaves the earlier ones replaced. The issue proposes checking all of replaceAttributes before the first change, and notes that failedUpdateLeavesTheFileInAgreementWithMemory (B2 row 4) will then need another mid-mutation failure.

2. UNKNOWN stamp after a failed save. Confirmed and fixed in 1d220ab. The new test reads the stamp through a link to itself (ELOOP) while a save fails and leaves a partial file. At 16fc3757 the next init() drops the document and throws SAXParseException: Premature end of file.

• Keeping the previous stamp does not fix it. For a new document the previous stamp is MISSING, and the partial file does not match MISSING either: the same test stays red with that change.
• The fix is the second guard you proposed: fileAppearedOverNewDocument() does not treat an unknown stamp as a file that appeared.
• For that rule to apply only after the connector's own save, 16fc375 makes createDocument() start from FileStamp.MISSING instead of reading the stamp again. That also closes a window between buildDocument()'s exists() and that read. A file that appeared in the window was taken for the connector's own, and the first dispose() overwrote it with the empty document, with no log line (fileThatAppearsAsTheNewDocumentIsCreatedIsNotOverwritten, red at 57df639c).
• The retry after such a failure logs UPDATE COLLISION against the connector's own partial write. The body lists this under Known limits.

3. Streamed load. Done in 42b24ec: the parser reads the file through a CheckedInputStream.

• On the caveat: Xerces 2.6.2 read to EOF in every file of a probe (48 files: with and without a BOM, ISO-8859-1, 100 KB of trailing white space, comments and a processing instruction after the root). The new test racyLoadWithMarkupAfterTheRootIsNotParsedAgain puts 100 KB of comment after the root.
• A parser that stopped earlier would cost reloads inside the racy window, not a missed change, because a different CRC reads as a change.
• Xerces reads in blocks (about 3,150 reads for a 6.4 MB file; single-byte reads only for the XML declaration), so the stream is not wrapped in a buffer.

4. Predicate with side effects. Done in 57df639: fileAppearedOverNewDocument() is a plain check, and init() and dispose() call dropNewDocument(). Behaviour is unchanged.

While answering point 2 I found that B2 row 17 (the stamp createDocument() sets) had lost its pin with T3 (946df85c). The collision check now skips a path with no file, so the whole module stayed green with the assignment removed. ef7e35b adds loadedStoreMovedBackBeforeTheNewDocumentIsSavedIsKept. 6e0346c fixes the SKIP condition of the point-2 test, which called the method under test.

The PR body has a new "Fourth review fixes" table (V1-V6), with updated line numbers, counts and Known limits. mvn -o -pl OpenICF-xml-connector verify at ef7e35b0, on JDK 26 and on JDK 11: surefire 196 run, 0 failed, 0 skipped; failsafe 1 run, 0 failed.

[vharseko]

Review of the fixes for review 5471660670: the six commits from 57df639c to ef7e35b0 on top of 946df85c, checked against the answer in 6085280873 and the "Fourth review fixes" table. All four points are closed, and I found no new correctness bug.

• 2 (UNKNOWN stamp). fileAppearedOverNewDocument() now has stamp.isKnown(). 16fc375 makes the Javadoc's premise true: createDocument() starts from FileStamp.MISSING, so the only way a new document gets an UNKNOWN stamp is the finally of a failed save. It also closes the exists()-to-read window (V2).
• 3 (streamed load). The CheckedInputStream sees the bytes the parser reads, the stamp is still taken before the open, and a parser that stops early only costs a reload.
• 4 (side effects). init() split in two has the same truth table as the old || chain. The document != null guard is equivalent, as the body says.
• V3. It restores the pin that B2 row 17 lost with 946df85c.

Checked locally at ef7e35b0: XMLHandlerReloadTests + FileStampTests, 52 run, 0 failed, 0 skipped. With stamp.isKnown() && removed, only ownPartialWriteBehindAnUnreadableStampIsSavedAgain fails (SAXParseException: Premature end of file). With createDocument() reading the stamp again, only fileThatAppearsAsTheNewDocumentIsCreatedIsNotOverwritten fails. Both match rows V4 and V2.

Two points to address:

1. Known limits names only one side of the UNKNOWN rule

After a new document's save fails and its stamp cannot be read, any file at the path is taken for the connector's own partial write. The body lists the harmless consequence: the retry logs a false UPDATE COLLISION. It does not list the other one. A store written there by someone else in that window is overwritten, with an UPDATE COLLISION line. Before 1d220ab that store was kept and the new document was dropped. Choosing the connector's side is reasonable: it takes two failures plus an outside writer. I'd still add one sentence under Known limits, so the trade-off is written down.

2. StatFailingFile redirects every toPath(), not only the stamp

ownPartialWriteBehindAnUnreadableStampIsSavedAgain works because FileStamp.read goes through toPath(), while XmlDocumentWriter.write opens the file with new FileOutputStream(File) and isFile() uses the path string. If the writer ever moves to Files.newOutputStream(file.toPath()), the save fails on ELOOP before writing a byte. The case then fails at assertTrue(file.exists()) with no hint why. One line in the fixture's Javadoc saying which callers it diverts would help whoever hits that.

[maximthomas]

Answers to review 5480057341 (of ef7e35b0). The branch is now at c7f327f6, one commit on top of ef7e35b0.

1. Known limits. Added. The UNKNOWN bullet now gives both sides. Any file at the path is taken for the connector's own partial write, so the retry logs a false UPDATE COLLISION against that write; a store that someone else wrote there before the retry is overwritten, with the same line, where before 1d220ab it was kept and the new document was dropped. The bullet also says why the connector's side is taken: losing a store this way needs both failures and an outside writer in between, while giving way made every later call parse the connector's own partial write and fail (V4).

2. StatFailingFile. Done in c7f327f. The fixture's Javadoc now says that only callers of toPath() are diverted, that isFile() and the FileOutputStream of XmlDocumentWriter.write use the path string, and what a writer that opened toPath() would do. Checked with such a writer (Files.newOutputStream(file.toPath()) in XmlDocumentWriter.write): the case fails at XMLHandlerReloadTests.java:563, assertTrue(file.exists()), as you described.

The PR body has the new Known limits text, a note on this review, the commit count (24) and the tip. mvn -o -pl OpenICF-xml-connector verify at c7f327f6, on JDK 26 and on JDK 11: surefire 196 run, 0 failed, 0 skipped; failsafe 1 run, 0 failed.