It sort of feels like it'd be nice to combine the fetching of the currently started task with the fetching of the latest completed task.

One thing I'm not seeing in this PR is how we talk about different environments and the completion of their tasks independently. I expected to see some sort of environment ID or long-running job per environment or something like that.

Is it the responsibility of the enviroments to make sure that the IDs are monotonically increasing just within that environment, or across all environments?

It seems to me that job success is at most once. If these tasks fail with a permanent error, they are not retried by anything. Cancellation is a mechanism to cause failure. One thing we might want to expose is a way to mark a task as required. In jobs parlance, this would mean that it's non-cancelable. A non-cancelable job treats errors as retryable by default (as opposed to permanent), and it prevents user cancelation. I suspect for the hard-coded tasks, we'll want to mark them as uncancelable.

why are you passing an execCfg and then not using it?

I view an execCfg argument to a function as a code smell

makes sense, I just realized that you might not need to load it. You can probably:

j := &jobs.Job{id: jobs.AutoConfigRunnerJobID}
infoStorage := j.InfoStorage(txn)
...

edit: noticed the id field is not exported so you'd have to add a method to create a Job struct if you want to follow this approach

why are you passing an execCfg and then not using it?

Makes sense, thanks

why are you passing an execCfg and then not using it?

why are you passing an execCfg and then not using it?

ok, that i will do.

Something about this interface feels a little off. I guess it's the fact that it always hands you all the tasks, and that it seems to be something you tell about task completion, but it doesn't have a clear contract around how that affects behavior. The channel-based API also feels a bit fishy.

Is it important for the client to get more than one task at a time?
What about something like:

// Peek will block waiting for the first task the provider believes still needs to be run.
// It will return an error if the context is canceled while waiting.
Peek(ctx context.Context) (autoconfigpb.Task, error)

// Pop will report the completion of all tasks with ID up to completed.
Pop(ctx context.Context, completed TaskID) error

This doesn't implement the contract The channel receives an initial event immediately.

Consider revisiting that.

q: is this a temporary work around while we figure out the registry-level fix? If it is, should we add a TODO?

The comment on Run states:

// The assumption is that these jobs were created by
// this registry and thus are pre-claimed by it.

I don't think that there is any guarantee that prevJobID was created by this node's registry. If the assumption doesn't hold, my read of the code is that the waiting set won't get notified on job completion since it isn't being run by the current registry?

+1, maybe we add a TODO for now @knz to capture this idea of non-cancellable, required tasks?

stale comment?

I currently see only one caller of Pop when the auto config runner sees the last completed task > the task we are processing. Do we expect each task runner to Pop the task on completion?

nit: maybe a quick comment explaining why it is disabled.

Done.

I had the same concern originally (internal convo)

Bottom line is Run falls back to polling if the job is not locally registered already.

actually there was no good reason. Removed.

Given that 99242 has been addressed can we remove this?

nit: leftover commented code