Skip to content

fix(speculation): exhaust cancelled viable paths - #1036

Merged
sbalabanov merged 1 commit into
mainfrom
wt/sqc013
Nov 13, 2026
Merged

sbalabanov merged 1 commit into
mainfrom
wt/sqc013

Conversation

@sbalabanov

@sbalabanov sbalabanov commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

TL;DR: Requests no longer stay stuck when their last viable build is cancelled.

Summary

This fixes “nothing left to build, but the request never finishes”: a cancelled last viable build was neither retried nor counted as exhausted. A batch now finishes as failed only after its dependencies settle and every viable path failed or was cancelled; other viable paths can continue, and valid passed results can proceed when their prerequisites allow. Cancelled builds are not retried, and their recorded outcome remains Cancelled.

Repro

  1. Start a batch with all dependencies settled and just one viable build.
  2. Have the build provider cancel that build, then let the normal build-signal and speculation processing run.
  3. Before this change, no CI remains running, but the batch and request never finish. With this change, the batch finishes as failed and its requests finish with an error; the build/path stays Cancelled.

Test Plan

  • TestRun_ProviderCancellationExhaustsHead checks that the last cancelled path stays Cancelled while the batch is marked failed before its requests are concluded.
  • TestHasNoViableFuture and TestDecide check that unfinished dependencies, other viable work, and valid passed results are not treated as exhausted.

Focused speculation, sticky-allocator, and build-signal tests passed.

Revert Plan

Revert this PR. Do not reverse terminal outcomes or rewrite Cancelled facts; older consumers may strand heads again.

Issues

@sbalabanov

Copy link
Copy Markdown
Contributor Author

[independent-review]

Reviewed 71a96b195a206c41863d676c11d5f65331f79ae2 independently. Two changes would make this smaller and safer:

  1. Fix termination during queue initialization — tool/submitqueue/recovercancelledpaths/main.go:89,119 intercepts SIGINT/SIGTERM, but NewQueue blocks in context-free DB.Ping(). A TCP peer that accepts without completing the MySQL handshake leaves the command running after both signals; this was independently reproduced. The simplest fix is to retain default signal termination for this one-shot command rather than add a broader initialization abstraction. Stable recovery IDs already make interrupted retries safe. Confidence: 95%. Verify with the stalled-handshake scenario and the existing tool test target.
  2. Reduce test-only branching — speculate/run_test.go:799 repeats shared path-set/batch CAS fault coverage. Keep one straight-line provider-cancellation regression that asserts Cancelled/attempt 1, persistence order, and conclusion; existing shared CAS tests already cover the failure mechanics. No additional tests are needed. Confidence: 90%.

Local mechanical checks/builds and affected test targets passed; all 12 assigned files were reviewed. Draft code-CI skips are not passing code validation.

Generated with the repository's .agents/skills/autoreview workflow; findings rechecked at the SHA above.

@sbalabanov

Copy link
Copy Markdown
Contributor Author

[agent-response SQC-013] Addressed both findings from #1036 (comment) in 555ed2e.

Removed signal interception from the one-shot CLI. With a TCP peer accepting the queue connection but sending no MySQL handshake, SIGINT exited with -2 in 0.001s and SIGTERM with -15 in 0.001s. Stable recovery-ID behavior is unchanged; the isolated MySQL replay still deduplicated two applies into one nudge and converged through ordinary consumers.

Reduced the provider-cancellation regression to one straight-line proof of Cancelled/attempt 1, persistence order, and conclusion; shared fault-window tests retain CAS/publish coverage. Both affected Bazel test targets passed. The Test Plan sentence was corrected accordingly.

@sbalabanov sbalabanov changed the title fix(speculation): exhaust cancelled viable paths and recover quiet queues fix(speculation): exhaust cancelled viable paths Nov 9, 2026
@sbalabanov
sbalabanov marked this pull request as ready for review November 9, 2026 23:05
@sbalabanov
sbalabanov requested review from a team and behinddwalls as code owners November 9, 2026 23:06
Count a provider-cancelled path as an exhausted attempt once dependencies
settle and every remaining viable path has failed or been cancelled. Keep
the recorded Cancelled outcome, usable alternatives and user cancellation
semantics unchanged.

Validation: uncached speculation, sticky allocator and build-signal unit
targets, orchestrator build, formatting, lint, tidy and Gazelle checks.
@cla-assistant

cla-assistant Bot commented Nov 13, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sbalabanov
sbalabanov merged commit f9b8dc2 into main Nov 13, 2026
22 of 23 checks passed
@sbalabanov
sbalabanov deleted the wt/sqc013 branch November 13, 2026 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants