Skip to content

fix(swift-ios): hydrate live environments independently - #10761

Closed
saphid wants to merge 12 commits into
pingdotgg:t3code/rebuild-mobile-app-swiftfrom
saphid:pr/swiftui-live-bootstrap-20260908
Closed

saphid wants to merge 12 commits into
pingdotgg:t3code/rebuild-mobile-app-swiftfrom
saphid:pr/swiftui-live-bootstrap-20260908

Conversation

@saphid

@saphid saphid commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Review this layer

GitHub shows this PR against t3code/rebuild-mobile-app-swift, so its diff includes every earlier PR in the chain. Review only this layer's change:

This layer's diff: +2558 / −464 lines across 9 files; app code +887 / −316, the rest is tests and docs.

Cold startup waits for aggregate metadata, while one passive environment can delay or stale the others. Start independent foreground hydration and passive live-shell workers, retain authoritative cached state across environment switches, and reject results from superseded socket/bootstrap generations. Preserve the current active-shell batching API and coalesced delta application.

Verification: all current-head GitHub checks pass, including contract fixtures and native tests. Focused bootstrap tests cover foreground restart after an interrupted cold bootstrap, archive ordering, and fresh hydration/archive loading after pairing into another environment. Skipped checks are not claimed as executed proof.

Delivery: stacked. This branch contains #10758, #10759; merge the chain in order (#10758 → #10759 → #10761 → #10762 → #10763 → #10764 → #10765 → #10766 → #14021 → #10767). Each PR's own change is its top commit(s).

Base: Theo’s t3code/rebuild-mobile-app-swift, 157476f1fb.

Verification scope: Native incremental-bootstrap and passive-live-shell suites passed on this head. Tests hold one peer shell/catalogue while healthy rows publish, transfer ownership on selection, cancel background workers, and reject obsolete bootstrap authority. These are protocol and ownership checks; no physical-device startup benchmark is claimed. The native CI job linked above contains the passing receipts. The tests exercise the protocol, state, or accessibility-value boundary changed here.

Independent cross-provider review was unavailable: claude auth status returned exit 1 with loggedIn: false. No Claude review or maintainer approval is claimed.

Refresh onto 157476f1fb (2026-09-27)

Rebased with the thread-sync chain onto the current t3code/rebuild-mobile-app-swift (157476f1fb). Head 9cd6d823df.

  • Focused tests at this layer, measured before the review fixes below (NativeMultiEnvironmentTests, NativeThreadCatchUpTests, NativeRetryIdentityTests, FeatureRootModelTests, iOS Simulator, per-test time limits): 84 XCTest + 100 Swift Testing passed (also NativeGitHubRoutingTests, NativePassiveThreadRefreshTests).
  • Full T3CodeTests at the chain top (68d92c2752): 343 XCTest cases passed (1 skipped). Swift Testing ran 892 tests; the only 2 failures are UsageModelsTests currency formatting, which fail identically on unmodified 157476f1fb.
  • Independent review: devin -p --sandbox --permission-mode smart --model swe-2-max over the whole chain diff, exit 0. No blocker, high or medium findings. A dead-state finding was fixed. A report that Stop state outlives a vanished thread was not changed, because stopSurvivesCloseReopenAndMissingReconnectSeed requires that behaviour.
  • Rebase changes: removed consumeFallbackShell and lastShellEventAt, which this PR left unused. Tests that read peer settings now wait for each peer's hydrated, configured snapshot, and the configuration test server answers dispatchCommand because configured peers keep their socket.

Review fixes (round 6: Macroscope and CI)

  • A transient environment catalog read failure ended the active client's configuration subscription, so later provider and settings updates were ignored. That publish is now skipped instead. No dedicated unit test: the file-backed environment store caches its document and has no failure seam.
  • CI caught upstream's testResponseStreamingSavesOnTheSelectedEnvironmentOnly failing on this chain: a passive peer's bootstrap catalogue probe could finish after the user saved a preference there and replace it with the older value. The probe result is now discarded when a config has already arrived.

Review fixes (round 8: Macroscope and Astra)

  • A peer whose credential was rejected before its catalogue loaded kept opening a probe client every 20 seconds. The catalogue probe now stops once the peer needs pairing; re-pairing starts a new worker. The probe's own error cannot show the rejection, because the socket ticket 401 surfaces as a timeout (Astra caught that in the first version). No dedicated test: showing that probes stop needs a way to wait for the absence of requests. GPT-6 Astra (codex exec -m gpt-6-astra -c model_reasoning_effort="xhigh" --sandbox read-only) confirmed the fix for paired peers and that re-pairing restarts the worker.

Follow-ups (minor, not blocking)

  • Astra noted that testResponseStreamingSavesOnTheSelectedEnvironmentOnly covers the probe race by timing only; a gated config response would make it deterministic. A test proving that config events keep publishing after a failed catalog read would need a failure seam in the environment store.
  • A T3 Connect (managed DPoP) peer whose shell read stays rejected is shown as disconnected rather than needing pairing, so its catalogue probe keeps retrying (Astra).
  • Removing or disabling a peer cancels its worker before the environment store write; if that write fails, the peer has no worker until the next aggregate refresh recreates it (Macroscope).
  • A successful HTTP fallback for a peer shows it as connected while its live stream is still down (Macroscope).
  • When createThreadAndSend returns a thread ID different from the one the app queued, the pending thread and its detail are replaced by the server's thread, as upstream already does. Delivered-message retention does not carry over to the new ID.

Model and harness: GPT-6 / Codex. Rebase onto 157476f1fb: Claude Opus 5.5 / Claude Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XXL 1,000+ changed lines (additions + deletions). labels Sep 8, 2026
@saphid
saphid marked this pull request as ready for review September 8, 2026 22:10
Comment thread apps/swift-ios/Features/Root/FeatureRootModel.swift
Comment thread apps/swift-ios/App/NativeFeatureClient.swift
@macroscopeapp

macroscopeapp Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This is an XXL production Swift runtime redesign spanning independent HTTP/WebSocket hydration, background lifecycle management, environment ownership, cache authority, and message-delivery state. The concurrency and state-machine changes have broad user-visible impact, with several concrete edge cases still documented as follow-ups.

Not approved because:

  • 5 blocking correctness issues found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@saphid

saphid commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@t3dotgg Could you review this SwiftUI reliability/performance fix? It will hydrate live environments independently. It does not change the visible interface. This is a dependent PR; prerequisites: #10732. Its diff includes those prerequisites and needs rebasing after they land.

A detached accepted-command refresh could finish after deleteThread and
publish the deleted thread again. Cancel it once the server accepts the
delete.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the pr/swiftui-live-bootstrap-20260908 branch from d0c88c3 to d9f456f Compare September 27, 2026 00:56
github-actions Bot and others added 3 commits September 27, 2026 18:36
…ng older state

A detail read that started before a newer send was accepted could publish
after it and hide the newer message until the next read. Skip publishing a
superseded read (its refresh loop reads again), and stop a cancelled shell
refresh from writing a shell read before the change.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ives

A Stop accepted during a send's detail read discarded that read without
requesting another, and an older detail could replace the transcript before
the accepted message reached it. Re-read detail after a discarded read, and
keep delivered messages until a server transcript includes them.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Do not retain a delivered message that a server transcript already
confirmed, keep only its display copy, and drop retained messages when
their thread details are removed or cleared.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the pr/swiftui-live-bootstrap-20260908 branch from d9f456f to f34fe39 Compare September 27, 2026 20:29

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium App/NativeFeatureClient.swift:4224

A transient failure from runtime.environments() terminates configurationTask, so later provider and settings events are no longer applied or published until polling restarts. Because the load occurs inside the for await body and is handled only by the outer catch, catch this per-event failure and continue consuming the subscription.

-                    let environments = try await self.runtime.environments()
+                    let environments: [Environment]
+                    do {
+                        environments = try await self.runtime.environments()
+                    } catch is CancellationError {
+                        return
+                    } catch {
+                        continue
+                    }
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/swift-ios/App/NativeFeatureClient.swift around line 4224:

A transient failure from `runtime.environments()` terminates `configurationTask`, so later provider and settings events are no longer applied or published until polling restarts. Because the load occurs inside the `for await` body and is handled only by the outer `catch`, catch this per-event failure and continue consuming the subscription.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #10761 (fix(swift-ios): keep applying config events after a catalog read fails): a failed catalog read now skips that publish and the subscription keeps running.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

Comment thread apps/swift-ios/Features/Root/FeatureRootModel.swift Outdated
Evicting a cached thread detail forgot its accepted messages, so a stale
reopen hid them. A restored outbox or a new task's local detail was also
recorded as the server transcript, so its accepted prompt was treated as
confirmed and disappeared on the next older read. Only server transcripts
now confirm delivery, and eviction keeps retained messages.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the pr/swiftui-live-bootstrap-20260908 branch from f34fe39 to b2403a0 Compare September 27, 2026 21:14
if serverMessageIDs[submission.threadID]?.contains(submission.identity.messageID) != true {
var message = queuedMessage(for: submission)
message.state = .complete
deliveredAwaitingDetail[submission.identity.messageID] = (submission.threadID, message)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Root/FeatureRootModel.swift:1870

On the creation path where createThreadAndSend returns a different thread.id, the accepted user message disappears from deliveredAwaitingDetail until a later server transcript includes it. completeQueuedSubmission stores it under the provisional submission.threadID, then drainOutbox calls removeThread(id: submission.threadID), whose forgetDeliveredMessages deletes that entry. Preserve the retained message across provisional-thread removal or associate it with the returned real thread ID.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/swift-ios/Features/Root/FeatureRootModel.swift around line 1870:

On the creation path where `createThreadAndSend` returns a different `thread.id`, the accepted user message disappears from `deliveredAwaitingDetail` until a later server transcript includes it. `completeQueuedSubmission` stores it under the provisional `submission.threadID`, then `drainOutbox` calls `removeThread(id: submission.threadID)`, whose `forgetDeliveredMessages` deletes that entry. Preserve the retained message across provisional-thread removal or associate it with the returned real thread ID.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing in this PR. When the server returns a different thread ID, the pending thread and its detail are replaced by the server's thread; upstream drops that pending detail the same way. Retention only covers stale reads of the same thread. Listed as a follow-up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

github-actions Bot and others added 4 commits September 28, 2026 07:26
A newer send can be delivered while an older one waits to retry. Delivered
copies were placed before every queued copy, which reversed them. Merge
both by send time.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A transient environment catalog read failure ended the active client's
configuration subscription, so later provider and settings updates were
ignored. Skip that publish instead.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e probe

A passive peer's catalogue probe started at bootstrap could finish after
the user saved a preference there and replace it with the older value.
Discard the probe when a config already arrived.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the pr/swiftui-live-bootstrap-20260908 branch from b2403a0 to e3e7ec1 Compare September 27, 2026 22:06

func removeEnvironment(id: String) async throws {
shellConnectionIDsByEnvironmentID[id] = nil
aggregateRefreshWorkers.removeValue(forKey: id)?.task.cancel()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium App/NativeFeatureClient.swift:673

removeEnvironment and setEnvironmentEnabled cancel the passive worker before their throwing runtime operations commit, so a failed runtime.remove, credential revocation, or runtime.setEnabled leaves an enabled environment without live shell and catalogue refreshes. Move worker cancellation until after the respective operation succeeds, and preserve or restart the worker when it fails.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/swift-ios/App/NativeFeatureClient.swift around line 673:

`removeEnvironment` and `setEnvironmentEnabled` cancel the passive worker before their throwing runtime operations commit, so a failed `runtime.remove`, credential revocation, or `runtime.setEnabled` leaves an enabled environment without live shell and catalogue refreshes. Move worker cancellation until after the respective operation succeeds, and preserve or restart the worker when it fails.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing in this PR: a failed environment-store write after cancelling the worker leaves the peer without a worker only until the next aggregate refresh, which recreates workers for any enabled peer missing one. Listed as a follow-up in the PR body.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

Comment on lines +4610 to +4614
owner.applyEnvironmentLoad(EnvironmentShellLoad(
environment: environment, client: client, shell: shell, config: nil
))
if membershipChanged { owner.rebuildEntityIndexes(saved) }
owner.publishAggregateSnapshot(saved, changedEnvironmentIDs: [environment.id])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium App/NativeFeatureClient.swift:4610

A successful HTTP fallback changes the environment state back to .connected, so after followShell marks the peer .reconnecting, the client reports live updates as connected while the stream is still down. Preserve the reconnecting state after applyEnvironmentLoad until followShell receives a new snapshot.

             owner.applyEnvironmentLoad(EnvironmentShellLoad(
                 environment: environment, client: client, shell: shell, config: nil
             ))
+            markStreamPaused(environment)
             if membershipChanged { owner.rebuildEntityIndexes(saved) }
             owner.publishAggregateSnapshot(saved, changedEnvironmentIDs: [environment.id])
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/swift-ios/App/NativeFeatureClient.swift around lines 4610-4614:

A successful HTTP fallback changes the environment state back to `.connected`, so after `followShell` marks the peer `.reconnecting`, the client reports live updates as connected while the stream is still down. Preserve the reconnecting state after `applyEnvironmentLoad` until `followShell` receives a new snapshot.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged: after a successful HTTP fallback the peer shows as connected while its stream is still reconnecting. The data is current; only the label overstates live updates. Listed as a follow-up in the PR body.

A peer whose credential was rejected before its catalogue loaded kept
opening a probe client every 20 seconds. The probe's own error cannot show
the rejection (the socket ticket 401 surfaces as a timeout), so stop once
the peer's shell read has marked it as needing pairing; re-pairing starts
a new worker.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
// A read begun before the stream lost completeness cannot
// repair that gap, even if the socket identity is unchanged.
state.authorityRevision &+= 1
markStreamPaused(environment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium App/NativeFeatureClient.swift:4477

A rejected passive shell is changed from .needsPairing back to .reconnecting, so the UI loses the re-pair prompt and requestRepair() immediately wakes refreshShell to retry invalid credentials. Gate this recovery path on the existing .needsPairing state and stop the subscription worker without overwriting it.

+                guard owner?.environmentConnectionStates[environment.id] != .needsPairing else { return }
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/swift-ios/App/NativeFeatureClient.swift around line 4477:

A rejected passive shell is changed from `.needsPairing` back to `.reconnecting`, so the UI loses the re-pair prompt and `requestRepair()` immediately wakes `refreshShell` to retry invalid credentials. Gate this recovery path on the existing `.needsPairing` state and stop the subscription worker without overwriting it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed further up the stack in #10764 (fix(swift-ios): share a peer's credential rejection with its live stream): the rejection is shared with the peer's stream, so stream closure keeps Needs pairing and does not wake the HTTP loop before the back-off (test livePeerRejectedCredentialStopsReconciling). The chain merges in order.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

@saphid

saphid commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

@t3dotgg this is ready for your review and merge.

Merge order: stacked chain, merge in order: #10758 → #10759 → #10761 → #10762 → #10763 → #10764 → #10765 → #10766 → #10767. #12655 (composer focus freeze) is independent and can merge at any time.

Evidence at head 9cd6d823df:

  • CI: all checks pass on this head.
  • Full T3CodeTests at the chain top (70258b6a6b, iOS Simulator): 343 XCTest cases passed (1 skipped). Swift Testing ran 892 tests; the only 2 failures are UsageModelsTests currency formatting, which fail identically on unmodified 157476f1fb.
  • Each review fix has a focused regression test that fails without the fix, except where the description says why not.
  • An earlier build of this chain (before the review rounds) fixed the stuck-send behaviour on a physical iPhone, as confirmed by the user. The current heads have not been re-verified on a device.

Independent review: No full-layer Astra review. Astra checked this layer's review fixes: the config-subscription and catalogue-probe fixes (round 6, minor test notes only) and the rejected-peer catalogue fix (the first version was rejected; the revision was confirmed for paired peers, and managed peers are a listed follow-up). The whole chain also had the SWE-2 Max review (exit 0, no blocker/high/medium).

Macroscope findings on the latest heads have been verified against the source: real ones are fixed, and the rest have a written reason and are listed under Follow-ups in the description.

Copy link
Copy Markdown
Member

Note

This comment is posted by Julius' dot

This rewrites foreground bootstrap, passive environment workers, and cache ownership. That substantial reliability change needs a maintainer-triaged issue or agreed scope under the prior approval rule. Neither this thread nor #10732 supplies that direction. Please establish the failure and intended hydration model with maintainers, then request reconsideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants