diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 740a30b..09374b0 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -88,12 +88,36 @@ jobs: tagName: ${{ github.ref_name }} releaseName: "AFKode ${{ github.ref_name }}" releaseBody: "See the commit history for changes. Installers below; the updater feeds from latest.json." - releaseDraft: false + # Stays a draft until every matrix leg has actually finished — see + # the `publish` job below. Without this, if e.g. only Windows and + # Linux finish before macOS notarization fails, users could already + # be downloading (and the updater already pointing at) a release + # that's missing the macOS build entirely. + releaseDraft: true prerelease: false args: ${{ matrix.args }} - winget: + # Only flips the release public once every matrix leg above succeeded — + # `needs.release.result` reflects the matrix job as a whole (success only + # if every leg succeeded), not just the leg that happens to finish last. + # If any leg failed, this job is skipped and the draft is left exactly as + # it is — assets from whichever platforms did succeed, still private — + # for a human to inspect rather than either quietly going out partial or + # getting deleted. + publish: needs: release + if: needs.release.result == 'success' + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - name: Make the release public + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: gh release edit "${{ github.ref_name }}" --draft=false --repo ${{ github.repository }} + + winget: + needs: publish runs-on: windows-latest # Requires the package to exist in winget-pkgs (initial PR merged); # a failure here must not affect the release itself. diff --git a/.github/workflows/version-bump.yml b/.github/workflows/version-bump.yml index b44e1e1..1f821df 100644 --- a/.github/workflows/version-bump.yml +++ b/.github/workflows/version-bump.yml @@ -1,7 +1,14 @@ name: Version Bump -# Runs after every push to main and decides, from Conventional Commits since -# the last tag, whether this merge deserves a new version: fix: -> patch, +# Runs after the "CI" workflow finishes on main, and only proceeds if it +# succeeded — never bump/tag/release a commit that doesn't even compile. +# (This used to run directly on `push: branches: [main]`, racing CI instead +# of waiting for it: a commit that failed to compile could still get bumped, +# tagged, and dispatched to release.yml, which would then fail after the +# tag was already public.) +# +# Once gated on a green CI run, it decides from Conventional Commits since +# the last tag whether this deserves a new version: fix: -> patch, # feat: -> minor, a `!` after the type/scope or a `BREAKING CHANGE:` footer # -> major. Anything else (chore/docs/refactor/test/ci/style, or a push with # no new commits since the last tag) bumps nothing and the job exits early. @@ -19,29 +26,49 @@ name: Version Bump # release.yml via the API (`gh workflow run`), which release.yml has to # opt into with `workflow_dispatch:`. on: - push: - branches: [main] + workflow_run: + workflows: ["CI"] + types: [completed] permissions: contents: write actions: write +# The bump commit's own push re-triggers "CI" (it runs on every push to +# main), which in turn re-triggers this workflow — the guard below turns +# that into a fast, harmless no-op (see it below), but two overlapping runs +# racing to push/tag at once would still be possible without this: e.g. a +# human push and the bump commit's CI run completing within moments of each +# other. Queue them instead of letting them interleave. +concurrency: + group: version-bump-main + cancel-in-progress: false + jobs: bump: runs-on: ubuntu-latest - # Without this, the bump commit's own push to main would re-trigger this - # same workflow. (In practice GITHUB_TOKEN pushes already don't trigger - # further runs — see the note above — but this keeps the job correct - # even if that ever changes, e.g. a maintainer force-pushing a manual - # bump commit under their own account.) - # The whole value must be quoted: an unquoted YAML scalar containing + # Only proceed for a green CI run on main — never for a failed/cancelled + # run, and never for CI runs from a PR branch (those also trigger "CI"). + # The commit-message check is the same anti-recursion guard as before, + # now reading workflow_run's own head_commit instead of push's. + # The whole if: value must be quoted: an unquoted YAML scalar containing # ": " (a colon followed by a space, as in 'chore: bump version to') # gets parsed as a nested mapping key instead of plain text, which is - # an invalid workflow file GitHub Actions won't even schedule a job for. - if: "${{ !startsWith(github.event.head_commit.message, 'chore: bump version to') }}" + # an invalid workflow file GitHub Actions won't even schedule a job for + # (this exact bug broke this workflow's first real run). + if: >- + ${{ + github.event.workflow_run.conclusion == 'success' && + github.event.workflow_run.head_branch == 'main' && + !startsWith(github.event.workflow_run.head_commit.message, 'chore: bump version to') + }} steps: - uses: actions/checkout@v4 with: + # Pin to the exact commit CI just validated, not whatever main's + # tip happens to be when this job starts — those should be the + # same commit, but pinning removes any doubt/race. + ref: ${{ github.event.workflow_run.head_sha }} fetch-depth: 0 - name: Determine bump level from commits since the last tag diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 8aaf24d..bae9b38 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -12,9 +12,11 @@ checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" name = "afkode" version = "0.8.20" dependencies = [ + "anyhow", "base64 0.22.1", "core-foundation", "core-graphics 0.24.0", + "libc", "portable-pty", "serde", "serde_json", diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index dcfc9d1..87dc764 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -32,6 +32,18 @@ portable-pty = "0.9" tiny_http = "0.12" base64 = "0.22" +[dev-dependencies] +# portable-pty's Child/MasterPty traits return anyhow::Error, but portable_pty +# only imports it privately (no public re-export) — needed by name to +# implement those traits for the fakes in lib.rs's test module. Already +# resolved transitively via portable-pty itself; this just makes it directly +# nameable from test code. +anyhow = "1" +# MasterPty::process_group_leader's #[cfg(unix)] signature returns +# Option — same situation as anyhow above, needed by name only +# to implement the trait for the unix-only cfg branch of the test fakes. +libc = "0.2" + [target.'cfg(windows)'.dependencies] windows-sys = { version = "0.59", features = [ "Win32_Foundation", diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 27f1a89..5062a19 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -112,6 +112,22 @@ struct PtySession { static SESSION_GEN: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); +/// True only if the session at `id` still exists and is this generation — +/// used while a reader thread is streaming output, to decide whether it's +/// still the owner of that id or should back off (see the reader thread in +/// `spawn_pty`). +fn session_is_current(sessions: &HashMap, id: &str, gen: u64) -> bool { + sessions.get(id).is_some_and(|s| s.gen == gen) +} + +/// True only if the session at `id` still exists but belongs to a *newer* +/// generation — a reused id (e.g. a webview reload) raced ahead of this +/// thread's own teardown. False (not true) if the entry is simply gone +/// already; that's a normal exit, not a supersession. +fn session_is_superseded(sessions: &HashMap, id: &str, gen: u64) -> bool { + sessions.get(id).is_some_and(|s| s.gen != gen) +} + /// `Child::wait()` blocks until the OS finishes tearing the process down — /// on Unix a killed/exited child stays a zombie in the process table until /// something calls `wait()` on it. Do that reaping on its own thread, never @@ -125,6 +141,30 @@ fn reap(mut child: Box) { }); } +/// Inserts `session` at `id`, reaping whatever session was already there. +/// Under normal operation nothing is there (the id is fresh) or the caller +/// already ran `kill_pty` first — but a reused id can race ahead of its own +/// teardown (e.g. a webview reload spawning a replacement before the old +/// session's reader thread notices), and `HashMap::insert` would otherwise +/// silently drop the previous `PtySession`, orphaning its child. +fn insert_session(state: &PtyState, id: String, session: PtySession) { + let previous = state.sessions.lock().unwrap().insert(id, session); + if let Some(mut prev) = previous { + let _ = prev.child.kill(); + reap(prev.child); + } +} + +/// The part of `kill_pty` that doesn't need a live `tauri::State` — kept +/// separate so it's callable from a unit test with a plain `PtyState`. +fn kill_pty_impl(state: &PtyState, id: &str) { + let session = state.sessions.lock().unwrap().remove(id); + if let Some(mut session) = session { + let _ = session.child.kill(); + reap(session.child); + } +} + #[derive(Default)] struct PtyState { sessions: Mutex>, @@ -859,7 +899,8 @@ async fn spawn_pty( let gen = SESSION_GEN.fetch_add(1, std::sync::atomic::Ordering::Relaxed); let job = child.process_id().and_then(job_for_child); - let previous = state.sessions.lock().unwrap().insert( + insert_session( + &state, id.clone(), PtySession { master: pair.master, @@ -869,14 +910,6 @@ async fn spawn_pty( _job: job, }, ); - // The id was already occupied — a reused tab id (e.g. a webview reload) - // raced ahead of that old session's own teardown. Its reader thread will - // see the `gen` mismatch and back off instead of reaping it, so kill and - // reap it here or it's an orphaned, never-waited-on process. - if let Some(mut prev) = previous { - let _ = prev.child.kill(); - reap(prev.child); - } // Reader thread: pump PTY output to the frontend as UTF-8 chunks. // 32 KiB reads coalesce bursty TUI redraws into fewer IPC events. @@ -890,14 +923,8 @@ async fn spawn_pty( // currently owns that id — duplicated/interleaved lines while the // user types in the new session. let is_current = || { - app.try_state::().is_some_and(|state| { - state - .sessions - .lock() - .unwrap() - .get(&id) - .is_some_and(|s| s.gen == gen) - }) + app.try_state::() + .is_some_and(|state| session_is_current(&state.sessions.lock().unwrap(), &id, gen)) }; loop { match reader.read(&mut chunk) { @@ -955,7 +982,7 @@ async fn spawn_pty( let mut superseded = false; let session = app.try_state::().and_then(|state| { let mut sessions = state.sessions.lock().unwrap(); - if sessions.get(&id).is_some_and(|s| s.gen != gen) { + if session_is_superseded(&sessions, &id, gen) { superseded = true; return None; } @@ -1022,18 +1049,165 @@ async fn resize_pty( #[tauri::command] async fn kill_pty(state: State<'_, PtyState>, id: String) -> Result<(), String> { - let session = { - let mut sessions = state.sessions.lock().unwrap(); - sessions.remove(&id) - }; - if let Some(mut session) = session { - let _ = session.child.kill(); - // Reap off-thread — see `reap`'s doc comment. - reap(session.child); - } + kill_pty_impl(&state, &id); Ok(()) } +#[cfg(test)] +mod pty_session_tests { + use super::*; + + // A fake Child/MasterPty pair — these tests only care about session + // *bookkeeping* (which entry ends up under an id, whether the old one + // was reaped), not a real OS process. A real PTY was tried initially, + // but even a single one, on this host, made `Drop`-ing its `master` + // (a synchronous ConPTY/conhost teardown wait) take anywhere from under + // a second to 60+ seconds — the same slow-but-synchronous-drop pattern + // that `kill_pty_impl`'s real callers rely on `reap()`'s background + // thread to keep off the async runtime for the *child* half; the + // `master`/`writer` half's drop isn't similarly deferred (a candidate + // for the next stability pass, out of scope here). Too flaky and slow + // for a unit test either way — a fake sidesteps host-dependent PTY + // teardown timing entirely. + #[derive(Debug)] + struct FakeChild; + impl portable_pty::ChildKiller for FakeChild { + fn kill(&mut self) -> std::io::Result<()> { + Ok(()) + } + fn clone_killer(&self) -> Box { + Box::new(FakeChild) + } + } + impl Child for FakeChild { + fn try_wait(&mut self) -> std::io::Result> { + Ok(Some(portable_pty::ExitStatus::with_exit_code(0))) + } + fn wait(&mut self) -> std::io::Result { + Ok(portable_pty::ExitStatus::with_exit_code(0)) + } + fn process_id(&self) -> Option { + None + } + #[cfg(windows)] + fn as_raw_handle(&self) -> Option { + None + } + } + + #[derive(Debug)] + struct FakeMasterPty; + impl MasterPty for FakeMasterPty { + fn resize(&self, _size: PtySize) -> Result<(), anyhow::Error> { + Ok(()) + } + fn get_size(&self) -> Result { + Ok(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + } + fn try_clone_reader(&self) -> Result, anyhow::Error> { + Ok(Box::new(std::io::empty())) + } + fn take_writer(&self) -> Result, anyhow::Error> { + Ok(Box::new(std::io::sink())) + } + #[cfg(unix)] + fn process_group_leader(&self) -> Option { + None + } + #[cfg(unix)] + fn as_raw_fd(&self) -> Option { + None + } + #[cfg(unix)] + fn tty_name(&self) -> Option { + None + } + } + + fn fake_session() -> PtySession { + PtySession { + master: Box::new(FakeMasterPty), + writer: std::sync::Arc::new(Mutex::new(Box::new(std::io::sink()) as Box)), + child: Box::new(FakeChild), + gen: SESSION_GEN.fetch_add(1, std::sync::atomic::Ordering::Relaxed), + _job: None, + } + } + + #[test] + fn kill_pty_on_unknown_id_is_a_noop() { + let state = PtyState::default(); + // Must not panic, and must leave the (already-empty) map alone. + kill_pty_impl(&state, "does-not-exist"); + assert!(state.sessions.lock().unwrap().is_empty()); + } + + #[test] + fn kill_pty_removes_and_kills_the_matching_session() { + let state = PtyState::default(); + let session = fake_session(); + state + .sessions + .lock() + .unwrap() + .insert("tab-1".into(), session); + + kill_pty_impl(&state, "tab-1"); + + assert!( + state.sessions.lock().unwrap().get("tab-1").is_none(), + "killed session must be removed from the map" + ); + } + + #[test] + fn inserting_over_a_reused_id_reaps_the_previous_child() { + let state = PtyState::default(); + let first = fake_session(); + let first_gen = first.gen; + insert_session(&state, "reused-id".into(), first); + + let second = fake_session(); + let second_gen = second.gen; + assert_ne!(first_gen, second_gen); + + // Replacing "reused-id" must not panic and must leave the *new* + // session (not the old one) behind under that id. + insert_session(&state, "reused-id".into(), second); + + let sessions = state.sessions.lock().unwrap(); + let current = sessions.get("reused-id").expect("id still present"); + assert_eq!(current.gen, second_gen, "the newer session must win"); + } + + #[test] + fn session_generation_checks_distinguish_current_superseded_and_missing() { + let mut sessions: HashMap = HashMap::new(); + sessions.insert("tab-1".into(), fake_session()); + let gen = sessions.get("tab-1").unwrap().gen; + + // Same generation: current, not superseded. + assert!(session_is_current(&sessions, "tab-1", gen)); + assert!(!session_is_superseded(&sessions, "tab-1", gen)); + + // A newer generation reused the id: no longer current, and flagged + // as superseded (a live reader thread should back off, not tear + // this down as if it were its own exit). + assert!(!session_is_current(&sessions, "tab-1", gen + 1)); + assert!(session_is_superseded(&sessions, "tab-1", gen + 1)); + + // The id was never (or no longer) present: neither current nor + // superseded — this is a plain, ordinary exit, not a race. + assert!(!session_is_current(&sessions, "missing", gen)); + assert!(!session_is_superseded(&sessions, "missing", gen)); + } +} + /// A pre-existing npm install with a custom `prefix` (nvm-windows, a manual /// `npm config set prefix`, ...) puts its global bin dir somewhere neither /// hardcoded fallback below covers. Shelling out to `npm` (a .cmd shim —