[#171] Skip CSV snapshot rows without a uid and delete the snapshots a failed sync copied

[maximthomas]

Fixes #171.

CSVFileConnector.sync() no longer fails on a snapshot row without a uid, and a failed sync no longer leaves behind the snapshots it copied.

• The delete pass skips a snapshot row whose uid cell is empty or blank and logs a warning with the line and the file. Such a row was never reported: a live row without a uid fails the sync before any delta. A sync from a snapshot that has one now reaches handleResult. The token moves with the next change; until then every poll logs the warning again.
• sync() copies the live file before the diff when the token is null or names a missing snapshot, and again after the diff when it reports changes. When the sync fails before the handler accepts the token, it now deletes the copies it made, under the write lock. A kept copy could be returned by getLatestSyncToken, and it stopped a retry on the unchanged live file: sync(null) failed with "Unable to copy CSV file for sync operation", and a sync from the old snapshot reported the same changes again without moving the token. A snapshot named by the caller's token is never deleted, and neither is the copy made before the diff once the handler has returned from a delta: every delta carries that copy's token, and a consumer that stores the token of each handled delta resumes from it.

getLatestSyncToken takes only the read lock, so a call that overlaps the failing sync can still return a copy's token before the copy is deleted. A sync from that token then starts over from the live file, as it does for any token without a snapshot, and never reports the changes made since that token. A sync from that token that takes the write lock before the copy is deleted runs from the copy, but if it reports no change it hands back the same token, and the sync after it starts over.

Test strength

arm	case	mutant killed	red at
CSVFileConnector.java:577 delete pass, null uid → skip	snapshotRowWithoutUidIsSkipped[",Jane,Doe,pw1"]	no guard	ccb33e9
same, blank uid	snapshotRowWithoutUidIsSkipped["\" \",Jane,Doe,pw1"]	uid == null instead of StringUtil.isBlank	ccb33e9
same, warning	skippedSnapshotRowIsLogged	warning removed	ccb33e9
CSVFileConnector.java:681 finally deletes the copy	failedSyncDeletesTheSnapshotItCopied	no cleanup	ac832fe
CSVFileConnector.java:497 same when the sync fails before it holds the read lock	syncThatCannotListItsSnapshotsDeletesTheSnapshotItCopied (POSIX, non-root)	no cleanup in the write-locked block	cb7965d
keep: token's snapshot	failedSyncKeepsTheSnapshotOfItsToken	deletes syncOrigin	that mutant only (green at ac832fe)
keep: completed sync	successfulSyncKeepsTheSnapshotItCopied	no !completed	that mutant only (green at ac832fe)
completed after handleResult	syncWhoseTokenIsNotDeliveredDeletesTheSnapshotItCopied	completed set before the token block	ac832fe
CSVFileConnector.java:670 the copy made after the diff is tracked	syncWhoseTokenIsNotDeliveredDeletesTheSnapshotItCopiedAfterTheDiff	that copy not added to copies	ac832fe
CSVFileConnector.java:638 a handled delta keeps the copy	failedSyncKeepsTheSnapshotOfADeltaItHandled	no copies.clear()	0390dc5
same, only after handle returns	syncWhoseFirstDeltaFailsDeletesTheSnapshotItCopied	copies.clear() before handle	that mutant only (green at 0390dc5)
CSVFileConnector.java:588 same in the delete pass	failedSyncKeepsTheSnapshotOfADeleteItHandled (relies on the header cached at init: the live columns are permuted after it)	no copies.clear()	0390dc5
deleteSnapshot warning	snapshotThatCannotBeDeletedIsLogged (POSIX, non-root)	result of delete() ignored	ac832fe
deleteSnapshot write lock	—	unpinnable by construction: another sync in this JVM can enter its read section only between readLock().unlock() and writeLock().lock(), a window a test cannot open or hold without the private static lock map. The lock keeps a copy from being deleted while another sync reads it; it does not stop getLatestSyncToken from returning the copy's token earlier	—

[vharseko]

nit (non-blocking): SyncOpDiffTest.java:337 — syncThatCannotListItsSnapshotsDeletesTheSnapshotItCopied expects exactly NullPointerException, which is what the unchecked dataDir.list() in scrubSyncFiles happens to throw today. The test is about the cleanup, not the exception type: once #174 reworks scrubSyncFiles (e.g. a ConnectorIOException on an unlistable directory), this test goes red although the copy is still deleted. expectThrows(RuntimeException.class, ...) would keep it pinned to the cleanup only.

Not raised here, as they are already tracked: the start-copy collision and the token kept when <csv>.<mtime> exists (#174), snapshot retention (#174), the handler-stop semantics (#175), and the external-writer race (#172).

[maximthomas]

Re review 5493443471: agreed, the test is about the cleanup and the NPE is incidental. Changing only the expected type would open another gap, though: RuntimeException also matches the ConnectorException("Unable to copy CSV file for sync operation") that a sync throws before any copy exists, and the empty-directory assertion then holds with nothing to clean up.

535f9c4 widens the expectation to RuntimeException and adds a witness that the copy was made. Before the sync the test sets the directory's mtime to 0; afterwards it asserts that the mtime moved. Only creating or deleting an entry in the directory moves it.

syncThatCannotListItsSnapshotsDeletesTheSnapshotItCopied, run alone on macOS against four mutants of CSVFileConnector (green = the mutant survives):

expectation	no cleanup in the write-locked block	copy fails first	directory listed before the copy	ConnectorIOException on an unlistable directory (#174-like)
NullPointerException (8bbbe4c)	red	red	green	red
RuntimeException only	red	green	green	green
RuntimeException + mtime witness (535f9c4)	red	red	red	green

Without mutants the test is green, and so is the whole module (122 tests, 0 skipped). The "red at cb7965d" cell in the description still holds: at cb7965d the test fails on the leftover copy.

[vharseko]

Re 6140592007: thanks — the mtime witness closes the gap I missed in my own suggestion; RuntimeException alone would indeed have accepted the pre-copy ConnectorException with nothing to clean up.

Checked 535f9c4 locally (macOS, JDK 21):

• SyncOpDiffTest: 34 tests, 0 failures, 0 skipped (the POSIX test ran).
• ConnectorException thrown before Files.copy ("copy fails first"): red, on the new assertion The sync failed before it copied the CSV file.
• ConnectorIOException on an unlistable directory in scrubSyncFiles (#174-like): green.

Both match your table. No further findings.