From e877b5f2e81b1741aee90e843c8cccc22f9eef24 Mon Sep 17 00:00:00 2001 From: Alexander Petric Date: Sat, 12 Sep 2026 02:55:35 -0400 Subject: [PATCH] fix: clear a stale git auto-pull failure and show the status time (#11100) * fix: show when the last git auto-pull status was recorded * chore: bump ee-repo-ref for the auto-pull status fix * fix: show the git auto-pull status age with TimeAgo instead of a year-less date * test: pin that a stale auto-pull recovery cannot overwrite a newer state * chore: bump ee-repo-ref for the conditional auto-pull recovery * fix: keep TimeAgo counting past the first hour in noSeconds mode * chore: bump ee-repo-ref for the clear_auto_pull_failure contract note * fix: guard TimeAgo's boundary scheduler against invalid dates and pin same-head newer failures * chore: bump ee-repo-ref for the timestamp-guarded auto-pull recovery * test: cover a same-second newer failure surviving a stale auto-pull recovery * chore: bump ee-repo-ref for the whole-failure recovery match * test: name the recovery helper after its input, not its staleness * chore: update ee-repo-ref to c6df9fdd9826efb40d3586a9f97d17dee98ac6ef This commit updates the EE repository reference after PR #793 was merged in windmill-ee-private. Previous ee-repo-ref: 6aff80b80cae4944a4a78a6b9244019bc37f368b New ee-repo-ref: c6df9fdd9826efb40d3586a9f97d17dee98ac6ef Automated by sync-ee-ref workflow. --------- Co-authored-by: Ruben Fiszel Co-authored-by: windmill-internal-app[bot] --- backend/ee-repo-ref.txt | 2 +- .../fixtures/git_sync_autopull_recovery.sql | 10 ++ backend/tests/git_sync_autopull_recovery.rs | 143 ++++++++++++++++++ backend/windmill-git-sync/src/lib.rs | 8 +- frontend/src/lib/components/TimeAgo.svelte | 22 +-- .../git_sync/GitSyncRepositoryCard.svelte | 29 +++- 6 files changed, 196 insertions(+), 18 deletions(-) create mode 100644 backend/tests/fixtures/git_sync_autopull_recovery.sql create mode 100644 backend/tests/git_sync_autopull_recovery.rs diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index a9c3f7e1be..1587e30b21 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -c9b043f2860fdae150c8c4bf03f3ec98b7f300e5 +c6df9fdd9826efb40d3586a9f97d17dee98ac6ef diff --git a/backend/tests/fixtures/git_sync_autopull_recovery.sql b/backend/tests/fixtures/git_sync_autopull_recovery.sql new file mode 100644 index 0000000000..6392da50ab --- /dev/null +++ b/backend/tests/fixtures/git_sync_autopull_recovery.sql @@ -0,0 +1,10 @@ +-- A workspace whose auto-pulled repository last recorded a head-check failure while +-- already synced to head "aaa": the state a recovery write is decided on. + +INSERT INTO workspace (id, name, owner) VALUES ('ap-ws', 'ap-ws', 'test-user'); + +INSERT INTO workspace_settings (workspace_id, git_sync) VALUES + ('ap-ws', '{"repositories":[{"git_repo_resource_path":"$res:u/admin/repo", + "auto_pull":{"enabled":true,"mode":"polling", + "last_synced_sha":{"main":"aaa"}, + "last_pull_status":{"success":false,"at":1,"error":"head check failed: x"}}}]}'); diff --git a/backend/tests/git_sync_autopull_recovery.rs b/backend/tests/git_sync_autopull_recovery.rs new file mode 100644 index 0000000000..d3ba8b7c9f --- /dev/null +++ b/backend/tests/git_sync_autopull_recovery.rs @@ -0,0 +1,143 @@ +//! A recorded auto-pull failure is cleared once the tracked head is observed again +//! at the already-synced sha, and only then: the decision is taken on a snapshot, +//! so the write must re-check the stored row rather than overwrite it. +#![cfg(all(feature = "enterprise", feature = "private"))] + +use sqlx::{Pool, Postgres}; +use std::collections::HashMap; +use uuid::Uuid; +use windmill_common::workspaces::AutoPullStatus; +use windmill_git_sync::{clear_auto_pull_failure, persist_auto_pull_state}; + +const WS: &str = "ap-ws"; +const REPO: &str = "$res:u/admin/repo"; + +/// The failure the fixture records, as the poller would have read it. +fn fixture_failure() -> AutoPullStatus { + AutoPullStatus { + synced_sha: None, + at: 1, + job_id: None, + success: false, + error: Some("head check failed: x".to_string()), + } +} + +fn recovered(head: &str) -> AutoPullStatus { + AutoPullStatus { + synced_sha: Some(head.to_string()), + at: 2, + job_id: None, + success: true, + error: None, + } +} + +async fn stored_auto_pull(db: &Pool) -> anyhow::Result { + let git_sync: serde_json::Value = + sqlx::query_scalar("SELECT git_sync FROM workspace_settings WHERE workspace_id = $1") + .bind(WS) + .fetch_one(db) + .await?; + Ok(git_sync["repositories"][0]["auto_pull"].clone()) +} + +/// The recovery every test below runs, decided on the fixture's failure at head +/// "aaa": live in the first test, stale in the two that move the stored state +/// first. +async fn recovery_for_the_fixture_failure(db: &Pool) -> anyhow::Result<()> { + clear_auto_pull_failure( + db, + WS, + REPO, + "main", + "aaa", + &fixture_failure(), + &recovered("aaa"), + ) + .await?; + Ok(()) +} + +#[sqlx::test(fixtures("git_sync_autopull_recovery"))] +async fn recovery_clears_the_failure_at_the_synced_head(db: Pool) -> anyhow::Result<()> { + recovery_for_the_fixture_failure(&db).await?; + + let auto_pull = stored_auto_pull(&db).await?; + assert_eq!(auto_pull["last_pull_status"]["success"], true); + assert!(auto_pull["last_pull_status"].get("error").is_none()); + assert_eq!(auto_pull["last_pull_status"]["synced_sha"], "aaa"); + assert_eq!( + auto_pull["last_synced_sha"]["main"], "aaa", + "the sha map is not part of a recovery write" + ); + Ok(()) +} + +/// Between the poller observing head "aaa" unchanged and its recovery write, a +/// webhook may have enqueued newer head "bbb". The stale recovery must leave that +/// optimistic state (sha, success, job id) in place; the job's completion hook +/// relies on it, and rolling the sha back would re-enqueue "bbb" on the next tick. +#[sqlx::test(fixtures("git_sync_autopull_recovery"))] +async fn stale_recovery_leaves_a_newer_state_alone(db: Pool) -> anyhow::Result<()> { + let job_id = Uuid::new_v4(); + let advanced = AutoPullStatus { + synced_sha: Some("bbb".to_string()), + at: 3, + job_id: Some(job_id), + success: true, + error: None, + }; + persist_auto_pull_state( + &db, + WS, + REPO, + &HashMap::from([("main".to_string(), "bbb".to_string())]), + &advanced, + ) + .await?; + + recovery_for_the_fixture_failure(&db).await?; + + let auto_pull = stored_auto_pull(&db).await?; + assert_eq!(auto_pull["last_synced_sha"]["main"], "bbb"); + assert_eq!(auto_pull["last_pull_status"]["synced_sha"], "bbb"); + assert_eq!(auto_pull["last_pull_status"]["job_id"], job_id.to_string()); + assert_eq!(auto_pull["last_pull_status"]["at"], 3); + Ok(()) +} + +/// The head can stay at "aaa" while a newer failure is recorded (a later head +/// check, a pull job that failed). A recovery decided on the older failure must +/// not paper over the newer one, whether it differs by timestamp or, within the +/// same second, only by its error. +#[sqlx::test(fixtures("git_sync_autopull_recovery"))] +async fn stale_recovery_keeps_a_newer_failure_at_the_same_head( + db: Pool, +) -> anyhow::Result<()> { + let same_sha = HashMap::from([("main".to_string(), "aaa".to_string())]); + for newer in [ + AutoPullStatus { + at: 5, + error: Some("head check failed: later".to_string()), + ..fixture_failure() + }, + AutoPullStatus { + error: Some("head check failed: same second".to_string()), + ..fixture_failure() + }, + ] { + persist_auto_pull_state(&db, WS, REPO, &same_sha, &newer).await?; + + recovery_for_the_fixture_failure(&db).await?; + + let auto_pull = stored_auto_pull(&db).await?; + assert_eq!(auto_pull["last_pull_status"]["success"], false); + assert_eq!(auto_pull["last_pull_status"]["at"], newer.at); + assert_eq!( + auto_pull["last_pull_status"]["error"], + newer.error.as_deref().unwrap() + ); + } + Ok(()) +} diff --git a/backend/windmill-git-sync/src/lib.rs b/backend/windmill-git-sync/src/lib.rs index 2bfc7d2f77..ccb14e9028 100644 --- a/backend/windmill-git-sync/src/lib.rs +++ b/backend/windmill-git-sync/src/lib.rs @@ -14,10 +14,10 @@ pub mod git_sync_oss; #[cfg(feature = "private")] pub use git_sync_ee::{ - enqueue_git_pull_dry_run, enqueue_git_pull_job, handle_deployment_metadata, - handle_deployment_metadata_batch, handle_fork_branch_creation, persist_auto_pull_state, - reconcile_and_enqueue_pull, reconcile_fork_branch_pull, record_auto_pull_failure, - tally_deployed_object_changes, + clear_auto_pull_failure, enqueue_git_pull_dry_run, enqueue_git_pull_job, + handle_deployment_metadata, handle_deployment_metadata_batch, handle_fork_branch_creation, + persist_auto_pull_state, reconcile_and_enqueue_pull, reconcile_fork_branch_pull, + record_auto_pull_failure, tally_deployed_object_changes, }; #[cfg(not(feature = "private"))] diff --git a/frontend/src/lib/components/TimeAgo.svelte b/frontend/src/lib/components/TimeAgo.svelte index cf3f019432..6138cdb6da 100644 --- a/frontend/src/lib/components/TimeAgo.svelte +++ b/frontend/src/lib/components/TimeAgo.svelte @@ -27,18 +27,16 @@ let interval onMount(() => { - // compact schedules itself below; it needs no fixed rate. - if (compact) return + // compact and noSeconds schedule themselves below; they need no fixed rate. + if (compact || noSeconds) return - // Update every minute for noSeconds mode, every second otherwise. - const intervalMs = noSeconds ? 60000 : 1000 interval = setInterval(() => { computeDate() if (!isRecent) { clearInterval(interval) interval = undefined } - }, intervalMs) + }, 1000) // Add explicit cleanup return () => { @@ -49,21 +47,27 @@ // Waking on the boundary of the unit on screen, rather than at a fixed rate: `2h` only // changes on the hour, and a row that reads `5d` must not hold a 1s timer to find that // out. Re-armed when `date` changes, so an item edited to now leaves its day-long wait. + // noSeconds rides the same schedule: its relative forms keep counting hours and days, + // so a fixed-rate timer that stops once the date is an hour old would freeze them. $effect(() => { - if (!compact) return + if (!compact && !noSeconds) return const at = date + // An absent or unparsable date has no boundary to wait for: the delay below would + // be NaN, which setTimeout runs immediately, and the tick would re-arm itself in + // a tight loop. + if (Number.isNaN(new Date(at).getTime())) return let handle: ReturnType | undefined const tick = () => { computeDate() - handle = setTimeout(tick, compactDelayMs(at)) + handle = setTimeout(tick, nextUnitBoundaryMs(at)) } - handle = setTimeout(tick, compactDelayMs(at)) + handle = setTimeout(tick, nextUnitBoundaryMs(at)) return () => { handle && clearTimeout(handle) } }) - function compactDelayMs(dateString: string): number { + function nextUnitBoundaryMs(dateString: string): number { const secs = secondsAgo(new Date(dateString)) const left = secs < 60 diff --git a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte index a0082cf87a..515f57ea01 100644 --- a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte +++ b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte @@ -22,6 +22,7 @@ import type { GitSyncRepository } from './GitSyncContext.svelte' import GitSyncModeDisplay from './GitSyncModeDisplay.svelte' import Toggle from '$lib/components/Toggle.svelte' + import TimeAgo from '$lib/components/TimeAgo.svelte' import EEOnly from '$lib/components/EEOnly.svelte' import { GitSyncService, ResourceService, VariableService } from '$lib/gen' @@ -57,6 +58,10 @@ const gitSyncTestJob = $derived(idx !== null ? gitSyncContext.gitSyncTestJobs?.[idx] : null) let confirmingDelete = $state(false) + function pullStatusDate(status: { at: number }): string { + return new Date(status.at * 1000).toISOString() + } + // Enable/disable automatic repo → workspace pulls, managing the optional // auto_pull object without binding into a possibly-undefined value. function setAutoPullEnabled(enabled: boolean) { @@ -919,10 +924,18 @@ {#if repo.auto_pull.last_pull_status.success} Last synced{repo.auto_pull.last_pull_status.synced_sha ? ` to ${repo.auto_pull.last_pull_status.synced_sha.slice(0, 7)}` - : ''}. + : ''} + . {:else} - Last sync failed{repo.auto_pull.last_pull_status.error + Last sync failed + {repo.auto_pull.last_pull_status.error ? `: ${repo.auto_pull.last_pull_status.error}` : ''}. @@ -1000,13 +1013,21 @@ {#if repo.auto_pull.last_pull_status.success} Last synced{repo.auto_pull.last_pull_status.synced_sha ? ` to ${repo.auto_pull.last_pull_status.synced_sha.slice(0, 7)}` - : ''}. + : ''} + . {viaWebhook ? ' Syncing instantly via webhook.' : ' Checking the tracked branch about every minute.'} {:else} - Last sync failed{repo.auto_pull.last_pull_status.error + Last sync failed + {repo.auto_pull.last_pull_status.error ? `: ${repo.auto_pull.last_pull_status.error}` : ''}.