Code Analysis Bug Report

CRITICAL — Logic Bugs That Produce Wrong Results

1. get_random_daily() includes instead of excludes

File: src/mission/mod.rs:88

if m.is_daily() && name_set.contains(m.get_name()) {
    Some(m.get_starting_mission(user_id))

The caller at mission.rs:176-178 builds an exclude list of missions the user already has, then calls get_random_daily(..., &exclude). But the filter keeps missions whose names are in the exclude set — the exact opposite of what's needed. Should be !name_set.contains(...).

Impact: Users get duplicate daily missions instead of new ones.

---

2. missions::insert() — missing mission_id binding

File: src/db/missions.rs:212-224

query(
    "INSERT INTO missions
     (user_id, mission_id, name, data)
     VALUES
     ($1, $2, $3, $4)",
)
.bind(mission.user_id)
.bind(&mission.name)
.bind(&mission.data.to_json())
.execute(db)
.await

SQL expects 4 parameters ($1, $2, $3, $4) for (user_id, mission_id, name, data), but only 3 .bind() calls exist:

• $1 <- user_id
• $2 <- name (should be mission_id)
• $3 <- data
• $4 <- nothing

This will panic at runtime with a parameter count mismatch.

Impact: Any code path that calls missions::insert() crashes.

---

3. rewards::count_batch() — query_scalar used for tuple

File: src/db/rewards/mod.rs:98

let counts: Vec<(Uuid, i64)> = query_scalar(
    "SELECT
      (source ->> 'details')::uuid as mission_id,
      COUNT(*) as count
    FROM rewards
    ...
    GROUP BY 1",
)

query_scalar extracts a single column per row. This query returns two columns. Should be query_as::<_, (Uuid, i64)>. Will fail at runtime.

Impact: Any code path that calls count_batch() crashes.

---

4. Push token update — delete-before-create race

File: src/http/api/protected/push_token.rs:64-108

The handler:

1. Deletes the old token from DB (line 64)
2. Deletes the old SNS endpoint (line 75)
3. Creates the new SNS endpoint (line 84)
4. Inserts the new token (line 103)

If step 3 or 4 fails, the old token is already gone. The user loses push notifications with no recovery path.

Fix: Reverse the order — create the new endpoint first, insert the new token, then delete the old one.

Impact: Users can permanently lose push notifications on transient AWS failures.

---

HIGH — Panics That Crash the Server

5. get_room() — bare .unwrap() on DB result

File: src/http/api/protected/mod.rs:419

// for testing
pub async fn get_room(
    ctx: State<ApiContext>,
    user_id: UserId,
) -> Result<Json<db::FullRoomData>, StatusCode> {
    let room = db::lessons::get_room(&ctx.db, user_id.0).await.unwrap();
    Ok(Json(room))
}

Marked "for testing" but exposed as a real endpoint. Any DB error or missing room data panics the handler.

Fix: Replace .unwrap() with .map_err(|e| { error!(...); StatusCode::BAD_REQUEST })?.

---

6. Username length validated before trimming

File: src/http/api/protected/user.rs:31-40

if username.len() > 15 || username.len() < 3 {
    return Ok(Json(types::UsernameChangeResponse::CharCountErr));
}

let username: String = username.trim().to_string();
let username: String = username
    .split(' ')
    .filter(|s| !s.is_empty())
    .collect::<Vec<_>>()
    .join(" ");
// no re-validation of length after trimming

Length check happens on line 31, then whitespace is trimmed and collapsed on lines 35-40. Input " a " passes the length check (len=7) but becomes "a" (len=1) after processing. No re-validation.

Fix: Move the length check to after the trim/collapse, or add a second check.

Impact: Single-character or empty usernames can be saved to the DB.

---

MEDIUM — Data Consistency / Edge Cases

7. daily_login progress percentage is inverted

File: src/mission/types/daily_login.rs:44-45

let time_to_next_day = Self::DAY_IN_SECONDS - now % Self::DAY_IN_SECONDS;
let progress_percent = 100.0 / Self::DAY_IN_SECONDS as f64 * time_to_next_day as f64;

This calculates time remaining until the next day, not time elapsed. Progress shows ~100% at the start of the day and ~0% at the end.

The expires_at on line 51 is also wrong — time_to_next_day + DAY_IN_SECONDS gives a duration, not an absolute timestamp. It should be now + time_to_next_day.

---

8. get_ranking_for_user — GROUP BY missing user_id

File: src/db/rankings.rs:286

GROUP BY u.username, u.flag

If two users in the same ranking group have identical username and flag, their scores get merged into one row. Should include u.user_id in the GROUP BY (and SELECT).

---

9. Analytics handler — errors discard transaction silently

File: src/http/api/protected/mod.rs:293-306

if let Err(_) =
    db::analytics::insert_session(transaction.as_mut(), user_id.0, &analytics.session).await
{
    warn!("Error inserting analytics session for user {}", user_id.0);
    return;
}

On error, the function returns without committing or rolling back. The Err(_) pattern discards the actual error, making debugging impossible. Should be Err(e) with e logged.

Note: sqlx will roll back on drop, so this isn't a leaked transaction — but the swallowed error details are still a problem.

---

10. category_unlock — potential division by zero

File: src/mission/types/category_unlock.rs:74

progress: (100.0 / levelup_score_relative as f64 * current_score_relative as f64)

If levelup_score_relative is 0 (current and previous level have the same score requirement), this divides by zero producing Infinity, which .clamp(0.0, 100.0) will not catch (Infinity > 100.0 is true, but NaN.clamp() returns NaN).

Currently safe because LEVEL_UP_POINTS has strictly increasing values, but fragile if the array is ever modified.

---

11. SQL syntax error in rewards::update() FIXED

Trailing comma removed (src/db/rewards/mod.rs:227).

---

12. Missing advisory lock in game_report FIXED

Advisory lock added at src/http/api/protected/game_report.rs:30-35.