devantler-tech / devantler-tech/ksail

TestApplierCancelsDiscoveryBeforePatch races a 503 against the cancellation it asserts on

Open Beginner friendly
#6,961 3 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.