Repository navigation
progress: tell a cancel that was never sent from one that may have been - #158
Conversation
CancelBuild returned the caller's bare context error when the caller gave up before the signal went out, including while it waited for a progress poll or an overlapping cancel to release the reserved session. A context error that comes back after the signal was sent looks the same, so a consumer had to treat "never sent" as "may have reached the build". Return ErrCancelNotDispatched, wrapped together with the context error, only when nothing was sent; errors after dispatch are unchanged.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
🤖 Review 1/2 (correctness) of 228b7de: 0 blocking, 3 non-blocking. Inspected the full diff against the merge base ca9a8f3 ( The change is correct. BlockingNone. Non-blocking1. The wait for the session ignores the caller's context, so the sentinel arrives only after the wait ends (progress.go#L321-L322). Test case: CancelBuild returns promptly once the caller's deadline passes while a poll holds the sessionpackage progress_test
import (
"context"
"sync"
"testing"
"time"
"github.com/stretchr/testify/assert"
)
func TestCancelBuildReturnsWhenTheCallerGivesUpWhileWaiting(t *testing.T) {
blocking := newBlockingSession()
close(blocking.releaseSignal)
tracker := runningTrackerWithBuild(t, blocking.session())
var workers sync.WaitGroup
defer workers.Wait()
workers.Go(func() {
_, err := tracker.Progress(t.Context())
assert.NoError(t, err)
})
<-blocking.pollEntered
defer close(blocking.releasePoll)
callerCtx, cancel := context.WithTimeout(t.Context(), 50*time.Millisecond)
defer cancel()
outcome := make(chan error, 1)
workers.Go(func() { outcome <- tracker.CancelBuild(callerCtx) })
select {
case err := <-outcome:
t.Logf("returned while the poll still held the session: %v", err)
case <-time.After(time.Second):
t.Fatal("CancelBuild is still waiting 950ms after the caller's 50ms deadline passed")
}
}
2. The headline scenario, the context ending during the wait, is not pinned. The three subtests in TestCancelBuildReportsNotDispatchedWhenTheCallerGivesUpWaitingForTheSession end the caller's context (L491, L517, L542) without any synchronisation that Test case: the caller's context is read only after the session is heldpackage progress_test
import (
"context"
"sync"
"sync/atomic"
"testing"
"time"
"github.com/block/pg-sprite/pkg/progress"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// errProbeCtx reports a context that ends exactly when the poll releases the
// session, and records whether CancelBuild read it before that.
type errProbeCtx struct {
context.Context
released *atomic.Bool
readEarly atomic.Bool
}
func (c *errProbeCtx) Err() error {
if !c.released.Load() {
c.readEarly.Store(true)
return nil
}
return context.Canceled
}
func TestCancelBuildReadsTheCallersContextOnlyOnceItHoldsTheSession(t *testing.T) {
blocking := newBlockingSession()
close(blocking.releaseSignal)
tracker := runningTrackerWithBuild(t, blocking.session())
var workers sync.WaitGroup
defer workers.Wait()
workers.Go(func() {
_, err := tracker.Progress(t.Context())
assert.NoError(t, err)
})
<-blocking.pollEntered
var released atomic.Bool
caller := &errProbeCtx{Context: t.Context(), released: &released}
outcome := make(chan error, 1)
workers.Go(func() { outcome <- tracker.CancelBuild(caller) })
// Give CancelBuild every chance to read the context while the poll still
// holds the session.
time.Sleep(200 * time.Millisecond)
released.Store(true)
close(blocking.releasePoll)
err := awaitCancelOutcome(t, outcome)
require.ErrorIs(t, err, progress.ErrCancelNotDispatched)
assert.False(t, caller.readEarly.Load(), "the caller's context must be read only once the session is held")
assert.Equal(t, int32(0), blocking.signals.Load())
}Passes on 228b7de. With the 3. The doc comment over-claims in the other direction (progress.go#L309-L313, mirrored in docs/progress-report.md#L217-L220). "Any other error, a context error included, may have come back after the signal was sent" is not true of Verified
This review was generated by Claude Code (claude-sonnet-5-5). |
|
🤖 Review 2/2 (OSS adoption and integration ease) of 228b7de: 0 blocking, 4 non-blocking. Read the change the way someone embedding The change is a good fit for an importer. It is additive, it fits the existing BlockingNone. Non-blocking1. The consumer's decision is spread across three places and not stated as a table.
A short table like this in 2. Check order is part of the contract and is not documented. Because the error matches both the sentinel and the context error, a consumer that already has 3. 4. Naming and vocabulary are slightly off from the neighbours. The three existing sentinels read as states of the build ( What works well for adopters
This review was generated by Claude Code (claude-sonnet-5-5). |
aparajon
left a comment
aparajon
left a comment
There was a problem hiding this comment.
🤖 Stamping with comments: 0 blocking, 7 non-blocking (see the two review comments above).
This stamp was left by Claude Code (claude-sonnet-5-5).
…ntext The poll gate is a one-slot channel instead of a mutex, so CancelBuild can stop waiting for an in-flight poll or another cancel the instant its caller's context ends, and report ErrCancelNotDispatched then rather than when the holder lets go. A caller that gives up while the build ends during the wait is told ErrNoActiveBuild, as before. The observers and the handoff fences take the gate unconditionally, as they did the mutex. CancelBuild's doc comment and docs/progress-report.md carry the four-class outcome table (sent / not sent / nothing delivered / unknown) and name the check order a consumer needs: the sentinel before the context error. The earlier text claimed every error other than the sentinel might follow a sent signal, which was not true of ErrNoActiveBuild, ErrBuildNotRunning or ErrBuildUnobservable. docs/schemabot-integration.md's Cancel row points at the table. Tests pin the wait ending with the caller (the poll is never released until the outcome is in hand), the build ending during the wait, and the context being read only once the session is held, through a probe context that records the read.
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/158, follow-up commit The wait for the reserved session is now bounded by the caller's context (one-slot channel gate in place of the mutex), the context is read only once the session is held and that is pinned, and the consumer-facing contract is one four-class outcome table with its check order, linked from the integration doc.
Source: block/pg-sprite#158, review comments 6103553458 and 6103554024, review 5481260631 at head |
2 participants
Tracker.CancelBuildnow returnsErrCancelNotDispatched, wrapped together with the caller's context error, when the caller gave up before the cancel signal was sent — and its wait for the reserved session ends when the caller's context does, not when the session's holder lets go.Purpose
CancelBuildmay have to wait behind a progress poll or another cancel before it can send the signal.ErrCancelNotDispatchedis returned only when nothing was sent, so the consumer can tell the two cases apart; the wait itself is bounded by the caller's context, so the answer arrives when the caller gives up.Why
An orchestrator that calls
CancelBuildhas to decide whether the operator's cancel may have reached the build. If the signal may have gone out, it must read the build's latercancelled-externallyreturn as the operator's cancel. If nothing was sent, it must not, because an outside cancellation is then a failure to retry.Before this change it could not decide from the error.
CancelBuildtakes the reserved session only after any in-flight progress poll or overlapping cancel releases it. A caller whose deadline passes during that wait gets its bare context error back, and no signal was sent. A context error can also come back after the signal was sent, for example when the signal's own deadline expires. Both look likecontext.DeadlineExceeded, so the consumer has to assume the worse case and record a cancel that never happened.Example: an operator's cancel is behind a
Progresspoll, and the caller's deadline passes before the poll returns. The cancel sends nothing, but the orchestrator records it as possibly signalled. If someone else then cancels the backend, that cancellation is reported as the operator's.What
progress.ErrCancelNotDispatched.CancelBuildreturns it, wrapped withctx.Err(), from two places and no others: when the caller's context ends while it waits for the reserved session, and from the check that runs after the session is held and before the signal query.errors.Ismatches both the sentinel andcontext.Canceled/context.DeadlineExceeded.Progress,SetConcurrentBuild,StopConcurrentBuild,SetWorkSourceandStopWorkSourcetake the gate unconditionally, exactly as they took the mutex; onlyCancelBuildwaits under its caller's context. A caller that gives up while the build ends during the wait still getsErrNoActiveBuild, not a stale "not sent".CancelBuild's doc comment anddocs/progress-report.md(new### Cancelling a concurrent index buildanchor) carry the four-class outcome table — sent / not sent / nothing delivered (ErrNoActiveBuild,ErrBuildNotRunning,ErrBuildUnobservable) / unknown — and the check order: test for the sentinel before the context error, with a three-line consumer snippet.docs/schemabot-integration.md'sCancelrow points at the table.SAFETY.md'spkg/progressentry names the gate.ErrNoActiveBuild. The caller's context is read only once the session is held, pinned by a probe context that records the read and hooks the wait (no sleeps). A context error after the signal was sent does not carry the sentinel.Before / after, caller's deadline passes while a progress poll holds the session:
Outcome table
CancelBuildresultnilErrCancelNotDispatched(+ context error)ErrNoActiveBuild,ErrBuildNotRunning,ErrBuildUnobservableVerification
go test ./pkg/progress/ -race -count=3— green.TestCancelBuildReturnsWhenTheCallerGivesUpWhileWaiting,TestCancelBuildReportsNoActiveBuildWhenTheBuildEndsWhileTheCallerWaitsandTestCancelBuildReadsTheCallersContextOnlyOnceItHoldsTheSession; actx.Err()check hoisted above the wait fails the last two.SKIP_INTEGRATION=1 go test ./...— green.PG_VERSION=16 go test -race ./pkg/executor/on the concurrent-index-build tests that callCancelBuildagainst a real server — green.make lint— 0 issues.Veto items
ErrCancelNotDispatchedwhile the message and docs say "not sent". Renaming toErrCancelNotSentis cheap pre-1.0 and I will do it on request; "dispatched" was kept because the identifier describes the request leaving this process, and the message describes what the server saw.ErrNoActiveBuild, read under the state lock without the gate. The alternative — always the sentinel once the wait is abandoned — would tell the caller to retry against a build that is gone.🤖 Generated with Amp (Claude Opus 4.6)