Skip to content

Change dataLogWriter from using line based file length tracking#288

Open
urnotsam wants to merge 1 commit into
devfrom
bytebased-log-writer
Open

Change dataLogWriter from using line based file length tracking#288
urnotsam wants to merge 1 commit into
devfrom
bytebased-log-writer

Conversation

@urnotsam

@urnotsam urnotsam commented Sep 9, 2025 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

User description

…te based

PR Type

Enhancement, Tests

Description

  • Refactor DataLogWriter to use byte-based log rotation

  • Update configuration to specify max log size in bytes

  • Adjust tests to validate byte-based log rotation logic

  • Improve log file handling and test coverage for new logic

Changes walkthrough 📝

Relevant files
Enhancement
Config.ts
Switch log writer config to byte-based limits                       

src/Config.ts

  • Change log writer config from entry-based to byte-based limits
  • Update default config values to reflect byte-based sizing
  • Rename config keys for clarity (e.g., maxReceiptEntries →
    maxReceiptBytes)
    		• +6/-6     
    DataLogWriter.ts
    Refactor DataLogWriter to use byte-based rotation               

    src/Data/DataLogWriter.ts

  • Refactor DataLogWriter to track bytes instead of entries
  • Update rotation logic to use byte thresholds
  • Adjust log file initialization and rotation for byte counting
  • Update log messages and method signatures for new logic
    		• +19/-21 
    Tests
    DataLogWriter.test.ts
    Update DataLogWriter tests for byte-based logic                   

    test/unit/src/Data/DataLogWriter.test.ts

  • Update mocks and test setup for byte-based log limits
  • Revise tests to check byte-based rotation and counters
  • Adjust assertions and test data for new logic
  • Ensure coverage for new byte-based behaviors
    		• +21/-17 
    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @github-actions

    github-actions Bot commented Sep 9, 2025

    Copy link
    Copy Markdown
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🏅 Score: 92
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Log Rotation Edge Case

    The new byte-based rotation logic increments totalNumberOfBytes after writing each entry, but the check for exceeding the max is performed before the write. This could allow a single large entry to push the log file over the intended byte limit. Review if this is acceptable for your use case, or if you want stricter enforcement.

     
      if (this.totalNumberOfBytes >= this.maxNumberBytesPerLog) {
    await this.appendData(`End: Number of bytes: ${this.totalNumberOfBytes}\n`)
    await this.endStream()
    this.totalNumberOfBytes = 0
    await this.rotateLogFile()
    await this.setActiveLog()
  }
  // eslint-disable-next-line security/detect-object-injection
  await this.appendData(this.writeQueue[i])
  this.dataWriteIndex += 1
  this.totalNumberOfBytes += Buffer.byteLength(this.writeQueue[i], 'utf8')
}
    Test Coverage for Edge Cases

    While the tests cover the main byte-based rotation logic, ensure there are tests for edge cases such as writing entries larger than the max byte limit, and for concurrent writes that could trigger rotation mid-batch.

     
      it('should rotate log when bytes exceed max', async () => {
    // Only mock exists for CycleLogWriter's active log
    mockedExistsSync
      .mockReturnValueOnce(true) // CycleLogWriter
      .mockReturnValueOnce(false) // ReceiptLogWriter
      .mockReturnValueOnce(false) // OriginalTxDataLogWriter
      .mockReturnValueOnce(false) // ReceiptOverwriteLogWriter

    // Mock readFile calls only for CycleLogWriter
    mockedReadFile
      .mockResolvedValueOnce('cycle-log1.txt') // CycleLogWriter active log
    mockedStat
      .mockResolvedValueOnce({ size: 1000 } as any) // File size that exceeds max

    await initDataLogWriter()

    expect(CycleLogWriter.totalNumberOfBytes).toBe(0) // Reset after rotation
    expect(CycleLogWriter.logCounter).toBe(2) // Incremented
  })

  it('should handle errors when reading active log file', async () => {
    // Only mock exists for CycleLogWriter's active log
    mockedExistsSync
      .mockReturnValueOnce(true) // CycleLogWriter
      .mockReturnValueOnce(false) // ReceiptLogWriter
      .mockReturnValueOnce(false) // OriginalTxDataLogWriter
      .mockReturnValueOnce(false) // ReceiptOverwriteLogWriter

    mockedReadFile.mockRejectedValueOnce(new Error('Read error'))

    await initDataLogWriter()

    expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('Failed to read active log file:'))
  })
})

describe('writeToLog', () => {
  beforeEach(async () => {
    await initDataLogWriter()
  })

  it('should write data to the log', async () => {
    const testData = 'test log entry\n'

    await CycleLogWriter.writeToLog(testData)

    expect(mockWriteStream.write).toHaveBeenCalledWith(testData)
    expect(CycleLogWriter.totalNumberOfBytes).toBe(15)
  })

  it('should queue multiple writes', async () => {
    const data1 = 'entry1\n'
    const data2 = 'entry2\n'
    const data3 = 'entry3\n'

    await Promise.all([
      CycleLogWriter.writeToLog(data1),
      CycleLogWriter.writeToLog(data2),
      CycleLogWriter.writeToLog(data3),
    ])

    expect(mockWriteStream.write).toHaveBeenCalledWith(data1)
    expect(mockWriteStream.write).toHaveBeenCalledWith(data2)
    expect(mockWriteStream.write).toHaveBeenCalledWith(data3)
    expect(CycleLogWriter.totalNumberOfBytes).toBe(21)
  })

  it('should handle write errors', async () => {
    mockWriteStream.write.mockImplementationOnce(() => {
      throw new Error('Write error')
    })

    await CycleLogWriter.writeToLog('test data\n')

    expect(consoleErrorSpy).toHaveBeenCalledWith('Error while writing data to log file', expect.any(Error))
  })

  it('should handle backpressure', async () => {
    mockWriteStream.write.mockReturnValueOnce(false)
    let drainCallback: (() => void) | undefined
    mockWriteStream.once.mockImplementationOnce((event, callback) => {
      if (event === 'drain') {
        drainCallback = callback as () => void
      }
    })

    const writePromise = CycleLogWriter.writeToLog('test data\n')

    // Simulate drain event
    if (drainCallback) drainCallback()

    await writePromise

    expect(mockWriteStream.once).toHaveBeenCalledWith('drain', expect.any(Function))
  })
})

describe('log rotation', () => {
  beforeEach(async () => {
    await initDataLogWriter()
  })

  it('should rotate log file when counter reaches max/2', async () => {
    CycleLogWriter.logCounter = 4
    mockedReaddir.mockResolvedValue(['old-cycle-log6.txt', 'old-cycle-log7.txt'] as any)

    await CycleLogWriter.rotateLogFile()

    expect(CycleLogWriter.logCounter).toBe(5)
    expect(mockedUnlink).toHaveBeenCalledTimes(2)
    expect(mockedRename).toHaveBeenCalledTimes(5) // Files 6-10
  })

  it('should rotate log file when counter reaches max', async () => {
    CycleLogWriter.logCounter = 9
    mockedReaddir.mockResolvedValue(['old-cycle-log1.txt', 'old-cycle-log2.txt'] as any)

    await CycleLogWriter.rotateLogFile()

    expect(CycleLogWriter.logCounter).toBe(10)
    expect(mockedUnlink).toHaveBeenCalledTimes(2)
    expect(mockedRename).toHaveBeenCalledTimes(5) // Files 1-5
  })

  it('should reset counter when exceeding max', async () => {
    CycleLogWriter.logCounter = 10

    await CycleLogWriter.rotateLogFile()

    expect(CycleLogWriter.logCounter).toBe(1)
  })

  it('should handle rotation with verbose logging', async () => {
    ;(config as any).VERBOSE = true
    mockedReaddir.mockResolvedValue(['old-cycle-log1.txt'] as any)

    await CycleLogWriter.rotateLogFile()

    expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('Rotated log file:'))
    ;(config as any).VERBOSE = false
  })
})

describe('setActiveLog', () => {
  beforeEach(async () => {
    await initDataLogWriter()
  })

  it('should set active log file correctly', async () => {
    await CycleLogWriter.setActiveLog()

    expect(mockedAppendFile).toHaveBeenCalledWith(
      expect.stringContaining(`cycle-log${CycleLogWriter.logCounter}.txt`),
      ''
    )
    expect(mockedWriteFile).toHaveBeenCalledWith(
      expect.stringContaining('active-cycle-log.txt'),
      `cycle-log${CycleLogWriter.logCounter}.txt`
    )
    expect(mockedCreateWriteStream).toHaveBeenCalledWith(
      expect.stringContaining(`cycle-log${CycleLogWriter.logCounter}.txt`),
      { flags: 'a' }
    )
  })
})

describe('deleteOldLogFiles', () => {
  beforeEach(async () => {
    await initDataLogWriter()
  })

  it('should delete old log files with matching prefix', async () => {
    mockedReaddir.mockResolvedValue([
      'old-cycle-log1.txt',
      'old-cycle-log2.txt',
      'cycle-log3.txt',
      'other-file.txt',
    ] as any)

    const result = await CycleLogWriter.deleteOldLogFiles('old')

    expect(result.oldLogFiles).toEqual(['old-cycle-log1.txt', 'old-cycle-log2.txt'])
    expect(result.promises).toHaveLength(2)
  })

  it('should return empty arrays when no matching files', async () => {
    mockedReaddir.mockResolvedValue(['other-file.txt'] as any)

    const result = await CycleLogWriter.deleteOldLogFiles('old')

    expect(result.oldLogFiles).toEqual([])
    expect(result.promises).toHaveLength(0)
  })
})

describe('endStream', () => {
  beforeEach(async () => {
    await initDataLogWriter()
  })

  it('should end stream successfully', async () => {
    CycleLogWriter.totalNumberOfBytes = 50

    await CycleLogWriter.endStream()

    expect(mockWriteStream.end).toHaveBeenCalled()
    expect(consoleLogSpy).toHaveBeenCalledWith('✅ Finished writing 50 bytes.')
  })

  it('should handle errors when ending stream', async () => {
    mockWriteStream.end.mockImplementationOnce(() => {
      throw new Error('End stream error')
    })

    await expect(CycleLogWriter.endStream()).rejects.toThrow('End stream error')

    expect(consoleErrorSpy).toHaveBeenCalledWith('Error while ending stream', expect.any(Error))
  })
})

describe('automatic rotation during writes', () => {
  beforeEach(async () => {
    await initDataLogWriter()
  })

  it('should rotate log when reaching max entries during write', async () => {
    CycleLogWriter.totalNumberOfBytes = 100
    CycleLogWriter.maxNumberBytesPerLog = 100

    await CycleLogWriter.writeToLog('final entry\n')

    expect(mockWriteStream.write).toHaveBeenCalledWith('End: Number of bytes: 100\n')
    expect(mockWriteStream.end).toHaveBeenCalled()
    expect(CycleLogWriter.totalNumberOfBytes).toBe(12) // Reset and new entry ('final entry\n')
    expect(CycleLogWriter.logCounter).toBe(2) // Incremented
  })
})

describe('concurrent write handling', () => {
  beforeEach(async () => {
    await initDataLogWriter()
  })

  it('should handle concurrent writes properly', async () => {
    const writes = Array(10)
      .fill(null)
      .map((_, i) => `entry${i}\n`)

    await Promise.all(writes.map((data) => CycleLogWriter.writeToLog(data)))

    expect(mockWriteStream.write).toHaveBeenCalledTimes(10)
    expect(CycleLogWriter.totalNumberOfBytes).toBe(70)
  })

    @github-actions

    github-actions Bot commented Sep 9, 2025

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @aniketdivekar aniketdivekar aniketdivekar approved these changes

    Assignees

    No one assigned

    Labels

    Review effort 4/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @urnotsam @aniketdivekar