[#343] Tamper-evident CSV audit: start the rotation checks once a resumed file is verified

[vharseko]

Fixes #343.

Cause

SecureCsvWriter built its RotatableWriter before it read the keys and verified the file it resumes, and RotatableWriter schedules the rotation and retention checks in its constructor. A resumed file due for a rotation (rotationInterval, a rotation time, a lowered maxFileSize) was rotated at the first check when the construction outlasted the check interval. The file and its .keystore were renamed, and postRotationAction wrote new keys while the constructor still read or verified them, so the topic failed to start:

• with "Final HMAC key doesn't match expected value" when the rotation runs during the verification, as traced in Tamper-evident CSV audit: the rotator can rotate a resumed file while the writer still verifies it #343;
• with "Expecting to find an initial key into the keystore" when it runs before the keystore is loaded. This is what the new test gets at master. There, postRotationAction also fails with a NullPointerException and leaves the time of the last rotation as it was, so the rotator renames the new live file again at each check.

Fix

• RotatableWriter gets a public constructor with a startChecks flag, and startRotationAndRetentionChecks(). The existing constructors start the checks as before. The check interval is still validated in the constructor, so an invalid interval fails as before.
• SecureCsvWriter constructs it without the checks, and starts them once its constructor has succeeded: after the keys are read and the file is verified. The file is still opened first, so a file that cannot be opened still fails only its topic with an IOException.

This also closes two smaller windows. The checks used to start before registerRotationHooks(), so a check could run with the no-op hooks. A writer that fails to construct no longer leaves a running rotator thread behind; the file it leaves open is #340.

I tried a simpler fix first and dropped it: opening the file only after the verification. Then the keystore of a new file is stored before the file is opened. A log directory that cannot be created then fails the whole startup with a RuntimeException, instead of failing only the topic with an IOException as at master. CsvAuditEventHandlerTest.canConfigureCsvHandlerFromJsonAndRegisterWithAuditService fails on it.

Tests

• SecureCsvWriterTest.shouldRotateResumedFileOnlyOnceItIsVerified: a written file, last modified 2 hours ago, is resumed with a 1 hour rotationInterval and a 10 ms check interval. Each read of the handler keystore takes 100 ms, so the construction outlasts many check intervals. The construction succeeds, then the file is rotated, and the archive passes verification with its own keystore. The test fails with the SecureCsvWriter of master, and when the writer starts the checks in RotatableWriter's constructor.
• RotatableWriterTest.testEvaluatesPoliciesPeriodicallyOnlyOnceTheChecksAreStarted: a file due for a rotation is not rotated during 50 check intervals before startRotationAndRetentionChecks(), and is rotated after it. The test fails when the flag is ignored.
• audit/core, audit/json and audit/handler-csv pass locally. Both new tests passed 3 more runs.

Other open PRs

• This PR merges with [#340] Tamper-evident CSV audit: close the file of a writer that fails to resume it #341 and with [#339] Resume a tamper-evident CSV file with no event since the last rotation #342 without conflicts, and audit/handler-csv passes on each merge.
• Note for [#340] Tamper-evident CSV audit: close the file of a writer that fails to resume it #341: its writer tests check that the rotator thread terminates after a failed construction. With this PR, the rotator has not started yet at that point. So those tests pass whether or not the file is closed, and no test covers the close any more.

[vharseko]

Closing in favour of #355, which fixes #343 too.

Both PRs make the first rotation check run only once the resumed file is verified. #355 does it in SecureCsvWriter alone, by constructing the writer last. This PR does it by adding a constructor flag and startRotationAndRetentionChecks() to the public API of RotatableWriter in audit/core.

#355 is preferred because:

• it leaves the API of audit/core as it is;
• a failure while the keys are read or the file is verified comes before the writer opens the file and before the rotator starts, so nothing is left open. With this PR the file is still open at that point: the close that [#340] Tamper-evident CSV audit: close the file of a writer that fails to resume it #341 adds is still needed, and the tests of [#340] Tamper-evident CSV audit: close the file of a writer that fails to resume it #341 no longer cover it, as the description above notes.

The other window this PR closed, a check that could run before registerRotationHooks(), is listed under "Not in this PR" in #355.