Discarded job continues execution (context not cancelled)

[NanoBjorn]

Following #1105 (comment), I think this is worth a separate issue.

As I already described in #1105, there is a theoretically possible situation when actually running and healthy job is marked as discarded. And while it is marked as discarded worker might still continue executing it because it did not receive any notification about job being discarded. It creates a situation which might violate unique_key invariant, as happened in the linked issue — discarded job goroutine is actually running and new job can be potentially scheduled, not violating unique key constraint as from db point of view this is correct

river/riverdriver/riverpgxv5/migration/main/006_bulk_unique.up.sql

Lines 30 to 33 in a8d8127

	CREATE UNIQUE INDEX IF NOT EXISTS river_job_unique_idx ON /* TEMPLATE: schema */river_job (unique_key)
	WHERE unique_key IS NOT NULL
	AND unique_states IS NOT NULL
	AND /* TEMPLATE: schema */river_job_state_in_bitmask(unique_states, state);

I guess it either could be done through pg_notify or polling job row every few seconds, and the second one is not great for db. I don't know what are other good options, so creating this issue to discuss

[bgentry]

Hi @NanoBjorn, manually marking jobs as discarded (it sounds like this is what you're doing?) is not a supported or recommended way to stop them from running. There's a JobCancel API to do this in a best-effort fashion and which uses LISTEN/NOTIFY if not in poll-only mode, but it's still dependent on your workers actually returning once the context is cancelled. If your workers don't respond to cancellation, all you can really do is kill the node that's running it and eventually it'll be rescued by the job rescuer. Cancellation also doesn't have a polling fallback because we don't want to continuously poll all running job rows on all clients due to the perf overhead.

The system's correctness can't be guaranteed if you're manually manipulating DB state outside of the provided interfaces. Some manual tweaks can be harmless, others aren't, and it's pretty nuanced.

Let me know if I've misunderstood the issue here!

[NanoBjorn]

manually marking jobs as discarded (it sounds like this is what you're doing?)

No, I am not doing it. In the linked issue I described a situation when river worker marked job on another worker as discarded, but that worker was unaware of this and continued execution. We are not interfering with the river_job table manually.

[bgentry]

Ah, got it, so this is specifically when the JobRescuer decides that a job is "stuck" and has exhausted all its attempts, thus moving it to discarded even though it's technically still alive running somewhere.

When cancelling we do emit a NOTIFY which lets the running client know that it should cancel the job's context to try to kill it; however this is only going to help in your situation if you're in a LISTEN-enabled setup (not PollOnly) and if your jobs actually respond to context cancellation. Do you think that would be applicable? We could conceivably emit a slightly different event (it's not quite semantically the same as a regular cancel) and the job would still need to be put into either retryable or discarded as appropriate, although maybe we could have the rescuer allow an extra grace period for rescued jobs to allow themselves to exit after this signal is emitted.

Altogether though I'm not sure this is all worth it or that it's the right solution to the problem. Ultimately your issue is that the rescuer is a global process (at least on a given Postgres schema) with a single set of configs, and you'd have preferred (or at least expected to have) different settings for different queues. Is that right?

We've certainly thought about having a far more flexible rescuer but we haven't had a lot of reasons to build that yet. If it were trivial for you to run your own maintenance processes on leader nodes then you could also do this yourself, but atm we haven't made that straightforward as we weren't ready to commit to those internal interfaces long term.