Skip to content

Conversation

addaleax
Copy link
Collaborator

@addaleax addaleax commented Oct 6, 2025

@addaleax addaleax requested a review from a team as a code owner October 6, 2025 15:06
@Copilot Copilot AI review requested due to automatic review settings October 6, 2025 15:06
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes log flushing on exit by making the logging flush operations asynchronous. The changes ensure that log files are properly written before the application terminates, preventing potential data loss.

  • Changed flush() and detachLogger() methods to return Promise<void> instead of void
  • Added await calls for proper asynchronous handling of log flushing operations
  • Introduced error handling in the start() method to ensure cleanup occurs even when exceptions are thrown

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
packages/logging/src/types.ts Updated interface to make flush and detachLogger methods async
packages/logging/src/logging-and-telemetry.ts Implemented async flush behavior with proper log.flush() awaiting
packages/e2e-tests/test/e2e.spec.ts Added tests to verify log flushing works on normal exit and exception scenarios
packages/cli-repl/src/cli-repl.ts Updated calls to use async flush methods and added error handling wrapper

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +2209 to +2211
const log = await readReplLogFile(
shell.output.replace(/Error: uh oh/g, '').trim()
);
Copy link
Preview

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message filtering using regex replacement is fragile and could inadvertently remove legitimate log content. Consider a more robust approach to extract the log path from shell output.

Suggested change
const log = await readReplLogFile(
shell.output.replace(/Error: uh oh/g, '').trim()
);
// Extract the log path from the shell output by finding the line that looks like a log file path
const logPathLine = shell.output
.split('\n')
.map(line => line.trim())
.find(line => line.length > 0 && /\.log$/.test(line));
expect(logPathLine, 'Could not find log path in shell output').to.be.a('string');
const log = await readReplLogFile(logPathLine);

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I had similar thoughts here as well, although I was ultimately feeling like this is just fine for a test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant