Deregistration doesn't happen when runner process exits or crashes

[jonathan528i]

Currently, if the runner application exits either normally (e.g. runner auto updates) or due to an error (e.g., exit code 1), the runner is not deregistered automatically.

In my docker compose file, I have set restart: always to try to keep runners available, but if the runner process exits and then the container is restarted, a new runner instance is registered and the old instance is orphaned. Our runner was continuously exiting due to errors at launch, and we ended up registering 10,000 runner instances, hitting the limit of allowed runner instances.

From what I can tell, this seems to happen because the deregister trap set up in entrypoint.sh only catches external signals to kill the runner like SIGTERM or SIGINT, but the runner exiting by itself isn't covered.

I think if we add EXIT to the list of signals to catch, that should allow deregistration to happen when the runner process exits by itself:

trap_with_arg deregister_runner SIGINT SIGQUIT SIGTERM INT TERM QUIT EXIT

[myoung34]

Im open to a PR if we can add a test for it

[jonathan528i]

Im open to a PR if we can add a test for it

#582 posted.

[myoung34]

Looking at your PR I dont think were solving the issue youre describing

but the runner exiting by itself isn't covered.

The runner exiting by itself is only for 2 reasons:

1. an update and auto updates arent enabled
2. ephemeral runners (which deregistration does work)
3. start up errors (would not register as a runner so no deregistration would be needed)

If its erroring at launch, thats different and Id be curious about the logs because it should have errored before it registered successfully

[jonathan528i]

The runner exiting by itself is only for 2 reasons:

an update and auto updates arent enabled
ephemeral runners (which deregistration does work)
start up errors (would not register as a runner so no deregistration would be needed)

I'm not that familiar with the specifics, but just from looking at entrypoint.sh, the registration seems to be done via ./config.sh which I think doesn't really run the github runner yet?

configure_runner() {
  # ...
  ./config.sh \
      --url "${_SHORT_URL}" \
      --token "${RUNNER_TOKEN}" \
      --name "${_RUNNER_NAME}" \
      --work "${_RUNNER_WORKDIR}" \
      --labels "${_LABELS}" \
      --runnergroup "${_RUNNER_GROUP}" \
      --unattended \
      --replace \
      "${ARGS[@]}"
}

If its erroring at launch, thats different and Id be curious about the logs because it should have errored before it registered successfully

Sorry, because the runner ran for a very long time (due to restart: always) before we noticed it had maxed out 10,000 runner registrations, I don't have the logs available before the runner failed to register a new instance.

However, you can try remove my change in the PR and run the new test I added. The new test runs /entrypoint.sh /bin/bash -c "exit 1" which simulates the runner crashing on launch. You'll see that the test will fail (i.e. deregister does not occur) if EXIT is not trapped.

Just for reference, here's what the test output shows when I remove EXIT from the call to trap_with_arg deregister_runner:

Command: /entrypoint.sh /bin/bash -c "exit 1": exit-status: matches expectation: 1
Command: /entrypoint.sh /bin/bash -c "exit 1": stdout:
Expected
    "object: *bytes.Reader"
to have patterns
    ["Runner reusage is disabled","Caught EXIT - Deregistering runner"]
the missing elements were
    ["Caught EXIT - Deregistering runner"]


Failures/Skipped:

Command: /entrypoint.sh /bin/bash -c "exit 1": stdout:
Expected
    "object: *bytes.Reader"
to have patterns
    ["Runner reusage is disabled","Caught EXIT - Deregistering runner"]
the missing elements were
    ["Caught EXIT - Deregistering runner"]

Total Duration: 1.228s
Count: 2, Failed: 1, Skipped: 0
INFO: Stopping container
INFO: Deleting container

[myoung34]

Im happy to merge it, though I really dont believe it will resolve the long term problem you had