[BUG] Calls to `client.NewWorker` cannot be cancelled, causing application to hang if server is unavailable

[solidDoWant]

Describe the issue
Calls to client.NewWorker do not support cancellation (i.e. a context), so the application/goroutine will be blocked while this function is ran. When called in the main goroutine, this can cause the entire application to become unresponsive. Here's a couple of cases where the function call will either take a long time, or retry indefinitely:

• Server is completely unavailable
• Client is configured to use TLS but the server has it disabled

Environment

• SDK: Go v0.79.43
• Engine: self-hosted v0.79.43

Expected behavior
Callers should be able to cancel this long-running function call.

Code to Reproduce, Logs, or Screenshots

package main

import (
	"context"
	"fmt"
	"log"
	"os"
	"os/signal"
	"syscall"

	hatchet "github.com/hatchet-dev/hatchet/sdks/go"
)

func main() {
	ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
	defer cancel()

	if err := run(ctx); err != nil {
		fmt.Fprintln(os.Stderr, err)
		os.Exit(1)
	}
}

func run(ctx context.Context) error {
	client, err := hatchet.NewClient()
	if err != nil {
		return fmt.Errorf("create Hatchet client: %w", err)
	}

	// This cannot be cancelled even when OS signals are received. Wrapping this in a goroutine isn't
	// a great solution because the goroutine will leak and continue attempting connections until
	// the process is killed.
	worker, err := client.NewWorker("worker")
	if err != nil {
		return fmt.Errorf("create Hatchet worker: %w", err)
	}

	log.Println("connected to Hatchet")
	return nil
}

Run the above with the HATCHET_CLIENT_TOKEN var set but a TLS/non-TLS server/client mismatch, or with the server entirely unavailable, and try to exit the program without forcibly killing the process.

Additional context
N/A

[BloggerBust]

I investigated this and was able to reproduce it locally. Here is what I observed:

client.NewWorker(...) does block for a long time when the server is unavailable, and cancellation from a signal derived caller context does not unblock it. In my repro, the blocking was much longer with gRPC retry enabled, and much shorter with retry disabled, but the startup path still does not give the caller any way to cancel it.

Looking through the current code path, NewWorker(...) performs startup RPCs during worker construction and registration. The first blocking point is the engine capability check via GetVersion(...), and if workflows are being registered there is a second blocking point in workflow registration via PutWorkflowV1(...). The startup path currently does not accept or propagate a caller context, and those RPCs end up running through context.Background() based calls.

From an API design point of view, if worker construction is going to perform network I/O, the ideal function signature would accept a context.Context. For the current SDK though, the least disruptive fix seems like an additive context aware variant, something along the lines of:

NewWorkerContext(ctx context.Context, name string, options ...WorkerOption)

with the existing NewWorker(...) delegating to context.Background() for backward compatibility.

If that sounds reasonable, I would be happy to contribute a PR.

[solidDoWant]

Sounds great to me! Thanks for looking into this.

Another possible route could be to add a WorkerOption for this and store the context in the workerConfig when constructing the worker. This would avoid increasing the API surface area, though I know storing a context in a struct is a Go anti-pattern.

[BloggerBust]

I think there are probably two separate questions here:

1. how the caller cancels or times out NewWorker(...)
2. what the default startup behavior should be when the server is unavailable

For (1), I still think passing a caller context.Context through the startup function signature is the right approach. The immediate problem is that the relevant startup RPCs currently end up using context.Background(), so the caller has no way to cancel or bound the call.

For (2), I do think there may be value in making startup behavior configurable in the future. For example, some users may want a worker to keep retrying across server restarts (possibly for convenience during maintenance windows), while others may want startup to fail fast. That feels like a different scope from how cancellation should be expressed, though.

So my current thinking is:

• use context.Context for caller controlled cancellation and timeout
• treat any broader startup retry or persistence policy as a separate configuration question

I am curious what the maintainers think.

[gregfurman]

Thanks for opening this and the discussions here 🙂

This issue seems related to #2765 which should be fixed by #3127

Once the above PR is in, I'll bump this thread so you can re-test?

[Akhilesh29]

after running locally, tbh the root cause is probably two separate gaps that together make cancellation completely cooked:

1. NewWorker doesn't accept a context.Context at all, so there's literally no cancellation signal to pass down in the first place.
2. even if context was threaded through, the internal retry loop is probably just vibing with bare time.Sleep and never checks ctx.Done(), so cancellation wouldn't propagate mid-retry anyway.

the cleanest fix imo would be to tackle both at once — change the signature to client.NewWorker(ctx context.Context, name string), pass that context down to grpc.DialContext and swap every time.Sleep(d) in the retry loop with:

select {
case <-time.After(d):
case <-ctx.Done():
    return ctx.Err()
}

if this sounds reasonable to the maintainers, happy to contribute a pr for it.

[gregfurman]

OK I originally misread this.

I don't think we should be changing the method signature for NewWorker since that's quite a far reaching breaking change, but I agree that we should be propagating a context to those internal calls.

after running locally, tbh the root cause is probably two separate gaps that together make cancellation completely cooked: [...]

Agree 👆

TBH the main consideration here would be how to maintain backwards compatability given that so many of those method signatures don't accept a context.Context param. Unfortunately, I think storing a ctx as an attribute of the Worker is probably simplest.

However, the above PR I linked would need to be merged before even that could solve this issue since the underlying methods being called still don't accept a context param i.e

func (w *Worker) RegisterWorkflowV1(workflow *contracts.CreateWorkflowVersionRequest) error {
       // ...
	return w.client.Admin().PutWorkflowV1(workflow) // NOTE: this will change to accept a ctx with PR #3127
}

Suppose then there's the question of whether a context should be passed in via WorkerOptions (proposed by @solidDoWant) or as a standalone method like NewWorkerContext(ctx context.Context, ...) (proposed by @BloggerBust).

While I'm personally in favour of the former WorkerOptions approach, I'm concerned that setting context.Context via the functional arguments could obfuscate that a ctx actually should be passed from the caller -- whereas this approach makes it appear optional. The latter approach is more obvious but comes at the cost of another method to maintain.

(Btw here's a relevant article on using structs and context)

Perhaps there's a third option though: what about if we include Close() or Stop() methods on the Client and Worker that (when called) can interrupt these requests instead of relying on a ctx being passed in? I've used the https://github.com/Jeffail/shutdown package before and that is SUPER useful for situations like this.

Otherwise, I'll chat to the team and see what they think.

[Akhilesh29]

cool

[github-actions]

This issue has been stale for 90 days. Please update the issue or comment to keep it active. Otherwise, it will be closed in 5 days.