Skip to content

Change dataLogWriter from using line based file length tracking #288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Change dataLogWriter from using line based file length tracking #288

wants to merge 1 commit into from

Conversation

@urnotsam
Copy link
Contributor

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

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
    Copy link
    Contributor

    github-actions bot commented Sep 9, 2025

    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
    Copy link
    Contributor

    github-actions bot commented Sep 9, 2025

    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.

    3 participants

    @urnotsam @aniketdivekar