Handle nil HistoryURI during visibility archival #4880
Conversation
| historyURI, err := carchiver.NewURI("test:///history/archival") | ||
| var historyURI carchiver.URI | ||
| var err error | ||
| if !c.NilHistoryUri { |
There was a problem hiding this comment.
This is a bit crude because if we just did c.HistoryURI - we wouldn't know whether to use default or if it's unset
| @@ -260,7 +265,7 @@ func (a *archiver) archiveVisibility(ctx context.Context, request *Request, logg | |||
| HistoryLength: request.HistoryLength, | |||
| Memo: request.Memo, | |||
| SearchAttributes: searchAttributes, | |||
| HistoryArchivalUri: request.HistoryURI.String(), | |||
There was a problem hiding this comment.
This value is not initialized here when history archival is disabled
There was a problem hiding this comment.
Does the visibility archiver need to have the value for the historyuri?
An alternative fix would be to always fill out both URIs in the request generation method so that it never reaches here nil. Then it could presumably have the correct value instead of an empty string.
There was a problem hiding this comment.
fill out both URIs
Fill them out how?
The branching for filling this out is currently set on history archival being enabled.
URI values also cannot be empty (parsing will fail).
How can we fill out the value if it were to not be available?
My understanding of this record is to record the current values for the fields and the error is that this crashes when history URI is nil (but should be empty)
|
Thanks for the fix! I'm sorry that this caused an incident for y'all, but we appreciate you fixing it to prevent it from happening to others. One thing I'd like to point out:
This won't happen because we don't use archival in Cloud. |
|
Thank you for reviewing! I realized after opening the PR the feature isn't offered in cloud but hopefully this still helps the community (or Temporal should this feature be used in cloud in the future) |
|
@MichaelSnowden Just to make sure, do we need to do anything with this PR for merging or do we just leave it in your hands? |
Handle cases of
nilHistoryArchivalURIwhen performing visibility archivalSee #4879 for details
This PR is a suggested fix for #4879
We are not fully familiar with the codebase so it's ok to close and make a better fix (or point us in the right direction; we are happy to make the patch)
Added a test that sets the URI to nil during visibility archival. It panics before the change and passes after
The risk is lower than before this fix. This is not, however, a comprehensive check on when similar panics can occur
This error has resulted in a P0 incident for us but it is also somewhat unlikely given it has not happened in such a long time. This may bring down cloud clusters as well in theory.