Chain: keep a snapshot request alive until it answers its caller

[heifner]

/v1/producer/create_snapshot can close its connection without a response, hanging the caller. Seen in CI as a snapshot_api_test failure on the ubsan shard — http.client.RemoteDisconnected on the second snapshot, with nodeop alive and exiting cleanly afterwards.

The request that endpoint schedules is a one-time request anchored at head + 1, and snapshot_scheduler::on_start_block runs it at start_block_num + 1. unschedule_snapshot_requests collected the request as soon as irreversibility reached start_block_num. Irreversibility never runs ahead of the applied head, so committing block start_block_num lands LIB exactly on the deletion threshold one block before the firing height is applied. The request is erased unexecuted, and destroying the stored completion callback releases the last reference to the caller's HTTP session. Under normal production irreversibility trails the head by two blocks, so the firing height is applied long before the threshold is reached; the window opens for any node applying a run of blocks back to back — catching up after a stall, syncing, or switching forks. In the CI failure the node's application thread was starved for 8.6s while blocks 224 through 232 accumulated, then applied them in one batch.

Request lifetime. One-time requests are collected at lib_height > start_block_num, a threshold first observed while the firing height is being applied, so it can never precede that block's block_start signal. end_block_num no longer bounds them at all: scheduling accepts end_block_num == start_block_num — the shape /v1/producer/schedule_snapshot builds when a caller pins a snapshot to one block, and what Node.scheduleSnapshotAt sends — and expiring on it collected the request one block before it could ever fire, losing it to the same ordering for exactly the requests most specific about which block they want. Recurring requests expire on their end block as before.

Callback lifetime. A request's caller is answered exactly once, and something always answers it. The callback lives in one consumable slot owned by the request and shared with everything that can reach the caller: the handler on each pending snapshot the request produces, and the removal path. Answering moves the callback out of that slot, so whoever gets there first is the only one — which matters because a request outlives its own snapshot in several ways. A snapshot that is forked out is discarded unfinalized and the still-scheduled request regenerates it on the adopted branch; a spent request stays scheduled until irreversibility passes its firing height; and a request can be cancelled at any point, including while a snapshot from it is pending.

Removal. remove_request is the only way a request leaves the container. Both expiry and explicit cancellation through /v1/producer/unschedule_snapshot go through it, and a caller still waiting is resolved with a snapshot_execution_exception rather than having its callback destroyed. An outstanding create_snapshot request is listed by get_snapshot_requests, so its id is available to cancel.

Delivery. The completion handler settles the scheduler's own bookkeeping before answering, and logs and drops what the callback throws. A caller failing on its own account — writing to a client that has gone away — previously escaped into the snapshot pipeline: on_irreversible_block skipped notify_snapshot_finalized for a snapshot that had finalized, which is how provider mode learns to vote on it, and create_snapshot re-entered the handler through CATCH_AND_CALL to report the caller's own failure back to it.

tests/test_snapshot_scheduler.cpp gains cases for each of these, every one failing with only its own fix reverted: the catch-up ordering for the open-ended and exact-range request shapes, a forked-out snapshot regenerating and answering the original caller, a throwing callback resolved once, and cancellation before execution, while a snapshot is in flight, and after delivery. tests/nodeop_snapshot_forked_test.py already issued a create_snapshot across a fork but discarded the result, so an unanswered request left a dead RPC thread rather than a failure; it now asserts the call came back with a snapshot.

[huangminghuang]

Two inline findings from the delegated OCR review.

[huangminghuang]

[P1] Preserve the callback across fork retries

Clearing req.next when execution merely starts breaks the intentional fork-regeneration path. If the snapshotted block is forked out, the same one-time request runs again when the adopted branch reapplies start_block_num + 1, but the regenerated pending snapshot now receives an empty HTTP callback. If the losing pending snapshot is invalidated when LIB lands at that height, its finalization exception is logged and dropped, so /v1/producer/create_snapshot is never answered. Please retain the callback until the execution completion wrapper actually resolves it, then clear it there; the existing nodeop_snapshot_forked_test.py flow should also assert this timing.

[heifner]

Fixed in e6c7f5a — confirmed, and the mechanism is exactly as you describe. pending_snapshot::finalize throws when the snapshotted block is forked out, on_irreversible_block swallows it with FC_LOG_AND_DROP, so the completion wrapper never runs and the caller is not answered. The regeneration on the adopted branch was the only thing left that could answer it, and it was handed an empty callback.

on_start_block no longer touches the callback. execute_snapshot's completion handler drops the request's copy immediately after http_next resolves, via clear_delivered_request_callback. Clearing at delivery rather than never also matters in the other direction: a next_function permits exactly one invocation across all of its copies, and a spent request stays scheduled until irreversibility passes its firing height, so a further fork at start_block_num + 1 could otherwise reach the same callback a second time.

New case forked_out_snapshot_regenerates_and_still_answers_caller drives it: fire, invalidate the pending snapshot with a competing block id at the snapshotted height, refire, promote. It fails with only this half reverted — check outcome.successes == 1u has failed [0 != 1].

On nodeop_snapshot_forked_test.py — good catch that it should assert this. It already issues the create_snapshot across the fork, but discarded the result, so a dropped callback showed up only as a silently dead RPC thread and the test could still pass. It now records the outcome, joins with a timeout, and asserts a snapshot_name came back.

[huangminghuang]

[P2] Keep exact-range one-shot requests through the firing signal

The lib_height >= req.end_block_num arm still deletes a valid one-time request when end_block_num == start_block_num and LIB reaches that height before on_start_block(start_block_num + 1). That accepted shape is already used by the scheduler tests, so the catch-up ordering still loses those requests; only the new end_block_num == UINT32_MAX test is fixed. Please special-case one-time expiry so equality at start_block_num cannot remove it, and add the exact-range ordering as a regression test.

[heifner]

Fixed in e6c7f5a — confirmed, and it is reachable beyond the scheduler tests: Node.scheduleSnapshotAt() sends {start_block_num: n, end_block_num: n}, and nodeop_snapshot_diff_test.py uses it against a node that is replaying and then syncing to catch up, which is the same back-to-back ordering.

One-time requests no longer consult end_block_num at all:

bool marked_for_deletion = req.block_spacing ? lib_height >= req.end_block_num
                                             : lib_height > req.start_block_num;

Scheduling enforces end_block_num >= start_block_num, so dropping that arm can only delay collection, and only in the end == start case — a one-time request is still always collected once irreversibility passes start_block_num, never left scheduled indefinitely.

New case exact_range_onetime_request_survives_lib_reaching_its_start_block covers the ordering for that shape and fails with only this half reverted — critical check f.scheduler.get_snapshot_requests().snapshot_requests.size() == 1u has failed [0 != 1].

[huangminghuang]

Two additional inline findings from the delegated OCR review of the current head.

[huangminghuang]

[P2] Clear the stored callback even when the response handler throws

http_next(...) runs before this clear. If it throws (for example while writing to a disconnected HTTP client), on_irreversible_block() logs and swallows the exception, then unschedule_snapshot_requests() still sees req.next and invokes the same next_function again. next_function permits exactly one invocation across all copies, so the second call is undefined behavior. Please make clearing exception-safe—clear before invocation or use a scope guard—and cover a throwing completion callback.

[heifner]

Fixed in 72575a1 — confirmed. Chasing it turned up a second instance of the same hazard on a different route: in irreversible read mode create_snapshot invokes the completion handler inside the region guarded by CATCH_AND_CALL(next), so a throwing delivery re-enters that same handler and calls the caller's next_function again directly, without unschedule_snapshot_requests being involved at all.

Three changes, one per path:

• The caller's callback now lives in a shared_ptr slot the handler moves out of before use, so the first delivery is the only one whichever copy of the handler runs — that closes the CATCH_AND_CALL re-entry.
• clear_delivered_request_callback runs before the call rather than after, so a throwing callback cannot leave req.next set for the removal path to resolve again — the case you reported.
• The delivery itself is wrapped in FC_LOG_AND_DROP. A caller failing on its own account should not propagate into the snapshot pipeline, and letting it escape had a second cost: on_irreversible_block skipped notify_snapshot_finalized for a snapshot that had in fact finalized, which is how provider mode learns to vote on it.

I also moved the delivery below the handler's own bookkeeping, so a failing caller cannot leave the request's pending_snapshots half-pruned.

throwing_completion_callback_is_resolved_once covers it: the callback counts its invocations and throws, and a single irreversible block both delivers the snapshot and carries LIB past the firing height, which is the ordering that resolved it twice. With the clear moved back after the call it fails — check invocations == 1u has failed [2 != 1].

[huangminghuang]

[P2] Resolve callbacks when a request is explicitly unscheduled

This new contract says a request removed before delivery resolves its callback with snapshot_execution_exception, but public unschedule_snapshot() still erases the entry without invoking next. An outstanding /v1/producer/create_snapshot request is visible through get_snapshot_requests(), so explicitly unscheduling that id can still destroy the last HTTP-session reference and reproduce the unanswered connection this PR fixes. Please route explicit cancellation and automatic expiry through a common callback-resolving removal path, with a regression test for manual unscheduling.

[heifner]

Fixed in 72575a1 — confirmed, and reachable exactly as you describe: get_snapshot_requests() lists an outstanding create_snapshot request like any other, so its id can go straight to /v1/producer/unschedule_snapshot.

Removal now has a single path. A private remove_request(sri, undelivered_reason) takes the callback out, erases the entry, and resolves the callback with a snapshot_execution_exception; unschedule_snapshot and unschedule_snapshot_requests both go through it. The reason is an fc::log_message built at each call site, so the expiry case keeps its irreversible-block height and each keeps its own log context.

Two tests: unscheduling_an_outstanding_request_reports_failure_to_caller, which fails against the old unschedule_snapshot with check outcome.errors == 1u has failed [0 != 1]; and unscheduling_a_delivered_request_does_not_re_resolve_caller, which pins the other direction — a request that has already answered its caller stays scheduled until irreversibility passes its firing height, and cancelling it in that window must not reach the callback again.

[huangminghuang]

[P2] Share callback consumption with in-flight execution handlers

remove_request() invokes this copy of req.next, but execute_snapshot() has already copied the same next_function into a separate caller_slot captured by the pending snapshot. If the request is unscheduled after on_start_block() creates that pending snapshot but before it becomes irreversible, this path answers the caller with an error; when the snapshot later finalizes, its still-populated slot invokes the already-consumed callback again, which violates next_function’s exactly-once contract and is undefined behavior. The new tests cover cancellation before execution and after delivery, but not this in-flight window. Please make the consumable slot request-wide/shared with removal (or invalidate the pending handler) and add a schedule → start → unschedule → finalize regression test.

[heifner]

Fixed in b4a3109 — confirmed. The two slots were independent by construction: execute_snapshot built its own from a copy of req.next, so neither side could see the other's consumption. The same crossing existed between two snapshots in flight from one request, which a fork can produce — the second execute_snapshot call made a second independent slot from a req.next that was still populated.

Taking your first option: the slot is now the request's and is shared rather than copied. snapshot_schedule_information holds a shared_ptr<next_function<snapshot_information>>, on_start_block passes that pointer to execute_snapshot instead of a copy of what it points at, and both the completion handler and remove_request go through take_pending_answer, which moves the callback out if one is still waiting. Whoever gets there first empties the slot for everyone else. clear_delivered_request_callback is gone with it — consuming the shared slot is the clear, and it no longer has to find the request by id to do it.

A null slot means "scheduled with no caller" and stays distinct from an emptied one, so requests restored from snapshot-schedule.json need no special case.

unscheduling_an_in_flight_request_answers_caller_once is the schedule → start → unschedule → finalize case: cancellation answers the caller, the pending snapshot then finalizes and notifies its subscribers as usual but does not answer again. Against the previous commit it fails with check outcome.successes == 0u has failed [1 != 0].

[huangminghuang]

Approved — the latest shared callback-slot change resolves the in-flight cancellation/double-invocation issue, and the full diff is clean.

Non-blocking request before merge: please refresh the PR description. It still describes clearing the callback in on_start_block and only the original two changes/tests, while the current PR instead uses a request-wide consumable callback slot and now includes fork-retry preservation, throwing-callback hardening, explicit/in-flight cancellation handling, and the additional regression coverage.