fix(proof_data_handler): Unlock jobs on transient errors

[popzxc]

What ❔

Currently, proof data handler locks the job for proof generation, and starts fetching required data after that.
If any error happens during fetching, the method will err, and the job will remain locked.

This PR changes it, so that if any error occurs, we unlock the job before we return an error.

Additionally, it reduces the amount of non-necessary panics in the touched code, and adds some docs.

Why ❔

Correctness & efficiency.

Checklist

• PR title corresponds to the body of PR (we generate changelog entries from PRs).
• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• Code has been formatted via zk fmt and zk lint.

[pbeza]

@popzxc I think you forgot to update this diagram. I prepared a PR fixing it: #2722.

BTW, I might be repeating myself, but unlike @perekopskiy, I think we should use an ENUM for the status in the db schema itself (instead of TEXT). Right now, we just have an enum in the code here. Having it in the SQL schema too makes it easier to understand and work with the business logic.

[popzxc]

@pbeza thanks for looking into it!

Regarding enums and stronger typing: I don't think @perekopskiy meant that "enums bad, strong typing bad" -- he raised a valid question at the beginning -- what's the cost of this solution? For the prover DB it's even more important. A few immediate things that come to mind:

• we will have to have a blocking migration to introduce it (and I'm not sure if it will be quick). Alternatively, we will have to have multi-step migration, which is always tricky.
• each time we want to add a new state (and we might! especially with the external provers that we have in mind), we will have to do a migration.
• the ergonomics of using enums in SQLx are questionable, and it forces queries to look more dynamic than they in fact are.
• it's some work that has to be done, and we're always at capacity. Time doing it can be spent on something more urgent.

On the other hand, what are the drawbacks of having it in text? The main one is the possibility of typo/error/using non-existent status. It is a valid concern, but in that particular case it's easily discoverable (each typo will inevitably cause the prover to deadlock). Moreover, we have DAL as a layer of isolation, so the surface for bugs is pretty narrow.

Just in case, I as well don't want to say "enums bad, strong typing bad". What I want to say is that everything is a tradeoff and the answer is usually "it depends". If you have strong opinions -- you are more than welcome to write a design doc, compare pros/cons of both approaches, and present it -- you will be heard, and your proposal will be discussed. But it must come with a good motivation to change something that works and works well (even if not fully idiomatic).

[pbeza]

Thanks for the detailed explnation!

Just to clarify my PoV: I think you might be overlooking the human factor in coding. You, @popzxc, and @perekopskiy are likely very familiar and comfortable with the codebase, aware of many design decisions made over the last months or years. However, if someone is new to the code, having status as TEXT feels unnatural unless there's clear explanation in front of them.

One of the key reasons why using ENUMs would be a more natural choice here is that it makes the code easier to reason about. Using TEXT introduces the temptation to add new statuses in an ad hoc manner, rather than properly engineering a fully-fledged solution. In fact, we're about to introduce a new status responsible for permanently skipping batches (cc @thomasknauth). This status will only be represented in Rust's enum, not in the DB's ENUM, which may be sufficient.

That said, I get why you went with TEXT instead of ENUM. I worked on a project once where they made similar trade-offs with the database. The database setup wasn’t very idiomatic either, but it was a performance trade-off in that case.

Anyway, thanks for the thoughtful discussion! ⚔️