Skip to content
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

Add a seperate logger for worker processes #87

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

Add a seperate logger for worker processes #87

wants to merge 1 commit into from

Conversation

tanuj-shardeum
Copy link
Contributor

No description provided.

@tanuj-shardeum tanuj-shardeum self-assigned this Sep 24, 2024
@github-actions GitHub Actions
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Logger Initialization
The workerProcessLogger is initialized in initLogger function but there is no check to ensure that the logger is correctly configured or handling of potential errors during its initialization.

Logging Level
The logging level and the type of messages logged (info, debug, error) should be reviewed to ensure they are appropriate for their use cases to avoid verbose logging in production.

Error Handling
There are multiple instances where errors are logged but not handled. For example, when no worker is available, the error is logged but the process continues without handling the absence of a worker.

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 24, 2024
View reviewed changes
src/primary-process/index.ts
// Set interval to check receipt count every 15 seconds
setInterval(async () => {
for (const [, worker] of newWorkers) {
worker.kill()
}
if (receiptLoadTraker < config.receiptLoadTrakerLimit) {
if (config.workerProcessesDebugLog)
console.log(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)
Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
Loading
.

Copilot Autofix AI about 2 months ago

To fix the log injection issue, we need to sanitize the user-provided values before logging them. Specifically, we should remove any newline characters from the config.receiptLoadTrakerLimit value before it is included in the log message. This can be done using String.prototype.replace to ensure no line endings are present in the user input.

Suggested changeset 1
src/primary-process/index.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/primary-process/index.ts b/src/primary-process/index.ts
--- a/src/primary-process/index.ts
+++ b/src/primary-process/index.ts
@@ -50,4 +50,6 @@
     if (receiptLoadTraker < config.receiptLoadTrakerLimit) {
-      if (config.workerProcessesDebugLog)
-        Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)
+      if (config.workerProcessesDebugLog) {
+        const sanitizedLimit = String(config.receiptLoadTrakerLimit).replace(/\n|\r/g, "");
+        Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${sanitizedLimit}`)
+      }
       // Kill the extra workers from the end of the array
EOF
@@ -50,4 +50,6 @@
if (receiptLoadTraker < config.receiptLoadTrakerLimit) {
if (config.workerProcessesDebugLog)
Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${config.receiptLoadTrakerLimit}`)
if (config.workerProcessesDebugLog) {
const sanitizedLimit = String(config.receiptLoadTrakerLimit).replace(/\n|\r/g, "");
Logger.workerProcessLogger.debug(`Receipt load is below the limit: ${receiptLoadTraker}/${sanitizedLimit}`)
}
// Kill the extra workers from the end of the array
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
jairajdev
jairajdev reviewed Sep 26, 2024
View reviewed changes
archiver-log.json
@@ -1,7 +1,7 @@
{
"saveConsoleOutput": true,
"dir": "archiver-logs",
"files": { "main": "", "fatal": "", "net": "" },
"files": { "main": "", "fatal": "", "net": "", "workerProcess": "" },
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we give the name as workerHandler instead? I think since it's more about the debug logs by the main process handling the worker processes.

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

@jairajdev jairajdev jairajdev left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@tanuj-shardeum tanuj-shardeum

Labels
Review effort [1-5]: 3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tanuj-shardeum @jairajdev