4756-race-condition-in-job-request-follow

[rdf-gds]

The CLI can try to follow a job before it has been assigned a name.

Rather than exiting with an error, keep watching the stream until the job has a name.

[samsimpson1]

Does this sleep actually do anything meaningful? All it'd do is delay it processing another message from the channel. Wouldn't we want to process the next one immediately? I imagine we'd eventually get a message when the name is assigned, right?

[AP-Hunt]

Stops it hammering the K8s API, and stops it eating the user's CPU time in tight(ish) loop for a period of time

[samsimpson1]

But this is using a watch, so the connection will be open anyway, and k8s will always send every event its watching for

[AP-Hunt]

Ah yeah you're right, didn't spot that. But without the sleep, we'd never get a timeout. It'd get stuck waiting on the channel forever, potentially.

This block of code might be better structured as

ctx_ := context.WithTimeout(ctx, 1 * time.Second)

for {
    select {
    case <- ctx_.Done():
        // timeout occurred 

    case <- w.ResultChan():
        // Result has come in
        // break if successful  
    }
}

[rdf-gds]

Now I understand the concept of the watch stream. Change made.

[rdf-gds]

I'm not sure how to differentiate between a timeout waiting for a jobRequest name (bad) and a timeout waiting for an approval (normal). Feels like never getting a name might be so rare that we could just leave it to the user..?

[AP-Hunt]

It could feasibly never get a name if it enters the conflict state, or if there's a straight up error or crash loop in the controller.

I think you probably only care about timing out once the job enters/has the approved or started states. Before that (pending), you're right that there isn't really any valid timeout we could apply.

Perhaps something like

func awaitJobRequest(name) (*JobRequest, err) {
    watcher := watch(name)
    var timeout *context.Context
    
    for {
        evt := <- watcher.Chan()
        obj := evt.Object().(*JobRequest)
        
        select obj.Status.State {
            case Approved, Started:
                if timeout == nil {
                     timeout := context.WithTimeout(context.Background(), 10 * time.Second) 
                }
                
                select {
                case <-timeout.Done(): // only matches if there's anything on the channel
                    return nil, fmt.Errorf("timed out")
                default:
                    // drops out the select
                }
                
                if hasJobName(obj) {
                    return obj, nil
                }
        }
    }
    
}

Start the timer when you first see the approved/started state. Otherwise, let is run for eternity.

[rdf-gds]

I've added a select to catch a SIGINT. You all understand the context of the tool better than me, but it feels to me like that's enough.

[AP-Hunt]

I think this is a reasonable approach, but context.Background() (the context used by JobRequestClient) isn't wired up for SIGINT handling out of the box. Cobra provides a context which is though. If we pass that through to CreateJobRequestClient the rest of this code should work as intended.

[rdf-gds]

change made

[github-actions]

Coverage report

Overall coverage: 85.8% 🔴 - Change vs. main: 👇 -.7%

Package	Statements Covered	Happy?	Change vs Main
command-line-arguments	100.0%	⭐	👈 +/- 0%
github.com/alphagov/govuk-cli/cmd	82.5%	🔴	☝️ +.4%
github.com/alphagov/govuk-cli/internal/jobrequest	86.5%	🔴	👇 -1.1%
github.com/alphagov/govuk-cli/internal/kubernetes	87.5%	🔴	👈 +/- 0%
github.com/alphagov/govuk-cli/internal/style	100.0%	⭐	👈 +/- 0%
github.com/alphagov/govuk-cli/internal/whoami	80.0%	🔴	👈 +/- 0%