fix(sdk): harden internal thread management in SystemMetrics

[dmitryduev]

Fixes WB-NNNN
Fixes #NNNN

Description

• Saw a few cannot join thread before it is started errors in Sentry after we released 0.13.5 being triggered by an asset.finish() statement in a Colab environment. Attempted many things, but could not repro. To mitigate, added guardrails around thread joining to SystemMetrics-related code.
• Saw another error like the following:

Cmd('git') failed due to: exit code(128)
  cmdline: git rev-parse --show-toplevel
  stderr: 'fatal: unsafe repository ('/home/<user>' is owned by someone else)

in git root probing and added some guardrails there as well.

Testing

Tried to break things with Colab, but no luck, unfortunately :(

Checklist

• Include reference to internal ticket "Fixes WB-NNNN" and/or GitHub issue "Fixes #NNNN" (if applicable)
• Ensure PR title compliance with the conventional commits standards

[codecov]

Codecov Report

Merging #4439 (a0606c2) into main (202de65) will increase coverage by 0.01%.
The diff coverage is 46.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4439      +/-   ##
==========================================
+ Coverage   83.07%   83.09%   +0.01%     
==========================================
  Files         267      267              
  Lines       33739    33750      +11     
==========================================
+ Hits        28030    28044      +14     
+ Misses       5709     5706       -3     

Flag	Coverage Δ
functest	55.83% <46.66%> (+0.03%)	⬆️
unittest	73.43% <46.66%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
wandb/sdk/wandb_settings.py	94.46% <ø> (ø)
wandb/sdk/lib/git.py	77.71% <40.00%> (-1.31%)	⬇️
wandb/sdk/internal/system/assets/interfaces.py	91.95% <50.00%> (-3.23%)	⬇️
wandb/sdk/internal/system/system_monitor.py	90.72% <50.00%> (-1.84%)	⬇️
wandb/integration/tensorboard/monkeypatch.py	91.35% <0.00%> (-2.47%)	⬇️
wandb/sdk/wandb_run.py	90.89% <0.00%> (+0.05%)	⬆️
wandb/sdk/internal/internal_api.py	89.79% <0.00%> (+0.51%)	⬆️
wandb/sdk/lib/printer.py	89.24% <0.00%> (+0.63%)	⬆️
wandb/sdk/lib/sock_client.py	94.17% <0.00%> (+1.05%)	⬆️
... and 1 more

[raubitsj]

approve that it is likely fine, but some small things to consider.

[raubitsj]

not a blocker
we should probably capture some telemetry for this if it is easy..

[dmitryduev]

hmm... what would we do with that information? it usually happens when the home dir of the user running the script is owned by someone else.
it's easy to do, just kind of clumsy: I'd set a flag on the GitRepo object if that happened, then would check for that in Settings as it's one of the places where it's used, and then pick it up in init. Should I still do it?

[raubitsj]

im not so sure about this is_alive - join pairing.
I think the thing that saves you is only if shutdown_event is alway set in this case? is it?

otherwise your monitor loop might still be called on a thread that was just about to be started even thbough you just nulled out the process attribute.

[dmitryduev]

Removed the is_alive check, it's useless and won't fix anything. Replaced with a try/except: if the thread has not been started for some reason (in colab, we would stop the system metrics process with a pause request, so I could imagine something going wrong just before we do that while thing were still being set up), attempting to join it won't fail, but we'd still null the attribute.