Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions crates/api/src/github_access/cached.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@ use super::types::{AccessError, AccessResult, GithubAccessChecker};
pub(super) const GITHUB_API_BASE_URL: &str = "https://api.github.com";

pub(super) const CACHE_TYPE_REPO_IDS: &str = "accessible_repo_ids";
pub(super) const CACHE_TTL_SECONDS: i64 = 600; // 10 minutes
pub(super) const STALE_MAX_SECONDS: i64 = 3600; // 1 hour
// Issue #105: relaxed cache lifetimes. The repositories table is reconciled
// independently by webhook + worker sync, so we can serve cached user→repo
// mappings for longer without risking long-term staleness.
pub(super) const CACHE_TTL_SECONDS: i64 = 3600; // 1 hour (valid)
pub(super) const STALE_MAX_SECONDS: i64 = 86_400; // 24 hours (stale-while-error)
pub(super) const SYNC_CACHE_TYPE: &str = "installation_repos_sync";
pub(super) const SYNC_TTL_SECONDS: i64 = 600; // 10 minutes
pub(super) const SYNC_TTL_SECONDS: i64 = 1800; // 30 minutes

/// Caching decorator that stores `list_accessible_repo_ids` results in PostgreSQL.
/// Falls back to stale cache on rate-limit errors (stale-while-error).
Expand Down Expand Up @@ -148,8 +151,11 @@ impl GithubAccessChecker for CachedGithubAccessChecker {
}
Ok(result)
}
Err(ref e @ AccessError::RateLimited) => {
// 5. On RateLimited only – try stale cache
Err(e) => {
// 5. On any error – try stale cache so a transient GitHub failure
// (rate limit, expired user token, upstream blip) does not break the
// repository list. The repositories table itself is reconciled by
// webhook + worker sync, so the cached id set is still meaningful.
let stale_duration = chrono::Duration::seconds(STALE_MAX_SECONDS);
if let Ok(Some(stale)) = boardflow_db::queries::github_api_cache::get_stale_cache(
&self.pool,
Expand All @@ -163,15 +169,11 @@ impl GithubAccessChecker for CachedGithubAccessChecker {
tracing::warn!(
user_id = %user_id,
error = %format!("{e:?}"),
"using stale cache for accessible_repo_ids due to rate limiting"
"using stale cache for accessible_repo_ids after GitHub error"
);
return Ok(Some(ids));
}
}
Err(AccessError::RateLimited)
}
Err(e) => {
// 6. Non-rate-limit errors (TokenExpired, Upstream) – propagate directly
Err(e)
}
}
Expand Down
26 changes: 26 additions & 0 deletions crates/api/src/routes/webhook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,19 @@ async fn handle_installation_event(pool: &PgPool, body: &[u8]) -> StatusCode {
}
};

if let Err(err) = boardflow_db::queries::github_installation_sync_state::upsert_webhook_seen(
pool,
event.installation.id,
)
.await
{
tracing::warn!(
error = %err,
installation_id = event.installation.id,
"failed to stamp installation webhook_seen_at"
);
}

match event.action.as_str() {
"created" => {
for repo in &event.repositories {
Expand Down Expand Up @@ -190,6 +203,19 @@ async fn handle_installation_repositories_event(pool: &PgPool, body: &[u8]) -> S
}
};

if let Err(err) = boardflow_db::queries::github_installation_sync_state::upsert_webhook_seen(
pool,
event.installation.id,
)
.await
{
tracing::warn!(
error = %err,
installation_id = event.installation.id,
"failed to stamp installation webhook_seen_at"
);
}

match event.action.as_str() {
"added" => {
for repo in &event.repositories_added {
Expand Down
92 changes: 83 additions & 9 deletions crates/api/tests/github_cache_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,10 +279,11 @@ async fn test_cleanup_expired_cache() {
};
let user_id = create_test_user_with_token(&pool, "gho_cache_test_8").await;

// Insert expired >1 hour ago (should be cleaned)
// Issue #105: cleanup preserves entries up to 24h past expiration so they
// remain available as stale-while-error fallbacks. Insert expired >24h ago.
sqlx::query(
"INSERT INTO github_api_cache (user_id, cache_type, value_json, expires_at, created_at, updated_at) \
VALUES ($1, 'cleanup_type', '[1]'::jsonb, NOW() - INTERVAL '2 hours', NOW(), NOW()) \
VALUES ($1, 'cleanup_type', '[1]'::jsonb, NOW() - INTERVAL '36 hours', NOW(), NOW()) \
ON CONFLICT (user_id, cache_type) DO UPDATE SET value_json = EXCLUDED.value_json, expires_at = EXCLUDED.expires_at",
)
.bind(user_id)
Expand All @@ -296,6 +297,40 @@ async fn test_cleanup_expired_cache() {
assert!(deleted >= 1);
}

#[tokio::test]
#[serial]
async fn test_cleanup_preserves_recent_stale_cache() {
let Some(pool) = setup_pool().await else {
return;
};
let user_id = create_test_user_with_token(&pool, "gho_cache_cleanup_preserve").await;

// Expired 2 hours ago — within the 24h stale window, must survive cleanup.
sqlx::query(
"INSERT INTO github_api_cache (user_id, cache_type, value_json, expires_at, created_at, updated_at) \
VALUES ($1, 'preserve_type', '[2]'::jsonb, NOW() - INTERVAL '2 hours', NOW(), NOW()) \
ON CONFLICT (user_id, cache_type) DO UPDATE SET value_json = EXCLUDED.value_json, expires_at = EXCLUDED.expires_at",
)
.bind(user_id)
.execute(&pool)
.await
.unwrap();

boardflow_db::queries::github_api_cache::cleanup_expired_cache(&pool)
.await
.unwrap();

let stale = boardflow_db::queries::github_api_cache::get_stale_cache(
&pool,
user_id,
"preserve_type",
chrono::Duration::hours(24),
)
.await
.unwrap();
assert_eq!(stale, Some(serde_json::json!([2])));
}

// ─── CachedGithubAccessChecker integration tests ─────────────────────────────

/// Test: CachedGithubAccessChecker.invalidate_cache removes all user's cache
Expand Down Expand Up @@ -450,10 +485,11 @@ async fn test_cached_checker_stale_fallback_with_mock_inner_rate_limited() {
assert_eq!(result, Ok(Some(vec![5001, 5002, 5003])));
}

/// Test: TokenExpired from inner does NOT use stale cache – propagates error
/// Issue #105: TokenExpired from inner falls back to stale cache so a
/// transient GitHub token failure does not break the repository list.
#[tokio::test]
#[serial]
async fn test_cached_checker_token_expired_no_stale_fallback() {
async fn test_cached_checker_token_expired_stale_fallback() {
use boardflow_api::github_access::TokenExpiredGithubAccessChecker;

let Some(pool) = setup_pool().await else {
Expand All @@ -478,14 +514,13 @@ async fn test_cached_checker_token_expired_no_stale_fallback() {
let checker = CachedGithubAccessChecker::with_inner(inner, pool.clone(), None);

let result = checker.list_accessible_repo_ids(token).await;
// Should propagate TokenExpired error, NOT return stale cache
assert_eq!(result, Err(AccessError::TokenExpired));
assert_eq!(result, Ok(Some(vec![6001, 6002])));
}

/// Test: Upstream error from inner does NOT use stale cache – propagates error
/// Issue #105: Upstream error from inner falls back to stale cache.
#[tokio::test]
#[serial]
async fn test_cached_checker_upstream_error_no_stale_fallback() {
async fn test_cached_checker_upstream_error_stale_fallback() {
use boardflow_api::github_access::UpstreamErrorGithubAccessChecker;

let Some(pool) = setup_pool().await else {
Expand All @@ -510,7 +545,46 @@ async fn test_cached_checker_upstream_error_no_stale_fallback() {
let checker = CachedGithubAccessChecker::with_inner(inner, pool.clone(), None);

let result = checker.list_accessible_repo_ids(token).await;
// Should propagate Upstream error, NOT return stale cache
assert_eq!(result, Ok(Some(vec![7001, 7002])));
}

/// Issue #105: TokenExpired with no stale cache still propagates the error
/// (the caller decides — e.g. list_repositories — but the cache layer must
/// not fabricate an empty list).
#[tokio::test]
#[serial]
async fn test_cached_checker_token_expired_no_stale_returns_error() {
use boardflow_api::github_access::TokenExpiredGithubAccessChecker;

let Some(pool) = setup_pool().await else {
return;
};
let token = "gho_mock_token_expired_no_stale";
let _user_id = create_test_user_with_token(&pool, token).await;

let inner: Arc<dyn GithubAccessChecker> = Arc::new(TokenExpiredGithubAccessChecker);
let checker = CachedGithubAccessChecker::with_inner(inner, pool.clone(), None);

let result = checker.list_accessible_repo_ids(token).await;
assert_eq!(result, Err(AccessError::TokenExpired));
}

/// Issue #105: Upstream error with no stale cache still propagates the error.
#[tokio::test]
#[serial]
async fn test_cached_checker_upstream_error_no_stale_returns_error() {
use boardflow_api::github_access::UpstreamErrorGithubAccessChecker;

let Some(pool) = setup_pool().await else {
return;
};
let token = "gho_mock_upstream_error_no_stale";
let _user_id = create_test_user_with_token(&pool, token).await;

let inner: Arc<dyn GithubAccessChecker> = Arc::new(UpstreamErrorGithubAccessChecker);
let checker = CachedGithubAccessChecker::with_inner(inner, pool.clone(), None);

let result = checker.list_accessible_repo_ids(token).await;
assert!(matches!(result, Err(AccessError::Upstream(_))));
}

Expand Down
89 changes: 89 additions & 0 deletions crates/api/tests/webhook_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,95 @@ async fn test_webhook_repos_removed_different_installation() {
);
}

// --- Issue #105: webhook events stamp installation sync_state ---

#[tokio::test]
#[serial]
async fn test_webhook_installation_created_stamps_sync_state() {
let Some(pool) = setup_pool().await else {
return;
};

let installation_id = rand_i64();
let body = serde_json::json!({
"action": "created",
"installation": { "id": installation_id },
"repositories": []
});
let body_bytes = serde_json::to_vec(&body).unwrap();
let signature = compute_signature(WEBHOOK_SECRET, &body_bytes);

let app = create_test_app(pool.clone());
let response = app
.oneshot(
Request::builder()
.method("POST")
.uri("/api/v1/github/webhook")
.header("Content-Type", "application/json")
.header("X-GitHub-Event", "installation")
.header("X-Hub-Signature-256", &signature)
.body(Body::from(body_bytes))
.unwrap(),
)
.await
.unwrap();

assert_eq!(response.status(), StatusCode::OK);

let state = boardflow_db::queries::github_installation_sync_state::find_by_installation_id(
&pool,
installation_id,
)
.await
.unwrap()
.expect("sync_state row should exist after webhook");
assert!(state.webhook_seen_at.is_some());
}

#[tokio::test]
#[serial]
async fn test_webhook_repos_added_stamps_sync_state() {
let Some(pool) = setup_pool().await else {
return;
};

let installation_id = rand_i64();
let body = serde_json::json!({
"action": "added",
"installation": { "id": installation_id },
"repositories_added": [],
"repositories_removed": []
});
let body_bytes = serde_json::to_vec(&body).unwrap();
let signature = compute_signature(WEBHOOK_SECRET, &body_bytes);

let app = create_test_app(pool.clone());
let response = app
.oneshot(
Request::builder()
.method("POST")
.uri("/api/v1/github/webhook")
.header("Content-Type", "application/json")
.header("X-GitHub-Event", "installation_repositories")
.header("X-Hub-Signature-256", &signature)
.body(Body::from(body_bytes))
.unwrap(),
)
.await
.unwrap();

assert_eq!(response.status(), StatusCode::OK);

let state = boardflow_db::queries::github_installation_sync_state::find_by_installation_id(
&pool,
installation_id,
)
.await
.unwrap()
.expect("sync_state row should exist after webhook");
assert!(state.webhook_seen_at.is_some());
}

// --- Test: unknown event returns 200 ---

#[tokio::test]
Expand Down
26 changes: 26 additions & 0 deletions crates/config/src/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ pub struct WorkerConfig {
pub poll_interval_secs: u64,
pub timeout_sweep_interval_secs: u64,
pub cache_cleanup_interval_secs: u64,
pub installation_sync_interval_secs: u64,
pub installation_sync_stale_after_secs: u64,
pub installation_sync_min_interval_secs: u64,
pub installation_sync_max_per_sweep: u64,
pub github_app_id: Option<u64>,
pub github_private_key_pem: Option<String>,
pub app_domain: String,
Expand All @@ -33,12 +37,34 @@ impl WorkerConfig {
});
}

let installation_sync_interval_secs =
parse_env_or("INSTALLATION_SYNC_INTERVAL_SECS", 1800u64)?;
if installation_sync_interval_secs == 0 {
return Err(ConfigError::InvalidValue {
var: "INSTALLATION_SYNC_INTERVAL_SECS".to_string(),
reason: "must be greater than 0".to_string(),
});
}

Ok(Self {
db: DatabaseConfig::from_env()?,
s3: S3Config::from_env(),
poll_interval_secs: parse_env_or("POLL_INTERVAL_SECS", 2u64)?,
timeout_sweep_interval_secs: parse_env_or("TIMEOUT_SWEEP_INTERVAL_SECS", 60u64)?,
cache_cleanup_interval_secs,
installation_sync_interval_secs,
installation_sync_stale_after_secs: parse_env_or(
"INSTALLATION_SYNC_STALE_AFTER_SECS",
86_400u64,
)?,
installation_sync_min_interval_secs: parse_env_or(
"INSTALLATION_SYNC_MIN_INTERVAL_SECS",
3600u64,
)?,
installation_sync_max_per_sweep: parse_env_or(
"INSTALLATION_SYNC_MAX_PER_SWEEP",
50u64,
)?,
github_app_id,
github_private_key_pem: optional_env("GITHUB_PRIVATE_KEY_PEM"),
app_domain: std::env::var("BOARDFLOW_APP_DOMAIN").unwrap_or_else(|_| {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
DROP INDEX IF EXISTS idx_repositories_updated_at_github_id;
DROP INDEX IF EXISTS idx_github_installation_sync_state_stale;
DROP TABLE IF EXISTS github_installation_sync_state;
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
-- Issue #105: per-installation sync state for webhook freshness / worker reconciliation
CREATE TABLE github_installation_sync_state (
installation_id BIGINT PRIMARY KEY,
webhook_seen_at TIMESTAMPTZ,
last_sync_started_at TIMESTAMPTZ,
last_sync_completed_at TIMESTAMPTZ,
last_sync_status TEXT CONSTRAINT github_installation_sync_state_status_check
CHECK (last_sync_status IN ('success', 'failed')),
last_error TEXT,
updated_at TIMESTAMPTZ NOT NULL DEFAULT now()
);

-- Worker periodically picks installations needing reconciliation. The selector
-- is "webhook_seen_at IS NULL OR webhook_seen_at < cutoff OR last_sync_status = 'failed'",
-- so an index on webhook_seen_at + last_sync_status keeps that scan cheap as
-- installations accumulate.
CREATE INDEX idx_github_installation_sync_state_stale
ON github_installation_sync_state (webhook_seen_at NULLS FIRST, last_sync_status);

-- Issue #105: list_repositories paginates by (updated_at DESC, github_repository_id DESC).
-- Adding a composite index lets the cursor pagination be index-only.
CREATE INDEX idx_repositories_updated_at_github_id
ON repositories (updated_at DESC, github_repository_id DESC);
Loading
Loading