apollo_batcher: add abort_proposal api

[itamar-starkware]

TL;DR

Extracted proposal abortion logic into a dedicated abort_proposal method while maintaining backward compatibility with the existing SendProposalContent::Abort approach.

What changed?

• Added a new public abort_proposal method that directly handles proposal abortion with proper error handling for unknown proposals
• Modified the existing SendProposalContent::Abort handler to call the new method internally
• Added AbortProposal request/response variants to the communication layer
• Implemented the new abort_proposal method in the BatcherClient trait and its implementations
• Added comprehensive test coverage for the new abort functionality, including edge cases like aborting unknown proposals, aborting after finish, and aborting after abort

How to test?

Run the existing test suite, which now includes:

• abort_unknown_proposal() - tests aborting a non-existent proposal
• abort_after_finish() - tests aborting an already finished proposal
• send_proposal_content_after_abort() - tests sending content after abortion
• abort_after_abort() - tests double abortion
• abort_proposal_test() - tests the new abort method with metrics validation

Why make this change?

This change provides a cleaner, more direct API for proposal abortion while maintaining backward compatibility. The new method offers better error handling and a more intuitive interface for callers who need to abort proposals, preparing for the eventual migration away from the SendProposalContent::Abort approach.

---

Note

Medium Risk
Touches the batcher’s proposal lifecycle/termination path and request routing, so mistakes could leave validation streams uncleared or misreport proposal state. Changes are localized and covered by expanded unit tests and dashboard instrumentation.

Overview
Adds a dedicated abort_proposal API for aborting in-flight validation proposals, and threads it through the infra layer via new BatcherRequest::AbortProposal / BatcherResponse::AbortProposal and a new BatcherClient::abort_proposal method.

Refactors the existing SendProposalContent::Abort path to be a backward-compatible shim over abort_proposal, centralizing proposal-existence checks via ensure_validate_proposal_exists and updating tests/metrics and Grafana dashboards to cover the new request variant.

^{Written by Cursor Bugbot for commit 1433f50. This will update automatically on new commits. Configure here.}

[reviewable-StarkWare]

This change is 

[itamar-starkware]

• apollo_batcher: use send_txs_request and remove send_proposal_content #12976
• apollo_batcher: add send_txs_for_proposal api #12975
• apollo_batcher: use finish proposal api #12972
• apollo_batcher: add finish_proposal api #12952
• apollo_batcher: use abort_proposal api #12949
• apollo_batcher: add abort_proposal api #12945 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[ArniStarkware]

crates/apollo_batcher/src/batcher.rs line 571 at r1 (raw file):

        if !self.validate_tx_streams.contains_key(&proposal_id) {
            return Err(BatcherError::ProposalNotFound { proposal_id });
        }

This changes the functionality. What is the story? Did we design a new test for this case?
Were we fine earlier with "aborting proposals" where the proposal did not exist (thus it was not active??)

Code quote:

        if !self.validate_tx_streams.contains_key(&proposal_id) {
            return Err(BatcherError::ProposalNotFound { proposal_id });
        }

[ArniStarkware]

@ArniStarkware reviewed 9 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on itamar-starkware and TzahiTaub).

---

crates/apollo_batcher/src/batcher.rs line 571 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

This changes the functionality. What is the story? Did we design a new test for this case?
Were we fine earlier with "aborting proposals" where the proposal did not exist (thus it was not active??)

I take that back. I see how this check comes from send_proposal_content.

---

crates/apollo_batcher/src/batcher_test.rs line 773 at r1 (raw file):

}

/// TODO(Itamar): Remove this test once all callers migrate to `abort_proposal`.

Suggestion:

/// TODO(Itamar): Remove this test once all callers migrate to `abort_proposal`. The test-case is covered by `abort_proposal_test`.

[ArniStarkware]

crates/apollo_batcher/src/batcher.rs line 571 at r1 (raw file):

        if !self.validate_tx_streams.contains_key(&proposal_id) {
            return Err(BatcherError::ProposalNotFound { proposal_id });
        }

Extract to a function.

Suggestion:

    fn proposal_in_validate_tx_streams(poroposal_id: Type) -> Result<(), BatcherError> {
        if !self.validate_tx_streams.contains_key(&proposal_id) {
            return Err(BatcherError::ProposalNotFound { proposal_id });
        }
    Ok(())
}

[ArniStarkware]

crates/apollo_batcher/src/batcher_test.rs line 678 at r1 (raw file):

}

/// TODO(Itamar): Remove this test once all cases are tested separately for each method.

comment -> Two slashes.
Docstring -> Three slashes.

Suggestion:

// TODO(Itamar): Remove this test once all cases are tested separately for each method.

[ArniStarkware]

@ArniStarkware made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on itamar-starkware and TzahiTaub).

---

crates/apollo_batcher_types/src/communication.rs line 64 at r1 (raw file):

    async fn validate_block(&self, input: ValidateBlockInput) -> BatcherClientResult<()>;

    /// TODO(Itamar): Remove this method once all callers migrate to their own method.

Suggestion:

    // TODO(Itamar): Remove this method once all callers migrate to their own method.

[itamar-starkware]

@itamar-starkware made 4 comments and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ArniStarkware and TzahiTaub).

---

crates/apollo_batcher/src/batcher.rs line 571 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Extract to a function.

Done

---

crates/apollo_batcher/src/batcher_test.rs line 678 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

comment -> Two slashes.
Docstring -> Three slashes.

My mistake. Done.

---

crates/apollo_batcher_types/src/communication.rs line 64 at r1 (raw file):

    async fn validate_block(&self, input: ValidateBlockInput) -> BatcherClientResult<()>;

    /// TODO(Itamar): Remove this method once all callers migrate to their own method.

Done

---

crates/apollo_batcher/src/batcher_test.rs line 773 at r1 (raw file):

}

/// TODO(Itamar): Remove this test once all callers migrate to `abort_proposal`.

Done

[ArniStarkware]

@ArniStarkware reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on TzahiTaub).

[TzahiTaub]

@TzahiTaub reviewed 9 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on itamar-starkware).

---

crates/apollo_batcher/src/batcher_test.rs line 771 at r2 (raw file):

    let result = batcher.abort_proposal(PROPOSAL_ID).await;
    assert_eq!(result, Err(BatcherError::ProposalNotFound { proposal_id: PROPOSAL_ID }));
}

Consider keeping the previous - single test multiple cases structure, as I feel it's easier to comprehend than having these multiple tests. All test cases are small variants of the same expected behavior.

Suggestion:

#[derive(Clone)]
enum EndAction {
    Finish,
    Abort,
}
#[derive(Clone)]
enum AfterEndAction {
    Abort,
    SendContent(SendProposalContent),
}
#[rstest]
#[case::abort_after_finish(FirstAction::Finish, SecondAction::Abort)]
#[case::send_txs_after_abort(
    FirstAction::Abort,
    SecondAction::SendContent(SendProposalContent::Txs(test_txs(0..1)))
)]
#[case::send_finish_after_abort(
    FirstAction::Abort,
    SecondAction::SendContent(SendProposalContent::Finish(DUMMY_FINAL_N_EXECUTED_TXS))
)]
#[case::send_abort_after_abort(FirstAction::Abort, SecondAction::Abort)]
#[tokio::test]
async fn proposal_not_found_after_terminal_action(
    #[case] end_action: EndAction,
    #[case] after_end_action: AfterEndAction,
) {
    let mut batcher = create_batcher_with_active_validate_block(Ok(
        BlockExecutionArtifacts::create_for_testing().await,
    ))
    .await;
    match end_action {
        EndAction::Abort => {
            batcher.abort_proposal(PROPOSAL_ID).await.unwrap();
        }
        EndAction::Finish => {
            let end = SendProposalContentInput {
                proposal_id: PROPOSAL_ID,
                content: SendProposalContent::Finish(DUMMY_FINAL_N_EXECUTED_TXS),
            };
            batcher.send_proposal_content(end).await.unwrap();
        }
    }
    let result = match after_end_action {
        AfterEndAction::Abort => batcher.abort_proposal(PROPOSAL_ID).await,
        AfterEndAction::SendContent(content) => {
            let input = SendProposalContentInput { proposal_id: PROPOSAL_ID, content };
            batcher.send_proposal_content(input).await.and(Ok(()))
        }
    };
    assert_eq!(result, Err(BatcherError::ProposalNotFound { proposal_id: PROPOSAL_ID }));
}

[TzahiTaub]

@TzahiTaub made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on itamar-starkware).

---

crates/apollo_batcher/src/batcher_test.rs line 771 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Consider keeping the previous - single test multiple cases structure, as I feel it's easier to comprehend than having these multiple tests. All test cases are small variants of the same expected behavior.

This comment (+the enums) are relevant to the new tests above as well.

[TzahiTaub]

@TzahiTaub reviewed 8 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on itamar-starkware).

[ArniStarkware]

@ArniStarkware reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on itamar-starkware).

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

[cursor]

Redundant existence check in abort via send_proposal_content

Low Severity

When send_proposal_content receives SendProposalContent::Abort, ensure_validate_proposal_exists is called twice — once at the top of send_proposal_content (line 489) and again inside abort_proposal (line 569). The second call is always redundant when reached through send_proposal_content, since &mut self prevents concurrent modification between the two checks. The reviewer (ArniStarkware) also flagged this — extracting the check into a helper that both callers share was suggested, but the duplication wasn't resolved.

Additional Locations (1)

• crates/apollo_batcher/src/batcher.rs#L568-L569

 

[TzahiTaub]

@TzahiTaub made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on itamar-starkware).

---

crates/apollo_batcher/src/batcher_test.rs line 675 at r4 (raw file):

)]
// TODO(Itamar): Remove this case once we migrate to `abort_proposal`.
// This case will be tested in `send_proposal_content_after_abort`.

This doesn't seem to be covered in the send_proposal_content_after_abort that later changed to the proposal_not_found_after_terminal_action test. In there we get ProposalNotFound while in all cases here we expect a different result. An invalid block doesn't behave the same as a terminated block. Please fix the test (in the entire stack) - AFAIK, keep this test and adjust it.

Code quote:

// TODO(Itamar): Remove this case once we migrate to `abort_proposal`.
// This case will be tested in `send_proposal_content_after_abort`.