Fix BzlMod exception handling

[katre]

Almost all of the skyfunctions defined for bzlmod support have no exception handling, which means that when an exception does occur, Skyframe drops it and bazel crashes in an inconsistent state.

Issue #18567 is an example of this.

I've done a brief survey of the existing skyfunctions under src/main/java/com/google/devtools/build/lib/bazel/bzlmod/

• BazelDepGraphFunction
  • key: BazelDepGraphValue.KEY -> singleton instance
  • calls:
    • BazelLockFileValue.KEY
      • no exceptions handled
    • BazelModuleResolutionValue.KEY
      • no exceptions handled
    • ModuleFileValue.key
      • no exceptions handled
    • ClientEnvironmentFunction.key
      • no exceptions handled
  • throws:
    • BazelDepGraphFunctionException
      • ExternalDepsException
• BazelLockFileFunction
  • key: BazelLockFileValue.KEY -> singleton instance
  • calls:
    • FileValue.key
      • no exceptions handled
  • throws:
    • BazelLockfileFunctionException
      • ExternalDepsException
• BazelModuleInspectorFunction
  • key: BazelModuleInspectorValue.KEY -> singleton instance
  • calls:
    • ModuleFileValue.KEY_FOR_ROOT_MODULE
    • BazelModuleResolutionValue.KEY
  • throws:
    • nothing
• BazelModuleResolutionFunction
  • key: BazelModuleResolutionValue.KEY -> singleton instance
  • calls:
    • ClientEnvironmentFunction.key
      • no exceptions handled
    • ModuleFileValue.KEY_FOR_ROOT_MODULE
      • no exceptions handled
  • throws:
    • BazelModuleResolutionFunctionException
      • ExternalDepsException
• ModuleFileFunction
  • key: ModuleFileValue.Key -> moduleKey, override
  • calls:
    • FileValue.key (twice)
      • no exceptions handled
    • RepositoryDirectoryValue.key
      • no exceptions handled
  • throws:
    • ModuleFileFunctionException
      • ExternalDepsException
• SingleExtensionEvalFunction
  • key: ModuleExtensionId -> bzlFileLabel, extensionName
  • calls:
    • SingleExtensionUsagesValue.key
      • no exceptions handled
    • BzlLoadValue.keyForBzlmod
      • handles BzlLoadFailedException
  • throws:
    • SingleExtensionEvalFunctionException
      • ExternalDepsException
      • IOException
• SingleExtensionUsagesFunction
  • key: ModuleExtensionId
  • calls:
    • BazelDepGraphValue.KEY
  • throws:
    • nothing

Every SkyKey can potentially throw the exceptions listed under its Skyfunction (for example, requesting BazelDepGraphValue.KEY can potentially raise ExternalDepsException.

Therefore, each of these functions needs to have existing env.getValue calls converted to env.getValueOrException calls, and then the exception needs to be handled properly, generally by re-throwing it.

[katre]

@Wyverald Please tell me whether there are more skyfunctions that need to be audited.

[katre]

• BzlmodRepoRuleFunction
  • key: BzlmodRepoRuleValue.Key
  • calls:
  • ModuleFileValue.KEY_FOR_ROOT_MODULE
  • throws:
  • BzlmodRepoRuleFunctionException
    • InvalidRuleException
    • NoSuchPakcageException
    • EvalException
    • ExternalDepsException

[Wyverald]

Therefore, each of these functions needs to have existing env.getValue calls converted to env.getValueOrException calls, and then the exception needs to be handled properly, generally by re-throwing it.

I thought you'd only use env.getValueOrException when you can handle the exception in question? If you can't handle it, the normal procedure is to let it propagate naturally. (See javadoc for env.getValueOrThrow) We could also catch and re-throw but that should only happen if we can add more context (see javadoc for Skyfunction#compute)

What the Bzlmod skyfunctions are doing right now essentially means that they can't tolerate any exceptions. Hence whoever is requesting the Bzlmod skyvalues should be aware that they can throw exceptions of various types.

I'm not sure what's causing CTF to crash, but it seems to me that we need to fix CTF, not the Bzlmod skyfunctions. We could have the Bzlmod skyfunctions wrap the exception with more info, but that's an enhancement, not a bug fix.

[katre]

Okay, based on that I think we need to specifically make the following changes:

1. Handle ExternalDepsException in PrerequisiteProducer
2. Handle ExternalDepsException in ToolchaonResolutionFunction
   1. Toolchain external failures are different than direct dependencies and may need more context, like "why is this even a dependency?"
3. Convert SingleExtensionEvalFunction's IOException to an ExternalDepsException.

We should also audit the other exceptions thrown from BzlmodRepoRuleFunction to ensure they are being handled somewhere.

[katre]

Okay, so I can deal with the error from #18567 if I add checks for ExternalDepsException at TargetPatternUtil.expandTargetPatterns.

However, if I add equivalent checks in SingleToolchainResolutionFunction, it's never called (because the BugReport.logUnexpected call in TargetPatternUtil has caused a crash already).

So, is the right answer to handle the exception in TargetPatternUtil? Is it to remove the BugReport.logUnexpected check? I can't figure out the right level for this.

[katre]

I can't work on this any further, so I am unassigning it and marking it as untriaged so that the bzlmod team can pick it up.

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.