feat(db): scheduler foundation for v0.4.0 auto-publish (PR F1)#69
Merged
Conversation
First PR of the v0.4.0 stack per docs/roadmap-v0.4.md. Data layer only — the background task that consumes this module lands in PR F2; the UI lands in PR F3. Same foundation pattern as PR #56 (post_groups) used before the multi-network composer wired in. ## Migration 019 Two columns added to `post_history` for retry bookkeeping: - `failed_attempts INTEGER NOT NULL DEFAULT 0` — bounded counter, the scheduler gives up at MAX_FAILED_ATTEMPTS (3) and flips the row to `status = 'failed'`. - `last_attempt_at TEXT` — nullable RFC 3339 UTC timestamp, drives the backoff-window math without a separate attempts history table. Both `#[serde(default)]` on the Rust struct so deserialising a pre-v0.4 PostRecord JSON (e.g. from the in-flight composer state when the user upgrades mid-session) gracefully fills the defaults. ## db::scheduler module Six items, each verified by an in-memory pool test: - `MAX_FAILED_ATTEMPTS` const (3) - `BACKOFF_MINUTES` const ([0, 5, 30, 120] — 0 for never-tried, then 5min / 30min / 2h backoff) - `list_due_for_publish(pool)` — single SQL with a CASE clause encoding the backoff schedule directly; returns posts whose `scheduled_at` is past AND whose retry window has elapsed - `try_lock_for_publish(pool, post_id)` — atomic flip `'draft' → 'publishing'` via `WHERE status = 'draft'`; returns `false` when another worker raced. Lock-row pattern prevents two concurrent app launches from double-publishing the same row. - `mark_publish_attempt_failed(pool, post_id)` — increments counter, sets `last_attempt_at`, flips to `'failed'` once the threshold is crossed. Returns enum WillRetry vs GaveUp so the scheduler can route to the right notification. - `reset_retry_state(pool, post_id)` — clears counter + timestamp back to `status = 'draft'`. Called from the Calendar reschedule flow in PR F3 when the user manually fixes a failure (token reconnect, etc.). ## Critical SQL bug avoided `scheduled_at <= datetime('now')` doesn't work — SQLite's `datetime('now')` returns `'YYYY-MM-DD HH:MM:SS'` (space separator, no T, no Z) which sorts lexicographically BELOW `chrono::Utc::now().to_rfc3339()` writes (`'YYYY-MM-DDTHH:MM:SS.fffZ'`) for the SAME wall-clock instant. Two tests caught this on first run (`list_due_returns_overdue_drafts_only` failed because the RFC 3339 scheduled_at compared as STRICTLY GREATER than `datetime('now')` even when the instant was in the past). Fix: bind `Utc::now().to_rfc3339()` as a parameter so producer + consumer share the same string convention. Documented inline. ## Tests - +10 `db::scheduler::tests` — covers list_due (overdue match, future skip, backoff-window skip, backoff-window expiration, max-attempts skip, non-draft status skip), try_lock (success + race), mark_failed (will-retry x 2 + gave-up), reset, and a policy-table lock-in test. - +1 `db::migration_tests::scheduler_retry_columns_exist_after_migration_019` — confirms NOT NULL DEFAULT 0 on `failed_attempts` and nullable on `last_attempt_at` via PRAGMA inspection. Total: Rust 237/237 (was 226), clippy + fmt clean. ## Targeted dead_code allows PR F2 will consume every symbol exposed here. Until then, six items (1 enum + 4 fns + 2 consts) carry `#[allow(dead_code)]` with an inline note pointing at PR F2 — same pattern as PR #56 used before #57 picked it up. ## Next: PR F2 Background task `tokio::spawn` at app startup. Poll loop every 60s. For each due row: try_lock → publish_post / publish_linkedin_post → mark_failed on Err / no-op on Ok (publish_post already updates status to 'published'). Notification on final-failure via tauri-plugin-notification. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Guide du·de la relecteur·riceIntroduit la couche de base de la base de données pour le planificateur de publication automatique v0.4.0 en ajoutant des colonnes de suivi de nouvelle tentative à Diagramme de flux pour le cycle de vie des nouvelles tentatives du planificateur et des statutsflowchart LR
A[Publication planifiée en brouillon\nstatut draft\nfailed_attempts = 0] -->|list_due_for_publish correspond| B[try_lock_for_publish\nfixe status = publishing\nmet à jour last_attempt_at]
B --> C[Tentative de publication]
C -->|success| D[status published\nfailed_attempts inchangé]
C -->|error| E[mark_publish_attempt_failed\nincrémente failed_attempts\nmet à jour last_attempt_at]
E -->|PublishFailureOutcome WillRetry\nfailed_attempts < MAX_FAILED_ATTEMPTS| A
E -->|PublishFailureOutcome GaveUp\nfailed_attempts >= MAX_FAILED_ATTEMPTS| F[status failed\naucune nouvelle tentative]
F -->|Replanification par l'utilisateur·rice\nreset_retry_state| A
Modifications au niveau des fichiers
Conseils et commandesInteragir avec Sourcery
Personnaliser votre expérienceAccédez à votre tableau de bord pour :
Obtenir de l’aide
Original review guide in EnglishReviewer's GuideIntroduces the database-layer foundation for the v0.4.0 auto-publish scheduler by adding retry bookkeeping columns to post_history, a new db::scheduler module with scheduling/backoff/locking APIs, and tests that lock in the retry policy and migration contract. Flow diagram for scheduler retry and status lifecycleflowchart LR
A[Draft scheduled post\nstatus draft\nfailed_attempts = 0] -->|list_due_for_publish matches| B[try_lock_for_publish\nsets status = publishing\nupdates last_attempt_at]
B --> C[Publish attempt]
C -->|success| D[status published\nfailed_attempts unchanged]
C -->|error| E[mark_publish_attempt_failed\nincrements failed_attempts\nupdates last_attempt_at]
E -->|PublishFailureOutcome WillRetry\nfailed_attempts < MAX_FAILED_ATTEMPTS| A
E -->|PublishFailureOutcome GaveUp\nfailed_attempts >= MAX_FAILED_ATTEMPTS| F[status failed\nno further retries]
F -->|User reschedules\nreset_retry_state| A
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - j’ai trouvé 3 problèmes et laissé quelques retours de haut niveau :
- Le planning de backoff est actuellement dupliqué entre
BACKOFF_MINUTESet leCASE failed_attemptscodé en dur danslist_due_for_publish. Il serait préférable de centraliser ça (par ex. via un helper ou au moins un test dédié qui vérifie explicitement que les minutes duCASESQL correspondent àBACKOFF_MINUTES) afin d’éviter une dérive silencieuse si la politique change. reset_retry_statedéfinit inconditionnellementstatus = 'draft'pour lepost_iddonné. Si cela est prévu uniquement pour des posts ayant déjà échoué, il serait plus sûr de restreindre leUPDATEavec unWHERE id = ? AND status = 'failed'(ou similaire) pour éviter de remettre accidentellement à l’étatdraftune ligne déjà correctementpublished.
Prompt pour les agents IA
Please address the comments from this code review:
## Overall Comments
- The backoff schedule is currently duplicated between `BACKOFF_MINUTES` and the hard-coded `CASE failed_attempts` in `list_due_for_publish`; consider centralizing this (e.g. via a helper or at least a dedicated test that explicitly asserts the SQL CASE minutes against `BACKOFF_MINUTES`) to avoid silent drift if the policy changes.
- `reset_retry_state` unconditionally sets `status = 'draft'` for the given `post_id`; if this is intended only for previously failed posts, it may be safer to restrict the `UPDATE` with a `WHERE id = ? AND status = 'failed'` (or similar) to avoid accidentally resetting a legitimately `published` row.
## Individual Comments
### Comment 1
<location path="src-tauri/src/db/scheduler.rs" line_range="141-150" />
<code_context>
+/// Should be called by the scheduler in PR F2 when the actual publish
+/// call (`publish_post` / `publish_linkedin_post`) returned `Err`.
+#[allow(dead_code)]
+pub async fn mark_publish_attempt_failed(
+ pool: &SqlitePool,
+ post_id: i64,
+) -> Result<PublishFailureOutcome, String> {
+ let now = Utc::now().to_rfc3339();
+ // Single UPDATE that does the accounting: the CASE flips the
+ // status to 'failed' once the increment crosses MAX_FAILED_ATTEMPTS,
+ // otherwise returns the row to draft so the next polling pass can
+ // pick it up after the backoff window.
+ sqlx::query(
+ "UPDATE post_history
+ SET failed_attempts = failed_attempts + 1,
+ last_attempt_at = ?,
+ status = CASE
+ WHEN failed_attempts + 1 >= ? THEN 'failed'
+ ELSE 'draft'
+ END
+ WHERE id = ?",
+ )
+ .bind(&now)
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard the failure accounting by status to avoid incrementing retries in unexpected states.
The UPDATE in `mark_publish_attempt_failed` currently runs for any row with the given `id`, regardless of `status`. If this is called when a post is already `failed`, `published`, or still `draft`, it will still increment `failed_attempts` and may flip `status` again. Please constrain the UPDATE with an appropriate status predicate (e.g. `WHERE id = ? AND status = 'publishing'`) so only in-flight publishes are affected and retry state can’t be corrupted by misuse from other call sites.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/db/scheduler.rs" line_range="188-158" />
<code_context>
+/// underlying problem (reconnected account, etc.) and want a fresh
+/// budget. Called by the Calendar reschedule flow in PR F3.
+#[allow(dead_code)]
+pub async fn reset_retry_state(pool: &SqlitePool, post_id: i64) -> Result<(), String> {
+ sqlx::query(
+ "UPDATE post_history
+ SET failed_attempts = 0,
+ last_attempt_at = NULL,
+ status = 'draft'
+ WHERE id = ?",
+ )
+ .bind(post_id)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider restricting `reset_retry_state` to specific statuses to avoid racing with in-flight publishes.
Because this updates `failed_attempts`, `last_attempt_at`, and `status` unconditionally, a concurrent call while a post is `publishing` (or locked by another worker) can overwrite the scheduler’s state and lead to confusing outcomes (e.g., a publish completing after the status has been reset). Consider adding a status guard in the `WHERE` clause (e.g., `status IN ('failed', 'draft')` or just `'failed'`) and surfacing when no rows are updated so callers can handle concurrent edits explicitly.
Suggested implementation:
```rust
#[allow(dead_code)]
pub async fn reset_retry_state(pool: &SqlitePool, post_id: i64) -> Result<bool, String> {
let result = sqlx::query(
"UPDATE post_history
SET failed_attempts = 0,
last_attempt_at = NULL,
status = 'draft'
WHERE id = ?
AND status IN ('draft', 'failed')",
)
.bind(post_id)
.execute(pool)
.await
.map_err(|e| e.to_string())?;
Ok(result.rows_affected() == 1)
}
```
1. Update all call sites of `reset_retry_state` to handle the new `Result<bool, String>` signature, checking the boolean to detect when no row was updated (e.g., due to a concurrent state change).
2. If your scheduler uses additional transient states (such as `'queued'` or `'retrying'`) that are also safe to reset from, extend the `status IN ('draft', 'failed')` clause accordingly to match your state machine.
</issue_to_address>
### Comment 3
<location path="src-tauri/src/db/scheduler.rs" line_range="73" />
<code_context>
+/// `failed_attempts` so the SQL stays one query — no per-row decision
+/// in Rust.
+#[allow(dead_code)]
+pub async fn list_due_for_publish(pool: &SqlitePool) -> Result<Vec<PostRecord>, String> {
+ let now_rfc3339 = Utc::now().to_rfc3339();
+ let rows: Vec<sqlx::sqlite::SqliteRow> = sqlx::query(
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the scheduler’s retry handling by moving backoff logic into Rust and using an UPDATE … RETURNING to collapse multi-step DB operations into single statements.
You can reduce complexity without changing behavior by:
1. **Moving the backoff window logic out of SQL and into Rust**, using the existing `BACKOFF_MINUTES`.
2. **Using `UPDATE … RETURNING` in `mark_publish_attempt_failed`** to avoid the two-step UPDATE+SELECT.
### 1. Simplify `list_due_for_publish` SQL and centralize backoff in Rust
Right now the retry policy is duplicated and encoded in a complex `CASE` + `datetime` expression. You already have `BACKOFF_MINUTES`; you can use it to keep a single source of truth and make the SQL easier to read.
Example refactor:
```rust
#[allow(dead_code)]
pub async fn list_due_for_publish(pool: &SqlitePool) -> Result<Vec<PostRecord>, String> {
let now = Utc::now();
let now_rfc3339 = now.to_rfc3339();
// Simpler SQL: let SQLite only enforce basic constraints.
let rows: Vec<sqlx::sqlite::SqliteRow> = sqlx::query(
"SELECT id, network, caption, hashtags, status, created_at, published_at,
scheduled_at, image_path, images, ig_media_id, account_id, published_url,
group_id, failed_attempts, last_attempt_at
FROM post_history
WHERE status = 'draft'
AND scheduled_at IS NOT NULL
AND scheduled_at <= ?
AND failed_attempts < ?
ORDER BY scheduled_at ASC",
)
.bind(&now_rfc3339)
.bind(MAX_FAILED_ATTEMPTS)
.fetch_all(pool)
.await
.map_err(|e| e.to_string())?;
// Apply retry window in Rust using BACKOFF_MINUTES as sole source of truth.
let due = rows
.into_iter()
.filter_map(|row| {
let mut post = row_to_post_record(&row)?;
// Convert RFC3339 fields to chrono types
let last_attempt_at = post
.last_attempt_at
.as_deref()
.and_then(|s| chrono::DateTime::parse_from_rfc3339(s).ok())
.map(|dt| dt.with_timezone(&Utc));
let failed_attempts = post.failed_attempts;
let backoff_minutes = BACKOFF_MINUTES
.get(failed_attempts as usize)
.copied()
.unwrap_or_else(|| *BACKOFF_MINUTES.last().unwrap());
let next_allowed = last_attempt_at
.map(|t| t + chrono::Duration::minutes(backoff_minutes))
// never tried → eligible immediately when scheduled_at <= now
.unwrap_or(now);
if next_allowed <= now {
Some(post)
} else {
None
}
})
.collect();
Ok(due)
}
```
This keeps all behavior but:
- Removes the `CASE` + `datetime` expression from SQL.
- Uses `BACKOFF_MINUTES` directly so your retry policy is defined in one place.
- Makes the `backoff_minutes_table_matches_documented_policy` test strictly about the Rust constants (and you can drop any tests that only exist to keep SQL and Rust in sync).
### 2. Use `UPDATE … RETURNING` in `mark_publish_attempt_failed`
You can avoid the second query and keep the logic in one place:
```rust
#[allow(dead_code)]
pub async fn mark_publish_attempt_failed(
pool: &SqlitePool,
post_id: i64,
) -> Result<PublishFailureOutcome, String> {
let now = Utc::now().to_rfc3339();
// Single statement: increment, set status, and read back.
let (failed_attempts, status): (i64, String) = sqlx::query_as(
"UPDATE post_history
SET failed_attempts = failed_attempts + 1,
last_attempt_at = ?,
status = CASE
WHEN failed_attempts + 1 >= ? THEN 'failed'
ELSE 'draft'
END
WHERE id = ?
RETURNING failed_attempts, status",
)
.bind(&now)
.bind(MAX_FAILED_ATTEMPTS)
.bind(post_id)
.fetch_one(pool)
.await
.map_err(|e| e.to_string())?;
if status == "failed" {
Ok(PublishFailureOutcome::GaveUp { failed_attempts })
} else {
Ok(PublishFailureOutcome::WillRetry { failed_attempts })
}
}
```
This keeps the same semantics but:
- Removes the need to mentally track “before” and “after” states across two DB calls.
- Reduces DB round-trips and simplifies error handling.
If you adopt these two changes, the module’s core logic becomes easier to follow, and the retry policy has a single authoritative definition.
</issue_to_address>Sourcery est gratuit pour l’open source - si vous aimez nos revues de code, pensez à les partager ✨
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- The backoff schedule is currently duplicated between
BACKOFF_MINUTESand the hard-codedCASE failed_attemptsinlist_due_for_publish; consider centralizing this (e.g. via a helper or at least a dedicated test that explicitly asserts the SQL CASE minutes againstBACKOFF_MINUTES) to avoid silent drift if the policy changes. reset_retry_stateunconditionally setsstatus = 'draft'for the givenpost_id; if this is intended only for previously failed posts, it may be safer to restrict theUPDATEwith aWHERE id = ? AND status = 'failed'(or similar) to avoid accidentally resetting a legitimatelypublishedrow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The backoff schedule is currently duplicated between `BACKOFF_MINUTES` and the hard-coded `CASE failed_attempts` in `list_due_for_publish`; consider centralizing this (e.g. via a helper or at least a dedicated test that explicitly asserts the SQL CASE minutes against `BACKOFF_MINUTES`) to avoid silent drift if the policy changes.
- `reset_retry_state` unconditionally sets `status = 'draft'` for the given `post_id`; if this is intended only for previously failed posts, it may be safer to restrict the `UPDATE` with a `WHERE id = ? AND status = 'failed'` (or similar) to avoid accidentally resetting a legitimately `published` row.
## Individual Comments
### Comment 1
<location path="src-tauri/src/db/scheduler.rs" line_range="141-150" />
<code_context>
+/// Should be called by the scheduler in PR F2 when the actual publish
+/// call (`publish_post` / `publish_linkedin_post`) returned `Err`.
+#[allow(dead_code)]
+pub async fn mark_publish_attempt_failed(
+ pool: &SqlitePool,
+ post_id: i64,
+) -> Result<PublishFailureOutcome, String> {
+ let now = Utc::now().to_rfc3339();
+ // Single UPDATE that does the accounting: the CASE flips the
+ // status to 'failed' once the increment crosses MAX_FAILED_ATTEMPTS,
+ // otherwise returns the row to draft so the next polling pass can
+ // pick it up after the backoff window.
+ sqlx::query(
+ "UPDATE post_history
+ SET failed_attempts = failed_attempts + 1,
+ last_attempt_at = ?,
+ status = CASE
+ WHEN failed_attempts + 1 >= ? THEN 'failed'
+ ELSE 'draft'
+ END
+ WHERE id = ?",
+ )
+ .bind(&now)
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard the failure accounting by status to avoid incrementing retries in unexpected states.
The UPDATE in `mark_publish_attempt_failed` currently runs for any row with the given `id`, regardless of `status`. If this is called when a post is already `failed`, `published`, or still `draft`, it will still increment `failed_attempts` and may flip `status` again. Please constrain the UPDATE with an appropriate status predicate (e.g. `WHERE id = ? AND status = 'publishing'`) so only in-flight publishes are affected and retry state can’t be corrupted by misuse from other call sites.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/db/scheduler.rs" line_range="188-158" />
<code_context>
+/// underlying problem (reconnected account, etc.) and want a fresh
+/// budget. Called by the Calendar reschedule flow in PR F3.
+#[allow(dead_code)]
+pub async fn reset_retry_state(pool: &SqlitePool, post_id: i64) -> Result<(), String> {
+ sqlx::query(
+ "UPDATE post_history
+ SET failed_attempts = 0,
+ last_attempt_at = NULL,
+ status = 'draft'
+ WHERE id = ?",
+ )
+ .bind(post_id)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider restricting `reset_retry_state` to specific statuses to avoid racing with in-flight publishes.
Because this updates `failed_attempts`, `last_attempt_at`, and `status` unconditionally, a concurrent call while a post is `publishing` (or locked by another worker) can overwrite the scheduler’s state and lead to confusing outcomes (e.g., a publish completing after the status has been reset). Consider adding a status guard in the `WHERE` clause (e.g., `status IN ('failed', 'draft')` or just `'failed'`) and surfacing when no rows are updated so callers can handle concurrent edits explicitly.
Suggested implementation:
```rust
#[allow(dead_code)]
pub async fn reset_retry_state(pool: &SqlitePool, post_id: i64) -> Result<bool, String> {
let result = sqlx::query(
"UPDATE post_history
SET failed_attempts = 0,
last_attempt_at = NULL,
status = 'draft'
WHERE id = ?
AND status IN ('draft', 'failed')",
)
.bind(post_id)
.execute(pool)
.await
.map_err(|e| e.to_string())?;
Ok(result.rows_affected() == 1)
}
```
1. Update all call sites of `reset_retry_state` to handle the new `Result<bool, String>` signature, checking the boolean to detect when no row was updated (e.g., due to a concurrent state change).
2. If your scheduler uses additional transient states (such as `'queued'` or `'retrying'`) that are also safe to reset from, extend the `status IN ('draft', 'failed')` clause accordingly to match your state machine.
</issue_to_address>
### Comment 3
<location path="src-tauri/src/db/scheduler.rs" line_range="73" />
<code_context>
+/// `failed_attempts` so the SQL stays one query — no per-row decision
+/// in Rust.
+#[allow(dead_code)]
+pub async fn list_due_for_publish(pool: &SqlitePool) -> Result<Vec<PostRecord>, String> {
+ let now_rfc3339 = Utc::now().to_rfc3339();
+ let rows: Vec<sqlx::sqlite::SqliteRow> = sqlx::query(
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the scheduler’s retry handling by moving backoff logic into Rust and using an UPDATE … RETURNING to collapse multi-step DB operations into single statements.
You can reduce complexity without changing behavior by:
1. **Moving the backoff window logic out of SQL and into Rust**, using the existing `BACKOFF_MINUTES`.
2. **Using `UPDATE … RETURNING` in `mark_publish_attempt_failed`** to avoid the two-step UPDATE+SELECT.
### 1. Simplify `list_due_for_publish` SQL and centralize backoff in Rust
Right now the retry policy is duplicated and encoded in a complex `CASE` + `datetime` expression. You already have `BACKOFF_MINUTES`; you can use it to keep a single source of truth and make the SQL easier to read.
Example refactor:
```rust
#[allow(dead_code)]
pub async fn list_due_for_publish(pool: &SqlitePool) -> Result<Vec<PostRecord>, String> {
let now = Utc::now();
let now_rfc3339 = now.to_rfc3339();
// Simpler SQL: let SQLite only enforce basic constraints.
let rows: Vec<sqlx::sqlite::SqliteRow> = sqlx::query(
"SELECT id, network, caption, hashtags, status, created_at, published_at,
scheduled_at, image_path, images, ig_media_id, account_id, published_url,
group_id, failed_attempts, last_attempt_at
FROM post_history
WHERE status = 'draft'
AND scheduled_at IS NOT NULL
AND scheduled_at <= ?
AND failed_attempts < ?
ORDER BY scheduled_at ASC",
)
.bind(&now_rfc3339)
.bind(MAX_FAILED_ATTEMPTS)
.fetch_all(pool)
.await
.map_err(|e| e.to_string())?;
// Apply retry window in Rust using BACKOFF_MINUTES as sole source of truth.
let due = rows
.into_iter()
.filter_map(|row| {
let mut post = row_to_post_record(&row)?;
// Convert RFC3339 fields to chrono types
let last_attempt_at = post
.last_attempt_at
.as_deref()
.and_then(|s| chrono::DateTime::parse_from_rfc3339(s).ok())
.map(|dt| dt.with_timezone(&Utc));
let failed_attempts = post.failed_attempts;
let backoff_minutes = BACKOFF_MINUTES
.get(failed_attempts as usize)
.copied()
.unwrap_or_else(|| *BACKOFF_MINUTES.last().unwrap());
let next_allowed = last_attempt_at
.map(|t| t + chrono::Duration::minutes(backoff_minutes))
// never tried → eligible immediately when scheduled_at <= now
.unwrap_or(now);
if next_allowed <= now {
Some(post)
} else {
None
}
})
.collect();
Ok(due)
}
```
This keeps all behavior but:
- Removes the `CASE` + `datetime` expression from SQL.
- Uses `BACKOFF_MINUTES` directly so your retry policy is defined in one place.
- Makes the `backoff_minutes_table_matches_documented_policy` test strictly about the Rust constants (and you can drop any tests that only exist to keep SQL and Rust in sync).
### 2. Use `UPDATE … RETURNING` in `mark_publish_attempt_failed`
You can avoid the second query and keep the logic in one place:
```rust
#[allow(dead_code)]
pub async fn mark_publish_attempt_failed(
pool: &SqlitePool,
post_id: i64,
) -> Result<PublishFailureOutcome, String> {
let now = Utc::now().to_rfc3339();
// Single statement: increment, set status, and read back.
let (failed_attempts, status): (i64, String) = sqlx::query_as(
"UPDATE post_history
SET failed_attempts = failed_attempts + 1,
last_attempt_at = ?,
status = CASE
WHEN failed_attempts + 1 >= ? THEN 'failed'
ELSE 'draft'
END
WHERE id = ?
RETURNING failed_attempts, status",
)
.bind(&now)
.bind(MAX_FAILED_ATTEMPTS)
.bind(post_id)
.fetch_one(pool)
.await
.map_err(|e| e.to_string())?;
if status == "failed" {
Ok(PublishFailureOutcome::GaveUp { failed_attempts })
} else {
Ok(PublishFailureOutcome::WillRetry { failed_attempts })
}
}
```
This keeps the same semantics but:
- Removes the need to mentally track “before” and “after” states across two DB calls.
- Reduces DB round-trips and simplifies error handling.
If you adopt these two changes, the module’s core logic becomes easier to follow, and the retry policy has a single authoritative definition.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+141
to
+150
| pub async fn mark_publish_attempt_failed( | ||
| pool: &SqlitePool, | ||
| post_id: i64, | ||
| ) -> Result<PublishFailureOutcome, String> { | ||
| let now = Utc::now().to_rfc3339(); | ||
| // Single UPDATE that does the accounting: the CASE flips the | ||
| // status to 'failed' once the increment crosses MAX_FAILED_ATTEMPTS, | ||
| // otherwise returns the row to draft so the next polling pass can | ||
| // pick it up after the backoff window. | ||
| sqlx::query( |
There was a problem hiding this comment.
issue (bug_risk): Protéger la comptabilisation des échecs via le statut pour éviter d’incrémenter les tentatives de nouvelle exécution dans des états inattendus.
Le UPDATE dans mark_publish_attempt_failed s’exécute actuellement pour n’importe quelle ligne avec l’id donné, quel que soit le status. Si cette fonction est appelée alors qu’un post est déjà failed, published ou encore draft, elle incrémentera tout de même failed_attempts et pourra modifier à nouveau status. Merci de restreindre le UPDATE avec un prédicat de statut approprié (par ex. WHERE id = ? AND status = 'publishing') afin que seules les publications en cours soient affectées et que l’état de retry ne puisse pas être corrompu par une mauvaise utilisation depuis d’autres points d’appel.
Original comment in English
issue (bug_risk): Guard the failure accounting by status to avoid incrementing retries in unexpected states.
The UPDATE in mark_publish_attempt_failed currently runs for any row with the given id, regardless of status. If this is called when a post is already failed, published, or still draft, it will still increment failed_attempts and may flip status again. Please constrain the UPDATE with an appropriate status predicate (e.g. WHERE id = ? AND status = 'publishing') so only in-flight publishes are affected and retry state can’t be corrupted by misuse from other call sites.
| WHEN failed_attempts + 1 >= ? THEN 'failed' | ||
| ELSE 'draft' | ||
| END | ||
| WHERE id = ?", |
There was a problem hiding this comment.
suggestion (bug_risk): Envisagez de restreindre reset_retry_state à des statuts spécifiques afin d’éviter les courses avec des publications en cours.
Comme cette requête met à jour failed_attempts, last_attempt_at et status de manière inconditionnelle, un appel concurrent alors qu’un post est en cours de publishing (ou verrouillé par un autre worker) peut écraser l’état du scheduler et conduire à des situations déroutantes (par exemple, une publication qui se termine après que le statut ait été réinitialisé). Envisagez d’ajouter une condition sur le statut dans la clause WHERE (par ex. status IN ('failed', 'draft') ou seulement 'failed') et de faire remonter le cas où aucune ligne n’est mise à jour afin que les appelants puissent gérer explicitement les modifications concurrentes.
Implémentation suggérée :
#[allow(dead_code)]
pub async fn reset_retry_state(pool: &SqlitePool, post_id: i64) -> Result<bool, String> {
let result = sqlx::query(
"UPDATE post_history
SET failed_attempts = 0,
last_attempt_at = NULL,
status = 'draft'
WHERE id = ?
AND status IN ('draft', 'failed')",
)
.bind(post_id)
.execute(pool)
.await
.map_err(|e| e.to_string())?;
Ok(result.rows_affected() == 1)
}- Mettez à jour tous les points d’appel de
reset_retry_statepour gérer la nouvelle signatureResult<bool, String>, en vérifiant le booléen pour détecter les cas où aucune ligne n’a été mise à jour (par ex. à cause d’un changement d’état concurrent). - Si votre scheduler utilise des états transitoires supplémentaires (comme
'queued'ou'retrying') qui sont également sûrs à réinitialiser, étendez la clausestatus IN ('draft', 'failed')en conséquence pour refléter votre machine à états.
Original comment in English
suggestion (bug_risk): Consider restricting reset_retry_state to specific statuses to avoid racing with in-flight publishes.
Because this updates failed_attempts, last_attempt_at, and status unconditionally, a concurrent call while a post is publishing (or locked by another worker) can overwrite the scheduler’s state and lead to confusing outcomes (e.g., a publish completing after the status has been reset). Consider adding a status guard in the WHERE clause (e.g., status IN ('failed', 'draft') or just 'failed') and surfacing when no rows are updated so callers can handle concurrent edits explicitly.
Suggested implementation:
#[allow(dead_code)]
pub async fn reset_retry_state(pool: &SqlitePool, post_id: i64) -> Result<bool, String> {
let result = sqlx::query(
"UPDATE post_history
SET failed_attempts = 0,
last_attempt_at = NULL,
status = 'draft'
WHERE id = ?
AND status IN ('draft', 'failed')",
)
.bind(post_id)
.execute(pool)
.await
.map_err(|e| e.to_string())?;
Ok(result.rows_affected() == 1)
}- Update all call sites of
reset_retry_stateto handle the newResult<bool, String>signature, checking the boolean to detect when no row was updated (e.g., due to a concurrent state change). - If your scheduler uses additional transient states (such as
'queued'or'retrying') that are also safe to reset from, extend thestatus IN ('draft', 'failed')clause accordingly to match your state machine.
| /// `failed_attempts` so the SQL stays one query — no per-row decision | ||
| /// in Rust. | ||
| #[allow(dead_code)] | ||
| pub async fn list_due_for_publish(pool: &SqlitePool) -> Result<Vec<PostRecord>, String> { |
There was a problem hiding this comment.
issue (complexity): Envisagez de simplifier la gestion des retries du scheduler en déplaçant la logique de backoff dans le code Rust et en utilisant un UPDATE … RETURNING pour regrouper les opérations multi‑étapes en base en instructions uniques.
Vous pouvez réduire la complexité sans changer le comportement en :
- Déplaçant la logique de fenêtre de backoff hors du SQL vers Rust, en utilisant le
BACKOFF_MINUTESexistant. - Utilisant
UPDATE … RETURNINGdansmark_publish_attempt_failedpour éviter le duoUPDATE + SELECT.
1. Simplifier le SQL de list_due_for_publish et centraliser le backoff dans Rust
Actuellement, la politique de retry est dupliquée et encodée dans une expression CASE + datetime complexe. Vous avez déjà BACKOFF_MINUTES ; vous pouvez l’utiliser pour garder une seule source de vérité et rendre le SQL plus lisible.
Exemple de refactorisation :
#[allow(dead_code)]
pub async fn list_due_for_publish(pool: &SqlitePool) -> Result<Vec<PostRecord>, String> {
let now = Utc::now();
let now_rfc3339 = now.to_rfc3339();
// SQL plus simple : laisser SQLite n’appliquer que les contraintes de base.
let rows: Vec<sqlx::sqlite::SqliteRow> = sqlx::query(
"SELECT id, network, caption, hashtags, status, created_at, published_at,
scheduled_at, image_path, images, ig_media_id, account_id, published_url,
group_id, failed_attempts, last_attempt_at
FROM post_history
WHERE status = 'draft'
AND scheduled_at IS NOT NULL
AND scheduled_at <= ?
AND failed_attempts < ?
ORDER BY scheduled_at ASC",
)
.bind(&now_rfc3339)
.bind(MAX_FAILED_ATTEMPTS)
.fetch_all(pool)
.await
.map_err(|e| e.to_string())?;
// Appliquer la fenêtre de retry en Rust en utilisant BACKOFF_MINUTES comme unique source de vérité.
let due = rows
.into_iter()
.filter_map(|row| {
let mut post = row_to_post_record(&row)?;
// Conversion des champs RFC3339 en types chrono
let last_attempt_at = post
.last_attempt_at
.as_deref()
.and_then(|s| chrono::DateTime::parse_from_rfc3339(s).ok())
.map(|dt| dt.with_timezone(&Utc));
let failed_attempts = post.failed_attempts;
let backoff_minutes = BACKOFF_MINUTES
.get(failed_attempts as usize)
.copied()
.unwrap_or_else(|| *BACKOFF_MINUTES.last().unwrap());
let next_allowed = last_attempt_at
.map(|t| t + chrono::Duration::minutes(backoff_minutes))
// jamais tenté → éligible immédiatement quand scheduled_at <= now
.unwrap_or(now);
if next_allowed <= now {
Some(post)
} else {
None
}
})
.collect();
Ok(due)
}Cela conserve tout le comportement, mais :
- Supprime l’expression
CASE+datetimedu SQL. - Utilise directement
BACKOFF_MINUTESpour que votre politique de retry ne soit définie qu’à un seul endroit. - Rend le test
backoff_minutes_table_matches_documented_policystrictement centré sur les constantes Rust (et vous pouvez supprimer les tests qui existent uniquement pour garder le SQL et Rust synchronisés).
2. Utiliser UPDATE … RETURNING dans mark_publish_attempt_failed
Vous pouvez éviter la seconde requête et regrouper la logique au même endroit :
#[allow(dead_code)]
pub async fn mark_publish_attempt_failed(
pool: &SqlitePool,
post_id: i64,
) -> Result<PublishFailureOutcome, String> {
let now = Utc::now().to_rfc3339();
// Instruction unique : incrément, mise à jour du statut, puis lecture.
let (failed_attempts, status): (i64, String) = sqlx::query_as(
"UPDATE post_history
SET failed_attempts = failed_attempts + 1,
last_attempt_at = ?,
status = CASE
WHEN failed_attempts + 1 >= ? THEN 'failed'
ELSE 'draft'
END
WHERE id = ?
RETURNING failed_attempts, status",
)
.bind(&now)
.bind(MAX_FAILED_ATTEMPTS)
.bind(post_id)
.fetch_one(pool)
.await
.map_err(|e| e.to_string())?;
if status == "failed" {
Ok(PublishFailureOutcome::GaveUp { failed_attempts })
} else {
Ok(PublishFailureOutcome::WillRetry { failed_attempts })
}
}On garde les mêmes sémantiques, mais :
- Il n’est plus nécessaire de suivre mentalement les états « avant » et « après » à travers deux appels DB.
- On réduit les allers‑retours vers la base et on simplifie la gestion d’erreurs.
Si vous adoptez ces deux changements, la logique centrale du module devient plus facile à suivre et la politique de retry a une définition unique et autoritative.
Original comment in English
issue (complexity): Consider simplifying the scheduler’s retry handling by moving backoff logic into Rust and using an UPDATE … RETURNING to collapse multi-step DB operations into single statements.
You can reduce complexity without changing behavior by:
- Moving the backoff window logic out of SQL and into Rust, using the existing
BACKOFF_MINUTES. - Using
UPDATE … RETURNINGinmark_publish_attempt_failedto avoid the two-step UPDATE+SELECT.
1. Simplify list_due_for_publish SQL and centralize backoff in Rust
Right now the retry policy is duplicated and encoded in a complex CASE + datetime expression. You already have BACKOFF_MINUTES; you can use it to keep a single source of truth and make the SQL easier to read.
Example refactor:
#[allow(dead_code)]
pub async fn list_due_for_publish(pool: &SqlitePool) -> Result<Vec<PostRecord>, String> {
let now = Utc::now();
let now_rfc3339 = now.to_rfc3339();
// Simpler SQL: let SQLite only enforce basic constraints.
let rows: Vec<sqlx::sqlite::SqliteRow> = sqlx::query(
"SELECT id, network, caption, hashtags, status, created_at, published_at,
scheduled_at, image_path, images, ig_media_id, account_id, published_url,
group_id, failed_attempts, last_attempt_at
FROM post_history
WHERE status = 'draft'
AND scheduled_at IS NOT NULL
AND scheduled_at <= ?
AND failed_attempts < ?
ORDER BY scheduled_at ASC",
)
.bind(&now_rfc3339)
.bind(MAX_FAILED_ATTEMPTS)
.fetch_all(pool)
.await
.map_err(|e| e.to_string())?;
// Apply retry window in Rust using BACKOFF_MINUTES as sole source of truth.
let due = rows
.into_iter()
.filter_map(|row| {
let mut post = row_to_post_record(&row)?;
// Convert RFC3339 fields to chrono types
let last_attempt_at = post
.last_attempt_at
.as_deref()
.and_then(|s| chrono::DateTime::parse_from_rfc3339(s).ok())
.map(|dt| dt.with_timezone(&Utc));
let failed_attempts = post.failed_attempts;
let backoff_minutes = BACKOFF_MINUTES
.get(failed_attempts as usize)
.copied()
.unwrap_or_else(|| *BACKOFF_MINUTES.last().unwrap());
let next_allowed = last_attempt_at
.map(|t| t + chrono::Duration::minutes(backoff_minutes))
// never tried → eligible immediately when scheduled_at <= now
.unwrap_or(now);
if next_allowed <= now {
Some(post)
} else {
None
}
})
.collect();
Ok(due)
}This keeps all behavior but:
- Removes the
CASE+datetimeexpression from SQL. - Uses
BACKOFF_MINUTESdirectly so your retry policy is defined in one place. - Makes the
backoff_minutes_table_matches_documented_policytest strictly about the Rust constants (and you can drop any tests that only exist to keep SQL and Rust in sync).
2. Use UPDATE … RETURNING in mark_publish_attempt_failed
You can avoid the second query and keep the logic in one place:
#[allow(dead_code)]
pub async fn mark_publish_attempt_failed(
pool: &SqlitePool,
post_id: i64,
) -> Result<PublishFailureOutcome, String> {
let now = Utc::now().to_rfc3339();
// Single statement: increment, set status, and read back.
let (failed_attempts, status): (i64, String) = sqlx::query_as(
"UPDATE post_history
SET failed_attempts = failed_attempts + 1,
last_attempt_at = ?,
status = CASE
WHEN failed_attempts + 1 >= ? THEN 'failed'
ELSE 'draft'
END
WHERE id = ?
RETURNING failed_attempts, status",
)
.bind(&now)
.bind(MAX_FAILED_ATTEMPTS)
.bind(post_id)
.fetch_one(pool)
.await
.map_err(|e| e.to_string())?;
if status == "failed" {
Ok(PublishFailureOutcome::GaveUp { failed_attempts })
} else {
Ok(PublishFailureOutcome::WillRetry { failed_attempts })
}
}This keeps the same semantics but:
- Removes the need to mentally track “before” and “after” states across two DB calls.
- Reduces DB round-trips and simplifies error handling.
If you adopt these two changes, the module’s core logic becomes easier to follow, and the retry policy has a single authoritative definition.
5 tasks
thierryvm
added a commit
that referenced
this pull request
May 11, 2026
…urcery on PR #69) (#70) Address 3 Sourcery comments on the v0.4.0 scheduler foundation (PR #69): 1. `mark_publish_attempt_failed` now requires `WHERE status = 'publishing'` so a misfire on a draft/failed/published row cannot corrupt the retry counter. Folded the post-UPDATE state read into the same statement via `UPDATE ... RETURNING` (1 round-trip instead of 2). Adds new `PublishFailureOutcome::Skipped` variant for the no-op case so the scheduler can log it without notifying. 2. `reset_retry_state` now requires `WHERE status IN ('draft', 'failed')` so an accidental call on a published row can't silently un-publish it, and a call while the scheduler holds the row in 'publishing' can't race the lock. Signature changes from `Result<(), String>` to `Result<bool, String>` so the caller can surface the no-op to the user. 3. Backoff window check moved out of the SQL CASE into Rust against BACKOFF_MINUTES — eliminates the silent-drift risk between the const table and the duplicated `WHEN 0 THEN 0 WHEN 1 THEN 5 ...` SQL literal. New `backoff_window_elapsed` helper keeps the filter logic isolated and unit-test-friendly. Three new regression tests lock the new contracts: - mark_failed_skips_when_status_is_not_publishing - reset_retry_state_rejects_published_row - reset_retry_state_rejects_publishing_row Existing tests still green; total: 13 scheduler tests, 240 lib tests. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
3 tasks
thierryvm
added a commit
that referenced
this pull request
May 12, 2026
Wires the data layer mergé in PR #69+#70 to a real tokio task. Polls SQLite every 60s for due drafts, atomically locks each row via try_lock_for_publish, dispatches through the existing publish_post / publish_linkedin_post commands, and surfaces a desktop notification when a post exhausts MAX_FAILED_ATTEMPTS. The scheduler stays a dumb pump: zero in-Rust retry timers, all backoff lives in db::scheduler::BACKOFF_MINUTES re-read every tick. That survives app restarts without losing track of pending posts. Architecture: - `tick` / `process_post` take traits (PublishDispatcher + Notifier) so the loop is testable without an AppHandle (real Tauri runtime is too heavy to stand up in #[tokio::test]). Production impls are thin shells: TauriDispatcher routes by network, TauriNotifier wraps tauri-plugin-notification. - Pre-check token expiry against accounts.token_expires_at (added in migration 014). Expired token short-circuits with a clear error rather than waiting for Meta/LinkedIn to 401 — the dispatch still counts as a failed attempt so the retry budget burns down even on doomed tokens. New deps: - tauri-plugin-notification = "2" + notification:default capability - async-trait = "0.1" for the dispatcher seam (already transitive) Tests: +9 in src/scheduler.rs (240 -> 249 lib tests). Cover the headline F2 E2E (overdue draft -> published after one tick), retry budget exhaustion -> notification, lock-contention skip, backoff window respect between failures, and the three token-expiry branches. Out of scope (deferred to follow-ups): - Scheduler UI badges in Composer/Dashboard (PR F3) - Sourcery #70 complexity follow-ups (task #94) — DateTime<Utc> parsing in PostRecord touches frontend serde, warrants a dedicated PR rather than smuggling it through F2. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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.
v0.4.0 stack — PR F1 / 3
First PR of the auto-publish scheduler per `docs/roadmap-v0.4.md`. Pure data layer + tests — no background task, no UI. Same foundation pattern as #56 (post_groups) used before the multi-network composer wired in.
Migration 019
`failed_attempts INTEGER NOT NULL DEFAULT 0` + `last_attempt_at TEXT` (nullable). Additive, retro-compat by construction.
db::scheduler module
Critical bug avoided (would have shipped without tests)
`scheduled_at <= datetime('now')` returned zero rows on first test pass. Root cause: SQLite's `datetime('now')` returns `'YYYY-MM-DD HH:MM:SS'` (space separator, no T, no Z) which sorts lexicographically BELOW `chrono::Utc::now().to_rfc3339()` writes (`'YYYY-MM-DDTHH:MM:SS.fffZ'`) — `T` (0x54) > space (0x20). Same wall-clock instant, different string comparison result.
Fix: bind `Utc::now().to_rfc3339()` as a parameter so producer (insert) + consumer (query) share the same string convention. Documented inline as a gotcha for future maintainers.
Tests
Rust 237/237 (was 226), clippy + fmt clean.
Next
PR F2 lands the background `tokio::spawn` that polls every 60 s, locks via `try_lock_for_publish`, calls existing `publish_post` / `publish_linkedin_post`, and routes failures through `mark_publish_attempt_failed` + native notifications. ETA ~1 PR.
🤖 Generated with Claude Code
Résumé par Sourcery
Introduction d’une prise en charge au niveau base de données pour un planificateur d’auto-publication, incluant des métadonnées de nouvelle tentative/backoff et des requêtes dédiées au planificateur, servant de base à la fonctionnalité de publication en arrière-plan de la v0.4.0.
Nouvelles fonctionnalités :
Corrections de bugs :
datetime('now')de SQLite.Améliorations :
Tests :
post_history.Original summary in English
Summary by Sourcery
Introduce database-level support for an auto-publish scheduler, including retry/backoff metadata and dedicated scheduler queries, as the foundation for the v0.4.0 background publishing feature.
New Features:
Bug Fixes:
Enhancements:
Tests: