Skip to content

Use server timestamp for api/submit reviews - #414

Open
helq wants to merge 1 commit into
sashiko-dev:mainfrom
helq:upstream/server-timestamp
Open

Use server timestamp for api/submit reviews#414
helq wants to merge 1 commit into
sashiko-dev:mainfrom
helq:upstream/server-timestamp

Conversation

@helq

@helq helq commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

On top of #411 . The only new commit is the last:

api: use server timestamp for API-injected mbox submissions

Add a submitted_at field to Event::RawMboxSubmitted, stamped with
the server's current time when a raw mbox is submitted via POST
/api/submit (Inject variant). This timestamp is propagated through
metadata.received_date and used as the patchset date instead of
the email's Date: header.

This prevents stale mbox timestamps (which can be arbitrarily far
in the past) from skewing queue ordering. The original email date
is preserved in the messages table for display purposes.

The Thread and Remote submission paths are unaffected as they
already use server-side timestamps via create_fetching_patchset().

@rgushchin

Copy link
Copy Markdown
Member

🤖 AI Code Review for PR #414 (upstream/server-timestamp)

Summary of Changes

This PR builds on top of #411 and adds server-side timestamping for raw mbox submissions (/api/submit), while incorporating the deduplication and received_parts tracking improvements from previous commits in the branch.


🔴 Critical Findings & Regression Risks

1. Incomplete Patchset Reviews due to Missing Physical Patch Rows (Commit 61a3127)

  • Location: src/db.rs in Database::create_patch() (lines ~2207–2248)
  • Description:
    When a commit SHA was previously ingested (e.g., as a singleton) and is subsequently submitted as part of a multi-patch range:
    if let Some(existing_ps) = existing_patchset_id
        && existing_ps != patchset_id
    {
        self.conn
            .execute(
                "UPDATE patchsets SET received_parts = MIN(COALESCE(received_parts, 0) + 1, total_parts) WHERE id = ?",
                libsql::params![patchset_id],
            )
            .await?;
        ...
        return Ok(row.get(0)?);
    }
    While this avoids stealing the patch from the old patchset, no row is inserted into patches for patchset_id.
  • Impact:
    • Omitted Diffs in AI Review: Reviewer::review_patchset_task calls get_patch_diffs(patchset_id), which queries SELECT ... FROM patches WHERE patchset_id = ?. The shared patch is completely missing from the retrieved diffs, causing the AI reviewer to review an incomplete series.
    • API/UI Mismatch: GET /api/patchset/:id (get_patchset_details) will report received_parts: 4, total_parts: 4, but the patches array will only contain 3 patches.
  • Suggested Solution:
    Because patches.message_id is defined as UNIQUE in schema.sql, either:
    1. Namespace the message_id for git imports / ranges (e.g. format!("{}#{}", patchset_id, sha) or format!("{}@{}", sha, root_msg_id)), so each patchset physically owns its own patch records.
    2. Or update the database schema constraint from message_id UNIQUE to UNIQUE(patchset_id, part_index).

2. Ineffective Duplicate Check in submit_patch (Commit b3a7ee1)

  • Location: src/api.rs (submit_patch) & src/db.rs (has_patchset_by_msgid)
  • Description:
    In submit_patch:
    let id = sha.clone();
    match state.db.has_patchset_by_msgid(&id).await {
        Ok(true) => { /* skip fetch */ }
    }
    state.db.create_fetching_patchset(&format!("{}@sashiko.local", id), ...).await;
    has_patchset_by_msgid queries:
    SELECT 1 FROM patchsets WHERE cover_letter_message_id = ? OR cover_letter_message_id = ?
    passing [sha, <sha>].
    However, remote fetch patchsets store cover_letter_message_id = format!("{}@sashiko.local", sha).
  • Impact:
    has_patchset_by_msgid always returns false for remote fetch submissions, so the deduplication logic never triggers.
  • Suggested Fix:
    Update has_patchset_by_msgid to also check format!("{}@sashiko.local", msgid):
    pub async fn has_patchset_by_msgid(&self, msgid: &str) -> Result<bool> {
        let synthetic = format!("{}@sashiko.local", msgid);
        let mut rows = self
            .conn
            .query(
                "SELECT 1 FROM patchsets WHERE cover_letter_message_id IN (?, ?, ?)",
                libsql::params![msgid, format!("<{}>", msgid), synthetic],
            )
            .await?;
        Ok(rows.next().await.ok().flatten().is_some())
    }

🟡 Minor / Observation Items

3. Global Side-Effect on NNTP Timestamps (Commit 020910a)

  • Location: src/main.rs (process_parsed_article, line ~1992)
  • Observation:
    Changing metadata.date to metadata.received_date.unwrap_or(metadata.date) in process_parsed_article applies globally. For standard email/NNTP messages, received_date is populated by extract_received_date(raw_email) from the Received: header.
    This subtly shifts the patchset timestamp for standard mailing list patches from the email's RFC 2822 Date: header to the MTA relay timestamp.

4. Concurrency Loop Blocking in Reviewer (Commit 551dbf5)

  • Location: src/reviewer.rs (process_pending_patchsets)
  • Observation:
    self.semaphore.acquire_owned().await? is called inside the synchronous for loop before spawning background tasks. If all worker permits are saturated, the loop blocks on the next item, leaving subsequent pending patchsets in the Pending state until a permit frees up.

🔒 Security Analysis

  • SQL Parameterization: All database interactions in src/db.rs use parameterized queries (libsql::params!). No dynamic SQL concatenation found.
  • Resource Limits: Decompression in fetch_and_inject_thread properly bounds both download size (MAX_MBOX_DOWNLOAD) and decompressed size (MAX_MBOX_DECOMPRESSED) to 50 MiB.
  • Input Sanitization: Synthetic IDs and git parameters are safely scoped and validated.

Add a submitted_at field to Event::RawMboxSubmitted, stamped with
the server's current time when a raw mbox is submitted via POST
/api/submit (Inject variant). This timestamp is propagated through
metadata.received_date and used as the patchset date instead of
the email's Date: header.

This prevents stale mbox timestamps (which can be arbitrarily far
in the past) from skewing queue ordering. The original email date
is preserved in the messages table for display purposes.

The Thread and Remote submission paths are unaffected as they
already use server-side timestamps via create_fetching_patchset().

Signed-off-by: Elkin Cruz <elkin@google.com>
@helq
helq force-pushed the upstream/server-timestamp branch from 020910a to 5951281 Compare August 21, 2026 18:41
@helq

helq commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Thank you for your comments I have revised the issues and cleaned up the commits. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants