Skip to content

[Code Quality] NativeMethods.GetFileIdentity — two empty catch blocks with stale 'Fallback or log failure here if necessary' TODO comments #1015

@Christophe-Rogiers

Description

@Christophe-Rogiers

Severity: Info

File: src/Servy.Core/Native/NativeMethods.cs lines 779–811

Finding

public static FileIdentity GetFileIdentity(FileStream fs)
{
    var identity = new FileIdentity();
    try
    {
        if (GetFileInformationByHandle(fs.SafeFileHandle.DangerousGetHandle(), out var info))
        {
            identity.VolumeSerialNumber = info.VolumeSerialNumber;
            identity.FileIndex = ((ulong)info.FileIndexHigh << 32) | info.FileIndexLow;
            identity.IsValidHandleInfo = true;
        }
    }
    catch
    {
        // Fallback or log failure here if necessary
    }

    try
    {
        long origPos = fs.Position;
        fs.Seek(0, SeekOrigin.Begin);
        byte[] buffer = new byte[64];
        int read = fs.Read(buffer, 0, buffer.Length);
        identity.PrefixHash = Convert.ToBase64String(buffer, 0, read);
        fs.Seek(origPos, SeekOrigin.Begin);
    }
    catch
    {
        // Fallback or log failure here if necessary
    }

    return identity;
}

Two empty catch blocks, both annotated with the identical placeholder comment // Fallback or log failure here if necessary. This is a TODO that has never been actioned. GetFileIdentity is used by LogTailer to detect log rotation; if the file-handle probe or the prefix-hash probe silently fails (handle was closed, file was truncated, file was locked), the returned identity is a default-initialized struct — IsValidHandleInfo false, PrefixHash null — and the rotation-detection caller will treat it as 'no identity available' without ever knowing why. That is the textbook 3am operator finding: a tail log that mysteriously stops following after a rotation, with no log line indicating which probe failed.

GetFileInformationByHandle is a P/Invoke into kernel32; the only realistic exceptions are wrapper-induced (ObjectDisposedException from fs.SafeFileHandle, AccessViolationException). Both deserve a log line.

Suggested fix

Replace both empty catches with a debug-level log, and at minimum capture the exception type:

catch (Exception ex) when (LogAndContinue(ex, "GetFileInformationByHandle probe failed"))
{
    // already logged via filter
}

…or inject IServyLogger and call _logger.Debug($"GetFileIdentity prefix-hash probe failed: {ex.GetType().Name} {ex.Message}") so the call sites at least surface a single diagnostic line per probe failure.

The TODO comment can also be removed once the decision is made — leaving placeholder TODOs in production code is itself a maintenance smell.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions