Fix CodeQL note-severity alerts: ignored error status of file and stream calls

[vharseko]

Addresses 37 of the 40 open java/ignored-error-status-of-call code scanning alerts (all alerts still open in the repository are note severity; the warning/error ones were fixed in #791 and #793).

Real defects behind the alerts

• SizeLimitInputStream.skip() credited the requested byte count instead of what the parent stream actually skipped, so getBytesRead() could over-report. ASN1InputStreamReader now skips trailing SEQUENCE bytes in a loop — InputStream.skip may legitimately skip fewer bytes than requested without reaching EOF, which is exactly what BufferedInputStream does — and raises a fatal decode error when they cannot be skipped, instead of silently leaving the reader in the middle of the sequence. The same helper is used by skipElement(), which also removes spurious "truncated value" failures on buffered input.
• MultifileTextWriter.rotate() reported a successful rotation even when renameTo failed, and then re-opened the un-rotated file with append=false, truncating it. The file is now kept and appended to, totalFilesRotated is not incremented, and the failure is reported — but only after the writer has been re-opened, and behind a latch:
  • the report has to reach a usable writer. When this writer backs the error log (the shipped cn=File-Based Error Logger is synchronous, logs error, and has a size limit rotation policy attached), logging from inside rotate() comes straight back into writeRecord() on the same thread through the reentrant monitor. Logging before constructWriter(...) sent the message to a writer which had just been closed, and left it still over the size limit, so the record triggered another rotation and the pair recursed until the stack blew up. The latch is set before the writer is re-opened, because constructWriter(...) logs permission warnings of its own which re-enter writeRecord() the same way.
  • the latch also stops a persistent failure (read-only log directory, or a target that cannot be replaced on Windows) from repeating flush + close + renameTo + re-open + an ERROR line for every single record. Retries are left to the RotaterThread, once per interval, and the latch is cleared by the first successful rename.
  • lastRotationTime is only refreshed on a successful rotation, so a time based policy asks for a rotation again on the next interval rather than going quiet for a whole period.
• A log file that a retention policy fails to delete is no longer counted in totalFilesCleaned / lastCleanCount, and the failure is reported once.
• LDIFExportConfig creates the export file atomically in FAIL mode, closing the TOCTOU window between exists() and the creation, and only changes permissions of a file it created itself.
• The referential integrity plugin now reports an update log file it could not delete and recreate; previously its entries were silently replayed again on the next startup. The message names the file and states that the records will be processed again.

Silently dropped write failures

Configuration (.startok, config-changes.ldif), compressed schema, concatenated schema, task state, LDIF backend and backup descriptor updates are moved into place with the existing StaticUtils.renameFile(), which throws on failure so that the surrounding handlers emit their existing messages and alerts instead of leaving the new data in the temporary file. Redundant explicit deletes of the rename targets were dropped, as renameFile() performs and checks them.

One exception: in DefaultCompressedSchema.save(), keeping the previous token data as schematokens.dat.save stays best effort and is only logged. save() runs on the entry encoding path, so making that step fatal would turn a backup problem into a failed add or modify. Only the .tmp → live swing fails the operation.

Directories are created with Files.createDirectories(), a failed creation of the changelog last-rotation-time marker raises ChangelogException, and the remaining createNewFile() results are either consumed meaningfully or removed where the output stream creates the file anyway. The creation of the instance lib directory in InstallerHelper.writeSetOpenDSJavaHome() is removed rather than checked, since nothing reads that path any more.

Upgrade note

config-changes.ldif is now applied with renameFile(), so a failure to move config.ldif aside or to put the patched file in place propagates as an InitializationException instead of being ignored. A read-only config directory, or a stale config.ldif.prechanges that cannot be deleted, now prevents startup with an explicit error rather than silently starting from unpatched configuration.

Left as false positives

LDAPConnectionFactory:1035, LDAPPassThroughAuthenticationPolicyFactory:548 (unbounded ConcurrentLinkedQueue) and OnDiskMergeImporter:3126 (ArrayBlockingQueue filled to exactly its capacity) — offer() cannot return false there, so no code change was made.

Testing

New regression tests, each of which fails against the code it covers:

• SizeLimitInputStreamTestCase — getBytesRead() after a short skip() over a partially consumed BufferedInputStream, the cap at the size limit, and a skip that reaches the end of the parent stream.
• ASN1InputStreamReaderTestCase — readEndSequence() with unread trailing components and skipElement(), both over a BufferedInputStream whose skip() returns short without reaching EOF.
• MultifileTextWriterTestCase — a successful rotation, a failed rotation (rotated name pre-created as a non-empty directory, which renameTo refuses on every platform) asserting append-not-truncate and an unchanged totalFilesRotated, a failed rotation of a writer wired to a TextErrorLogPublisher, which reproduced the StackOverflowError, and a warning logged while the writer is re-opened after a failed rotation — hooked through the exists() check that FilePermission.setPermissions performs exactly where the permission warnings are logged — which recursed the same way.

Suites run:

• opendj-core: org.forgerock.opendj.io and com.forgerock.opendj.util — 676 tests, all passing.
• opendj-server-legacy (-Pprecommit): MultifileTextWriterTestCase, AbstractTextAccessLogPublisherTest, DebugLogPublisherTest, LDIFBackendTestCase, ReferentialIntegrityPluginTestCase, TestBackupAndRestore, BackupManagerTestCase, TaskBackendTestCase, SchemaBackendTestCase, TestImportAndExport — 369 tests, all passing. Earlier runs also covered FileReplicaDBTest, LogFileTest, LogTest, ReplicationEnvironmentTest, TestImportAndExport and AddSchemaFileTaskTestCase.

Not in scope

LDIFConnectionHandler.processLDIFFile() contains a pre-existing while (true) with no break, one line above code this PR touches. It is tracked as #828 rather than fixed here.

[maximthomas]

Nice sweep — the ASN.1 fixes and the renameFile() conversions are correct, and the three offer() false positives are rightly left alone. Verified locally: opendj-core + opendj-server-legacy compile clean, and org.forgerock.opendj.io passes (1582 invocations, 0 failures).

The MultifileTextWriter change, however, is reachable with the stock config.ldif and needs rework before merge.

Rotation failure recurses until StackOverflowError (blocker)

opendj-server-legacy/src/main/java/org/opends/server/loggers/MultifileTextWriter.java:611

The new logger.error(...) sits between writer.close() and constructWriter(...):

writer.flush(); writer.close();                      // writer now CLOSED
final boolean renamed = currentFile.renameTo(newFile);
if (!renamed) {
    logger.error(LocalizableMessage.raw("Unable to rotate log file %s to %s", ...));  // <-- here
}
constructWriter(currentFile, ..., append || !renamed, bufferSize);  // writer/outputStream replaced

When this instance backs the errors log, that call comes straight back in, on the same thread:

rotate()  [writer closed, outputStream still the old over-limit MeteredStream]
  -> logger.error(...) -> OpenDJLoggerAdapter.publish() -> ErrorLogger.log()
  -> TextErrorLogPublisher.log() -> writer.writeRecord()   // SAME instance
  -> synchronized(this) [reentrant] -> written + size + 1 >= sizeLimit
  -> rotate()  -> renameTo fails again -> logger.error(...) -> ...

Every gate passes with the shipped opendj-server-legacy/resource/config/config.ldif:

gate	value
ds-cfg-asynchronous (cn=File-Based Error Logger)	false — no async decoupling
ds-cfg-default-severity	includes error → isEnabledFor true
cn=Size Limit Rotation Policy attached	yes (100 MB) → sizeLimit > 0
outputStream.written after failed rename	seeded file.length(), append=true keeps it ≥ sizeLimit

The first failed rotation may or may not recurse; constructWriter(append=true) then writes the pending record, pushing the file past sizeLimit, so the next record recurses unconditionally. LogPublisherErrorHandler.handleCloseError has no latch (unlike handleWriteError), so stderr floods until the stack blows.

With sizeLimit == 0 (time-only rotation) you get the mirror image: the record goes to the closed writer and is lost, while lastRotationTime is refreshed anyway (TimeLimitRotationPolicy.java:72 reads it) — deferring the retry a full 7 days. Either way the operator never sees the message in logs/errors, which is the whole point of the fix.

Move the logger.error(...) below constructWriter(...) (where the existing logger.trace already lives).

Failed rotation triggers rotate() on every subsequent record (major)

Same method. constructWriter seeds MeteredStream with file.length(), so append || !renamed leaves the file at/over sizeLimit. writeRecord calls rotate() inline — it is not only driven by RotaterThread:

synchronized(this) {
  if (sizeLimit > 0 && outputStream.written + size + 1 >= sizeLimit) {
    rotate();          // MultifileTextWriter.java:552
  }
  ...
}

Under a persistent failure (read-only log dir; on Windows a target that exists or an open handle) every log record does flush + close + renameTo + reopen + an ERROR line. The old code truncated — data loss, but stable. Please latch the failure and suppress size-triggered attempts until the next RotaterThread tick or a successful rename.

Compressed-schema backup failure now fails LDAP writes (major)

opendj-server-legacy/src/main/java/org/opends/server/core/DefaultCompressedSchema.java:232

if (liveFile.exists()) {
  renameFile(liveFile, new File(liveFile.getAbsolutePath() + ".save"));   // now throws
}
renameFile(tempFile, liveFile);

save() is called from storeAttribute / storeObjectClasses (:65-77), i.e. the entry-encoding write path, and the enclosing catch (Exception) turns this into a DirectoryException with the server error result code. Previously a failed .save rename was ignored and the update still landed on POSIX. An undeletable .save now turns a cosmetic backup problem into a failed add/modify. Suggest keeping the .save step best-effort and making only the tempFile -> liveFile swing fatal.

Wrong argument for ERR_PLUGIN_REFERENT_REPLACE_LOGFILE (minor)

opendj-server-legacy/src/main/java/org/opends/server/plugins/ReferentialIntegrityPlugin.java:869

if (!logFile.delete() || !logFile.createNewFile()) {
  logger.error(ERR_PLUGIN_REFERENT_REPLACE_LOGFILE, logFileName);   // %s is the *reason*
}
} catch (IOException io) {
  logger.error(ERR_PLUGIN_REFERENT_REPLACE_LOGFILE, io.getMessage());

The catalog entry is An error occurred replacing the ... update log file: %s, where %s is the reason — as the sibling call two lines down shows. Passing the path yields ...update log file: /path/logs/referint. Pass a reason instead, and make it say the entries will be replayed.

Adjacent infinite loop (pre-existing, major)

opendj-server-legacy/src/main/java/org/opends/server/protocols/LDIFConnectionHandler.java:476-488 — the block directly above the line this PR changes:

if (new File(renamedPath).exists()) {
  int i=2;
  while (true) {
    if (! new File(renamedPath + "." + i).exists()) {
      renamedPath = renamedPath + "." + i;    // no break
    }
    i++;
  }
}

No break on any path, and renamedPath keeps growing (x.2, x.2.3, …) → thread hangs, String grows until OOM. Reachable when two failed LDIF runs land in the same GMT second. Not introduced here, but it is exactly this sweep's defect class, one line from the diff.

Nits

• Misleading rotate comment: // Do not overwrite an already rotated file — File.renameTo does overwrite on POSIX. With TimeStampNaming's second-granularity names (yyyyMMddHHmmss'Z'), two rotations in the same second silently destroy the earlier file and report success. renamed == false is in practice a Windows / read-only / EXDEV path.
• Same bug left in the same file: MultifileTextWriter.java:426-427 does file.delete(); totalFilesCleaned++; — structurally identical to the totalFilesRotated++-after-failed-renameTo bug fixed 180 lines below.
• Redundant deletes not dropped: still present immediately before a renameFile() that already deletes the target — types/BackupDirectory.java:336, backends/task/TaskScheduler.java:1258, config/ConfigurationHandler.java:825, backends/LDIFBackend.java:289. BackupDirectory.java:311 also still ignores dir.mkdirs() while equivalent sites were converted to Files.createDirectories.
• New failure surface for an unused directory: quicksetup/installer/InstallerHelper.java:984 — Files.createDirectories(libDir) is the last statement in the method and everything consuming libDir is commented out, yet it can now abort an install step where mkdir() was silent.
• Startup behaviour change: config/ConfigurationHandler.java:1612-1617 — config-changes.ldif rename failures now propagate to InitializationException, so a read-only config dir or a stale non-deletable config.ldif.prechanges prevents startup. Intended, but worth a release note. Also consider naming .prechanges in the message: if the second rename fails, config.ldif no longer exists.
• Raw message for a new ERROR: LocalizableMessage.raw in MultifileTextWriter bypasses the catalog. There is precedent, but a new ERROR in the logging subsystem is better as a LoggerMessages entry — as the ChangelogException added in this same PR correctly does.
• No tests: cheap and valuable additions — SizeLimitInputStream.getBytesRead() after a short skip(); readEndSequence() with trailing components over a BufferedInputStream (the case that motivated skipFully); and a MultifileTextWriter rotation-failure test (pre-create the rotated name) asserting append-not-truncate, totalFilesRotated unchanged, and no recursion.

_{Not re-run locally: the opendj-server-legacy suite (needs -Pprecommit). CI was still queued at review time, including all three windows-latest jobs — the ones most relevant to the renameTo behaviour above.}

[vharseko]

Thanks — the analysis of the rotation path is correct, including the reachability through the shipped config.ldif. I verified every step of the chain and reproduced the recursion, then fixed it. Pushed as a separate commit, d465627.

Rotation failure recurses until StackOverflowError (blocker) — fixed

Confirmed end to end: LocalizedLogger → OpenDJLoggerAdapter.publish() (loggers/slf4j/OpenDJLoggerAdapter.java:126,152) → ErrorLogger.log() iterating publishers synchronously (ErrorLogger.java:97-102) → TextErrorLogPublisher.log() → writer.writeRecord() (TextErrorLogPublisher.java:476), with this.writer being the MultifileTextWriter itself when asynchronous is false (TextErrorLogPublisher.java:133-139). All four gates you listed are open with the shipped config, and handleCloseError has no latch.

logger.error(...) now runs below constructWriter(...), and a rotationFailed latch is set before the call — the reentrant writeRecord() has to find a writer which was already re-opened and must not start another rotation:

if (renamed)
{
  logger.trace("Log file %s rotated and renamed to %s", currentFile, newFile);
  totalFilesRotated++;
  rotationFailed = false;
  lastRotationTime = TimeThread.getCalendar();
}
else if (!rotationFailed)
{
  rotationFailed = true;
  logger.error(ERR_LOGGER_ERROR_ROTATING_FILE, currentFile, newFile);
}

You were right about the sizeLimit == 0 variant too, so lastRotationTime is now only refreshed on a successful rotation. A time based policy therefore keeps asking for a rotation on every RotaterThread interval instead of going quiet for a whole period, and the latch keeps that retry from re-logging every tick.

Failed rotation triggers rotate() on every subsequent record (major) — fixed

Same latch: writeRecord() now reads sizeLimit > 0 && !rotationFailed && outputStream.written + size + 1 >= sizeLimit. RotaterThread calls rotate() directly, so retries still happen once per interval, and the latch is cleared by the first successful rename.

Compressed-schema backup failure now fails LDAP writes (major) — fixed

Agreed on the split you suggested: the liveFile → .save step is best effort again (logged through a new WARN_COMPRESSEDSCHEMA_CANNOT_SAVE_PREVIOUS_DATA), only the tempFile → liveFile swing stays fatal. One nuance on the description: on POSIX both delete() and renameTo() need write permission on the same directory, so the case where .save cannot be deleted but the rename would have worked is essentially Windows (open handle) or an immutable attribute — the window is narrower than "previously the update still landed on POSIX". The split is right regardless, since a failed backup should never fail an add or modify.

Wrong argument for ERR_PLUGIN_REFERENT_REPLACE_LOGFILE (minor) — fixed

Confirmed, plugin.properties:236 expects a reason in %s. Rather than inventing a reason string in code, processLog() now uses a dedicated entry which names the file and states the consequence:

ERR_PLUGIN_REFERENT_CANNOT_REPLACE_LOGFILE_130=The Referential Integrity plugin update log file %s
 could not be deleted and created again after its records were processed. Those records will be
 processed again the next time the log file is read

The catch (IOException) two lines down keeps using the original message with io.getMessage().

Adjacent infinite loop (pre-existing) — tracked separately

Confirmed, and it is identical in the base commit — while (true) with no break on any path, and renamedPath growing x.2, x.2.3, … until OutOfMemoryError. Since it is neither a CodeQL alert nor touched by this PR, I filed it as #828 with a repro and a suggested fix rather than widening this changeset.

Nits

• Misleading rotate comment — right, TimeThread formats with yyyyMMddHHmmss'Z' (TimeThread.java:111) and renameTo replaces the target on POSIX. The comment is gone and the rotate() javadoc now says so explicitly.
• Same bug in the same file — fixed: a file the retention policy fails to delete no longer bumps totalFilesCleaned, lastCleanCount counts what was actually deleted, and the failure is reported once through a new WARN_LOGGER_ERROR_DELETING_FILE (latched, because the same files come back on every interval).
• Redundant deletes — all four dropped (BackupDirectory, TaskScheduler, ConfigurationHandler, LDIFBackend), and BackupDirectory now creates its directory with Files.createDirectories(). Note for the record that these File.delete() calls were never alerts: the rule only flags mkdir/createNewFile/renameTo here, so the "37 of 40" count is unaffected — it is purely the redundancy you pointed at.
• New failure surface for an unused directory — the whole block in InstallerHelper.writeSetOpenDSJavaHome() is removed rather than made safe, since nothing reads libDir any more.
• Startup behaviour change — added to the PR description as an upgrade note.
• Raw message — moved to LoggerMessages (ERR_LOGGER_ERROR_ROTATING_FILE_34).
• No tests — added, and each one fails against the previous code:

test	against the previous code
SizeLimitInputStreamTestCase.testSkipAccountsForTheBytesActuallySkipped	expected:<3> but was:<16>
SizeLimitInputStreamTestCase.testSkipStopsAtTheEndOfTheParentStream	fails
ASN1InputStreamReaderTestCase.testDecodeSequenceIncompleteReadOverBufferedStream	fails
ASN1InputStreamReaderTestCase.testSkipElementOverBufferedStream	fails
MultifileTextWriterTestCase.testFailedRotationOfTheErrorLogDoesNotRecurse	java.lang.StackOverflowError

The last one wires a TextErrorLogPublisher to the writer under test and makes the rename fail portably by pre-creating the rotated name as a non-empty directory, so it reproduces exactly the path you described. The other two MultifileTextWriterTestCase cases cover the successful rotation and the append-not-truncate behaviour.

Testing

• opendj-core: 676 tests (org.forgerock.opendj.io plus the new com.forgerock.opendj.util case), all passing.
• opendj-server-legacy (-Pprecommit): MultifileTextWriterTestCase, AbstractTextAccessLogPublisherTest, DebugLogPublisherTest, LDIFBackendTestCase, ReferentialIntegrityPluginTestCase, TestBackupAndRestore, BackupManagerTestCase, TaskBackendTestCase, SchemaBackendTestCase, TestImportAndExport — 369 tests, all passing.

org.opends.quicksetup.ConfigurationTest fails in my working copy, but for an unrelated local reason: target/package still held a stale opendj-5.1.2-SNAPSHOT-slim.zip containing an already configured instance (db/userRoot/*.jdb, locks/, config.ldif.startok), so setup exits with 3 and "Server Already Configured" before any of this code runs. It passes from a clean build; the CI Windows jobs are the ones worth watching for the renameTo behaviour.

[maximthomas]

constructWriter() logs two permission warnings of its own (:205, :211), and by the time it
runs, outputStream has already been re-seeded to file.length() — over the size limit, since the
failed rename forces append. When this writer backs the error log, those warnings re-enter
writeRecord() on this thread while rotationFailed is still false, so the size check fires and
rotate() is called again: unbounded recursion until the stack blows, which I reproduced against
this branch. Setting the latch before the re-open closes the window; report keeps the failure
logged exactly once, so report-once, latch-clear-on-success and the lastRotationTime semantics
are all unchanged and the new test still passes.

Suggested change

	final boolean renamed = currentFile.renameTo(newFile);
	// The latch must be set before the writer is re-opened: constructWriter() logs warnings of its
	// own, and when this writer backs the error log they come straight back into writeRecord() on
	// this thread, where they must not trigger another rotation attempt.
	final boolean report = !renamed && !rotationFailed;
	rotationFailed = !renamed;
	try
	{
	// If the file could not be rotated then keep appending to it rather than truncating it.
	constructWriter(currentFile, filePermissions, encoding, append || !renamed,
	bufferSize);
	}
	catch (Exception e)
	{
	logger.traceException(e);
	errorHandler.handleOpenError(currentFile, e);
	}
	if (renamed)
	{
	logger.trace("Log file %s rotated and renamed to %s", currentFile, newFile);
	totalFilesRotated++;
	lastRotationTime = TimeThread.getCalendar();
	}
	else if (report)
	{
	logger.error(ERR_LOGGER_ERROR_ROTATING_FILE, currentFile, newFile);
	}

[vharseko]

Applied as-is in 00fd5fa — confirmed: the permission warnings in constructWriter() fire after the stream has been re-seeded with the over-limit file length and before the latch was set, so the window was real.

Since FilePermission.setPermissions is static and there is no portable way to make it fail, the new testWarningDuringReopenAfterFailedRotationDoesNotRecurse drives the same window through the File itself: setPermissions checks exists() exactly where the permission warnings are logged, after the writer has been re-opened, and the hooked file logs a warning through the very error logger this writer backs. Against d465627 it dies with StackOverflowError, the stack cycling through that hook; with the latch set before the re-open it passes and asserts that the warning lands in the log file, that the record which triggered the rotation is not lost, and that the rotation failure is reported exactly once. MultifileTextWriterTestCase passes as a whole (4 tests, -Pprecommit failsafe run).