[maximthomas]

The XML connector no longer writes its store in place. XmlDocumentWriter.write writes a copy next to the store, syncs it to disk and renames it over the store, so after a save the store is either as it was or complete. create, update and delete now save before they return: a save that fails makes the call throw, and the change is undone.

Before, the store was saved only in dispose(), which the framework calls after every operation, through new FileOutputStream(file). That truncated the store before Saxon wrote it again, so a crash, a kill or a full disk in that window left a truncated store, and every later operation failed with SAXParseException until someone repaired the file. A save that failed was only logged by the framework (it logs what dispose() throws): the call returned success, and the change was lost when the next call loaded the file again.

case	before	after
create whose save fails after it has started (a full disk; in the tests, a value Saxon cannot encode, "x\uD800y")	returns a uid; the store is left partial, and every later call fails with SAXParseException	throws ConnectorException (Saxon reports a full disk the same way as a value it cannot encode, see below); the store is unchanged, no copy is left, and the entry is gone from memory too
create, update or delete on a read-only store file	returns success; the change is gone after the next call	ConnectorIOException caused by AccessDeniedException: <store>: the file is read-only; the store reads as before
the process is killed while it saves	a truncated store	the store as it was, or complete; a <store>.<digits>.tmp copy may be left next to it
a change to a new store, while a file appeared at its path that the connector never loaded	returns success; dispose() keeps the file and drops the change	ConnectorException: <path> appeared before the new document was saved; the change is not saved, and the file is loaded instead

How the store is written (XmlDocumentWriter.write):

• The copy is Files.createTempFile(<store's directory>, <store's name> + ".", ".tmp"): a new name in the store's directory, readable by its owner only on POSIX file systems while it is written.
• After the document is written, the copy is synced with FileDescriptor.sync() (an fsync). Unlike FileChannel.force, it does not fail on a thread whose interrupt flag is set, so a save on such a thread succeeds, as it did before. The copy then takes the store's group when the process may set it; otherwise it keeps the group it was created with (the process's group, or the directory's on BSD and macOS and in a setgid directory) and gets no group permissions. It also takes the store's mode, the way CSVFileConnector.finishRewrite does (Rewrite the CSV file through a private copy next to it and never replace it with a fragment #131). Then Files.move(copy, store, REPLACE_EXISTING, ATOMIC_MOVE), without ATOMIC_MOVE where the file system cannot do it, and a best-effort sync of the directory.
• Whatever fails before the rename completes, the copy is deleted and the store is not touched. A copy that cannot be deleted is logged.
• The exception is a file system that cannot rename atomically (ATOMIC_MOVE gives AtomicMoveNotSupportedException, which a local file system never does within one directory). There the store is replaced by a plain move, which removes it first, so a failure in that window loses the store. Such a file system could save before, and the fallback keeps it saving.
• A store file that Files.isWritable rejects is refused with AccessDeniedException before anything is written. Without this check the rename would replace a read-only file, which FileOutputStream refused to open.
• A store reached through a symbolic link keeps the link: the copy is written next to the real file and renamed over it. A link to a file that does not exist yet is followed, as FileOutputStream followed it: the file is created behind the link, and a link into a missing directory fails with NoSuchFileException. After 40 such links (a cycle, for one) the save fails with FileSystemException: <path>: Too many levels of symbolic links.
• A new store file is created readable by its owner only (rw-------) on POSIX file systems; before, it got the process's umask (usually rw-r--r--). Elsewhere it takes the directory's inherited permissions.
• write returns the CRC-32 of the bytes now in the file.

How a change is saved (XMLHandlerImpl):

• create, update and delete save at their end, inside ConcurrentXMLHandler's write lock. A call that changed nothing (an update with no attributes) does not write the file.
• An IOException from the steps around Saxon's serializer throws ConnectorIOException: the store cannot be opened, created, synced or renamed (a read-only store, a directory that cannot be written). A failure inside the serializer throws ConnectorException, as before, and so does a full disk on APFS: Saxon reports it as a SAXException caused by IOException: No space left on device, the same way it reports a value it cannot encode. Linux file systems were not probed.
• The change is undone by loading the file again, also after an Error such as an OutOfMemoryError, so another caller that holds the handler at the same time sees the file's content, not the refused change. If that load fails too (a directory at the path), its exception is added to the save's as suppressed; when both are one instance (HotSpot throws one shared OutOfMemoryError once its preallocated ones are used up), that instance is thrown as it is. The following dispose() has nothing to save, and the next call loads the file again.
• dispose() still saves what is left, as before, and only logs a failure: a new empty store (createFileIfNotExists), and the change of an update that failed midway (XML connector: update, delete and authenticate break on an entry below another element #182).
• The file's stamp is taken only after a save succeeds. A failed save writes nothing at the path, so a file that is there over a new document is never the connector's own partial write, and fileAppearedOverNewDocument() is now unsavedNewFile && file.isFile().

What operators may notice:

• A store whose directory is not writable now fails every save, with AccessDeniedException naming <store>.<digits>.tmp. Before, a writable store file in such a directory was written in place.
• A store that is a single-file bind mount (a Docker or Kubernetes volume mounted over the file itself) now fails every save: the rename gives EBUSY. Mount the directory that holds the store instead.
• On Windows, replacing the store needs DELETE access to it, and fails with a sharing violation while another process holds it open without FILE_SHARE_DELETE (a java.io reader in another JVM, some editors, backup or antivirus tools).
• In a directory with the sticky bit set, the process must own the store.
• A save needs free space for a second full copy of the store.
• A store whose file name is longer than about 230 bytes can no longer be saved: the copy's name adds up to 25 (a dot, up to 20 digits and .tmp), past the usual limit of 255 bytes.
• While several callers use one store at once (ConcurrentXMLHandler with more than one invoker), each change now writes the whole file before it returns; before, the file was written once, when the last caller left. See the second table below.

What a save costs

XmlPerf (one store of N accounts: 2.9 MB at 10,000, 11.7 MB at 40,000), 5 rounds of one JVM per build after a discarded warm-up round, builds alternating in each round; a cell is the median (min-max) of the 5 JVMs, each value the median of 5 calls. nosync is this branch with the copy's FileDescriptor.sync() removed (the directory sync kept). macOS 15.7, Intel i7-4850HQ, internal SSD (APFS), JDK 26; load average 2.6-5.4.

facade.create (init, create, dispose; one write of the file per call in every build), ms:

build	10,000 entries	40,000 entries
master	58 (54-93)	249 (243-285)
this branch	80 (70-107)	308 (302-355)
nosync	73 (70-89)	296 (295-328)

facade.create while another connector instance holds the store, ms:

build	10,000 entries	40,000 entries
master	2 (2-3)	2 (1-2)
this branch	66 (62-78)	216 (210-228)
nosync	63 (61-65)	214 (212-290)

The copy's sync costs 2-12 ms per save here (the medians of this branch and nosync), 1-9% of a create, and the ranges overlap. This branch is 22 ms (10,000) and 59 ms (40,000 entries) slower than master; nosync is still 15 and 47 ms slower, in the other steps of the replace that it keeps: a new file, the group and mode copy, the rename and the directory sync. The cost of an fsync depends on the disk and the file system; this is one SSD under APFS. Under overlapping callers a create now writes the whole store before it returns, where master wrote nothing until the last caller left.

Not fixed here

• Hard links to the store, its owner, its ACLs (POSIX ACL entries as well) and its extended attributes are not kept. On Windows the new file takes the directory's inheritable ACL, so a store whose own ACL is narrower than its directory's is widened by its first save.
• A copy left behind by a process that was killed while it saved is not removed automatically. It is a full copy of the store, which can hold __PASSWORD__ values in clear text. On POSIX it is readable by its owner only, or has the store's mode if the process died after the copy took it.
• When the load after a failed save fails too, other callers that hold the handler get Data file does not exists until the last one leaves.
• An update that fails midway (XML connector: update, delete and authenticate break on an entry below another element #182) throws before it saves, but dispose() still saves its partial change, as before.
• On macOS fsync does not flush the drive's cache (F_FULLFSYNC does, and FileChannel.force issues it there), so a save there is not guaranteed to survive a power loss. On Linux FileDescriptor.sync() and FileChannel.force(true) are both fsync.

Not pinned by any test:

• the copy's sync (a test pins only that it works on an interrupted thread), the directory sync, the group copy and the fallback without ATOMIC_MOVE;
• the if (dirty) branch of dropNewDocument (reachable only if appendChild or the index update in create throws);
• taking the stamp only after a successful save (the same result, except for a file that appears between the check and the stamp).

Tests

Nine tests of XMLHandlerReloadTests pinned behaviour that no longer exists, and are removed:

• four pinned how a new document dealt with its own partial write after a failed save: retryAfterAFailedSaveDoesNotReportItsOwnPartialWrite, ownPartialWriteOfANewDocumentIsSavedAgain, ownPartialWriteOfAnUnchangedNewDocumentIsNotLoaded, ownPartialWriteBehindAnUnreadableStampIsSavedAgain (with their StatFailingFile fixture). A failed save no longer writes at the path; failedSaveOfANewDocumentWritesNoFile pins that.
• five pinned what dispose() did with a change that waited in memory: kept and retried after a failed save, or dropped for a file that appeared: failedSaveKeepsTheChangeInMemoryAndRetriesIt, changedNewDocumentGivesWayToAFileThatAppears, fileThatAppearsDuringAChangeToANewDocumentIsKept, changeToANewDocumentIsSavedOnceThePathIsFree, changesDroppedForAFileThatAppearsStayDroppedWhenItGoes. A change no longer waits for dispose().

outsideEditDuringAChangeIsReportedAndOverwritten now expects the UPDATE COLLISION at the change, not at dispose(). XMLConnectorTests#failedSaveIsLoggedAndThrown now expects a java.nio.file exception in the log line, not FileNotFoundException.

arm	case	mutant killed	red at master
XmlDocumentWriter.java:166 the copy, in the store's directory	XmlDocumentWriterTests#failedWriteLeavesTheFileAsItWas (an unencodable value: the store's bytes unchanged, no copy left); #copyIsPrivateAndNextToTheFileWhileItIsWritten; #newFileIsReadableByItsOwnerOnly	an in-place write; a copy in java.io.tmpdir; a copy created with the umask's mode	yes
:166 a store in a directory that cannot be written is refused	#storeInADirectoryThatCannotBeWrittenIsRefusedAndLeftAlone	an in-place write where the directory cannot be written	yes
:191-193, :231 the store's mode, after the write	copyIsPrivateAndNextToTheFileWhileItIsWritten (store rw-r-----: rw------- while written, rw-r----- after)	mode copied before the write; mode not set	yes
:161 a link to the store is resolved	#fileReachedThroughALinkIsReplacedBehindTheLink	no toRealPath()	no: FileOutputStream follows links
:153-159 a dangling link is followed, at most 40 hops	#danglingLinkKeepsTheLinkAndCreatesTheFileBehindIt; #danglingLinkIntoAMissingDirectoryFailsAndKeepsTheLink; #linkCycleIsRefused (10 s timeout); #relativeDanglingLinksAreFollowedFromEachLinksDirectory (a chain of two links with relative content)	loop removed (all three); bound removed (the cycle times out); the content not resolved against the link's directory; a bound of 1 (the last two: the chain)	the cycle only (FileNotFoundException); the other three keep master's behaviour
:162-164 a read-only store is refused	#readOnlyFileIsRefusedAndLeftAlone; XMLConnectorTests#changeToAReadOnlyStoreFailsAndLosesNothing (create, update, delete)	no check	yes
:202-206 the copy is deleted on failure	failedWriteLeavesTheFileAsItWas (the write fails); #failedRenameLeavesNoCopy (a directory at the store's path: the rename fails)	no delete; replaced = true before the rename	the first only
:177, :189 the copy is written and synced without a channel	#writeOnAnInterruptedThreadSucceedsAndKeepsTheFlag (the file written, the flag still set)	FileChannel.open(copy, WRITE).force(true) after the write; the stream's getChannel().force(true) (both throw ClosedByInterruptException)	no: master wrote through FileOutputStream
XMLHandlerImpl.java:286, :397, :418 save before the call returns	XMLHandlerReloadTests#createIsInTheFileBeforeTheCallReturns, #updateIsInTheFileBeforeTheCallReturns, #deleteIsInTheFileBeforeTheCallReturns; changeToAReadOnlyStoreFailsAndLosesNothing	each call removed	yes
:569 nothing changed, nothing written	#updateWithNoAttributesLeavesTheFileAlone (kept test: the mtime)	check removed	no
:578-585 a failed save undoes the change	XMLConnectorTests#failedSaveFailsTheCreateAndLosesNothing; changeToAReadOnlyStoreFailsAndLosesNothing; #failedSaveUndoesTheChangeForEveryUser (another caller holds the handler); #failedFirstSaveOfANewDocumentDropsTheChange; #changeToANewDocumentFailsWhenAFileAppeared; #saveFailureIsThrownWhenTheFileCannotBeReadAgain	no undo	yes
:578 an Error from the save undoes the change too	#errorDuringASaveUndoesTheChange (an OutOfMemoryError from the save: the entry is gone from memory, dispose() writes nothing)	catch (RuntimeException e)	yes
:591-599 the undo's steps	dirty = false: failedSaveFailsTheCreateAndLosesNothing, failedFirstSaveOfANewDocumentDropsTheChange, failedSaveUndoesTheChangeForEveryUser, saveFailureIsThrownWhenTheFileCannotBeReadAgain; unsavedNewFile = false: failedFirstSaveOfANewDocumentDropsTheChange; buildDocument(): changeToANewDocumentFailsWhenAFileAppeared, failedSaveUndoesTheChangeForEveryUser, saveFailureIsThrownWhenTheFileCannotBeReadAgain	each step removed	yes
:579-585 a failed load is added as suppressed	saveFailureIsThrownWhenTheFileCannotBeReadAgain; #errorWhileTheFileIsReadAgainIsAddedToTheSaveError (both throw an Error)	load not guarded (its exception replaces the save's); suppressed dropped; catch (RuntimeException reloadFailure) (the second test)	yes
:582 the save's own exception is not added to itself	#saveErrorThrownAgainByTheReloadIsThrownAsItIs (the save and the load throw one OutOfMemoryError instance: the caller gets that instance, with nothing suppressed, and dispose() writes nothing); saveFailureIsThrownWhenTheFileCannotBeReadAgain, errorWhileTheFileIsReadAgainIsAddedToTheSaveError (two instances)	guard removed (IllegalArgumentException: Self-suppression not permitted replaces the error); a guard that is never true (the last two)	yes
:556 IOException -> ConnectorIOException	changeToAReadOnlyStoreFailsAndLosesNothing; saveFailureIsThrownWhenTheFileCannotBeReadAgain	ConnectorException.wrap	yes
:555 the log line names the cause	XMLConnectorTests#failedSaveIsLoggedAndThrown	the cause left out	yes (master logged java.io.FileNotFoundException)
:573-576 a file that appeared over a new document refuses the change	changeToANewDocumentFailsWhenAFileAppeared	check removed	yes
:530 dispose()'s failed save of a new document writes no file	#failedSaveOfANewDocumentWritesNoFile	not run separately (the writer's rows)	yes (a partial file)
:541-543 a collision is reported at the change	#outsideEditDuringAChangeIsReportedAndOverwritten	saveChange() removed from create	yes

"Red at master": the branch's tests run against master's main code (4648e38): 136 tests of the three classes, 23 failures (21 tests; changeToAReadOnlyStoreFailsAndLosesNothing has 3 rows). Every mutant was run against the branch's tip with the test classes of its row; each was killed.

mvn -pl OpenICF-xml-connector verify: 266 unit tests (248 − 9 + 27) and 1 IT, green on JDK 26 and JDK 11.

Fixes #160

[vharseko]

Reviewed the head 20d1450d against the description. The copy-and-rename write, the undo of a failed save and the stamp handling all hold up. I found one small thing.

XMLHandlerImpl.saveChange() (line 582): guard addSuppressed against self-suppression. Once a JVM has thrown four heap OutOfMemoryErrors, HotSpot stops handing out preallocated errors. From then on it throws one shared instance with no stack trace. On JDK 21, OOMs 5 to 8 were the same object. Suppose the save runs out of heap and the reload does too, which is likely because the reload parses the whole store. Then save() and revert() throw that same instance, and e.addSuppressed(reloadFailure) throws IllegalArgumentException: Self-suppression not permitted from inside the catch block. The caller gets an IllegalArgumentException, with the OOM only as its cause, instead of the Error, and throw e is never reached. C2's preallocated fast-throw exceptions can collide the same way (-XX:+OmitStackTraceInFastThrow is on by default). errorWhileTheFileIsReadAgainIsAddedToTheSaveError uses two distinct errors, so it does not catch this.

} catch (RuntimeException | Error reloadFailure) {
    if (reloadFailure != e) {
        e.addSuppressed(reloadFailure);
    }
}

[maximthomas]

Fixed in 626793cb: saveChange() adds the load's failure as suppressed only when it is not the save's own exception (XMLHandlerImpl.java:582).

I checked the premise first. With -Xmx64m, a loop that allocates new long[Integer.MAX_VALUE - 8] eight times gets distinct OutOfMemoryErrors and then one shared instance with no stack trace: from the fifth on JDK 21.0.2 and JDK 26, from the third on JDK 11.0.25. On all three, addSuppressed on that instance with itself throws IllegalArgumentException: Self-suppression not permitted.

The new test XMLHandlerReloadTests#saveErrorThrownAgainByTheReloadIsThrownAsItIs injects one OutOfMemoryError instance into both the save and the load. At 20d1450d it fails with Expected OutOfMemoryError to be thrown, but IllegalArgumentException was thrown, caused by Self-suppression not permitted. With the guard, the caller gets that instance with nothing suppressed, and the following dispose() writes nothing. The other side is pinned too: a guard that is never true fails errorWhileTheFileIsReadAgainIsAddedToTheSaveError and saveFailureIsThrownWhenTheFileCannotBeReadAgain.

mvn -pl OpenICF-xml-connector verify: 266 unit tests and 1 IT, green on JDK 26 and JDK 11. The description now has the sentence on a load that throws the save's own exception, a row for :582 in the tests table, the shifted line numbers and the new counts (red at master: 136 tests, 23 failures).

[vharseko]

Reviewed 626793cb against my previous round. The fix holds.

saveChange() now adds the reload's failure as suppressed only when it is a different instance (XMLHandlerImpl.java:582), so a shared OutOfMemoryError thrown by both the save and the reload reaches the caller as it is, instead of IllegalArgumentException: Self-suppression not permitted.

I ran the new saveErrorThrownAgainByTheReloadIsThrownAsItIs locally on JDK 21. It is green at 626793cb. With the guard removed it fails with Expected OutOfMemoryError to be thrown, but IllegalArgumentException was thrown. The two-instance tests errorWhileTheFileIsReadAgainIsAddedToTheSaveError and saveFailureIsThrownWhenTheFileCannotBeReadAgain still pass and pin the other side. The line references in the description match the head, and CI is green.

Approving.