## What changed?
- Applied the server-owned subset of testifylint auto-fixes.
- Exact commands that were run:
```sh
.bin/golangci-lint-v2.9.0 run --allow-parallel-runners --concurrency 4 --fix --enable-only testifylint --build-tags disable_grpc_modules,test_dep --timeout 20m --config=.github/.golangci.yml
make goimports
git diff --check
```
- No manual or AI changes were made; unless commented as such.
- Some fixes caused lint errors; those were reverted again.
- Changes were all reviewed by me.
Remove usage of github.com/pkg/errors and replace with standard library
errors and fmt.Errorf wrapping.
Changes:
- common/finalizer/finalizer.go
- service/worker/migration/activities.go
- remove pkg/errors direct dependency via go mod tidy
Addresses part of #9244
## What changed?
- Replaced `github.com/pkg/errors` with Go standard library `errors` in
`common/finalizer/finalizer.go` (only uses `errors.New`)
- Replaced `errors.WithMessage` with `fmt.Errorf` and `%w` in
`service/worker/migration/activities.go`
- Dropped direct `pkg/errors` dependency from `go.mod`; it remains as an
indirect dependency from `uber-go/tally`
## Why?
`github.com/pkg/errors` is deprecated and unmaintained. The standard
library `errors` and `fmt.Errorf` with `%w` cover the same use cases and
support `errors.Is` and `errors.As` for error unwrapping.
## How did you test it?
- [x] built
- [x] run locally and tested manually
- [x] covered by existing tests
- [ ] added new unit test(s)
- [ ] added new functional test(s)
## Potential risks
None. The changes are localized and behavior is unchanged; tests for the
modified packages pass.
pleas commti and push it
There’s nothing new to commit: the working tree has no staged or
modified files.
The pkg/errors changes are already committed and pushed on
remove-pkg-errors (commit 52dbb1dbf).
The only untracked items are test artifacts under
common/persistence/tests/test_0315064513_*, which look like temporary
files and shouldn’t be committed.
---------
Co-authored-by: alex.stanfield <13949480+chaptersix@users.noreply.github.com>
## What changed?
This updates the server code to use the shorthand log tag constructors
introduced in #9174.
As part of this it _does_ make a breaking change to the `Bool`
constructor: it now takes in the key as a string to be consistent.
The only inconsistent one is now `Error`, but that's used so heavily
that changing it is likely not worth the time.
## Why?
Consistency!
## How did you test it?
Existing tests
## Potential risks
The only risk is that I _have_ introduced a breaking change to the Bool
constructor. I'm happy to undo that if my reviewers desire: my goal is
minimal breaking changes.
I'd prefer none, but I made this change to stir up discussion
## What changed?
Reduce info logging of Finalizer component.
## Why?
Not that useful (anymore) actually; replaced it with a metric to still
have some observability.
## How did you test it?
- [ ] built
- [ ] run locally and tested manually
- [x] covered by existing tests
- [ ] added new unit test(s)
- [ ] added new functional test(s)
## What changed?
<!-- Describe what has changed in this PR -->
Remove license header from every file. Because it is really hard to
follow in this PR here is the summary:
1. License header is removed from all `.go` and `.proto` files
:fireworks::fireworks:🎆.
2. `LICENSE` file in the root directory has only Temporal and Uber
copyrights.
3. 5 other `LICENSE` files added to the packages which have copyrights
different from Temporal and Uber: Datadog, Xargin, "Mat Ryer, Tyler
Bunnell and contributors".
4. `license_file` flag is removed from all code generation tools.
5. `copyright_file` flag is removed from `go:generate mockgen`
directive.
6. All copyright related targets are removed from `Makefile`.
7. Updated Temporal copyright year to 2025 everywhere.
## Why?
<!-- Tell your future self why have you made these changes -->
I double checked with legal department that it is not needed to have
license header in every file. One file per repo is enough. I put all
copyrights to the root `LICENSE` file and removed header from all other
files. Also updated tools and `Makefile`.
## What changed?
<!-- Describe what has changed in this PR -->
Moved all packages inside `/internal` to `/common`.
## Why?
<!-- Tell your future self why have you made these changes -->
While the `internal` package provides the benefit of preventing any
external party from depending on them, we find that in practice this has
little benefit.
Our compatibility guarantees do not coverage any APIs in `common`
anyway.
Furthermore, splitting commonly-used packages into two places hinders
discoverability. And adds (albeit minor) mental friction.
## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
Compiler.
## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
External users could start relying on these packages. But that's their
risk assessment to make. There are no guarantees (just like for any
other package in this repo!) outside of the external APIs.
## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->
## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
## What changed?
<!-- Describe what has changed in this PR -->
Make `Finalizer` release memory earlier.
## Why?
<!-- Tell your future self why have you made these changes -->
See PR comment to explain the background.
## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
Existing tests still pass.
## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
Since `callbacks` is never read/written again after clearing it, it
should be safe.
## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->
## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
## What changed?
Make import formatting a part of the build, and test validity with
"ensure-no-changes" target
## Why?
So we never discuss import order again (hopefully)
## How did you test it?
its a build change, so the build will test it!
## Potential risks
As long as build works, the only thing I risk is the ire of coworker who
want imports to be different ;)
## Documentation
N/A
## Is hotfix candidate?
No
## What changed?
<!-- Describe what has changed in this PR -->
(1) Create pool after validation.
(2) Tracking metrics.
## Why?
<!-- Tell your future self why have you made these changes -->
(1) Use fewer resources in case Finalizer is a noop.
(2) Improve observability.
## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
Extended existing tests.
## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->
## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
## What changed?
<!-- Describe what has changed in this PR -->
Clear all workflow contexts, found in the cache, that belong to a
History Shard that is being closed.
## Why?
<!-- Tell your future self why have you made these changes -->
Right now, when a History Shard is closed, any non-durable Update that
is still in-flight will be "lost", meaning that there is no way to
access it, cancel it or complete it anymore. The client that issued the
Update is not informed about this either (neither is the Frontend).
A better experience is to forcibly cancel any in-flight Updates
belonging to the History Shard that is closing. That way, the Frontend
can re-issue the Update request (which will land on a new History
Shard). The client won't even know that the History Shard was closed.
However, if the retry happens towards the end of the Update timeout, the
client's request might still time out after all.
## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
Added unit tests and functional test.
Before going live, this will have to be validated by thorough end-to-end
testing.
## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
### Code Complexity
Since the workflow cache already has all the workflow contexts, I had
considered adding a by-shardID index a method to iterate over them
there. However, its cache implementation is based on `cache.Cache` (ie a
generic cache) it would have been awkward to add it there. Therefore, I
decided to track the workflow contexts in a separate place, the
Finalizer, which adds to the complexity.
### Memory Leak
There is no way to list the workflow contexts that belong to a History
Shard. Therefore, a new "Finalizer" was introduced that maintains a list
of cleanup callbacks: one callback per workflow context, grouped by
History Shard.
But since the workflow cache will eventually evict workflow contexts
that are no longer needed, if the Finalizer holds on to these cleanup
callbacks past the lifetime of the workflow context, the GC won't be
able to free them from memory. Therefore, a lifecycle hook was
introduced that removes any expired workflow contexts from the
Finalizer.
### Delayed History Shard Closure
The step to close the History Shard should not lock up if some/any of
the workflow contexts cannot be cleared. To clear them, the lock for the
workflow needs to be obtained. To prevent this from blocking
indefinitely, a timeout was introduced. It's better to make sure the
History Shard closes eventually than to clear all workflow contexts.
PS: Logging has been added to be able to monitor the timeouts and
completion rate of the Finalizer.
PPS: If any production issues should occur, the Finalizer's cleanup step
can be disabled entirely by setting the dynamic config for the timeout
to zero. However, the registering/de-registering of callbacks cannot be
disabled.
## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->
## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
