test: gh attach image evidence (temp) - #2
Open
nwparker wants to merge 1 commit into
Open
Conversation
Owner
Author
Attachments
|
nwparker
added a commit
that referenced
this pull request
Jul 28, 2026
…persist a watermark past them (stablyai#10816) * fix(mobile): keep the reconnect watermark alive across the app's own teardown The catch-up added in stablyai#8690 could never run. app/index.tsx unsubscribes the notification stream on every non-'connected' state and builds a fresh subscription on reconnect, so the closure holding the ready-counter, the delivered watermark and the seen-set is destroyed exactly when a reconnect needs them. Every reconnect looked like a cold open, `reconnectReadyCount` was always 1, and notifications dispatched while the socket was down were never fetched. Move that state to a per-host module-scope session so it survives the teardown. Refs stablyai#8591 Co-authored-by: Orca <help@stably.ai> * fix(mobile): tag the notification watermark with a counter epoch so a desktop restart can't kill catch-up The desktop's notification `seq` is a per-process in-memory counter that starts at 0 on every launch. The mobile client's watermark is persisted in AsyncStorage and monotonic. After a desktop restart the two index different counters, so a client holding seq 57 meets a fresh counter at 2, `57 >= 2` cuts everything, and reconnect catch-up dies silently until the new process out-dispatches the old watermark — 57 notifications later. Users see nothing and get no error (stablyai#8591). Stamp every dispatched notification with an epoch identifying the counter lifetime, ride it on the `ready` frame and the getMissedSince response, and persist it beside the watermark. A watermark whose epoch doesn't match the live counter is void: the client resets to 0 and the desktop returns its retained buffer instead of nothing. The epoch param is optional on the wire in both directions, so a client or daemon that predates it degrades to today's seq-only cut rather than erroring. Also extracts the OS-permission helpers to notification-permissions.ts (re- exported, so no importer changes) to keep mobile-notifications.ts under its max-lines budget. Mutation-tested: 3 mutations applied to the epoch logic, 3 killed — including the storage-seed race guard, whose first mutant survived until the deferred-read test was added. * fix(mobile): make the notification watermark atomic and counter-scoped Round-1 review found four ways the epoch fix could still lose notifications. All four are addressed here. 1. Seen-set survived an epoch change. Seen-keys are seq-derived, and terminal bells carry no notificationId (they key on `seq:N` alone). After a restart the fresh counter re-issues low seqs, so a replayed post-restart bell was dropped as a duplicate of a bell from the previous counter. The dedup window belongs to one counter lifetime, so it is cleared on epoch change. 2. Legacy watermarks were trusted. Pre-upgrade installs stored a bare seq with no epoch. Adopting the first observed epoch as "nothing changed" left that unprovenanced seq cutting a counter it was never measured against — stablyai#8591 through the upgrade path. An epoch-less seq no longer survives adoption. 3. seq and epoch were separate storage keys. A process death between the two writes left epoch-B beside seq-57-from-A: a pair that looks internally valid on the next launch and is therefore trusted. They are now one JSON value, which cannot tear, with a read-only migration from the legacy key. 4. Sessions were never retired. They live at module scope so they survive the subscription teardown a reconnect performs, so host removal is the only thing that can drop them. Removal now retires the session and its watermark. Mutation-tested: 3 mutations, 3 killed. The first version of the bell test passed with the fix removed — it exercised the live path, which only adds to the seen-set; only the replay path consults it. Rewritten against the replay path, it fails with `expected 1 to be 2`: the literal lost notification. Mobile notifications + transport: 355 passed. Desktop replay: 11/11. * fix(mobile): catch up on the first connection after a cold open Catch-up hung off 'has this process connected before', which is false on the first ready of a fresh launch — exactly the post-upgrade / post-eviction case that loses everything between the stored watermark and the next live seq. Wait for the persisted read, then catch up whenever this device has delivered for the host before; a first-ever pairing still gets no replay. Co-authored-by: Orca <help@stably.ai> * fix(mobile): serialize live delivery behind the watermark seed, and key catch-up on the record Co-authored-by: Orca <help@stably.ai> * test(mobile): pin the two catch-up mechanisms mutation testing found unguarded Mutating each mechanism of the stablyai#8591 fix in turn showed two survived with the suite still green: the seed's epoch-provenance check, and the host session outliving the subscription teardown. Both are load-bearing, so pin them. - seen-set survives teardown: the desktop's retained buffer replays a notification already delivered live, and only the session-scoped seen-set stops a duplicate banner. - a seed resolving after a live epoch was adopted must not reinstate the dead watermark. Not reachable through subscribeToDesktopNotifications today ('ready' awaits the seed first), so it asserts on the exported pair and says so. Co-authored-by: Orca <help@stably.ai> * fix(mobile): serialize notification delivery per host so the watermark can't outrun what was shown Addresses two MAJOR findings from review of this branch. MAJOR #1 — the watermark could be persisted past a notification the user never saw. `deliverLive` advanced `lastDeliveredSeq` before awaiting the local show, and replay + live delivery ran concurrently, so a live seq 11 handled while catch-up was still showing seq 6 persisted 11. A process death before 7..10 were shown lost them permanently: the next launch asks the desktop for seq > 11. This predates the branch — `origin/main` advances the watermark at the same point — so it is a residual this fix closes, not a regression the branch introduced. It is fixed here because the branch is what makes the watermark load-bearing. Three changes: - the advance moves AFTER the show/dismiss await, so the watermark means "everything up to here reached the user" rather than "was dispatched" - a per-host `deliveryTail` promise chain (`enqueueHostDelivery`) serializes deliveries, so a monotonic advance is also an in-order one - the catch-up batch is ONE queue entry, not one per event. Awaiting per event returns to the event loop between replays and let a live event slot in between seq 6 and 7 — which is exactly the interleave being fixed. The RPC stays outside the queue: `sendRequest` waits up to 30s and holding the chain for that would stall live delivery on a slow link. MAJOR #2 — every delivery awaits the persisted read, so an AsyncStorage read that never settled disabled the host's notifications for the whole app lifetime, with no error and nothing to see. The seed is now bounded at 3s; a late seed still applies when it lands. Proceeding unseeded is strictly better: the watermark stays 0, so catch-up over-fetches and the seen-set de-duplicates. Serializing removed an overlap the duplicate-suppression relied on: `showLocalNotification` deduped two same-id events by observing the first still pending when the second arrived. With deliveries serialized the first completes first, so the second saw no pending state and scheduled a second banner for the same notification. The claim moves to enqueue time, where the overlap is still observable. Dismisses are deliberately not claimed — a dismiss for a shown id is what retires it. Evidence — each mechanism disabled individually against the unchanged suite: - batch-as-one-entry -> reverted to per-item enqueue: ordering test fails - watermark advance -> moved back before the await: ordering test fails - seed timeout -> removed: wedged-read test fails - live-path claim -> removed: concurrent-dedup test fails - replay-path claim -> removed: cross-path dedup test fails Each kills exactly one test, so no mechanism is unguarded and none is redundant. `mobile-notifications.test.ts`'s local `flushAsync` drained 10 microtask ticks. Deliveries are now several awaits deeper, so a fixed tick count under-drains; it yields to the macrotask queue instead. Verified with real timers that the behavior it asserts is unchanged — only the drain depth was wrong. Full mobile suite: 344 files, 2499 passed, 2 skipped. tsc clean, oxlint clean. --------- Co-authored-by: Orca <help@stably.ai>
nwparker
pushed a commit
that referenced
this pull request
Jul 28, 2026
* fix(runtime): surface desktop RPC startup failures * fix(runtime): isolate RPC failure telemetry * fix(runtime): satisfy the changed-code quality gate and kill vacuous dialog tests The `no-floating-promises` label span covers the whole `app.whenReady().then()` callback, so adding lines inside it made a long-standing finding overlap changed code. `void` is the linter's own suppression; no `.catch()` on purpose. The startup-failure tests were vacuous: mutation runs showed the wait-for-show deferral, the destroyed-window guard, the `closed` companion event, listener cleanup, the cause walk, the cycle guard, and the truncation bound could all be deleted with every test still green. The "not called yet" assertion ran before any microtask, so it passed either way. * test(runtime): de-brittle the desktop RPC-failure source assertions Anchoring the slice on the full destructure and matching the whole dialog call expression made an innocuous rename break the test with a cryptic 'expected -1'. Match the shape that is actually the contract instead. * test(runtime): repair the silently-unbounded desktop startup slice The desktopEnd anchor comment lost a word in 98b00d3, so indexOf returned -1 and slice(start, -1) covered index.ts to EOF. Moving the dialog call to a path that never runs at startup still passed. Anchor on code instead, and assert both bounds so a future reword fails loudly. * test(runtime): bound the attach anchors in the startup ordering slice Round 3 bounded the desktop pair but left attachStart/attachEnd unguarded in the same test: deleting the PTY startup barrier from attachMainWindowServices() and breaking the rateLimits.attach(window) end anchor still left the case green. * test(startup): bound the last two unguarded slice anchors in this file Rounds 3 and 4 fixed the desktop and attach pairs; two instances of the same class survived in the same file, both proven vacuous by mutation: - it stablyai#3 never bounded readyEnd. Renaming the `pairing:` payload key makes it -1, widening readyPayload from 372B to ~52KB. Moving the reconciliation status out of the serve-ready payload (its whole point) but leaving it later in index.ts then kept all 6 cases green. - it #2 bounded desktopWindowStart against reconciliationStart rather than serveEnd. An earlier `Promise.resolve(openMainWindow())` steals the anchor, collapsing desktopStartup to '' while every existing guard still passes, so its only assertion — a negative — succeeds against an empty string. Both mutants now fail. `src/main/ipc/pty-startup-barrier-ordering.test.ts:11` has the same latent shape; left alone as out of scope for this PR. * fix(runtime): keep walking the cause chain past an unmapped code getErrorCode returned the first code it found, so an outer wrapper carrying an unrecognised code masked a nested EACCES/ENOSPC and classified it unknown. Only a mapped code ends the walk now; every other input classifies as before. Unreachable today (writeSecureFile rethrows raw fs errors with .code intact), but the classifier's job is surviving whatever error shape reaches it. * fix(runtime): tell the user what to fix, not just to restart The dialog's only advice was "Restart Orca to try again", which is true for address_in_use and wrong for the rest: permissions, a full or read-only disk, and a missing data folder all survive a relaunch, so the user restarted, hit the same failure and had no next step. Route the error class we already compute into the copy so each cause names the thing the user has to change. Guidance and telemetry now derive from the same classifier, so they cannot drift apart. * fix(runtime): guide users through long RPC paths * fix(runtime): avoid false window listener warning * fix(runtime): guard destroyed window before web contents
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
1 participant
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.
Temporary PR to validate attaching image evidence without committing it to the diff.
Made with Orca 🐋