Cover the reconnection paths #625 leaves unpinned - #1
Draft
Conversation
Two things an agent gets wrong here without being told. The test command exists only in `.travis.yml`, which is stale and no longer runs anywhere — so the command has to be reconstructed, and the absence of CI makes a wrong reconstruction easy to miss. Integration is a SwiftPM `binaryTarget`, so a merge to `develop` reaches no consumer and pointing an app at a branch silently pairs that branch's source with an older release's binary. Reasoning about how a change ships depends on knowing this. `CLAUDE.md` defers to `AGENTS.md` so both toolchains read one file. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
The a8c CI toolkit's Buildkite plugin expects a `test` lane, and going through fastlane gives the test step a JUnit report to annotate failures from. The macOS lane only builds: the `Simperium OSX` scheme has an empty test action, so compiling is all that can guard the macOS-only sources. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Runs the unit tests and the macOS build on every pull request, tag, and push to `develop` or `main`. The pipeline needs the matching `simperium-ios` pipeline definition in the `buildkite-ci` repo to exist before it can run. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It pinned Xcode 12 and has not run since the project left Travis. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
--- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
--- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The constraint came over from a sibling repo's `Gemfile`; 2.238 is what the lock already resolves to. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The path was written out by hand in the Fastfile, the pipeline's `artifact_paths` and the annotation call, so three places had to agree. It now comes from `TEST_RESULTS_DIR` in `shared-pipeline-vars`; the lane keeps a default so a local `fastlane ios test` still works. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
--- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`:rubygems:` stays a name: it is a Buildkite custom emoji with no Unicode character behind it. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mac agents run the step's command in a VM, and `hostmgr generate buildkite-job` forwards only `BUILDKITE_*` variables into it, so the pipeline's `env` block set `TEST_RESULTS_DIR` on the host job and the script saw nothing — `unbound variable` under `set -u`. `artifact_paths` was unaffected: interpolation happens at upload, on the host, and bakes the value into `BUILDKITE_ARTIFACT_PATHS`. `shared-pipeline-vars` stays the single definition; the script now sources it directly, and the `env` entry is gone since it never crossed the boundary. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`shared-pipeline-vars` is only loaded at pipeline-upload time, so sourcing it from the job script was working around the wrong thing. Exporting the already-interpolated value in the step's `command` keeps the directory defined in one place without the job re-reading that file. Per @AliSoftware's review on #627. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
The CI script reads `report.junit` by path, so the report's type and name are part of the contract between the lane and the step rather than a fastlane default to inherit. Per Copilot's review on #627. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
Pipeline-upload interpolation bakes the path into the command, so the job does not need an env-var re-export or an explanation of why `env:` cannot carry it onto the mac queue. Suggested as an alternative to the export dance in @AliSoftware's review on #627. --- Generated with the help of Grok, https://x.ai Co-Authored-By: Grok 4.6 <noreply@x.ai>
The script is the one that reads the report, so it names the file and tells the lane what to write. --- Generated with the help of Grok, https://x.ai Co-Authored-By: Grok 4.6 <noreply@x.ai>
House style prefers `arg: arg` over the omitted-value shorthand. --- Generated with the help of Grok, https://x.ai Co-Authored-By: Grok 4.6 <noreply@x.ai>
`set -u` already makes `$1` fatal. --- Generated with the help of Grok, https://x.ai Co-Authored-By: Grok 4.6 <noreply@x.ai>
The script argument and `artifact_paths` already live in the same pipeline step, so an interpolated env var is leftover indirection. --- Generated with the help of Grok, https://x.ai Co-Authored-By: Grok 4.6 <noreply@x.ai>
A socket close that arrives before the connection finishes opening is treated as an intentional close: no retry is scheduled, the socket is nil'd, and networkManagersStarted still reads YES, so no automatic path ever rebuilds the connection. Sync silently stops until the app is relaunched or reachability drops completely. These tests document the expected behavior and currently fail: - testSocketClosedBeforeFinishingHandshakeSchedulesReconnection - testStartNetworkManagersRestartsBucketsEvenWhenAlreadyFlaggedAsStarted See SIMPL-75. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013N7ff8Y5ytsSLQ225boWwG
Fixes a permanent sync wedge (SIMPL-75): - SPWebSocketInterface treated a close arriving before webSocketDidOpen as intentional and never retried, even though intentional closes nil the delegate first and can't reach that handler. Any delivered close now schedules a retry, guarded by networkEnabled like didFailWithError. - startNetworkManagers early-returned whenever networkManagersStarted was set, but the flag only records that start ran once — not that a socket exists. After the close above, every automatic restart path (reachability regained, OSX wake) became a no-op, so sync stayed dead until an app relaunch or a full reachability drop. It now re-runs start: for each bucket, which is idempotent for healthy connections. - stop: cancelled every pending perform request on the interface rather than just its own reconnection retry; the cancel is now scoped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013N7ff8Y5ytsSLQ225boWwG
#625 changes three behaviours and pins two of them. The narrowing of `stop:`'s `cancelPreviousPerformRequestsWithTarget:` ships with no test that notices its removal: reverting that line to the old broad cancel leaves all seven of the PR's tests green. The socket teardown that runs after a close is only exercised through the retry it enables, and the retry assertions are `openAttempts > 0`, so a regression that scheduled a burst of reconnections would pass. `start:` returning early for an authenticated channel is the reason `startNetworkManagers` can now re-run for every bucket on each restart, so that early return is load-bearing and gets a test of its own. Each test was checked against a targeted mutation of `SPWebSocketInterface.m` and fails only under the mutation it is meant to catch. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mokagio
force-pushed
the
mokagio/simpl-75-websocket-reconnection-test-coverage
branch
from
September 2, 2026 05:20
fe75fa2 to
67c7fd1
Compare
The test file's `SPReconnectionDelay = 2.5` was not the reconnection delay — the delay is 2, and 2.5 was the test's wait budget. Two of the tests assert that no reconnection happened once the wait elapses, so a production delay raised past 2.5 would have left them passing without ever giving a retry the chance to fire. `SPWebSocketHeartbeatInterval` and `SPWebSocketTimeoutInterval` are the existing convention for naming these intervals. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`-webSocket:didCloseWithCode:` now treats every close that reaches it as unexpected and schedules a reconnection, so an intentional stop must not let one through. It did not, but only because `SPRWebSocket`'s `close` is asynchronous — the ordering here made the guarantee accidental, and contradicted the comment on the delegate method asserting it. `-[SPWebSocket handleTimeout:]` already detaches first. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`-dealloc` tore the network down, which reaches `-[SPWebSocketChannel stop]` and its main-thread `NSAssert`. Nothing guarantees which thread the last release lands on: `SPLogger` reads its weak delegate on its own queue, so a log message still in flight can hold the final reference to a `Simperium` and free it there. The teardown now takes the buckets as an argument so `-dealloc` can hand them to the main queue, which it could not do while the work needed `self`. This was already failing this branch's build on CI. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on Simperium#625. Adds test coverage for the reconnection behaviour that PR changes; no production code is touched.
Why
Reviewing Simperium#625 locally, three of its behaviours turned out not to be pinned by its own tests:
stop:cancel narrowing has no test. Reverting that line to the oldcancelPreviousPerformRequestsWithTarget:selfleaves all seven of Retry websocket connections that drop before completing the handshake Simperium/simperium-ios#625's tests green — includingtestStopCancelsPendingReconnection, which looks like it guards it.openAttempts > 0. A regression that scheduled a burst of reconnections rather than one would pass. Given the change's whole risk profile is "does this now reconnect too aggressively", that is the assertion worth tightening.networkEnabled == NObranch.Separately,
start:returning early for an authenticated channel is what makesstartNetworkManagerssafe to re-run per bucket on every restart — the load-bearing claim in that method's new comment. It had no test.Gotcha
CountingWebSocketInterfaceoverridesopenWebSocketwithout callingsuper, so the de-duplication inside the realopenWebSocketnever runs under test. That rules out a "repeated closes collapse to one reconnection" test at this level — it would be asserting on the double, not the code.testSingleCloseSchedulesExactlyOneReconnectioncovers what is reachable.How to test
114 tests, 0 failures locally (iOS 26.x simulator).
Each new test was also checked against a targeted mutation of
SPWebSocketInterface.m— broadening thestop:cancel, droppingstart:'s authenticated-channel early return, dropping the socket release indidCloseWithCode:, and double-scheduling the retry. Every mutation failed exactly the test meant to catch it, and nothing else.Opened by Claude Code (Opus 5) on behalf of @mokagio with approval.