devantler-tech / devantler-tech/ksail
TestApplierCancelsDiscoveryBeforePatch races a 503 against the cancellation it asserts on
- Dominant language
- Go
- Stars
- 165
- Forks
- 12
- Avg merge
- 5h 41m
- Merged PRs (30d)
- 337
Description
> 🤖 Generated by the Agentic Engineer
## Evidence
`TestApplierCancelsDiscoveryBeforePatch` (`pkg/svc/ephemeral/applier_test.go`) failed on an unrelated PR's CI run ([job 102014656923](https://github.com/devantler-tech/ksail/actions/runs/34211783243/job/102014656923), 2026-09-08T09:57:58Z) while `main` was green and the PR touched **zero** files under `pkg/svc/ephemeral`:
```
--- FAIL: TestApplierCancelsDiscoveryBeforePatch (0.00s)
Error: Target error should be in err chain:
expected: "context canceled"
in chain: "resolve ConfigMap//settings: discover v1: the server is currently unable to handle the request"
```
## Root cause
The test's HTTP handler does two things at once, then the test asserts on only one of the two outcomes they can produce:
```go
cancel()
writer.WriteHeader(http.StatusServiceUnavailable)
...
require.ErrorIs(t, err, context.Canceled)
```
Cancelling the context **and** returning a 503 makes two different errors reachable from one code path. Whichever the discovery client notices first wins the race: the cancellation surfaces `context.Canceled`, and the 503 surfaces `the server is currently unable to handle the request`. Both are correct behaviour of the code under test, so the assertion is over-specified relative to what the scenario actually guarantees.
## Impact
A non-deterministic required check on an unrelated PR. It costs a diagnosis and a re-run each time it lands, and — worse — it trains readers to treat a red `🧪 Test` on this package as noise, which is exactly how a real regression here would get waved through.
## Expected behaviour
The test's real invariant is in its name and in its handler guard: **no PATCH is issued once discovery has been cancelled** (`t.Error("PATCH after discovery cancellation")`). That assertion is already deterministic. The error assertion should be made deterministic too, rather than dropped — weakening it to `require.Error` would stop pinning *why* the apply stopped.
## Acceptance criteria
- [ ] The scenario admits exactly one error source, so `context.Canceled` is the only reachable failure — e.g. the handler cancels and then serves a **valid** discovery response, leaving cancellation as the sole cause rather than racing a 503 against it.
- [ ] The `PATCH after discovery cancellation` guard is retained unchanged.
- [ ] `require.ErrorIs(err, context.Canceled)` is retained (not relaxed to `require.Error`).
- [ ] Test passes under `go test -race -count=50 ./pkg/svc/ephemeral/` with no failures.
Rough size: S.
Contributor guide
Research direction
Open pkg/svc/ephemeral/applier_test.go and find TestApplierCancelsDiscoveryBeforePatch. Start by reading its HTTP handler and cancellation flow; make the scenario expose only context.Canceled while retaining the PATCH-after-cancellation guard. Run go test -race -count=50 ./pkg/svc/ephemeral/ and confirm it passes without failures.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- testing
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 88/100