Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AbortException leaves an orphaned job file in the queue #20

Closed
0xDEC0DE opened this issue Apr 27, 2022 · 3 comments
Closed

AbortException leaves an orphaned job file in the queue #20

0xDEC0DE opened this issue Apr 27, 2022 · 3 comments
Labels

Comments

@0xDEC0DE
Copy link
Contributor

Spinach tasks with retries set do not clean themselves up completely if the job throws an AbortException. This is easiest to demonstrate with the RedisBroker:

Steps to reproduce

  • Run the following test case:
from spinach import AbortException, Engine, RedisBroker
from redis.client import Redis

redis = Redis(host=YOUR_REDIS_HOST)
spin = Engine(RedisBroker(redis))

@spin.task(name='fail', max_retries=5)
def fail():
    raise AbortException

spin.schedule(fail)
spin.start_workers(stop_when_queue_empty=True)
  • On the Redis, run KEYS spinach/_running*

Expected result

(empty array)

Actual behavior

The job file remains in the queue, e.g.:

1) "spinach/_running-jobs-on-broker-bcf5eb0c-f3ed-4270-afb8-d44e02b8bbad"

Errata

Tasks that do not set max_retries appear to clean themselves up properly.

@NicolasLM NicolasLM added the bug label Apr 28, 2022
@NicolasLM
Copy link
Owner

I confirm that this is a bug.

@bigjools
Copy link
Collaborator

I had an initial look to see if I can figure out the problem and didn't get anywhere. We could try setting job.retries = job.max_retries to force the cancellation instead of job.max_retries=0 and see if that helps? Something must be causing
self._broker.remove_job_from_running(job) from getting called in the _result_notifier_func.

@bigjools
Copy link
Collaborator

Anyway it would be nice to get this fixed and then we can do a release with the other change I just added.

0xDEC0DE pushed a commit to 0xDEC0DE/spinach that referenced this issue Apr 30, 2022
The Redis broker uses a non-zero value for `max_retries` to determine
if it should remove a job from the "running" state.  So when an
`AbortException` is thrown, max out the `job.retries` counter,
rather than zeroing out the `job.max_retries` counter.  This will
have the same effect, but play more nicely with what the Redis
broker is expecting.

The MemoryBroker is unaffected by this.

Fixes Issue NicolasLM#20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants