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.

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

network-audit intermittently fails on Linux #6189

Closed
mkarolin opened this issue Sep 26, 2019 · 10 comments
Closed

network-audit intermittently fails on Linux #6189

mkarolin opened this issue Sep 26, 2019 · 10 comments
Labels
audit-non-blocking dev-concern priority/P3 The next thing for us to work on. It'll ride the trains. QA/No release-notes/exclude

Comments

@mkarolin
Copy link
Contributor

On CI, only on Linux, occasionally fails with an error similar to:

15:00:47  undefined:17221
15:00:47  {"phase":2,"source":{"id":82,"type":8}]}
15:00:47                                        ^
15:00:47  
15:00:47  SyntaxError: Unexpected token ] in JSON at position 77930745
15:00:47      at JSON.parse ()
15:00:47      at start (/home/ubuntu/jenkins/workspace/ois-rebrand-linux-changelog-6093/lib/start.js:144:23)
15:00:47      at Command.listener (/home/ubuntu/jenkins/workspace/ois-rebrand-linux-changelog-6093/node_modules/commander/index.js:315:8)
15:00:47      at Command.emit (events.js:203:13)
15:00:47      at Command.parseArgs (/home/ubuntu/jenkins/workspace/ois-rebrand-linux-changelog-6093/node_modules/commander/index.js:654:12)
15:00:47      at Command.parse (/home/ubuntu/jenkins/workspace/ois-rebrand-linux-changelog-6093/node_modules/commander/index.js:474:21)
15:00:47      at Object. (/home/ubuntu/jenkins/workspace/ois-rebrand-linux-changelog-6093/scripts/commands.js:155:4)
15:00:47      at Module._compile (internal/modules/cjs/loader.js:868:30)
15:00:47      at Object.Module._extensions..js (internal/modules/cjs/loader.js:879:10)
15:00:47      at Module.load (internal/modules/cjs/loader.js:731:32)
@mkarolin
Copy link
Contributor Author

I believe this started happening with C77

@rebron
Copy link
Collaborator

rebron commented Oct 15, 2019

cc: @diracdeltas Can you take a look?

@diracdeltas diracdeltas self-assigned this Oct 15, 2019
@diracdeltas
Copy link
Member

@jumde does this error look familiar to you? i vaguely remember you may have fixed something like this in the past

@jumde
Copy link
Contributor

jumde commented Oct 15, 2019

This looks like a parsing error, most of the errors that I have looked at involved process being terminated early/not-at-all. I can look at this, not seen this error on mac though.

@rebron rebron added the priority/P3 The next thing for us to work on. It'll ride the trains. label Nov 19, 2019
@bsclifton
Copy link
Member

bsclifton commented Dec 3, 2019

Problem appears to be here:

brave-browser/lib/start.js

Lines 140 to 143 in 9cc247b

if (!jsonContent.endsWith('}]}')) {
const n = jsonContent.lastIndexOf('},')
jsonContent = jsonContent.substring(0, n) + '}]}'
}

(introduced with #2787 to fix a problem on Windows)

Going to look at a better solution to that problem...

@diracdeltas
Copy link
Member

Going to look at a better solution to that problem...

maybe only do the json hack if the platform is windows?

bsclifton added a commit that referenced this issue Dec 3, 2019
bsclifton added a commit that referenced this issue Dec 3, 2019
@bsclifton bsclifton added this to the 1.4.x - Nightly milestone Dec 3, 2019
bsclifton added a commit that referenced this issue Dec 4, 2019
@bsclifton
Copy link
Member

@diracdeltas interestingly enough, it seems the output is always malformed (when testing on Linux). Maybe the way the process gets terminated doesn't let it clean up properly?

@diracdeltas
Copy link
Member

@bsclifton i wonder if that's because it's sending SIGKILL instead of SIGTERM? https://github.com/brave/brave-browser/pull/7182/files#diff-c1a0ca0ae9997d37108b9580a48466bcR98

@bsclifton
Copy link
Member

@diracdeltas yes - I think that's definitely part of the problem. Before that was introduced (with #4036) it was hanging indefinitely. I'm curious if SIGTERM for all cases would work? I suspect there was an issue with it (@jumde would know)

Important to note though: because this is abruptly being terminated, there are logs which haven't been flushed (because the JSON isn't terminated properly). This means the integrity of the check is questionable, since there is currently the band-aid in place which corrects the missing line ending. There could be entries (which might be considered violations) which haven't been written to the log which are dropped when the process is terminated

I'm going to create a new issue for the integrity - maybe you can help me prioritize? 😄

@bsclifton bsclifton removed this from the 1.4.x - Nightly milestone Dec 4, 2019
bsclifton added a commit that referenced this issue Dec 4, 2019
bsclifton added a commit that referenced this issue Dec 9, 2019
bsclifton added a commit that referenced this issue Dec 9, 2019
@bsclifton bsclifton modified the milestone: 1.4.x - Nightly Dec 9, 2019
@bsclifton bsclifton removed their assignment Dec 10, 2019
@bsclifton
Copy link
Member

Closing as a duplicate of #7207

@bbondy bbondy added this to the Closed / Invalid milestone Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-non-blocking dev-concern priority/P3 The next thing for us to work on. It'll ride the trains. QA/No release-notes/exclude
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants