Skip to content

fix(prover-client): reject stale job promises and count timeouts toward retry limit#21842

Merged
PhilWindle merged 4 commits into
merge-train/spartanfrom
spy/fix-proving-broker-audit-711-715
Apr 21, 2026
Merged

fix(prover-client): reject stale job promises and count timeouts toward retry limit#21842
PhilWindle merged 4 commits into
merge-train/spartanfrom
spy/fix-proving-broker-audit-711-715

Conversation

@spypsy

@spypsy spypsy commented Mar 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • Fixes A-711: cleanUpProvingJobState was calling deferred.promise.catch(() => {}) before deferred.reject() to suppress unhandled rejections, but this doesn't work — .catch() creates a new branched promise; any code already awaiting the original promise still receives an unhandled rejection. Fixed by resolving with { status: 'rejected', reason: '...' } instead, consistent with how the rest of the class settles promises, and making unhandled rejections impossible.

Fixes A-711

spypsy added 2 commits March 20, 2026 13:54
@spypsy
fix(prover-client): reject stale job promises and count timeouts towa…
f970150 
…rd retry limit

Fixes A-711: cleanUpProvingJobState was deleting promises without settling
them first, causing any awaiter to hang forever. Now rejects each unsettled
promise before removal.

Fixes A-715: timed-out jobs were re-enqueued without incrementing the retry
counter, allowing them to loop forever bypassing maxRetries. Now increments
the retry count on each timeout re-enqueue.

Adds regression tests for both fixes.

Made-with: Cursor
@spypsy
fix: suppress unhandled rejection when no consumer awaits cleaned-up …
bc43227 
…job promise

Made-with: Cursor
Base automatically changed from merge-train/spartan to next March 20, 2026 22:34
@PhilWindle PhilWindle requested a review from alexghr March 25, 2026 09:29
PhilWindle
PhilWindle requested changes Mar 31, 2026
View reviewed changes
Comment thread yarn-project/prover-client/src/proving_broker/proving_broker.ts Outdated
@@ -632,8 +637,10 @@ export class ProvingBroker implements ProvingJobProducer, ProvingJobConsumer, Pr
const now = this.msTimeSource();
const msSinceLastUpdate = now - metadata.lastUpdatedAt;
if (msSinceLastUpdate >= this.jobTimeoutMs) {
const retries = this.retries.get(id) ?? 0;

@PhilWindle PhilWindle Mar 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to make this change. We should limit this PR to the other change only.

@spypsy spypsy Apr 2, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undid A-715 fixes, will do in different PR

@PhilWindle PhilWindle Apr 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting of this.retries should have been removed shouldn't it?

alexghr
alexghr reviewed Mar 31, 2026
View reviewed changes
Comment thread yarn-project/prover-client/src/proving_broker/proving_broker.ts Outdated
Comment thread yarn-project/prover-client/src/proving_broker/proving_broker.ts Outdated
@spypsy spypsy changed the base branch from next to merge-train/spartan April 2, 2026 10:59
@spypsy spypsy force-pushed the spy/fix-proving-broker-audit-711-715 branch 3 times, most recently from 6ecca5c to 1da727e Compare April 2, 2026 11:54
@spypsy spypsy force-pushed the spy/fix-proving-broker-audit-711-715 branch from 1da727e to 5034e17 Compare April 7, 2026 11:32
@PhilWindle PhilWindle merged commit aef1970 into merge-train/spartan Apr 21, 2026
12 checks passed
@PhilWindle PhilWindle deleted the spy/fix-proving-broker-audit-711-715 branch April 21, 2026 11:28
@AztecBot AztecBot mentioned this pull request Apr 21, 2026
Merged
chrismarino pushed a commit to chrismarino/aztec-packages that referenced this pull request May 5, 2026
@PhilWindle
feat: merge-train/spartan (AztecProtocol#22682)
7acf613 
BEGIN_COMMIT_OVERRIDE
fix(kv-store): ensure LMDB cursor is closed on iteration abort (AztecProtocol#22509)
fix(telemetry-client): use appropriate histogram buckets for L1 gas
prices (AztecProtocol#22512)
fix(telemetry-client): log warning when BatchSpanProcessor drops spans
(AztecProtocol#22511)
fix(stdlib): wrap HA signer databaseUrl in SecretValue (AztecProtocol#22510)
fix(prover-client): don't mark in-progress epoch N jobs as stale when
epoch N+1 starts (AztecProtocol#22508)
chore: (A-730) graceful shutdown for services in node startup failure
path (AztecProtocol#22112)
fix(prover-client): reject stale job promises and count timeouts toward
retry limit (AztecProtocol#21842)
feat(archiver): validate historical L1 log availability at startup
(AztecProtocol#22644)
fix(archiver): do not query MessageSent events by blockhash (AztecProtocol#22641)
refactor(e2e): skip initial sequencer in p2p and epochs tests (AztecProtocol#22535)
fix: handle missing L1 finalized block on devnets (AztecProtocol#22663)
fix(world-state): treat historical block 0 queries as historical, not
latest (AztecProtocol#22679)
fix(sequencer): re-check parent checkpoint validity before pipelined L1
submission (AztecProtocol#22586)
fix(world-state): make block 0 a first-class historical block (AztecProtocol#22711)
chore: show all running versions (AztecProtocol#22376)
chore: fix prettier inside worktrees (AztecProtocol#22557)
feat: use optimized verifier for rollup (AztecProtocol#21840)
fix(kv-store): skip pool creation on ephemeral deleteDb to unstick
browser tests (AztecProtocol#22693)
chore: rm claude lockfile (AztecProtocol#22718)
fix(e2e): wait for first checkpoint in fee_asset_price_oracle_gossip
test (AztecProtocol#22719)
chore(prover-node): track estimated L1 fee when proof publishing is
disabled (AztecProtocol#22691)
fix(ci): rerun squashed PR check on base branch change (AztecProtocol#22713)
feat(archiver): decouple calldata from blob fetching in L1 synchronizer
(AztecProtocol#22716)
refactor(e2e): enable pipelining in e2e_epochs tests (AztecProtocol#22544)
feat(p2p): reject and evict txs with insufficient max fee per gas
(AztecProtocol#22118)
refactor(world-state): always index block 0 regardless of initial tree
size (AztecProtocol#22724)
fix(e2e): fix redistribution test (AztecProtocol#22729)
END_COMMIT_OVERRIDE
@AztecBot AztecBot mentioned this pull request May 7, 2026
Merged
rangozd pushed a commit to rangozd/aztec-packages that referenced this pull request May 16, 2026
@dbanks12
chore(ci): flag ProvingBroker "does not retry if job is stale" as fla…
8a62f05 
…ke (AztecProtocol#23047)

Flagging `ProvingBroker > Retries > does not retry if job is stale` as a
flake in `.test_patterns.yml`. Failure surfaced on an unrelated wallet
PR — `dbanks12`'s wallet refactor — at
http://ci.aztec-labs.com/64a972aafaa40dd0.

## Failure

```
● ProvingBroker › Retries › does not retry if job is stale

  Store is closed

  > 99 |             throw new Error('Store is closed');
        |                   ^

  at AztecLMDBStoreV2.transactionAsync (yarn-project/kv-store/dest/lmdb-v2/store.js:99:19)
  at SingleEpochDatabase.transactionAsync [as batchWrite]
    (yarn-project/prover-client/src/proving_broker/proving_broker_database/persisted.ts:45:22)
  at KVBrokerDatabase.batchWrite [as commitWrites]
    (yarn-project/prover-client/src/proving_broker/proving_broker_database/persisted.ts:120:14)
```

The broker tries to commit the final `reportProvingJobError` write after
the per-epoch LMDB store has already been closed (the test advances the
epoch from 1 → 3, which causes the epoch-1 store to be torn down). The
race is between the epoch advance / cleanup path and the final error
write — a timing flake, not a logic bug.

## Owner

Test was authored by `@alexghr` in AztecProtocol#9400 (`feat: new proving broker
implementation`) and most recently edited by `@alexghr` in AztecProtocol#22508
(`fix(prover-client): don't mark in-progress epoch N jobs as stale when
epoch N+1 starts`). `@spypsy` has also recently fixed retries-related
races in this file (AztecProtocol#21842, AztecProtocol#22355). Pinging Alex as primary owner; tag
Spyros if it's actually a retry-counter race rather than a
store-lifecycle race.

## Other branches

Spot-checked the most recent failed runs on `merge-train/fairies` and
`merge-train/spartan` — none of them hit this same `proving_broker` /
`Store is closed` failure in the data window I sampled. The flake has
only been observed on the one wallet PR run linked above so far.

## Pattern entry

The new entry uses both `regex` (test file path) and `error_regex`
(`does not retry if job is stale|Store is closed`) so unrelated failures
in `proving_broker.test.ts` still fail CI — only this specific timing
race gets quarantined to a Slack ping.

---
*Created by
[claudebox](https://claudebox.work/v2/sessions/b4b6eb63ff789d29) ·
group: `aztec`*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexghr alexghr alexghr left review comments

@PhilWindle PhilWindle PhilWindle approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@spypsy @alexghr @PhilWindle