[IAmJSD]

Root cause

Issue #38 has two halves.

1. Purchase fan-out (original finding). TestExecuteMultiAccount_RunsAccountsInParallel (internal/purchase/execution_test.go) proved the parallel multi-account fan-out with a wall-clock bound. Each PurchaseCommitment mock slept perCallDelay = 300ms, and the whole executePurchase pipeline had to finish under parallelMaxBound = 500ms. That leaves 200ms of scheduling margin on whatever runner executes it. The three cited spots (old lines 853/925/957, now 1177/1249/1280) are that test's constants, its sleep and its assert.Less(elapsed, ...). The invariant is sound, but on a loaded runner or under -race a correct implementation can fail.
2. Scheduler fan-out (audit A16-011). TestFanOutPerAccount_RespectsParallelismLimit (internal/scheduler/scheduler_test.go) only asserted peak <= 3. A serial fanOutPerAccount peaks at 1 and passed every assertion. A regression to a serial loop, or a limit honored only on the high side, stayed green.

Fix

Both changes are test-only. Each test now asserts parallelism structurally with an atomic in-flight counter, a CAS-loop peak tracker and a channel barrier. No production code changed.

• Purchase (commit 74135be): each PurchaseCommitment call counts itself in flight and waits at a barrier that closes when both accounts' calls have arrived. The test asserts peak >= 2 and that no call timed out at the barrier. A parallel fan-out opens the barrier at once, so the test never sleeps on the passing path (~1ms, down from ~300ms). A serial loop leaves the first call alone at the barrier until barrierTimeout = 2s, the peak stays 1, and the test fails.

• Scheduler (commit 5c02695): with 20 accounts and CUDLY_MAX_ACCOUNT_PARALLELISM=3, the first 3 calls wait at a one-shot barrier that opens once 3 calls are in flight. The test asserts:

  • peak == 3, so the fan-out must reach the limit;
  • peak <= 3 (kept), so it never exceeds it;
  • zero barrier timeouts.

  The existing 5ms hold after the barrier stays, so a fan-out that ignored the limit still pushes the peak above 3. On a timeout the barrier opens for all later calls, so a serial fan-out fails after one 2s timeout rather than one per account. A passing run never waits on the timeout (~40ms).

Verification

All evidence is unit/mock-level, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null set because this machine's default AWS profile is invalid. Nothing touches real cloud APIs. Every mutation below was applied locally and reverted, and the final tree has no changes outside the two test files.

Purchase: fail when the code under test is sequential

go test ./internal/purchase/ -run TestExecuteMultiAccount_RunsAccountsInParallel -count=3:

Probe	Result
CUDLY_MAX_ACCOUNT_PARALLELISM=1	FAIL 3/3, 2.00s each
execution.RunForAccountsWithConcurrency(...) in executeMultiAccount replaced by a plain serial for loop	FAIL 3/3, 2.00s each

Both probes fail with the same messages:

Error: "1" is not greater than or equal to "2"
Messages: executeMultiAccount must run accounts in parallel: peak in-flight PurchaseCommitment calls was 1, want 2
Messages: a PurchaseCommitment call waited 2s at the barrier without the other account's call arriving: accounts ran serially

Scheduler: fail-before / fail-after

go test ./internal/scheduler/ -run '^TestFanOutPerAccount_RespectsParallelismLimit$' -count=3. Each probe mutates g.SetLimit(execution.ConcurrencyFromEnv()) in fanOutPerAccount:

Probe	Old test (main)	New test
Serial: SetLimit(min(ConcurrencyFromEnv(), 1))	PASS 3/3 (0.11s): the A16-011 gap	FAIL 3/3, ~2.1s: peak in-flight was 1, want 3; 1 account call(s) waited 2s at the barrier without 3 calls in flight
Below limit: SetLimit(min(ConcurrencyFromEnv(), 2))	(not run)	FAIL 3/3, ~2.06s: peak in-flight was 2, want 3 + barrier timeout
Unbounded: SetLimit(-1)	(not run)	FAIL 3/3, 0.01s: peak in-flight must not exceed ...; peak in-flight was 20, want 3
Env ignored: SetLimit(execution.DefaultMaxConcurrency)	(not run)	FAIL 3/3, 0.01s: peak in-flight was 20, want 3

Stability and speed on the real code

• go test -race -count=20 -run '^(TestFanOutPerAccount_RespectsParallelismLimit|TestExecuteMultiAccount_RunsAccountsInParallel)$' ./internal/scheduler/ ./internal/purchase/ passes: scheduler ok 2.308s, purchase ok 1.808s, 4.1s wall in total.
• Prebuilt -race binaries with -test.v, 20 runs each:
  • scheduler: 20/20 PASS at 0.04s per run (old: 0.11s);
  • purchase: 20/20 PASS at 0.00-0.01s per run (old: 0.30-0.31s).
• go test -count=1 ./internal/purchase/ ./internal/scheduler/ ./internal/mocks/ passes. internal/mocks holds the repo-wide test guards.

Note on reproducing the old purchase flake: the old purchase test did not flake locally under CPU contention. The case for that half rests on the 200ms margin argument in the issue, not on an observed local failure. Neither new test has a timing margin on its passing path. They only need the fan-out goroutines to reach the fake within 2s of each other.

Static checks

• go build ./... and go vet ./internal/purchase/ ./internal/scheduler/ are clean.
• golangci-lint v2.10.1 run --allow-parallel-runners on ./internal/purchase/... and ./internal/scheduler/... reports 0 issues. gofmt -l is clean.
• gocyclo -over 10 -ignore "_test\.go" on both packages is silent (test-only changes).
• scripts/gosec-hook.sh on both changed files is clean.
• git-secrets 1.3.0 ran in a fresh git init repo with an isolated HOME, after scripts/setup-git-secrets.sh. --scan passed on both changed files and on all tracked files, and scripts/test-git-secrets-allowlist.sh gave 109 PASS, 0 FAIL.

Out of scope

• internal/purchase/execute_detached_test.go:106 uses time.Since only as a lower bound (> 30s deadline headroom). Load can't make that fail, so it was left alone.

Labels

Could a maintainer please mirror the issue's triage labels onto this PR: impact/internal, type/chore, priority/p3, severity/low, urgency/eventually, effort/s.

Closes #38

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 5d4ab6b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 51cf98b2-7eaa-4278-8b29-95298fcef33e

Commits

Reviewing files that changed from the base of the PR and between 5d4ab6b and 5c02695.

Files selected for processing: 2

• internal/purchase/execution_test.go
• internal/scheduler/scheduler_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 5c02695

Scope: two test-only commits, 74135be test(purchase) and 5c02695 test(scheduler). No production code, migrations or exported APIs change. Base production code equals head, so the mutation probes below stand in for fail-before.

Findings (none blocking):

• [non-blocking] internal/scheduler/scheduler_test.go (the new function body) - The upper bound (peak <= limit) is caught only because of the 5 ms hold after the barrier opens.
  • It works: the limit+1 probe fails as expected.
  • Removing the sleep would quietly weaken the upper-bound check. The comment already says so, so no change is needed.
• [non-blocking] Both tests create a time.After(2s) timer per call that is never stopped. On go1.23+ these are garbage-collected, so the cost is negligible.

Attacked: mutation probes on a scratch copy, each restored and confirmed identical with cmp and with git show 5c026959:<file>. All 5 were caught:

1. g.SetLimit(1) at scheduler.go:610: FAILS, with peak 1 against a wanted 3 and one barrier timeout (2.13 s).
2. SetLimit(ConcurrencyFromEnv() - 1): FAILS, with peak 2 against a wanted 3 (2.07 s).
3. A serial loop at scheduler.go:620/656, with g.Go(func…{…}) changed to func…{…}(): FAILS, with peak 1 against a wanted 3. The failure is reported after one timeout, not 20, as the body says.
4. SetLimit(ConcurrencyFromEnv() + 1): FAILS. Peak 4 breaks both LessOrEqual and Equal, so the upper bound is real.
5. The purchase account fan-out limit at execution.go:234 set to 1: TestExecuteMultiAccount_RunsAccountsInParallel FAILS (peak 1 against a wanted 2).
   • This also shows that testify runs .Run outside its mutex. Otherwise the passing barrier would deadlock.

Hang analysis:

• Every barrier wait is bounded by time.After(2s).
• The scheduler's timeout path calls release() through sync.Once, so later calls cannot block.
• The purchase barrier has exactly 2 arrivals, matched by .Twice().
• The worst case is about 2 s per test.

Verified locally:

• Head:
  • go build ./... and go vet ./internal/scheduler/ ./internal/purchase/: OK.
  • gofmt -l on both test files: clean.
  • go test -count=1 ./internal/scheduler/ ./internal/purchase/: ok.
• Flakiness under CPU load, from 20 busy-loop processes, under the heavy lock:
  • -race -count=50 on each new test: ok.
  • -race -cpu=1 -count=50 on each new test: ok.
  • That is 200 loaded runs, with no failures and no races.
• Merged tree:
  • git merge-tree --write-tree against origin/main efc1373 is clean.
  • Main changed executeSinglePurchase to m.budgets.perRecTimeout() but did not touch the fan-out code.
  • With GOTOOLCHAIN=go1.26.10: build, vet, go test -count=1 on both packages, and -race -count=10 on both tests are all ok.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE 5c02695

Independent adversarial review of PR #1219 (closes #38) at 5c02695, base main. Test-only: internal/purchase/execution_test.go and internal/scheduler/scheduler_test.go.

Both halves of #38 are addressed.

• TEST-05: the wall-clock bound in TestExecuteMultiAccount_RunsAccountsInParallel is replaced by an atomic in-flight peak and a two-party barrier. The passing path no longer has a timing margin.
• A16-011: TestFanOutPerAccount_RespectsParallelismLimit now requires peak == limit, using a one-shot barrier that opens at limit arrivals. It keeps the <= limit upper bound and the 5ms hold, so an unbounded fan-out still pushes the peak past the limit.

Reasoning checked: testify runs RunFn outside the mock mutex, so the two PurchaseCommitment calls can really overlap. Under a correct SetLimit(3), the 3rd arrival happens while the first 2 are blocked, so the barrier opens deterministically. After a timeout the barrier opens for all later calls, so a serial fan-out costs one 2s wait, not twenty.

Findings: none.

Attacked:

• go vet is clean on both packages. go test -race -count=5 on both tests passes. go test ./internal/purchase/ ./internal/scheduler/ ./internal/mocks/ passes.
• Mutation probes (each reverted, file restored byte-identical):
  • Scheduler SetLimit(min(ConcurrencyFromEnv(), 1)) (serial): FAILS, peak 1 (killed).
  • Scheduler SetLimit(min(ConcurrencyFromEnv(), 2)) (below the limit): FAILS, peak 2 (killed).
  • Scheduler SetLimit(-1) (unbounded): FAILS, peak 20 (killed).
  • Purchase with CUDLY_MAX_ACCOUNT_PARALLELISM=1: TestExecuteMultiAccount_RunsAccountsInParallel FAILS after 2.01s (killed).

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 5c026959fa5a292591fbaa79af066c2b790f67a9

Blocking:

• none

Non-blocking:

• internal/scheduler/scheduler_test.go: the new assert.Equal(limit, peak) makes the existing assert.LessOrEqual(peak, limit) redundant. Both fail on an unbounded fan-out, so the over-limit case reports two messages. Keeping both reads fine; mentioning it only in case you want one message per failure mode.
• barrierTimeout = 2s is the only time budget left. On a correct fan-out it only needs the goroutines to reach the fake within 2s of each other, which is far more slack than the old 200ms margin. I ran it under load average ~60 with -race -count=10 and it was stable.

Attacked:

• Checked both barriers for deadlock and double-close. Purchase: close(allArrived) runs only when arrived hits exactly 2. Scheduler: sync.Once guards the close. A serial run therefore pays one timeout and then completes, and the deferred inflight.Add(-1) still runs after the 5ms hold, so the upper-bound check keeps its meaning.
• Fail-before on the scheduler (test-only PR, so I mutated production code instead): with g.SetLimit(min(execution.ConcurrencyFromEnv(), 1)) in fanOutPerAccount, the new test FAILs (peak in-flight was 1, want 3 plus a barrier timeout) while main's version of the test PASSes (0.20s). That reproduces the A16-011 gap and shows this PR closes it.
• Purchase: CUDLY_MAX_ACCOUNT_PARALLELISM=1 makes the new test FAIL with both messages after 2.01s.
• Commands: go build ./..., go vet (with and without -tags integration) on purchase and scheduler, go test -race -short -count=1 ./internal/purchase/ ./internal/scheduler/ (ok), and -race -count=10 on the two tests (ok). Mutations reverted, and the tree is clean.

Body-stale: none. I spot-checked the mutation table (serial probe) and the purchase parallelism=1 probe, and both reproduce as described.

_{Reviewer: rB3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 5c026959fa5a292591fbaa79af066c2b790f67a9 merged onto main (5aa3df5b), together with #1155, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1270, #1272, #1275, #1283, #1286, #1293, #1297, #1299, #1300, #1301, #1304, #1307, #1311, #1313, #1325 in that order (combined tree f3a3331f, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 5c026959 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 5c026959fa5a292591fbaa79af066c2b790f67a9.