mirror of
https://github.com/coder/coder.git
synced 2026-09-01 14:53:15 +08:00
J. Scott Miller
fd83c35531
`TestProvisionerd/CloseCancelsJob` closes the daemon inside the
provisioner parse/init callback, which cancels the in-flight
`AcquireJobWithCancel` RPC immediately after the job is delivered. dRPC
can report `context.Canceled` from the server-side `stream.Send` even
after the message was successfully sent, causing a spurious
`assert.NoError` failure.
The tolerance for this race already existed in the `acquireOne` test
helper (added in #17448 for coder/internal#584), but the inline acquire
handlers each carried their own bare `assert.NoError`, so
`CloseCancelsJob` never got it and flaked.
This extracts a single `assertAcquireNoError` helper that tolerates
`context.Canceled` on an acquire-stream `Send`/`Recv` (and fails on any
other error), and routes every `AcquireJobWithCancel` test handler and
`acquireOne` through it. The tolerance now has one definition and cannot
drift out of sync. If a job is never delivered, the affected tests still
fail waiting on their completion channels.
Fixes PLAT-172. Refs https://github.com/coder/internal/issues/1478
<details>
<summary>Investigation and validation</summary>
- The reported failure was `assert.NoError` at
`provisionerd_test.go:112` (commit 615be176) receiving `context
canceled`. The same dRPC behavior is documented in `retryable()` in
`provisionerd.go` and was already handled in
`acquireOne.acquireWithCancel`.
- Prior art: PR #17448 (merged 2025-04-17) added the identical tolerance
to `acquireOne` to fix coder/internal#584 (`flake:
TestProvisionerd/MaliciousTar`), which failed with the same signature.
That fix did not reach the inline handlers, which is the gap addressed
here.
- The natural timing race did not reproduce locally (~96,000 iterations,
including single-threaded and race-detector runs); it requires the
server `Send` goroutine to be descheduled past cancel propagation, which
only manifests under CI load.
- To validate deterministically, the handler was temporarily
instrumented to surface `context.Canceled` once the stream context
cancelled after a successful `Send`. With a bare `assert.NoError` this
reproduced the exact CI failure (`context canceled` at line 112); with
the tolerance it passed. The instrumentation was reverted.
- Scope note: `CloseCancelsJob` is the only handler with an observed CI
failure (it closes synchronously at acquisition). The other handlers
shut down only after job progress, so their first `Send` is not
realistically racing a cancel; applying the helper there is consistency
and drift-prevention, not a fix for an observed flake. This resolves
review finding CRF-1 in code.
</details>
> Generated by Coder Agents on behalf of @jscottmiller.