Renew session tokens eagerly (on-start heal + 24h cadence) so server-minted claims propagate

[dubadub]

Why

Part of the server-side sync-enforcement rollout: the sync server now mints a sync_until (unix ts) claim on every renewed JWT when the account has the sync entitlement. Tokens minted before that rollout — and, unavoidably, any token in a session file loaded at cook server boot — carry no such claim at all. CookCLI's session tokens live 100 days, and the renewal threshold was "renew only when < 2h of life remains," so existing sessions would have gone ~100 days before ever hitting the server again and picking up the claim.

This brings CookCLI's renewal cadence in line with the other Cooklang clients: sync-agent renews when near expiry OR ≥24h since last refresh; the editor renews on start + every 24h.

What changed

• src/sync/session.rs

  • JwtClaims gains sync_until: Option<i64> (#[serde(default)]) — presence, not value, is what matters client-side.
  • New jwt_expires_in(jwt) -> Result<i64> and jwt_lacks_sync_until_claim(jwt) -> Result<bool>, both built on the existing unverified-decode helper.
  • Removed the now-superseded should_refresh_jwt (folded into the above + the new decision function).
  • pub(crate) tests_support::make_test_jwt — a small cross-module test helper so runner's tests can build a well-formed JWT without duplicating the encoding logic.
  • Unit tests decode hand-built JWTs (base64url, alg: none) with the claim present, absent, and explicitly null.

• src/sync/runner.rs

  • Renew decision extracted into a pure function:
    should_renew(expires_in_secs, since_last_renew_secs, claim_missing_and_not_yet_healed) -> bool
    • renews when expires_in_secs < 7200 (2h) — unchanged
    • renews when since_last_renew_secs >= 86400 (24h) — new cadence
    • renews unconditionally when claim_missing_and_not_yet_healed — the on-start heal
  • start_token_refresh now runs an immediate check at boot (heals a session missing sync_until without waiting for the first hourly tick), then continues on the existing hourly cadence. Both the boot check and the loop go through one shared check_and_maybe_renew so the decision logic isn't duplicated.
  • "Since last renew" is tracked in-memory (no session-file schema change); when this process hasn't renewed yet, it falls back to the session file's mtime, so a cook server restart doesn't reset the 24h clock to zero.
  • The missing-claim trigger fires at most once per process boot (tracked with a bool): a free-tier token lacks sync_until permanently, not just until healed, so re-checking it every tick would otherwise hit /api/sessions/renew every hour forever for free users.
  • Renewal failures are now split by cause (see "Review fix" below), instead of one anyhow error for every kind of failure.
  • Full truth table for should_renew covered by unit tests, plus tests for the mtime fallback helper.

No changes to login/logout flows or the session file format.

Review fix: 401 vs. transient renewal failures

An earlier version of this PR collapsed every /api/sessions/renew failure — a hard 401 as much as a network blip — into the same anyhow::Error, and kept the old token unconditionally on any of them. That meant a revoked/deleted account (or any other reason the server hands back a 401) would retry forever with sync silently dead and no way for the local UI to tell the user to re-login.

Fixed by typing the failure:

• refresh_token now returns Result<String, RenewError>, with RenewError split into:
  • AuthRejected — HTTP 401 specifically. The token itself was rejected; retrying with the same token can never succeed.
  • Transient(anyhow::Error) — everything else: network failure, timeout, non-401 non-2xx, unexpected body.
• classify_renew_failure(status) -> RenewError is a pure function doing just the status-code split (401 vs. everything else), independently unit-tested.
• apply_renew_outcome(...) (split out of check_and_maybe_renew for testability) now branches on the two cases:
  • AuthRejected → restores the pre-fix/original behavior for this case only: delete the session file, clear the in-memory session. This reuses the exact same "no session" pathway SyncSession::load already uses for a naturally-expired token, so the local UI's existing sync_logged_in == false → "Login to CookCloud" rendering picks it up automatically — no new status/reason plumbing needed.
  • Transient → unchanged from before: log and keep the old token for the next tick.
• New tests: classifies_401_as_auth_rejected, classifies_503_as_transient, classifies_other_4xx_as_transient_not_auth_rejected (a 404/500/etc is not grounds to log the user out — only 401 is), auth_rejected_clears_the_session_and_deletes_the_file, transient_failure_keeps_the_session_and_the_file, successful_renewal_replaces_the_session_and_records_the_time.

Verification

• cargo build --features sync — clean
• cargo test --workspace — all passing (105 in the main test binary, including 26 new/changed tests in sync::runner and sync::session)
• cargo clippy --workspace --all-targets --all-features -- -D warnings — clean
• cargo fmt --all -- --check — clean

Notes

• Point 3 of the original spec ("renewal failures keep current behavior... never delete the session on a failed renew") was actually a behavior change for non-401 failures, not a no-op: the prior code deleted the session and logged the user out on any failed renewal call. This PR keeps that fix for transient failures and, per review, reinstates deletion specifically for 401s (see above).
• Known pre-existing issue, not touched by this PR (flagging per review so it's on record): check_and_maybe_renew reads the session under a lock, awaits the network call, then re-acquires the lock to write the result. If a logout happens in that window, the in-flight renewal can resurrect the just-logged-out session (both the in-memory state and the file) once it completes. This read-lock → await → write-lock shape predates this PR (the original start_token_refresh had the same structure) — it's a candidate for a follow-up, not something this PR expands scope to fix.

Do not merge — opening for review.

[claude]

Review

Nicely scoped change with strong test coverage on the pure should_renew truth table and the new JWT-claim helpers — the extraction of check_and_maybe_renew so the boot-time heal and the hourly loop share one code path is a good simplification over the old duplicated match arms.

Potential issue: renewal failures are now all treated as transient

In check_and_maybe_renew (src/sync/runner.rs), every failure from refresh_token — including an HTTP 401 from /sessions/renew — now just logs and keeps the old token for the next tick:

Err(e) => {
    tracing::error!("Failed to refresh JWT: {e}");
}

refresh_token special-cases 401 into "Authentication expired" (runner.rs:308-310), which suggests the server is telling the client the session is genuinely revoked/invalid (password change, admin revoke, etc.), not a transient blip. Previously any renewal failure deleted the session; now no renewal failure does. That closes the "logged out on a transient network error" bug the PR is fixing, but it also removes the only path that ever cleared a permanently-dead session before its 100-day exp lapses — the client will keep retrying hourly against a token the server will never accept again, with no signal to the user that they need to re-authenticate.

Worth considering: keep the network/5xx case non-destructive (the actual fix here), but delete the session on a 401 specifically, e.g. by having refresh_token return a typed error (or a RefreshError::Unauthorized variant) that check_and_maybe_renew can match on. Flagging since the PR description calls out this behavior change explicitly but the write-up focuses on "don't log out on transient failure" — the permanent-failure case seems like an unintended side effect of the same fix rather than a deliberate choice.

Minor / non-blocking

• Heal attempt is consumed even on failure: claim_heal_attempted is set to true before knowing whether the renewal succeeds (runner.rs:213-216, documented intentionally as "one attempt, win or lose"). If that first attempt fails on a transient error at boot, a paid account stuck on a pre-rollout token won't get force-healed again this process — but it'll still pick up the claim within 24h via the cadence check, so the blast radius is small. Just worth double-checking this matches intent for accounts that need the claim sooner.
• file_age_secs silently returns None on clock skew (modified.elapsed() erroring if mtime is in the future) — falls back to "no cadence signal," which is a safe default, not a bug.
• No test exercises check_and_maybe_renew/refresh_token end-to-end (e.g. mocked 401 vs. network error), only the pure should_renew table. Given the finding above, a test asserting session-deletion-on-401 vs. session-preserved-on-network-error would pin down the intended behavior.

Nits

• jwt_lacks_sync_until_claim's doc comment is excellent at explaining the free-tier-vs-stale-token ambiguity — no notes there.
• Didn't independently run cargo test/clippy in this environment (blocked by sandboxing here); relying on the PR description's report that both are clean.

No security concerns beyond the above (JWT decoding remains intentionally unverified client-side, consistent with existing code; session file permissions unchanged at 0600).

[claude]

Review of src/sync/runner.rs and src/sync/session.rs

Nice PR overall — the extraction of should_renew and apply_renew_outcome into pure, unit-testable functions is a good pattern, and the truth-table test coverage for the renewal decision plus the 401-vs-transient split is thorough. A few notes:

Consistency nit: hand-rolled Display vs. existing thiserror convention

RenewError (runner.rs) implements std::fmt::Display by hand:

impl std::fmt::Display for RenewError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { ... }
}

sync/device_flow.rs already establishes the local convention for exactly this kind of typed network-error enum, using #[derive(thiserror::Error)] with #[error("...")] attributes (DeviceFlowError). thiserror is already a workspace dependency. Matching that pattern here would be more idiomatic and consistent within the same module family:

#[derive(Debug, thiserror::Error)]
enum RenewError {
    #[error("authentication rejected (401 Unauthorized)")]
    AuthRejected,
    #[error("{0}")]
    Transient(#[from] anyhow::Error),
}

(Also gives you impl std::error::Error for RenewError for free, which the current hand-rolled version doesn't provide.)

Pre-existing race is widened by this change (already disclosed, just flagging the scope increase)

The PR description already calls out the read-lock → await → write-lock race in check_and_maybe_renew/apply_renew_outcome: a logout during an in-flight renewal can resurrect the just-deleted session (both file and in-memory state) once the renewal completes. Agreed this is pre-existing and fine to defer, but worth noting explicitly: this PR increases how often a renewal is in-flight (immediate boot check + 24h cadence + hourly ticks, vs. only near-expiry before), so the window in which a logout can race a renewal gets meaningfully wider in practice. Might be worth prioritizing as an immediate follow-up rather than a someday-fix, especially since it's security-adjacent (a logged-out account's session silently coming back).

Minor: one-shot heal attempt is consumed even on a transient failure

if claim_missing_and_not_yet_healed {
    // One attempt, win or lose — see the comment above.
    *claim_heal_attempted = true;
}

If the very first heal attempt at boot hits a transient failure (network blip, 503, etc.), claim_heal_attempted is still set, so the immediate-heal fast path never fires again for the rest of the process. Looks intentional per the comment, and the normal 24h/2h cadence will still eventually pick up the claim on the next successful renewal, so this isn't a correctness bug — just worth double-checking it's the desired tradeoff vs., e.g., only marking it attempted on Ok or AuthRejected (not on Transient).

Test coverage

Genuinely strong — the should_renew truth table, the file_age_secs mtime fallback tests, and the apply_renew_outcome outcome-split tests (auth_rejected_clears_the_session_and_deletes_the_file, transient_failure_keeps_the_session_and_the_file, successful_renewal_replaces_the_session_and_records_the_time) all directly exercise the behavior this PR changes without needing a live HTTP round trip. No gaps I'd ask for.

Nothing else jumped out

No security concerns beyond the pre-existing unverified-JWT-decode pattern (already correctly scoped/commented as intentional — token comes from the auth server over HTTPS). Session file permissions (0o600) are preserved. No performance concerns — this is a low-frequency background task.

This PR is marked "Do not merge — opening for review," so treating the above as early feedback rather than blockers.