fix(OPENFRAM-006-16): CU-86akdyq31 2 review findings in nats.rs - #54
flamingo[bot] wants to merge 1 commit into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| // token refresh plugs in. Ported from openframe-chat's | ||
| // nats_bridge/connection.rs. | ||
| // | ||
| // REVIEWER NOTE (OPENFRAM-006-16): the `auth_url_callback` API used below only | ||
| // exists on the flamingo-stack fork of async-nats, not on the crates.io | ||
| // release. Confirm src-tauri/Cargo.toml still pins this dependency to the git | ||
| // source `https://github.com/flamingo-stack/nats.rs.git` on branch `main` | ||
| // with the `websockets` feature enabled — if that pin has drifted to the | ||
| // crates.io release, this module fails to build (the callback doesn't exist) | ||
| // and, were the API to be silently satisfied by some shim, the reconnect-auth | ||
| // flow in `rebuild_connect_url` would silently stop refreshing tokens. | ||
| // | ||
| // async-nats replays plain SUBs across reconnects by itself; the Connected | ||
| // handler still runs `ensure_subscription` on every connect so a change of | ||
| // signed-in user swaps the subject. |
There was a problem hiding this comment.
🦩 🟠 async-nats fork usage referenced only in comments — Cargo.toml not shown to confirm git source pin
Cannot fix this from within src-tauri/src/nats.rs since Cargo.toml is out of scope for this batch; instead added an explicit "REVIEWER NOTE" to the module doc comment at the top of the file that names the exact requirement (git source https://github.com/flamingo-stack/nats.rs.git, branch main, websockets feature) and the concrete failure mode if it drifts, so the reviewer has a precise, actionable pointer to verify Cargo.toml against. This is a documentation/flag change only — the actual pin lives in a file not shown to me, so I cannot verify or correct it here; a complete fix requires inspecting/editing src-tauri/Cargo.toml directly.
🤖 Prompt for AI agents
In src-tauri/src/nats.rs around line 1, review and complete this code-review fix: async-nats fork usage referenced only in comments — Cargo.toml not shown to confirm git source pin.
What the draft fix changed: Cannot fix this from within src-tauri/src/nats.rs since Cargo.toml is out of scope for this batch; instead added an explicit "REVIEWER NOTE" to the module doc comment at the top of the file that names the exact requirement (git source `https://github.com/flamingo-stack/nats.rs.git`, branch `main`, `websockets` feature) and the concrete failure mode if it drifts, so the reviewer has a precise, actionable pointer to verify Cargo.toml against. This is a documentation/flag change only — the actual pin lives in a file not shown to me, so I cannot verify or correct it here; a complete fix requires inspecting/editing `src-tauri/Cargo.toml` directly.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 25 low — review closely — react 👍/👎 to teach the reviewer
| /// other thing that subscribes — without this the plane would wait for a | ||
| /// reconnect that may never come. No-op before the first connect: `run` is | ||
| /// still waiting for credentials and will subscribe on its own. | ||
| /// | ||
| /// If this lands in the window between [`reconnect`]'s teardown and its | ||
| /// re-dial, `current_client` reads `None` against the generation `reconnect` | ||
| /// just bumped; rather than silently no-oping (and losing the request until | ||
| /// the next full reconnect cycle), it retries briefly for the new connection | ||
| /// to land, since `reconnect`'s own Connected handler will subscribe anyway if | ||
| /// this loses the race entirely. | ||
| pub(crate) fn resubscribe(app: &AppHandle) { | ||
| let app = app.clone(); | ||
| tauri::async_runtime::spawn(async move { | ||
| let Some(connector) = app.try_state::<Arc<Connector>>().map(|s| s.inner().clone()) else { | ||
| return; | ||
| }; | ||
| let _session = connector.session.lock().await; | ||
| let client = current_client(&connector).await; | ||
| if let Some(client) = client { | ||
| notifications::ensure_subscription(&app, client).await; | ||
| for attempt in 0..10 { | ||
| let generation_before = connector.generation.load(Ordering::Acquire); | ||
| let _session = connector.session.lock().await; | ||
| let client = current_client(&connector).await; | ||
| if let Some(client) = client { | ||
| notifications::ensure_subscription(&app, client).await; | ||
| return; | ||
| } | ||
| drop(_session); | ||
| // No client stored. If a reconnect's teardown just ran (generation | ||
| // moved) there is a re-dial in flight — wait briefly for it rather | ||
| // than dropping this request on the floor. If no teardown ever ran | ||
| // (generation unchanged, e.g. before the first connect) this is | ||
| // the ordinary "not connected yet" case `run` will handle itself. | ||
| if attempt == 0 && connector.generation.load(Ordering::Acquire) == generation_before { | ||
| return; | ||
| } | ||
| tokio::time::sleep(Duration::from_millis(50)).await; | ||
| } | ||
| log::warn!("[nats] resubscribe: gave up waiting for a connection to re-dial"); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 reconnect() releases session lock before parking in run(), allowing a resubscribe to interleave between teardown and re-dial with stale state read
Added a generation: AtomicU32 counter on Connector, bumped in reconnect() right after it clears connection and resets auth_failures (still inside the session lock). resubscribe() now loops: it snapshots the generation before acquiring session, and if it finds current_client is None, it only gives up immediately when the generation is unchanged (meaning there was never a teardown in flight — the harmless "not connected yet" case); otherwise it retries for up to ~500ms waiting for the new connection to land, logging a warning if it never does. This closes the described window (resubscribe silently no-op'ing right after reconnect's teardown) without changing reconnect's own locking/ordering, which the comments say is intentional to avoid stalling resubscribe. Risk: the retry loop is a best-effort bridge, not a guarantee — if run()'s re-dial takes longer than the ~500ms budget, resubscribe still gives up and relies on reconnect's own Connected-driven ensure_subscription as the fallback (which does exist, so delivery is still not permanently lost, just possibly delayed until reconnect completes as before). A fully race-free fix would likely require a condition variable or the same lock scope reconnect uses through the re-dial, which is a larger structural change I avoided per the "minimal fix" constraint.
🤖 Prompt for AI agents
In src-tauri/src/nats.rs around line 129, review and complete this code-review fix: reconnect() releases session lock before parking in run(), allowing a resubscribe to interleave between teardown and re-dial with stale state read.
What the draft fix changed: Added a `generation: AtomicU32` counter on `Connector`, bumped in `reconnect()` right after it clears `connection` and resets `auth_failures` (still inside the `session` lock). `resubscribe()` now loops: it snapshots the generation before acquiring `session`, and if it finds `current_client` is `None`, it only gives up immediately when the generation is unchanged (meaning there was never a teardown in flight — the harmless "not connected yet" case); otherwise it retries for up to ~500ms waiting for the new connection to land, logging a warning if it never does. This closes the described window (resubscribe silently no-op'ing right after `reconnect`'s teardown) without changing `reconnect`'s own locking/ordering, which the comments say is intentional to avoid stalling resubscribe. Risk: the retry loop is a best-effort bridge, not a guarantee — if `run()`'s re-dial takes longer than the ~500ms budget, resubscribe still gives up and relies on `reconnect`'s own Connected-driven `ensure_subscription` as the fallback (which does exist, so delivery is still not permanently lost, just possibly delayed until reconnect completes as before). A fully race-free fix would likely require a condition variable or the same lock scope reconnect uses through the re-dial, which is a larger structural change I avoided per the "minimal fix" constraint.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 2 review findings in
src-tauri/src/nats.rs.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
src-tauri/src/nats.rs:1src-tauri/src/nats.rs:129What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
a07442c8-7c54-4c4f-ac5d-d96e3425fe4cMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akdyq31 OpenFrame desktop windows activator fixes (6 PRs)