Repository navigation
fix(durable): keep polling when the store doesn't answer a claim - #37
Open
juicycleff wants to merge 2 commits into
Open
juicycleff wants to merge 2 commits into
juicycleff wants to merge 2 commits into
Conversation
Run used to stop the worker on the first claim error that wasn't lease contention, and a claim that timed out counted. A stopped Worker can't start again and the engine doesn't build another, so starts and signals kept landing in the store with nothing claiming their tasks. Loomplane hit this when its Postgres was overloaded for a few minutes: one "claim execution task: timeout" and plans sat in planning until the process restarted. A claim the store didn't answer now comes back wrapped in ErrClaimUnknown, and Run retries it. Each poller backs off with full jitter from PollInterval up to 5s, and starts over once a claim gets an answer. The retirement compatibility read before each claim gets the same treatment. Retrying can't run a task twice. Nothing was processed, and a grant that committed while its reply was lost keeps its lease until expiry, when the next claim takes a new epoch. The lost claim still counts in UnknownClaims, so a drain can't report success. Definitive refusals (ErrInvalid, ErrWriterCompatibility) and errors while processing a claimed task still stop Run. Retrying could hide an error that never clears, like a missing table, so the worker reports it. ClaimStatus gains Failures and FailingSince for each task kind, WorkerStatus gains ClaimFailures and ClaimFailingSince, and Engine.Health fails while any kind's claims keep failing. It clears on the next answered claim. Two tests changed with the contract. TestWorkerExecutionTimeoutPollerFailure now checks that an unanswered timeout claim is retried and reported, and that a refused one still stops Run. TestStopWorkersRetainsWorkerErrorAndFinalCleanup uses a refused claim, since it's about keeping a fatal error through shutdown. New tests inject a claim timeout, once rolled back and once committed with the reply lost. The run completes and its activity runs once. Others cover claims that keep failing, unavailable retirement readiness, and engine health going unhealthy and back. GOWORK=off go test ./... passes, as do the durable and engine packages under -race (the new tests 3 times), golangci-lint with 0 issues, vet and the tidy diff. The integration-tagged store/postgres run was noisy on a machine at load average 36 to 53. TestDurableOutboxActivation (namespace coordination busy) and TestDurableRunChainHistoryBoundary/timeout (execution deadline expired) fail the same way on main. TestDurableRunChainHistoryBoundary/failure hit a task write timeout once, on a path this change doesn't touch, then passed twice in a row with the load lower. TestDurableActivityRetryRecovery and TestDurableRuntimeCancelRecovery each failed once on a store timeout or a lost container port, then passed on main and on this branch. The full package run hit go test's 10 minute limit, so the tests after the history boundary one didn't run there.
govulncheck started failing the Security job and both qualification scans after GO-2026-6809 was published: crypto/x509 in go1.26.9 doesn't distrust some compromised intermediate certificates. go1.26.10 fixes it. main went green on 085bd3f before the advisory existed, so it fails the same way now. CI's setup-go, GOTOOLCHAIN and the shared workflow's go-versions, the Makefile qualification targets, the qualification scripts, the toolchain lines in go.mod and qualification/go.mod, and the docs that name the version all move to go1.26.10. The go directives in querypreguard and writerfloor stay at 1.26.9, since they're a floor and don't decide which toolchain builds anything. With go1.26.10, govulncheck -test reports no vulnerabilities in the root, qualification and qualification/writerfloor, go mod tidy -diff is clean in all three, and go build plus the durable runtime and engine tests under -race pass.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Run used to stop the worker on the first claim error that wasn't lease
contention, and a claim that timed out counted. A stopped Worker can't
start again and the engine doesn't build another, so starts and signals
kept landing in the store with nothing claiming their tasks. Loomplane hit
this when its Postgres was overloaded for a few minutes: one "claim
execution task: timeout" and plans sat in planning until the process
restarted.
A claim the store didn't answer now comes back wrapped in ErrClaimUnknown,
and Run retries it. Each poller backs off with full jitter from
PollInterval up to 5s, and starts over once a claim gets an answer. The
retirement compatibility read before each claim gets the same treatment.
Retrying can't run a task twice. Nothing was processed, and a grant that
committed while its reply was lost keeps its lease until expiry, when the
next claim takes a new epoch. The lost claim still counts in
UnknownClaims, so a drain can't report success. Definitive refusals
(ErrInvalid, ErrWriterCompatibility) and errors while processing a claimed
task still stop Run.
Retrying could hide an error that never clears, like a missing table, so
the worker reports it. ClaimStatus gains Failures and FailingSince for each
task kind, WorkerStatus gains ClaimFailures and ClaimFailingSince, and
Engine.Health fails while any kind's claims keep failing. It clears on the
next answered claim.
Two tests changed with the contract. TestWorkerExecutionTimeoutPollerFailure
now checks that an unanswered timeout claim is retried and reported, and
that a refused one still stops Run. TestStopWorkersRetainsWorkerErrorAndFinalCleanup
uses a refused claim, since it's about keeping a fatal error through
shutdown.
New tests inject a claim timeout, once rolled back and once committed with
the reply lost. The run completes and its activity runs once. Others cover
claims that keep failing, unavailable retirement readiness, and engine
health going unhealthy and back. GOWORK=off go test ./... passes, as do
the durable and engine packages under -race (the new tests 3 times),
golangci-lint with 0 issues, vet and the tidy diff.
The integration-tagged store/postgres run was noisy on a machine at load
average 36 to 53. TestDurableOutboxActivation (namespace coordination
busy) and TestDurableRunChainHistoryBoundary/timeout (execution deadline
expired) fail the same way on main. TestDurableRunChainHistoryBoundary/failure
hit a task write timeout once, on a path this change doesn't touch, then
passed twice in a row with the load lower. TestDurableActivityRetryRecovery
and TestDurableRuntimeCancelRecovery each failed once on a store timeout or
a lost container port, then passed on main and on this branch. The full
package run hit go test's 10 minute limit, so the tests after the history
boundary one didn't run there.
The second commit moves the toolchain to go1.26.10. GO-2026-6809 (crypto/x509) was published after main's last green run, and govulncheck failed the Security job and both qualification scans here because of it. main fails the same way now, so you'll want this in either way.