Rework the Lock API so that locks can be auto-renewed.#482
Conversation
| case <-ctx.Done(): | ||
| return | ||
| case <-timer.C: | ||
| callCtx, cancel := context.WithTimeout(ctx, interval) |
There was a problem hiding this comment.
🟡 🤖 If Redis is unreachable, the workflow can keep running past lease expiry. A retry's callCtx allows a full extra interval, and the elapsed-TTL check runs only after the attempt returns. So the lease context may not cancel until ~8s after the key expired, when another replica can already hold the lock. The old design cancelled the workflow 2s before the TTL.
Fix: declare the lease lost no later than lastRenewed.Add(ttl), e.g. cap the retry callCtx at time.Until(lastRenewed.Add(ttl)).
There was a problem hiding this comment.
+1. Also provided more context on the comment below, maybe also with some buffer time.
There was a problem hiding this comment.
Done.
| ) | ||
|
|
||
| // maxRPCDeadline is the max deadline for all RPC methods exposed by this server. | ||
| const maxRPCDeadline = 10 * time.Minute |
There was a problem hiding this comment.
🟢 🤖 The PR description says the backstop is 5 minutes; this sets 10. Worth aligning one or the other.
There was a problem hiding this comment.
Done.
Jeffrey Ying (Jefftree)
left a comment
Jeffrey Ying (Jefftree)
left a comment
There was a problem hiding this comment.
👋 Ben asked me to take a look at this, I work on the leader election library in k8s core. Providing a couple suggestions based on my experience with k8s
| slog.WarnContext(ctx, "lock renewal found lease no longer owned", "key", key) | ||
| return | ||
|
|
||
| case time.Since(lastRenewed) >= ttl: |
There was a problem hiding this comment.
k8s has precedence in that we explicitly accept that lease is lost well ahead of the ttl duration if renew is deemed unhealthy. Inflight operations generally take time stop (eg: I see that there was an explicit stop at 28s instead of stopping at the full 30s). Getting here means the renew failed multiple times already and another instance is taking over. I understand substate has a different set of requirements so it doesn't need to exactly follow k8s shape, just providing it as reference. K8s uses a separate Deadline parameter kept below the lease duration (defaults to 2/3 of actual deadline) to account for things like clock skew so the holder gives up while there's still time left on a lease lock. This gives the renewal 2 tries before accepting that the renewal isn't working.
Suggestion: keep a padding here
There was a problem hiding this comment.
Thanks for the context! Done.
| case <-ctx.Done(): | ||
| return | ||
| case <-timer.C: | ||
| callCtx, cancel := context.WithTimeout(ctx, interval) |
There was a problem hiding this comment.
+1. Also provided more context on the comment below, maybe also with some buffer time.
| value1 := "token-1" | ||
| value2 := "token-2" | ||
| ttl := 10 * time.Second | ||
| ttl := 300 * time.Millisecond // renewed every ~100ms; overriding s.lockTTL lets the test avoid waiting out the real lockTTL. |
There was a problem hiding this comment.
nit: using a real clock has been sometimes brittle with CI and race conditions. Would recommend eventually transitioning to inject a fake clock that you can deterministically control to test for edge cases.
There was a problem hiding this comment.
we might be able to use https://go.dev/blog/testing-time
There was a problem hiding this comment.
Good idea. Done.
| } | ||
|
|
||
| // 2. Fast-forward time past TTL | ||
| mr.FastForward(6 * time.Second) |
There was a problem hiding this comment.
nit: I'd imagine you want to keep testing the path of actual TTL expiry (crash/give up) resulting in allowing another client to grab the lock? With renewals, maybe that involves two clients now.
There was a problem hiding this comment.
Done.
f55bfbe to
927aa64
Compare
Instead of requiring callers to specify a fixed deadline for the lock, let the store return a `Lock` object that performs auto-renewal in background. This fixes an issue when a workflow takes longer than the hard-coded TTL, most commonly when atelet takes long times to initialize an actor (e.g. pulling / untaring large OCI images). Leases are taken for 30 seconds, but renewed every 10 seconds (TTL/3). Removing that implicit TTL-based limit means a caller that sets no deadline of its own (e.g. kubectl-ate) could otherwise hang a Resume/Suspend/Pause indefinitely. MaxDeadlineUnaryInterceptor adds a 5-minute ceiling on every RPC as a hang-prevention backstop.
927aa64 to
a19123b
Compare
|
LGTM! |
f38f94b
into
agent-substrate:main
3 participants
Instead of requiring callers to specify a fixed deadline for the lock, let the store return a
Lockobject that performs auto-renewal in background.This fixes an issue when a workflow takes longer than the hard-coded TTL, most commonly when atelet takes long times to initialize an actor (e.g. pulling / untaring large OCI images).
Leases are taken for 30 seconds, but renewed every 10 seconds (TTL/3).
Removing that implicit TTL-based limit means a caller that sets no deadline of its own (e.g. kubectl-ate) could otherwise hang a Resume/Suspend/Pause indefinitely. MaxDeadlineUnaryInterceptor adds a 10-minute ceiling on every RPC as a hang-prevention backstop.