Skip to content

fix(test): end the CI unit-test hang by making Turbine buffer emissions - #21

Merged
fardavide merged 6 commits into
mainfrom
claude/pr-merge-checks-timing-fbn0p8
Aug 5, 2026
Merged

fix(test): end the CI unit-test hang by making Turbine buffer emissions#21
fardavide merged 6 commits into
mainfrom
claude/pr-merge-checks-timing-fbn0p8

Conversation

@fardavide

@fardavide fardavide commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Fixes the "test-process hang" that survived #17 and is currently keeping main red: the unit-test job dies ~90s into the run with exit code 1 and no test summary, on both PR and push runs.

Root cause

Turbine.expectInitial recursed on unchanged state whenever the current value equaled the expected one. That recursion has no suspension point, so the task never yields: it pins a cooperative-pool thread and allocates async frames without bound until a different value happens to arrive.

  • Locally (many cores) the view model's next emission lands in milliseconds on a spare pool thread and breaks the spin — tests pass, nobody notices.
  • On the 3–4-core CI runner all eight turbine tests spin concurrently, occupy the entire cooperative pool, and starve the very load() work that would produce the interrupting emissions. The process stalls, memory grows, and it dies without reporting — taking the unrelated suites down with it.

Fix

RealTurbine is rebuilt on an AsyncStream: every emission is buffered and consumed oldest-first, and waiting is a genuine suspension (iterator.next(isolation:)), so the pool is never touched. expectInitial keeps its contract — consume the initial value if the subscription caught it, or put the first value back for value() if the view model progressed before we subscribed. Verified against every call site that the FIFO semantics return exactly what the old latest-only code returned (each observed field is emitted once per load/send, with removeDuplicates upstream).

Two Swift 6 strict-concurrency requirements shaped the implementation: the protocol's associatedtype Value must be Sendable (values hop into the MainActor-isolated witness), and the stream iterator is advanced via a local copy with next(isolation: #isolation) (SE-0421), since a mutating async call on an isolated stored property — or a plain nonisolated next() — is rejected.

Also in this PR

  • TurbineTests (new, in the previously empty TestTests target): 4 regression tests; the first is the minimal reproduction that spins forever under the old code.
  • .github/rulesets/protect-main.json: importable branch ruleset requiring all four CI checks on main with an empty bypass list. Creating rulesets needs repo-admin, so apply it via Settings → Rules → New ruleset → Import a ruleset, or gh api repos/fardavide/Swiftly/rulesets --method POST --input .github/rulesets/protect-main.json.
  • ci.yml is net-unchanged: an aggregate "CI passed" gate job was added and later removed in favor of requiring the four job checks directly.

Verification

Full CI dispatched on this branch: run 30963134024 — all jobs green. The unit-test step completes in 82s with a full summary, where it previously died at ~90s.

Merging this turns main green again.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Myrncy7JAwkAjnfT8jm2h6

claude added 4 commits August 4, 2026 23:35
PR #17 could be merged while its checks were still running (and the
unit-test job went on to fail) because main has no required status
checks. Add a single always()-guarded job that fails unless every CI
job succeeded, so a branch ruleset only needs to require the one stable
check name "CI passed" instead of tracking all four job names.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Myrncy7JAwkAjnfT8jm2h6
The Turbine helper's expectInitial recursed on unchanged state with no
suspension point whenever the current value equaled the expected one:
the task never yields, pinning a cooperative-pool thread and allocating
async frames without bound. Locally the view model's next emission
interrupts the spin within milliseconds, but on the small CI runners
all turbine tests spin at once, occupy the whole pool, starve the very
work that would emit the next value, and the test process dies ~90s in
with no summary — the "test-process hang" that survived PR #17.

Rebuild the helper on an AsyncStream: every emission is buffered and
consumed oldest-first, waiting is a real suspension, and expectInitial
consumes the initial value when it is still there or puts the first
value back when the source progressed before subscription. Add
TurbineTests pinning both semantics; the first test is the minimal
reproduction of the spin.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Myrncy7JAwkAjnfT8jm2h6
Constrain the protocol's Value to Sendable: requirement calls hop into
the MainActor-isolated implementation, and the unconstrained
associatedtype made that hop ill-formed through any Turbine. Advance
the AsyncStream iterator via a local copy: a mutating async call on an
isolated stored property is rejected because the exclusive access
would span the suspension, and the iterator is only a handle to the
stream's shared storage, so a copy is equivalent.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Myrncy7JAwkAjnfT8jm2h6
The plain next() is nonisolated, so even on a local copy the call
sends the non-Sendable, main-actor-region iterator across an isolation
boundary and Swift 6 rejects it. next(isolation:) runs the advance
isolated to the caller, keeping the whole step on the main actor.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Myrncy7JAwkAjnfT8jm2h6
@semanticdiff-com

semanticdiff-com Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review changes with  SemanticDiff

Changed Files
File Status
  SwiftlyCore/Sources/Common/Test/Turbine.swift  11% smaller
  .github/rulesets/protect-main.json  0% smaller
  SwiftlyCore/Tests/Common/TestTests/TurbineTests.swift  0% smaller

claude added 2 commits August 5, 2026 04:46
The branch ruleset will require the four job checks directly instead
of a single aggregate name.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Myrncy7JAwkAjnfT8jm2h6
Creating a ruleset needs repository admin permission, so it cannot be
automated from CI or a scoped session. Keep the intended configuration
versioned here; apply it via Settings -> Rules -> New ruleset -> Import
a ruleset, or POST it to the rulesets API.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Myrncy7JAwkAjnfT8jm2h6
@fardavide fardavide changed the title ci: add a "CI passed" aggregate gate job for branch protection fix(test): end the CI unit-test hang by making Turbine buffer emissions Aug 5, 2026
@fardavide
fardavide enabled auto-merge (squash) August 5, 2026 04:54
@fardavide
fardavide merged commit 3d848b2 into main Aug 5, 2026
4 checks passed
@fardavide
fardavide deleted the claude/pr-merge-checks-timing-fbn0p8 branch August 5, 2026 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants