I notice that most (all?) of the actual references to diskstore.txid use txid + 1. Would it make sense to name it nextTxID instead, and initialize it to 1, so it's clear which value should be assigned to new transactions? If the current approach is preferred due to other context, please add a comment to the definition of txid clarifying that it refers to the most recently completed transaction id rather than the next available one (I would have left this note there but github doesn't let me comment on code the PR doesn't modify :-) )

diskstore.txid is indeed used with txid+1 always. Will rename to nextTxID as proposed.

transaction ids seem natural for operations but their role for files is less obvious. Could there be a comment here to describe the intention? E.g. the way it looks to me is something like:

// The checkpoint is assigned the next available transaction id. The first operation
// after a successful checkpoint will be (fileTxID + 1).

Added comment:

	// The checkpoint is assigned the next available transaction id. This
	// guarantees that all existing log entries are 'older' then the checkpoint
	// file and subsequenent operations.  The first operation after a successful
	// checkpoint will be (fileTxID + 1).

If there's an error here, should we delete the temporary file?

Related, it looks like checkpointTmpFile always uses the file name "checkpoint.new". This means that if a checkpoint is incomplete (whether because of an explicit error or just incomplete cleanup), it might obstruct the creation of new ones (this will usually not be the case since you use O_TRUNC etc but it could happen e.g. if the beat is restarted by a different user than previously, or if file permissions are otherwise changed). It would be a bit more robust to include the txid in the temporary file as well, so there will ~never be a conflict.

Restarting as another user that doesn't have access should trigger an error early when opening the store.

Changing permissions life can indeed be a risk, even for this file. I opted to not include the txid in the temporary file, as we continuously will need to try to cleanup if there was some error in the meantime. Messing with permissions on existing files or the directory can lead to other problems as well, so I didn't really account for that. In order to prevent a potential attack on the temporary file I wouldn't use the txid for the temporary file, but really create a full random name.

I will add logic to delete an existing checkpoint.new on init time (this is in openStore which is not shown here), so we can fail with permission errors on Beats start. Plus I will add a os.Remove that tries to delete the checkpoint.new file if it still exists after the renaming. Any concerns with that?

please document how home and active are used by this function

renamed home to homePath and active to checkpointFilePath + add godoc.

// updateActiveMarker overwrites the active.dat file in the home directory with
// the path of the most recent checkpoint file.
// The active file will be written to `<homePath>`/active.dat.
func updateActiveMarker(log *logp.Logger, homePath, checkpointFilePath string) error {

activeLink and tmpLink are both assigned the same value, which is probably not intended? It also doesn't match the filenames listed in the comments below.

This is an error. They should be different.

The comment says this sorts the data files, but I don't see any sorting happen. It looks more like it expects an already-sorted data files list. In fact it looks like diskstore.dataFiles is always required to be sorted by txid, but I'm not sure of that -- can the definition of dataFiles document whether / when it is required to be sorted?

The sort happens in listDataFiles. We keep the sequence ordered by always trying to delete all but the last entry, and appending new entries to the end. Will add a comment to the diskstore type dataFiles field that it requires the list to be sorted by ID via isTxIDLessEqual

It looks like keep will always be length 1 in this function, can we just make it a dataFileInfo and assign it from s.dataFiles[L-1] instead?

It is correct that the length is 1. We advance through the slice by finally assigning (assuming we haven't had an error) s.dataFiles = keep. If len(dataFiles) == 1, then this operation becomes a noop.
if len(dataFiles) > 1, then the active data file is the last one, while the other ones are all considered old. I think I will rather split the slice into two variables: oldDataFiles []dataFileInfo and activeDataFile dataFileInfo. Then it is more clear which is which and we can always attempt to remove all entries in oldDataFiles. This also removes the need to keep the list sorted.

the comment above WriteCheckpoint says active link isn't a symlink, which seems accurate, but the comments / error message in here still say "symlink" -- update?

yeah, it's missing an update.

I'm not sure what "complete is false" is referring to here, is this an outdated comment or is it referring to a value this PR doesn't reference explicitly?

this can be removed. It's an artifact of an older change